Re: [PATCH v16 net-next 2/2] octeontx2-pf: add mqprio bandwidth offload for NIX TX schedulers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] net: octeontx2: TOCTOU race in otx2_mqprio_netdev_tc_work() applies stale TC map
- [Medium] net: octeontx2: Fragile error rollback in otx2_mqprio_restore_old() risks ENOMEM
--

commit 422c585ce4a53f6ab1d9b75ad9b1360545a98293
Author: Ratheesh Kannoth <rkannoth@marvell.com>

octeontx2-pf: add mqprio bandwidth offload for NIX TX schedulers

Adds TC_SETUP_QDISC_MQPRIO offload for channel-mode mqprio with rate shaping
on PF and VF RVU netdevices. It programs per-queue MDQ CIR/PIR for non-QoS
transmit queues via the NIX TX scheduler mailbox and bounces the netdev to
rebuild the TX scheduler hierarchy on configuration changes.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> index ddb46b580c3b6..edd7c02efb47f 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
[ ... ]
> @@ -1600,14 +1620,802 @@ static int otx2_setup_tc_block(struct net_device *netdev,
[ ... ]
> +static void otx2_mqprio_netdev_tc_work(struct work_struct *work)
> +{
> +	struct otx2_mqprio *mqprio = container_of(work, struct otx2_mqprio,
> +						  netdev_tc_work);
> +	struct otx2_nic *pfvf = container_of(mqprio, struct otx2_nic, mqprio);
> +
> +	if (!pfvf->mqprio.rate_limit || !pfvf->old_mq_snap)
> +		return;
> +
> +	rtnl_lock();
> +	otx2_mqprio_apply_snap_netdev(pfvf->netdev, pfvf->old_mq_snap);
> +	rtnl_unlock();
> +}

[Severity: Medium]
Could this lead to a race condition against a concurrent tc qdisc deletion?

In otx2_mqprio_netdev_tc_work, the shared state pfvf->mqprio.rate_limit is
evaluated outside the rtnl_lock, but the state application is done inside it.
When an mqprio replace operation fails, it schedules this workqueue to
asynchronously restore the old netdev TC mapping.

If an administrator concurrently issues a tc qdisc del, the delete operation
holds rtnl_lock, clears the TC mappings to 0, and disables offload. Meanwhile,
the workqueue thread locklessly checks pfvf->mqprio.rate_limit, finds it
true, and waits for rtnl_lock.

Once the delete finishes, the workqueue acquires the lock and unconditionally
re-applies the old TC mapping onto the now-cleared netdev. This leaves the
netdev's TC routing state incorrectly mapped (num_tc > 0) even after the
mqprio qdisc has been removed, potentially misrouting or dropping traffic.

Should this condition be checked inside the RTNL critical section instead?

[ ... ]
> +static int otx2_mqprio_restore_old(struct otx2_nic *pfvf)
> +{
> +	struct mq_offload_snap *snap = pfvf->old_mq_snap;
> +	struct net_device *netdev = pfvf->netdev;
> +	u16 num_txq = pfvf->hw.non_qos_queues;
> +	int tc, txq, err;
> +
> +	if (!snap)
> +		return 0;
> +
> +	err = otx2_mqprio_alloc_cache(pfvf, false);
> +	if (err)
> +		return err;

[Severity: Medium]
Can this trigger an ENOMEM failure during error recovery?

When otx2_setup_tc_mqprio fails (e.g., during hardware programming), it
invokes otx2_mqprio_restore_old to revert. This function then calls
otx2_mqprio_alloc_cache(pfvf, false).

Passing false for the replacing parameter deliberately bypasses the
cache-reuse optimization, forcing an unnecessary devm_kcalloc allocation.
Because this error recovery runs when operations fail, which can coincide with
memory pressure, this introduces a fallible allocation step into a critical
unwind path.

If memory allocation fails during this error rollback, the rollback aborts,
leaving the hardware and software states inconsistent (hardware contains
partial new limits, software is missing the restored limits).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260918015906.1255204-1-rkannoth@marvell.com?part=2