[PATCH v2] ata: libata: Fix memory leak for error path in ata_host_alloc()

[PATCH v2] ata: libata: Fix memory leak for error path in ata_host_alloc()

[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>
---
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);
-- 
2.39.2

Re: [PATCH v2] ata: libata: Fix memory leak for error path in ata_host_alloc()

[Yu Kuai]

在 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);
> 

Re: [PATCH v2] ata: libata: Fix memory leak for error path in ata_host_alloc()

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

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

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2] ata: libata: Fix memory leak for error path in ata_host_alloc()

[Zheng Qixing]

在 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);

Re: [PATCH v2] ata: libata: Fix memory leak for error path in ata_host_alloc()

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

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

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2] ata: libata: Fix memory leak for error path in ata_host_alloc()

[Zheng Qixing]

在 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);
>>

Re: [PATCH v2] ata: libata: Fix memory leak for error path in ata_host_alloc()

[Niklas Cassel]

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