Messages in this thread | | | Date | Fri, 16 Dec 2022 16:57:33 +0100 | From | David Hildenbrand <> | Subject | Re: [PATCH 1/2] mm/uffd: Fix pte marker when fork() without fork event |
| |
>> >> Wouldn't it be cleaner to be able to "clean" specific markers from a PTE >> marker without having to special case on each and everyone? I mean, only >> uffd-wp is really special such that it might disappear for the target. > > Quotting the commit message in patch 2: > > Currently there is a priority difference between the uffd-wp bit and the > swapin error entry, in which the swapin error always has higher priority > (e.g. we don't need to wr-protect a swapin error pte marker). > > If there will be a 3rd bit introduced, we'll probably need to consider a > more involved approach so we may need to start operate on the bits. > Let's leave that for later. > > I actually started the fix with something like that, but I noticed it's not > needed to add more code if there's no 3rd bit introduced so I dropped that. > I decided to go the simpler change approach and leave that for later.
Okay, makes sense.
> >> >> Something like (pseudocode): >> >> if (!userfaultfd_wp(dst_vma)) >> pte_marker_clear_uff_wp(entry); >> if (!pte_marker_empty(entry)) { >> pte = make_pte_marker(pte_marker_get(entry)); >> set_pte_at(dst_mm, addr, dst_pte, pte); >> } >> >> Then this fix would be correct and backport-able even without #2. And it >> would work for new types of markers :) > > When that comes, we may need one set_pte_marker_at() taking care of empty > pte markers, otherwise there can be a lot of such check.
Right. In the future it might be cleaner.
> >> >> >> I'd prefer a fix that doesn't break something else temporarily, even if the >> stable backport might require 5 additional minutes to do. So squashing #2 >> into #1 would also work. > > The thing is whether do we care about someone: (1) explicitly checkout at > the commit of patch 1, then (2) runs the kernel, hit a swapnin error, (3) > fork(), and (4) access the swapin error page in the child.
I'm more concerned about backports, when one backports #1 but not #2. In theory, patch #2 fixes patch #1, because that introduced IMHO a real regression -- a possible memory corruption when discarding a hwpoison marker. Warnings are not nice but at least indicate that something needs a second look.
> > To me I don't care even starting from (1).. because it really shouldn't > happen at all in any serious environment. > > The other reason is these are indeed two issues to solve. Even if by > accident we kept the swapin error in old code we'll probably dump an > warning which is not wanted either. It's not something someone will really > get benefit from.. > > So like many other places, I don't have a strong opinion, but personally I > prefer the current approach.
Me neither, two patches just felt more complicated than it should be.
Anyhow, the final code change LGTM.
-- Thanks,
David / dhildenb
| |