| 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
| ||
|
Message-ID: <6cedd632-d555-4c17-81cb-984af73f2c08@gmail.com>
Date: Wed, 8 May 2024 07:44:22 +0200
From: Heiner Kallweit <hkallweit1@...il.com>
To: Maxime Chevallier <maxime.chevallier@...tlin.com>, davem@...emloft.net
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
thomas.petazzoni@...tlin.com, Andrew Lunn <andrew@...n.ch>,
Jakub Kicinski <kuba@...nel.org>, Eric Dumazet <edumazet@...gle.com>,
Paolo Abeni <pabeni@...hat.com>, Russell King <linux@...linux.org.uk>,
linux-arm-kernel@...ts.infradead.org,
Christophe Leroy <christophe.leroy@...roup.eu>,
Herve Codina <herve.codina@...tlin.com>,
Florian Fainelli <f.fainelli@...il.com>,
Vladimir Oltean <vladimir.oltean@....com>,
Köry Maincent <kory.maincent@...tlin.com>,
Jesse Brandeburg <jesse.brandeburg@...el.com>, Marek Behún
<kabel@...nel.org>, Piergiorgio Beruto <piergiorgio.beruto@...il.com>,
Oleksij Rempel <o.rempel@...gutronix.de>,
Nicolò Veronese <nicveronese@...il.com>,
Simon Horman <horms@...nel.org>, mwojtas@...omium.org,
Nathan Chancellor <nathan@...nel.org>, Antoine Tenart <atenart@...nel.org>
Subject: Re: [PATCH net-next 2/2] net: phy: phy_link_topology: Lazy-initialize
the link topology
On 07.05.2024 12:28, Maxime Chevallier wrote:
> Having the net_device's init path for the link_topology depend on
> IS_REACHABLE(PHYLIB)-protected helpers triggers errors when modules are being
> built with phylib as a module as-well, as they expect netdev->link_topo
> to be initialized.
>
> Move the link_topo initialization at the first PHY insertion, which will
> both improve the memory usage, and make the behaviour more predicatble
> and robust.
>
> Signed-off-by: Maxime Chevallier <maxime.chevallier@...tlin.com>
> Fixes: 6916e461e793 ("net: phy: Introduce ethernet link topology representation")
> Closes: https://lore.kernel.org/netdev/2e11b89d-100f-49e7-9c9a-834cc0b82f97@gmail.com/
> Closes: https://lore.kernel.org/netdev/20240409201553.GA4124869@dev-arch.thelio-3990X/
> ---
> drivers/net/phy/phy_link_topology.c | 31 ++++++---------------
> include/linux/netdevice.h | 2 ++
> include/linux/phy_link_topology.h | 23 ++++++++--------
> include/linux/phy_link_topology_core.h | 23 +++-------------
> net/core/dev.c | 38 ++++++++++++++++++++++----
> 5 files changed, 58 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
> index 0e36bd7c15dc..b1aba9313e73 100644
> --- a/drivers/net/phy/phy_link_topology.c
> +++ b/drivers/net/phy/phy_link_topology.c
> @@ -12,29 +12,6 @@
> #include <linux/rtnetlink.h>
> #include <linux/xarray.h>
>
> -struct phy_link_topology *phy_link_topo_create(struct net_device *dev)
> -{
> - struct phy_link_topology *topo;
> -
> - topo = kzalloc(sizeof(*topo), GFP_KERNEL);
> - if (!topo)
> - return ERR_PTR(-ENOMEM);
> -
> - xa_init_flags(&topo->phys, XA_FLAGS_ALLOC1);
> - topo->next_phy_index = 1;
> -
> - return topo;
> -}
> -
> -void phy_link_topo_destroy(struct phy_link_topology *topo)
> -{
> - if (!topo)
> - return;
> -
> - xa_destroy(&topo->phys);
> - kfree(topo);
> -}
> -
> int phy_link_topo_add_phy(struct net_device *dev,
> struct phy_device *phy,
> enum phy_upstream upt, void *upstream)
> @@ -43,6 +20,14 @@ int phy_link_topo_add_phy(struct net_device *dev,
> struct phy_device_node *pdn;
> int ret;
>
> + if (!topo) {
> + ret = netdev_alloc_phy_link_topology(dev);
This function is implemented in net core, but used only here.
So move the implementation here?
> + if (ret)
> + return ret;
> +
> + topo = dev->link_topo;
> + }
> +
> pdn = kzalloc(sizeof(*pdn), GFP_KERNEL);
> if (!pdn)
> return -ENOMEM;
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index cf261fb89d73..25a0a77cfadc 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -4569,6 +4569,8 @@ void __hw_addr_unsync_dev(struct netdev_hw_addr_list *list,
> const unsigned char *));
> void __hw_addr_init(struct netdev_hw_addr_list *list);
>
> +int netdev_alloc_phy_link_topology(struct net_device *dev);
> +
> /* Functions used for device addresses handling */
> void dev_addr_mod(struct net_device *dev, unsigned int offset,
> const void *addr, size_t len);
> diff --git a/include/linux/phy_link_topology.h b/include/linux/phy_link_topology.h
> index 166a01710aa2..3501f9a9e932 100644
> --- a/include/linux/phy_link_topology.h
> +++ b/include/linux/phy_link_topology.h
> @@ -32,10 +32,12 @@ struct phy_device_node {
> struct phy_device *phy;
> };
>
> -struct phy_link_topology {
> - struct xarray phys;
> - u32 next_phy_index;
> -};
> +#if IS_ENABLED(CONFIG_PHYLIB)
> +int phy_link_topo_add_phy(struct net_device *dev,
> + struct phy_device *phy,
> + enum phy_upstream upt, void *upstream);
> +
> +void phy_link_topo_del_phy(struct net_device *dev, struct phy_device *phy);
>
> static inline struct phy_device
> *phy_link_topo_get_phy(struct net_device *dev, u32 phyindex)
> @@ -53,13 +55,6 @@ static inline struct phy_device
> return NULL;
> }
>
> -#if IS_REACHABLE(CONFIG_PHYLIB)
> -int phy_link_topo_add_phy(struct net_device *dev,
> - struct phy_device *phy,
> - enum phy_upstream upt, void *upstream);
> -
> -void phy_link_topo_del_phy(struct net_device *dev, struct phy_device *phy);
> -
> #else
> static inline int phy_link_topo_add_phy(struct net_device *dev,
> struct phy_device *phy,
> @@ -72,6 +67,12 @@ static inline void phy_link_topo_del_phy(struct net_device *dev,
> struct phy_device *phy)
> {
> }
> +
> +static inline struct phy_device *
> +phy_link_topo_get_phy(struct net_device *dev, u32 phyindex)
> +{
> + return NULL;
> +}
> #endif
>
> #endif /* __PHY_LINK_TOPOLOGY_H */
> diff --git a/include/linux/phy_link_topology_core.h b/include/linux/phy_link_topology_core.h
> index 0a6479055745..f9c0520806fb 100644
> --- a/include/linux/phy_link_topology_core.h
> +++ b/include/linux/phy_link_topology_core.h
> @@ -2,24 +2,9 @@
> #ifndef __PHY_LINK_TOPOLOGY_CORE_H
> #define __PHY_LINK_TOPOLOGY_CORE_H
>
> -struct phy_link_topology;
> -
> -#if IS_REACHABLE(CONFIG_PHYLIB)
> -
> -struct phy_link_topology *phy_link_topo_create(struct net_device *dev);
> -void phy_link_topo_destroy(struct phy_link_topology *topo);
> -
> -#else
> -
> -static inline struct phy_link_topology *phy_link_topo_create(struct net_device *dev)
> -{
> - return NULL;
> -}
> -
> -static inline void phy_link_topo_destroy(struct phy_link_topology *topo)
> -{
> -}
> -
> -#endif
> +struct phy_link_topology {
> + struct xarray phys;
> + u32 next_phy_index;
> +};
>
This is all which is left in this header. As this header is public anyway,
better move this definition to phy_link_topology.h?
> #endif /* __PHY_LINK_TOPOLOGY_CORE_H */
> diff --git a/net/core/dev.c b/net/core/dev.c
> index d2ce91a334c1..1b4ffc273a04 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -10256,6 +10256,35 @@ static void netdev_do_free_pcpu_stats(struct net_device *dev)
> }
> }
>
> +int netdev_alloc_phy_link_topology(struct net_device *dev)
> +{
> + struct phy_link_topology *topo;
> +
> + topo = kzalloc(sizeof(*topo), GFP_KERNEL);
> + if (!topo)
> + return -ENOMEM;
> +
> + xa_init_flags(&topo->phys, XA_FLAGS_ALLOC1);
> + topo->next_phy_index = 1;
> +
> + dev->link_topo = topo;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(netdev_alloc_phy_link_topology);
> +
> +static void netdev_free_phy_link_topology(struct net_device *dev)
> +{
> + struct phy_link_topology *topo = dev->link_topo;
> +
> + if (!topo)
> + return;
> +
> + xa_destroy(&topo->phys);
> + kfree(topo);
> + dev->link_topo = NULL;
Give the compiler a chance to remove this function if
CONFIG_PHYLIB isn't enabled.
if (IS_ENABLED(CONFIG_PHYLIB) && topo) {
xa_destroy(&topo->phys);
kfree(topo);
dev->link_topo = NULL;
}
> +}
> +
> /**
> * register_netdevice() - register a network device
> * @dev: device to register
> @@ -10998,11 +11027,6 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
> #ifdef CONFIG_NET_SCHED
> hash_init(dev->qdisc_hash);
> #endif
> - dev->link_topo = phy_link_topo_create(dev);
> - if (IS_ERR(dev->link_topo)) {
> - dev->link_topo = NULL;
> - goto free_all;
> - }
>
> dev->priv_flags = IFF_XMIT_DST_RELEASE | IFF_XMIT_DST_RELEASE_PERM;
> setup(dev);
> @@ -11092,7 +11116,9 @@ void free_netdev(struct net_device *dev)
> free_percpu(dev->xdp_bulkq);
> dev->xdp_bulkq = NULL;
>
> - phy_link_topo_destroy(dev->link_topo);
> +#if IS_ENABLED(CONFIG_PHYLIB)
> + netdev_free_phy_link_topology(dev);
> +#endif
>
Then the conditional compiling can be removed here.
> /* Compatibility with error handling in drivers */
> if (dev->reg_state == NETREG_UNINITIALIZED ||
Powered by blists - more mailing lists