netdev - Re: [PATCH v2 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: <93ae54eb-5287-8d63-5109-973bfacc6b74@gmail.com>
Date:   Thu, 6 Apr 2023 06:07:45 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Vladimir Oltean <vladimir.oltean@....com>, netdev@...r.kernel.org,
        Jakub Kicinski <kuba@...nel.org>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>, Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH v2 net-next] net: dsa: replace NETDEV_PRE_CHANGE_HWTSTAMP
 notifier with a stub



On 4/6/2023 4:42 AM, 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.

Jakub is there a general desire to move away from notifiers? If so, do 
you have a list of things that may no longer belong there?

> 
> In addition to that, the notifier chain gets called even if there is no
> DSA in the system and no one is interested in applying any restriction.
> 
> 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.
> 
> There is no point in replicating the function pointers towards
> __dsa_master_hwtstamp_validate() once for every CPU port (dev->dsa_ptr).
> Instead, it is sufficient to introduce a singleton struct dsa_stubs,
> built into the kernel, which contains a single function pointer to
> __dsa_master_hwtstamp_validate().
> 
> I find this approach 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>

Reviewed-by: Florian Fainelli <f.fainelli@...il.com>
-- 
Florian