> From: Sergey Shtylyov, Sent: Friday, November 17, 2023 3:12 AM
>
> On 11/16/23 5:15 AM, Yoshihiro Shimoda wrote:
> [...]
>
> >>>>> Fix races between ravb_tx_timeout_work() and functions of net_device_ops
> >>>>> and ethtool_ops by using rtnl_trylock() and rtnl_unlock(). Note that
> >>>>> since ravb_close() is under the rtnl lock and calls cancel_work_sync(),
> >>>>> ravb_tx_timeout_work() should calls rtnl_trylock(). Otherwise, a deadlock
> >>>>> may happen in ravb_tx_timeout_work() like below:
> >>>>>
> >>>>> CPU0 CPU1
> >>>>> ravb_tx_timeout()
> >>>>> schedule_work()
> >>>>> ...
> >>>>> __dev_close_many()
> >>>>> // Under rtnl lock
> >>>>> ravb_close()
> >>>>> cancel_work_sync()
> >>>>> // Waiting
> >>>>> ravb_tx_timeout_work()
> >>>>> rtnl_lock()
> >>>>> // This is possible to cause a deadlock
> >>>>>
> >>>>> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
> >>>>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
> >>>>
> >>>> Reviewed-by: Sergey Shtylyov <s.shtylyov@xxxxxx>
> >>
> >> [...]
> >>
> >>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> >>>>> index 0ef0b88b7145..300c1885e1e1 100644
> >>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
> >>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> >>>>> @@ -1874,6 +1874,9 @@ static void ravb_tx_timeout_work(struct work_struct *work)
> >>>>> struct net_device *ndev = priv->ndev;
> >>>>> int error;
> >>>>>
> >>>>> + if (!rtnl_trylock())
> >>>>> + return;
> >>>>
> >>>> I wonder if we should reschedule the work here...
> >>>
> >>> I think so. But, it should reschedule the work if the netif is still running because
> >>> Use-after-free issue happens again when cancel_work_sync() is calling. Also, I also think
> >>> we should use schedule_delayed_work() instead. So, I'll submit such a patch as v3.
> >>
> >> I'm not really sure about that one. Note that cancel_work_sync() should
> >> work with the works requeueing themselves, the comments say...
> >
> > I'm sorry, I completely mistook to explain this... I should have said:
>
> Don't worry, my uncertainty was about using the "delayed" flavor of
> the works. :-)
>
> > It should not reschedule the work if the netif is not running because
> > ~~~ ~~~
> > use-after-free issue happens again when cancel_work_sync() is called from ravb_remove().
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Well, it's called from ravb_close() -- which is called by the networking
> core when unregister_netdev() is called bt ravb_remove()...
You're correct. I'm sorry for my lack explanation again...
> > Also, I completely misunderstood the behavior of cancel_{schedule_}work_sync().
>
> cancel_{delayed_}work_sync(), you meant...
Yes...
> > In the function(s), since WORK_STRUCT_PENDING_BIT is set, schedule_{delayed_}work()
> > will not schedule the work anymore. So, I'll drop a condition netif_running()
> > from the ravb_tx_timeout_work().
>
> Hm, this caused me to rummage in the work queue code for more time than
> I could afford... still not sure what you meant... :-/
I'm sorry for bothering you about this topic...
In the v3 patch, the rescheduling code was:
---
+ if (!rtnl_trylock()) {
+ if (netif_running(ndev))
+ schedule_delayed_work(&priv->work, msecs_to_jiffies(10));
+ return;
+ }
---
However, we can implement this like the following:
---
+ if (!rtnl_trylock()) {
+ schedule_delayed_work(&priv->work, msecs_to_jiffies(10));
+ return;
+ }
---
The schedule_{delayed}_work() will not be queued after cancel_{delayed_}work_sync()
was called, because WORK_STRUCT_PENDING_BIT was set in cancel_{delayed_}work_sync()
like the following:
---
cancel_work_sync()
-> __cancel_work_timer()
-> try_to_grab_pending()
-> if (!test_and_set_bit(WORK_STRUCT_PENDING_BIT, ...)
schedule_work()
-> queue_work()
-> queue_work_on()
-> if (test_and_set_bit(WORK_STRUCT_PENDING_BIT, ...)
-> __queue_work()
---
Best regards,
Yoshihiro Shimoda
> > Best regards,
> > Yoshihiro Shimoda
> [...]
>
> MBR, Sergey
