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

[Paolo Abeni <pabeni@redhat.com>]

On 5/19/26 3:28 PM, Paolo Abeni wrote:
> From: AI Reviewer <ai@example.com>
> 
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://netdev-ai.bots.linux.dev/sashiko/
> ---
> net: mana: Create separate EQs for each vPort
> 
> This patch moves EQ allocation from per-device to per-vPort, exports
> mana_create_eq()/mana_destroy_eq() for use by the RDMA driver, takes
> a vport reference for RSS QPs, and adds cross-port PD-sharing
> rejection for raw QPs along with serialization between
> mana_set_channels() and the RDMA vport configuration.
> 
>> diff --git a/drivers/infiniband/hw/mana/main.c b/drivers/infiniband/hw/mana/main.c
>> index ac5e75dd3494..f8a9013f0ca3 100644
>> --- a/drivers/infiniband/hw/mana/main.c
>> +++ b/drivers/infiniband/hw/mana/main.c
>> @@ -40,13 +42,27 @@ int mana_ib_cfg_vport(struct mana_ib_dev *dev, u32 port, struct mana_ib_pd *pd,
>>  
>>  	pd->vport_use_count++;
>>  	if (pd->vport_use_count > 1) {
>> +		/* Reject cross-port PD sharing. EQs and vport config
>> +		 * are per-port, so the PD must stay bound to the port
>> +		 * that was configured on the first raw QP creation.
>> +		 */
>> +		if (pd->vport_port != port) {
>> +			pd->vport_use_count--;
>> +			mutex_unlock(&pd->vport_mutex);
>> +			ibdev_dbg(&dev->ib_dev,
>> +				  "PD already bound to port %u\n",
>> +				  pd->vport_port);
>> +			return -EINVAL;
>> +		}
>>  		ibdev_dbg(&dev->ib_dev,
>>  			  "Skip as this PD is already configured vport\n");
>>  		mutex_unlock(&pd->vport_mutex);
>>  		return 0;
>>  	}
>>  
>> -	err = mana_cfg_vport(mpc, pd->pdn, doorbell_id);
>> +	pd->vport_port = port;
>> +
>> +	err = mana_cfg_vport(mpc, pd->pdn, doorbell_id, true);
>>  	if (err) {
>>  		pd->vport_use_count--;
>>  		mutex_unlock(&pd->vport_mutex);
> 
> [Low]
> In the failure path of mana_ib_cfg_vport(), if mana_cfg_vport() or the
> later mana_create_eq() fails, vport_use_count is rolled back to 0 but
> pd->vport_port keeps the value just written.
> 
> Is the contract that pd->vport_port is only meaningful while
> vport_use_count > 0?  That invariant is not documented in the struct
> mana_ib_pd comment, and a future reader who consults pd->vport_port
> without first checking vport_use_count under vport_mutex would see stale
> data.  Would it be worth either resetting pd->vport_port on the rollback
> path or documenting the rule explicitly?
> 
>> @@ -55,15 +71,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;
>> +	}
> 
> [ ... ]
> 
>> diff --git a/drivers/infiniband/hw/mana/qp.c b/drivers/infiniband/hw/mana/qp.c
>> index 0fbcf449c134..108ec4c5ce51 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);
>> +
> 
> [Critical]
> The cross-port PD-sharing check added to mana_ib_cfg_vport() is not
> mirrored here.  mana_ib_create_qp_rss() takes the user-supplied port
> from ucmd.port and only checks that vport_use_count is non-zero, so the
> RSS path will happily attach to a different port than the one bound to
> the PD by the first raw QP.
> 
> If a user creates a raw QP on port A (which sets pd->vport_port=A and
> allocates mpc_A->eqs via mana_create_eq()), then creates an RSS QP on
> the same PD with ucmd.port=B where port B has no Ethernet up and no
> RDMA activity, mpc_B->eqs is NULL.  The subsequent code in this same
> function:
> 
> 	eq = &mpc->eqs[cq->comp_vector % mpc->num_queues];
> 	cq_spec.attached_eq = eq->eq->id;
> 
> would then dereference NULL through mpc_B->eqs.
> 
> If port B does have Ethernet up, mpc_B->eqs is owned by the Ethernet
> driver and the RSS QP attaches to those EQs.  When the QP is destroyed
> mana_ib_destroy_qp_rss() calls mana_ib_uncfg_vport(mdev, pd, qp->port=B),
> and once pd->vport_use_count reaches 0 mana_ib_uncfg_vport() will run
> mana_destroy_eq(mpc_B) on Ethernet's live EQs and call mana_uncfg_vport
> on a port whose apc->vport_use_count was never incremented by this PD,
> tripping the WARN_ON in mana_uncfg_vport() and corrupting Ethernet's
> vport state.  Meanwhile port A's EQs and vport configuration are leaked
> because nothing on this PD will destroy them.
> 
> Should mana_ib_create_qp_rss() apply the same pd->vport_port == port
> check that mana_ib_cfg_vport() now performs, before bumping
> vport_use_count?

Sashiko reported alredy this problematic pattern in 2 separate places.
Please ensure that there are no other similar buggy usage pattern
elsewhere in the newly introduced code.

/P