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