Re: [PATCH] dma-debug: fix debug_dma_assert_idle(), use rcu_read_lock()

[Linus Torvalds <torvalds@linux-foundation.org>]

On Fri, Aug 14, 2020 at 5:26 PM Hugh Dickins <hughd@google.com> wrote:
>
> We used to rely on page count there, and on trylock_page() only; but
> there was at least one user whose app went wrong when occasionally we
> COWed the page, just because something else momentarily took a reference
> to it, or locked it.  Around 2006, bug report from 2004: I did look up
> the history a week ago, but was interrupted before taking notes.

I actually think you may be talking about the exact problem that that
debug patch from Dan was originally created for:

  0abdd7a81b7e dma-debug: introduce debug_dma_assert_idle()
  77873803363c net_dma: mark broken

and your memory sounds exactly like that net_dma case (and the timing
matches roughly too - the NET_DMA code was merged in 2006, but I think
people had been playing trial games with it before that).

IOW, net_dma was horribly broken, and just couldn't deal with COW
because it did things wrong.

The thing is, doing extra COW's really shouldn't matter in _any_
half-way correct situation. There's a few cases:

 - user space writing to it, so we COW.

   This is the "simple" case that is obvious and we've always done the
same thing. User space will get the new copy, and there's no possible
situation when that can be wrong.

 - get_user_pages() for reading.

   This is the one we actually used to get wrong, and when another
user *didn't* cow, the data that was read might not match what the
original get_uiser_pages() case expected.

    But in this case, the bug only happened when we didn't cow
aggressively enough.

 - get_user_pages() for writing

   This is another 'simple" case, because it does the COW at
get_user_pages() time and gets it's own copy (which is also installed
in the thread that does the GUP, of course, so a subsequent fork an
danother write can obviously cause *further* COW action).

But in no case should an extra COW matter. Except if somebody uses
get_user_pages() to write to the page, and the COW "hides" that write
by giving a new copy to whoever expected to see it, but that's exactly
the case that Dan's patch was supposed to notice.

And since it never triggered outside of that invalid net_dma case, I
don't think any other case really ever existed.

Yes, I can well imagine that some people loved the concept of that TCP
receive copy offload, but it really was broken, and was removed
entirely by Dan in commit 7bced397510a ("net_dma: simple removal") a
year after being marked broken (the author date makes it look like
it's just a couple of weeks after being marked broken, but the commit
date for that removal is September 2014).

So I don't think that the trylock and checking page counts is a
correctness issue.

It had better not be, because anybody that writes to a shared-cow page
 without breaking COW is simply broken.

No, I really think that the real worry about doing more aggressive
copying is that it doesn't steal back the KSM page or the swap cache
page, so it will leave those pages around, and while they should then
be really easy for the VM to reclaim, I really worry that we have a
couple of decades of VM reclaim tuning with that swap cache reuse
behavior (KSM, not so much).

And while it works fine on my machine, I currently have 40GB or RAM
free, because honestly, the stuff I do doesn't need all that much
memory, and I ridiculously overspecced my new machine RAM'wise. So
nothing I will do would show any problems.

                Linus