linux-kernel - Re: [PATCH net 2/2] net: ethernet: oa_tc6: fix tx skb race condition between reference pointers

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <c8498820-e794-49a2-9724-f4d2e1ca8082@intel.com>
Date: Wed, 20 Nov 2024 11:57:28 -0800
From: Jacob Keller <jacob.e.keller@...el.com>
To: Parthiban Veerasooran <parthiban.veerasooran@...rochip.com>,
	<davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<pabeni@...hat.com>, <horms@...nel.org>, <saeedm@...dia.com>,
	<anthony.l.nguyen@...el.com>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <andrew@...n.ch>, <corbet@....net>,
	<linux-doc@...r.kernel.org>, <robh+dt@...nel.org>,
	<krzysztof.kozlowski+dt@...aro.org>, <conor+dt@...nel.org>,
	<devicetree@...r.kernel.org>, <horatiu.vultur@...rochip.com>,
	<ruanjinjie@...wei.com>, <steen.hegelund@...rochip.com>,
	<vladimir.oltean@....com>
CC: <masahiroy@...nel.org>, <alexanderduyck@...com>, <krzk+dt@...nel.org>,
	<robh@...nel.org>, <rdunlap@...radead.org>, <hkallweit1@...il.com>,
	<linux@...linux.org.uk>, <UNGLinuxDriver@...rochip.com>,
	<Thorsten.Kummermehr@...rochip.com>, <Pier.Beruto@...emi.com>,
	<Selvamani.Rajagopal@...emi.com>, <Nicolas.Ferre@...rochip.com>,
	<benjamin.bigler@...nformulastudent.ch>, <linux@...ler.io>,
	<markku.vorne@...power.com>
Subject: Re: [PATCH net 2/2] net: ethernet: oa_tc6: fix tx skb race condition
 between reference pointers



On 11/20/2024 5:51 AM, Parthiban Veerasooran wrote:
> There are two skb pointers to manage tx skb's enqueued from n/w stack.
> waiting_tx_skb pointer points to the tx skb which needs to be processed
> and ongoing_tx_skb pointer points to the tx skb which is being processed.
> 
> SPI thread prepares the tx data chunks from the tx skb pointed by the
> ongoing_tx_skb pointer. When the tx skb pointed by the ongoing_tx_skb is
> processed, the tx skb pointed by the waiting_tx_skb is assigned to
> ongoing_tx_skb and the waiting_tx_skb pointer is assigned with NULL.
> Whenever there is a new tx skb from n/w stack, it will be assigned to
> waiting_tx_skb pointer if it is NULL. Enqueuing and processing of a tx skb
> handled in two different threads.
> 
> Consider a scenario where the SPI thread processed an ongoing_tx_skb and
> it assigns next tx skb from waiting_tx_skb pointer to ongoing_tx_skb
> pointer without doing any NULL check. At this time, if the waiting_tx_skb
> pointer is NULL then ongoing_tx_skb pointer is also assigned with NULL.
> After that, if a new tx skb is assigned to waiting_tx_skb pointer by the
> n/w stack and there is a chance to overwrite the tx skb pointer with NULL
> in the SPI thread. Finally one of the tx skb will be left as unhandled,
> resulting packet missing and memory leak.
> 
> To overcome the above issue, check waiting_tx_skb pointer is not NULL
> along with ongoing_tx_skb pointer's NULL check before proceeding to assign
> the tx skb from waiting_tx_skb pointer to ongoing_tx_skb pointer.
> 
> Fixes: 53fbde8ab21e ("net: ethernet: oa_tc6: implement transmit path to transfer tx ethernet frames")
> Signed-off-by: Parthiban Veerasooran <parthiban.veerasooran@...rochip.com>
> ---
>  drivers/net/ethernet/oa_tc6.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
> index 4c8b0ca922b7..e1e7c6e07966 100644
> --- a/drivers/net/ethernet/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6.c
> @@ -1003,7 +1003,7 @@ static u16 oa_tc6_prepare_spi_tx_buf_for_tx_skbs(struct oa_tc6 *tc6)
>  	 */
>  	for (used_tx_credits = 0; used_tx_credits < tc6->tx_credits;
>  	     used_tx_credits++) {
> -		if (!tc6->ongoing_tx_skb) {
> +		if (!tc6->ongoing_tx_skb && tc6->waiting_tx_skb) {
>  			tc6->ongoing_tx_skb = tc6->waiting_tx_skb;
>  			tc6->waiting_tx_skb = NULL;

It is unclear to me how this additional check completely resolves race
conditions? Is there some other locking or synchronization such the
second thread cannot have updated waiting_tx_skb either prior or after
this check?

This feels like you want some sort of atomic exchange operation...

>  		}