[PATCH] dmaengine: Simplify error handling path in '__dma_async_device_channel

dmaengine.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH] dmaengine: Simplify error handling path in '__dma_async_device_channel_register()'
@ 2020-02-26  9:07 Christophe JAILLET
  2020-02-26 10:01 ` Dan Carpenter
  0 siblings, 1 reply; 3+ messages in thread
From: Christophe JAILLET @ 2020-02-26  9:07 UTC (permalink / raw)
  To: dan.j.williams, vkoul, dave.jiang
  Cc: dmaengine, linux-kernel, kernel-janitors, Christophe JAILLET

If 'chan->dev = kzalloc()' fails, there is no need to explicitly call
'free_percpu()'. It is already called in the error handling path.
So it can be removed.

While at it, add a 'chan->local = NULL;' in the error handling path after
the 'free_percpu()' call. It is maybe useless, but can not hurt.

Fixes: d2fb0a043838 ("dmaengine: break out channel registration")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Not sure a Fixes tag is required for just a clean-up. I added it if the
move of the 'chan->local = NULL;' makes a real sense.
---
 drivers/dma/dmaengine.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index c3b1283b6d31..6bb6e88c6019 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -978,11 +978,8 @@ static int __dma_async_device_channel_register(struct dma_device *device,
 	if (!chan->local)
 		goto err_out;
 	chan->dev = kzalloc(sizeof(*chan->dev), GFP_KERNEL);
-	if (!chan->dev) {
-		free_percpu(chan->local);
-		chan->local = NULL;
+	if (!chan->dev)
 		goto err_out;
-	}
 
 	/*
 	 * When the chan_id is a negative value, we are dynamically adding
@@ -1008,6 +1005,7 @@ static int __dma_async_device_channel_register(struct dma_device *device,
 
  err_out:
 	free_percpu(chan->local);
+	chan->local = NULL;
 	kfree(chan->dev);
 	if (atomic_dec_return(idr_ref) == 0)
 		kfree(idr_ref);
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] dmaengine: Simplify error handling path in '__dma_async_device_channel_register()'
  2020-02-26  9:07 [PATCH] dmaengine: Simplify error handling path in '__dma_async_device_channel_register()' Christophe JAILLET
@ 2020-02-26 10:01 ` Dan Carpenter
  2020-02-27 21:29   ` Christophe JAILLET
  0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2020-02-26 10:01 UTC (permalink / raw)
  To: Christophe JAILLET
  Cc: dan.j.williams, vkoul, dave.jiang, dmaengine, linux-kernel,
	kernel-janitors

On Wed, Feb 26, 2020 at 10:07:07AM +0100, Christophe JAILLET wrote:
> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> index c3b1283b6d31..6bb6e88c6019 100644
> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
> @@ -978,11 +978,8 @@ static int __dma_async_device_channel_register(struct dma_device *device,
>  	if (!chan->local)
>  		goto err_out;
>  	chan->dev = kzalloc(sizeof(*chan->dev), GFP_KERNEL);
> -	if (!chan->dev) {
> -		free_percpu(chan->local);
> -		chan->local = NULL;
> +	if (!chan->dev)
>  		goto err_out;
> -	}
>  
>  	/*
>  	 * When the chan_id is a negative value, we are dynamically adding
> @@ -1008,6 +1005,7 @@ static int __dma_async_device_channel_register(struct dma_device *device,
>  
>   err_out:

Rule of thumb:  If the error label is "err" or "out" it's probably
going to be buggy.  This code is free everything style error handling.
We hit an error so something were allocated and some were not.  It's
always complicated to undo things which we didn't do.

>  	free_percpu(chan->local);
> +	chan->local = NULL;
>  	kfree(chan->dev);
>  	if (atomic_dec_return(idr_ref) == 0)
>  		kfree(idr_ref);

The ref counting on "idr_ref" is also wrong.

   967  
   968          if (tchan->dev) {
   969                  idr_ref = tchan->dev->idr_ref;
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This is 1+ references.  We don't increment it.

   970          } else {
   971                  idr_ref = kmalloc(sizeof(*idr_ref), GFP_KERNEL);
   972                  if (!idr_ref)
   973                          return -ENOMEM;
   974                  atomic_set(idr_ref, 0);
                        ^^^^^^^^^^^^^^^^^^^^^^
This is 0 references (Wrong.  Everything starts with 1 reference).

   975          }
   976  
   977          chan->local = alloc_percpu(typeof(*chan->local));
   978          if (!chan->local)
   979                  goto err_out;
   980          chan->dev = kzalloc(sizeof(*chan->dev), GFP_KERNEL);
   981          if (!chan->dev) {
   982                  free_percpu(chan->local);
   983                  chan->local = NULL;
   984                  goto err_out;
   985          }
   986  
   987          /*
   988           * When the chan_id is a negative value, we are dynamically adding
   989           * the channel. Otherwise we are static enumerating.
   990           */
   991          chan->chan_id = chan_id < 0 ? chancnt : chan_id;
   992          chan->dev->device.class = &dma_devclass;
   993          chan->dev->device.parent = device->dev;
   994          chan->dev->chan = chan;
   995          chan->dev->idr_ref = idr_ref;
   996          chan->dev->dev_id = device->dev_id;
   997          atomic_inc(idr_ref);
                ^^^^^^^^^^^^^^^^^^^
Probably if device_register() fails we don't want to free idr_ref, it
should instead be handled by device_put().

   998          dev_set_name(&chan->dev->device, "dma%dchan%d",
   999                       device->dev_id, chan->chan_id);
  1000  
  1001          rc = device_register(&chan->dev->device);
  1002          if (rc)
  1003                  goto err_out;
  1004          chan->client_count = 0;
  1005          device->chancnt = chan->chan_id + 1;
  1006  
  1007          return 0;
  1008  
  1009   err_out:
  1010          free_percpu(chan->local);
  1011          kfree(chan->dev);
  1012          if (atomic_dec_return(idr_ref) == 0)
  1013                  kfree(idr_ref);

If alloc_percpu() fails this is decrementing something which was never
incremented.  That's the classic error of trying to undo things which we
didnt do.

Presumably here we only want to free if we allocated the "idr_ref"
ourselves.  atomic_dec_return() returns the new reference count after
the decrement so on most paths it's going to be -1 which is a leak and
on the other paths there is a chance that it's going to lead to a use
after free.  There is no situation where this will do the correct thing.

Probably the cleanest fix it to just move the idr_ref allocation after
the other allocations so that we always rely on device_register() to
handle the clean ups.

  1014          return rc;
  1015  }

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] dmaengine: Simplify error handling path in '__dma_async_device_channel_register()'
  2020-02-26 10:01 ` Dan Carpenter
@ 2020-02-27 21:29   ` Christophe JAILLET
  0 siblings, 0 replies; 3+ messages in thread
From: Christophe JAILLET @ 2020-02-27 21:29 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: dan.j.williams, vkoul, dave.jiang, dmaengine, linux-kernel,
	kernel-janitors

Hi,

Thanks Dan for the additional comments.
However, their are to much things that I won't be able to test my self.

So unfortunately, I won't feel comfortable enough for a V2 with all your 
suggestions.
Please, you, or anybody else, go ahead and propose the corresponding fixes.

CJ


Le 26/02/2020 à 11:01, Dan Carpenter a écrit :
> On Wed, Feb 26, 2020 at 10:07:07AM +0100, Christophe JAILLET wrote:
>> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
>> index c3b1283b6d31..6bb6e88c6019 100644
>> --- a/drivers/dma/dmaengine.c
>> +++ b/drivers/dma/dmaengine.c
>> @@ -978,11 +978,8 @@ static int __dma_async_device_channel_register(struct dma_device *device,
>>   	if (!chan->local)
>>   		goto err_out;
>>   	chan->dev = kzalloc(sizeof(*chan->dev), GFP_KERNEL);
>> -	if (!chan->dev) {
>> -		free_percpu(chan->local);
>> -		chan->local = NULL;
>> +	if (!chan->dev)
>>   		goto err_out;
>> -	}
>>   
>>   	/*
>>   	 * When the chan_id is a negative value, we are dynamically adding
>> @@ -1008,6 +1005,7 @@ static int __dma_async_device_channel_register(struct dma_device *device,
>>   
>>    err_out:
> Rule of thumb:  If the error label is "err" or "out" it's probably
> going to be buggy.  This code is free everything style error handling.
> We hit an error so something were allocated and some were not.  It's
> always complicated to undo things which we didn't do.
>
>>   	free_percpu(chan->local);
>> +	chan->local = NULL;
>>   	kfree(chan->dev);
>>   	if (atomic_dec_return(idr_ref) == 0)
>>   		kfree(idr_ref);
> The ref counting on "idr_ref" is also wrong.
>
>     967
>     968          if (tchan->dev) {
>     969                  idr_ref = tchan->dev->idr_ref;
>                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This is 1+ references.  We don't increment it.
>
>     970          } else {
>     971                  idr_ref = kmalloc(sizeof(*idr_ref), GFP_KERNEL);
>     972                  if (!idr_ref)
>     973                          return -ENOMEM;
>     974                  atomic_set(idr_ref, 0);
>                          ^^^^^^^^^^^^^^^^^^^^^^
> This is 0 references (Wrong.  Everything starts with 1 reference).
>
>     975          }
>     976
>     977          chan->local = alloc_percpu(typeof(*chan->local));
>     978          if (!chan->local)
>     979                  goto err_out;
>     980          chan->dev = kzalloc(sizeof(*chan->dev), GFP_KERNEL);
>     981          if (!chan->dev) {
>     982                  free_percpu(chan->local);
>     983                  chan->local = NULL;
>     984                  goto err_out;
>     985          }
>     986
>     987          /*
>     988           * When the chan_id is a negative value, we are dynamically adding
>     989           * the channel. Otherwise we are static enumerating.
>     990           */
>     991          chan->chan_id = chan_id < 0 ? chancnt : chan_id;
>     992          chan->dev->device.class = &dma_devclass;
>     993          chan->dev->device.parent = device->dev;
>     994          chan->dev->chan = chan;
>     995          chan->dev->idr_ref = idr_ref;
>     996          chan->dev->dev_id = device->dev_id;
>     997          atomic_inc(idr_ref);
>                  ^^^^^^^^^^^^^^^^^^^
> Probably if device_register() fails we don't want to free idr_ref, it
> should instead be handled by device_put().
>
>     998          dev_set_name(&chan->dev->device, "dma%dchan%d",
>     999                       device->dev_id, chan->chan_id);
>    1000
>    1001          rc = device_register(&chan->dev->device);
>    1002          if (rc)
>    1003                  goto err_out;
>    1004          chan->client_count = 0;
>    1005          device->chancnt = chan->chan_id + 1;
>    1006
>    1007          return 0;
>    1008
>    1009   err_out:
>    1010          free_percpu(chan->local);
>    1011          kfree(chan->dev);
>    1012          if (atomic_dec_return(idr_ref) == 0)
>    1013                  kfree(idr_ref);
>
> If alloc_percpu() fails this is decrementing something which was never
> incremented.  That's the classic error of trying to undo things which we
> didnt do.
>
> Presumably here we only want to free if we allocated the "idr_ref"
> ourselves.  atomic_dec_return() returns the new reference count after
> the decrement so on most paths it's going to be -1 which is a leak and
> on the other paths there is a chance that it's going to lead to a use
> after free.  There is no situation where this will do the correct thing.
>
> Probably the cleanest fix it to just move the idr_ref allocation after
> the other allocations so that we always rely on device_register() to
> handle the clean ups.
>
>    1014          return rc;
>    1015  }
>
> regards,
> dan carpenter
>
>


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2020-02-27 21:29 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-26  9:07 [PATCH] dmaengine: Simplify error handling path in '__dma_async_device_channel_register()' Christophe JAILLET
2020-02-26 10:01 ` Dan Carpenter
2020-02-27 21:29   ` Christophe JAILLET

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).