netdev - Re: [REGRESSION] Recent swiotlb DMA_FROM_DEVICE fixes break ath9k-based AP

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: <CAHk-=wh+TzZhMCiPPnchC4FSeS53-QCN3RYqxLAh4ahKMLoj9A@mail.gmail.com>
Date:   Sun, 27 Mar 2022 13:04:53 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     David Laight <David.Laight@...lab.com>
Cc:     Halil Pasic <pasic@...ux.ibm.com>,
        Toke Høiland-Jørgensen <toke@...e.dk>,
        Robin Murphy <robin.murphy@....com>,
        Christoph Hellwig <hch@....de>,
        Maxime Bizon <mbizon@...ebox.fr>,
        Oleksandr Natalenko <oleksandr@...alenko.name>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Kalle Valo <kvalo@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Olha Cherevyk <olha.cherevyk@...il.com>,
        iommu <iommu@...ts.linux-foundation.org>,
        linux-wireless <linux-wireless@...r.kernel.org>,
        Netdev <netdev@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        stable <stable@...r.kernel.org>
Subject: Re: [REGRESSION] Recent swiotlb DMA_FROM_DEVICE fixes break
 ath9k-based AP

On Sun, Mar 27, 2022 at 12:23 PM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> So I will propose that we really make it very much about practical
> concerns, and we document things as
>
>  (a) the "sync" operation has by definition a "whose _future_ access
> do we sync for" notion.
>
>      So "dma_sync_single_for_cpu()" says that the future accesses to
> this dma area is for the CPU.
>
>      Note how it does *NOT* say that the "CPU owns the are". That's
> bullsh*t, and we now know it's BS.
>
>  (b) but the sync operation also has a "who wrote the data we're syncing"
>
>      Note that this is *not* "who accessed or owned it last", because
> that's nonsensical: if we're syncing for the CPU, then the only reason
> to do so is because we expect that the last access was by the device,
> so specifying that separately would just be redundant and stupid.
>
>      But specifying who *wrote* to the area is meaningful and matters.

We could also simply require that the bounce buffer code *remember*
who wrote to it last.

So when the ath9k driver does

 - setup:

                bf->bf_buf_addr = dma_map_single(sc->dev, skb->data,
                                                 common->rx_bufsize,
                                                 DMA_FROM_DEVICE);

we clear the bounce buffer and remember that the state of the bounce
buffer is "device wrote to it" (because DMA_FROM_DEVICE).

Then, we have an interrupt or other event, and ath9k does

 - rc event:

        dma_sync_single_for_cpu(sc->dev, bf->bf_buf_addr,
                                common->rx_bufsize, DMA_FROM_DEVICE);

        ret = ath9k_hw_process_rxdesc_edma(ah, rs, skb->data);
        if (ret == -EINPROGRESS) {
                /*let device gain the buffer again*/
                dma_sync_single_for_device(sc->dev, bf->bf_buf_addr,
                                common->rx_bufsize, DMA_FROM_DEVICE);
                return false;
        }

and the first dma_sync_single_for_cpu() now sees "Ok, I want the CPU
buffer, and I remember that the device wrote to it, so I will copy
from the bounce buffer". It's still DMA_FROM_DEVICE, so that "the
device wrote to it" doesn't change.

When the CPU then decides "ok, that wasn't it", and does that
dma_sync_single_for_device(), the bounce buffer code goes "Ok, the
last operation was that the device wrote to the buffer, so the bounce
buffer is still valid and I should do nothing".

Voila, no ath9k breakage, and it all still makes perfect sense.

And that sounds like an even more logical model than the one where we
tell the bounce buffer code what the previous operation was, but it
involves basically the DMA mapping code remembering what the last
direction was. That makes perfect sense to me, but it's certainly not
what the DMA mapping code has traditionally done, which makes me
nervous that it would just expose a _lot_ of other drivers that do odd
things.

The "good news" (if there is such a thing) is that usually the
direction doesn't actually change. So if you use DMA_FROM_DEVICE
initially, you'll continue to use that. So there is probably basically
never any difference between "what was the previous operation" and
"what is the next operation".

So maybe practically speaking, we don't care.

Anyway, I do think we have choices here on how to describe things.

I do think that the "DMA code doesn't have to remember" model has the
advantage that remembering is always an added complexity, and
operations that behave differently depending on previous history are
always a bit harder to think about because of that. Which is why I
think that model I outlined in the previous email is probably the most
straightforward one.

                 Linus