Re: Missing wiphy lock in ieee80211_color_collision_detection_work

[Remi Pommarel <repk@triplefau.lt>]

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