Re: [PATCH v4 1/4] mm/shmem: Unconditionally set pte dirty in mfill_atomic_install_pte

[Hugh Dickins]

On Mon, 27 Sep 2021, Peter Xu wrote:
> On Thu, Sep 23, 2021 at 08:56:33PM -0700, Hugh Dickins wrote:
> > I'm not going to NAK this, but you and I have different ideas of
> > "very nice cleanups".  Generally, you appear (understandably) to be
> > trying to offload pieces of work from your larger series, but often
> > I don't see the sense of them, here in isolation anyway.
> > 
> > Is this a safe transformation of the existing code? Yes, I believe so
> > (at least until someone adds some PTESAN checker which looks to see
> > if any ptes are dirty in vmas to which user never had write access).
> > But it took quite a lot of lawyering to arrive at that conclusion.
> 
> I can get your point there, but I keep a skeptical view if there'll be a tool
> called PTESAN that asserts VM_WRITE for pte_dirty.
> 
> After we've noticed the arm64 implementation of pte_mkdirty() last time, I've
> already started to not bind the ideas on VM_WRITE or pte_write() for pte dirty.

Yes, I know that there are good cases of pte_dirty() without pte_write():
that's why I said "never had write access" - never.

> 
> As I said before, that's quite natural when I think "the uffd-way", because
> uffd can easily arm a page with read-only but while the page is dirty.  I think
> you'll answer that with "we should mark the page dirty instead" in this case,
> as you stated below.  I also agree.  However if we see pte_dirty a major way to
> track data dirty information, and at last when it'll be converged into the
> PageDirty, I think it doesn't really matter a huge lot to us if we set pte or
> page dirty, or is it?

Please, imagine the faulting process has a PROT_READ,MAP_SHARED mmap of
/etc/passwd, or any of a million files you would not want it to write to.

The process serving that fault by doing the ioctl has (in my perhaps
mistaken mental model of userfaultfd) greater privilege, and is able
to fill in the contents of what /etc/passwd should contain: it fills the
right data into the page, which is preserved by being marked PageDirty.

The faulting process can never write to that page, and that pte ought
never to be marked dirty.

Marking the pte dirty is not a security problem: doing so does not
grant write access (though it's easy to imagine incorrect code elsewhere
that "deduces" pte_write() from pte_dirty() for some reason - I'm pretty
sure we have had such instances in the past, if not now).  But it is an
anomaly that would be better avoided.

Which is precisely why there is the "writable" check before doing it at
present ("writable" being a stand-in for the FAULT_FLAG_WRITE not visible
at this end).  (There is also "|| !page_in_cache" - that's to allow for a
read-only mmap of a page supplied by mcopy_atomic_pte(): which I argue
would be better with a SetPageDirty() in the caller than a pte_mkdirty()
here; but anonymous pages are less of a worry than shared file pages.)

So, as I said before, I believe what you're doing in this patch happens
to be a safe transformation of existing code; but not a nice cleanup.

> 
> > 
> > Is this a cleanup? No, it's a dirtyup.
> > 
> > shmem_mfill_atomic_pte() does SetPageDirty (before unlocking page)
> > because that's where the page contents are made dirty.  You could
> > criticise it for doing SetPageDirty even in the zeropage case:
> > yes, we've been lazy there; but that's a different argument.
> > 
> > If someone is faulting this page into a read-only vma, it's
> > surprising to make the pte dirty there.  What would be most correct
> > would be to keep the SetPageDirty in shmem_mfill_atomic_pte()
> > (with or without zeropage optimization), and probably SetPageDirty
> > in some other places in mm/userfaultfd.c (I didn't look where) when
> > the page is filled with supplied data, and mfill_atomic_install_pte()
> > only do that pte_mkdirty() when it's serving a FAULT_FLAG_WRITE.
> 
> That's a good point, and yeah if we can unconditionally mark PageDirty it'll be
> great too; I think what bothered me most in the past was that the condition to
> check dirty is too complicated, for which myself has been debugging for two
> cases where we should apply the dirty bit but we forgot; each of the debugging
> process took me a few days or more to figure out, thanks to my awkward
> debugging skills.
> 
> Then I noticed, why not we do the way around if for 99% of the cases they're
> dirty in real systems?  Say, let's set dirty unconditionally and see when there
> (could have, which I still doubt) is a negative effect on having some page
> dirty, we track that from a "degraded" performance results.  Then we convert
> some hard-to-debug data corrupt issues into "oh previously this programs runs
> at speed 100x, now it runs 99x, why I got 1% performance lost?"  I even highly
> doubt whether it'll come true: for the uffd case (which is the only case I
> modified in this patch), I can hardly tell how many people would like to use
> the mappings read-only, and how much they'll suffer from that extra dirty bit
> or PageDirty.
> 
> That's why I really like this patch to happen, I want to save time for myself,
> and for anyone who will be fighting for another dirty lost issues.

I've lost time on missed dirties too, so I ought to be more sympathetic
to your argument than I am: I'm afraid I read it as saying that you don't
really understand "dirty", so want to do it more often to be "safe".
Not a persuasive argument.

> 
> > 
> > I haven't looked again (I have a pile of mails to respond to!),
> > but when I looked before I think I found that the vmf flags are
> > not available to the userfaultfd ioctler.  If so, then it would
> > be more appropriate to just leave the mkdirty to the hardware on
> > return from fault (except - and again I cannot spend time researching
> > this - perhaps I'm too x86-centric, and there are other architectures
> > on which the software *must* do the mkdirty fixup to avoid refaulting
> > forever - though probably userfaultfd state would itself prevent that).
> 
> If it's based on the fact that we'll set PageDirty for file-backed, then it
> looks okay, but not usre.
> 
> One thing to mention is pte_mkdirty() also counts in soft dirty by nature.  I'm
> imagining a program that was soft-dirty tracked and somehow using UFFDIO_COPY
> as the major data filler (so the task itself may not write to the page directly
> hence HW won't set dirty bit there).  If with pte_mkdirty the other userspace
> tracker with soft-dirty can still detect this, while with PageDirty I believe
> it can't.  From that POV I'm not sure whether I can say that as proactively
> doing pte_mkdirty is a safer approach just in case such an use case exist, as
> myself can't say they're illegal, so pte_dirty is a superset of PageDirty not
> vice versa.

And this is not persuasive either: without much deeper analysis
(which I'll decline to do!), it's impossible to tell whether an excess of
pte_mkdirty()s is good or bad for the hypothetical uffd+softdirty tracker:
you're guessing good, I'm guessing bad.

How about a compromise (if you really want to continue with this patch):
you leave the SetPageDirty(page) in shmem_mfill_atomic_pte(), where I
feel a responsibility for it; but you do whatever works for you with
pte_mkdirty() at the mm/userfaultfd.c end?

(In the course of writing this, it has occurred to me that a much nicer
solution might be to delete mfill_atomic_install_pte() altogether, and
change the userfaultfd protocol so that handle_userfault() returns the
page supplied by ioctler, for process to map into its own userspace
in its usual way. But that's a big change, that neither of would be
keen to make; and it would not be surprising if it turned out actually
to be a very bad change - perhaps tried and abandoned before the "atomic"
functions were decided on. I wouldn't even dare mention it, unless that
direction might happen to fit in with something else you're plannng.)

Hugh