Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Critical] Missing cross-port validation in `mana_ib_create_qp_rss()` allows userspace to trigger NULL pointer dereferences and corrupt other ports' queues. - [High] Use of boolean flag `channel_changing` as an ad-hoc lock in `mana_set_channels()` violates networking subsystem guidelines. --
commit ed2a523d0427d1b949add95925cad26c8b468ff8 Author: Long Li <[email protected]> Date: Fri May 15 04:05:03 2026 +0000 net: mana: Create separate EQs for each vPort This commit separates EQs for each vPort, removing the shared EQ mechanism from the MANA driver to prepare for dedicated MSI-X vector assignment. RSS QPs are updated to take a vport reference, ensuring EQs outlive QP consumers. > diff --git a/drivers/infiniband/hw/mana/qp.c b/drivers/infiniband/hw/mana/qp.c > index 0fbcf449c134b..108ec4c5ce51c 100644 > --- a/drivers/infiniband/hw/mana/qp.c > +++ b/drivers/infiniband/hw/mana/qp.c [ ... ] > @@ -155,6 +156,18 @@ static int mana_ib_create_qp_rss(struct ib_qp *ibqp, > struct ib_pd *pd, > > qp->port = port; > > + /* Take a reference on the vport to ensure EQs outlive this QP. > + * The vport must already be configured by a raw QP. > + */ > + mutex_lock(&mana_pd->vport_mutex); > + if (!mana_pd->vport_use_count) { > + mutex_unlock(&mana_pd->vport_mutex); > + ret = -EINVAL; > + goto fail; > + } > + mana_pd->vport_use_count++; > + mutex_unlock(&mana_pd->vport_mutex); > + Does this code need to verify that the requested port matches mana_pd->vport_port? While mana_ib_cfg_vport() correctly rejects cross-port PD sharing for raw QPs, an RSS QP creation could specify a different port (e.g., Port B) using the same PD bound to Port A. If Port B is unconfigured, could fetching its mpc result in a NULL pointer dereference when accessing mpc->eqs below? If Port B is active, could this lead to erroneously destroying Port B's EQs or underflowing its vport_use_count when the PD drops its references during teardown? > for (i = 0; i < ind_tbl_size; i++) { > struct mana_obj_spec wq_spec = {}; [ ... ] > diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c > b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c > index 04350973e19e3..4633acc976f0a 100644 > --- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c > +++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c [ ... ] > @@ -462,10 +467,22 @@ static int mana_set_channels(struct net_device *ndev, > unsigned int old_count = apc->num_queues; > int err; > > + /* Set channel_changing to block RDMA from grabbing the vport > + * during the detach/attach window. mana_cfg_vport() checks > + * this flag under vport_mutex and returns -EBUSY if set. > + */ > + mutex_lock(&apc->vport_mutex); > + if (!apc->port_is_up && apc->vport_use_count) { > + mutex_unlock(&apc->vport_mutex); > + return -EBUSY; > + } > + apc->channel_changing = true; > + mutex_unlock(&apc->vport_mutex); > + Is channel_changing acting as an ad-hoc lock here? The networking subsystem guidelines discourage using boolean flags set on entry and cleared on exit to guard code sections against concurrent access. Since mana_detach() and mana_attach() are executed while this flag is held to prevent mana_cfg_vport() from proceeding, could this be replaced with a standard locking mechanism to ensure lockdep visibility? > err = mana_pre_alloc_rxbufs(apc, ndev->mtu, new_count); > if (err) { > netdev_err(ndev, "Insufficient memory for new allocations"); > - return err; > + goto clear_flag; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
