Re: [PATCH v18 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 2 potential issue(s) to consider:
- [High] tc: use-after-free in otx2_mqprio_netdev_tc_work due to premature work cancellation
- [Medium] tc: mqprio offload state leak blocks future channel configuration
--

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

octeontx2: add mqprio bandwidth offload for NIX TX schedulers

This commit 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 introduces a work item for
restoring TC configurations.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c
[ ... ]
> @@ -287,6 +287,21 @@ static int otx2_set_channels(struct net_device *dev,
>  		return -EINVAL;
>  	}
>  
> +	if (pfvf->mqprio.rate_limit &&
> +	    (channel->tx_count != pfvf->hw.tx_queues ||
> +	     channel->rx_count != pfvf->hw.rx_queues)) {
> +		netdev_info(dev,
> +			    "Not permitted to change channel count while MQ prio is active\n");
> +		return -EINVAL;
> +	}
> +
> +	if ((pfvf->old_mq_snap || pfvf->cur_mq_snap) &&
> +	    channel->tx_count < pfvf->hw.tx_queues) {
> +		netdev_err(dev,
> +			   "Cannot reduce TX queues after mqprio bandwidth offload was configured\n");
> +		return -EINVAL;
> +	}

[Severity: Medium]
Because pfvf->old_mq_snap is never cleared upon qdisc deletion (as seen
in otx2_mqprio_clear_sw below), will this permanently block reducing TX
queues via ethtool until the driver is reloaded?

[ ... ]
> 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
[ ... ]
> @@ -3622,7 +3641,9 @@ static void otx2_remove(struct pci_dev *pdev)
>  	otx2_cgx_config_linkevents(pf, false);
>  
>  	otx2_unregister_dl(pf);
> +	otx2_shutdown_tc_mqprio(pf);
>  	unregister_netdev(netdev);

[Severity: High]
Could calling otx2_shutdown_tc_mqprio() before unregister_netdev() lead
to a use-after-free?

Concurrent netlink operations might queue work via
otx2_mqprio_defer_netdev_tc_restore() while waiting on rtnl_lock(). Once
the remove path finishes unregistering and frees the netdev, the deferred
work could execute, access the freed pfvf->netdev, and corrupt memory.

[ ... ]
> 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
[ ... ]
> +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]
Will this leak pfvf->old_mq_snap when the qdisc is deleted? The teardown
path clears the software state by calling otx2_mqprio_clear_sw(), but
neither this function nor otx2_teardown_tc_mqprio() frees the snapshot.

[ ... ]
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
[ ... ]
> @@ -840,7 +841,9 @@ static void otx2vf_remove(struct pci_dev *pdev)
>  #endif
>  
>  	otx2_unregister_dl(vf);
> +	otx2_shutdown_tc_mqprio(vf);
>  	unregister_netdev(netdev);

[Severity: High]
Does this exhibit the same use-after-free risk as the PF remove path
above, where pending tc setup work could be deferred and executed after
unregister_netdev() has completed?

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