Re: [PATCH net-next v9 1/6] net: mana: Create separate EQs for each vPort

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Race condition in `mana_set_channels()` allows an RDMA application to steal vport ownership during an Ethernet reconfiguration, leading to out-of-bounds memory accesses and interface breakage.
- [Critical] Per-PD reference counting for per-port EQs causes a scope mismatch, leading to NULL pointer dereferences and resource leaks when a Protection Domain is shared across multiple ports.
--

commit 31a11c151b6c97fb0cd32cd8787a2fa1cd92856b
Author: Long Li <longli@microsoft.com>
Subject: net: mana: Create separate EQs for each vPort

To prepare for assigning vPorts to dedicated MSI-X vectors, remove EQ
sharing among the vPorts and create dedicated EQs for each vPort.

> diff --git a/drivers/infiniband/hw/mana/main.c b/drivers/infiniband/hw/mana/main.c
> index ac5e75dd3494d..6159bd03a0217 100644
> --- a/drivers/infiniband/hw/mana/main.c
> +++ b/drivers/infiniband/hw/mana/main.c
[ ... ]
> @@ -55,15 +57,23 @@ int mana_ib_cfg_vport(struct mana_ib_dev *dev, u32 port, struct mana_ib_pd *pd,
>  		return err;
>  	}
>  
> -	mutex_unlock(&pd->vport_mutex);
>  
> -	pd->tx_shortform_allowed = mpc->tx_shortform_allowed;
> -	pd->tx_vp_offset = mpc->tx_vp_offset;
> +	err = mana_create_eq(mpc);
> +	if (err) {
> +		mana_uncfg_vport(mpc);
> +		pd->vport_use_count--;
> +	} else {
> +		pd->tx_shortform_allowed = mpc->tx_shortform_allowed;
> +		pd->tx_vp_offset = mpc->tx_vp_offset;
> +	}
>  
> -	ibdev_dbg(&dev->ib_dev, "vport handle %llx pdid %x doorbell_id %x\n",
> -		  mpc->port_handle, pd->pdn, doorbell_id);
> +	mutex_unlock(&pd->vport_mutex);

Could this lead to a NULL pointer dereference or resource leak if a Protection
Domain is shared across multiple ports?

The vport_use_count is tracked per-PD (pd->vport_use_count), but Event Queues
are now managed per-port (mpc->eqs).

If a user creates a QP on Port 1, pd->vport_use_count becomes 1 and mpc1->eqs
are allocated. If the user then creates a QP on Port 2 using the same PD, the
code in mana_ib_cfg_vport() will see pd->vport_use_count > 1 and bypass
configuration for Port 2.

This leaves mpc2->eqs as NULL if the Ethernet interface is down, leading to a
NULL dereference when EQs are accessed. When QPs are later destroyed, it
will also destroy the context of whichever port the last QP belonged to,
permanently leaking the hardware configuration and EQs of the port that was
actually initialized.

[ ... ]
> diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> index 04350973e19e3..e121834d17f3a 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> @@ -454,18 +454,40 @@ static int mana_set_coalesce(struct net_device *ndev,
>  	return err;
>  }
>  
> +/* mana_set_channels - change the number of queues on a port
> + *
> + * Returns -EBUSY if the port is down and RDMA holds the vport with
> + * EQs sized to the current num_queues.
> + */
>  static int mana_set_channels(struct net_device *ndev,
>  			     struct ethtool_channels *channels)
>  {
>  	struct mana_port_context *apc = netdev_priv(ndev);
>  	unsigned int new_count = channels->combined_count;
>  	unsigned int old_count = apc->num_queues;
> +	bool locked = false;
>  	int err;
>  
> +	/* When the port is down, hold vport_mutex for the entire
> +	 * operation to serialize against RDMA's mana_cfg_vport().
> +	 * This is safe because mana_detach()/mana_attach() skip
> +	 * vport teardown/setup when port_st_save is false.
> +	 * When the port is up, Ethernet owns the vport exclusively
> +	 * so no locking against RDMA is needed.
> +	 */
> +	if (!apc->port_is_up) {
> +		mutex_lock(&apc->vport_mutex);
> +		if (apc->vport_use_count) {
> +			mutex_unlock(&apc->vport_mutex);
> +			return -EBUSY;
> +		}
> +		locked = true;
> +	}
>  
>  	err = mana_pre_alloc_rxbufs(apc, ndev->mtu, new_count);
>  	if (err) {
>  		netdev_err(ndev, "Insufficient memory for new allocations");
> -		return err;
> +		goto unlock;
>  	}
>  
>  	err = mana_detach(ndev, false);

Can this lead to a race condition with RDMA operations when the port is up?

If apc->port_is_up is true, the function does not acquire apc->vport_mutex.
It proceeds to call mana_detach(), which calls mana_uncfg_vport() and lowers
apc->vport_use_count to 0.

Because no lock is held across the detach/attach window, a concurrent RDMA
operation like mana_ib_create_qp_raw() could grab apc->vport_mutex, observe
a use count of 0, and steal ownership of the vport.

If the RDMA thread allocates apc->eqs based on old_count, and then
mana_set_channels() updates apc->num_queues = new_count, subsequent RDMA
queue indexing operations may read out of bounds. The Ethernet interface will
also fail to reattach because RDMA now owns the vport, leaving the interface
broken.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513220956.402058-1-longli@microsoft.com?part=1