netdev - RE: mmiowb() vs wmb() with write combined MMIO

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]

Date:   Tue, 13 Mar 2018 13:51:45 +0000
From:   "Elior, Ariel" <Ariel.Elior@...ium.com>
To:     "Chopra, Manish" <Manish.Chopra@...ium.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     "john.fastabend@...il.com" <john.fastabend@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        Eric Dumazet <eric.dumazet@...il.com>,
        "Kalderon, Michal" <Michal.Kalderon@...ium.com>,
        "yuvalm@...lanox.com" <yuvalm@...lanox.com>,
        Michal Schmidt <mschmidt@...hat.com>
Subject: RE: mmiowb() vs wmb() with write combined MMIO

> has uncovered an issue in qede NIC driver [/drivers/net/ethernet/qlogic/qede] and
> this driver is seriously broken for basic L2.
That's a bit extreme. There is a race condition bug that needs fixing.

> Before the series from John, driver was running fine, probably with hiding a serious
> bug underneath it which got exposed with his series.
> 
> Driver uses mmiowb() after writing TX doorbell in function
> qede_update_tx_producer() to synchronize the write between CPUs and
> that TX doorbell area, driver maps with write combined memory using ioremap_wc().
Yup, that's the problem. Write combined bar is relaxed-ordering bar, so you must use
wmb() after the store to flush the wc_buffer. If driver gets scheduled on another cpu
before the buffer is flushed and writes to the same address, we can be in trouble.

> This is causing an issue where HW seems to be reading an unexpected producer value
> [not as was written on doorbell, probably an older producer]
> and causing various SKB null condition hit while handling TX completion. This seems to
> be indicating that mmiowb() doesn't seems to be synchronizing/flushing
> writes here, at least on x86_64 SMP,  it's just like a compiler barrier [Not really a
> hardware fence].
> 
> If we replace mmiowb() with wmb() we never hit the problem. wmb() seems to be
> adding some synchronization/flush mechanism here.
> Moreover, if we map doorbell with ioremap_nocache() we never hit problem with
> mmiowb() being there even.
Makes sense.

> Any suggestions or help about this barrier differences will be appreciated here in
> order to prepare a proper fix.
Please prepare the fix with wmb() after the store to flush the write combined buffer

> 
> Thanks in Advance !!
> Manish Chopra