netdev - Re: [PATCH net-next] net: ifdefy the wireless pointers in struct net

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: <74bdbec0580ed05d0f18533eae9af50bc0a4a0ef.camel@sipsolutions.net>
Date:   Tue, 17 May 2022 09:48:24 +0200
From:   Johannes Berg <johannes@...solutions.net>
To:     Florian Fainelli <f.fainelli@...il.com>,
        Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net
Cc:     netdev@...r.kernel.org, edumazet@...gle.com, pabeni@...hat.com,
        alex.aring@...il.com, stefan@...enfreihafen.org,
        mareklindner@...mailbox.ch, sw@...onwunderlich.de, a@...table.cc,
        sven@...fation.org, linux-wireless@...r.kernel.org,
        linux-wpan@...r.kernel.org
Subject: Re: [PATCH net-next] net: ifdefy the wireless pointers in struct
 net_device

On Mon, 2022-05-16 at 19:12 -0700, Florian Fainelli wrote:
> 
> On 5/16/2022 2:56 PM, Jakub Kicinski wrote:
> > Most protocol-specific pointers in struct net_device are under
> > a respective ifdef. Wireless is the notable exception. Since
> > there's a sizable number of custom-built kernels for datacenter
> > workloads which don't build wireless it seems reasonable to
> > ifdefy those pointers as well.
> > 
> > While at it move IPv4 and IPv6 pointers up, those are special
> > for obvious reasons.
> > 
> > Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> 
> Could not we move to an union of pointers in the future since in many 
> cases a network device can only have one of those pointers at any given 
> time?

Then at the very least we'd need some kind of type that we can assign to
disambiguate, because today e.g. we have a netdev notifier (and other
code) that could get a non-wireless netdev and check like this:

static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
                                         unsigned long state, void *ptr)
{
        struct net_device *dev = netdev_notifier_info_to_dev(ptr);
        struct wireless_dev *wdev = dev->ieee80211_ptr;
[...]
        if (!wdev)
                return NOTIFY_DONE;



We could probably use the netdev->dev.type for this though, that's just
a pointer we can compare to. We'd have to set it differently (today
cfg80211 sets it based on whether or not you have ieee80211_ptr, we'd
have to turn that around), but that's not terribly hard, especially
since wireless drivers now have to call cfg80211_register_netdevice()
anyway, rather than register_netdevice() directly.


Something like the patch below might do that, but I haven't carefully
checked it yet, nor checked if there are any paths in mac80211/drivers
that might be doing this check - and it looks from Jakub's patch that
batman code would like to check this too.

johannes

diff --git a/net/wireless/core.c b/net/wireless/core.c
index 3e5d12040726..6ea2a597f4ca 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -1192,7 +1192,7 @@ void cfg80211_unregister_wdev(struct wireless_dev *wdev)
 }
 EXPORT_SYMBOL(cfg80211_unregister_wdev);
 
-static const struct device_type wiphy_type = {
+const struct device_type wiphy_type = {
 	.name	= "wlan",
 };
 
@@ -1369,6 +1369,9 @@ int cfg80211_register_netdevice(struct net_device *dev)
 
 	lockdep_assert_held(&rdev->wiphy.mtx);
 
+	/* this lets us identify our netdevs in the future */
+	SET_NETDEV_DEVTYPE(dev, &wiphy_type);
+
 	/* we'll take care of this */
 	wdev->registered = true;
 	wdev->registering = true;
@@ -1394,7 +1397,7 @@ static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
 	struct cfg80211_registered_device *rdev;
 	struct cfg80211_sched_scan_request *pos, *tmp;
 
-	if (!wdev)
+	if (!netdev_is_wireless(dev))
 		return NOTIFY_DONE;
 
 	rdev = wiphy_to_rdev(wdev->wiphy);
@@ -1403,7 +1406,6 @@ static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
 
 	switch (state) {
 	case NETDEV_POST_INIT:
-		SET_NETDEV_DEVTYPE(dev, &wiphy_type);
 		wdev->netdev = dev;
 		/* can only change netns with wiphy */
 		dev->features |= NETIF_F_NETNS_LOCAL;
diff --git a/net/wireless/core.h b/net/wireless/core.h
index 2c195067ddff..e256ea5caf49 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -219,6 +219,13 @@ void cfg80211_init_wdev(struct wireless_dev *wdev);
 void cfg80211_register_wdev(struct cfg80211_registered_device *rdev,
 			    struct wireless_dev *wdev);
 
+extern const struct device_type wiphy_type;
+
+static inline bool netdev_is_wireless(struct net_device *dev)
+{
+	return dev && dev->dev.type == &wiphy_type && dev->ieee80211_ptr;
+}
+
 static inline void wdev_lock(struct wireless_dev *wdev)
 	__acquires(wdev)
 {
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 342dfefb6eca..58bc3750c380 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -182,7 +182,7 @@ __cfg80211_rdev_from_attrs(struct net *netns, struct nlattr **attrs)
 
 		netdev = __dev_get_by_index(netns, ifindex);
 		if (netdev) {
-			if (netdev->ieee80211_ptr)
+			if (netdev_is_wireless(netdev))
 				tmp = wiphy_to_rdev(
 					netdev->ieee80211_ptr->wiphy);
 			else
@@ -2978,7 +2978,7 @@ static int nl80211_dump_wiphy_parse(struct sk_buff *skb,
 			ret = -ENODEV;
 			goto out;
 		}
-		if (netdev->ieee80211_ptr) {
+		if (netdev_is_wireless(netdev)) {
 			rdev = wiphy_to_rdev(
 				netdev->ieee80211_ptr->wiphy);
 			state->filter_wiphy = rdev->wiphy_idx;
@@ -3364,7 +3364,7 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct genl_info *info)
 		int ifindex = nla_get_u32(info->attrs[NL80211_ATTR_IFINDEX]);
 
 		netdev = __dev_get_by_index(genl_info_net(info), ifindex);
-		if (netdev && netdev->ieee80211_ptr)
+		if (netdev_is_wireless(netdev))
 			rdev = wiphy_to_rdev(netdev->ieee80211_ptr->wiphy);
 		else
 			netdev = NULL;