netdev - Re: [PATCH net] net: fix races in netdev_tx_sent_queue()/dev

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: <670ff3b19b7f4_34b350294fc@willemb.c.googlers.com.notmuch>
Date: Wed, 16 Oct 2024 13:11:13 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Eric Dumazet <edumazet@...gle.com>, 
 "David S . Miller" <davem@...emloft.net>, 
 Jakub Kicinski <kuba@...nel.org>, 
 Paolo Abeni <pabeni@...hat.com>
Cc: netdev@...r.kernel.org, 
 eric.dumazet@...il.com, 
 Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH net] net: fix races in
 netdev_tx_sent_queue()/dev_watchdog()

Eric Dumazet wrote:
> Some workloads hit the infamous dev_watchdog() message:
> 
> "NETDEV WATCHDOG: eth0 (xxxx): transmit queue XX timed out"
> 
> It seems possible to hit this even for perfectly normal
> BQL enabled drivers:
> 
> 1) Assume a TX queue was idle for more than dev->watchdog_timeo
>    (5 seconds unless changed by the driver)
> 
> 2) Assume a big packet is sent, exceeding current BQL limit.
> 
> 3) Driver ndo_start_xmit() puts the packet in TX ring,
>    and netdev_tx_sent_queue() is called.
> 
> 4) QUEUE_STATE_STACK_XOFF could be set from netdev_tx_sent_queue()
>    before txq->trans_start has been written.
> 
> 5) txq->trans_start is written later, from netdev_start_xmit()
> 
>     if (rc == NETDEV_TX_OK)
>           txq_trans_update(txq)
> 
> dev_watchdog() running on another cpu could read the old
> txq->trans_start, and then see QUEUE_STATE_STACK_XOFF, because 5)
> did not happen yet.
> 
> To solve the issue, write txq->trans_start right before one XOFF bit
> is set :
> 
> - _QUEUE_STATE_DRV_XOFF from netif_tx_stop_queue()
> - __QUEUE_STATE_STACK_XOFF from netdev_tx_sent_queue()
> 
> From dev_watchdog(), we have to read txq->state before txq->trans_start.
> 
> Add memory barriers to enforce correct ordering.
> 
> In the future, we could avoid writing over txq->trans_start for normal
> operations, and rename this field to txq->xoff_start_time.
> 
> Fixes: bec251bc8b6a ("net: no longer stop all TX queues in dev_watchdog()")
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>

Reviewed-by: Willem de Bruijn <willemb@...gle.com>