Re: kernel BUG at include/linux/swapops.h:204!

From: Peter Xu <peterx@redhat.com>
To: Hugh Dickins <hughd@google.com>
Cc: Igor Raits <igor@gooddata.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Hillf Danton <hdanton@sina.com>,
	Axel Rasmussen <axelrasmussen@google.com>,
	linux-mm@kvack.org
Subject: Re: kernel BUG at include/linux/swapops.h:204!
Date: Mon, 19 Jul 2021 20:34:37 -0400	[thread overview]
Message-ID: <YPYaHR9KGIlHwo9R@t490s> (raw)
In-Reply-To: <757b684a-67b5-999b-7f2d-b55fb1c61fd8@google.com>

On Mon, Jul 19, 2021 at 03:42:41PM -0700, Hugh Dickins wrote:
> On Mon, 19 Jul 2021, Peter Xu wrote:
> > On Mon, Jul 19, 2021 at 12:11:21PM -0700, Hugh Dickins wrote:
> > > 
> > > But I'm confident that 8f34f1eac382 will prove to be the fix, so Peter
> > > please prepare some backports of that for the various stable/longterm
> > > kernels that need it - I've not looked into whether it applies cleanly,
> > > or depends on other commits too.  You fixed several related but different
> > > things in that commit: but this one is the worst, because it can corrupt
> > > even those who are not using UFFD_WP at all.
> > 
> > Looks right to me, b569a1760782 ("userfaultfd: wp: drop _PAGE_UFFD_WP properly
> > when fork", 2020-04-07) seems to be the culprit.  I didn't notice the side
> > effect in the bug or in the fix, or it should have already land stables. I am
> > very sorry for such a preliminary bug that caused this fallout - I really can't
> > tell why I completely didn't look at is_swap_pte() that's so obvious indeed.
> > 
> > I checked it up, 5.6.y doesn't have the issue commit yet as it's not marked as
> > "fixes". It started to show up in 5.7.y~5.13.y. 5.14-rc1 has 8f34f1eac382 which
> > is the fix.  So I think we need the fix or equivalent fix for 5.7.y~5.13.y.
> > 
> > 5.12.y & 5.13.y can pick up the fix 8f34f1eac382 cleanly.  For the olders
> > (5.7.y~5.11.y) they can't.  I plan to revert b569a1760782 instead.
> > 
> ...
> > 
> > Please let me know if there's any comment on the backport plan above, or I'll
> > prepare the patches for all the branches before tomorrow.
> 
> Thanks for getting on to it so quickly, Peter.
> 
> The only non-EOL stable/longterm releases are then 5.13, 5.12 and 5.10.

I see, thanks.  I haven't explicitly backported patches to stable; it's a good
chance to learn about the bits, though.

> 
> I have no appreciation of the importance of UFFD_EVENT_FORK support
> for uffd-wp.  And no appreciation of the importance of the other bugs
> you fixed in 8f34f1eac382, and other uffd-wp fixes you may have made
> recently, some backported, some not.
> 
> But I think it is worth giving 5.10, the longterm, a little more
> consideration: don't be driven by whether 8f34f1eac382 applies cleanly
> (all 5.13 and 5.12 would require then is a mail to GregKH Cc stable
> asking him to add that commit), but by how important the support is
> to users of 5.10, and how far away from working safely it is - maybe
> a 5.10-specific patch would be worthwhile, maybe not, I cannot judge.

I am not aware of anyone who's using fork with uffd-wp.  CRIU is the major user
per my knowledge that uses uffd fork, but still it shouldn't be using uffd-wp.
I know other users that use uffd-wp, but never with fork event.

Per my understanding, if above is true then it's probably not a good candidate
for such patches that fixing uffd-wp + fork to be backported to stable, as in
stable tree rules there's one entry:

  - It must fix a real bug that bothers people (not a, “This could be a
    problem…” type thing).

https://www.kernel.org/doc/html/v5.13/process/stable-kernel-rules.html

That's also one reason I didn't add Fixes for some patches because I am not
sure whether that'll help anyone, and the worst case is if someone hit some
issue we can backport explicitly.

There're a few other things that made me a bit worried before backporting the
full patch, even for 5.12/5.13, as there're requirements on the patch:

  - It cannot be bigger than 100 lines, with context.
  - It must fix only one thing.

The full patch (after squashed) contains ~200 LOC with context, and it does fix
a few more things... Do you know whether that would be a problem?

I'm not sure how strict would stable branches be, but if that's one blocker
then we'll only be able to either pick up the real fix for copy_pmd_range() or
just revert the issue commit which is just a few more lines; that should also
keep the tree cleaner.  From that sense, maybe it's easier to just revert for
all the branches (5.10/5.12/5.13).  Then if someone broke with uffd-wp+fork, I
can backport the full patch with better reasoning.

Thoughts?

Thanks,

-- 
Peter Xu