[PATCH net] octeontx2-af: fix partial NIX Tx scheduler queue allocation

[PATCH net] octeontx2-af: fix partial NIX Tx scheduler queue allocation

[Ratheesh Kannoth]

nix_txsch_alloc() could return success after allocating fewer Tx
scheduler queues than requested, or after failing to allocate a
queue under the fixed topology mapping. Propagate allocation
failures to the mailbox handler and roll back any partially
allocated queue indices on error.

Fixes: 5d9b976d4480 ("octeontx2-af: Support fixed transmit scheduler topology")
Signed-off-by: Ratheesh Kannoth <rkannoth@marvell.com>
---
 .../ethernet/marvell/octeontx2/af/rvu_nix.c   | 33 +++++++++++++++----
 1 file changed, 26 insertions(+), 7 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..28e70bae51ca 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
+++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
@@ -2195,12 +2195,14 @@ static int nix_check_txschq_alloc_req(struct rvu *rvu, int lvl, u16 pcifunc,
 	return 0;
 }
 
-static void nix_txsch_alloc(struct rvu *rvu, struct nix_txsch *txsch,
-			    struct nix_txsch_alloc_rsp *rsp,
-			    int lvl, int start, int end)
+static int nix_txsch_alloc(struct rvu *rvu, struct nix_txsch *txsch,
+			   struct nix_txsch_alloc_rsp *rsp,
+			   int lvl, int start, int end)
 {
+	int want_contig = rsp->schq_contig[lvl];
 	struct rvu_hwinfo *hw = rvu->hw;
 	u16 pcifunc = rsp->hdr.pcifunc;
+	int want = rsp->schq[lvl];
 	int idx, schq;
 
 	/* For traffic aggregating levels, queue alloc is based
@@ -2221,7 +2223,7 @@ static void nix_txsch_alloc(struct rvu *rvu, struct nix_txsch *txsch,
 			rsp->schq[lvl] = 1;
 			rsp->schq_list[lvl][0] = start;
 		}
-		return;
+		return 0;
 	}
 
 	/* Adjust the queue request count if HW supports
@@ -2233,7 +2235,7 @@ static void nix_txsch_alloc(struct rvu *rvu, struct nix_txsch *txsch,
 		if (idx >= (end - start) || test_bit(schq, txsch->schq.bmap)) {
 			rsp->schq_contig[lvl] = 0;
 			rsp->schq[lvl] = 0;
-			return;
+			return -ENOMEM;
 		}
 
 		if (rsp->schq_contig[lvl]) {
@@ -2246,7 +2248,7 @@ static void nix_txsch_alloc(struct rvu *rvu, struct nix_txsch *txsch,
 			set_bit(schq, txsch->schq.bmap);
 			rsp->schq_list[lvl][0] = schq;
 		}
-		return;
+		return 0;
 	}
 
 	/* Allocate contiguous queue indices requesty first */
@@ -2277,6 +2279,22 @@ static void nix_txsch_alloc(struct rvu *rvu, struct nix_txsch *txsch,
 		/* Update how many were allocated */
 		rsp->schq[lvl] = idx;
 	}
+
+	if ((want_contig && !rsp->schq_contig[lvl]) || (want && !rsp->schq[lvl]))
+		goto err;
+
+	return 0;
+
+err:
+	for (idx = 0; idx < rsp->schq_contig[lvl]; idx++)
+		clear_bit(rsp->schq_contig_list[lvl][idx], txsch->schq.bmap);
+
+	for (idx = 0; idx < rsp->schq[lvl]; idx++)
+		clear_bit(rsp->schq_list[lvl][idx], txsch->schq.bmap);
+
+	rsp->schq_contig[lvl] = 0;
+	rsp->schq[lvl] = 0;
+	return -ENOMEM;
 }
 
 int rvu_mbox_handler_nix_txsch_alloc(struct rvu *rvu,
@@ -2335,7 +2353,8 @@ int rvu_mbox_handler_nix_txsch_alloc(struct rvu *rvu,
 			end = txsch->schq.max;
 		}
 
-		nix_txsch_alloc(rvu, txsch, rsp, lvl, start, end);
+		if (nix_txsch_alloc(rvu, txsch, rsp, lvl, start, end))
+			goto err;
 
 		/* Reset queue config */
 		for (idx = 0; idx < req->schq_contig[lvl]; idx++) {
-- 
2.43.0

Re: [PATCH net] octeontx2-af: fix partial NIX Tx scheduler queue allocation

[netdev-bot+sashiko]

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

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

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] The patch says it fixes nix_txsch_alloc() 'return[ing] success
  after allocating fewer Tx scheduler queues than requested'.
- [High] The new rollback in nix_txsch_alloc() (rvu_nix.c:2289-2293)
  takes its loop bounds (rsp->schq_contig[lvl] and rsp->schq[lvl], both…
- [Medium] The new `if (nix_txsch_alloc(...)) goto err;` inside the per-
  level loop of rvu_mbox_handler_nix_txsch_alloc() (rvu_nix.c:2356-2357)…
- [Low] The commit message says nix_txsch_alloc() 'could return success
  after allocating fewer Tx scheduler queues than requested, or after…

--

octeontx2-af: fix partial NIX Tx scheduler queue allocation

This patch makes nix_txsch_alloc() return an error when it cannot satisfy
a Tx scheduler queue request, and clears the bitmap bits it set for the
failing level. rvu_mbox_handler_nix_txsch_alloc() then returns
NIX_AF_ERR_TLX_ALLOC_FAIL when that happens.

> nix_txsch_alloc() could return success after allocating fewer Tx
> scheduler queues than requested, or after failing to allocate a
> queue under the fixed topology mapping.

[Severity: Low]
Can either of these happen when the requester is well behaved?

nix_txsch_alloc() has one caller, rvu_mbox_handler_nix_txsch_alloc(). That
caller first runs nix_check_txschq_alloc_req() on every level, and it holds
rvu->rsrc_lock across both loops.

For the fixed mapping, the pre-check already does this:

drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c:nix_check_txschq_alloc_req() {
    ...
		schq = start + (pcifunc & RVU_PFVF_FUNC_MASK);
		if (end <= txsch->schq.max && schq < end &&
		    !test_bit(schq, txsch->schq.bmap))
			free_cnt = 1;
		else
			free_cnt = 0;
    ...
}

That rejects the same idx >= (end - start) || test_bit() condition that
now returns -ENOMEM.

For the non-fixed mapping, the pre-check uses rvu_rsrc_free_count() and
rvu_rsrc_check_contig(). These do the same bitmap search that the
allocation does. Bits are only set inside nix_txsch_alloc(), so the full
request should always be satisfied.

As far as I can tell, the new failure paths only run if the peer changes
the shared mailbox request between the check and the allocation. Could the
commit message describe that case? And is the Fixes: tag still
appropriate, given that it suggests a user-visible bug for normal callers?

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> index 153eb57bad06..28e70bae51ca 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> @@ -2195,12 +2195,14 @@ static int nix_check_txschq_alloc_req(struct rvu *rvu, int lvl, u16 pcifunc,
>  	return 0;
>  }
>  
> -static void nix_txsch_alloc(struct rvu *rvu, struct nix_txsch *txsch,
> -			    struct nix_txsch_alloc_rsp *rsp,
> -			    int lvl, int start, int end)
> +static int nix_txsch_alloc(struct rvu *rvu, struct nix_txsch *txsch,
> +			   struct nix_txsch_alloc_rsp *rsp,
> +			   int lvl, int start, int end)
>  {
> +	int want_contig = rsp->schq_contig[lvl];
>  	struct rvu_hwinfo *hw = rvu->hw;
>  	u16 pcifunc = rsp->hdr.pcifunc;
> +	int want = rsp->schq[lvl];
>  	int idx, schq;

[ ... ]

> @@ -2277,6 +2279,22 @@ static void nix_txsch_alloc(struct rvu *rvu, struct nix_txsch *txsch,
>  		/* Update how many were allocated */
>  		rsp->schq[lvl] = idx;
>  	}
> +
> +	if ((want_contig && !rsp->schq_contig[lvl]) || (want && !rsp->schq[lvl]))
> +		goto err;

[Severity: High]
Does this catch a short non-contiguous allocation? The check only fires
when rsp->schq[lvl] is zero. If 0 < idx < want after the non-contiguous
loop, nix_txsch_alloc() still returns 0. That is the "fewer Tx scheduler
queues than requested" case the commit message says it fixes.

The caller does not check again. Its reset loops are bounded by the
requested count, not the allocated count:

drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c:rvu_mbox_handler_nix_txsch_alloc() {
    ...
		for (idx = 0; idx < req->schq[lvl]; idx++) {
			schq = rsp->schq_list[lvl][idx];
			if (!(TXSCH_MAP_FLAGS(pfvf_map[schq]) &
			    NIX_TXSCHQ_CFG_DONE))
				pfvf_map[schq] = TXSCH_MAP(pcifunc, 0);
			nix_reset_tx_linkcfg(rvu, blkaddr, lvl, schq);
    ...
}

On a short allocation, this loop reads rsp->schq_list[lvl][] slots that
were never filled in. Those are usually zero, but the peer can write them.
The loop then resets linkcfg, shaping and schedule for those queue
indices. If CFG_DONE is not set, it also assigns pfvf_map ownership to
pcifunc. The queue may belong to another PF or VF.

On the PF side, otx2_qos_txschq_alloc() in nic/qos.c copies only
rsp->schq[lvl] queue IDs but keeps cfg->schq[] at the requested counts.
otx2_qos_txschq_fill_cfg_schq() then gives stale IDs to the extra child
nodes.

A short allocation can happen if the requester raises req->schq[lvl] in
the shared mailbox after nix_check_txschq_alloc_req() reads it and before
the handler reads it again here:

		rsp->schq[lvl] = req->schq[lvl];

Should the check require rsp->schq[lvl] == want and
rsp->schq_contig[lvl] == want_contig? Should the reset loops in the
handler use the allocated rsp counts instead of the req counts?

> +
> +	return 0;
> +
> +err:
> +	for (idx = 0; idx < rsp->schq_contig[lvl]; idx++)
> +		clear_bit(rsp->schq_contig_list[lvl][idx], txsch->schq.bmap);
> +
> +	for (idx = 0; idx < rsp->schq[lvl]; idx++)
> +		clear_bit(rsp->schq_list[lvl][idx], txsch->schq.bmap);

[Severity: High]
Is it safe to take both the loop bounds and the clear_bit() indices from
rsp here?

The response message is not a private copy. otx2_mbox_alloc_msg_rsp()
places it at mdev->mbase + tx_start + ... . For the AF's own VFs,
mdev->mbase is the ioremap_wc() BAR4 region shared with the VF, and the
AF processes the message directly in that memory.

Nothing limits rsp->schq[lvl] or rsp->schq_contig[lvl] to
MAX_TXSCHQ_PER_FUNC. Nothing checks the list entries against
txsch->schq.max either.

A peer that rewrites these fields before the rollback runs could cause
two problems:

  - A larger rsp->schq[lvl] makes the loop read past the schq_list row.

  - clear_bit() can run on any u16 index, up to 65535. That writes past
    the end of the kmalloc'd txsch->schq.bmap. It can also clear bits for
    queues owned by other functions, which then get allocated twice.

The peer can also force this err path. nix_check_txschq_alloc_req()
validates req->schq_contig[lvl]. The handler then reads it again into
rsp->schq_contig[lvl], and nix_txsch_alloc() reads it again from rsp. If
the value is raised in between, bitmap_find_next_zero_area() fails and
execution goes to err.

The existing reset loops in the handler already index pfvf_map[schq] with
values from rsp->schq_list. This rollback adds another write target to
that same pattern.

Could the allocated counts and indices be kept in kernel-private storage
for the rollback? Alternatively, they could be checked against
MAX_TXSCHQ_PER_FUNC and txsch->schq.max before use.

> +
> +	rsp->schq_contig[lvl] = 0;
> +	rsp->schq[lvl] = 0;
> +	return -ENOMEM;
>  }
>  
>  int rvu_mbox_handler_nix_txsch_alloc(struct rvu *rvu,
> @@ -2335,7 +2353,8 @@ int rvu_mbox_handler_nix_txsch_alloc(struct rvu *rvu,
>  			end = txsch->schq.max;
>  		}
>  
> -		nix_txsch_alloc(rvu, txsch, rsp, lvl, start, end);
> +		if (nix_txsch_alloc(rvu, txsch, rsp, lvl, start, end))
> +			goto err;

[Severity: Medium]
Does this goto leak the queues already allocated for levels 0..lvl-1?

For those earlier levels, nix_txsch_alloc() has already set their bits in
txsch->schq.bmap. The reset loops have also set pfvf_map[schq] to
TXSCH_MAP(pcifunc, 0) and reset their hardware config. The err label only
does this:

err:
	rc = NIX_AF_ERR_TLX_ALLOC_FAIL;
exit:
	mutex_unlock(&rvu->rsrc_lock);

The requesters treat a non-zero rc as "nothing allocated" and return.
These include otx2_qos_txschq_alloc(), otx2_txsch_alloc() and
otx2_pfc_txschq_alloc_one(). They never learn the queue IDs from the
earlier levels, so they cannot release them with NIX_TXSCH_FREE.

Those queues stay allocated to pcifunc until nix_txschq_free() runs at LF
teardown, and each retry can leak more. Before this patch nothing could
fail after the pre-check, so this error path is new.

The commit message also says the patch will "roll back any partially
allocated queue indices on error", but here only the failing level is
rolled back.

>  
>  		/* Reset queue config */
>  		for (idx = 0; idx < req->schq_contig[lvl]; idx++) {

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

Re: [PATCH net] octeontx2-af: fix partial NIX Tx scheduler queue allocation

[Ratheesh Kannoth]

On 2026-09-28 at 14:02:32, netdev-bot+sashiko@kernel.org (netdev-bot+sashiko@kernel.org) wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider.
>
> Critical: 0 · High: 2 · Medium: 1 · Low: 1
>
> 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] The patch says it fixes nix_txsch_alloc() 'return[ing] success
>   after allocating fewer Tx scheduler queues than requested'.

ACK

pw-bot: changes-requested