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: <87y12myaj9.fsf@toke.dk>
Date: Thu, 17 Oct 2024 18:11:22 +0200
From: Toke Høiland-Jørgensen <toke@...hat.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 <edumazet@...gle.com> writes:

> 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: Toke Høiland-Jørgensen <toke@...hat.com>