Re: [PATCH] dmaengine: Simplify error handling path in '__dma_async_device_channel_register()'

[Christophe JAILLET <christophe.jaillet@wanadoo.fr>]

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
>
>