In __pata_platform_probe(), devm_kzalloc() is assigned to ap->ops and
there is a dereference of it right after that, which could introduce a
NULL pointer dereference bug.
Fix this by adding a NULL check of ap->ops.
This bug was found by a static analyzer.
Builds with 'make allyesconfig' show no new warnings,
and our static analyzer no longer warns about this code.
Fixes: f3d5e4f18dba ("ata: pata_of_platform: Allow to use 16-bit wide data transfer")
Signed-off-by: Zhou Qingyang <[email protected]>
---
The analysis employs differential checking to identify inconsistent
security operations (e.g., checks or kfrees) between two code paths
and confirms that the inconsistent operations are not recovered in the
current function or the callers, so they constitute bugs.
Note that, as a bug found by static analysis, it can be a false
positive or hard to trigger. Multiple researchers have cross-reviewed
the bug.
drivers/ata/pata_platform.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
index 028329428b75..021ef9cbcbc1 100644
--- a/drivers/ata/pata_platform.c
+++ b/drivers/ata/pata_platform.c
@@ -128,6 +128,8 @@ int __pata_platform_probe(struct device *dev, struct resource *io_res,
ap = host->ports[0];
ap->ops = devm_kzalloc(dev, sizeof(*ap->ops), GFP_KERNEL);
+ if (ap->ops)
+ return -ENOMEM;
ap->ops->inherits = &ata_sff_port_ops;
ap->ops->cable_detect = ata_cable_unknown;
ap->ops->set_mode = pata_platform_set_mode;
--
2.25.1
On 1/24/22 7:45 PM, Zhou Qingyang wrote:
> In __pata_platform_probe(), devm_kzalloc() is assigned to ap->ops and
> there is a dereference of it right after that, which could introduce a
> NULL pointer dereference bug.
>
> Fix this by adding a NULL check of ap->ops.
>
> This bug was found by a static analyzer.
>
> Builds with 'make allyesconfig' show no new warnings,
> and our static analyzer no longer warns about this code.
>
> Fixes: f3d5e4f18dba ("ata: pata_of_platform: Allow to use 16-bit wide data transfer")
> Signed-off-by: Zhou Qingyang <[email protected]>
Reviewed-by: Sergey Shtylyov <[email protected]>
[...]
MBR, Sergey
On 1/25/22 01:45, Zhou Qingyang wrote:
> In __pata_platform_probe(), devm_kzalloc() is assigned to ap->ops and
> there is a dereference of it right after that, which could introduce a
> NULL pointer dereference bug.
>
> Fix this by adding a NULL check of ap->ops.
>
> This bug was found by a static analyzer.
>
> Builds with 'make allyesconfig' show no new warnings,
> and our static analyzer no longer warns about this code.
>
> Fixes: f3d5e4f18dba ("ata: pata_of_platform: Allow to use 16-bit wide data transfer")
> Signed-off-by: Zhou Qingyang <[email protected]>
> ---
> The analysis employs differential checking to identify inconsistent
> security operations (e.g., checks or kfrees) between two code paths
> and confirms that the inconsistent operations are not recovered in the
> current function or the callers, so they constitute bugs.
>
> Note that, as a bug found by static analysis, it can be a false
> positive or hard to trigger. Multiple researchers have cross-reviewed
> the bug.
See below. Reviewing the fix would be good too :)
>
> drivers/ata/pata_platform.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
> index 028329428b75..021ef9cbcbc1 100644
> --- a/drivers/ata/pata_platform.c
> +++ b/drivers/ata/pata_platform.c
> @@ -128,6 +128,8 @@ int __pata_platform_probe(struct device *dev, struct resource *io_res,
> ap = host->ports[0];
>
> ap->ops = devm_kzalloc(dev, sizeof(*ap->ops), GFP_KERNEL);
> + if (ap->ops)
This of course should be "if (!ap->ops)", obviously...
I fixed that when applying.
> + return -ENOMEM;
> ap->ops->inherits = &ata_sff_port_ops;
> ap->ops->cable_detect = ata_cable_unknown;
> ap->ops->set_mode = pata_platform_set_mode;
--
Damien Le Moal
Western Digital Research
On 1/25/22 18:46, Sergey Shtylyov wrote:
> On 1/24/22 7:45 PM, Zhou Qingyang wrote:
>
>> In __pata_platform_probe(), devm_kzalloc() is assigned to ap->ops and
>> there is a dereference of it right after that, which could introduce a
>> NULL pointer dereference bug.
>>
>> Fix this by adding a NULL check of ap->ops.
>>
>> This bug was found by a static analyzer.
>>
>> Builds with 'make allyesconfig' show no new warnings,
>> and our static analyzer no longer warns about this code.
>>
>> Fixes: f3d5e4f18dba ("ata: pata_of_platform: Allow to use 16-bit wide data transfer")
>> Signed-off-by: Zhou Qingyang <[email protected]>
>
> Reviewed-by: Sergey Shtylyov <[email protected]>
Please see my note about the typo in the fix. I kept you RB tag. Let me
know if that is OK.
>
> [...]
>
> MBR, Sergey
--
Damien Le Moal
Western Digital Research
Hello!
On 1/27/22 5:15 AM, Damien Le Moal wrote:
>>> In __pata_platform_probe(), devm_kzalloc() is assigned to ap->ops and
>>> there is a dereference of it right after that, which could introduce a
>>> NULL pointer dereference bug.
>>>
>>> Fix this by adding a NULL check of ap->ops.
>>>
>>> This bug was found by a static analyzer.
>>>
>>> Builds with 'make allyesconfig' show no new warnings,
>>> and our static analyzer no longer warns about this code.
>>>
>>> Fixes: f3d5e4f18dba ("ata: pata_of_platform: Allow to use 16-bit wide data transfer")
>>> Signed-off-by: Zhou Qingyang <[email protected]>
>>
>> Reviewed-by: Sergey Shtylyov <[email protected]>
>
> Please see my note about the typo in the fix. I kept you RB tag. Let me
Oops, didn't expect such a stupid bug, so my brain did add the missing ! prolly... :-)
Not clear how he managed to shut up his static analyzer with this patch...
> know if that is OK.
Of course, sorry for the messy review! :-)
>> [...]
MBR, Sergey
On Tue, Jan 25, 2022 at 12:45:25AM +0800, Zhou Qingyang wrote:
> In __pata_platform_probe(), devm_kzalloc() is assigned to ap->ops and
> there is a dereference of it right after that, which could introduce a
> NULL pointer dereference bug.
>
> Fix this by adding a NULL check of ap->ops.
>
> This bug was found by a static analyzer.
>
> Builds with 'make allyesconfig' show no new warnings,
> and our static analyzer no longer warns about this code.
>
> Fixes: f3d5e4f18dba ("ata: pata_of_platform: Allow to use 16-bit wide data transfer")
> Signed-off-by: Zhou Qingyang <[email protected]>
> ---
As stated in the past, please do not make contributions to the Linux
kernel until umn.edu has properly resolved its development issues.
> The analysis employs differential checking to identify inconsistent
> security operations (e.g., checks or kfrees) between two code paths
> and confirms that the inconsistent operations are not recovered in the
> current function or the callers, so they constitute bugs.
>
> Note that, as a bug found by static analysis, it can be a false
> positive or hard to trigger. Multiple researchers have cross-reviewed
> the bug.
>
> drivers/ata/pata_platform.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
> index 028329428b75..021ef9cbcbc1 100644
> --- a/drivers/ata/pata_platform.c
> +++ b/drivers/ata/pata_platform.c
> @@ -128,6 +128,8 @@ int __pata_platform_probe(struct device *dev, struct resource *io_res,
> ap = host->ports[0];
>
> ap->ops = devm_kzalloc(dev, sizeof(*ap->ops), GFP_KERNEL);
> + if (ap->ops)
> + return -ENOMEM;
This change seems to leak memory. Damien, please revert it.
thanks,
greg k-h
On 1/28/22 19:11, Greg KH wrote:
> On Tue, Jan 25, 2022 at 12:45:25AM +0800, Zhou Qingyang wrote:
>> In __pata_platform_probe(), devm_kzalloc() is assigned to ap->ops and
>> there is a dereference of it right after that, which could introduce a
>> NULL pointer dereference bug.
>>
>> Fix this by adding a NULL check of ap->ops.
>>
>> This bug was found by a static analyzer.
>>
>> Builds with 'make allyesconfig' show no new warnings,
>> and our static analyzer no longer warns about this code.
>>
>> Fixes: f3d5e4f18dba ("ata: pata_of_platform: Allow to use 16-bit wide data transfer")
>> Signed-off-by: Zhou Qingyang <[email protected]>
>> ---
>
> As stated in the past, please do not make contributions to the Linux
> kernel until umn.edu has properly resolved its development issues.
Aouch. My apologies. I forgot about this. Thank you for the reminder.
>
>> The analysis employs differential checking to identify inconsistent
>> security operations (e.g., checks or kfrees) between two code paths
>> and confirms that the inconsistent operations are not recovered in the
>> current function or the callers, so they constitute bugs.
>>
>> Note that, as a bug found by static analysis, it can be a false
>> positive or hard to trigger. Multiple researchers have cross-reviewed
>> the bug.
>>
>> drivers/ata/pata_platform.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
>> index 028329428b75..021ef9cbcbc1 100644
>> --- a/drivers/ata/pata_platform.c
>> +++ b/drivers/ata/pata_platform.c
>> @@ -128,6 +128,8 @@ int __pata_platform_probe(struct device *dev, struct resource *io_res,
>> ap = host->ports[0];
>>
>> ap->ops = devm_kzalloc(dev, sizeof(*ap->ops), GFP_KERNEL);
>> + if (ap->ops)
>> + return -ENOMEM;
>
> This change seems to leak memory. Damien, please revert it.
I fixed the patch when applying, so there is no leak. This is a genuine
(potential) bug fix. Must I revert ? Is the "no contribution from
umn.edu" an unbreakable rule ?
>
> thanks,
>
> greg k-h
--
Damien Le Moal
Western Digital Research
On Fri, Jan 28, 2022 at 08:50:04PM +0900, Damien Le Moal wrote:
> On 1/28/22 19:11, Greg KH wrote:
> > On Tue, Jan 25, 2022 at 12:45:25AM +0800, Zhou Qingyang wrote:
> >> In __pata_platform_probe(), devm_kzalloc() is assigned to ap->ops and
> >> there is a dereference of it right after that, which could introduce a
> >> NULL pointer dereference bug.
> >>
> >> Fix this by adding a NULL check of ap->ops.
> >>
> >> This bug was found by a static analyzer.
> >>
> >> Builds with 'make allyesconfig' show no new warnings,
> >> and our static analyzer no longer warns about this code.
> >>
> >> Fixes: f3d5e4f18dba ("ata: pata_of_platform: Allow to use 16-bit wide data transfer")
> >> Signed-off-by: Zhou Qingyang <[email protected]>
> >> ---
> >
> > As stated in the past, please do not make contributions to the Linux
> > kernel until umn.edu has properly resolved its development issues.
>
> Aouch. My apologies. I forgot about this. Thank you for the reminder.
>
> >
> >> The analysis employs differential checking to identify inconsistent
> >> security operations (e.g., checks or kfrees) between two code paths
> >> and confirms that the inconsistent operations are not recovered in the
> >> current function or the callers, so they constitute bugs.
> >>
> >> Note that, as a bug found by static analysis, it can be a false
> >> positive or hard to trigger. Multiple researchers have cross-reviewed
> >> the bug.
> >>
> >> drivers/ata/pata_platform.c | 2 ++
> >> 1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
> >> index 028329428b75..021ef9cbcbc1 100644
> >> --- a/drivers/ata/pata_platform.c
> >> +++ b/drivers/ata/pata_platform.c
> >> @@ -128,6 +128,8 @@ int __pata_platform_probe(struct device *dev, struct resource *io_res,
> >> ap = host->ports[0];
> >>
> >> ap->ops = devm_kzalloc(dev, sizeof(*ap->ops), GFP_KERNEL);
> >> + if (ap->ops)
> >> + return -ENOMEM;
> >
> > This change seems to leak memory. Damien, please revert it.
>
> I fixed the patch when applying, so there is no leak.
Really? What happened to the memory that ata_host_alloc() created above
this call? How is that freed?
> This is a genuine (potential) bug fix.
As I tell others, how can kmalloc() ever fail here, so odd of this being
a real bugfix are so low it's not funny. So take these types of
cleanups as a last-resort only after you have strongly validated that
they are correct. The current group of people trying to do these fixes
have a horrible track-record and are getting things wrong way more than
they should be. And so it is worse having code that "looks" correct vs.
something that is "obviously we need to handle this some day".
> Must I revert ?
If it's buggy you should, see my above question about ata_host_alloc(),
is there a cleanup path somewhere that I am missing?
thanks,
greg k-h
On 1/29/22 00:57, Greg KH wrote:
> On Fri, Jan 28, 2022 at 08:50:04PM +0900, Damien Le Moal wrote:
>> On 1/28/22 19:11, Greg KH wrote:
>>> On Tue, Jan 25, 2022 at 12:45:25AM +0800, Zhou Qingyang wrote:
>>>> In __pata_platform_probe(), devm_kzalloc() is assigned to ap->ops and
>>>> there is a dereference of it right after that, which could introduce a
>>>> NULL pointer dereference bug.
>>>>
>>>> Fix this by adding a NULL check of ap->ops.
>>>>
>>>> This bug was found by a static analyzer.
>>>>
>>>> Builds with 'make allyesconfig' show no new warnings,
>>>> and our static analyzer no longer warns about this code.
>>>>
>>>> Fixes: f3d5e4f18dba ("ata: pata_of_platform: Allow to use 16-bit wide data transfer")
>>>> Signed-off-by: Zhou Qingyang <[email protected]>
>>>> ---
>>>
>>> As stated in the past, please do not make contributions to the Linux
>>> kernel until umn.edu has properly resolved its development issues.
>>
>> Aouch. My apologies. I forgot about this. Thank you for the reminder.
>>
>>>
>>>> The analysis employs differential checking to identify inconsistent
>>>> security operations (e.g., checks or kfrees) between two code paths
>>>> and confirms that the inconsistent operations are not recovered in the
>>>> current function or the callers, so they constitute bugs.
>>>>
>>>> Note that, as a bug found by static analysis, it can be a false
>>>> positive or hard to trigger. Multiple researchers have cross-reviewed
>>>> the bug.
>>>>
>>>> drivers/ata/pata_platform.c | 2 ++
>>>> 1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
>>>> index 028329428b75..021ef9cbcbc1 100644
>>>> --- a/drivers/ata/pata_platform.c
>>>> +++ b/drivers/ata/pata_platform.c
>>>> @@ -128,6 +128,8 @@ int __pata_platform_probe(struct device *dev, struct resource *io_res,
>>>> ap = host->ports[0];
>>>>
>>>> ap->ops = devm_kzalloc(dev, sizeof(*ap->ops), GFP_KERNEL);
>>>> + if (ap->ops)
>>>> + return -ENOMEM;
>>>
>>> This change seems to leak memory. Damien, please revert it.
>>
>> I fixed the patch when applying, so there is no leak.
>
> Really? What happened to the memory that ata_host_alloc() created above
> this call? How is that freed?
>
>> This is a genuine (potential) bug fix.
>
> As I tell others, how can kmalloc() ever fail here, so odd of this being
> a real bugfix are so low it's not funny. So take these types of
> cleanups as a last-resort only after you have strongly validated that
> they are correct. The current group of people trying to do these fixes
> have a horrible track-record and are getting things wrong way more than
> they should be. And so it is worse having code that "looks" correct vs.
> something that is "obviously we need to handle this some day".
I completely agree that this is not fixing any real bug reported in the
field. And as you say, an error here is more than unlikely. I accepted
the patch only on the ground of code correctness.
>
>> Must I revert ?
>
> If it's buggy you should, see my above question about ata_host_alloc(),
> is there a cleanup path somewhere that I am missing?
The resources allocated by ata_host_alloc() are attached to the device
(devres and drv_data) so they will be freed by ata_devres_release() when
the dev is dropped due to the probe error. I think the return that the
patch introduces is fine as is.
If I am misunderstanding the devres handling, please let me know.
In any case, I will make sure to ignore patches from umn.edu for now.
Thanks.
--
Damien Le Moal
Western Digital Research
On Sat, Jan 29, 2022 at 09:12:19AM +0900, Damien Le Moal wrote:
> On 1/29/22 00:57, Greg KH wrote:
> > On Fri, Jan 28, 2022 at 08:50:04PM +0900, Damien Le Moal wrote:
> >> On 1/28/22 19:11, Greg KH wrote:
> >>> On Tue, Jan 25, 2022 at 12:45:25AM +0800, Zhou Qingyang wrote:
> >>>> In __pata_platform_probe(), devm_kzalloc() is assigned to ap->ops and
> >>>> there is a dereference of it right after that, which could introduce a
> >>>> NULL pointer dereference bug.
> >>>>
> >>>> Fix this by adding a NULL check of ap->ops.
> >>>>
> >>>> This bug was found by a static analyzer.
> >>>>
> >>>> Builds with 'make allyesconfig' show no new warnings,
> >>>> and our static analyzer no longer warns about this code.
> >>>>
> >>>> Fixes: f3d5e4f18dba ("ata: pata_of_platform: Allow to use 16-bit wide data transfer")
> >>>> Signed-off-by: Zhou Qingyang <[email protected]>
> >>>> ---
> >>>
> >>> As stated in the past, please do not make contributions to the Linux
> >>> kernel until umn.edu has properly resolved its development issues.
> >>
> >> Aouch. My apologies. I forgot about this. Thank you for the reminder.
> >>
> >>>
> >>>> The analysis employs differential checking to identify inconsistent
> >>>> security operations (e.g., checks or kfrees) between two code paths
> >>>> and confirms that the inconsistent operations are not recovered in the
> >>>> current function or the callers, so they constitute bugs.
> >>>>
> >>>> Note that, as a bug found by static analysis, it can be a false
> >>>> positive or hard to trigger. Multiple researchers have cross-reviewed
> >>>> the bug.
> >>>>
> >>>> drivers/ata/pata_platform.c | 2 ++
> >>>> 1 file changed, 2 insertions(+)
> >>>>
> >>>> diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
> >>>> index 028329428b75..021ef9cbcbc1 100644
> >>>> --- a/drivers/ata/pata_platform.c
> >>>> +++ b/drivers/ata/pata_platform.c
> >>>> @@ -128,6 +128,8 @@ int __pata_platform_probe(struct device *dev, struct resource *io_res,
> >>>> ap = host->ports[0];
> >>>>
> >>>> ap->ops = devm_kzalloc(dev, sizeof(*ap->ops), GFP_KERNEL);
> >>>> + if (ap->ops)
> >>>> + return -ENOMEM;
> >>>
> >>> This change seems to leak memory. Damien, please revert it.
> >>
> >> I fixed the patch when applying, so there is no leak.
> >
> > Really? What happened to the memory that ata_host_alloc() created above
> > this call? How is that freed?
> >
> >> This is a genuine (potential) bug fix.
> >
> > As I tell others, how can kmalloc() ever fail here, so odd of this being
> > a real bugfix are so low it's not funny. So take these types of
> > cleanups as a last-resort only after you have strongly validated that
> > they are correct. The current group of people trying to do these fixes
> > have a horrible track-record and are getting things wrong way more than
> > they should be. And so it is worse having code that "looks" correct vs.
> > something that is "obviously we need to handle this some day".
>
> I completely agree that this is not fixing any real bug reported in the
> field. And as you say, an error here is more than unlikely. I accepted
> the patch only on the ground of code correctness.
>
> >
> >> Must I revert ?
> >
> > If it's buggy you should, see my above question about ata_host_alloc(),
> > is there a cleanup path somewhere that I am missing?
>
> The resources allocated by ata_host_alloc() are attached to the device
> (devres and drv_data) so they will be freed by ata_devres_release() when
> the dev is dropped due to the probe error. I think the return that the
> patch introduces is fine as is.
>
> If I am misunderstanding the devres handling, please let me know.
Where does the data allocated in ata_host_alloc() on this line:
host = kzalloc(sz, GFP_KERNEL);
Get attached to a devres structure?
It's a kref-managed data structure (see the call to kref_init() a few
lines down), and the memory will be freed if you release the last
reference on the kref, but that has nothing to do with devres.
There's also the ports memory attached to the host structure as well,
that is controlled by the lifetime of the kref, not a devres reference
that I can see.
Or am I missing some link somewhere here?
thanks,
greg k-h
On 2022/01/29 16:06, Greg KH wrote:
> On Sat, Jan 29, 2022 at 09:12:19AM +0900, Damien Le Moal wrote:
>> On 1/29/22 00:57, Greg KH wrote:
>>> On Fri, Jan 28, 2022 at 08:50:04PM +0900, Damien Le Moal wrote:
>>>> On 1/28/22 19:11, Greg KH wrote:
>>>>> On Tue, Jan 25, 2022 at 12:45:25AM +0800, Zhou Qingyang wrote:
>>>>>> In __pata_platform_probe(), devm_kzalloc() is assigned to ap->ops and
>>>>>> there is a dereference of it right after that, which could introduce a
>>>>>> NULL pointer dereference bug.
>>>>>>
>>>>>> Fix this by adding a NULL check of ap->ops.
>>>>>>
>>>>>> This bug was found by a static analyzer.
>>>>>>
>>>>>> Builds with 'make allyesconfig' show no new warnings,
>>>>>> and our static analyzer no longer warns about this code.
>>>>>>
>>>>>> Fixes: f3d5e4f18dba ("ata: pata_of_platform: Allow to use 16-bit wide data transfer")
>>>>>> Signed-off-by: Zhou Qingyang <[email protected]>
>>>>>> ---
>>>>>
>>>>> As stated in the past, please do not make contributions to the Linux
>>>>> kernel until umn.edu has properly resolved its development issues.
>>>>
>>>> Aouch. My apologies. I forgot about this. Thank you for the reminder.
>>>>
>>>>>
>>>>>> The analysis employs differential checking to identify inconsistent
>>>>>> security operations (e.g., checks or kfrees) between two code paths
>>>>>> and confirms that the inconsistent operations are not recovered in the
>>>>>> current function or the callers, so they constitute bugs.
>>>>>>
>>>>>> Note that, as a bug found by static analysis, it can be a false
>>>>>> positive or hard to trigger. Multiple researchers have cross-reviewed
>>>>>> the bug.
>>>>>>
>>>>>> drivers/ata/pata_platform.c | 2 ++
>>>>>> 1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
>>>>>> index 028329428b75..021ef9cbcbc1 100644
>>>>>> --- a/drivers/ata/pata_platform.c
>>>>>> +++ b/drivers/ata/pata_platform.c
>>>>>> @@ -128,6 +128,8 @@ int __pata_platform_probe(struct device *dev, struct resource *io_res,
>>>>>> ap = host->ports[0];
>>>>>>
>>>>>> ap->ops = devm_kzalloc(dev, sizeof(*ap->ops), GFP_KERNEL);
>>>>>> + if (ap->ops)
>>>>>> + return -ENOMEM;
>>>>>
>>>>> This change seems to leak memory. Damien, please revert it.
>>>>
>>>> I fixed the patch when applying, so there is no leak.
>>>
>>> Really? What happened to the memory that ata_host_alloc() created above
>>> this call? How is that freed?
>>>
>>>> This is a genuine (potential) bug fix.
>>>
>>> As I tell others, how can kmalloc() ever fail here, so odd of this being
>>> a real bugfix are so low it's not funny. So take these types of
>>> cleanups as a last-resort only after you have strongly validated that
>>> they are correct. The current group of people trying to do these fixes
>>> have a horrible track-record and are getting things wrong way more than
>>> they should be. And so it is worse having code that "looks" correct vs.
>>> something that is "obviously we need to handle this some day".
>>
>> I completely agree that this is not fixing any real bug reported in the
>> field. And as you say, an error here is more than unlikely. I accepted
>> the patch only on the ground of code correctness.
>>
>>>
>>>> Must I revert ?
>>>
>>> If it's buggy you should, see my above question about ata_host_alloc(),
>>> is there a cleanup path somewhere that I am missing?
>>
>> The resources allocated by ata_host_alloc() are attached to the device
>> (devres and drv_data) so they will be freed by ata_devres_release() when
>> the dev is dropped due to the probe error. I think the return that the
>> patch introduces is fine as is.
>>
>> If I am misunderstanding the devres handling, please let me know.
>
> Where does the data allocated in ata_host_alloc() on this line:
> host = kzalloc(sz, GFP_KERNEL);
>
> Get attached to a devres structure?
>
> It's a kref-managed data structure (see the call to kref_init() a few
> lines down), and the memory will be freed if you release the last
> reference on the kref, but that has nothing to do with devres.
>
> There's also the ports memory attached to the host structure as well,
> that is controlled by the lifetime of the kref, not a devres reference
> that I can see.
>
> Or am I missing some link somewhere here?
Checking again, the path is not super obvious, but it is there:
ata_devres_release() calls ata_host_put(), which drops the kref on the ata_host
memory and will thus free it when the last ref on the dev is dropped.
ata_host_release() is used for that and this function does free everything,
including the port memory within the ata_host structure.
So in essence, the ata_host and ata_port resources are like devres. I wonder if
actually making them real devres would make the code cleaner and simpler.
I will dig into this more to make sure there is no memory leak. As Sasha pointed
out, it seems that the last ref on the dev is actually never dropped. So there
may be an actual memory leak on error but it was there already.
--
Damien Le Moal
Western Digital Research