lkml.org 
[lkml]   [2020]   [Feb]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] dmaengine: Simplify error handling path in '__dma_async_device_channel_register()'
From
Date
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
>
>

\
 
 \ /
  Last update: 2020-02-27 22:30    [W:0.037 / U:1.472 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site