Re: [PATCH v2 net-next 5/9] octeontx2-af: PAN switch TL1 scheduling and NPC channel control

[Paolo Abeni]

From: AI Reviewer <ai@xxxxxxxxxxx>

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/
---
octeontx2-af: PAN switch TL1 scheduling and NPC channel control

This change adds NIX_TXSCH_ALLOC_FLAG_PAN so nix_txsch_alloc can request
more than one aggregate-level (TL1) index for PAN/switch mode, adds
TXSCHQ_FREE_PAN_TL1 to permit freeing those indices via
nix_txschq_free_one, and adds a set_chanmask field to
npc_install_flow_req. It also widens the NIX LF SQ bitmap allocation
and extends struct sg_list.

A few questions inline below.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> index 7ac26629d9af..28ebf0a8d3d6 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> @@ -1157,6 +1157,8 @@ struct nix_txsch_alloc_req {
>  	/* Scheduler queue count request at each level */
>  	u16 schq_contig[NIX_TXSCH_LVL_CNT]; /* No of contiguous queues */
>  	u16 schq[NIX_TXSCH_LVL_CNT]; /* No of non-contiguous queues */
> +#define NIX_TXSCH_ALLOC_FLAG_PAN BIT_ULL(0)
> +	u64 flags;
>  };
>  
>  struct nix_txsch_alloc_rsp {
> @@ -1175,6 +1177,7 @@ struct nix_txsch_alloc_rsp {
>  struct nix_txsch_free_req {
>  	struct mbox_msghdr hdr;
>  #define TXSCHQ_FREE_ALL BIT_ULL(0)
> +#define TXSCHQ_FREE_PAN_TL1 BIT_ULL(1)
>  	u16 flags;
>  	/* Scheduler queue level to be freed */
>  	u16 schq_lvl;

[Critical]
Should these two new flags be gated on the requester being the AF via
is_pffunc_af(req->hdr.pcifunc)?

Before this change, the aggregate level (TL1) was intentionally opaque
to non-AF callers: nix_txschq_free_one() short-circuited with
if (lvl >= hw->cap.nix_tx_aggr_lvl) return 0; and nix_txsch_alloc()
forced a single TL queue tied to the caller's mapped Tx link.

With the new flags there does not appear to be any is_pffunc_af() check
on the alloc or free paths, so any PF/VF can pass NIX_TXSCH_ALLOC_FLAG_PAN
to bypass the Tx-link mapping and touch TL1 slots that belong to
other links or other pcifuncs, or pass TXSCHQ_FREE_PAN_TL1 to free a
TL1 index. The existing comment in nix_txschq_free() says:

    /* Do not clear pcifunc in txsch->pfvf_map[schq] because VF might
     * be using this TL1 queue
     */

so if a PF frees a TL1 that a VF is still transmitting through, after
rvu_free_rsrc(&txsch->schq, schq) and
pfvf_map[schq] = TXSCH_MAP(0, NIX_TXSCHQ_FREE) that hardware slot is
reallocatable while the VF still uses it. Is that intended?

> @@ -2105,6 +2108,7 @@ struct npc_install_flow_req {
>  	u8 hw_prio;
>  	u8  req_kw_type; /* Key type to be written */
>  	u8 alloc_entry;	/* only for cn20k */
> +	u8 set_chanmask;

[Medium]
The commit message says:

    For NPC install flow, add set_chanmask so callers can keep a
    non-default chan_mask when the requester is not the AF; without
    it, chan_mask was always forced to 0xFFF for non-AF functions.

Where is set_chanmask consumed? A grep across the tree finds only this
struct field definition and no reader; the existing non-AF override in
rvu_npc_fs.c is unchanged:

    if (!is_pffunc_af(req->hdr.pcifunc))
            req->chan_mask = rvu_get_cpt_chan_mask(rvu);

Should the corresponding "keep chan_mask" hunk that consults
set_chanmask be part of this commit, or was it meant for a follow-up?

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> index d2734f6d6d92..35c91cdc50b9 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c

[ ... ]

> @@ -2138,11 +2138,14 @@ static int nix_check_txschq_alloc_req(struct rvu *rvu, int lvl, u16 pcifunc,
>  	if (!req_schq)
>  		return 0;
>  
> -	link = nix_get_tx_link(rvu, pcifunc);
> +	if (req->flags & NIX_TXSCH_ALLOC_FLAG_PAN)
> +		link = hw->cgx_links + hw->lbk_links + 1;
> +	else
> +		link = nix_get_tx_link(rvu, pcifunc);
>  
>  	/* For traffic aggregating scheduler level, one queue is enough */
>  	if (lvl >= hw->cap.nix_tx_aggr_lvl) {
> -		if (req_schq != 1)
> +		if (req_schq != 1 && !(req->flags & NIX_TXSCH_ALLOC_FLAG_PAN))
>  			return NIX_AF_ERR_TLX_ALLOC_FAIL;
>  		return 0;
>  	}

[Critical]
Can this branch let a caller-controlled count reach the alloc loop
without a MAX_TXSCHQ_PER_FUNC ceiling?

For non-PAN, aggregate-level requests were previously rejected unless
req_schq == 1. With NIX_TXSCH_ALLOC_FLAG_PAN, the check is skipped and
this function returns 0 before the general guard further down:

    if (free_cnt < req_schq || req->schq[lvl] > MAX_TXSCHQ_PER_FUNC ||
        req->schq_contig[lvl] > MAX_TXSCHQ_PER_FUNC)
            return NIX_AF_ERR_TLX_ALLOC_FAIL;

rvu_mbox_handler_nix_txsch_alloc() later copies the request into rsp
and iterates:

    for (idx = 0; idx < req->schq_contig[lvl]; idx++) {
            schq = rsp->schq_contig_list[lvl][idx];
            ...
            pfvf_map[schq] = TXSCH_MAP(pcifunc, 0);
            nix_reset_tx_linkcfg(rvu, blkaddr, lvl, schq);
            nix_reset_tx_shaping(rvu, blkaddr, nixlf, lvl, schq);
            nix_reset_tx_schedule(rvu, blkaddr, lvl, schq);
    }

rsp->schq_contig_list[lvl] and rsp->schq_list[lvl] are each
MAX_TXSCHQ_PER_FUNC entries. If req->schq_contig[lvl] exceeds
MAX_TXSCHQ_PER_FUNC, idx runs past the array, schq is read from
adjacent memory, and pfvf_map[schq] = TXSCH_MAP(pcifunc, 0) writes at
an attacker-controlled offset. Should the PAN branch keep the
MAX_TXSCHQ_PER_FUNC bound?

[High]
The PAN link index is computed as:

    link = hw->cgx_links + hw->lbk_links + 1;

and the caller then does end = link + 1. For the TL1 level,
txsch->schq.max is programmed from NIX_AF_TL1_CONST in
nix_setup_txschq():

    txsch->schq.max = cfg & 0xFFFF;

Is there anything guaranteeing that hw->cgx_links + hw->lbk_links + 2
is less than txsch->schq.max for TL1? The non-PAN path is bounded
because nix_get_tx_link() only returns values in the range
[0, cgx_links + lbk_links]. The new path adds one beyond that with no
ceiling check, so set_bit(schq, txsch->schq.bmap) below and later
NIX_AF_TL1X_CIR(schq) register writes could target undefined TL1
indices on silicon where TL1 count is small.

> @@ -2179,11 +2182,41 @@ static void nix_txsch_alloc(struct rvu *rvu, struct nix_txsch *txsch,
>  	struct rvu_hwinfo *hw = rvu->hw;
>  	u16 pcifunc = rsp->hdr.pcifunc;
>  	int idx, schq;
> +	bool alloc;
>  
>  	/* For traffic aggregating levels, queue alloc is based
>  	 * on transmit link to which PF_FUNC is mapped to.
>  	 */
>  	if (lvl >= hw->cap.nix_tx_aggr_lvl) {
> +		if (start != end) {
> +			idx = 0;
> +			alloc = false;
> +			for (schq = start; schq <= end; schq++, idx++) {
> +				if (test_bit(schq, txsch->schq.bmap))
> +					continue;
> +
> +				set_bit(schq, txsch->schq.bmap);
> +
> +				/* A single TL queue is allocated each time */
> +				if (rsp->schq_contig[lvl]) {
> +					alloc = true;
> +					rsp->schq_contig_list[lvl][idx] = schq;
> +					continue;
> +				}
> +
> +				if (rsp->schq[lvl]) {
> +					alloc = true;
> +					rsp->schq_list[lvl][idx] = schq;
> +					continue;
> +				}
> +			}
> +
> +			if (!alloc)
> +				dev_err(rvu->dev,
> +					"Could not allocate schq at lvl=%u start=%u end=%u\n",
> +					lvl, start, end);
> +			return;
> +		}

[Critical]
A few concerns on this new block.

First, idx advances unconditionally in the for-clause even when the
continue-on-busy path is taken. Consider start=L, end=L+1 where slot
L is already set in the bitmap and req->schq_contig[lvl] == 1:

    iter 1: schq=L busy       -> continue (idx becomes 1)
    iter 2: schq=L+1 free     -> set_bit(L+1); rsp->schq_contig_list[lvl][1] = L+1

rsp->schq_contig_list[lvl][0] stays as 0. The caller then loops
idx = 0 .. req->schq_contig[lvl] - 1 in rvu_mbox_handler_nix_txsch_alloc()
and picks up schq = rsp->schq_contig_list[lvl][0] = 0. That triggers
pfvf_map[0] = TXSCH_MAP(pcifunc, 0) if NIX_TXSCHQ_CFG_DONE is not set,
and nix_reset_tx_linkcfg/shaping/schedule(rvu, blkaddr, lvl, 0),
disturbing TL1[0]. Can this hijack TL1[0] from its real owner?

Second, this loop calls set_bit(schq, txsch->schq.bmap) once for each
free slot in [start, end], but never decrements
rsp->schq_contig[lvl]/rsp->schq[lvl] or otherwise reports how many
were placed into the response list. With req->schq_contig[lvl] == 1
and both slots free, both bits get set, but only one is reported back.
The bit for the extra slot then has no matching entry in pfvf_map, and
nix_txschq_free_one() gates on:

    if (TXSCH_MAP_FUNC(pfvf_map[schq]) != pcifunc) {
            rc = NIX_AF_ERR_TLX_INVALID;
            goto err;
    }

so the client cannot free it. Is the resulting bitmap leak intended?

Third, on the "no slot free" path, alloc stays false, dev_err() is
logged, and the function returns without clearing rsp->schq_contig[lvl]
or rsp->schq[lvl]. The mbox handler returns success, so consumers see
non-zero counts and then read rsp->schq_contig_list[lvl][0] == 0
(zero-initialized rsp) as a valid schq. Should this path signal
failure back to the caller?

> +		/* A single TL queue is allocated */
>  		if (rsp->schq_contig[lvl]) {
>  			rsp->schq_contig[lvl] = 1;

[ ... ]

> @@ -2300,11 +2333,14 @@ int rvu_mbox_handler_nix_txsch_alloc(struct rvu *rvu,
>  		rsp->schq[lvl] = req->schq[lvl];
>  		rsp->schq_contig[lvl] = req->schq_contig[lvl];
>  
> -		link = nix_get_tx_link(rvu, pcifunc);
> +		if (req->flags & NIX_TXSCH_ALLOC_FLAG_PAN)
> +			link = hw->cgx_links + hw->lbk_links + 1;
> +		else
> +			link = nix_get_tx_link(rvu, pcifunc);
>  
>  		if (lvl >= hw->cap.nix_tx_aggr_lvl) {
>  			start = link;
> -			end = link;
> +			end = link + !!(req->flags & NIX_TXSCH_ALLOC_FLAG_PAN);
>  		} else if (hw->cap.nix_fixed_txschq_mapping) {
>  			nix_get_txschq_range(rvu, pcifunc, link, &start, &end);
>  		} else {

[High]
The commit message states:

    Add TXSCHQ_FREE_PAN_TL1 so TL1 entries in that path can be freed
    via nix_txsch_free where they were previously skipped.

Is nix_txsch_free() actually updated by this patch? Only
nix_txschq_free_one() gets a new gate. The bulk path nix_txschq_free()
still has if (lvl >= hw->cap.nix_tx_aggr_lvl) continue; in both its
scan and free loops, so aggregate-level bits set by the new
nix_txsch_alloc() block above are never cleared by that path, whether
it is reached via TXSCHQ_FREE_ALL or via nix_lf_teardown() on LF close
or FLR. Was a matching hunk in nix_txschq_free() intended?

> @@ -2669,7 +2705,9 @@ static int nix_txschq_free_one(struct rvu *rvu,
>  	schq = req->schq;
>  	txsch = &nix_hw->txsch[lvl];
>  
> -	if (lvl >= hw->cap.nix_tx_aggr_lvl || schq >= txsch->schq.max)
> +	if ((lvl >= hw->cap.nix_tx_aggr_lvl &&
> +	     !(req->flags & TXSCHQ_FREE_PAN_TL1)) ||
> +	    schq >= txsch->schq.max)
>  		return 0;
>  
>  	pfvf_map = txsch->pfvf_map;
-- 
This is an AI-generated review.