netdev - Re: [RFC PATCH net-next] net: dsa: replace NETDEV_PRE_CHANGE

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: <ZC3Ue5i/zjZkvMGy@corigine.com>
Date:   Wed, 5 Apr 2023 22:05:15 +0200
From:   Simon Horman <simon.horman@...igine.com>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     netdev@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>, Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [RFC PATCH net-next] net: dsa: replace
 NETDEV_PRE_CHANGE_HWTSTAMP notifier with a stub

On Wed, Apr 05, 2023 at 07:51:15PM +0300, Vladimir Oltean wrote:
> There was a sort of rush surrounding commit 88c0a6b503b7 ("net: create a
> netdev notifier for DSA to reject PTP on DSA master"), due to a desire
> to convert DSA's attempt to deny TX timestamping on a DSA master to
> something that doesn't block the kernel-wide API conversion from
> ndo_eth_ioctl() to ndo_hwtstamp_set().
> 
> What was required was a mechanism that did not depend on ndo_eth_ioctl(),
> and what was provided was a mechanism that did not depend on
> ndo_eth_ioctl(), while at the same time introducing something that
> wasn't absolutely necessary - a new netdev notifier.
> 
> There have been objections from Jakub Kicinski that using notifiers in
> general when they are not absolutely necessary creates complications to
> the control flow and difficulties to maintainers who look at the code.
> So there is a desire to not use notifiers.
> 
> Take the model of udp_tunnel_nic_ops and introduce a stub mechanism,
> through which net/core/dev_ioctl.c can call into DSA even when
> CONFIG_NET_DSA=m.
> 
> Compared to the code that existed prior to the notifier conversion, aka
> what was added in commits:
> - 4cfab3566710 ("net: dsa: Add wrappers for overloaded ndo_ops")
> - 3369afba1e46 ("net: Call into DSA netdevice_ops wrappers")
> 
> this is different because we are not overloading any struct
> net_device_ops of the DSA master anymore, but rather, we are exposing a
> rather specific functionality which is orthogonal to which API is used
> to enable it - ndo_eth_ioctl() or ndo_hwtstamp_set().
> 
> Also, what is similar is that both approaches use function pointers to
> get from built-in code to DSA.
> 
> Since the new functionality does not overload the NDO of any DSA master,
> there is no point in replicating the function pointers towards
> __dsa_master_hwtstamp_validate() once for every CPU port (dev->dsa_ptr).
> But rather, it is fine to introduce a singleton struct dsa_stubs,
> built-in to the kernel, which contains a single function pointer to
> __dsa_master_hwtstamp_validate().
> 
> I find this approach rather preferable to what we had originally,
> because dev->dsa_ptr->netdev_ops->ndo_do_ioctl() used to require going
> through struct dsa_port (dev->dsa_ptr), and so, this was incompatible
> with any attempts to add any data encapsulation and hide DSA data
> structures from the outside world.
> 
> Link: https://lore.kernel.org/netdev/20230403083019.120b72fd@kernel.org/
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>

...

> diff --git a/include/net/dsa_stubs.h b/include/net/dsa_stubs.h
> new file mode 100644
> index 000000000000..27a1ad85c038
> --- /dev/null
> +++ b/include/net/dsa_stubs.h
> @@ -0,0 +1,49 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * include/net/dsa_stubs.h - Stubs for the Distributed Switch Architecture framework
> + */
> +
> +#include <linux/mutex.h>
> +#include <linux/netdevice.h>
> +#include <linux/net_tstamp.h>
> +#include <net/dsa.h>
> +
> +#if IS_ENABLED(CONFIG_NET_DSA)
> +
> +extern const struct dsa_stubs *dsa_stubs;
> +extern struct mutex dsa_stubs_lock;
> +
> +struct dsa_stubs {
> +	int (*master_hwtstamp_validate)(struct net_device *dev,
> +					const struct kernel_hwtstamp_config *config,
> +					struct netlink_ext_ack *extack);
> +};
> +
> +static inline int dsa_master_hwtstamp_validate(struct net_device *dev,
> +					       const struct kernel_hwtstamp_config *config,
> +					       struct netlink_ext_ack *extack)
> +{
> +	int err;
> +
> +	if (!netdev_uses_dsa(dev))
> +		return 0;
> +
> +	mutex_lock(&dsa_stubs_lock);
> +
> +	if (dsa_stubs)
> +		err = dsa_stubs->master_hwtstamp_validate(dev, config, extack);
> +
> +	mutex_unlock(&dsa_stubs_lock);

nit: clang-16 tells me that err is uninitialised here if dsa_stubs is false.

> +
> +	return err;
> +}
> +
> +#else

...

> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 8abc1658ac47..36bb7533ddd6 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -3419,16 +3419,6 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb,
>  
>  		return NOTIFY_OK;
>  	}
> -	case NETDEV_PRE_CHANGE_HWTSTAMP: {
> -		struct netdev_notifier_hwtstamp_info *info = ptr;
> -		int err;
> -
> -		if (!netdev_uses_dsa(dev))
> -			return NOTIFY_DONE;
> -
> -		err = dsa_master_pre_change_hwtstamp(dev, info->config, extack);

nit: clang-16 tell me that extack is now unused in this function.

> -		return notifier_from_errno(err);
> -	}
>  	default:
>  		break;
>  	}