* Re: [PATCH net] octeontx2-af: Handle pool context address update in aura WRITE operation
2026-09-01 10:50 [PATCH net] octeontx2-af: Handle pool context address update in aura WRITE operation nshettyj
@ 2026-09-05 19:52 ` netdev-bot+sashiko
0 siblings, 0 replies; 2+ messages in thread
From: netdev-bot+sashiko @ 2026-09-05 19:52 UTC (permalink / raw)
To: nshettyj
Cc: netdev, linux-kernel, asekhar, sgoutham, rkannoth, gakula,
sbhatta, andrew+netdev, davem, edumazet, kuba, pabeni
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:
Critical: 0 · High: 1 (1) · Medium: 1 (1) · Low: 0
- [High] In rvu_npa_aq_enq_inst()'s NPA_AQ_INSTOP_WRITE path, the new
gate `if (req->aura_mask.pool_addr)` treats a documented bit-granular…
- [Medium] npa_aura_translate_pool_addr() validates and rewrites
req->aura.pool_addr in place, where `req` points directly into the…
Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it defeats the
bounds check the patch adds: rvu_mbox_handler_npa_lf_alloc() validates…
- [Medium] npa_aura_translate_pool_addr() dereferences
pfvf->pool_ctx->qsize/iova/entry_sz after only a NULL test, with no…
--
octeontx2-af: Handle pool context address update in aura WRITE operation
This adds a helper npa_aura_translate_pool_addr() which checks that
pfvf->pool_ctx exists, bounds checks the pool_id in aura.pool_addr and
converts it into the pool context IOVA. The helper is then used from both
the NPA_AQ_INSTOP_WRITE and NPA_AQ_INSTOP_INIT legs of
rvu_npa_aq_enq_inst().
A few questions below about the translation in the WRITE path and about the
inputs the new bounds check relies on.
> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npa.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npa.c
> index e2a33e46b48ad..c8255271f84eb 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npa.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npa.c
> @@ -58,6 +58,19 @@ static int npa_aq_enqueue_wait(struct rvu *rvu, struct rvu_block *block,
> return 0;
> }
>
> +static int npa_aura_translate_pool_addr(struct rvu_pfvf *pfvf, u64 *pool_addr)
> +{
> + if (!pfvf->pool_ctx)
> + return NPA_AF_ERR_AQ_ENQUEUE;
[Severity: Medium]
This is a pre-existing issue and not something this patch introduces, but
now that the dereference is factored into a helper it is worth asking: what
keeps pfvf->pool_ctx alive between this NULL test and the field reads just
below it?
npa_ctx_free() does:
qmem_free(rvu->dev, pfvf->pool_ctx);
pfvf->pool_ctx = NULL;
and it is reached both from rvu_mbox_handler_npa_lf_free() and from the FLR
work item:
__rvu_flr_handler() -> rvu_blklf_teardown(BLKADDR_NPA)
-> rvu_npa_lf_teardown() -> npa_ctx_free()
The FLR side runs under rvu->flr_lock, while rvu_npa_aq_enq_inst() runs from
the mailbox worker holding only aq->lock, which is the per-block admin queue
lock and is unrelated to the pfvf context memory. With disjoint lock sets,
can an npa_aq_enq for the same pcifunc read pool_ctx->qsize, iova and
entry_sz after qmem_free() has released them, and program the resulting
garbage IOVA into the aura?
The same unsynchronised pattern already exists for pfvf->aura_ctx at the top
of rvu_npa_aq_enq_inst(), so this looks like it wants serialisation of NPA
context alloc/free against AQ enqueue rather than more pointer checks.
> +
> + if (*pool_addr >= pfvf->pool_ctx->qsize)
> + return NPA_AF_ERR_PARAM;
[Severity: High]
This isn't a bug introduced by this patch, but it does limit what this new
bounds check can guarantee. How much can pool_ctx->qsize be trusted here?
rvu_mbox_handler_npa_lf_alloc() only rejects a zero nr_pools:
if (req->aura_sz > NPA_AURA_SZ_MAX ||
req->aura_sz == NPA_AURA_SZ_0 || !req->nr_pools)
return NPA_AF_ERR_PARAM;
...
err = qmem_alloc(rvu->dev, &pfvf->pool_ctx, req->nr_pools, hwctx_size);
and qmem_alloc() computes the allocation size in int:
qmem->alloc_sz = (qsize * entry_sz) + OTX2_ALIGN;
qmem->base = dma_alloc_attrs(dev, qmem->alloc_sz, &qmem->iova, ...);
...
qmem->qsize = qsize;
Since the pool context size is a power of two, a mailbox request with
nr_pools = 0x2000000 and entry_sz = 128 makes the product wrap to 0, so only
the alignment padding is actually allocated while qmem->qsize keeps the full
0x2000000. Does this then let npa_aura_translate_pool_addr() accept any
pool_id below that inflated qsize and return iova + pool_id * entry_sz well
past the real DMA allocation, which NPA will fetch and update as an
NPA_POOL_S?
If so, the bound probably needs to be enforced in
rvu_mbox_handler_npa_lf_alloc() / qmem_alloc() with checked size arithmetic
before qsize is recorded.
> +
> + *pool_addr = pfvf->pool_ctx->iova +
> + (*pool_addr * pfvf->pool_ctx->entry_sz);
> + return 0;
> +}
> +
> int rvu_npa_aq_enq_inst(struct rvu *rvu, struct npa_aq_enq_req *req,
> struct npa_aq_enq_rsp *rsp)
> {
> @@ -116,6 +129,12 @@ int rvu_npa_aq_enq_inst(struct rvu *rvu, struct npa_aq_enq_req *req,
> case NPA_AQ_INSTOP_WRITE:
> /* Copy context and write mask */
> if (req->ctype == NPA_AQ_CTYPE_AURA) {
> + if (req->aura_mask.pool_addr) {
> + rc = npa_aura_translate_pool_addr(pfvf,
> + &req->aura.pool_addr);
> + if (rc)
> + break;
> + }
[Severity: High]
Is a nonzero aura_mask.pool_addr the right condition here? mbox.h documents
aura_mask as bit granular write enable data:
/* Mask data when op == WRITE (1=write, 0=don't write) */
and npa_aura_s declares pool_addr as a full 64-bit word:
u64 pool_addr; /* W0 */
so a requester can pass a partial mask, for example BIT_ULL(16). In-tree
users do set partial masks on other fields, e.g. aura_mask.ena = 1 in
otx2_xsk.c and aura_mask.nix0_bpid = GENMASK(8, 0) in otx2_dcbnl.c.
With a partial mask, this code translates the whole 64-bit word to
pool_ctx->iova + pool_id * entry_sz, and then the mask is handed to the
admin queue unchanged:
> memcpy(mask, &req->aura_mask,
> sizeof(struct npa_aura_s));
> memcpy(ctx, &req->aura, sizeof(struct npa_aura_s));
Hardware commits (old_pool_addr & ~mask) | (translated & mask), so doesn't
the aura end up with a spliced pointer that is neither the previous value nor
the requested pool context? The bounds check in
npa_aura_translate_pool_addr() only constrains the fully written value, so
the merged address can fall outside [iova, iova + qsize * entry_sz) and NPA
will still dereference it as an NPA_POOL_S, whose stack_base and fc_addr
drive further DMA. Meanwhile the AF records the aura as programmed in
pfvf->aura_bmap further down the function.
Repeated single bit masked writes would also let the requester steer
individual bits of the pool pointer, since each masked bit takes its value
from a translated address of a pool_id the requester chose.
Would it be better to force the mask for the translated word, for example
setting req->aura_mask.pool_addr = ~0ULL after translating, or to reject any
aura_mask.pool_addr other than 0 or ~0ULL, rather than passing the requester
mask through?
[Severity: Medium]
Related question about where req lives. __rvu_mbox_handler() takes the
message straight out of the shared mailbox region:
msg = mdev->mbase + offset;
...
err = rvu_process_mbox_msg(mbox, devid, msg);
so req points into memory the requesting PF/VF can keep writing. The gate
if (req->aura_mask.pool_addr), the *pool_addr >= qsize test inside the
helper, and the later memcpy() of req->aura and req->aura_mask into the AQ
context and mask areas are all separate fetches of that memory.
Can a requester that flips aura_mask.pool_addr from zero to nonzero, or
rewrites aura.pool_addr, after the check has run get the raw untranslated
value into the hardware aura? aq->lock only serialises AF side use of the
admin queue result buffer, it does not stop the peer from writing the
message.
The INIT leg has the same validate in place and recopy window, which the two
paths now share through the helper. Would snapshotting aura.pool_addr and
aura_mask.pool_addr into local variables once, validating the snapshot, and
building the AQ context and mask from it close this?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260901105029.1667495-1-nshettyj%40marvell.com
^ permalink raw reply [flat|nested] 2+ messages in thread