| 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
| ||
|
Message-ID: <0ee096e8-8a72-40fc-824d-aaacfbd12dec@alu.unizg.hr>
Date: Wed, 18 Oct 2023 21:42:05 +0200
From: Mirsad Todorovac <mirsad.todorovac@....unizg.hr>
To: Simon Horman <horms@...nel.org>
Cc: Heiner Kallweit <hkallweit1@...il.com>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, nic_swsd@...ltek.com,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Marco Elver <elver@...gle.com>
Subject: Re: [PATCH v2 3/3] r8169: fix the KCSAN reported data-race in rtl_tx
while reading TxDescArray[entry].opts1
On 10/18/23 15:56, Simon Horman wrote:
> On Tue, Oct 17, 2023 at 10:43:36PM +0200, Mirsad Todorovac wrote:
>> On 10/17/23 22:01, Simon Horman wrote:
>>> On Mon, Oct 16, 2023 at 11:47:56PM +0200, Mirsad Goran Todorovac wrote:
>>>> KCSAN reported the following data-race:
>>>>
>>>> ==================================================================
>>>> BUG: KCSAN: data-race in rtl8169_poll (drivers/net/ethernet/realtek/r8169_main.c:4368 drivers/net/ethernet/realtek/r8169_main.c:4581) r8169
>>>>
>>>> race at unknown origin, with read to 0xffff888140d37570 of 4 bytes by interrupt on cpu 21:
>>>> rtl8169_poll (drivers/net/ethernet/realtek/r8169_main.c:4368 drivers/net/ethernet/realtek/r8169_main.c:4581) r8169
>>>> __napi_poll (net/core/dev.c:6527)
>>>> net_rx_action (net/core/dev.c:6596 net/core/dev.c:6727)
>>>> __do_softirq (kernel/softirq.c:553)
>>>> __irq_exit_rcu (kernel/softirq.c:427 kernel/softirq.c:632)
>>>> irq_exit_rcu (kernel/softirq.c:647)
>>>> sysvec_apic_timer_interrupt (arch/x86/kernel/apic/apic.c:1074 (discriminator 14))
>>>> asm_sysvec_apic_timer_interrupt (./arch/x86/include/asm/idtentry.h:645)
>>>> cpuidle_enter_state (drivers/cpuidle/cpuidle.c:291)
>>>> cpuidle_enter (drivers/cpuidle/cpuidle.c:390)
>>>> call_cpuidle (kernel/sched/idle.c:135)
>>>> do_idle (kernel/sched/idle.c:219 kernel/sched/idle.c:282)
>>>> cpu_startup_entry (kernel/sched/idle.c:378 (discriminator 1))
>>>> start_secondary (arch/x86/kernel/smpboot.c:210 arch/x86/kernel/smpboot.c:294)
>>>> secondary_startup_64_no_verify (arch/x86/kernel/head_64.S:433)
>>>>
>>>> value changed: 0xb0000042 -> 0x00000000
>>>>
>>>> Reported by Kernel Concurrency Sanitizer on:
>>>> CPU: 21 PID: 0 Comm: swapper/21 Tainted: G L 6.6.0-rc2-kcsan-00143-gb5cbe7c00aa0 #41
>>>> Hardware name: ASRock X670E PG Lightning/X670E PG Lightning, BIOS 1.21 04/26/2023
>>>> ==================================================================
>>>>
>>>> The read side is in
>>>>
>>>> drivers/net/ethernet/realtek/r8169_main.c
>>>> =========================================
>>>> 4355 static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp,
>>>> 4356 int budget)
>>>> 4357 {
>>>> 4358 unsigned int dirty_tx, bytes_compl = 0, pkts_compl = 0;
>>>> 4359 struct sk_buff *skb;
>>>> 4360
>>>> 4361 dirty_tx = tp->dirty_tx;
>>>> 4362
>>>> 4363 while (READ_ONCE(tp->cur_tx) != dirty_tx) {
>>>> 4364 unsigned int entry = dirty_tx % NUM_TX_DESC;
>>>> 4365 u32 status;
>>>> 4366
>>>> → 4367 status = le32_to_cpu(tp->TxDescArray[entry].opts1);
>>>> 4368 if (status & DescOwn)
>>>> 4369 break;
>>>> 4370
>>>> 4371 skb = tp->tx_skb[entry].skb;
>>>> 4372 rtl8169_unmap_tx_skb(tp, entry);
>>>> 4373
>>>> 4374 if (skb) {
>>>> 4375 pkts_compl++;
>>>> 4376 bytes_compl += skb->len;
>>>> 4377 napi_consume_skb(skb, budget);
>>>> 4378 }
>>>> 4379 dirty_tx++;
>>>> 4380 }
>>>> 4381
>>>> 4382 if (tp->dirty_tx != dirty_tx) {
>>>> 4383 dev_sw_netstats_tx_add(dev, pkts_compl, bytes_compl);
>>>> 4384 WRITE_ONCE(tp->dirty_tx, dirty_tx);
>>>> 4385
>>>> 4386 netif_subqueue_completed_wake(dev, 0, pkts_compl, bytes_compl,
>>>> 4387 rtl_tx_slots_avail(tp),
>>>> 4388 R8169_TX_START_THRS);
>>>> 4389 /*
>>>> 4390 * 8168 hack: TxPoll requests are lost when the Tx packets are
>>>> 4391 * too close. Let's kick an extra TxPoll request when a burst
>>>> 4392 * of start_xmit activity is detected (if it is not detected,
>>>> 4393 * it is slow enough). -- FR
>>>> 4394 * If skb is NULL then we come here again once a tx irq is
>>>> 4395 * triggered after the last fragment is marked transmitted.
>>>> 4396 */
>>>> 4397 if (READ_ONCE(tp->cur_tx) != dirty_tx && skb)
>>>> 4398 rtl8169_doorbell(tp);
>>>> 4399 }
>>>> 4400 }
>>>>
>>>> tp->TxDescArray[entry].opts1 is reported to have a data-race and READ_ONCE() fixes
>>>> this KCSAN warning.
>>>>
>>>> 4366
>>>> → 4367 status = le32_to_cpu(READ_ONCE(tp->TxDescArray[entry].opts1));
>>>> 4368 if (status & DescOwn)
>>>> 4369 break;
>>>> 4370
>>>>
>>>> Fixes: ^1da177e4c3f4 ("initial git repository build")
>>>
>>> Hi Mirsad,
>>>
>>> The fixes tag above seems wrong.
>>
>> Hi, Simon,
>>
>> It is taken directly from "git blame" as you can check for yourself.
>>
>> It is supposed to tag the commits prior to the introduction of git.
>>
>> If you have a better idea how to denote those, I will be happy to learn,
>> but I have no better clue than what "git blame" gives ...
>
> Interesting, thanks for the explanation.
> I do think it's more usual, in such cases, to use the following.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Objection noted, thanks.
As the Linux kernel development deserves our best, I have submitted the v4 of the patch
series.
Best regards,
Mirsad Todorovac
>> Best regards,
>> Mirsad Todorovac
>>>> Cc: Heiner Kallweit <hkallweit1@...il.com>
>>>> Cc: nic_swsd@...ltek.com
>>>> Cc: "David S. Miller" <davem@...emloft.net>
>>>> Cc: Eric Dumazet <edumazet@...gle.com>
>>>> Cc: Jakub Kicinski <kuba@...nel.org>
>>>> Cc: Paolo Abeni <pabeni@...hat.com>
>>>> Cc: Marco Elver <elver@...gle.com>
>>>> Cc: netdev@...r.kernel.org
>>>> Link: https://lore.kernel.org/lkml/dc7fc8fa-4ea4-e9a9-30a6-7c83e6b53188@alu.unizg.hr/
>>>> Signed-off-by: Mirsad Goran Todorovac <mirsad.todorovac@....unizg.hr>
>>>> Acked-by: Marco Elver <elver@...gle.com>
>>>
>>> ...
>>
Powered by blists - more mailing lists