[v2,1/2] remoteproc: qcom: Handle reserved-memory allocation issues

  If Linux fails to allocate the dynamic reserved memory specified in the
device tree, the size of the reserved_mem will be 0. Add a check for
this to avoid using an invalid reservation.

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
New patch in v2, I wasn't aware of this until Bjorn posted a similar
patch for rmtfs:
https://lore.kernel.org/linux-arm-msm/20230530233643.4044823-4-quic_bjorande@quicinc.com/
---
 drivers/remoteproc/qcom_q6v5_mss.c  | 2 +-
 drivers/remoteproc/qcom_q6v5_wcss.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
  

On 6/14/23 17:31, Stephan Gerhold wrote:
> If Linux fails to allocate the dynamic reserved memory specified in the
> device tree, the size of the reserved_mem will be 0. Add a check for
> this to avoid using an invalid reservation.
> 
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>

Other uses of of_reserved_mem_lookup() also have unchecked uses of rmem 
[1], or check different things [2].

Does it make sense to put this check in the function itself?

I can't think of any obvious scenarios where it makes sense to 
differentiate between rmem being NULL vs having a size of zero at the 
time where a driver is fetching it.

As Bjorn described in the rmtfs patch, the memory allocation is 
essentially ignored, wouldn't it be better to print an error and 
invalidate the rmem in [3]?

[1]: 
https://elixir.bootlin.com/linux/v6.4-rc6/source/drivers/net/ethernet/mediatek/mtk_wed.c#L818
[2]: 
https://elixir.bootlin.com/linux/v6.4-rc6/source/drivers/remoteproc/rcar_rproc.c#L71
[3]: 
https://elixir.bootlin.com/linux/v6.4-rc6/source/drivers/of/of_reserved_mem.c#L276

// Caleb (they/them)
> ---
> New patch in v2, I wasn't aware of this until Bjorn posted a similar
> patch for rmtfs:
> https://lore.kernel.org/linux-arm-msm/20230530233643.4044823-4-quic_bjorande@quicinc.com/
> ---
>   drivers/remoteproc/qcom_q6v5_mss.c  | 2 +-
>   drivers/remoteproc/qcom_q6v5_wcss.c | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> index 70bffc9f33f6..a35ab6e860f3 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> @@ -1932,7 +1932,7 @@ static int q6v5_alloc_memory_region(struct q6v5 *qproc)
>   		return 0;
>   
>   	rmem = of_reserved_mem_lookup(node);
> -	if (!rmem) {
> +	if (!rmem || !rmem->size) {
>   		dev_err(qproc->dev, "unable to resolve metadata region\n");
>   		return -EINVAL;
>   	}
> diff --git a/drivers/remoteproc/qcom_q6v5_wcss.c b/drivers/remoteproc/qcom_q6v5_wcss.c
> index b437044aa126..9edab9d60c21 100644
> --- a/drivers/remoteproc/qcom_q6v5_wcss.c
> +++ b/drivers/remoteproc/qcom_q6v5_wcss.c
> @@ -882,7 +882,7 @@ static int q6v5_alloc_memory_region(struct q6v5_wcss *wcss)
>   		rmem = of_reserved_mem_lookup(node);
>   	of_node_put(node);
>   
> -	if (!rmem) {
> +	if (!rmem || !rmem->size) {
>   		dev_err(dev, "unable to acquire memory-region\n");
>   		return -EINVAL;
>   	}
>
  

On Thu, Jun 15, 2023 at 11:44:06AM +0100, Caleb Connolly wrote:
> On 6/14/23 17:31, Stephan Gerhold wrote:
> > If Linux fails to allocate the dynamic reserved memory specified in the
> > device tree, the size of the reserved_mem will be 0. Add a check for
> > this to avoid using an invalid reservation.
> > 
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> 
> Other uses of of_reserved_mem_lookup() also have unchecked uses of rmem [1],
> or check different things [2].
> 
> Does it make sense to put this check in the function itself?
> 
> I can't think of any obvious scenarios where it makes sense to differentiate
> between rmem being NULL vs having a size of zero at the time where a driver
> is fetching it.
> 
> As Bjorn described in the rmtfs patch, the memory allocation is essentially
> ignored, wouldn't it be better to print an error and invalidate the rmem in
> [3]?
> 

"Invalidating" isn't that easy because the reserved_mem is currently
stored in a simple array. Removing an entry would require shifting all
following values. But I suppose it would be easy to add the rmem->size
!= 0 check in of_reserved_mem_lookup() so it doesn't have to be checked
on all usages.

Given that no one seems to check for this at the moment I'm inclined to
agree with you that it would be better to handle this directly in
of_reserved_mem. Bjorn, what do you think?

Thanks,
Stephan

> [1]: https://elixir.bootlin.com/linux/v6.4-rc6/source/drivers/net/ethernet/mediatek/mtk_wed.c#L818
> [2]: https://elixir.bootlin.com/linux/v6.4-rc6/source/drivers/remoteproc/rcar_rproc.c#L71
> [3]: https://elixir.bootlin.com/linux/v6.4-rc6/source/drivers/of/of_reserved_mem.c#L276
> 
> // Caleb (they/them)
> > ---
> > New patch in v2, I wasn't aware of this until Bjorn posted a similar
> > patch for rmtfs:
> > https://lore.kernel.org/linux-arm-msm/20230530233643.4044823-4-quic_bjorande@quicinc.com/
> > ---
> >   drivers/remoteproc/qcom_q6v5_mss.c  | 2 +-
> >   drivers/remoteproc/qcom_q6v5_wcss.c | 2 +-
> >   2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> > index 70bffc9f33f6..a35ab6e860f3 100644
> > --- a/drivers/remoteproc/qcom_q6v5_mss.c
> > +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> > @@ -1932,7 +1932,7 @@ static int q6v5_alloc_memory_region(struct q6v5 *qproc)
> >   		return 0;
> >   	rmem = of_reserved_mem_lookup(node);
> > -	if (!rmem) {
> > +	if (!rmem || !rmem->size) {
> >   		dev_err(qproc->dev, "unable to resolve metadata region\n");
> >   		return -EINVAL;
> >   	}
> > diff --git a/drivers/remoteproc/qcom_q6v5_wcss.c b/drivers/remoteproc/qcom_q6v5_wcss.c
> > index b437044aa126..9edab9d60c21 100644
> > --- a/drivers/remoteproc/qcom_q6v5_wcss.c
> > +++ b/drivers/remoteproc/qcom_q6v5_wcss.c
> > @@ -882,7 +882,7 @@ static int q6v5_alloc_memory_region(struct q6v5_wcss *wcss)
> >   		rmem = of_reserved_mem_lookup(node);
> >   	of_node_put(node);
> > -	if (!rmem) {
> > +	if (!rmem || !rmem->size) {
> >   		dev_err(dev, "unable to acquire memory-region\n");
> >   		return -EINVAL;
> >   	}
> >
  

On Thu, Jun 15, 2023 at 04:51:44PM +0200, Stephan Gerhold wrote:
> On Thu, Jun 15, 2023 at 11:44:06AM +0100, Caleb Connolly wrote:
> > On 6/14/23 17:31, Stephan Gerhold wrote:
> > > If Linux fails to allocate the dynamic reserved memory specified in the
> > > device tree, the size of the reserved_mem will be 0. Add a check for
> > > this to avoid using an invalid reservation.
> > > 
> > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > 
> > Other uses of of_reserved_mem_lookup() also have unchecked uses of rmem [1],
> > or check different things [2].
> > 
> > Does it make sense to put this check in the function itself?
> > 
> > I can't think of any obvious scenarios where it makes sense to differentiate
> > between rmem being NULL vs having a size of zero at the time where a driver
> > is fetching it.
> > 
> > As Bjorn described in the rmtfs patch, the memory allocation is essentially
> > ignored, wouldn't it be better to print an error and invalidate the rmem in
> > [3]?
> > 
> 
> "Invalidating" isn't that easy because the reserved_mem is currently
> stored in a simple array. Removing an entry would require shifting all
> following values. But I suppose it would be easy to add the rmem->size
> != 0 check in of_reserved_mem_lookup() so it doesn't have to be checked
> on all usages.
> 
> Given that no one seems to check for this at the moment I'm inclined to
> agree with you that it would be better to handle this directly in
> of_reserved_mem. Bjorn, what do you think?
> 

I sent a v3 with the additional checks reverted. I'll work on a separate
patch series to improve this independently of this one for all users
(directly in of_reserved_mem).

Thanks,
Stephan