Hi,
Since commit 52e04b4ce5d0 ("mac80211: fix race in
ieee80211_register_hw()") the debugfs entries for mac80211 drivers are
broken.
https://git.kernel.org/linus/52e04b4ce5d03775b6a78f3ed1097480faacc9fd
Felix reported that the file /sys/kernel/debug/ieee80211/phy0/rc is now
located at /sys/kernel/debug/rc.
Before this commit we had the following flow:
1. wiphy_register()
-> creates /sys/kernel/debug/ieee80211/phy0/
-> fill rdev->wiphy.debugfsdir pointer
2. ieee80211_init_rate_ctrl_alg()
-> call rate_control_alloc()
-> use rdev->wiphy.debugfsdir pointer to
create /sys/kernel/debug/ieee80211/phy0/rc/
This works like expected.
With the commit the flow in ieee80211_register_hw() is the other way around:
2. ieee80211_init_rate_ctrl_alg()
-> call rate_control_alloc()
-> use rdev->wiphy.debugfsdir pointer (now NULL) to
create /sys/kernel/debug/rc/
2. wiphy_register()
-> creates /sys/kernel/debug/ieee80211/phy0/
-> fill rdev->wiphy.debugfsdir pointer
This patch was backported to multiple stable kernel versions:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=linux-5.4.y&id=a8ce3412e8a22279e1bdc81c3c2bacd3785c1577
Hauke
On Thu, 23 Apr 2020 at 02:29, Hauke Mehrtens <[email protected]> wrote:
>
> Hi,
>
> Since commit 52e04b4ce5d0 ("mac80211: fix race in
> ieee80211_register_hw()") the debugfs entries for mac80211 drivers are
> broken.
> https://git.kernel.org/linus/52e04b4ce5d03775b6a78f3ed1097480faacc9fd
>
> Felix reported that the file /sys/kernel/debug/ieee80211/phy0/rc is now
> located at /sys/kernel/debug/rc.
>
> Before this commit we had the following flow:
> 1. wiphy_register()
> -> creates /sys/kernel/debug/ieee80211/phy0/
> -> fill rdev->wiphy.debugfsdir pointer
> 2. ieee80211_init_rate_ctrl_alg()
> -> call rate_control_alloc()
> -> use rdev->wiphy.debugfsdir pointer to
> create /sys/kernel/debug/ieee80211/phy0/rc/
>
> This works like expected.
>
>
> With the commit the flow in ieee80211_register_hw() is the other way around:
> 2. ieee80211_init_rate_ctrl_alg()
> -> call rate_control_alloc()
> -> use rdev->wiphy.debugfsdir pointer (now NULL) to
> create /sys/kernel/debug/rc/
> 2. wiphy_register()
> -> creates /sys/kernel/debug/ieee80211/phy0/
> -> fill rdev->wiphy.debugfsdir pointer
>
Thanks for the detailed report. I missed to notice this hidden debugfs
dependency on "wiphy_register()". To address this issue, I think it's
reasonable to move creation of wiphy debugfs directory during
allocation of wiphy device as per following change (untested though):
diff --git a/net/wireless/core.c b/net/wireless/core.c
index 341402b..239e9ff 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -473,6 +473,10 @@ struct wiphy *wiphy_new_nm(const struct
cfg80211_ops *ops, int sizeof_priv,
}
}
+ /* add to debugfs */
+ rdev->wiphy.debugfsdir = debugfs_create_dir(wiphy_name(&rdev->wiphy),
+ ieee80211_debugfs_dir);
+
INIT_LIST_HEAD(&rdev->wiphy.wdev_list);
INIT_LIST_HEAD(&rdev->beacon_registrations);
spin_lock_init(&rdev->beacon_registrations_lock);
@@ -903,10 +907,6 @@ int wiphy_register(struct wiphy *wiphy)
list_add_rcu(&rdev->list, &cfg80211_rdev_list);
cfg80211_rdev_list_generation++;
- /* add to debugfs */
- rdev->wiphy.debugfsdir = debugfs_create_dir(wiphy_name(&rdev->wiphy),
- ieee80211_debugfs_dir);
-
cfg80211_debugfs_rdev_add(rdev);
nl80211_notify_wiphy(rdev, NL80211_CMD_NEW_WIPHY);
With this change we can remove this hidden debugfs dependency. Please
give it a try and let me know if it works for you.
-Sumit
>
> This patch was backported to multiple stable kernel versions:
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=linux-5.4.y&id=a8ce3412e8a22279e1bdc81c3c2bacd3785c1577
>
> Hauke
Hi Hauke, Sumit,
> > Felix reported that the file /sys/kernel/debug/ieee80211/phy0/rc is now
> > located at /sys/kernel/debug/rc.
Yeah, we noticed this the other day too.
> +++ b/net/wireless/core.c
> @@ -473,6 +473,10 @@ struct wiphy *wiphy_new_nm(const struct
> cfg80211_ops *ops, int sizeof_priv,
> }
> }
>
> + /* add to debugfs */
> + rdev->wiphy.debugfsdir = debugfs_create_dir(wiphy_name(&rdev->wiphy),
> + ieee80211_debugfs_dir);
This cannot work, we haven't committed to the name of the wiphy yet at
this point.
I have some fixes, I'll send them out asap.
johannes
Hi Johannes,
On Thu, 23 Apr 2020 at 13:29, Johannes Berg <[email protected]> wrote:
>
> Hi Hauke, Sumit,
>
> > > Felix reported that the file /sys/kernel/debug/ieee80211/phy0/rc is now
> > > located at /sys/kernel/debug/rc.
>
> Yeah, we noticed this the other day too.
>
> > +++ b/net/wireless/core.c
> > @@ -473,6 +473,10 @@ struct wiphy *wiphy_new_nm(const struct
> > cfg80211_ops *ops, int sizeof_priv,
> > }
> > }
> >
> > + /* add to debugfs */
> > + rdev->wiphy.debugfsdir = debugfs_create_dir(wiphy_name(&rdev->wiphy),
> > + ieee80211_debugfs_dir);
>
> This cannot work, we haven't committed to the name of the wiphy yet at
> this point.
Maybe I am missing something, can you please elaborate here?
Looking at the code, the default or requested wiphy name is configured
just above this and the rename API "cfg80211_dev_rename()" takes care
of renaming wiphy debugfs directory too.
-Sumit
>
> I have some fixes, I'll send them out asap.
>
> johannes
>
On Thu, 2020-04-23 at 14:27 +0530, Sumit Garg wrote:
> > > +++ b/net/wireless/core.c
> > > @@ -473,6 +473,10 @@ struct wiphy *wiphy_new_nm(const struct
> > > cfg80211_ops *ops, int sizeof_priv,
> > > }
> > > }
> > >
> > > + /* add to debugfs */
> > > + rdev->wiphy.debugfsdir = debugfs_create_dir(wiphy_name(&rdev->wiphy),
> > > + ieee80211_debugfs_dir);
> >
> > This cannot work, we haven't committed to the name of the wiphy yet at
> > this point.
>
> Maybe I am missing something, can you please elaborate here?
>
> Looking at the code, the default or requested wiphy name is configured
> just above this and the rename API "cfg80211_dev_rename()" takes care
> of renaming wiphy debugfs directory too.
Yes, but I think wiphy_register() can still fail at this point, due to
name clashes or so?
In any case, it'd be very strange to have a debugfs entry around when
the wiphy doesn't exist yet, and could possibly cause the same issue
that you fixed again, just through debugfs accesses?
Can you take a look at the patch I sent?
johannes
On Thu, 23 Apr 2020 at 14:29, Johannes Berg <[email protected]> wrote:
>
> On Thu, 2020-04-23 at 14:27 +0530, Sumit Garg wrote:
>
> > > > +++ b/net/wireless/core.c
> > > > @@ -473,6 +473,10 @@ struct wiphy *wiphy_new_nm(const struct
> > > > cfg80211_ops *ops, int sizeof_priv,
> > > > }
> > > > }
> > > >
> > > > + /* add to debugfs */
> > > > + rdev->wiphy.debugfsdir = debugfs_create_dir(wiphy_name(&rdev->wiphy),
> > > > + ieee80211_debugfs_dir);
> > >
> > > This cannot work, we haven't committed to the name of the wiphy yet at
> > > this point.
> >
> > Maybe I am missing something, can you please elaborate here?
> >
> > Looking at the code, the default or requested wiphy name is configured
> > just above this and the rename API "cfg80211_dev_rename()" takes care
> > of renaming wiphy debugfs directory too.
>
> Yes, but I think wiphy_register() can still fail at this point, due to
> name clashes or so?
>
> In any case, it'd be very strange to have a debugfs entry around when
> the wiphy doesn't exist yet, and could possibly cause the same issue
> that you fixed again, just through debugfs accesses?
>
Now I understood your point. Yeah it's definitely better to expose
debugfs after the wiphy device is registered.
> Can you take a look at the patch I sent?
Sure I will take a look.
-Sumit
>
> johannes
>