| Message ID | 20240822033050.2909195-1-zhengqixing@huaweicloud.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] ata: libata: Fix memory leak for error path in ata_host_alloc() | expand |
在 2024/08/22 11:30, Zheng Qixing 写道: > From: Zheng Qixing <zhengqixing@huawei.com> > > In ata_host_alloc(), if ata_port_alloc(host) fails to allocate memory > for a port, the allocated 'host' structure is not freed before returning > from the function. This results in a potential memory leak. > > This patch adds a kfree(host) before the error handling code is executed > to ensure that the 'host' structure is properly freed in case of an > allocation failure. > > Signed-off-by: Zheng Qixing <zhengqixing@huawei.com> LGTM Reviewed-by: Yu Kuai <yukuai3@huawei.com> Thanks. > --- > Changes in v2: > - error path is wrong in v1 > > drivers/ata/libata-core.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > index e4023fc288ac..f27a18990c38 100644 > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -5663,8 +5663,10 @@ struct ata_host *ata_host_alloc(struct device *dev, int n_ports) > } > > dr = devres_alloc(ata_devres_release, 0, GFP_KERNEL); > - if (!dr) > + if (!dr) { > + kfree(host); > goto err_out; > + } > > devres_add(dev, dr); > dev_set_drvdata(dev, host); >
On 8/22/24 12:30 PM, Zheng Qixing wrote: > From: Zheng Qixing <zhengqixing@huawei.com> > > In ata_host_alloc(), if ata_port_alloc(host) fails to allocate memory > for a port, the allocated 'host' structure is not freed before returning > from the function. This results in a potential memory leak. > > This patch adds a kfree(host) before the error handling code is executed > to ensure that the 'host' structure is properly freed in case of an > allocation failure. > > Signed-off-by: Zheng Qixing <zhengqixing@huawei.com> This needs a Fixes tag. So I added: Fixes: 2623c7a5f279 ("libata: add refcounting to ata_host") Cc: stable@vger.kernel.org> and applied to for-6.11-fixes. Thanks. > --- > Changes in v2: > - error path is wrong in v1 > > drivers/ata/libata-core.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > index e4023fc288ac..f27a18990c38 100644 > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -5663,8 +5663,10 @@ struct ata_host *ata_host_alloc(struct device *dev, int n_ports) > } > > dr = devres_alloc(ata_devres_release, 0, GFP_KERNEL); > - if (!dr) > + if (!dr) { > + kfree(host); > goto err_out; > + } > > devres_add(dev, dr); > dev_set_drvdata(dev, host);
On Thu, Aug 22, 2024 at 11:30:50AM +0800, Zheng Qixing wrote: > From: Zheng Qixing <zhengqixing@huawei.com> > > In ata_host_alloc(), if ata_port_alloc(host) fails to allocate memory > for a port, the allocated 'host' structure is not freed before returning > from the function. This results in a potential memory leak. This sentence is wrong. If ata_port_alloc() fails, we must have already called devres_alloc(ata_devres_release, ...); which means that when: ap = ata_port_alloc(host); if (!ap) goto err_out; ... err_out: devres_release_group(dev, NULL); return NULL; devres_release_group() will trigger a call to ata_host_release(). ata_host_release() calls kfree(host). So we will not leak "host" if ata_port_alloc() fails. > > This patch adds a kfree(host) before the error handling code is executed > to ensure that the 'host' structure is properly freed in case of an > allocation failure. > > Signed-off-by: Zheng Qixing <zhengqixing@huawei.com> > --- > Changes in v2: > - error path is wrong in v1 > > drivers/ata/libata-core.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > index e4023fc288ac..f27a18990c38 100644 > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -5663,8 +5663,10 @@ struct ata_host *ata_host_alloc(struct device *dev, int n_ports) > } > > dr = devres_alloc(ata_devres_release, 0, GFP_KERNEL); > - if (!dr) > + if (!dr) { > + kfree(host); > goto err_out; This code does free "host" if devres_alloc() fails, which looks correct, as "host" will currently be leaked if devres_alloc() fails. However, that is not what the commit log above claims :P Please update the commit message to reflect reality. Kind regards, Niklas
在 2024/8/23 9:10, Damien Le Moal 写道: > On 8/22/24 12:30 PM, Zheng Qixing wrote: >> From: Zheng Qixing <zhengqixing@huawei.com> >> >> In ata_host_alloc(), if ata_port_alloc(host) fails to allocate memory >> for a port, the allocated 'host' structure is not freed before returning >> from the function. This results in a potential memory leak. >> >> This patch adds a kfree(host) before the error handling code is executed >> to ensure that the 'host' structure is properly freed in case of an >> allocation failure. >> >> Signed-off-by: Zheng Qixing <zhengqixing@huawei.com> > This needs a Fixes tag. So I added: > > Fixes: 2623c7a5f279 ("libata: add refcounting to ata_host") > Cc: stable@vger.kernel.org> > > and applied to for-6.11-fixes. Thanks. Based on Niklas Cassel's suggestion, the commit message and the actual content of the patch do not match. It should state "if devres_alloc(ata_devres_release, 0, GFP_KERNEL) fails to allocate memory" instead of "if ata_port_alloc(host) fails to allocate memory for a port". Should I modify the commit message and submit a new version of the patch? Zheng Qixing >> --- >> Changes in v2: >> - error path is wrong in v1 >> >> drivers/ata/libata-core.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c >> index e4023fc288ac..f27a18990c38 100644 >> --- a/drivers/ata/libata-core.c >> +++ b/drivers/ata/libata-core.c >> @@ -5663,8 +5663,10 @@ struct ata_host *ata_host_alloc(struct device *dev, int n_ports) >> } >> >> dr = devres_alloc(ata_devres_release, 0, GFP_KERNEL); >> - if (!dr) >> + if (!dr) { >> + kfree(host); >> goto err_out; >> + } >> >> devres_add(dev, dr); >> dev_set_drvdata(dev, host);
On 8/26/24 11:49 AM, Zheng Qixing wrote: > > 在 2024/8/23 9:10, Damien Le Moal 写道: >> On 8/22/24 12:30 PM, Zheng Qixing wrote: >>> From: Zheng Qixing <zhengqixing@huawei.com> >>> >>> In ata_host_alloc(), if ata_port_alloc(host) fails to allocate memory >>> for a port, the allocated 'host' structure is not freed before returning >>> from the function. This results in a potential memory leak. >>> >>> This patch adds a kfree(host) before the error handling code is executed >>> to ensure that the 'host' structure is properly freed in case of an >>> allocation failure. >>> >>> Signed-off-by: Zheng Qixing <zhengqixing@huawei.com> >> This needs a Fixes tag. So I added: >> >> Fixes: 2623c7a5f279 ("libata: add refcounting to ata_host") >> Cc: stable@vger.kernel.org> >> >> and applied to for-6.11-fixes. Thanks. > > > Based on Niklas Cassel's suggestion, the commit message and the actual > content of the patch do not match. > > It should state "if devres_alloc(ata_devres_release, 0, GFP_KERNEL) > fails to allocate memory" instead of "if ata_port_alloc(host) fails to allocate > memory for a port". > > Should I modify the commit message and submit a new version of the patch? No need. I fixed it up. The commit message now is: ata: libata: Fix memory leak for error path in ata_host_alloc() In ata_host_alloc(), if devres_alloc() fails to allocate the device host resource data pointer, the already allocated ata_host structure is not freed before returning from the function. This results in a potential memory leak. Call kfree(host) before jumping to the error handling path to ensure that the ata_host structure is properly freed if devres_alloc() fails. > > > Zheng Qixing > > >>> --- >>> Changes in v2: >>> - error path is wrong in v1 >>> >>> drivers/ata/libata-core.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c >>> index e4023fc288ac..f27a18990c38 100644 >>> --- a/drivers/ata/libata-core.c >>> +++ b/drivers/ata/libata-core.c >>> @@ -5663,8 +5663,10 @@ struct ata_host *ata_host_alloc(struct device *dev, >>> int n_ports) >>> } >>> dr = devres_alloc(ata_devres_release, 0, GFP_KERNEL); >>> - if (!dr) >>> + if (!dr) { >>> + kfree(host); >>> goto err_out; >>> + } >>> devres_add(dev, dr); >>> dev_set_drvdata(dev, host); > >
在 2024/8/26 12:02, Damien Le Moal 写道: > On 8/26/24 11:49 AM, Zheng Qixing wrote: >> 在 2024/8/23 9:10, Damien Le Moal 写道: >>> On 8/22/24 12:30 PM, Zheng Qixing wrote: >>>> From: Zheng Qixing <zhengqixing@huawei.com> >>>> >>>> In ata_host_alloc(), if ata_port_alloc(host) fails to allocate memory >>>> for a port, the allocated 'host' structure is not freed before returning >>>> from the function. This results in a potential memory leak. >>>> >>>> This patch adds a kfree(host) before the error handling code is executed >>>> to ensure that the 'host' structure is properly freed in case of an >>>> allocation failure. >>>> >>>> Signed-off-by: Zheng Qixing <zhengqixing@huawei.com> >>> This needs a Fixes tag. So I added: >>> >>> Fixes: 2623c7a5f279 ("libata: add refcounting to ata_host") >>> Cc: stable@vger.kernel.org> >>> >>> and applied to for-6.11-fixes. Thanks. >> >> Based on Niklas Cassel's suggestion, the commit message and the actual >> content of the patch do not match. >> >> It should state "if devres_alloc(ata_devres_release, 0, GFP_KERNEL) >> fails to allocate memory" instead of "if ata_port_alloc(host) fails to allocate >> memory for a port". >> >> Should I modify the commit message and submit a new version of the patch? > No need. I fixed it up. The commit message now is: > > ata: libata: Fix memory leak for error path in ata_host_alloc() > > In ata_host_alloc(), if devres_alloc() fails to allocate the device host > resource data pointer, the already allocated ata_host structure is not > freed before returning from the function. This results in a potential > memory leak. > > Call kfree(host) before jumping to the error handling path to ensure > that the ata_host structure is properly freed if devres_alloc() fails. Thanks. :) Zheng Qixing > >> >> Zheng Qixing >> >> >>>> --- >>>> Changes in v2: >>>> - error path is wrong in v1 >>>> >>>> drivers/ata/libata-core.c | 4 +++- >>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c >>>> index e4023fc288ac..f27a18990c38 100644 >>>> --- a/drivers/ata/libata-core.c >>>> +++ b/drivers/ata/libata-core.c >>>> @@ -5663,8 +5663,10 @@ struct ata_host *ata_host_alloc(struct device *dev, >>>> int n_ports) >>>> } >>>> dr = devres_alloc(ata_devres_release, 0, GFP_KERNEL); >>>> - if (!dr) >>>> + if (!dr) { >>>> + kfree(host); >>>> goto err_out; >>>> + } >>>> devres_add(dev, dr); >>>> dev_set_drvdata(dev, host); >>
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index e4023fc288ac..f27a18990c38 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -5663,8 +5663,10 @@ struct ata_host *ata_host_alloc(struct device *dev, int n_ports) } dr = devres_alloc(ata_devres_release, 0, GFP_KERNEL); - if (!dr) + if (!dr) { + kfree(host); goto err_out; + } devres_add(dev, dr); dev_set_drvdata(dev, host);