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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] net: Use-After-Free due to TOCTOU race in workqueue cancellation during device removal
- [High] tc: heap Out-of-Bounds write in otx2_mqprio_restore_old via XDP detach
- [Medium] net: permanent lock-out of TX queue reduction due to old_mq_snap state leak
- [Medium] tc: otx2_tc_can_offload() incorrectly evaluates hw_features instead of features
- [Medium] tc: inappropriate use of devm_ allocations for dynamic runtime qdisc configuration
--

commit 1bd3302261ac3d0844dd5a4c5059dd408da6cacc
Author: Ratheesh Kannoth <rkannoth@marvell.com>
octeontx2: add mqprio bandwidth offload for NIX TX schedulers

This patch adds TC_SETUP_QDISC_MQPRIO offload with TC_MQPRIO_SHAPER_BW_RATE
on PF and VF netdevs. It programs per-queue MDQ CIR/PIR via the NIX TX
scheduler mailbox and reconfigures the netdev to apply the changes.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> @@ -2924,6 +2935,12 @@ static int otx2_xdp_setup(struct otx2_nic *pf, struct bpf_prog *prog)
>  	bool if_up = netif_running(pf->netdev);
>  	struct bpf_prog *old_prog;
>  
> +	if (prog && pf->mqprio.rate_limit) {
> +		netdev_err(dev,
> +			   "XDP: cannot attach while mqprio bandwidth offload is active\n");
> +		return -EOPNOTSUPP;
> +	}

[Severity: High]
Does this condition inadvertently allow XDP detach to proceed while MQPRIO
is active? If prog is NULL during a detach, this check is bypassed.
Detaching XDP decreases hw.non_qos_queues, which later leads to an
out-of-bounds array access in otx2_mqprio_restore_old() when restoring the
old layout.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> @@ -1600,14 +1619,907 @@ static int otx2_setup_tc_block(struct net_device *netdev,
[ ... ]
> +static int otx2_mqprio_alloc_cache(struct otx2_nic *pfvf, bool replacing)
> +{
> +	u16 num_txq = pfvf->hw.non_qos_queues;
> +	u64 *min_rate, *max_rate;
[ ... ]
> +	min_rate = devm_kcalloc(pfvf->dev, num_txq, sizeof(*min_rate), GFP_KERNEL);
> +	max_rate = devm_kcalloc(pfvf->dev, num_txq, sizeof(*max_rate), GFP_KERNEL);

[Severity: Medium]
Are devm_kcalloc() and devm_kfree() appropriate here? Since
otx2_mqprio_alloc_cache() is called during arbitrary runtime configurations
like adding or replacing a qdisc, using devres allocations will repeatedly
append nodes to the device devres list and require O(N) traversals on every
update. Should this use standard kcalloc() instead to tie the cache
lifecycle to the qdisc rather than the device probe/remove lifecycle?

> +static int otx2_mqprio_snap_copy(struct otx2_nic *pfvf,
> +				 struct mq_offload_snap **dst,
> +				 const struct tc_mqprio_qopt_offload *mqprio)
> +{
[ ... ]
> +	if (!*dst) {
> +		snap = devm_kzalloc(pfvf->dev, sizeof(*snap), GFP_KERNEL);

[Severity: Medium]
Similar to the rate cache allocations, does this dynamic allocation during
runtime need to use devm_kzalloc()? Repeated qdisc modifications might
endlessly append to the devres list.

> +static void otx2_mqprio_defer_netdev_tc_restore(struct otx2_nic *pfvf)
> +{
> +	if (READ_ONCE(pfvf->mqprio.defer_tc_work))
> +		return;
> +	schedule_work(&pfvf->mqprio.netdev_tc_work);
> +}

[Severity: High]
Could this lockless check introduce a TOCTOU race during device removal? If
the shutdown thread in otx2_shutdown_tc_mqprio() sets defer_tc_work and
calls cancel_work_sync() locklessly exactly after this check evaluates to
false but before schedule_work() runs, the work could be queued after the
cancellation attempt.

> +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;
[ ... ]
> +	for (tc = 0; tc < snap->num_tc; tc++) {
> +		u64 min_rate = snap->min_rate[tc];
> +		u64 max_rate = snap->max_rate[tc];
> +
> +		for (txq = snap->offset[tc];
> +		     txq < snap->offset[tc] + snap->count[tc]; txq++) {
> +			pfvf->mqprio.min_rate[txq] = min_rate;
> +			pfvf->mqprio.max_rate[txq] = max_rate;

[Severity: High]
Could this loop write out-of-bounds into the min_rate and max_rate arrays if
the number of active queues is reduced while MQPRIO is active?

If an XDP program is detached, hw.non_qos_queues shrinks.
otx2_mqprio_alloc_cache() will then allocate smaller arrays based on the
new num_txq count, but this loop restores rates using the old, larger indices
stored in snap->offset and snap->count.

> +static void otx2_mqprio_clear_sw(struct otx2_nic *pfvf)
> +{
> +	struct net_device *netdev = pfvf->netdev;
> +
> +	pfvf->mqprio.rate_limit = false;
> +	otx2_mqprio_clear_replace_state(pfvf);
> +	if (!otx2_mqprio_keep_netdev_tc(pfvf))
> +		netdev_set_num_tc(netdev, 0);
> +	otx2_mqprio_free_cache(pfvf);
> +}

[Severity: Medium]
Is there a state leak here for pfvf->old_mq_snap? This teardown path clears
the rate cache and resets rate_limit, but it leaves old_mq_snap allocated.
Later, otx2_set_channels() permanently blocks TX queue reduction because
old_mq_snap is still non-NULL, effectively locking users out of modifying
channel counts via ethtool after removing an mqprio qdisc.

> +static bool otx2_tc_can_offload(struct net_device *netdev)
> +{
> +	return !!(netdev->hw_features & NETIF_F_HW_TC);
> +}

[Severity: Medium]
Should this check netdev->features instead of netdev->hw_features? By
evaluating device capabilities rather than the currently enabled features,
this allows mqprio setup to succeed even if the user explicitly disabled
hardware TC offload via ethtool.

> +void otx2_shutdown_tc_mqprio(struct otx2_nic *nic)
> +{
> +	WRITE_ONCE(nic->mqprio.defer_tc_work, true);
> +	/* Publish defer_tc_work before cancel_work_sync(). */
> +	smp_wmb();
> +	cancel_work_sync(&nic->mqprio.netdev_tc_work);
> +	rtnl_lock();

[Severity: High]
This pairs with the lockless check in otx2_mqprio_defer_netdev_tc_restore().
By setting the flag and calling cancel_work_sync() outside of rtnl_lock(),
it leaves a race window where work might be scheduled immediately after
cancellation completes. When the device is later unregistered and freed,
the lingering queued work could execute and trigger a use-after-free on
the freed device structure.

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