netdev - Re: [PATCH net v2] ravb: Fix races between ravb_tx_timeout

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <5028f8cb-7d4e-e026-c4a9-3fabf22a1385@omp.ru>
Date: Thu, 23 Nov 2023 20:15:44 +0300
From: Sergey Shtylyov <s.shtylyov@....ru>
To: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
	"davem@...emloft.net" <davem@...emloft.net>, "edumazet@...gle.com"
	<edumazet@...gle.com>, "kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-renesas-soc@...r.kernel.org" <linux-renesas-soc@...r.kernel.org>
Subject: Re: [PATCH net v2] ravb: Fix races between ravb_tx_timeout_work() and
 net related ops

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