[PATCH net] octeontx2-af: validate MCS blocks before initialization

[PATCH net] octeontx2-af: validate MCS blocks before initialization

[Slavin Liu]

The MCS count is a maximum successful ID plus one, not proof of a
dense device set. Validate the complete range before programming any
block, so a failed lower-ID probe cannot cause a NULL dereference.

Detected by static analysis and reviewed with AI-assisted source auditing.

Fixes: 080bbd19c9dd ("octeontx2-af: cn10k: mcs: Add mailboxes for port related operations")
Assisted-by: LLM
Signed-off-by: Slavin Liu <bolin.liu@seu.edu.cn>
---
 drivers/net/ethernet/marvell/octeontx2/af/mcs_rvu_if.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/marvell/octeontx2/af/mcs_rvu_if.c b/drivers/net/ethernet/marvell/octeontx2/af/mcs_rvu_if.c
index fce22e314cac..0ee2d0d698e5 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/mcs_rvu_if.c
+++ b/drivers/net/ethernet/marvell/octeontx2/af/mcs_rvu_if.c
@@ -889,6 +889,11 @@ int rvu_mcs_init(struct rvu *rvu)
 		rvu_mcs_set_lmac_bmap(rvu);
 	}
 
+	for (mcs_id = 0; mcs_id < rvu->mcs_blk_cnt; mcs_id++) {
+		if (!mcs_get_pdata(mcs_id))
+			return -ENODEV;
+	}
+
 	/* Install default tcam bypass entry and set port to operational mode */
 	for (mcs_id = 0; mcs_id < rvu->mcs_blk_cnt; mcs_id++) {
 		mcs = mcs_get_pdata(mcs_id);

Re: [PATCH net] octeontx2-af: validate MCS blocks before initialization

[Ratheesh Kannoth]

On 2026-09-13 at 18:22:06, Slavin Liu (bolin.liu@seu.edu.cn) wrote:
> The MCS count is a maximum successful ID plus one, not proof of a
> dense device set. Validate the complete range before programming any
> block, so a failed lower-ID probe cannot cause a NULL dereference.
>
> Detected by static analysis and reviewed with AI-assisted source auditing.
>
> Fixes: 080bbd19c9dd ("octeontx2-af: cn10k: mcs: Add mailboxes for port related operations")
> Assisted-by: LLM
> Signed-off-by: Slavin Liu <bolin.liu@seu.edu.cn>
> ---
>  drivers/net/ethernet/marvell/octeontx2/af/mcs_rvu_if.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/mcs_rvu_if.c b/drivers/net/ethernet/marvell/octeontx2/af/mcs_rvu_if.c
> index fce22e314cac..0ee2d0d698e5 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/mcs_rvu_if.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/mcs_rvu_if.c
> @@ -889,6 +889,11 @@ int rvu_mcs_init(struct rvu *rvu)
>  		rvu_mcs_set_lmac_bmap(rvu);
>  	}
>
> +	for (mcs_id = 0; mcs_id < rvu->mcs_blk_cnt; mcs_id++) {
> +		if (!mcs_get_pdata(mcs_id))
> +			return -ENODEV;
> +	}
Do we need to move this up to apply for cn10kb as well ?
> +
>  	/* Install default tcam bypass entry and set port to operational mode */
>  	for (mcs_id = 0; mcs_id < rvu->mcs_blk_cnt; mcs_id++) {
>  		mcs = mcs_get_pdata(mcs_id);

Re: [PATCH net] octeontx2-af: validate MCS blocks before initialization

[netdev-bot+sashiko]

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

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

- [High] The new `return -ENODEV` in rvu_mcs_init()
  (mcs_rvu_if.c:892-896) aborts before mcs->rvu, mcs->pf, mcs->vf are…
- [High] The patch introduces a Time-Of-Check to Time-Of-Use (TOCTOU)
  race by locklessly pre-checking mcs devices before a subsequent…

Pre-existing issues:
- [High] mcs_remove() (mcs.c:1609-1620) never unlinks the block from the
  global list: it calls mcs_set_external_bypass(), free_irq(),…

--

octeontx2-af: validate MCS blocks before initialization

This adds a pre-pass in rvu_mcs_init() that walks every MCS block id in
[0, rvu->mcs_blk_cnt) and returns -ENODEV if mcs_get_pdata() returns NULL
for any of them, before the loop that programs the blocks.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/mcs_rvu_if.c b/drivers/net/ethernet/marvell/octeontx2/af/mcs_rvu_if.c
> index fce22e314cac0..0ee2d0d698e5c 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/mcs_rvu_if.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/mcs_rvu_if.c
> @@ -889,6 +889,11 @@ int rvu_mcs_init(struct rvu *rvu)
>  		rvu_mcs_set_lmac_bmap(rvu);
>  	}
>  
> +	for (mcs_id = 0; mcs_id < rvu->mcs_blk_cnt; mcs_id++) {
> +		if (!mcs_get_pdata(mcs_id))
> +			return -ENODEV;
> +	}

[Severity: High]
The check and the use are two independent walks of the same
unsynchronized list, so what does the pre-pass actually buy us?

mcs_get_pdata() in mcs.c walks the file-global mcs_list with a plain
list_for_each_entry() and no lock or RCU, and mcs_probe() links onto
that list with a bare list_add(). Nothing in rvu_mcs_init() pins the
blocks between the new validation loop and the programming loop below
it, which re-fetches the pointer:

	mcs = mcs_get_pdata(mcs_id);
	mcs_install_flowid_bypass_entry(mcs);

So the new loop only establishes that every id in [0, mcs_blk_cnt) was
resolvable at the moment it ran. If an MCS PCI function goes away in
the window - "echo <mcs-bdf> > /sys/bus/pci/drivers/mcs/unbind" while
AF probe is in progress - the second mcs_get_pdata() for that id can
still hand back NULL and mcs_install_flowid_bypass_entry(mcs) will
dereference it immediately, which is exactly the crash the commit
message says it prevents. The rest of the loop body (mcs->hw->lmac_cnt,
mcs->rvu, mcs->dev in the devm_kcalloc() calls) has the same problem.

Folding the NULL test into the existing programming loop, so the
pointer that is checked is the pointer that is used, would at least
remove the second lookup and is a strictly smaller change. That still
leaves the underlying question of how mcs_list is meant to be
serialized against driver unbind - is there an intended lock or
refcount here, or is the assumption that rvu_mcs_init() cannot run
concurrently with mcs_remove() at all? If it is the latter, please
spell that assumption out, because the pre-pass reads as if it were
defending against exactly that race.

While you are in here: rvu_mcs_set_lmac_bmap() above the new loop
already does mcs_get_pdata(0) and dereferences mcs->hw->lmac_cnt with
no check, so on the CN10K-B (mcs_blk_cnt == 1) path a missing block
faults before the new validation is ever reached. Should the check
move ahead of that call instead?

[Severity: High]
Does this early return leave the already-probed MCS blocks with live
interrupts pointing at uninitialized AF state?

The new return happens before the second loop assigns mcs->rvu, mcs->pf
and mcs->vf, and before INIT_LIST_HEAD(&rvu->mcs_intrq_head), INIT_WORK()
and alloc_workqueue("mcs_intr_wq") run.

Meanwhile every block that did probe already armed its interrupts in its
own probe, mcs_probe() -> mcs_register_interrupts(), which does
request_irq(..., mcs_ip_intr_handler, 0, "MCS_IP", mcs) and writes
MCSX_IP_INT_ENA_W1S / MCSX_CPM_RX_SLAVE_RX_INT_ENB, and only then does
list_add(&mcs->mcs_list, &mcs_list).

So after the -ENODEV, an MCS CPM interrupt would run:

mcs_ip_intr_handler()
  mcs_rx_misc_intr_handler()
    mcs_add_intr_wq_entry()

and mcs_add_intr_wq_entry() has no NULL guard:

	struct rvu *rvu = mcs->rvu;
	...
		pfvf = &mcs->pf[rvu_get_pf(rvu->pdev, pcifunc)];
	...
	list_add_tail(&qentry->node, &rvu->mcs_intrq_head);
	queue_work(rvu->mcs_intr_wq, &rvu->mcs_intr_work);

mcs->rvu is still NULL here since struct mcs is devm_kzalloc()'ed and
mcs->rvu is only set in the loop below the new check.

On the AF side, rvu_setup_hw_resources() does:

	err = rvu_mcs_init(rvu);
	if (err) {
		dev_err(rvu->dev, "%s: Failed to initialize mcs\n", __func__);
		goto nix_err;
	}

The nix_err path never calls rvu_mcs_exit(), and mcs_driver stays bound to
the MCS PCI functions, so nothing disables the interrupt enables that were
just programmed. Links and RX were already enabled by cgx_start_linkup()
earlier in rvu_setup_hw_resources(), and the surviving blocks are not in
external bypass, so received traffic carrying a SecTAG can still raise
MCS_CPM_RX_INT_SECTAG_V_EQ1.

The commit message says "so a failed lower-ID probe cannot cause a NULL
dereference". In that exact configuration, does the dereference just move
from probe time to interrupt time rather than going away? Would it make
sense to also guard mcs_add_intr_wq_entry() on !mcs->rvu / !mcs->pf, or to
disarm the probed blocks before returning?

[Severity: High]
This isn't a bug introduced by this patch, but is a non-NULL result from
mcs_get_pdata() actually proof that the block is live?

mcs_remove() does not unlink the block from the global list:

	static void mcs_remove(struct pci_dev *pdev)
	{
		struct mcs *mcs = pci_get_drvdata(pdev);

		/* Set MCS to external bypass */
		mcs_set_external_bypass(mcs, true);
		free_irq(pci_irq_vector(pdev, mcs->hw->ip_vec), mcs);
		pci_free_irq_vectors(pdev);
		pci_release_regions(pdev);
		pci_disable_device(pdev);
		pci_set_drvdata(pdev, NULL);
	}

There is no list_del(&mcs->mcs_list) matching the list_add(&mcs->mcs_list,
&mcs_list) in mcs_probe(). Both mcs and mcs->hw are devm_kzalloc()'ed on
the MCS PCI device, so devres frees them right after mcs_remove() returns
and a freed node stays linked in mcs_list. cgx_remove() does the
list_del(&cgx->cgx_list) that mcs_remove() omits.

mcs_get_pdata() then hands out that dangling pointer:

	list_for_each_entry(mcs_dev, &mcs_list, mcs_list) {
		if (mcs_dev->mcs_id == mcs_id)
			return mcs_dev;
	}
	return NULL;

So after "echo <mcs-bdf> > /sys/bus/pci/drivers/mcs/unbind", does the new
check distinguish "never probed" from "probed then freed"? The following
programming loop would then touch mcs->hw->lmac_cnt and mcs->dev on freed
memory, and the mailbox handlers that only validate
req->mcs_id < rvu->mcs_blk_cnt before touching mcs->hw, mcs->mcs_ops,
mcs->tx_sa_active and mutex_lock(&mcs->stats_lock) would do the same, with
MMIO going through a released pcim_iomap() mapping.

>  	/* Install default tcam bypass entry and set port to operational mode */
>  	for (mcs_id = 0; mcs_id < rvu->mcs_blk_cnt; mcs_id++) {
>  		mcs = mcs_get_pdata(mcs_id);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260913125206.109985-1-bolin.liu%40seu.edu.cn

RE: [EXTERNAL] [PATCH net] octeontx2-af: validate MCS blocks before initialization

[Geethasowjanya Akula]

>-----Original Message-----
>From: Slavin Liu <bolin.liu@seu.edu.cn>
>Sent: Sunday, September 13, 2026 6:22 PM
>To: Sunil Kovvuri Goutham <sgoutham@marvell.com>; Ratheesh Kannoth
><rkannoth@marvell.com>; Geethasowjanya Akula <gakula@marvell.com>;
>Subbaraya Sundeep Bhatta <sbhatta@marvell.com>;
>andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com;
>kuba@kernel.org; pabeni@redhat.com
>Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
>bolin.liu@seu.edu.cn
>Subject: [EXTERNAL] [PATCH net] octeontx2-af: validate MCS blocks before
>initialization
>The MCS count is a maximum successful ID plus one, not proof of a dense
>device set. Validate the complete range before programming any block, so a
>failed lower-ID probe cannot cause a NULL dereference.
>
>Detected by static analysis and reviewed with AI-assisted source auditing.
>
>Fixes: 080bbd19c9dd ("octeontx2-af: cn10k: mcs: Add mailboxes for port
>related operations")
>Assisted-by: LLM
>Signed-off-by: Slavin Liu <bolin.liu@seu.edu.cn>
>---
> drivers/net/ethernet/marvell/octeontx2/af/mcs_rvu_if.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
>diff --git a/drivers/net/ethernet/marvell/octeontx2/af/mcs_rvu_if.c
>b/drivers/net/ethernet/marvell/octeontx2/af/mcs_rvu_if.c
>index fce22e314cac..0ee2d0d698e5 100644
>--- a/drivers/net/ethernet/marvell/octeontx2/af/mcs_rvu_if.c
>+++ b/drivers/net/ethernet/marvell/octeontx2/af/mcs_rvu_if.c
>@@ -889,6 +889,11 @@ int rvu_mcs_init(struct rvu *rvu)
> 		rvu_mcs_set_lmac_bmap(rvu);
> 	}
>
>+	for (mcs_id = 0; mcs_id < rvu->mcs_blk_cnt; mcs_id++) {
>+		if (!mcs_get_pdata(mcs_id))
>+			return -ENODEV;
>+	}
>+
> 	/* Install default tcam bypass entry and set port to operational mode
>*/
> 	for (mcs_id = 0; mcs_id < rvu->mcs_blk_cnt; mcs_id++) {
> 		mcs = mcs_get_pdata(mcs_id);

Thanks, Slavin, for the patch.
The additional check can be moved into the existing loop below, which avoids iterating over the MCS blocks twice.

Also, on CN10KB silicon, the MCS block count is always 1. Therefore, the existing check: "if (!rvu->mcs_blk_cnt)" 
should be sufficient to handle the absence of MCS blocks.
          
@@ -892,6 +892,8 @@ int rvu_mcs_init(struct rvu *rvu)
        /* Install default tcam bypass entry and set port to operational mode */
        for (mcs_id = 0; mcs_id < rvu->mcs_blk_cnt; mcs_id++) {
                mcs = mcs_get_pdata(mcs_id);
+               if (!mcs_get_pdata(mcs_id))
+                       return -ENODEV
                mcs_install_flowid_bypass_entry(mcs);