* Re: [PATCH net] octeontx2-af: guard is_pf_cgxmapped() when PF to CGX map is absent
2026-09-15 2:29 [PATCH net] octeontx2-af: guard is_pf_cgxmapped() when PF to CGX map is absent Ratheesh Kannoth
@ 2026-09-18 5:30 ` netdev-bot+sashiko
2026-09-18 5:55 ` Ratheesh Kannoth
0 siblings, 1 reply; 4+ messages in thread
From: netdev-bot+sashiko @ 2026-09-18 5:30 UTC (permalink / raw)
To: rkannoth
Cc: davem, gakula, lcherian, linux-kernel, netdev, sgoutham,
andrew+netdev, edumazet, kuba, pabeni, saikrishnag
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0
- [Medium] The added `if (!rvu->pf2cgxlmac_map) return false;` in
`is_pf_cgxmapped()`…
Pre-existing issues:
- [Medium] Asymmetry between the NIX backpressure enable and disable
mailbox handlers for SDP interfaces.
--
octeontx2-af: guard is_pf_cgxmapped() when PF to CGX map is absent
This adds a NULL check for rvu->pf2cgxlmac_map at the top of
is_pf_cgxmapped(), so the helper returns false when the PF to CGX LMAC map
was never allocated. The stated goal is to avoid dereferencing the map in
RPM-less configurations where the bootloader supplies no RPM/firmware data.
> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu.h b/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
> index 094227404ef9f..1e0a5aeb3f577 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
> @@ -970,6 +970,9 @@ static inline int rvu_get_pf(struct pci_dev *pdev, u16 pcifunc)
> /* CGX APIs */
> static inline bool is_pf_cgxmapped(struct rvu *rvu, u8 pf)
> {
> + if (!rvu->pf2cgxlmac_map)
> + return false;
> +
[Severity: Medium]
Can this new branch ever be taken?
struct rvu is zero allocated in rvu_probe():
rvu = devm_kzalloc(dev, sizeof(*rvu), GFP_KERNEL);
so cgx_mapped_pfs starts at 0. rvu_map_cgx_lmac_pf() in rvu_cgx.c is the
only writer of cgx_mapped_pfs, and both the reset and the increment happen
only after pf2cgxlmac_map and cgxlmac2pf_map have been allocated
successfully:
rvu->pf2cgxlmac_map = devm_kmalloc(rvu->dev, size, GFP_KERNEL);
if (!rvu->pf2cgxlmac_map)
return -ENOMEM;
...
rvu->cgx_mapped_pfs = 0;
...
rvu->cgx_mapped_pfs++;
In exactly the configuration the commit message describes, rvu_cgx_init()
returns early:
rvu->cgx_cnt_max = cgx_get_cgxcnt_max();
if (!rvu->cgx_cnt_max) {
dev_info(rvu->dev, "No CGX devices found!\n");
return 0;
}
which leaves pf2cgxlmac_map NULL and cgx_mapped_pfs at 0. In that state the
pre-existing range test:
return (pf >= PF_CGXMAP_BASE && pf <= rvu->cgx_mapped_pfs) &&
is already false for every u8 pf, since PF_CGXMAP_BASE is 1 and
cgx_mapped_pfs is 0, and the && short circuits before is_sdp_pf() runs. If
the allocation itself fails, rvu_map_cgx_lmac_pf() returns -ENOMEM and
rvu_probe() aborts:
err = rvu_cgx_init(rvu);
if (err) {
dev_err(rvu->dev, "%s: Failed to initialize cgx\n", __func__);
goto cgx_err;
}
so the state "pf2cgxlmac_map == NULL with cgx_mapped_pfs != 0" does not seem
to exist, and no is_pf_cgxmapped() guarded caller (rvu_cgx.c, rvu_nix.c,
rvu_npc.c, rvu_debugfs.c) can reach the map with a NULL pointer.
Given that, is the Fixes: 3a4fa841b039 tag appropriate here, and can the
commit message wording "Several call paths use is_pf_cgxmapped() before
indexing pf2cgxlmac_map[], but the helper did not verify that the map
exists" be reworded to describe this as hardening rather than a NULL
dereference fix?
If a crash was actually observed, was it on one of the unguarded
dereferences instead? nix_lf_free() indexes the map directly, gated only by
pfvf->hw_rx_tstamp_en:
rvu->pf2cgxlmac_map[pf]
That site is not covered by this change and would still be reachable.
> return (pf >= PF_CGXMAP_BASE && pf <= rvu->cgx_mapped_pfs) &&
> !is_sdp_pf(rvu, rvu_make_pcifunc(rvu->pdev, pf, 0));
> }
[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch, but
while looking at the callers of this helper: is the SDP handling in
nix_bp_disable() and nix_bp_enable() in rvu_nix.c intentionally asymmetric?
is_pf_cgxmapped() deliberately returns false for SDP PFs because of the
!is_sdp_pf() term above. nix_bp_enable() classifies SDP before its gate and
admits it explicitly:
type = is_lbk_vf(rvu, pcifunc) ? NIX_INTF_TYPE_LBK : NIX_INTF_TYPE_CGX;
if (is_sdp_pfvf(rvu, pcifunc))
type = NIX_INTF_TYPE_SDP;
/* Enable backpressure only for CGX mapped PFs and LBK/SDP interface */
if (!is_pf_cgxmapped(rvu, pf) && type != NIX_INTF_TYPE_LBK &&
type != NIX_INTF_TYPE_SDP)
return 0;
nix_bp_disable() applies the gate first, while type is still
NIX_INTF_TYPE_CGX:
type = is_lbk_vf(rvu, pcifunc) ? NIX_INTF_TYPE_LBK : NIX_INTF_TYPE_CGX;
if (!is_pf_cgxmapped(rvu, pf) && type != NIX_INTF_TYPE_LBK)
return 0;
if (is_sdp_pfvf(rvu, pcifunc))
type = NIX_INTF_TYPE_SDP;
For an SDP requester the early return fires, which makes the following
is_sdp_pfvf() assignment unreachable. Does this leave BIT(16) of
NIX_AF_RX_CHANX_CFG set on the SDP RX channels after NIX_BP_DISABLE returns
success, so backpressure stays armed once the interface is torn down?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915022952.886639-1-rkannoth%40marvell.com
^ permalink raw reply [flat|nested] 4+ messages in thread