netdev - Re: [PATCH net-next 02/11] net: hold netdev instance lock during ndo_setup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250211182016.305f1c77@kernel.org>
Date: Tue, 11 Feb 2025 18:20:16 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Stanislav Fomichev <sdf@...ichev.me>
Cc: netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
 pabeni@...hat.com, Saeed Mahameed <saeed@...nel.org>
Subject: Re: [PATCH net-next 02/11] net: hold netdev instance lock during
 ndo_setup_tc

On Mon, 10 Feb 2025 11:20:34 -0800 Stanislav Fomichev wrote:
> Introduce new dev_setup_tc that handles the details and call it from
> all qdiscs/classifiers. The instance lock is still applied only to
> the drivers that implement shaper API so only iavf is affected.

> +int dev_setup_tc(struct net_device *dev, enum tc_setup_type type,
> +		 void *type_data)
> +{
> +	const struct net_device_ops *ops = dev->netdev_ops;
> +
> +	ASSERT_RTNL();
> +
> +	if (tc_can_offload(dev) && ops->ndo_setup_tc) {
> +		int ret = -ENODEV;
> +
> +		if (netif_device_present(dev)) {
> +			netdev_lock_ops(dev);
> +			ret = ops->ndo_setup_tc(dev, type, type_data);
> +			netdev_unlock_ops(dev);
> +		}
> +
> +		return ret;
> +	}
> +
> +	return -EOPNOTSUPP;

Why the indent? IMHO this would be cleaner:

	if (!tc_can_offload || !ops...
		return -ENOPNOTSUPP;
	if (!netif_device_present(dev))
		return -ENODEV:

	netdev_lock_ops(dev);
	...

> diff --git a/net/dsa/user.c b/net/dsa/user.c
> index 291ab1b4acc4..f2ac7662e4cc 100644
> --- a/net/dsa/user.c
> +++ b/net/dsa/user.c
> @@ -1729,10 +1729,7 @@ static int dsa_user_setup_ft_block(struct dsa_switch *ds, int port,
>  {
>  	struct net_device *conduit = dsa_port_to_conduit(dsa_to_port(ds, port));
>  
> -	if (!conduit->netdev_ops->ndo_setup_tc)
> -		return -EOPNOTSUPP;
> -
> -	return conduit->netdev_ops->ndo_setup_tc(conduit, TC_SETUP_FT, type_data);
> +	return dev_setup_tc(conduit, TC_SETUP_FT, type_data);

The netfilter / flow table offloads don't seem to test tc_can_offload(),
should we make that part of the check optional in dev_setup_tc() ?
Add a bool argument to ignore  tc_can_offload() ?

> @@ -855,10 +853,7 @@ void qdisc_offload_graft_helper(struct net_device *dev, struct Qdisc *sch,
>  	bool any_qdisc_is_offloaded;
>  	int err;
>  
> -	if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc)
> -		return;
> -
> -	err = dev->netdev_ops->ndo_setup_tc(dev, type, type_data);
> +	err = dev_setup_tc(dev, type, type_data);

Probably need to handle -EOPNOTSUPP here now?

>  	/* Don't report error if the graft is part of destroy operation. */
>  	if (!err || !new || new == &noop_qdisc)

> -	err = ops->ndo_setup_tc(dev, TC_SETUP_QDISC_CBS, &cbs);
> +	err = dev_setup_tc(dev, TC_SETUP_QDISC_CBS, &cbs);
>  	if (err < 0)
>  		pr_warn("Couldn't disable CBS offload for queue %d\n",
>  			cbs.queue);
> @@ -294,7 +289,7 @@ static int cbs_enable_offload(struct net_device *dev, struct cbs_sched_data *q,
>  	cbs.idleslope = opt->idleslope;
>  	cbs.sendslope = opt->sendslope;
>  
> -	err = ops->ndo_setup_tc(dev, TC_SETUP_QDISC_CBS, &cbs);
> +	err = dev_setup_tc(dev, TC_SETUP_QDISC_CBS, &cbs);

$ for f in $(git grep --files-with-matches TC_SETUP_QDISC_CBS -- drivers/ ); do \
	d=$(dirname $f); \
	git grep HW_TC -- $d || echo No match in $d; \
done

No match in drivers/net/dsa/microchip
No match in drivers/net/dsa/ocelot
No match in drivers/net/dsa/sja1105
drivers/net/ethernet/freescale/enetc/enetc_pf.c:        if (changed & NETIF_F_HW_TC) {
drivers/net/ethernet/freescale/enetc/enetc_pf.c:                err = enetc_set_psfp(ndev, !!(features & NETIF_F_HW_TC));
drivers/net/ethernet/freescale/enetc/enetc_pf_common.c:         ndev->features |= NETIF_F_HW_TC;
drivers/net/ethernet/freescale/enetc/enetc_pf_common.c:         ndev->hw_features |= NETIF_F_HW_TC;
drivers/net/ethernet/intel/igb/igb_main.c:              netdev->features |= NETIF_F_HW_TC;
drivers/net/ethernet/intel/igc/igc_main.c:      netdev->features |= NETIF_F_HW_TC;
drivers/net/ethernet/microchip/lan966x/lan966x_main.c:                   NETIF_F_HW_TC;
drivers/net/ethernet/microchip/lan966x/lan966x_main.c:  dev->hw_features |= NETIF_F_HW_TC;
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:              ndev->hw_features |= NETIF_F_HW_TC;
drivers/net/ethernet/ti/am65-cpsw-nuss.c:                                 NETIF_F_HW_TC;
drivers/net/ethernet/ti/cpsw_new.c:                               NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_TC;


Looks like some the these qdiscs will need to ignore the features too :(