Re: [net PATCH v2 3/6] eth: fbnic: use the Rx queue napi pointer to find the napi vector

On Thu, Sep 17, 2026 at 2:12 PM [off-list ref] wrote:

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

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

- [High] The invariant the patch relies on - "The pointer is NULL exactly
  while the datapath is down" - does not hold across PCIe…

Pre-existing issues:
- [High] fbnic_queue_stop() uses nv->qt[t].sub0.q_idx as the queue
  identifier for every triad, but that field is 0 for two kinds of…
- [High] fbnic_alloc_qt_page_pools(): when the queue has an unreadable
  memory provider, the second page_pool_create() overwrites the local…

--

Here is the review report.

eth: fbnic: use the Rx queue napi pointer to find the napi vector

The queue management ndos stop deriving the napi vector from
fbn->napi[idx % fbn->num_napi] and instead read the association that
fbnic_set_netif_napi() published in netdev_rx_queue.napi.  A new helper
fbnic_rxq_nv() performs the lookup, and fbnic_queue_mem_alloc() returns
-ENETDOWN when the pointer is NULL.

quoted

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
index 81a30e2d449b2..e93174fc1239f 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c

[ ... ]

quoted

@@ -2829,6 +2830,17 @@ void fbnic_napi_depletion_check(struct net_device *netdev)
      fbnic_wrfl(fbd);
 }

+/* Returns the napi vector servicing an Rx queue, or NULL if the datapath
+ * is torn down.  The association is published by fbnic_set_netif_napi()
+ * and cleared by fbnic_reset_netif_napi(), both under the instance lock.
+ */
+static struct fbnic_napi_vector *fbnic_rxq_nv(struct net_device *dev, int idx)
+{
+     struct napi_struct *napi = __netif_get_rx_queue(dev, idx)->napi;
+
+     return napi ? container_of(napi, struct fbnic_napi_vector, napi) : NULL;
+}
+

[Severity: High]
The commit message says "The pointer is NULL exactly while the datapath is
down".  Does that hold across the resume path?

In fbnic_pci.c, the publish and the enable are split across two calls with
the instance lock dropped in between:

__fbnic_pm_resume()
    netdev_lock()
    __fbnic_open()
        fbnic_set_netif_queues()
            fbnic_set_netif_napi()
                netif_queue_set_napi()   /* publishes rxq->napi */
    netdev_unlock()

__fbnic_pm_attach()
    if (netif_running(netdev))
        fbnic_up(fbn);               /* napi_enable_locked() happens here */
    netif_device_attach(netdev);

This one has me scratching my head in how the code was able to get
into this state in the first place and has been this way for so long.
I am assuming it was relying on the netif_device_detach/attach for
some security. I would probably fix this as a separate patch as it
requires restructuring for several reasons, and I would classify it as
a pre-existing issue.

First the fact that the fbnic_up operating outside of any locking is
problematic at best as it could easily race with a dev_close request
and end up leaving things in a bad state. Second it looks like this
pattern exists in other drivers as well such as igb so we will need to
likely fix it across several drivers.

Basically we should be holding the rtnl and the netdev_lock and run
things from open through up all in one shot, likely including the
netdev_device_attach. What I will probably look at doing is merging
the __fbnic_pm_resume and __fbnic_pm_attach calls all into
fbnic_pm_resume and then only call it from the err_resume instead of
running down two different paths.

[ ... ]

quoted

@@ -2841,8 +2853,16 @@ static int fbnic_queue_mem_alloc(struct net_device *dev,
      if (!netif_running(dev))
              return fbnic_alloc_qt_page_pools(fbn, qt, idx);

+     /* A failed PCIe recovery or resume can leave the datapath torn down
+      * while netif_running() is still true.  This ndo runs before
+      * netdev_rx_queue_restart() checks netif_running(), so bail out
+      * rather than touching rings and vectors that are already freed.
+      */
+     nv = fbnic_rxq_nv(dev, idx);
+     if (!nv)
+             return -ENETDOWN;
+

[Severity: High]
Continuing the point above: does a non-NULL rxq->napi actually imply the
datapath is up?  For the resume window the pointer is published while the
vectors exist but are not enabled, so this check passes and
fbnic_queue_stop() runs napi_disable_locked() on a NAPI that was only added
by netif_napi_add_config_locked(), which leaves NAPI_STATE_SCHED and
NAPI_STATE_NPSVC set:

net/core/dev.c:napi_disable_locked() {
        ...
        do {
                while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
                        usleep_range(20, 200);
                        val = READ_ONCE(n->state);
                }
        ...
}

napi_enable_locked() is the only thing that clears those bits, and it has
not run yet, so does this loop spin forever with the netdev instance lock
held, blocking the pending fbnic_up(), dev_close(), unregister and ethtool?

Would a "datapath enabled" state test, or moving fbnic_up() inside the
instance-locked region of the resume path, be a better fit than keying off
the published pointer?

Odds are pretty slim for hitting this race, but this would be
addressed via the locking bug fix I called out above. Again the
pattern before was more of a NULL pointer dereference that was a
guaranteed hit versus this locking race.

quoted

+     real = container_of(fbn->rx[idx], struct fbnic_q_triad, cmpl);
-     nv = fbn->napi[idx % fbn->num_napi];

      fbnic_ring_init(&qt->sub0, real->sub0.doorbell, real->sub0.q_idx,
                      real->sub0.flags);

[Severity: High]
This is a pre-existing issue, but since both branches of this ndo reach it,
could fbnic_alloc_qt_page_pools() be dereferencing an error pointer?  The
second page_pool_create() overwrites the local pp, and the error label then
destroys that value rather than the pool it should be releasing:

        qt->sub0.page_pool = pp;
        if (netif_rxq_has_unreadable_mp(fbn->netdev, rxq_idx)) {
                ...
                pp = page_pool_create(&pp_params);
                if (IS_ERR(pp))
                        goto err_destroy_sub0;
        ...
err_destroy_sub0:
        page_pool_destroy(pp);
        return PTR_ERR(pp);

net/core/page_pool.c:page_pool_destroy() only filters NULL:

        if (!pool)
                return;

        if (!page_pool_put(pool))
                return;

so page_pool_put() would dereference the ERR_PTR, and qt->sub0.page_pool
leaks because netdev_rx_queue_reconfig() does not call ndo_queue_mem_free
on the mem_alloc failure path.  The reachable trigger is a devmem or
io_uring bind on a queue with an unreadable memory provider where the
second page_pool_create() fails with -ENOMEM, or where the provider's
->init() returns an error.  Would saving the error before destroying
qt->sub0.page_pool address this?

[ ... ]

This is yet another pre-existing issue that would likely be better
fixed as it is an unrelated problem. Looks like it should be a 2 liner
to use sub0.page_pool instead of pp in page_pool_destroy, and then to
reset that back to NULL.

quoted

@@ -2915,7 +2935,7 @@ static int fbnic_queue_stop(struct net_device *dev, void *qmem, int idx)
      int err;

      real = container_of(fbn->rx[idx], struct fbnic_q_triad, cmpl);
-     nv = fbn->napi[idx % fbn->num_napi];
+     nv = fbnic_rxq_nv(dev, idx);
      fbnic_dbg_nv_exit(nv);

      napi_disable_locked(&nv->napi);

[Severity: High]
This isn't a bug introduced by this patch, but does the standalone-Tx
configuration this patch targets remain incorrect afterwards?  The rest of
fbnic_queue_stop() uses sub0.q_idx as the queue identifier:

        for (i = 0; i < nv->txt_count; i++)
                netif_stop_subqueue(dev, nv->qt[i].sub0.q_idx);
        fbnic_nv_disable(fbn, nv);

        for (t = 0; t < nv->txt_count + nv->rxt_count; t++) {
                err = fbnic_wait_queue_idle(fbn, t >= nv->txt_count,
                                            nv->qt[t].sub0.q_idx);

but sub0.q_idx is 0 for two kinds of triads.  For the placeholder Tx triad
of an Rx-only vector, fbnic_alloc_napi_vector() sets:

                } else {
                        fbnic_ring_init(&qt->sub0, db, 0,
                                        FBNIC_RING_F_DISABLED);
                }

and for the Rx BDQ rings:

                db = &uc_addr[FBNIC_QUEUE(rxq_idx) + FBNIC_QUEUE_BDQ_HPQ_TAIL];
                fbnic_ring_init(&qt->sub0, db, 0,
                                FBNIC_RING_F_CTX | FBNIC_RING_F_STATS);

so the real HW block index only lives in cmpl.q_idx.

I agree that this is a pre-existing bug, but I wouldn't fix it in this
patch. I would probably introduce yet another patch to resolve it.

By my count we have 3 pre-existing issues that aren't directly related
to the original problem. If possible I would prefer to address them
separately, otherwise I can tack on 3 additional diffs to the set and
resubmit.