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
