Re: [PATCH net] netpoll: bound the deferred transmit queue

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 1

- [Medium] In stacked netpoll configurations the new tail drop in
  `__netpoll_send_skb()` (net/core/netpoll.c:325-327) is completely…
- [Low] The new cap in `__netpoll_send_skb()` reads `npinfo->txq.qlen`
  via `skb_queue_len()` (a plain, unannotated load,…

--

netpoll: bound the deferred transmit queue

Adds a NETPOLL_TXQ_MAX cap of 1024 skbs on npinfo->txq and makes
__netpoll_send_skb() free the new skb and return NET_XMIT_DROP once the
queue is at that length, instead of letting the deferred queue grow until
the machine runs out of memory.

Link: https://lore.kernel.org/netdev/20061026225645.482978803@osdl.org/

> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index fe1e0cda5d6bf..8fd640955e463 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c

[ ... ]

> @@ -314,6 +322,10 @@ static netdev_tx_t __netpoll_send_skb(struct netpoll *np, struct sk_buff *skb)
>  	}
>  
>  	if (!dev_xmit_complete(status)) {
> +		if (skb_queue_len(&npinfo->txq) >= NETPOLL_TXQ_MAX) {
> +			dev_kfree_skb_irq(skb);
> +			goto out;
> +		}

[Severity: Medium]

The commit message says:

  "netconsole already counts NET_XMIT_DROP in its per-target
   xmit_drop_count, so the loss is visible in configfs."

Does that hold when the netconsole target sits on a stacked device such as
a bond, bridge, team, VLAN or macvlan?

In that case the queue that fills and gets capped is the lower device's
npinfo->txq, reached through a nested netpoll_send_skb().  For a bond:

net/core/netpoll.c:__netpoll_send_skb()   /* np->dev == bond0 */
  netpoll_start_xmit()
    bond_start_xmit()
      bond_dev_queue_xmit()
        if (unlikely(netpoll_tx_running(bond->dev)))
                return bond_netpoll_send_skb(...);
          netpoll_send_skb(slave->np)
            __netpoll_send_skb()          /* slave's txq hits the cap */
              ret = NET_XMIT_DROP;

The nested NET_XMIT_DROP is then propagated back as status in the outer
__netpoll_send_skb(), and NET_XMIT_DROP is 1:

include/linux/netdevice.h:dev_xmit_complete() {
	if (likely(rc < NET_XMIT_MASK))
		return true;
}

so the outer frame treats the drop as a completed transmit, skips the
!dev_xmit_complete(status) block, and falls through to:

	ret = NETDEV_TX_OK;
out:
	rcu_read_unlock();
	return ret;

drivers/net/netconsole.c:send_udp() only counts the drop when
result == NET_XMIT_DROP reaches it, so xmit_drop_count (configfs
transmit_errors) stays at zero for these setups.

The bridge and team wrappers are worse in this respect, since the return
value is dropped on the floor entirely:

net/bridge/br_private.h:
static inline void br_netpoll_send_skb(const struct net_bridge_port *p,
				       struct sk_buff *skb)
{
	netpoll_send_skb(p->np, skb);
}

Could either the commit message be narrowed to a target bound directly to a
physical device, or the drop path get its own accounting (for example
dev_core_stats_tx_dropped_inc()) so the loss is observable in the stacked
cases too?

[Severity: Low]

Is the cap intended to be a hard bound?  The length is read outside
npinfo->txq.lock, while the enqueue takes that lock on its own:

include/linux/skbuff.h:skb_queue_len() {
	return list_->qlen;
}

net/core/skbuff.c:skb_queue_tail() {
	spin_lock_irqsave(&list->lock, flags);
	__skb_queue_tail(list, newsk);
	spin_unlock_irqrestore(&list->lock, flags);
}

Since check and enqueue are not one critical section, can N CPUs each
observe qlen == 1023 and all enqueue, leaving the queue at 1023 + N?

The read is also a plain load racing with skb_queue_tail() from other
producers and with skb_dequeue()/skb_queue_head() in queue_process(),
which looks KCSAN-reportable.  Would skb_queue_len_lockless(), or doing the
test and the __skb_queue_tail() under a single
spin_lock_irqsave(&npinfo->txq.lock, flags), be preferable here?

The existing skb_queue_len(&npinfo->txq) == 0 test earlier in
__netpoll_send_skb() has the same annotation question, so that part is
pre-existing and not introduced by this patch; this patch adds a second
instance and bases the memory bound on it.

>  		skb_queue_tail(&npinfo->txq, skb);
>  		schedule_delayed_work(&npinfo->tx_work,0);
>  	}

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914041221.1028092-1-zack.gomez%40gmail.com