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()...
> Also, I completely misunderstood the behavior of cancel_{schedule_}work_sync().
cancel_{delayed_}work_sync(), you meant...
> 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... :-/
> Best regards,
> Yoshihiro Shimoda
[...]
MBR, Sergey
