Re: [PATCH mm-unstable fix] mm: userfaultfd: add new UFFDIO_POISON ioctl: fix

[Hugh Dickins <hughd@google.com>]

[-- Attachment #1: Type: text/plain, Size: 2635 bytes --]

On Wed, 12 Jul 2023, Axel Rasmussen wrote:
> On Tue, Jul 11, 2023 at 6:27 PM Hugh Dickins <hughd@google.com> wrote:
> >
> > Smatch has observed that pte_offset_map_lock() is now allowed to fail,
> > and then ptl should not be unlocked.  Use -EAGAIN here like elsewhere.
> >
> > Signed-off-by: Hugh Dickins <hughd@google.com>
> 
> Looks correct to me, thanks Hugh! If it's useful, feel free to take:
> 
> Reviewed-by: Axel Rasmussen <axelrasmussen@google.com>

Thanks, that will have helped Andrew (but would vanish into the merge
with your original when it moves from mm-unstable to mm-stable).

> 
> > ---
> > Axel, Peter: this seems right as a fix to the patch in mm-unstable;
> > but in preparing this, I noticed mfill_atomic()'s code before calling
> > mfill_atomic_pte(), and think that my original choice of -EFAULT was
> > therefore better than -EAGAIN for all of these; and that mfill_atomic()'s
> > BUG_ONs there would be better deleted (and is its BUG_ON(folio) safe??).
> > Something one of us should address, after this fixup is in akpm's tree.
> 
> Agreed, dealing with the BUG_ONs is something I have been meaning to
> do as checkpatch bothers me about it frequently.
> 
> What this code is trying to do is to enforce the contract that: if
> mfill_atomic_pte failed, then it must have released *folio and set it
> to NULL. But, you're exactly right that if we had such a bug, it would
> mean we leaked one page in an unusual error case - not great, but not
> worth crashing the kernel either.
> 
> For UFFDIO_POISON this doesn't really apply because it doesn't
> allocate or get a reference to a page in any case.
> 
> For other cases like UFFDIO_COPY where it does matter, I think it's
> fine as is regardless of the error code returned (as long as it's not
> -ENOENT :) ). mfill_atomic_pte_copy() is sure to set `*foliop = NULL`
> before it attempts this lock (in mfill_atomic_install_pte). So -EAGAIN
> or -EFAULT should work equally well at least from that perspective. I
> somewhat prefer -EAGAIN, but maybe I'm missing something.

Right, when I said -EFAULT better than -EAGAIN, I was only basing that
on -EFAULT being the code which was already used for pmd_trans_huge()
in the check above mfill_atomic()'s call to mfill_atomic_pte().

I don't disagree with you, and Peter too preferred -EAGAIN: it's just
better if they all use the same code (or maybe mfill_atomic()'s first
check for pmd_trans_huge() can be deleted along with the BUG_ONs - but
I have not checked through the cases, there may be very good reason to
keep that preliminary check there).

Hugh