Re: [PATCH net v2 1/2] octeontx2-pf: Fix aura BPID assignment when CONFIG_DCB is enabled

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
octeontx2-pf: Fix aura BPID assignment when CONFIG_DCB is enabled

This adds a NULL check on pfvf->queue_to_pfc_map before it is dereferenced
in the aura and CQ context init paths, since SDP VFs never allocate the map.
The BPID is now set to bpid[0] first and only overridden when CONFIG_DCB is
enabled and the map exists.

No Link: tags in the commit header.

> Fixes: 184fb40f731b ("octeontx2-pf: Avoid adding dcbnl_ops for LBK and SDP vf").

[Severity: Low]
This isn't a bug, but there is a trailing period after the closing
parenthesis of the Fixes tag.  The expected form is:

Fixes: 184fb40f731b ("octeontx2-pf: Avoid adding dcbnl_ops for LBK and SDP vf")

The stray '.' can confuse the Fixes tag parsers used for stable backport
selection.  Since this is a regression fix, would a [PATCH net] annotation
on the subject also be appropriate?

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/cn20k.c b/drivers/net/ethernet/marvell/octeontx2/nic/cn20k.c
> index dbf1731966082..f3903cce9bf78 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/cn20k.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/cn20k.c
> @@ -245,10 +245,12 @@ int cn20k_register_pfvf_mbox_intr(struct otx2_nic *pf, int numvfs)
>  static u8 cn20k_aura_bpid_idx(struct otx2_nic *pfvf, int aura_id)
>  {
>  #ifdef CONFIG_DCB
> -	return pfvf->queue_to_pfc_map[aura_id];
> -#else
> -	return 0;
> +	if (pfvf->queue_to_pfc_map)
> +		return pfvf->queue_to_pfc_map[aura_id];
> +	else
> +		return 0;
>  #endif
> +	return 0;
>  }

[Severity: Low]
Is the else branch here needed?  With CONFIG_DCB=y both branches return, so
the trailing return 0 after #endif is unreachable, and cn20k_aura_bpid_idx()
does not follow the "default first, then conditionally override" shape the
commit message describes for the other two hunks.  Would this be equivalent
and match the rest of the patch?

	static u8 cn20k_aura_bpid_idx(struct otx2_nic *pfvf, int aura_id)
	{
	#ifdef CONFIG_DCB
		if (pfvf->queue_to_pfc_map)
			return pfvf->queue_to_pfc_map[aura_id];
	#endif
		return 0;
	}

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> index 3d253132a17f0..b4691472d2a3e 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> @@ -1132,10 +1132,10 @@ int otx2_cq_init(struct otx2_nic *pfvf, u16 qidx)
>  		if (!is_otx2_lbkvf(pfvf->pdev)) {
>  			/* Enable receive CQ backpressure */
>  			aq->cq.bp_ena = 1;
> -#ifdef CONFIG_DCB
> -			aq->cq.bpid = pfvf->bpid[pfvf->queue_to_pfc_map[qidx]];
> -#else
>  			aq->cq.bpid = pfvf->bpid[0];
> +#ifdef CONFIG_DCB
> +			if (pfvf->queue_to_pfc_map)
> +				aq->cq.bpid = pfvf->bpid[pfvf->queue_to_pfc_map[qidx]];
>  #endif

[Severity: Low]
This isn't a bug, but the same #ifdef CONFIG_DCB plus NULL test idiom now
appears in three places: cn20k_aura_bpid_idx(), otx2_cq_init() and
otx2_aura_aq_init().  cn20k_aura_bpid_idx() already encapsulates exactly this
logic.  Would it be cleaner to generalize it into a single accessor, say
otx2_get_bpid_idx(pfvf, qidx), and use it from all three sites?

Alternatively, could queue_to_pfc_map simply be allocated unconditionally
(it is only pfvf->hw.rx_queues bytes) instead of being tied to dcbnl_ops
registration in otx2_dcbnl_set_ops()?  That coupling is what broke in
184fb40f731b.

Note that otx2_update_bpid_in_rqctx() in otx2_dcbnl.c still dereferences the
map with no NULL check:

	if (pfvf->queue_to_pfc_map[qidx] && pfc_enable) {
	...
	pfvf->queue_to_pfc_map[qidx] = vlan_prio;

That stays safe only because SDP/LBK VFs never register dcbnl_ops, so
pfvf->pfc_en remains 0 and the PFC ntuple path is never taken for them.  Is
that invariant worth documenting, or would the shared accessor make it moot?

> @@ -1433,10 +1433,11 @@ int otx2_aura_aq_init(struct otx2_nic *pfvf, int aura_id,
>  		 */
>  		if (pfvf->nix_blkaddr == BLKADDR_NIX1)
>  			aq->aura.bp_ena = 1;
> -#ifdef CONFIG_DCB
> -		aq->aura.nix0_bpid = pfvf->bpid[pfvf->queue_to_pfc_map[aura_id]];
> -#else
> +
>  		aq->aura.nix0_bpid = pfvf->bpid[0];
> +#ifdef CONFIG_DCB
> +		if (pfvf->queue_to_pfc_map)
> +			aq->aura.nix0_bpid = pfvf->bpid[pfvf->queue_to_pfc_map[aura_id]];
>  #endif

[Severity: High]
This isn't a bug introduced by this patch, but since the patch is hardening
accesses to queue_to_pfc_map, is it worth fixing the bounds problem on the
same array at the same time?

The map is allocated with only pfvf->hw.rx_queues bytes in
otx2_dcbnl_set_ops():

	pfvf->queue_to_pfc_map = devm_kzalloc(pfvf->dev, pfvf->hw.rx_queues,
					      GFP_KERNEL);

and otx2_update_bpid_in_rqctx() in otx2_dcbnl.c indexes it with a queue
index that comes from the ethtool ntuple ring_cookie:

	if (pfvf->queue_to_pfc_map[qidx] && pfc_enable) {
	...
	pfvf->queue_to_pfc_map[qidx] = vlan_prio;

Can qidx exceed hw.rx_queues here?  The range check in otx2_add_flow() is
gated on the interface not being a VF:

	if (!is_otx2_vf(pfvf->pcifunc) && !vf_num &&
	    ring >= pfvf->hw.rx_queues && fsp->ring_cookie != RX_CLS_FLOW_DISC)
		return -EINVAL;

so for a VF netdev the ring index is not validated, and otx2_add_flow_msg()
propagates it:

	req->index = ethtool_get_flow_spec_ring(ring_cookie);
	...
			if (pfvf->pfc_en & BIT(vlan_prio)) {
				pfc_rule = true;
				qidx = req->index;
			}
	...
	if (!err && pfc_rule)
		otx2_update_bpid_in_rqctx(pfvf, vlan_prio, qidx, true);

The AF handler in rvu_npc_fs.c assigns action.index = req->index without an
RQ count check, so the mbox call succeeds and the helper runs.  That looks
like an out-of-bounds read at a caller-chosen offset plus a one byte
out-of-bounds write of the VLAN priority.  The delete path in
otx2_remove_flow() passes the raw 64-bit ring_cookie into the same int qidx:

	otx2_update_bpid_in_rqctx(pfvf, 0,
				  flow->flow_spec.ring_cookie,
				  false);

Would adding the ring >= pfvf->hw.rx_queues check for VF netdevs, or bounds
checking qidx inside otx2_update_bpid_in_rqctx(), be the right fix?  This
requires CAP_NET_ADMIN on the VF netdev, which matters when the VF is handed
to a container or guest owner.
-- 
pw-bot: cr