Re: [PATCH net] net: ntb_netdev: Fix statistics races

[Simon Horman <horms@kernel.org>]

On Tue, Aug 25, 2026 at 02:57:39AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Pending RX and TX entries are leaked because their associated SKB buffers are not freed when the transport queue is destroyed.
> --
> 
> --- Patch [1]: [PATCH net] net: ntb_netdev: Fix statistics races ---
> commit fc558d0a991ec95505df001446270417159e2eaf
> Author: Koichiro Den <den@valinux.co.jp>
> 
> net: ntb_netdev: Fix statistics races
> 
> ntb_netdev updates shared net_device stats from per-QP RX and TX callbacks.
> This commit shifts the driver to use core-managed per-CPU dstats. It also
> tears down transport queues from ndo_uninit before the core frees dstats.
> 
> > diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
> > index 9c171697e7620..6a1e58d5d7d67 100644
> > --- a/drivers/net/ntb_netdev.c
> > +++ b/drivers/net/ntb_netdev.c
> [ ... ]
> > @@ -427,7 +428,19 @@ static int ntb_netdev_change_mtu(struct net_device *ndev, int new_mtu)
> >  	return rc;
> >  }
> >  
> > +static void ntb_netdev_uninit(struct net_device *ndev)
> > +{
> > +	struct ntb_netdev *dev = netdev_priv(ndev);
> > +	unsigned int q;
> > +
> > +	for (q = 0; q < dev->num_queues; q++) {
> > +		ntb_transport_free_queue(dev->queues[q].qp);
> 
> [Severity: High]
> This is a pre-existing issue, but does calling ntb_transport_free_queue()
> leak the SKBs attached to pending packets?

FWIIW, as a pre-existing issue I lean towards this being out-of-scope
for the patch under review. And, rather, a topic for possible follow-up.

> 
> Looking at ntb_transport_free_queue() in drivers/ntb/ntb_transport.c,
> it cleans up pending items from the queues like this:
> 
> drivers/ntb/ntb_transport.c:ntb_transport_free_queue() {
>     ...
>     while ((entry = ntb_list_rm(&qp->ntb_rx_q_lock, &qp->rx_post_q))) {
>         dev_warn(&pdev->dev, "Freeing item from non-empty rx_post_q\n");
>         kfree(entry);
>     }
>     ...
> }
> 
> It frees the entry, but it doesn't seem to free or return the attached
> client buffer (entry->cb_data). Since ntb_netdev uses cb_data to store
> allocated SKBs, does this unconditionally leak any in-flight packets
> when the queue is destroyed?
> 
> > +		dev->queues[q].qp = NULL;
> > +	}
> > +}
> > +
> >  static const struct net_device_ops ntb_netdev_ops = {
> > +	.ndo_uninit = ntb_netdev_uninit,
> >  	.ndo_open = ntb_netdev_open,
> >  	.ndo_stop = ntb_netdev_close,
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260824025720.3520288-1-den@valinux.co.jp?part=1