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

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

On 2/2/2026 2:58 AM, Jijie Shao wrote:

  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

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. 

on 2026/2/4 9:30, Jacob Keller wrote:

On 2/2/2026 2:58 AM, Jijie Shao wrote:

quoted

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

quoted

  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

quoted

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

on 2026/2/4 10:38, Jakub Kicinski wrote:

On Mon, 2 Feb 2026 18:58:37 +0800 Jijie Shao wrote:

quoted

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

On 2/4/2026 6:25 AM, Jijie Shao wrote:

quoted

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