linux-kernel - Re: igb driver can cause cache invalidation of non-owned memory?

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d34b7175-1024-8cc9-d422-d9f75f56c069@cogentembedded.com>
Date:   Wed, 12 Oct 2016 21:12:39 +0300
From:   Nikita Yushchenko <nikita.yoush@...entembedded.com>
To:     Alexander Duyck <alexander.duyck@...il.com>
Cc:     David Laight <David.Laight@...lab.com>,
        Eric Dumazet <edumazet@...gle.com>,
        David Miller <davem@...emloft.net>,
        Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
        intel-wired-lan <intel-wired-lan@...ts.osuosl.org>,
        Netdev <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Chris Healy <cphealy@...il.com>
Subject: Re: igb driver can cause cache invalidation of non-owned memory?

> It would make more sense to update the DMA API for
> __dma_page_cpu_to_dev on ARM so that you don't invalidate the cache if
> the direction is DMA_FROM_DEVICE.

No, in generic case it's unsafe.

If CPU issued a write to a location, and sometime later that location is
used as DMA buffer, there is danger that write is still in cache only,
and writeback is pending. Later this writeback can overwrite data
written to memory via DMA, causing corruption.

> The point I was trying to make is that you are invalidating the cache
> in both the sync_for_device and sync_for_cpu.  Do you really need that
> for ARM or do you need to perform the invalidation on sync_for_device
> if that may be pushed out anyway?  If you aren't running with with
> speculative look-ups do you even need the invalidation in
> sync_for_cpu?

I'm not lowlevel arm guru and I don't know for sure. Probably another
CPU core can be accessing locations neighbor page, causing specilative
load of locations in DMA page.

> Changing the driver code for this won't necessarily work on all
> architectures, and on top of it we have some changes planned which
> will end up making the pages writable in the future to support the
> ongoing XDP effort.  That is one of the reasons why I would not be
> okay with changing the driver to make this work.

Well I was not really serious about removing that sync_for_device() in
mainline :)   Although >20% throughput win that this provides is
impressive...

But what about doing something safer, e.g. adding a bit of tracking and
only sync_for_device() what was previously sync_for_cpu()ed?  Will you
accept that?

Nikita