[PATCH net v2] ravb: Fix races between ravb_tx_timeout_work() and net related ops

[PATCH net v2] ravb: Fix races between ravb_tx_timeout_work() and net related ops

[Yoshihiro Shimoda]

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@renesas.com>
---
Changes from v1:
https://lore.kernel.org/all/20231017085341.813335-1-yoshihiro.shimoda.uh@renesas.com/
 - Modify commit description.
 - Use goto in a error path.

 drivers/net/ethernet/renesas/ravb_main.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

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;
+
 	netif_tx_stop_all_queues(ndev);
 
 	/* Stop PTP Clock driver */
@@ -1907,7 +1910,7 @@ static void ravb_tx_timeout_work(struct work_struct *work)
 		 */
 		netdev_err(ndev, "%s: ravb_dmac_init() failed, error %d\n",
 			   __func__, error);
-		return;
+		goto out_unlock;
 	}
 	ravb_emac_init(ndev);
 
@@ -1917,6 +1920,9 @@ static void ravb_tx_timeout_work(struct work_struct *work)
 		ravb_ptp_init(ndev, priv->pdev);
 
 	netif_tx_start_all_queues(ndev);
+
+out_unlock:
+	rtnl_unlock();
 }
 
 /* Packet transmit function for Ethernet AVB */
-- 
2.25.1

Re: [PATCH net v2] ravb: Fix races between ravb_tx_timeout_work() and net related ops

[Sergey Shtylyov]

Hello!

On 10/19/23 2:33 PM, 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@renesas.com>

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]
> 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...

[...]

MBR, Sergey

Re: [PATCH net v2] ravb: Fix races between ravb_tx_timeout_work() and net related ops

[Simon Horman]

On Thu, Oct 19, 2023 at 08:33:08PM +0900, 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@renesas.com>

Reviewed-by: Simon Horman <horms@kernel.org>

RE: [PATCH net v2] ravb: Fix races between ravb_tx_timeout_work() and net related ops

[Yoshihiro Shimoda]

Hello,

> From: Yoshihiro Shimoda, Sent: Thursday, October 19, 2023 8:33 PM
> 
> 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@renesas.com>

Gentle ping. I confirmed that I could apply this patch on
the latest net.git / main branch.

Best regards,
Yoshihiro Shimoda

> ---
> Changes from v1:
<snip URL>
>  - Modify commit description.
>  - Use goto in a error path.
> 
>  drivers/net/ethernet/renesas/ravb_main.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> 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;
> +
>  	netif_tx_stop_all_queues(ndev);
> 
>  	/* Stop PTP Clock driver */
> @@ -1907,7 +1910,7 @@ static void ravb_tx_timeout_work(struct work_struct *work)
>  		 */
>  		netdev_err(ndev, "%s: ravb_dmac_init() failed, error %d\n",
>  			   __func__, error);
> -		return;
> +		goto out_unlock;
>  	}
>  	ravb_emac_init(ndev);
> 
> @@ -1917,6 +1920,9 @@ static void ravb_tx_timeout_work(struct work_struct *work)
>  		ravb_ptp_init(ndev, priv->pdev);
> 
>  	netif_tx_start_all_queues(ndev);
> +
> +out_unlock:
> +	rtnl_unlock();
>  }
> 
>  /* Packet transmit function for Ethernet AVB */
> --
> 2.25.1

Re: [PATCH net v2] ravb: Fix races between ravb_tx_timeout_work() and net related ops

[Jakub Kicinski]

On Tue, 14 Nov 2023 08:52:29 +0000 Yoshihiro Shimoda wrote:
> Gentle ping. I confirmed that I could apply this patch on
> the latest net.git / main branch.

At a glance the suggestion from Sergei makes sense.
You need to reply to him.

RE: [PATCH net v2] ravb: Fix races between ravb_tx_timeout_work() and net related ops

[Yoshihiro Shimoda]

> From: Jakub Kicinski, Sent: Wednesday, November 15, 2023 7:10 AM
> 
> On Tue, 14 Nov 2023 08:52:29 +0000 Yoshihiro Shimoda wrote:
> > Gentle ping. I confirmed that I could apply this patch on
> > the latest net.git / main branch.
> 
> At a glance the suggestion from Sergei makes sense.
> You need to reply to him.

Thank you for your reply. I completely overlooked his suggestion.
I'll check it and reply to him.

Best regards,
Yoshihiro Shimoda

RE: [PATCH net v2] ravb: Fix races between ravb_tx_timeout_work() and net related ops

[Yoshihiro Shimoda]

Hello Sergey,

> From: Sergey Shtylyov, Sent: Thursday, October 19, 2023 9:36 PM
> 
> Hello!
> 
> On 10/19/23 2:33 PM, 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@renesas.com>
> 
> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

Thank you for your review!

> [...]
> > 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.

Best regards,
Yoshihiro Shimoda

> [...]
> 
> MBR, Sergey

Re: [PATCH net v2] ravb: Fix races between ravb_tx_timeout_work() and net related ops

[Sergey Shtylyov]

On 11/15/23 5:09 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@renesas.com>
>>
>> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]

>>> 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...

> Best regards,
> Yoshihiro Shimoda

[...]

MBR, Sergey

RE: [PATCH net v2] ravb: Fix races between ravb_tx_timeout_work() and net related ops

[Yoshihiro Shimoda]

Hello,

> From: Sergey Shtylyov, Sent: Thursday, November 16, 2023 3:31 AM
> 
> On 11/15/23 5:09 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@renesas.com>
> >>
> >> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> 
> [...]
> 
> >>> 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:
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().
                                                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Also, I completely misunderstood the behavior of cancel_{schedule_}work_sync().
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().

Best regards,
Yoshihiro Shimoda


> > Best regards,
> > Yoshihiro Shimoda
> 
> [...]
> 
> MBR, Sergey

Re: [PATCH net v2] ravb: Fix races between ravb_tx_timeout_work() and net related ops

[Sergey Shtylyov]

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@renesas.com>
>>>>
>>>> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>>
>> [...]
>>
>>>>> 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

RE: [PATCH net v2] ravb: Fix races between ravb_tx_timeout_work() and net related ops

[Yoshihiro Shimoda]

> 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@renesas.com>
> >>>>
> >>>> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> >>
> >> [...]
> >>
> >>>>> 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

Re: [PATCH net v2] ravb: Fix races between ravb_tx_timeout_work() and net related ops

[Sergey Shtylyov]

Hello!

   Sorry for the late reply -- the damn dozen of the AVB patches fell on me this
Monday... :-/

On 11/17/23 3:07 AM, Yoshihiro Shimoda wrote:
[...]

>>> 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, ...)

   You seem to have lost ! here. :-)

>     -> __queue_work()

   Ah! Now it makes perfect sense. Sorry, this somehow evaded me... :-/

> ---
> 
> Best regards,
> Yoshihiro Shimoda

[...]

MBR, Sergey