[PATCH net] octeontx2-af: nix: Fix TX vtag upper bits programming

[PATCH net] octeontx2-af: nix: Fix TX vtag upper bits programming

[Ratheesh Kannoth]

From: Subbaraya Sundeep <sbhatta@marvell.com>

The upper bits of the vtag value above bit 47 were not written to
NIX_AF_TX_VTAG_DEFX_DATA, causing incorrect TX VLAN tag insertion
when extended vtag fields are used.

Fixes: 9a946def264d ("octeontx2-af: Modify nix_vtag_cfg mailbox to support TX VTAG entries")
Signed-off-by: Subbaraya Sundeep <sbhatta@marvell.com>
Signed-off-by: Ratheesh Kannoth <rkannoth@marvell.com>
---
 drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
index 153eb57bad06..091ef8dac7c9 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
+++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
@@ -3107,6 +3107,7 @@ static int nix_tx_vtag_alloc(struct rvu *rvu, int blkaddr,
 	mutex_unlock(&vlan->rsrc_lock);
 
 	regval = size ? vtag : vtag << 32;
+	regval |= (vtag & ~GENMASK_ULL(47, 0)) << 48;
 
 	rvu_write64(rvu, blkaddr,
 		    NIX_AF_TX_VTAG_DEFX_DATA(index), regval);
-- 
2.43.0

Re: [PATCH net] octeontx2-af: nix: Fix TX vtag upper bits programming

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 1 (2) · Medium: 0 · Low: 0

- [High] The statement added by this patch in nix_tx_vtag_alloc()
  (drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c:3110), `regval |=…

Pre-existing issues:
- [High] In the same TX-VTAG mailbox path this patch modifies, the error
  path of nix_tx_vtag_cfg() (rvu_nix.c:3191-3193) calls…
- [High] In the same TX-VTAG mailbox path this patch touches, the free
  path uses a fully caller-controlled signed index from the mailbox…

--

octeontx2-af: nix: Fix TX vtag upper bits programming

This adds one statement to nix_tx_vtag_alloc() so that the vtag bits above
bit 47 also end up in NIX_AF_TX_VTAG_DEFX_DATA, with the aim of fixing TX
VLAN tag insertion when the extended vtag fields are used.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> index 153eb57bad067..091ef8dac7c94 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> @@ -3107,6 +3107,7 @@ static int nix_tx_vtag_alloc(struct rvu *rvu, int blkaddr,
>  	mutex_unlock(&vlan->rsrc_lock);
>  
>  	regval = size ? vtag : vtag << 32;
> +	regval |= (vtag & ~GENMASK_ULL(47, 0)) << 48;

[Severity: High]
Is this new term always zero?

~GENMASK_ULL(47, 0) is 0xffff000000000000, so the mask keeps bits 63:48
where they already are, and the following << 48 shifts them out of the
64-bit word:

	regval |= (vtag & 0xffff000000000000ULL) << 48;   /* == 0 */

If that is right, the value handed to rvu_write64() below is bit for bit
the same as before the patch, and the condition described in the changelog
("The upper bits of the vtag value above bit 47 were not written to
NIX_AF_TX_VTAG_DEFX_DATA") is still present.

The in-tree producer of those upper bits is rvu_rep_tx_vlan_cfg() in
rvu_rep.c, which packs the EtherType at vtag0[63:48] and asks for
VTAGSIZE_T4:

	req.vtag_size = VTAGSIZE_T4;
	req.cfg_type = 0; /* tx vlan cfg */
	req.tx.cfg_vtag0 = true;
	req.tx.vtag0 = FIELD_PREP(NIX_VLAN_ETYPE_MASK, etype) | vlan_tci;

with NIX_VLAN_ETYPE_MASK defined in rvu_reg.h as GENMASK_ULL(63, 48).
Since the field is already at its final register position, and since
size == 0 makes nix_tx_vtag_alloc() compute regval = vtag << 32 and drop
everything above bit 31, doesn't the representor path still program
EtherType 0 while the NIX_VTAG_CFG mailbox returns success?

Would something like this be the intended expression instead?

	regval |= vtag & NIX_VLAN_ETYPE_MASK;

>  
>  	rvu_write64(rvu, blkaddr,
>  		    NIX_AF_TX_VTAG_DEFX_DATA(index), regval);

[Severity: High]
This isn't a bug introduced by this patch, but while looking at the same
TX VTAG mailbox path: is the error path of nix_tx_vtag_cfg() missing
vlan->rsrc_lock?

	err_free:
		if (req->tx.cfg_vtag0)
			nix_tx_vtag_free(rvu, blkaddr, pcifunc, rsp->vtag0_idx);

Every other caller holds the mutex around it, for example
nix_tx_vtag_decfg():

	mutex_lock(&vlan->rsrc_lock);

	if (req->tx.free_vtag0) {
		err = nix_tx_vtag_free(rvu, blkaddr, pcifunc, idx0);

and nix_free_tx_vtag_entries() does the same.

nix_tx_vtag_free() ends in rvu_free_rsrc(), which is a non-atomic
read-modify-write:

	void rvu_free_rsrc(struct rsrc_bmap *rsrc, int id)
	{
		if (!rsrc->bmap)
			return;

		__clear_bit(id, rsrc->bmap);
	}

and rvu_alloc_rsrc() pairs find_first_zero_bit() with __set_bit() on the
same word. AF mailbox work items for different PF/VFs can run at the same
time (rvu_mbox_init() allocates the workqueue with max_active == num), so
can this interleaving lose an allocation?

CPU0 (nix_tx_vtag_cfg err_free, no lock)
	reads bitmap word

CPU1 (nix_tx_vtag_alloc, holds rsrc_lock)
	__set_bit(index, bmap)

CPU0
	writes back the stale word, clearing CPU1's bit

The same index could then be handed out twice, letting two PF/VFs share one
NIX_AF_TX_VTAG_DEFX_DATA/CTL entry, or the entry could be leaked in the
opposite interleaving. The ownership store
vlan->entry2pfvf_map[rsp->vtag0_idx] = pcifunc also happens outside the
lock, i.e. not atomically with the bitmap allocation done under it.

[Severity: High]
This is a pre-existing issue too, but in the deconfiguration side of the
same path: are the mailbox-supplied indices ever range checked?

nix_tx_vtag_decfg() takes them straight from the request, as signed int
fields of struct nix_vtag_config, and dereferences the map with them:

	int idx0 = req->tx.vtag0_idx;
	int idx1 = req->tx.vtag1_idx;
	...
	if (req->tx.free_vtag0 && req->tx.free_vtag1)
		if (vlan->entry2pfvf_map[idx0] != pcifunc ||
		    vlan->entry2pfvf_map[idx1] != pcifunc)
			return NIX_AF_ERR_PARAM;

Note that comparison only runs when both free flags are set. With just
free_vtag0 set, the index reaches nix_tx_vtag_free() unchecked:

	vlan = &nix_hw->txvlan;
	if (vlan->entry2pfvf_map[index] != pcifunc)
		return NIX_AF_ERR_PARAM;

entry2pfvf_map is a devm_kcalloc'd array of vlan->rsrc.max entries
(NIX_TX_VTAG_DEF_MAX is 0x400), so does this read out of bounds for a
negative or too-large index? And if the sampled u16 happens to equal the
sender's pcifunc, execution continues to:

	rvu_write64(rvu, blkaddr,
		    NIX_AF_TX_VTAG_DEFX_DATA(index), 0x0ull);
	rvu_write64(rvu, blkaddr,
		    NIX_AF_TX_VTAG_DEFX_CTL(index), 0x0ull);

	vlan->entry2pfvf_map[index] = 0;
	rvu_free_rsrc(&vlan->rsrc, index);

where the register offset is 0x1A10 | (index) << 16, the map store is an
out-of-bounds write, and rvu_free_rsrc() does __clear_bit() outside the
bitmap.

rvu_mbox_handler_nix_vtag_cfg() only validates the flag combination before
dispatching:

		if (req->tx.free_vtag0 || req->tx.free_vtag1)
			return nix_tx_vtag_decfg(rvu, blkaddr, req);

Should nix_tx_vtag_decfg() reject indices outside [0, vlan->rsrc.max)
before using them?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915073058.992915-1-rkannoth%40marvell.com