Missing wiphy lock in ieee80211_color_collision_detection_work

Missing wiphy lock in ieee80211_color_collision_detection_work

[Nicolas Escande]

Hi guys,

Running a pretty hacked up kernel under lockdep, I've had a few splats like this

	[   75.453180] Call trace:
	[   75.455595]  cfg80211_bss_color_notify+0x220/0x260
	[   75.460341]  ieee80211_color_collision_detection_work+0x4c/0x5c
	[   75.466205]  process_one_work+0x434/0x6ec
	[   75.470169]  worker_thread+0x9c/0x624
	[   75.473794]  kthread+0x1a4/0x1ac
	[   75.476987]  ret_from_fork+0x10/0x20

Which shows we are calling cfg80211_obss_color_collision_notify and thus
cfg80211_bss_color_notify from ieee80211_color_collision_detection_work without
holding the wiphy's mtx.

It seems that we should either explicitely lock the phy using something like

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 847304a3a29a..481e1550cb21 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -4833,9 +4833,12 @@ void ieee80211_color_collision_detection_work(struct work_struct *work)
 		container_of(delayed_work, struct ieee80211_link_data,
 			     color_collision_detect_work);
 	struct ieee80211_sub_if_data *sdata = link->sdata;
+	struct ieee80211_local *local = sdata->local;
 
+	wiphy_lock(local->hw.wiphy);
 	cfg80211_obss_color_collision_notify(sdata->dev, link->color_bitmap,
 					     link->link_id);
+	wiphy_unlock(local->hw.wiphy);
 }
 
 void ieee80211_color_change_finish(struct ieee80211_vif *vif, u8 link_id)

Or switch to using the wiphy_work_queue infrastructure.

Did I miss something ? Which one should we do ? I'm not sure of all the
implications of switching to the wiphy work queue and why it did not get
converted like the color_change_finalize_work stuff ?

Re: Missing wiphy lock in ieee80211_color_collision_detection_work

[Nicolas Cavallari]

On 19/09/2024 10:15, Nicolas Escande wrote:
> Hi guys,
> 
> Running a pretty hacked up kernel under lockdep, I've had a few splats like this
> 
> 	[   75.453180] Call trace:
> 	[   75.455595]  cfg80211_bss_color_notify+0x220/0x260
> 	[   75.460341]  ieee80211_color_collision_detection_work+0x4c/0x5c
> 	[   75.466205]  process_one_work+0x434/0x6ec
> 	[   75.470169]  worker_thread+0x9c/0x624
> 	[   75.473794]  kthread+0x1a4/0x1ac
> 	[   75.476987]  ret_from_fork+0x10/0x20
> 
> Which shows we are calling cfg80211_obss_color_collision_notify and thus
> cfg80211_bss_color_notify from ieee80211_color_collision_detection_work without
> holding the wiphy's mtx.
> 
> It seems that we should either explicitely lock the phy using something like
> 
> diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
> index 847304a3a29a..481e1550cb21 100644
> --- a/net/mac80211/cfg.c
> +++ b/net/mac80211/cfg.c
> @@ -4833,9 +4833,12 @@ void ieee80211_color_collision_detection_work(struct work_struct *work)
>   		container_of(delayed_work, struct ieee80211_link_data,
>   			     color_collision_detect_work);
>   	struct ieee80211_sub_if_data *sdata = link->sdata;
> +	struct ieee80211_local *local = sdata->local;
>   
> +	wiphy_lock(local->hw.wiphy);
>   	cfg80211_obss_color_collision_notify(sdata->dev, link->color_bitmap,
>   					     link->link_id);
> +	wiphy_unlock(local->hw.wiphy);
>   }
>   
>   void ieee80211_color_change_finish(struct ieee80211_vif *vif, u8 link_id)
> 
> Or switch to using the wiphy_work_queue infrastructure.
> 
> Did I miss something ? Which one should we do ? I'm not sure of all the
> implications of switching to the wiphy work queue and why it did not get
> converted like the color_change_finalize_work stuff ?

ieee80211_color_collision_detection_work() used to lock the wdev mutex, now it 
does not hold anything since 076fc8775da("wifi: cfg80211: remove wdev mutex")

Also the rate limiting uses delayed_work_pending(), There is no wiphy work queue 
equivalent AFAIK, so the explicit lock is probably the way to go.

Re: Missing wiphy lock in ieee80211_color_collision_detection_work

[Johannes Berg]

On Thu, 2024-09-19 at 12:02 +0200, Nicolas Cavallari wrote:
> 
> > Did I miss something ? Which one should we do ? I'm not sure of all the
> > implications of switching to the wiphy work queue and why it did not get
> > converted like the color_change_finalize_work stuff ?
> 
> ieee80211_color_collision_detection_work() used to lock the wdev mutex, now it 
> does not hold anything since 076fc8775da("wifi: cfg80211: remove wdev mutex")
> 
> Also the rate limiting uses delayed_work_pending(), There is no wiphy work queue 
> equivalent AFAIK, so the explicit lock is probably the way to go.

That won't work, it's cancel_delayed_work_sync() under the wiphy mutex,
so that'll cause deadlocks (and should cause lockdep complaints about
them.)

johannes

Re: Missing wiphy lock in ieee80211_color_collision_detection_work

[Nicolas Escande]

On Thu Sep 19, 2024 at 12:22 PM CEST, Johannes Berg wrote:
> > 
> > Also the rate limiting uses delayed_work_pending(), There is no wiphy work queue 
> > equivalent AFAIK, so the explicit lock is probably the way to go.
>
> That won't work, it's cancel_delayed_work_sync() under the wiphy mutex,
> so that'll cause deadlocks (and should cause lockdep complaints about
> them.)
>
> johannes

Ok then we'll go the wiphy work queue way.

Re: Missing wiphy lock in ieee80211_color_collision_detection_work

[Remi Pommarel]

On Thu, Sep 19, 2024 at 12:22:10PM +0200, Johannes Berg wrote:
> On Thu, 2024-09-19 at 12:02 +0200, Nicolas Cavallari wrote:
> > 
> > > Did I miss something ? Which one should we do ? I'm not sure of all the
> > > implications of switching to the wiphy work queue and why it did not get
> > > converted like the color_change_finalize_work stuff ?
> > 
> > ieee80211_color_collision_detection_work() used to lock the wdev mutex, now it 
> > does not hold anything since 076fc8775da("wifi: cfg80211: remove wdev mutex")
> > 
> > Also the rate limiting uses delayed_work_pending(), There is no wiphy work queue 
> > equivalent AFAIK, so the explicit lock is probably the way to go.
> 
> That won't work, it's cancel_delayed_work_sync() under the wiphy mutex,
> so that'll cause deadlocks (and should cause lockdep complaints about
> them.)

Yes you are right, and AFAIU that is this kind of issue using wiphy work
queue would prevent. With wiphy work queue if wiphy_delayed_work_cancel
is called with wiphy lock held, work cannot be running after it returns;
making it handy to replace cancel_delayed_work_sync() with.

So, in my opinion, switching to wiphy work queue seems to be the
prefered solution here.

While there is no wiphy work queue equivalent of delayed_work_pending(),
I think using timer_pending(&link->color_collision_detect_work->timer)
to replace delayed_work_pending(), even if the semantic is a bit
different, would be ok to fulfill the rate limiting purpose. Having the
same delayed_work_pending() semantics on wiphy work queue would require
to take wiphy lock which seem a bit superfluous here.

Does that make sense ?

Thanks

-- 
Remi

Re: Missing wiphy lock in ieee80211_color_collision_detection_work

[Remi Pommarel]

On Fri, Sep 20, 2024 at 10:28:55AM +0200, Remi Pommarel wrote:
> On Thu, Sep 19, 2024 at 12:22:10PM +0200, Johannes Berg wrote:
> > On Thu, 2024-09-19 at 12:02 +0200, Nicolas Cavallari wrote:
> > > 
> > > > Did I miss something ? Which one should we do ? I'm not sure of all the
> > > > implications of switching to the wiphy work queue and why it did not get
> > > > converted like the color_change_finalize_work stuff ?
> > > 
> > > ieee80211_color_collision_detection_work() used to lock the wdev mutex, now it 
> > > does not hold anything since 076fc8775da("wifi: cfg80211: remove wdev mutex")
> > > 
> > > Also the rate limiting uses delayed_work_pending(), There is no wiphy work queue 
> > > equivalent AFAIK, so the explicit lock is probably the way to go.
> > 
> > That won't work, it's cancel_delayed_work_sync() under the wiphy mutex,
> > so that'll cause deadlocks (and should cause lockdep complaints about
> > them.)
> 
> Yes you are right, and AFAIU that is this kind of issue using wiphy work
> queue would prevent. With wiphy work queue if wiphy_delayed_work_cancel
> is called with wiphy lock held, work cannot be running after it returns;
> making it handy to replace cancel_delayed_work_sync() with.
> 
> So, in my opinion, switching to wiphy work queue seems to be the
> prefered solution here.
> 
> While there is no wiphy work queue equivalent of delayed_work_pending(),
> I think using timer_pending(&link->color_collision_detect_work->timer)
> to replace delayed_work_pending(), even if the semantic is a bit
> different, would be ok to fulfill the rate limiting purpose. Having the
> same delayed_work_pending() semantics on wiphy work queue would require
> to take wiphy lock which seem a bit superfluous here.

Forgot to mention that I am currently running a wiphy work version of
ieee80211_color_collision_detection_work() using timer_pending() as a
delayed_work_pending() substitute since a couple of hours and lockdep
did not complain so far.

-- 
Remi

Re: Missing wiphy lock in ieee80211_color_collision_detection_work

[Johannes Berg]

On Fri, 2024-09-20 at 10:41 +0200, Remi Pommarel wrote:
> > > That won't work, it's cancel_delayed_work_sync() under the wiphy mutex,
> > > so that'll cause deadlocks (and should cause lockdep complaints about
> > > them.)
> > 
> > Yes you are right, and AFAIU that is this kind of issue using wiphy work
> > queue would prevent. With wiphy work queue if wiphy_delayed_work_cancel
> > is called with wiphy lock held, work cannot be running after it returns;
> > making it handy to replace cancel_delayed_work_sync() with.

Right, and safe under the lock, since the lock is taken externally to
the actual work function. It either is completed or still on the list,
both cases are handled safely (do nothing and remove, respectively.)

> > So, in my opinion, switching to wiphy work queue seems to be the
> > prefered solution here.

Yes.

> > While there is no wiphy work queue equivalent of delayed_work_pending(),

Maybe we should add it?

> > I think using timer_pending(&link->color_collision_detect_work->timer)
> > to replace delayed_work_pending(), even if the semantic is a bit
> > different, would be ok to fulfill the rate limiting purpose.

I think you're right. We could as well check list_empty() or so, but it
wouldn't make a big difference. As you say:

> > Having the
> > same delayed_work_pending() semantics on wiphy work queue would require
> > to take wiphy lock

To really have it known _precisely_, that's true.

> >  which seem a bit superfluous here.

It's actually simply also not possible - if we could sleep in
ieee80211_obss_color_collision_notify() and take the wiphy mutex, we'd
probably have done this completely differently :)

And a hypothetical wiphy_delayed_work_pending() API should therefore not
be required to be called with the wiphy mutex held.

I think that perhaps we should add such a trivial inline instead of
open-coding the timer_pending() check, but I'm not sure whether or not
it should also check the list (i.e. check if timer has expired, but work
hasn't started yet): on the one hand it seems more appropriate, and if
actually holding the wiphy mutex it would in fact be completely correct,
on the other hand maybe it encourages being sloppily thinking the return
value is always perfect?

Right now I'd tend to have the check and document that it's only
guaranteed when under the wiphy mutex.


> Forgot to mention that I am currently running a wiphy work version of
> ieee80211_color_collision_detection_work() using timer_pending() as a
> delayed_work_pending() substitute since a couple of hours and lockdep
> did not complain so far.

:)

johannes

Re: Missing wiphy lock in ieee80211_color_collision_detection_work

[Remi Pommarel]

On Fri, Sep 20, 2024 at 12:00:08PM +0200, Johannes Berg wrote:
> On Fri, 2024-09-20 at 10:41 +0200, Remi Pommarel wrote:
> > > I think using timer_pending(&link->color_collision_detect_work->timer)
> > > to replace delayed_work_pending(), even if the semantic is a bit
> > > different, would be ok to fulfill the rate limiting purpose.
> 
> I think you're right. We could as well check list_empty() or so, but it
> wouldn't make a big difference. As you say:
> 
> > > Having the
> > > same delayed_work_pending() semantics on wiphy work queue would require
> > > to take wiphy lock
> 
> To really have it known _precisely_, that's true.
> 
> > >  which seem a bit superfluous here.
> 
> It's actually simply also not possible - if we could sleep in
> ieee80211_obss_color_collision_notify() and take the wiphy mutex, we'd
> probably have done this completely differently :)
> 
> And a hypothetical wiphy_delayed_work_pending() API should therefore not
> be required to be called with the wiphy mutex held.
> 
> I think that perhaps we should add such a trivial inline instead of
> open-coding the timer_pending() check, but I'm not sure whether or not
> it should also check the list (i.e. check if timer has expired, but work
> hasn't started yet): on the one hand it seems more appropriate, and if
> actually holding the wiphy mutex it would in fact be completely correct,
> on the other hand maybe it encourages being sloppily thinking the return
> value is always perfect?
> 
> Right now I'd tend to have the check and document that it's only
> guaranteed when under the wiphy mutex.

I had this exact train of thought and was replying that I did agree on
that, and then I checked the other *_pending semantics for which I tend
to forget the details. IIUC timer_pending() and work_pending() can both
return false if callback is still running (or even not yet started).
Thus the hypothetical wiphy_work_pending() semantics could allow to
implement it using timer_pending().

Adding a list_empty() check while making it more "precise" does not make
it "perfect" (even if there is no clear notion of what perfection should
be here) even with wiphy lock held. Indeed if wiphy_work_pending() is
called precisely after delayed work timer has cleared its pending state
but before the callback (i.e wiphy_delayed_work_timer()) has added the
work in the list both !timer_pending() and list_empty(work->entry) would
be true. So there is a small window where wiphy_work_pending() wouldn't
be more precise than just checking timer_pending() as show below:

      CPU0                               CPU1
 expire_timers
   detach_timer
                                    wiphy_work_pending() -> return false
   timer->function
     wiphy_work_queue
       list_add_tail()
                                    wiphy_work_pending() -> return false

-- 
Remi

Re: Missing wiphy lock in ieee80211_color_collision_detection_work

[Remi Pommarel]

On Fri, Sep 20, 2024 at 02:07:19PM +0200, Remi Pommarel wrote:
> On Fri, Sep 20, 2024 at 12:00:08PM +0200, Johannes Berg wrote:
> > On Fri, 2024-09-20 at 10:41 +0200, Remi Pommarel wrote:
> > > > I think using timer_pending(&link->color_collision_detect_work->timer)
> > > > to replace delayed_work_pending(), even if the semantic is a bit
> > > > different, would be ok to fulfill the rate limiting purpose.
> > 
> > I think you're right. We could as well check list_empty() or so, but it
> > wouldn't make a big difference. As you say:
> > 
> > > > Having the
> > > > same delayed_work_pending() semantics on wiphy work queue would require
> > > > to take wiphy lock
> > 
> > To really have it known _precisely_, that's true.
> > 
> > > >  which seem a bit superfluous here.
> > 
> > It's actually simply also not possible - if we could sleep in
> > ieee80211_obss_color_collision_notify() and take the wiphy mutex, we'd
> > probably have done this completely differently :)
> > 
> > And a hypothetical wiphy_delayed_work_pending() API should therefore not
> > be required to be called with the wiphy mutex held.
> > 
> > I think that perhaps we should add such a trivial inline instead of
> > open-coding the timer_pending() check, but I'm not sure whether or not
> > it should also check the list (i.e. check if timer has expired, but work
> > hasn't started yet): on the one hand it seems more appropriate, and if
> > actually holding the wiphy mutex it would in fact be completely correct,
> > on the other hand maybe it encourages being sloppily thinking the return
> > value is always perfect?
> > 
> > Right now I'd tend to have the check and document that it's only
> > guaranteed when under the wiphy mutex.
> 
> I had this exact train of thought and was replying that I did agree on
> that, and then I checked the other *_pending semantics for which I tend
> to forget the details. IIUC timer_pending() and work_pending() can both
> return false if callback is still running (or even not yet started).
> Thus the hypothetical wiphy_work_pending() semantics could allow to
> implement it using timer_pending().
> 
> Adding a list_empty() check while making it more "precise" does not make
> it "perfect" (even if there is no clear notion of what perfection should
> be here) even with wiphy lock held. Indeed if wiphy_work_pending() is
> called precisely after delayed work timer has cleared its pending state
> but before the callback (i.e wiphy_delayed_work_timer()) has added the
> work in the list both !timer_pending() and list_empty(work->entry) would
> be true. So there is a small window where wiphy_work_pending() wouldn't
> be more precise than just checking timer_pending() as show below:
> 
>       CPU0                               CPU1
>  expire_timers
>    detach_timer
>                                     wiphy_work_pending() -> return false
>    timer->function
>      wiphy_work_queue
>        list_add_tail()
>                                     wiphy_work_pending() -> return false

I meant wiphy_work_pending() -> return true here ^.

-- 
Remi

Re: Missing wiphy lock in ieee80211_color_collision_detection_work

[Johannes Berg]

On Fri, 2024-09-20 at 14:10 +0200, Remi Pommarel wrote:
> On Fri, Sep 20, 2024 at 02:07:19PM +0200, Remi Pommarel wrote:
> > On Fri, Sep 20, 2024 at 12:00:08PM +0200, Johannes Berg wrote:
> > > On Fri, 2024-09-20 at 10:41 +0200, Remi Pommarel wrote:
> > > > > I think using timer_pending(&link->color_collision_detect_work->timer)
> > > > > to replace delayed_work_pending(), even if the semantic is a bit
> > > > > different, would be ok to fulfill the rate limiting purpose.
> > > 
> > > I think you're right. We could as well check list_empty() or so, but it
> > > wouldn't make a big difference. As you say:
> > > 
> > > > > Having the
> > > > > same delayed_work_pending() semantics on wiphy work queue would require
> > > > > to take wiphy lock
> > > 
> > > To really have it known _precisely_, that's true.
> > > 
> > > > >  which seem a bit superfluous here.
> > > 
> > > It's actually simply also not possible - if we could sleep in
> > > ieee80211_obss_color_collision_notify() and take the wiphy mutex, we'd
> > > probably have done this completely differently :)
> > > 
> > > And a hypothetical wiphy_delayed_work_pending() API should therefore not
> > > be required to be called with the wiphy mutex held.
> > > 
> > > I think that perhaps we should add such a trivial inline instead of
> > > open-coding the timer_pending() check, but I'm not sure whether or not
> > > it should also check the list (i.e. check if timer has expired, but work
> > > hasn't started yet): on the one hand it seems more appropriate, and if
> > > actually holding the wiphy mutex it would in fact be completely correct,
> > > on the other hand maybe it encourages being sloppily thinking the return
> > > value is always perfect?
> > > 
> > > Right now I'd tend to have the check and document that it's only
> > > guaranteed when under the wiphy mutex.
> > 
> > I had this exact train of thought and was replying that I did agree on
> > that, and then I checked the other *_pending semantics for which I tend
> > to forget the details.

Haha, yes, no kidding.

> > IIUC timer_pending() and work_pending() can both
> > return false if callback is still running (or even not yet started).
> > Thus the hypothetical wiphy_work_pending() semantics could allow to
> > implement it using timer_pending().
> > 
> > Adding a list_empty() check while making it more "precise" does not make
> > it "perfect" (even if there is no clear notion of what perfection should
> > be here) even with wiphy lock held. Indeed if wiphy_work_pending() is
> > called precisely after delayed work timer has cleared its pending state
> > but before the callback (i.e wiphy_delayed_work_timer()) has added the
> > work in the list both !timer_pending() and list_empty(work->entry) would
> > be true. So there is a small window where wiphy_work_pending() wouldn't
> > be more precise than just checking timer_pending() as show below:
> > 
> >       CPU0                               CPU1
> >  expire_timers
> >    detach_timer
> >                                     wiphy_work_pending() -> return false
> >    timer->function
> >      wiphy_work_queue
> >        list_add_tail()
> >                                     wiphy_work_pending() -> return false
> 
> I meant wiphy_work_pending() -> return true here ^.
> 
Good point! The second call could even be before the list_add_tail and
still return false, so yeah, wiphy lock doesn't do anything.

But I guess we can live with both of these too, given sufficient
documentation :)

Agree also with Nicolas though that we could just ratelimit this
differently too, this was just a convenient way of achieving it with the
existing infrastructure.

johannes

Re: Missing wiphy lock in ieee80211_color_collision_detection_work

[Remi Pommarel]

On Fri, Sep 20, 2024 at 02:12:23PM +0200, Johannes Berg wrote:
> On Fri, 2024-09-20 at 14:10 +0200, Remi Pommarel wrote:
> > On Fri, Sep 20, 2024 at 02:07:19PM +0200, Remi Pommarel wrote:
> > > On Fri, Sep 20, 2024 at 12:00:08PM +0200, Johannes Berg wrote:
> > > > On Fri, 2024-09-20 at 10:41 +0200, Remi Pommarel wrote:
> > > > > > I think using timer_pending(&link->color_collision_detect_work->timer)
> > > > > > to replace delayed_work_pending(), even if the semantic is a bit
> > > > > > different, would be ok to fulfill the rate limiting purpose.
> > > > 
> > > > I think you're right. We could as well check list_empty() or so, but it
> > > > wouldn't make a big difference. As you say:
> > > > 
> > > > > > Having the
> > > > > > same delayed_work_pending() semantics on wiphy work queue would require
> > > > > > to take wiphy lock
> > > > 
> > > > To really have it known _precisely_, that's true.
> > > > 
> > > > > >  which seem a bit superfluous here.
> > > > 
> > > > It's actually simply also not possible - if we could sleep in
> > > > ieee80211_obss_color_collision_notify() and take the wiphy mutex, we'd
> > > > probably have done this completely differently :)
> > > > 
> > > > And a hypothetical wiphy_delayed_work_pending() API should therefore not
> > > > be required to be called with the wiphy mutex held.
> > > > 
> > > > I think that perhaps we should add such a trivial inline instead of
> > > > open-coding the timer_pending() check, but I'm not sure whether or not
> > > > it should also check the list (i.e. check if timer has expired, but work
> > > > hasn't started yet): on the one hand it seems more appropriate, and if
> > > > actually holding the wiphy mutex it would in fact be completely correct,
> > > > on the other hand maybe it encourages being sloppily thinking the return
> > > > value is always perfect?
> > > > 
> > > > Right now I'd tend to have the check and document that it's only
> > > > guaranteed when under the wiphy mutex.
> > > 
> > > I had this exact train of thought and was replying that I did agree on
> > > that, and then I checked the other *_pending semantics for which I tend
> > > to forget the details.
> 
> Haha, yes, no kidding.
> 
> > > IIUC timer_pending() and work_pending() can both
> > > return false if callback is still running (or even not yet started).
> > > Thus the hypothetical wiphy_work_pending() semantics could allow to
> > > implement it using timer_pending().
> > > 
> > > Adding a list_empty() check while making it more "precise" does not make
> > > it "perfect" (even if there is no clear notion of what perfection should
> > > be here) even with wiphy lock held. Indeed if wiphy_work_pending() is
> > > called precisely after delayed work timer has cleared its pending state
> > > but before the callback (i.e wiphy_delayed_work_timer()) has added the
> > > work in the list both !timer_pending() and list_empty(work->entry) would
> > > be true. So there is a small window where wiphy_work_pending() wouldn't
> > > be more precise than just checking timer_pending() as show below:
> > > 
> > >       CPU0                               CPU1
> > >  expire_timers
> > >    detach_timer
> > >                                     wiphy_work_pending() -> return false
> > >    timer->function
> > >      wiphy_work_queue
> > >        list_add_tail()
> > >                                     wiphy_work_pending() -> return false
> > 
> > I meant wiphy_work_pending() -> return true here ^.
> > 
> Good point! The second call could even be before the list_add_tail and
> still return false, so yeah, wiphy lock doesn't do anything.
> 
> But I guess we can live with both of these too, given sufficient
> documentation :)

Also, sorry for being bothersome, wiphy_delayed_work_queue() is not a
genuine equivalent of queue_delayed_work() as the latter does not
requeue work if already scheduled while the former does so effectively
changing the delayed work deadline (very mod_delayed_work() alike).

If we wanted to fix that, ieee80211_obss_color_collision_notify() would
simply use wiphy_delayed_work_queue() directly. But other calls would
have to be rechecked and switched to use wiphy_delayed_work_mod() if
needed be.

> 
> Agree also with Nicolas though that we could just ratelimit this
> differently too, this was just a convenient way of achieving it with the
> existing infrastructure.

Yes that would work also but still need to convert into wiphy work the
color collision work to avoid deadlock.

-- 
Remi

Re: Missing wiphy lock in ieee80211_color_collision_detection_work

[Nicolas Cavallari]

On 20/09/2024 10:28, Remi Pommarel wrote:
> On Thu, Sep 19, 2024 at 12:22:10PM +0200, Johannes Berg wrote:
>> On Thu, 2024-09-19 at 12:02 +0200, Nicolas Cavallari wrote:
>>>
>>>> Did I miss something ? Which one should we do ? I'm not sure of all the
>>>> implications of switching to the wiphy work queue and why it did not get
>>>> converted like the color_change_finalize_work stuff ?
>>>
>>> ieee80211_color_collision_detection_work() used to lock the wdev mutex, now it
>>> does not hold anything since 076fc8775da("wifi: cfg80211: remove wdev mutex")
>>>
>>> Also the rate limiting uses delayed_work_pending(), There is no wiphy work queue
>>> equivalent AFAIK, so the explicit lock is probably the way to go.
>>
>> That won't work, it's cancel_delayed_work_sync() under the wiphy mutex,
>> so that'll cause deadlocks (and should cause lockdep complaints about
>> them.)
> 
> Yes you are right, and AFAIU that is this kind of issue using wiphy work
> queue would prevent. With wiphy work queue if wiphy_delayed_work_cancel
> is called with wiphy lock held, work cannot be running after it returns;
> making it handy to replace cancel_delayed_work_sync() with.
> 
> So, in my opinion, switching to wiphy work queue seems to be the
> prefered solution here.
> 
> While there is no wiphy work queue equivalent of delayed_work_pending(),
> I think using timer_pending(&link->color_collision_detect_work->timer)
> to replace delayed_work_pending(), even if the semantic is a bit
> different, would be ok to fulfill the rate limiting purpose. Having the
> same delayed_work_pending() semantics on wiphy work queue would require
> to take wiphy lock which seem a bit superfluous here.
> 
> Does that make sense ?

At a cost of a spin_trylock, we can also simply take the __ratelimit() option 
(or code an equivalent) and schedule a wiphy_work immediately.

The goal is just to rate limit the work (otherwise netlink is flooded and the 
system is hosed). queue_delayed_work() and delayed_work_pending() was just an 
easy way of implementing it since it had to be a work anyway.