[PATCH net] net: hns3: fix double free issue for tx spare buffer

[PATCH net] net: hns3: fix double free issue for tx spare buffer

[Jijie Shao]

From: Jian Shen <shenjian15@huawei.com>

The driver missed to clear ring->tx_spare to NULL when
fail to initialize tx spare buffer. And it will try to
free the tx spare buffer in hns3_fini_ring() if tx_spare
is not NULL. So it may cause double free issue.

Fixes: 907676b130711 ("net: hns3: use tx bounce buffer for small packets")
Signed-off-by: Jian Shen <shenjian15@huawei.com>
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
index 7a9573dcab74..e879b04e21b0 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
@@ -1048,13 +1048,13 @@ static void hns3_init_tx_spare_buffer(struct hns3_enet_ring *ring)
 	int order;
 
 	if (!alloc_size)
-		return;
+		goto not_init;
 
 	order = get_order(alloc_size);
 	if (order > MAX_PAGE_ORDER) {
 		if (net_ratelimit())
 			dev_warn(ring_to_dev(ring), "failed to allocate tx spare buffer, exceed to max order\n");
-		return;
+		goto not_init;
 	}
 
 	tx_spare = devm_kzalloc(ring_to_dev(ring), sizeof(*tx_spare),
@@ -1092,6 +1092,13 @@ static void hns3_init_tx_spare_buffer(struct hns3_enet_ring *ring)
 	devm_kfree(ring_to_dev(ring), tx_spare);
 devm_kzalloc_error:
 	ring->tqp->handle->kinfo.tx_spare_buf_size = 0;
+not_init:
+	/* When driver init or reset_init, the ring->tx_spare is always NULL;
+	 * but when called from hns3_set_ringparam, it's usually not NULL, and
+	 * will be restored if hns3_init_all_ring() failed. So it's safe to set
+	 * ring->tx_spare to NULL here.
+	 */
+	ring->tx_spare = NULL;
 }
 
 /* Use hns3_tx_spare_space() to make sure there is enough buffer
-- 
2.33.0

Re: [PATCH net] net: hns3: fix double free issue for tx spare buffer

[Jacob Keller]

On 2/2/2026 2:58 AM, Jijie Shao wrote:
> From: Jian Shen <shenjian15@huawei.com>
> 
> The driver missed to clear ring->tx_spare to NULL when
> fail to initialize tx spare buffer. And it will try to
> free the tx spare buffer in hns3_fini_ring() if tx_spare
> is not NULL. So it may cause double free issue.
> 
> Fixes: 907676b130711 ("net: hns3: use tx bounce buffer for small packets")
> Signed-off-by: Jian Shen <shenjian15@huawei.com>
> Signed-off-by: Jijie Shao <shaojijie@huawei.com>
> ---
>   drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> index 7a9573dcab74..e879b04e21b0 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> @@ -1048,13 +1048,13 @@ static void hns3_init_tx_spare_buffer(struct hns3_enet_ring *ring)
>   	int order;
>   
>   	if (!alloc_size)
> -		return;
> +		goto not_init;
>   
>   	order = get_order(alloc_size);
>   	if (order > MAX_PAGE_ORDER) {
>   		if (net_ratelimit())
>   			dev_warn(ring_to_dev(ring), "failed to allocate tx spare buffer, exceed to max order\n");
> -		return;
> +		goto not_init;
>   	}
>   
>   	tx_spare = devm_kzalloc(ring_to_dev(ring), sizeof(*tx_spare),
> @@ -1092,6 +1092,13 @@ static void hns3_init_tx_spare_buffer(struct hns3_enet_ring *ring)
>   	devm_kfree(ring_to_dev(ring), tx_spare);


>   devm_kzalloc_error:
>   	ring->tqp->handle->kinfo.tx_spare_buf_size = 0;
> +not_init:
> +	/* When driver init or reset_init, the ring->tx_spare is always NULL;
> +	 * but when called from hns3_set_ringparam, it's usually not NULL, and
> +	 * will be restored if hns3_init_all_ring() failed. So it's safe to set
> +	 * ring->tx_spare to NULL here.
> +	 */
> +	ring->tx_spare = NULL;

This feels weird. If it was already NULL there's no reason to write a 
NULL to it. And if it isn't NULL, we should have released and 
overwritten it previously by whatever caller was going to free it, 
otherwise you'd just have a different kind of memory leak...

It looks like the only places that call devm_kfree on the tx_spare is 
hns3_fini_ring. It sets the value to NULL after freeing it. I am not 
following how it can ever be non-NULL where setting this to NULL and 
overwriting it would not lead to a memory leak (aside from the fact that 
this is devm memory and will be released on removing the device)

Is it because the ring memory isn't zero-initialized?? But that wouldn't 
be a double-free that would be a freeing a bad address...

Hmm. You create a copy of the ring parameters in hns3_set_ringparam. 
Then if that fails, you copy things back. Otherwise you release the 
tmp_rings... but the new rings didn't get cleared memory.. So the issue 
is that your ring structures effectively have garbage memory in them 
when initialized in set_ringparam.

You can't just check the old tx_spare and release it here because doing 
so would corrupt the memory pointed by the temporary copy made when 
changing ring paramters (as it would no longer be valid), and you can't 
assume it was zero initialized because your new rings were built in 
place on top of the old ring structures.

Ok. The fix makes sense but it does take a bit to understand why. You 
effectively have to treat the memory in tx_spare as "uninitialized garbage".

Other solutions would require more significant refactoring. You could 
memset or otherwise zero out the ring array structures entirely when 
allocating new rings in hns3_init_all_ring() for example. Not sure that 
is any easier to follow, and its not my driver so:

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

>   }
>   
>   /* Use hns3_tx_spare_space() to make sure there is enough buffer

Re: [PATCH net] net: hns3: fix double free issue for tx spare buffer

[Jijie Shao]

on 2026/2/4 9:30, Jacob Keller wrote:
>
>
> On 2/2/2026 2:58 AM, Jijie Shao wrote:
>> From: Jian Shen <shenjian15@huawei.com>
>>
>> The driver missed to clear ring->tx_spare to NULL when
>> fail to initialize tx spare buffer. And it will try to
>> free the tx spare buffer in hns3_fini_ring() if tx_spare
>> is not NULL. So it may cause double free issue.
>>
>> Fixes: 907676b130711 ("net: hns3: use tx bounce buffer for small 
>> packets")
>> Signed-off-by: Jian Shen <shenjian15@huawei.com>
>> Signed-off-by: Jijie Shao <shaojijie@huawei.com>
>> ---
>>   drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c 
>> b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>> index 7a9573dcab74..e879b04e21b0 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>> @@ -1048,13 +1048,13 @@ static void hns3_init_tx_spare_buffer(struct 
>> hns3_enet_ring *ring)
>>       int order;
>>         if (!alloc_size)
>> -        return;
>> +        goto not_init;
>>         order = get_order(alloc_size);
>>       if (order > MAX_PAGE_ORDER) {
>>           if (net_ratelimit())
>>               dev_warn(ring_to_dev(ring), "failed to allocate tx 
>> spare buffer, exceed to max order\n");
>> -        return;
>> +        goto not_init;
>>       }
>>         tx_spare = devm_kzalloc(ring_to_dev(ring), sizeof(*tx_spare),
>> @@ -1092,6 +1092,13 @@ static void hns3_init_tx_spare_buffer(struct 
>> hns3_enet_ring *ring)
>>       devm_kfree(ring_to_dev(ring), tx_spare);
>
>
>>   devm_kzalloc_error:
>>       ring->tqp->handle->kinfo.tx_spare_buf_size = 0;
>> +not_init:
>> +    /* When driver init or reset_init, the ring->tx_spare is always 
>> NULL;
>> +     * but when called from hns3_set_ringparam, it's usually not 
>> NULL, and
>> +     * will be restored if hns3_init_all_ring() failed. So it's safe 
>> to set
>> +     * ring->tx_spare to NULL here.
>> +     */
>> +    ring->tx_spare = NULL;
>
> This feels weird. If it was already NULL there's no reason to write a 
> NULL to it. And if it isn't NULL, we should have released and 
> overwritten it previously by whatever caller was going to free it, 
> otherwise you'd just have a different kind of memory leak...
>
> It looks like the only places that call devm_kfree on the tx_spare is 
> hns3_fini_ring. It sets the value to NULL after freeing it. I am not 
> following how it can ever be non-NULL where setting this to NULL and 
> overwriting it would not lead to a memory leak (aside from the fact 
> that this is devm memory and will be released on removing the device)
>
> Is it because the ring memory isn't zero-initialized?? But that 
> wouldn't be a double-free that would be a freeing a bad address...
>
> Hmm. You create a copy of the ring parameters in hns3_set_ringparam. 
> Then if that fails, you copy things back. Otherwise you release the 
> tmp_rings... but the new rings didn't get cleared memory.. So the 
> issue is that your ring structures effectively have garbage memory in 
> them when initialized in set_ringparam.
>
> You can't just check the old tx_spare and release it here because 
> doing so would corrupt the memory pointed by the temporary copy made 
> when changing ring paramters (as it would no longer be valid), and you 
> can't assume it was zero initialized because your new rings were built 
> in place on top of the old ring structures.
>
> Ok. The fix makes sense but it does take a bit to understand why. You 
> effectively have to treat the memory in tx_spare as "uninitialized 
> garbage".
>
> Other solutions would require more significant refactoring. You could 
> memset or otherwise zero out the ring array structures entirely when 
> allocating new rings in hns3_init_all_ring() for example. Not sure 
> that is any easier to follow, and its not my driver so:
>
> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Thank you for your careful review, I apologize for the trouble this patch has caused you.

We considered the logic to be quite complex, so we added comments in the code to explain it.
However, for reviewers who are not familiar with our driver, these comments might be a bit unclear.
I will add more detailed explanations in the commit message for v2.

Thanks,
Jijie Shao


>
>>   }
>>     /* Use hns3_tx_spare_space() to make sure there is enough buffer
>
>

Re: [PATCH net] net: hns3: fix double free issue for tx spare buffer

[Jacob Keller]

On 2/4/2026 6:25 AM, Jijie Shao wrote:
>> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
> 
> Thank you for your careful review, I apologize for the trouble this 
> patch has caused you.
> 
> We considered the logic to be quite complex, so we added comments in the 
> code to explain it.
> However, for reviewers who are not familiar with our driver, these 
> comments might be a bit unclear.
> I will add more detailed explanations in the commit message for v2.
> 
> Thanks,
> Jijie Shao

Appreciate it, thanks! Please feel free to add my Reviewed-by to v2 if 
the code remains unchanged.

Regards,
Jake

Re: [PATCH net] net: hns3: fix double free issue for tx spare buffer

[Jakub Kicinski]

On Mon, 2 Feb 2026 18:58:37 +0800 Jijie Shao wrote:
> The driver missed to clear ring->tx_spare to NULL when
> fail to initialize tx spare buffer. And it will try to
> free the tx spare buffer in hns3_fini_ring() if tx_spare
> is not NULL. So it may cause double free issue.

Please update this commit message so that reviewers don't have 
to do as much research. 

Re: [PATCH net] net: hns3: fix double free issue for tx spare buffer

[Jijie Shao]

on 2026/2/4 10:38, Jakub Kicinski wrote:
> On Mon, 2 Feb 2026 18:58:37 +0800 Jijie Shao wrote:
>> The driver missed to clear ring->tx_spare to NULL when
>> fail to initialize tx spare buffer. And it will try to
>> free the tx spare buffer in hns3_fini_ring() if tx_spare
>> is not NULL. So it may cause double free issue.
> Please update this commit message so that reviewers don't have
> to do as much research.

ok, I will send v2 to update the commit message

Thanks,
Jijie Shao