Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider: - [High] Memory leaks in `bnxt_rx_pkt` when `BNXT_RX_OFFLOAD_MODE` is active. - [High] Incomplete error rollback during offload attach leads to NULL pointer dereferences. - [High] Invalid free of knod core-owned memory in `bnxt_knod_uninit()`. - [High] Detaching the offload accelerator silently corrupts standard XDP state. - [High] Type confusion via unconditional cast of `netmem_ref` to `net_iov`. - [High] Prematurely setting `bp->knodev` to NULL during detach triggers multiple NULL pointer dereferences. - [High] Ignoring the return value of `bnxt_knod_init()` causes a NULL pointer dereference on unload. - [High] Head-of-line blocking and packet stalls in `bnxt_rx_offload_act_handler` due to restrictive limit based on TX availability. - [High] Unbalanced lifecycle operations for `knod_dev_start` leading to operational failure or resource leaks. - [High] Incorrect queue bounds check in `bnxt_rx_offload_act_handler()` leads to out-of-bounds access and stalls. --
--- Patch [11]: [RFC PATCH net-next 11/13] bnxt_en: add knod XDP offload support --- commit bc105761ac52d8c362d8a3b4da5f278514f84ca4 Author: Taehee Yoo <[email protected]> bnxt_en: add knod XDP offload support This commit introduces knod XDP offload support for the bnxt_en driver, allowing it to register as a knod NIC. It sets up the transmission of accelerator verdicts through the XDP TX ring and feeds received packets to the accelerator over the per-queue SPSC ring. > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c > b/drivers/net/ethernet/broadcom/bnxt/bnxt.c > index 7513618793daf..6ddf4bafc3d47 100644 > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c [ ... ] > @@ -2249,6 +2303,36 @@ static int bnxt_rx_pkt(struct bnxt *bp, struct > bnxt_cp_ring_info *cpr, > len = flags >> RX_CMP_LEN_SHIFT; > dma_addr = rx_buf->mapping; > > + if (BNXT_RX_OFFLOAD_MODE(bp)) { > + if (bnxt_alloc_rx_off_netmem(bp, rxr, rxr->rx_prod, > + GFP_ATOMIC)) { > + bnxt_reuse_rx_data(rxr, cons, data); > + bnapi->cp_ring.sw_stats->rx.rx_buf_errors++; > + } else { > + struct knod_work_priv *wpriv; > + struct spsc_bd *bd; > + > + if (bnapi->index >= KNOD_SPSC_MAX) { > + rc = 1; > + goto next_rx; [Severity: High] Does this path leak the current data buffer? It appears to jump to next_rx without freeing or reusing data. > + } > + > + wpriv = &bp->knodev->wpriv[bnapi->index]; > + if (spsc_produce(&wpriv->spsc_bds, (void **)&bd)) { > + bnxt_reuse_rx_data(rxr, cons, data); [Severity: High] If the accelerator SPSC ring is full and spsc_produce() fails, doesn't calling bnxt_reuse_rx_data() here permanently leak the newly allocated replacement buffer by overwriting it with the old data? > + } else { > + bd->netmem = (netmem_ref)data; > + bd->len = len; > + bd->off = bp->rx_offset; > + bd->page_idx = net_iov_binding_idx( > + netmem_to_net_iov((netmem_ref)data)); [Severity: High] Is it safe to unconditionally cast data to a net_iov here? The RX page pool allows standard pages, so if a standard page is used, could this cause type confusion and out-of-bounds reads? > + spsc_produce_commit(&wpriv->spsc_bds); > + } > + } > + rc = 1; > + goto next_rx; > + } > + > if (bnxt_xdp_attached(bp, rxr)) { [ ... ] > @@ -13356,6 +13489,7 @@ static int bnxt_open(struct net_device *dev) > BNXT_RESTART_ULP_SP_EVENT); > } > } > + bnxt_rx_offload_start(bp); [Severity: High] Is it intended to unconditionally call bnxt_rx_offload_start() here? If the preceding __bnxt_open_nic() fails, this will start the accelerator while the NIC remains down, which could leak active workers. > > return rc; > } [ ... ] > @@ -17198,6 +17336,7 @@ static int bnxt_init_one(struct pci_dev *pdev, const > struct pci_device_id *ent) > bnxt_print_device_info(bp); > > pci_save_state(pdev); > + bnxt_knod_init(bp); [Severity: High] Could this leave the driver in a corrupted state if memory allocation fails? bnxt_knod_init() can return -ENOMEM, but the error is ignored here. If it fails, bp->knetdev remains NULL, which might lead to a NULL pointer dereference during module unload in bnxt_knod_uninit(). > > return 0; > init_err_cleanup: > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c > b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c > index 9e5009be8e989..a264a2cf8302a 100644 > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c [ ... ] > @@ -441,19 +446,123 @@ static int bnxt_xdp_set(struct bnxt *bp, struct > bpf_prog *prog) > return 0; > } > > + > +static int bnxt_xdp_offload_set(struct bnxt *bp, int enable) > +{ [ ... ] > + if (enable) { > + bnxt_set_rx_skb_mode(bp, BNXT_FLAG_RX_OFFLOAD_MODE); > + xdp_features_set_redirect_target_locked(dev, true); > + } else { > + xdp_features_clear_redirect_target_locked(dev); > + bnxt_set_rx_skb_mode(bp, false); [Severity: High] Does detaching the offload accelerator silently corrupt standard XDP state? If a standard XDP program is still active (bp->xdp_prog), explicitly clearing the BNXT_FLAG_RX_PAGE_MODE flag here without restoring the correct settings for the active standard XDP program could cause packet processing failures. > + } > + bp->tx_nr_rings_xdp = tx_xdp; > + bp->tx_nr_rings = bp->tx_nr_rings_per_tc * tc + tx_xdp; > + tx_cp = bnxt_num_tx_to_cp(bp, bp->tx_nr_rings); > + bp->cp_nr_rings = max_t(int, tx_cp, bp->rx_nr_rings); > + bnxt_set_tpa_flags(bp); > + bnxt_set_ring_params(bp); > + > + if (netif_running(dev)) > + return bnxt_open_nic(bp, true, false); [Severity: High] When attaching offload to an already-running interface, is the accelerator ever started? This restarts the NIC via bnxt_open_nic(), but it seems to omit a call to bnxt_rx_offload_start(), which might leave the accelerator unable to process packets. > + > + return 0; > +} [ ... ] > +static int bnxt_rx_offload_xdp_attach(struct knod_dev *knodev) > +{ > + struct bnxt *bp = netdev_priv(knodev->netdev); > + int rc; > + > + WRITE_ONCE(bp->knodev, knodev); > + rc = bnxt_xdp_offload_set(bp, true); > + if (rc) > + WRITE_ONCE(bp->knodev, NULL); [Severity: High] Could this error handling path lead to a NULL pointer dereference? If bnxt_xdp_offload_set() sets BNXT_FLAG_RX_OFFLOAD_MODE but subsequently fails, bp->knodev is set back to NULL without clearing the offload flag. A later stats read or interface up might see the flag and dereference the NULL knodev. > + > + return rc; > +} > + > +static int bnxt_rx_offload_xdp_detach(struct knod_dev *knodev) > +{ > + struct bnxt *bp = netdev_priv(knodev->netdev); > + > + WRITE_ONCE(bp->knodev, NULL); > + bnxt_xdp_offload_set(bp, false); [Severity: High] Is it safe to set bp->knodev to NULL before shutting down the interface? Since BNXT_RX_OFFLOAD_MODE is still active until bnxt_xdp_offload_set() finishes, concurrent NAPI polling in bnxt_rx_pkt() or teardown paths like bnxt_rx_offload_clear_napi() could observe the flag and dereference the NULL pointer. > + > + return 0; > +} [ ... ] > +int bnxt_rx_offload_act_handler(struct bnxt_napi *bnapi, int budget) > +{ > + struct bnxt_tx_ring_info *txr = bnapi->tx_ring[0]; > + struct bnxt_rx_ring_info *rxr = bnapi->rx_ring; > + struct spsc_bd *bds[NAPI_POLL_WEIGHT]; > + u32 tx_avail, cnt, i, nxmit = 0; > + struct knod_dev *knodev; > + struct knod_work_priv *wpriv; > + struct napi_struct *napi; > + struct bnxt *bp = bnapi->bp; > + dma_addr_t mapping; > + > + knodev = bp->knodev; > + if (!knodev) > + return 0; > + > + if (bnapi->index >= bp->dev->real_num_tx_queues) { [Severity: High] Should this bounds check compare against KNOD_SPSC_MAX instead of the number of TX queues? If there are more RX queues than TX queues, valid RX NAPI instances might return early and abandon packets. Conversely, if there are many TX queues, this could fail to properly bound accesses to the wpriv array. > + this_cpu_inc(knodev->stats->tx_dropped); > + return 0; > + } > + > + wpriv = &knodev->wpriv[bnapi->index]; > + napi = READ_ONCE(wpriv->napi); > + if (!napi) > + return 0; > + > + tx_avail = bnxt_tx_avail(bp, txr); > + cnt = min_t(u32, NAPI_POLL_WEIGHT, tx_avail); > + cnt = min_t(u32, cnt, budget); > + if (!cnt) > + return 0; [Severity: High] Could this early return cause head-of-line blocking and permanent packet stalls? If the TX ring is full (tx_avail == 0), cnt becomes 0. Returning immediately skips knod_dev_xdp_drain_pass(), which could trap packets meant for the host stack indefinitely. > + > + spsc_release(&wpriv->spsc_bds, (void **)bds, cnt, &cnt); > + if (!cnt) > + return 0; > + > + for (i = 0; i < cnt; i++) { > + switch (bds[i]->act) { > + case KNOD_ACT_INFLIGHT: > + case KNOD_IPSEC_INFLIGHT: > + goto stop_release; > + case XDP_TX: > + mapping = > netmem_to_net_iov(bds[i]->netmem)->desc.dma_addr + > + bds[i]->off; [Severity: High] Can this path be reached when netmem is a standard page rather than a DMABUF? If so, using netmem_to_net_iov() without verifying netmem_is_net_iov() could misinterpret standard page structure fields as DMA addresses, potentially causing memory corruption. > + __bnxt_xmit_netmem(bp, txr, mapping, bds[i]->len, > + bds[i]->netmem); [ ... ] > +void bnxt_knod_uninit(struct bnxt *bp) > +{ > + knod_netdev_unregister(bp->knetdev); > + kfree(bp->knetdev); > + bp->knetdev = NULL; > + kfree(bp->knodev); [Severity: High] Is it correct to call kfree(bp->knodev) here? bp->knodev points to a struct knod_dev allocated and managed by the knod core framework, so freeing it from the driver could result in heap corruption or a use-after-free during device unbind. > + WRITE_ONCE(bp->knodev, NULL); > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=11
