Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] cfg80211: free the object allocated in wiphy_apply_custom_regulatory
@ 2021-07-23  5:09 Dongliang Mu
  2021-07-23  5:16 ` Dongliang Mu
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Dongliang Mu @ 2021-07-23  5:09 UTC (permalink / raw)
  To: Johannes Berg, Kalle Valo, David S. Miller, Jakub Kicinski,
	Luca Coelho, Ilan Peer
  Cc: Dongliang Mu, syzbot+1638e7c770eef6b6c0d0, Johannes Berg,
	linux-wireless, netdev, linux-kernel

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: syzbot+1638e7c770eef6b6c0d0@syzkaller.appspotmail.com
Fixes: beee24695157 ("cfg80211: Save the regulatory domain when setting custom regulatory")
Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
---
 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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] cfg80211: free the object allocated in wiphy_apply_custom_regulatory
  2021-07-23  5:09 [PATCH] cfg80211: free the object allocated in wiphy_apply_custom_regulatory Dongliang Mu
@ 2021-07-23  5:16 ` Dongliang Mu
  2021-07-23  8:37 ` Johannes Berg
  2021-07-23  9:18 ` xiaoqiang zhao
  2 siblings, 0 replies; 12+ messages in thread
From: Dongliang Mu @ 2021-07-23  5:16 UTC (permalink / raw)
  To: Johannes Berg, Kalle Valo, David S. Miller, Jakub Kicinski,
	Luca Coelho, Ilan Peer
  Cc: syzbot+1638e7c770eef6b6c0d0, Johannes Berg, linux-wireless,
	open list:NETWORKING [GENERAL],
	linux-kernel

On Fri, Jul 23, 2021 at 1:09 PM Dongliang Mu <mudongliangabcd@gmail.com> 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: syzbot+1638e7c770eef6b6c0d0@syzkaller.appspotmail.com
> Fixes: beee24695157 ("cfg80211: Save the regulatory domain when setting custom regulatory")
> Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> ---
>  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
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] cfg80211: free the object allocated in wiphy_apply_custom_regulatory
  2021-07-23  5:09 [PATCH] cfg80211: free the object allocated in wiphy_apply_custom_regulatory Dongliang Mu
  2021-07-23  5:16 ` Dongliang Mu
@ 2021-07-23  8:37 ` Johannes Berg
  2021-07-23  9:13   ` Dongliang Mu
  2021-07-23  9:18 ` xiaoqiang zhao
  2 siblings, 1 reply; 12+ messages in thread
From: Johannes Berg @ 2021-07-23  8:37 UTC (permalink / raw)
  To: Dongliang Mu, Kalle Valo, David S. Miller, Jakub Kicinski,
	Luca Coelho, Ilan Peer
  Cc: syzbot+1638e7c770eef6b6c0d0, linux-wireless, netdev, linux-kernel

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] cfg80211: free the object allocated in wiphy_apply_custom_regulatory
  2021-07-23  8:37 ` Johannes Berg
@ 2021-07-23  9:13   ` Dongliang Mu
  2021-07-23  9:18     ` Johannes Berg
  0 siblings, 1 reply; 12+ messages in thread
From: Dongliang Mu @ 2021-07-23  9:13 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Kalle Valo, David S. Miller, Jakub Kicinski, Luca Coelho,
	Ilan Peer, syzbot+1638e7c770eef6b6c0d0, linux-wireless,
	open list:NETWORKING [GENERAL],
	linux-kernel, Dan Carpenter

On Fri, Jul 23, 2021 at 4:37 PM Johannes Berg <johannes@sipsolutions.net> 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.

But its parent function - mac80211_hwsim_new_radio forgets to free
this object when the ieee80211_register_hw fails.

>
> +       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?

The problem occurs in the error handling code of
mac80211_hwsim_new_radio, not wiphy_apply_custom_regulatory. My commit
message may be not very clear.

So I think the code in the mac80211_hwsim_del_radio paired with
mac80211_hwsim_new_radio should be changed correspondingly. If I miss
any problems, please let me know.

I have successfully tested my patch in the syzbot dashboard [1].

[1] https://syzkaller.appspot.com/bug?extid=1638e7c770eef6b6c0d0

>
> Perhaps somewhere we have a case where wiphy->regd is leaked, but than
> that should be fixed more generally in cfg80211?
>
> johannes
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] cfg80211: free the object allocated in wiphy_apply_custom_regulatory
  2021-07-23  9:13   ` Dongliang Mu
@ 2021-07-23  9:18     ` Johannes Berg
  2021-07-23  9:30       ` Dongliang Mu
  0 siblings, 1 reply; 12+ messages in thread
From: Johannes Berg @ 2021-07-23  9:18 UTC (permalink / raw)
  To: Dongliang Mu
  Cc: Kalle Valo, David S. Miller, Jakub Kicinski, Luca Coelho,
	Ilan Peer, syzbot+1638e7c770eef6b6c0d0, linux-wireless,
	open list:NETWORKING [GENERAL],
	linux-kernel, Dan Carpenter

On Fri, 2021-07-23 at 17:13 +0800, Dongliang Mu wrote:
> On Fri, Jul 23, 2021 at 4:37 PM Johannes Berg <johannes@sipsolutions.net> 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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] cfg80211: free the object allocated in wiphy_apply_custom_regulatory
  2021-07-23  5:09 [PATCH] cfg80211: free the object allocated in wiphy_apply_custom_regulatory Dongliang Mu
  2021-07-23  5:16 ` Dongliang Mu
  2021-07-23  8:37 ` Johannes Berg
@ 2021-07-23  9:18 ` xiaoqiang zhao
  2021-07-23  9:25   ` Dongliang Mu
  2 siblings, 1 reply; 12+ messages in thread
From: xiaoqiang zhao @ 2021-07-23  9:18 UTC (permalink / raw)
  To: Dongliang Mu, Johannes Berg, Kalle Valo, David S. Miller,
	Jakub Kicinski, Luca Coelho, Ilan Peer
  Cc: syzbot+1638e7c770eef6b6c0d0, Johannes Berg, linux-wireless,
	netdev, linux-kernel



在 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: syzbot+1638e7c770eef6b6c0d0@syzkaller.appspotmail.com
> Fixes: beee24695157 ("cfg80211: Save the regulatory domain when setting custom regulatory")
> Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> ---
>  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);
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] cfg80211: free the object allocated in wiphy_apply_custom_regulatory
  2021-07-23  9:18 ` xiaoqiang zhao
@ 2021-07-23  9:25   ` Dongliang Mu
  2021-07-23  9:36     ` xiaoqiang zhao
  0 siblings, 1 reply; 12+ messages in thread
From: Dongliang Mu @ 2021-07-23  9:25 UTC (permalink / raw)
  To: xiaoqiang zhao
  Cc: Johannes Berg, Kalle Valo, David S. Miller, Jakub Kicinski,
	Luca Coelho, Ilan Peer, syzbot+1638e7c770eef6b6c0d0,
	Johannes Berg, linux-wireless, open list:NETWORKING [GENERAL],
	linux-kernel

On Fri, Jul 23, 2021 at 5:18 PM xiaoqiang zhao
<zhaoxiaoqiang007@gmail.com> 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: syzbot+1638e7c770eef6b6c0d0@syzkaller.appspotmail.com
> > Fixes: beee24695157 ("cfg80211: Save the regulatory domain when setting custom regulatory")
> > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> > ---
> >  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);
> >

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] cfg80211: free the object allocated in wiphy_apply_custom_regulatory
  2021-07-23  9:18     ` Johannes Berg
@ 2021-07-23  9:30       ` Dongliang Mu
  2021-07-23  9:42         ` Johannes Berg
  0 siblings, 1 reply; 12+ messages in thread
From: Dongliang Mu @ 2021-07-23  9:30 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Kalle Valo, David S. Miller, Jakub Kicinski, Luca Coelho,
	Ilan Peer, syzbot+1638e7c770eef6b6c0d0, linux-wireless,
	open list:NETWORKING [GENERAL],
	linux-kernel, Dan Carpenter

On Fri, Jul 23, 2021 at 5:18 PM Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Fri, 2021-07-23 at 17:13 +0800, Dongliang Mu wrote:
> > On Fri, Jul 23, 2021 at 4:37 PM Johannes Berg <johannes@sipsolutions.net> 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
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] cfg80211: free the object allocated in wiphy_apply_custom_regulatory
  2021-07-23  9:25   ` Dongliang Mu
@ 2021-07-23  9:36     ` xiaoqiang zhao
  2021-07-23  9:44       ` Dongliang Mu
  0 siblings, 1 reply; 12+ messages in thread
From: xiaoqiang zhao @ 2021-07-23  9:36 UTC (permalink / raw)
  To: Dongliang Mu
  Cc: Johannes Berg, Kalle Valo, David S. Miller, Jakub Kicinski,
	Luca Coelho, Ilan Peer, syzbot+1638e7c770eef6b6c0d0,
	Johannes Berg, linux-wireless, open list:NETWORKING [GENERAL],
	linux-kernel



在 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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] cfg80211: free the object allocated in wiphy_apply_custom_regulatory
  2021-07-23  9:30       ` Dongliang Mu
@ 2021-07-23  9:42         ` Johannes Berg
  2021-07-23  9:59           ` Dongliang Mu
  0 siblings, 1 reply; 12+ messages in thread
From: Johannes Berg @ 2021-07-23  9:42 UTC (permalink / raw)
  To: Dongliang Mu
  Cc: Kalle Valo, David S. Miller, Jakub Kicinski, Luca Coelho,
	Ilan Peer, syzbot+1638e7c770eef6b6c0d0, linux-wireless,
	open list:NETWORKING [GENERAL],
	linux-kernel, Dan Carpenter

Hi,

On Fri, 2021-07-23 at 17:30 +0800, Dongliang Mu wrote:
> if zhao in the thread is right, we don't need to add this free
> operation to wiphy_free().

Actually, no, that statement is not true.

All that zhao claimed was that the free happens correctly during
unregister (or later), and that is indeed true, since it happens from

ieee80211_unregister_hw()
 -> wiphy_unregister()
 -> wiphy_regulatory_deregister()


However, syzbot of course is also correct. Abstracting a bit and
ignoring mac80211, the problem is that here we assign it before
wiphy_register(), then wiphy_register() doesn't get called or fails, and
therefore we don't call wiphy_unregister(), only wiphy_free().

Hence the leak.

But you can also easily see from that description that it's not related
to hwsim - we should add a secondary round of cleanups in wiphy_free()
or even move the call to wiphy_regulatory_deregister() into
wiphy_free(), we need to look what else this does to see if we can move
it or not.

johannes


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] cfg80211: free the object allocated in wiphy_apply_custom_regulatory
  2021-07-23  9:36     ` xiaoqiang zhao
@ 2021-07-23  9:44       ` Dongliang Mu
  0 siblings, 0 replies; 12+ messages in thread
From: Dongliang Mu @ 2021-07-23  9:44 UTC (permalink / raw)
  To: xiaoqiang zhao
  Cc: Johannes Berg, Kalle Valo, David S. Miller, Jakub Kicinski,
	Luca Coelho, Ilan Peer, syzbot+1638e7c770eef6b6c0d0,
	Johannes Berg, linux-wireless, open list:NETWORKING [GENERAL],
	linux-kernel

On Fri, Jul 23, 2021 at 5:36 PM xiaoqiang zhao
<zhaoxiaoqiang007@gmail.com> wrote:
>
>
>
> 在 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

Thanks very much. This is really helpful.

>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] cfg80211: free the object allocated in wiphy_apply_custom_regulatory
  2021-07-23  9:42         ` Johannes Berg
@ 2021-07-23  9:59           ` Dongliang Mu
  0 siblings, 0 replies; 12+ messages in thread
From: Dongliang Mu @ 2021-07-23  9:59 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Kalle Valo, David S. Miller, Jakub Kicinski, Luca Coelho,
	Ilan Peer, syzbot+1638e7c770eef6b6c0d0, linux-wireless,
	open list:NETWORKING [GENERAL],
	linux-kernel, Dan Carpenter

On Fri, Jul 23, 2021 at 5:42 PM Johannes Berg <johannes@sipsolutions.net> wrote:
>
> Hi,
>
> On Fri, 2021-07-23 at 17:30 +0800, Dongliang Mu wrote:
> > if zhao in the thread is right, we don't need to add this free
> > operation to wiphy_free().
>
> Actually, no, that statement is not true.
>
> All that zhao claimed was that the free happens correctly during
> unregister (or later), and that is indeed true, since it happens from
>
> ieee80211_unregister_hw()
>  -> wiphy_unregister()
>  -> wiphy_regulatory_deregister()
>

Thanks for your explanation. Now the situation is more clear.

>
> However, syzbot of course is also correct. Abstracting a bit and
> ignoring mac80211, the problem is that here we assign it before
> wiphy_register(), then wiphy_register() doesn't get called or fails, and
> therefore we don't call wiphy_unregister(), only wiphy_free().

Yes, you're right. In this case, wiphy_register is not called. We
should not call wiphy_unregister() to clean up anything.

>
> Hence the leak.
>
> But you can also easily see from that description that it's not related
> to hwsim - we should add a secondary round of cleanups in wiphy_free()
> or even move the call to wiphy_regulatory_deregister() into
> wiphy_free(), we need to look what else this does to see if we can move
> it or not.

I agree to move the cleanup operation of regd to wiphy_free API.
That's the partial functionability of this function.

>
> johannes
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-07-23 10:00 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-23  5:09 [PATCH] cfg80211: free the object allocated in wiphy_apply_custom_regulatory Dongliang Mu
2021-07-23  5:16 ` Dongliang Mu
2021-07-23  8:37 ` Johannes Berg
2021-07-23  9:13   ` Dongliang Mu
2021-07-23  9:18     ` Johannes Berg
2021-07-23  9:30       ` Dongliang Mu
2021-07-23  9:42         ` Johannes Berg
2021-07-23  9:59           ` Dongliang Mu
2021-07-23  9:18 ` xiaoqiang zhao
2021-07-23  9:25   ` Dongliang Mu
2021-07-23  9:36     ` xiaoqiang zhao
2021-07-23  9:44       ` Dongliang Mu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).