The commit beee24695157 ("cfg80211: Save the regulatory domain when
setting custom regulatory") forgets to free the newly allocated regd
object.
Fix this by freeing the regd object in the error handling code and
deletion function - mac80211_hwsim_del_radio.
Reported-by: [email protected]
Fixes: beee24695157 ("cfg80211: Save the regulatory domain when setting custom regulatory")
Signed-off-by: Dongliang Mu <[email protected]>
---
drivers/net/wireless/mac80211_hwsim.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index ffa894f7312a..20b870af6356 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -3404,6 +3404,8 @@ static int mac80211_hwsim_new_radio(struct genl_info *info,
debugfs_remove_recursive(data->debugfs);
ieee80211_unregister_hw(data->hw);
failed_hw:
+ if (param->regd)
+ kfree_rcu(get_wiphy_regdom(data->hw->wiphy));
device_release_driver(data->dev);
failed_bind:
device_unregister(data->dev);
@@ -3454,6 +3456,8 @@ static void mac80211_hwsim_del_radio(struct mac80211_hwsim_data *data,
{
hwsim_mcast_del_radio(data->idx, hwname, info);
debugfs_remove_recursive(data->debugfs);
+ if (data->regd)
+ kfree_rcu(get_wiphy_regdom(data->hw->wiphy));
ieee80211_unregister_hw(data->hw);
device_release_driver(data->dev);
device_unregister(data->dev);
--
2.25.1
On Fri, Jul 23, 2021 at 1:09 PM Dongliang Mu <[email protected]> wrote:
>
> The commit beee24695157 ("cfg80211: Save the regulatory domain when
> setting custom regulatory") forgets to free the newly allocated regd
> object.
>
> Fix this by freeing the regd object in the error handling code and
> deletion function - mac80211_hwsim_del_radio.
>
> Reported-by: [email protected]
> Fixes: beee24695157 ("cfg80211: Save the regulatory domain when setting custom regulatory")
> Signed-off-by: Dongliang Mu <[email protected]>
> ---
> drivers/net/wireless/mac80211_hwsim.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
> index ffa894f7312a..20b870af6356 100644
> --- a/drivers/net/wireless/mac80211_hwsim.c
> +++ b/drivers/net/wireless/mac80211_hwsim.c
> @@ -3404,6 +3404,8 @@ static int mac80211_hwsim_new_radio(struct genl_info *info,
> debugfs_remove_recursive(data->debugfs);
> ieee80211_unregister_hw(data->hw);
> failed_hw:
> + if (param->regd)
> + kfree_rcu(get_wiphy_regdom(data->hw->wiphy));
Here kfree_rcu supports only 1 parameter. But some example code uses
the 2nd parameter: rcu_head or rcu. For example,
static void rcu_free_regdom(const struct ieee80211_regdomain *r)
{
......
kfree_rcu((struct ieee80211_regdomain *)r, rcu_head);
}
If this patch has any issues, please let me know.
> device_release_driver(data->dev);
> failed_bind:
> device_unregister(data->dev);
> @@ -3454,6 +3456,8 @@ static void mac80211_hwsim_del_radio(struct mac80211_hwsim_data *data,
> {
> hwsim_mcast_del_radio(data->idx, hwname, info);
> debugfs_remove_recursive(data->debugfs);
> + if (data->regd)
> + kfree_rcu(get_wiphy_regdom(data->hw->wiphy));
> ieee80211_unregister_hw(data->hw);
> device_release_driver(data->dev);
> device_unregister(data->dev);
> --
> 2.25.1
>
On Fri, 2021-07-23 at 13:09 +0800, Dongliang Mu wrote:
> The commit beee24695157 ("cfg80211: Save the regulatory domain when
> setting custom regulatory") forgets to free the newly allocated regd
> object.
Not really? It's not forgetting it, it just saves it?
+ new_regd = reg_copy_regd(regd);
+ if (IS_ERR(new_regd))
+ return;
+
+ tmp = get_wiphy_regdom(wiphy);
+ rcu_assign_pointer(wiphy->regd, new_regd);
+ rcu_free_regdom(tmp);
> Fix this by freeing the regd object in the error handling code and
> deletion function - mac80211_hwsim_del_radio.
This can't be right - the same would affect all other users of that
function, no?
Perhaps somewhere we have a case where wiphy->regd is leaked, but than
that should be fixed more generally in cfg80211?
johannes
On Fri, 2021-07-23 at 17:13 +0800, Dongliang Mu wrote:
> On Fri, Jul 23, 2021 at 4:37 PM Johannes Berg <[email protected]> wrote:
> >
> > On Fri, 2021-07-23 at 13:09 +0800, Dongliang Mu wrote:
> > > The commit beee24695157 ("cfg80211: Save the regulatory domain when
> > > setting custom regulatory") forgets to free the newly allocated regd
> > > object.
> >
> > Not really? It's not forgetting it, it just saves it?
>
> Yes, it saves the regd object in the function wiphy_apply_custom_regulatory.
Right.
> But its parent function - mac80211_hwsim_new_radio forgets to free
> this object when the ieee80211_register_hw fails.
But why is this specific to mac80211-hwsim?
Any other code calling wiphy_apply_custom_regulatory() and then failing
the subsequent wiphy_register() or otherwise calling wiphy_free() will
run into the same situation.
So why wouldn't we free this in wiphy_free(), if it exists?
johannes
在 2021/7/23 13:09, Dongliang Mu 写道:
> The commit beee24695157 ("cfg80211: Save the regulatory domain when
> setting custom regulatory") forgets to free the newly allocated regd
> object.
>
> Fix this by freeing the regd object in the error handling code and
> deletion function - mac80211_hwsim_del_radio.
>
> Reported-by: [email protected]
> Fixes: beee24695157 ("cfg80211: Save the regulatory domain when setting custom regulatory")
> Signed-off-by: Dongliang Mu <[email protected]>
> ---
> drivers/net/wireless/mac80211_hwsim.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
> index ffa894f7312a..20b870af6356 100644
> --- a/drivers/net/wireless/mac80211_hwsim.c
> +++ b/drivers/net/wireless/mac80211_hwsim.c
> @@ -3404,6 +3404,8 @@ static int mac80211_hwsim_new_radio(struct genl_info *info,
> debugfs_remove_recursive(data->debugfs);
> ieee80211_unregister_hw(data->hw);
> failed_hw:
> + if (param->regd)
> + kfree_rcu(get_wiphy_regdom(data->hw->wiphy));
> device_release_driver(data->dev);
hw->wiphy->regd may be NULL if previous reg_copy_regd failed, so how about:
if (hw->wiphy->regd)
rcu_free_regdom(get_wiphy_regdom(hw->wiphy))
> failed_bind:
> device_unregister(data->dev);
> @@ -3454,6 +3456,8 @@ static void mac80211_hwsim_del_radio(struct mac80211_hwsim_data *data,
> {
> hwsim_mcast_del_radio(data->idx, hwname, info);
> debugfs_remove_recursive(data->debugfs);
> + if (data->regd)
> + kfree_rcu(get_wiphy_regdom(data->hw->wiphy));
this is not correct, because ieee80211_unregister_hw below will free
data->hw_wiphy->regd
> ieee80211_unregister_hw(data->hw);
> device_release_driver(data->dev);
> device_unregister(data->dev);
>
On Fri, Jul 23, 2021 at 5:18 PM xiaoqiang zhao
<[email protected]> wrote:
>
>
>
> 在 2021/7/23 13:09, Dongliang Mu 写道:
> > The commit beee24695157 ("cfg80211: Save the regulatory domain when
> > setting custom regulatory") forgets to free the newly allocated regd
> > object.
> >
> > Fix this by freeing the regd object in the error handling code and
> > deletion function - mac80211_hwsim_del_radio.
> >
> > Reported-by: [email protected]
> > Fixes: beee24695157 ("cfg80211: Save the regulatory domain when setting custom regulatory")
> > Signed-off-by: Dongliang Mu <[email protected]>
> > ---
> > drivers/net/wireless/mac80211_hwsim.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
> > index ffa894f7312a..20b870af6356 100644
> > --- a/drivers/net/wireless/mac80211_hwsim.c
> > +++ b/drivers/net/wireless/mac80211_hwsim.c
> > @@ -3404,6 +3404,8 @@ static int mac80211_hwsim_new_radio(struct genl_info *info,
> > debugfs_remove_recursive(data->debugfs);
> > ieee80211_unregister_hw(data->hw);
> > failed_hw:
> > + if (param->regd)
> > + kfree_rcu(get_wiphy_regdom(data->hw->wiphy));
> > device_release_driver(data->dev);
>
> hw->wiphy->regd may be NULL if previous reg_copy_regd failed, so how about:
> if (hw->wiphy->regd)
> rcu_free_regdom(get_wiphy_regdom(hw->wiphy))
Previously I would like to use this API(rcu_free_regdom), but it is
static and located in non-global header file - reg.h.
>
> > failed_bind:
> > device_unregister(data->dev);
> > @@ -3454,6 +3456,8 @@ static void mac80211_hwsim_del_radio(struct mac80211_hwsim_data *data,
> > {
> > hwsim_mcast_del_radio(data->idx, hwname, info);
> > debugfs_remove_recursive(data->debugfs);
> > + if (data->regd)
> > + kfree_rcu(get_wiphy_regdom(data->hw->wiphy));
> this is not correct, because ieee80211_unregister_hw below will free
> data->hw_wiphy->regd
Can you point out the concrete code releasing regd? Maybe the link to elixir.
> > ieee80211_unregister_hw(data->hw);
> > device_release_driver(data->dev);
> > device_unregister(data->dev);
> >
On Fri, Jul 23, 2021 at 5:18 PM Johannes Berg <[email protected]> wrote:
>
> On Fri, 2021-07-23 at 17:13 +0800, Dongliang Mu wrote:
> > On Fri, Jul 23, 2021 at 4:37 PM Johannes Berg <[email protected]> wrote:
> > >
> > > On Fri, 2021-07-23 at 13:09 +0800, Dongliang Mu wrote:
> > > > The commit beee24695157 ("cfg80211: Save the regulatory domain when
> > > > setting custom regulatory") forgets to free the newly allocated regd
> > > > object.
> > >
> > > Not really? It's not forgetting it, it just saves it?
> >
> > Yes, it saves the regd object in the function wiphy_apply_custom_regulatory.
>
> Right.
>
> > But its parent function - mac80211_hwsim_new_radio forgets to free
> > this object when the ieee80211_register_hw fails.
>
> But why is this specific to mac80211-hwsim?
>
> Any other code calling wiphy_apply_custom_regulatory() and then failing
> the subsequent wiphy_register() or otherwise calling wiphy_free() will
> run into the same situation.
>
> So why wouldn't we free this in wiphy_free(), if it exists?
>
Hi Johannes,
if zhao in the thread is right, we don't need to add this free
operation to wiphy_free().
What we should do is to only handle regd in the error handling code of
mac80211_hwsim_new_radio. This will not affect other users of
mac80211-hwsim. Any idea?
> johannes
>
在 2021/7/23 17:25, Dongliang Mu 写道:
> Can you point out the concrete code releasing regd? Maybe the link to elixir.
>
>>> ieee80211_unregister_hw(data->hw);
>>> device_release_driver(data->dev);
>>> device_unregister(data->dev);
>>>
call graph seems like this:
ieee80211_unregister_hw
(https://elixir.bootlin.com/linux/v5.14-rc2/source/net/mac80211/main.c#L1368)
wiphy_unregister
(https://elixir.bootlin.com/linux/v5.14-rc2/source/net/wireless/core.c#L1011)
wiphy_regulatory_deregister
(https://elixir.bootlin.com/linux/v5.14-rc2/source/net/wireless/reg.c#L4057)
rcu_free_regdom