[PATCH] net: cxgb: Fix skb reference leak in espibug_workaround_t204()

[PATCH] net: cxgb: Fix skb reference leak in espibug_workaround_t204()

[Wentao Liang]

The timer handler takes an extra reference with skb_get() before handing
the skb to t1_sge_tx(), so that the skb survives until the DMA has
completed. When the command queue is full, t1_sge_tx() returns
NETDEV_TX_BUSY without queueing the skb, and the return value is
ignored, so the extra reference is never dropped.

Free the skb when t1_sge_tx() reports that it did not consume it.

Fixes: f1d3d38af757 ("[PATCH] chelsio: add support for other 10G boards")
Cc: stable@vger.kernel.org
Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
---
 drivers/net/ethernet/chelsio/cxgb/sge.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb/sge.c b/drivers/net/ethernet/chelsio/cxgb/sge.c
index d8fca697874a..d4677c5b8d74 100644
--- a/drivers/net/ethernet/chelsio/cxgb/sge.c
+++ b/drivers/net/ethernet/chelsio/cxgb/sge.c
@@ -2052,7 +2052,8 @@ static void espibug_workaround_t204(struct timer_list *t)
 			 * the skb once the DMA has completed.
 			 */
 			skb = skb_get(skb);
-			t1_sge_tx(skb, adapter, 0, adapter->port[i].dev);
+			if (t1_sge_tx(skb, adapter, 0, adapter->port[i].dev))
+				dev_kfree_skb_any(skb);
 		}
 	}
 	mod_timer(&sge->espibug_timer, jiffies + sge->espibug_timeout);
-- 
2.34.1

Re: [PATCH] net: cxgb: Fix skb reference leak in espibug_workaround_t204()

[Jakub Kicinski]

On Thu, 17 Sep 2026 10:54:25 +0000 Wentao Liang wrote:
> The timer handler takes an extra reference with skb_get() before handing
> the skb to t1_sge_tx(), so that the skb survives until the DMA has
> completed. When the command queue is full, t1_sge_tx() returns
> NETDEV_TX_BUSY without queueing the skb, and the return value is
> ignored, so the extra reference is never dropped.

You have sent way too many patches at once. We have a hard limit of 
15 patches per person, but really if you're sending low value garbage
you should not send more than 8. So that you can apply the feedback
you receive to later postings.

You may repost new version of the patches to which you receive feedback.
But I don't want to see any new patches from @iscas.ac.cn for
drivers/net and net/ for a month.

Please tell your coworkers/friends.
-- 
pv-bot: 15cnt

Re: [PATCH] net: cxgb: Fix skb reference leak in espibug_workaround_t204()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 (2) · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Incomplete fix: the identical skb reference leak remains in
  the sibling ESPI workaround timer callback espibug_workaround()…

Pre-existing issues:
- [High] Pre-existing skb_buff_head list corruption in the exact call
  path the patch touches: espibug_workaround_t204() re-submits the very…
- [High] Pre-existing dangling cached skb pointer leading to use-after-
  free and double free around the espibug_skb[] reference protocol that…

--

net: cxgb: Fix skb reference leak in espibug_workaround_t204()

The ESPI workaround timer for T204 adapters takes an extra reference on the
cached espibug skb before handing it to t1_sge_tx().  When t1_sge_tx()
returns NETDEV_TX_BUSY the skb was never queued, so the patch drops the
extra reference in that case.

> diff --git a/drivers/net/ethernet/chelsio/cxgb/sge.c b/drivers/net/ethernet/chelsio/cxgb/sge.c
> index d8fca697874a5..d4677c5b8d744 100644
> --- a/drivers/net/ethernet/chelsio/cxgb/sge.c
> +++ b/drivers/net/ethernet/chelsio/cxgb/sge.c
> @@ -2052,7 +2052,8 @@ static void espibug_workaround_t204(struct timer_list *t)
>  			 * the skb once the DMA has completed.
>  			 */
>  			skb = skb_get(skb);
> -			t1_sge_tx(skb, adapter, 0, adapter->port[i].dev);
> +			if (t1_sge_tx(skb, adapter, 0, adapter->port[i].dev))
> +				dev_kfree_skb_any(skb);
>  		}
>  	}
>  	mod_timer(&sge->espibug_timer, jiffies + sge->espibug_timeout);

[Severity: Medium]
Should the sibling handler espibug_workaround() in the same file get the
same treatment?  It has the identical sequence and still discards the
return value:

drivers/net/ethernet/chelsio/cxgb/sge.c:espibug_workaround() {
	        /* bump the reference count to avoid freeing of the
	         * skb once the DMA has completed.
	         */
	        skb = skb_get(skb);
	        t1_sge_tx(skb, adapter, 0, adapter->port[0].dev);
}

That callback is the one installed for single-port T2 boards:

	if (is_T2(sge->adapter)) {
		timer_setup(&sge->espibug_timer,
			    adapter->params.nports > 1 ? espibug_workaround_t204 : espibug_workaround,
			    0);

and subr.c lists single-port T2 boards (CHBT_BOARD_N210, CHBT_BOARD_CHT210,
CH_BRD_T210_1F, CH_BRD_T210_1CU) with .port_number = 1, so it is live code.

It also looks like the more exposed of the two: it does not test
netif_queue_stopped() before calling t1_sge_tx(), and sge->espibug_timeout
stays at 1 jiffy for nports == 1 versus HZ/100 for T204, so it re-enters the
same call much more often.

Since t1_sge_stop() only drops a single reference per slot:

	for (i = 0; i < MAX_NPORTS; i++)
		kfree_skb(sge->espibug_skb[i]);

and t1_start_xmit() only repopulates the slot when it reads NULL, does each
leaked reference make sge->espibug_skb[0] permanently unfreeable?  Would it
make sense to fix both call sites in one patch?

[Severity: High]
This isn't a bug introduced by this patch, but can the same skb pointer end
up enqueued twice on the scheduler queue in this exact path?

On T204 sge->tx_sched is non-NULL and skb->dev is still set from the
original t1_start_xmit(), so t1_sge_tx() always routes this submission
through sched_skb():

	if (sge->tx_sched && !qid && skb->dev) {
use_sched:
		...
		skb = sched_skb(sge, skb, credits);
		if (!skb) {
			spin_unlock(&q->lock);
			return NETDEV_TX_OK;
		}

sched_skb() enqueues unconditionally and can then leave the skb on the
queue while returning NULL:

drivers/net/ethernet/chelsio/cxgb/sge.c:sched_skb() {
		skbq = &s->p[skb->dev->if_port].skbq;
		__skb_queue_tail(skbq, skb);
		s->num++;
	...
	if (credits < MAX_SKB_FRAGS + 1)
		goto out;
}

That low-credit state (and the port quota check where len <=
s->p[s->port].avail fails) is what a stuck ESPI produces, which is exactly
when this timer handler runs.

Since skb_get() returns the same pointer that is cached in
sge->espibug_skb[i], and nothing in espibug_workaround_t204() tests whether
the previous submission is still sitting in the scheduler queue, the next
expiry hands the identical, still-queued pointer to __skb_queue_tail()
again.

Can that corrupt the skbq list (skb->next/prev pointing at itself) and
desynchronize s->num, leading to reclaim_completed_tx() later consuming an
skb the queue still references?  Note the new dev_kfree_skb_any() does not
cover this case, since sched_skb() success is reported as NETDEV_TX_OK.

[Severity: High]
This is a pre-existing issue, but the new dev_kfree_skb_any() call may make
it worse: is the cached pointer in sge->espibug_skb[] left dangling after
the interface goes down?

t1_sge_stop() frees the slot but never clears it:

drivers/net/ethernet/chelsio/cxgb/sge.c:t1_sge_stop() {
	for (i = 0; i < MAX_NPORTS; i++)
		kfree_skb(sge->espibug_skb[i]);
}

The only writer of the array is t1_start_xmit(), and it only publishes a
new skb when the slot reads NULL:

		if ((unlikely(!adapter->sge->espibug_skb[dev->if_port]))) {
			...
				adapter->sge->espibug_skb[dev->if_port] = skb;

So after cxgb_down() -> t1_sge_stop() drops the last reference, does the
slot keep pointing at freed memory while also blocking a fresh skb from
being cached?

On the following ifup, t1_sge_start() re-arms the timer:

	if (is_T2(sge->adapter))
		mod_timer(&sge->espibug_timer, jiffies + sge->espibug_timeout);

and espibug_workaround_t204() then reads skb->cb[0], writes into the buffer
via skb_copy_to_linear_data_offset(), calls skb_get() on it and hands it to
write_tx_descs() for DMA.  With this patch the same freed pointer can also
reach dev_kfree_skb_any() on the NETDEV_TX_BUSY path, and a second
interface down would run kfree_skb() on it again.

Would setting sge->espibug_skb[i] = NULL after the kfree_skb() in
t1_sge_stop() be the right fix here?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917105425.2147804-1-vulab%40iscas.ac.cn

Re: [PATCH] net: cxgb: Fix skb reference leak in espibug_workaround_t204()

[Wentao Liang]

Hi Jakub,

I am very sorry for the late reply, and for the flood of patches that
made it necessary. You are right on both points, and I apologize for the
extra work and confusion this caused.

You are right that I sent far more patches at once than anyone could
review, and more than I could follow up on. The hard limit of 15 patches
per person, and the guidance to stay at 8 or fewer when the changes are
of low value, exist so that the feedback on one posting can be applied to
the next. I did not respect that, and that is on me.

I understand why you asked for a one-month pause on new patches from
@iscas.ac.cn for drivers/net and net/. I have also gone back to the
kernel documentation on subsystem identification, patch set organization
and testing, and on how this kind of work should be submitted, and I will
follow it.

To be clear about how these patches are produced: we use static analysis
tools to find candidate issues, and then DeepSeek and GPT-6.1 Sol to
audit the candidates and help prepare the fixes. I review every change
and sign it off myself, and I take responsibility for anything I send.

This has been an important lesson. I am going through the remaining
feedback one item at a time by hand, and replying to each one promptly.
Going forward I will not send patches in bulk, and I will keep every
posting well within what I can review and answer. I have also shared
your message with my colleagues, as you asked.

The cxgb patch is a small reference leak fix in an old T2 driver. A v2
that also covers the single-port timer callback with the same pattern is
ready; I will send it separately as one patch rather than in this reply.
If you would rather I drop it, please tell me and I will.

Sorry again, and thank you for your patience.

Wentao

Re: [PATCH] net: cxgb: Fix skb reference leak in espibug_workaround_t204()

[Jakub Kicinski]

On Sun, 08 Nov 2026 10:34:46 -0000 Wentao Liang wrote:
> The cxgb patch is a small reference leak fix in an old T2 driver. A v2
> that also covers the single-port timer callback with the same pattern is
> ready; I will send it separately as one patch rather than in this reply.
> If you would rather I drop it, please tell me and I will.

Do not send revised versions of your patches.

They are all worthless slop.