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
