mm: userfaultfd: add new UFFDIO_POISON ioctl: fix

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 <[email protected]>

---
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.

 mm/userfaultfd.c | 4 ++++
 1 file changed, 4 insertions(+)

mm/userfaultfd.c +4 -0

@@ -300,7 +300,10 @@ static int mfill_atomic_pte_poison(pmd_t *dst_pmd,
        spinlock_t *ptl;
 
        _dst_pte = make_pte_marker(PTE_MARKER_POISONED);
+       ret = -EAGAIN;
        dst_pte = pte_offset_map_lock(dst_mm, dst_pmd, dst_addr, &ptl);
+       if (!dst_pte)
+               goto out;
 
        if (mfill_file_over_size(dst_vma, dst_addr)) {
                ret = -EFAULT;

@@ -319,6 +322,7 @@ static int mfill_atomic_pte_poison(pmd_t *dst_pmd,
        ret = 0;
 out_unlock:
        pte_unmap_unlock(dst_pte, ptl);
+out:
        return ret;
 }
 

Quoted 5 lines · Hugh Dickins

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 <[email protected]>

Looks correct to me, thanks Hugh! If it's useful, feel free to take:

Reviewed-by Axel Rasmussen <[email protected]>

Quoted 8 lines

---
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.

Quoted 5 lines · Axel Rasmussen

Quoted 4 lines · Hugh Dickins

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.

Quoted 5 lines

Quoted 1 line

Signed-off-by Hugh Dickins <[email protected]>

Looks correct to me, thanks Hugh! If it's useful, feel free to take:

Reviewed-by Axel Rasmussen <[email protected]>

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

Quoted 17 lines

Quoted 8 lines

---
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.

Quoted 10 lines

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).