Re: [PATCH net-next] net sysfs: remove deprecated empty wireless directory

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch removes the empty /sys/class/net/<iface>/wireless
  directory, which userspace can see.
- [Medium] The removed wireless_group_needed() matched WEXT-only netdevs,
  i.e. those with `ndev->wireless_handlers` set and no cfg80211…
- [Low] The kernel-doc for struct net_device::sysfs_groups in
  include/linux/netdevice.h still says "Space for optional device,…

--

net sysfs: remove deprecated empty wireless directory

This patch removes the empty "wireless" attribute group that
netdev_register_kobject() created for cfg80211 and wireless extensions
netdevs. It also shrinks net_device::sysfs_groups from 5 entries to 4.

Link: https://github.com/fedora-sysv/initscripts/commit/4e96fcecee7ab70295a5f2c52748cd57cbab60bb

> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 435be7880d7e5..ac943798abca6 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -2484,7 +2484,7 @@ struct net_device {
>  	struct dm_hw_stat_delta __rcu *dm_private;
>  #endif
>  	struct device		dev;
> -	const struct attribute_group *sysfs_groups[5];
> +	const struct attribute_group *sysfs_groups[4];

[Severity: Low]
This isn't a bug, but should the kernel-doc for this field be updated as
well? It still says:

 *	@sysfs_groups:	Space for optional device, statistics and wireless
 *			sysfs groups

After this patch, netdev_register_kobject() stores only the optional
driver group, netstat_group, netdev_phys_group and the NULL terminator.
The comment still lists a wireless slot that nothing fills, and it does
not mention the phys group.

>  	const struct attribute_group *sysfs_rx_queue_group;
>  
>  	const struct rtnl_link_ops *rtnl_link_ops;
> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> index 5c0402389c6d1..cb8c82d1a4da8 100644
> --- a/net/core/net-sysfs.c
> +++ b/net/core/net-sysfs.c
> @@ -910,28 +910,6 @@ static const struct attribute_group netstat_group = {
>  	.attrs  = netstat_attrs,
>  };
>  
> -static struct attribute *wireless_attrs[] = {
> -	NULL
> -};
> -
> -static const struct attribute_group wireless_group = {
> -	.name = "wireless",
> -	.attrs = wireless_attrs,
> -};

[Severity: Medium]
Could this cause a userspace regression? The group has no attributes,
so the existence of /sys/class/net/<iface>/wireless is the whole
interface. A script that tests for it with something like
"[ -d /sys/class/net/$IF/wireless ]" would quietly stop finding wireless
interfaces, without any error.

This removal has been done and reverted once before. Commit 35b2a113cb02
removed the directory. Commit 38c1a01cf10c, which the commit message
cites, added it back because the removal "broke (at least) Fedora's
networking scripts", and that fix was tagged for stable 3.6.

The commit message says:

  Last known user of this deprecated feature was Fedora, until 2012.

The 2012 commit said "at least", so it did not claim Fedora was the only
user. I can't confirm from the tree whether any current tools or local
scripts still do this check. raspi-config's list_wlan_interfaces is one
example worth checking.

There also seems to be no deprecation period and no Documentation/ABI
entry for this directory. Would it make sense for the commit message to
mention the earlier regression and revert? It could also point users to
the replacements: the phy80211 symlink and DEVTYPE=wlan set in
net/wireless/core.c, or nl80211.

> -
> -static bool wireless_group_needed(struct net_device *ndev)
> -{
> -#if IS_ENABLED(CONFIG_CFG80211)
> -	if (ndev->ieee80211_ptr)
> -		return true;
> -#endif
> -#if IS_ENABLED(CONFIG_WIRELESS_EXT)
> -	if (ndev->wireless_handlers)
> -		return true;
> -#endif

[Severity: Medium]
What about netdevs that use only wireless extensions? For these, the
wireless directory appears to be the only sign in sysfs that the device
is wireless.

One example is ps3_gelic_wireless:

drivers/net/ethernet/toshiba/ps3_gelic_wireless.c:gelic_wl_setup_netdev_ops() {
    ...
	netdev->wireless_handlers = &gelic_wl_wext_handler_def;
    ...
}

This driver never sets ieee80211_ptr. net/wireless/core.c adds the
phy80211 symlink and SET_NETDEV_DEVTYPE(dev, &wiphy_type) only for
cfg80211 wdevs, so this device gets neither.

After this patch, sysfs and uevents seem to give no way to tell that
such a device is wireless. Only /proc/net/wireless or wext ioctls like
SIOCGIWNAME would remain.

The ipw2x00 drivers also set wireless_handlers, but they get
ieee80211_ptr through libipw, so they keep phy80211.

Commit 38c1a01cf10c covered this case on purpose: "Also do it for both
drivers based on the old wireless extensions and cfg80211". Should the
commit message discuss the wext-only case, since these devices have no
sysfs replacement?

> -	return false;
> -}
> -

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/2af5fc30-4520-4768-bcdb-7b1058b39212%40gmail.com