mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH net v3] octeontx2-af: Fix BPID leak in nix_bp_enable()
@ 2026-09-28  6:57 nshettyj
  2026-09-28  6:59 ` netdev-bot+sinfo
  2026-10-01  9:57 ` netdev-bot+sashiko
  0 siblings, 2 replies; 3+ messages in thread
From: nshettyj @ 2026-09-28  6:57 UTC (permalink / raw)
  To: netdev, linux-kernel
  Cc: Rakesh Kudurumalla, Nitin Shetty J, Sunil Goutham,
	Ratheesh Kannoth, Geetha sowjanya, Subbaraya Sundeep,
	Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Simon Horman

From: Rakesh Kudurumalla <rkudurumalla@marvell.com>

For LBK interfaces, rvu_nix_get_bpid() allocates a BPID from the free
pool on every call. nix_bp_enable() called it unconditionally before
the loop, and again after the last channel was programmed, leaking a
BPID whenever that extra call's result went unused. With
req->chan_cnt == 0, the pre-loop call leaked a BPID on every call.

Move the allocation into the loop body so it runs exactly once per
channel actually programmed, and reject req->chan_cnt == 0 upfront.

Fixes: d6212d2e41a0 ("octeontx2-af: Create BPIDs free pool")
Signed-off-by: Nitin Shetty J <nshettyj@marvell.com>
Signed-off-by: Rakesh Kudurumalla <rkudurumalla@marvell.com>
---
changes in v3:
- Unwind BPID allocations and disable programmed channels on mid-loop failure in `nix_bp_enable()`.
- Report the actual BPID written per channel instead of reconstructing it arithmetically.
- Validate the BPID range in `nix_bp_disable()` before freeing it, preventing an out-of-bounds write.

changes in v2:
- Move the rvu_nix_get_bpid() call for LBK BPID allocation from before
  the loop into the loop body.
- Validate req->chan_cnt before allocating BPIDs.
- updated commit message and fix tag
---
 .../ethernet/marvell/octeontx2/af/rvu_nix.c   | 79 ++++++++++++++++---
 1 file changed, 66 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
index 153eb57bad06..1ef505009c3a 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
+++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
@@ -631,6 +631,15 @@ static int nix_bp_disable(struct rvu *rvu,
 
 		if (type == NIX_INTF_TYPE_LBK) {
 			bpid = cfg & GENMASK(8, 0);
+			/* Ignore channels that were never armed with an LBK
+			 * free-pool bpid (e.g. never enabled, or belonging
+			 * to a CGX/SDP range) - bpid - free_pool_base would
+			 * underflow and corrupt an unrelated bitmap word.
+			 */
+			if (bpid < bp->free_pool_base ||
+			    bpid >= bp->free_pool_base + bp->bpids.max)
+				continue;
+
 			mutex_lock(&rvu->rsrc_lock);
 			rvu_free_rsrc(&bp->bpids, bpid - bp->free_pool_base);
 			for (bpid = 0; bpid < bp->bpids.max; bpid++) {
@@ -738,6 +747,35 @@ static int rvu_nix_get_bpid(struct rvu *rvu, struct nix_bp_cfg_req *req,
 	return bpid;
 }
 
+static void nix_bp_enable_unwind(struct rvu *rvu, struct nix_bp *bp,
+				 int blkaddr, u16 chan_base, int chan_cnt,
+				 int type, bool cpt_link)
+{
+	u16 chan, chan_v, bpid;
+	u64 cfg;
+
+	for (chan = chan_base; chan < chan_base + chan_cnt; chan++) {
+		chan_v = nix_get_channel(chan, cpt_link);
+		cfg = rvu_read64(rvu, blkaddr, NIX_AF_RX_CHANX_CFG(chan_v));
+		rvu_write64(rvu, blkaddr, NIX_AF_RX_CHANX_CFG(chan_v),
+			    cfg & ~BIT_ULL(16));
+
+		if (type != NIX_INTF_TYPE_LBK)
+			continue;
+
+		bpid = cfg & GENMASK_ULL(8, 0);
+		if (bpid < bp->free_pool_base ||
+		    bpid >= bp->free_pool_base + bp->bpids.max)
+			continue;
+
+		mutex_lock(&rvu->rsrc_lock);
+		rvu_free_rsrc(&bp->bpids, bpid - bp->free_pool_base);
+		bp->fn_map[bpid - bp->free_pool_base] = 0;
+		bp->ref_cnt[bpid - bp->free_pool_base] = 0;
+		mutex_unlock(&rvu->rsrc_lock);
+	}
+}
+
 static int nix_bp_enable(struct rvu *rvu,
 			 struct nix_bp_cfg_req *req,
 			 struct nix_bp_cfg_rsp *rsp,
@@ -747,9 +785,12 @@ static int nix_bp_enable(struct rvu *rvu,
 	u16 pcifunc = req->hdr.pcifunc;
 	struct rvu_pfvf *pfvf;
 	u16 chan_base, chan;
-	s16 bpid, bpid_base;
+	struct nix_hw *nix_hw;
+	struct nix_bp *bp;
 	u16 chan_v;
+	s16 bpid;
 	u64 cfg;
+	int err;
 
 	pf = rvu_get_pf(rvu->pdev, pcifunc);
 	type = is_lbk_vf(rvu, pcifunc) ? NIX_INTF_TYPE_LBK : NIX_INTF_TYPE_CGX;
@@ -764,16 +805,27 @@ static int nix_bp_enable(struct rvu *rvu,
 	if (cpt_link && !rvu->hw->cpt_links)
 		return 0;
 
+	if (!req->chan_cnt)
+		return NIX_AF_ERR_INVALID_BPID_REQ;
+
 	pfvf = rvu_get_pfvf(rvu, pcifunc);
-	blkaddr = rvu_get_blkaddr(rvu, BLKTYPE_NIX, pcifunc);
+	err = nix_get_struct_ptrs(rvu, pcifunc, &nix_hw, &blkaddr);
+	if (err)
+		return err;
 
-	bpid_base = rvu_nix_get_bpid(rvu, req, type, chan_id);
+	bp = &nix_hw->bp;
 	chan_base = pfvf->rx_chan_base + req->chan_base;
-	bpid = bpid_base;
 
 	for (chan = chan_base; chan < (chan_base + req->chan_cnt); chan++) {
+		bpid = rvu_nix_get_bpid(rvu, req, type, chan_id);
 		if (bpid < 0) {
 			dev_warn(rvu->dev, "Fail to enable backpressure\n");
+			/* Undo the channels already enabled/allocated for
+			 * this request so their BPIDs and armed channels
+			 * don't leak until an FLR.
+			 */
+			nix_bp_enable_unwind(rvu, bp, blkaddr, chan_base,
+					     chan_id, type, cpt_link);
 			return -EINVAL;
 		}
 
@@ -783,16 +835,17 @@ static int nix_bp_enable(struct rvu *rvu,
 		cfg &= ~GENMASK_ULL(8, 0);
 		rvu_write64(rvu, blkaddr, NIX_AF_RX_CHANX_CFG(chan_v),
 			    cfg | (bpid & GENMASK_ULL(8, 0)) | BIT_ULL(16));
-		chan_id++;
-		bpid = rvu_nix_get_bpid(rvu, req, type, chan_id);
-	}
 
-	for (chan = 0; chan < req->chan_cnt; chan++) {
-		/* Map channel and bpid assign to it */
-		rsp->chan_bpid[chan] = ((req->chan_base + chan) & 0x7F) << 10 |
-					(bpid_base & 0x3FF);
-		if (req->bpid_per_chan)
-			bpid_base++;
+		/* Report the bpid actually programmed for this channel,
+		 * instead of reconstructing it arithmetically from the
+		 * first channel's bpid. For LBK, each call above can
+		 * return a non-contiguous bpid from the shared free pool,
+		 * so that reconstruction can diverge from what's actually
+		 * written into NIX_AF_RX_CHANX_CFG.
+		 */
+		rsp->chan_bpid[chan_id] = ((req->chan_base + chan_id) & 0x7F) << 10 |
+					  (bpid & 0x3FF);
+		chan_id++;
 	}
 	rsp->chan_cnt = req->chan_cnt;
 
-- 
2.48.1


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH net v3] octeontx2-af: Fix BPID leak in nix_bp_enable()
  2026-09-28  6:57 [PATCH net v3] octeontx2-af: Fix BPID leak in nix_bp_enable() nshettyj
@ 2026-09-28  6:59 ` netdev-bot+sinfo
  2026-10-01  9:57 ` netdev-bot+sashiko
  1 sibling, 0 replies; 3+ messages in thread
From: netdev-bot+sinfo @ 2026-09-28  6:59 UTC (permalink / raw)
  To: nshettyj
  Cc: netdev, linux-kernel, Rakesh Kudurumalla, Sunil Goutham,
	Ratheesh Kannoth, Geetha sowjanya, Subbaraya Sundeep,
	Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Simon Horman

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH net v3] octeontx2-af: Fix BPID leak in nix_bp_enable()
  2026-09-28  6:57 [PATCH net v3] octeontx2-af: Fix BPID leak in nix_bp_enable() nshettyj
  2026-09-28  6:59 ` netdev-bot+sinfo
@ 2026-10-01  9:57 ` netdev-bot+sashiko
  1 sibling, 0 replies; 3+ messages in thread
From: netdev-bot+sashiko @ 2026-10-01  9:57 UTC (permalink / raw)
  To: nshettyj
  Cc: netdev, linux-kernel, rkudurumalla, sgoutham, rkannoth, gakula,
	sbhatta, andrew+netdev, davem, edumazet, kuba, pabeni, horms

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] BPIDs can still leak permanently after this patch, so the fix is
  incomplete.
- [High] After a BPID is freed, NIX_AF_RX_CHANX_CFG still holds it, so a
  later disable can free it a second time.
- [High] A malformed LBK request can now hang the AF mailbox worker
  forever in the new nix_bp_enable_unwind().
- [Medium] The commit message leaves out the most important fix in the
  patch, plus two behaviour changes.
- [Medium] nix_bp_enable_unwind() works out which BPIDs to release by
  reading NIX_AF_RX_CHANX_CFG outside rvu->rsrc_lock, instead of…
- [Medium] When an enable fails partway, the new nix_bp_enable_unwind()
  turns backpressure off on channels that already had it before the…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is serious.
- [Medium] For LBK, rvu_nix_get_bpid() allocates a new BPID from the free
  pool on every call and ignores req->bpid_per_chan.
- [Medium] The response encoding in nix_bp_enable() loses the top channel
  bit.
- [Medium] nix_bp_disable() frees the BPID found in the target channel's
  CFG register without checking bp->fn_map[bpid - free_pool_base] ==…

--

octeontx2-af: Fix BPID leak in nix_bp_enable()

This patch moves the LBK BPID allocation in nix_bp_enable() into the
per-channel loop, so no extra BPID is allocated after the last channel. It
also rejects requests with chan_cnt == 0. It adds a range check before
freeing in nix_bp_disable(), a new nix_bp_enable_unwind() helper for
mid-loop failures, and reports each channel's programmed BPID in the
mailbox response.

> Move the allocation into the loop body so it runs exactly once per
> channel actually programmed, and reject req->chan_cnt == 0 upfront.

[Severity: Medium]
The commit message only covers the extra rvu_nix_get_bpid() call and the
chan_cnt == 0 check. Should it also describe the other three changes?

The first is the new range check in nix_bp_disable(). Before this patch, a
bpid below free_pool_base passed a negative id to rvu_free_rsrc(), and
__clear_bit() wrote before the start of bp->bpids.bmap. On silicon with
cpt_links this looks reachable on every open of an LBK VF. The never-armed
CPT channel has a BPID field below free_pool_base, and the open path always
sends the CPT disable:

otx2_init_hw_resources()
    otx2_nix_cpt_config_bp(pf, false);

The second is the new nix_bp_enable_unwind() rollback helper.

The third is the change in what rsp->chan_bpid[] reports for LBK. Each
channel now gets its own BPID instead of bpid_base.

These changes appear only in the v3 changelog below the --- line, which is
dropped when the patch is applied. The subject and the Fixes: tag say
nothing about the out-of-bounds write. Could the commit message describe
that fix so stable backporters can see it?

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> index 153eb57bad06..1ef505009c3a 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> @@ -631,6 +631,15 @@ static int nix_bp_disable(struct rvu *rvu,
>  
>  		if (type == NIX_INTF_TYPE_LBK) {
>  			bpid = cfg & GENMASK(8, 0);

[Severity: High]
This isn't a bug introduced by this patch, but the channel loop around this
code never checks req->chan_base or req->chan_cnt against the caller's
pfvf->rx_chan_cnt:

nix_bp_disable() {
    ...
	chan_base = pfvf->rx_chan_base + req->chan_base;
	for (chan = chan_base; chan < (chan_base + req->chan_cnt); chan++) {
		chan_v = nix_get_channel(chan, cpt_link);
		cfg = rvu_read64(rvu, blkaddr, NIX_AF_RX_CHANX_CFG(chan_v));
		rvu_write64(rvu, blkaddr, NIX_AF_RX_CHANX_CFG(chan_v),
			    cfg & ~BIT_ULL(16));
    ...
}

req->chan_base comes from the VF, which leads to three problems.

NIX_AF_RX_CHANX_CFG(a) is (0x1A30 | (a) << 15), and rvu_write64() uses
(block << 28) | offset. For channels >= 8192 the offset reaches bit 28.
Can the read-modify-write then land in NIX1, NPC or block 7 register
space?

A caller can also disarm or reprogram channels that belong to other
functions.

chan is a u16, but chan_base + req->chan_cnt is an int. If the sum is above
0xFFFF, chan wraps, and nothing in this loop can fail. Could a single
NIX_BP_DISABLE or NIX_CPT_BP_DISABLE from a CGX-mapped PF/VF or an LBK VF
spin the AF mailbox worker forever?

The LBK path in nix_bp_enable() has the same gap. Unlike the CGX and SDP
cases, the LBK case in rvu_nix_get_bpid() checks no range. This patch
already changes both loops. Could it add channel range validation too?

> +			/* Ignore channels that were never armed with an LBK
> +			 * free-pool bpid (e.g. never enabled, or belonging
> +			 * to a CGX/SDP range) - bpid - free_pool_base would
> +			 * underflow and corrupt an unrelated bitmap word.
> +			 */
> +			if (bpid < bp->free_pool_base ||
> +			    bpid >= bp->free_pool_base + bp->bpids.max)
> +				continue;

[Severity: High]
The comment says this skips channels that were never armed. The check only
tests the BPID range, though, and ignores BIT(16) and fn_map. Can a stale
BPID that is still in range get freed a second time here?

nix_bp_disable() and the new nix_bp_enable_unwind() both clear only
BIT(16). CFG[8:0] still holds the BPID they just returned to the pool:

	rvu_write64(rvu, blkaddr, NIX_AF_RX_CHANX_CFG(chan_v),
		    cfg & ~BIT_ULL(16));

rvu_nix_flr_free_bpids() frees BPIDs without touching NIX_AF_RX_CHANX_CFG
at all.

This sequence looks possible:

  VF A armed with BPID X
  VF A disabled, unwound or FLRed, X returned to the pool
  VF B allocates X
  VF A sends NIX_BP_DISABLE or NIX_CPT_BP_DISABLE
    (otx2_init_hw_resources() sends the CPT one on every open)
  nix_bp_disable() reads stale X from A's channel, passes the range
    check, and frees B's BPID
  VF C allocates X

B and C then share one hardware BPID, and B's fn_map/ref_cnt no longer
match the bitmap. Clearing CFG[8:0] on release, or checking BIT(16) and
fn_map ownership before freeing, would avoid this. Would either approach
work here?

> +
>  			mutex_lock(&rvu->rsrc_lock);
>  			rvu_free_rsrc(&bp->bpids, bpid - bp->free_pool_base);

[Severity: Medium]
This is a pre-existing issue, but nix_bp_disable() frees whatever BPID it
finds in the target channel's CFG register. It never checks that
bp->fn_map[bpid - bp->free_pool_base] == pcifunc.

req->chan_base isn't validated, so could a VF name another function's
channels, free that function's LBK BPID and clear its BP_ENA bit?

The fn_map loop that follows clears only entries the caller owns. The
victim's fn_map/ref_cnt stay set while its bitmap bit is cleared, and the
next rvu_alloc_rsrc() hands the BPID out again. The new range check keeps
the index in bounds, but it does not check ownership.

[ ... ]

> @@ -738,6 +747,35 @@ static int rvu_nix_get_bpid(struct rvu *rvu, struct nix_bp_cfg_req *req,
>  	return bpid;
>  }
>  
> +static void nix_bp_enable_unwind(struct rvu *rvu, struct nix_bp *bp,
> +				 int blkaddr, u16 chan_base, int chan_cnt,
> +				 int type, bool cpt_link)
> +{
> +	u16 chan, chan_v, bpid;
> +	u64 cfg;
> +
> +	for (chan = chan_base; chan < chan_base + chan_cnt; chan++) {

[Severity: High]
Can this loop run forever? chan is a u16, but chan_base + chan_cnt is an
int. When chan_base + chan_cnt >= 0x10000, chan wraps to 0 before it
reaches the limit.

For LBK, rvu_nix_get_bpid() never checks req->chan_base or req->chan_cnt.
A VF can therefore pick req->chan_base so that the u16 chan_base in
nix_bp_enable() is 0xFFFF, and send chan_cnt >= 2. Then:

nix_bp_enable()
    for (chan = chan_base; ...) wraps chan past 0xFFFF
        rvu_nix_get_bpid() allocates an LBK BPID on each pass
    rvu_nix_get_bpid() fails once the pool is empty
    nix_bp_enable_unwind(rvu, bp, blkaddr, chan_base, chan_id, ...)
        chan <= 0xFFFF < 0xFFFF + chan_id, so the loop never exits

Each pass does an MMIO read-modify-write on NIX_AF_RX_CHANX_CFG(chan_v),
including addresses outside the NIX block. It also takes rsrc_lock, and
there is no cond_resched(). Before this patch the same request returned
-EINVAL.

There is a related concern about the new response write in
nix_bp_enable():

	rsp->chan_bpid[chan_id] = ...

chan_id is now bounded only by the number of successful allocations. The
old response loop was bounded by req->chan_cnt. If bp->bpids.max is larger
than NIX_MAX_BPID_CHAN, could the wrapping loop write past the end of
rsp->chan_bpid[]?

> +		chan_v = nix_get_channel(chan, cpt_link);
> +		cfg = rvu_read64(rvu, blkaddr, NIX_AF_RX_CHANX_CFG(chan_v));
> +		rvu_write64(rvu, blkaddr, NIX_AF_RX_CHANX_CFG(chan_v),
> +			    cfg & ~BIT_ULL(16));

[Severity: Medium]
Does this turn off backpressure on channels that were already armed before
the request? nix_bp_enable() doesn't check whether a channel already has
BIT(16) set, and it doesn't save the original NIX_AF_RX_CHANX_CFG value. It
simply overwrites CFG[8:0] with a new BPID.

Suppose an LBK VF re-sends NIX_BP_ENABLE for channels that are already
armed, and a later iteration fails. One example is a 2-channel request when
only one BPID is free. The unwind then clears BIT(16) on every channel
programmed so far.

A channel that had working backpressure with BPID P ends up with
backpressure off and a freed BPID in CFG. P stays allocated, and nothing
refers to it.

Before this patch, the same failure left the channel armed.

> +
> +		if (type != NIX_INTF_TYPE_LBK)
> +			continue;
> +
> +		bpid = cfg & GENMASK_ULL(8, 0);
> +		if (bpid < bp->free_pool_base ||
> +		    bpid >= bp->free_pool_base + bp->bpids.max)
> +			continue;
> +
> +		mutex_lock(&rvu->rsrc_lock);
> +		rvu_free_rsrc(&bp->bpids, bpid - bp->free_pool_base);
> +		bp->fn_map[bpid - bp->free_pool_base] = 0;
> +		bp->ref_cnt[bpid - bp->free_pool_base] = 0;
> +		mutex_unlock(&rvu->rsrc_lock);

[Severity: Medium]
The unwind reads NIX_AF_RX_CHANX_CFG outside rsrc_lock to decide which
BPIDs to release. It then frees each one and zeroes fn_map/ref_cnt without
checking that fn_map[idx] == pcifunc. Can this race with an FLR of the
same VF?

__rvu_flr_handler() takes only rvu->flr_lock before it calls
rvu_nix_flr_free_bpids(). rvu_afvf_mbox_handler() takes no lock at all, so
the two can run at the same time:

  VF A: nix_bp_enable() iteration 0 allocates X and programs it
  FLR of A: rvu_nix_flr_free_bpids() frees X, register still holds X
  VF B: allocates X
  VF A: iteration 1 fails, nix_bp_enable_unwind() reads X from A's
        channel, frees it and zeroes fn_map[X]/ref_cnt[X]

B's live BPID is back in the pool, and the next allocation gives it to a
third function. Could the unwind free only the BPIDs this call allocated,
instead of reading them back from hardware?

nix_bp_disable() has the same pattern. It frees based only on the register
value and the range check.

> +	}
> +}

[ ... ]

> @@ -764,16 +805,27 @@ static int nix_bp_enable(struct rvu *rvu,

[ ... ]

>  	for (chan = chan_base; chan < (chan_base + req->chan_cnt); chan++) {
> +		bpid = rvu_nix_get_bpid(rvu, req, type, chan_id);

[Severity: Medium]
This isn't a bug introduced by this patch, but for LBK, rvu_nix_get_bpid()
allocates a new BPID from the free pool on every call and ignores
req->bpid_per_chan. mbox.h documents:

	/* bpid_per_chan = 0 assigns single bp id for range of channels */

With bpid_per_chan = 0 and chan_cnt > 1, shouldn't all the channels share
one BPID? In non-PFC mode the NIC side uses only pfvf->bpid[0] for CQ/aura
BPIDs. The BPIDs programmed on channels 1..n would then never be asserted,
and they still use up pool entries.

This patch keeps the allocation on every iteration and only changes the
response to report the per-channel BPIDs. In-tree LBK VFs skip
NIX_BP_ENABLE, so only other mailbox clients are affected.

[ ... ]

> @@ -783,16 +835,17 @@ static int nix_bp_enable(struct rvu *rvu,
>  		cfg &= ~GENMASK_ULL(8, 0);
>  		rvu_write64(rvu, blkaddr, NIX_AF_RX_CHANX_CFG(chan_v),
>  			    cfg | (bpid & GENMASK_ULL(8, 0)) | BIT_ULL(16));

[Severity: High]
Can BPIDs still leak permanently after this patch? For LBK, every enable
allocates a fresh BPID and overwrites CFG[8:0], without checking whether
the channel already holds one.

nix_bp_disable() then frees only the BPID it reads from each channel's
register. However, it clears ownership for every free-pool entry this
pcifunc owns:

nix_bp_disable() {
    ...
	rvu_free_rsrc(&bp->bpids, bpid - bp->free_pool_base);
	for (bpid = 0; bpid < bp->bpids.max; bpid++) {
		if (bp->fn_map[bpid] == pcifunc) {
			bp->fn_map[bpid] = 0;
			bp->ref_cnt[bpid] = 0;
    ...
}

rvu_nix_flr_free_bpids() only reclaims entries where fn_map[idx] ==
pcifunc. Any other BPID the function still holds keeps its bitmap bit set
with fn_map == 0, and nothing ever frees it.

Two sequences seem to hit this:

  An LBK VF sends NIX_BP_ENABLE, NIX_BP_ENABLE, then NIX_BP_DISABLE. The
  second enable overwrites the first BPID in CFG. The disable frees only
  the second BPID but wipes fn_map for both, so the first BPID is lost
  until the AF reloads.

  A VF enables RX BP (BPID A) and CPT BP (BPID B), disables RX BP, and
  then goes through FLR. The disable already wiped fn_map[B], so the FLR
  skips B.

Each round leaks one BPID from the shared LBK pool. Once the pool is
empty, NIX_BP_ENABLE fails with NIX_AF_ERR_INVALID_BPID for every LBK VF.

The new comment in nix_bp_enable() says "don't leak until an FLR". Doesn't
this path prevent the FLR from reclaiming them?

[ ... ]

> +		rsp->chan_bpid[chan_id] = ((req->chan_base + chan_id) & 0x7F) << 10 |
> +					  (bpid & 0x3FF);
> +		chan_id++;

[Severity: Medium]
This is a pre-existing issue, but the rewritten line keeps the old encoding,
which doesn't fit in a u16 chan_bpid[] element. 0x7F << 10 is 0x1FC00,
which needs 17 bits, so bit 6 of the channel index is lost.

mbox_handler_nix_bp_enable() on the NIC side decodes a 7-bit channel index:

	chan_id = ((rsp->chan_bpid[chan] >> 10) & 0x7F);

Won't channel indices >= 64 then alias onto lower pfvf->bpid[] entries?
struct nix_bp_cfg_rsp is sized for NIX_MAX_BPID_CHAN (256) channels, which
this encoding cannot represent. In-tree CGX requests stay well below 64
channels, so only SDP or LBK requests with more than 64 channels hit this.

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-10-01  9:57 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-28  6:57 [PATCH net v3] octeontx2-af: Fix BPID leak in nix_bp_enable() nshettyj
2026-09-28  6:59 ` netdev-bot+sinfo
2026-10-01  9:57 ` netdev-bot+sashiko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®