Re: [PATCH v2] mm/userfaultfd: detect VMA replacement after copy retry in mfill_copy_folio_retry()

[David CARLIER]

Hi Mike,

▎ Is it possible at all that after all that dance vma pointer will
remain
  ▎ the same?

  Yes, VMA structs are slab-allocated so after munmap frees the old
VMA
  and mmap allocates a new one, SLUB can hand back the same address.
The
  pointer matches but it's a different VMA — which is exactly why the
  snapshot is needed.

  ▎ This isn't a bug, but struct vm_area_struct uses vm_flags, not
flags.
  ▎ Will this cause a compilation error?

  This is a false positive from Sashiko. vm_area_struct has a union at
  include/linux/mm_types.h:956-960:

  union {
      const vm_flags_t vm_flags;
      vma_flags_t flags;
  };
  Peter explicitly asked to use vma_flags_t / vma->flags since
vm_flags_t
  is being deprecated (see vma_flags_to_legacy()).

  ▎ If the original VMA was file-backed (s->inode is non-NULL), but is
  ▎ concurrently replaced by an anonymous VMA during the lock-dropped
  ▎ window, vma->vm_file will be NULL. Does accessing
  ▎ vma->vm_file->f_inode here cause a NULL pointer dereference?

  Good catch, this is a real bug. Will fix with a vm_file NULL guard.

  ▎ Filesystem eviction paths often acquire locks (like i_rwsem) that
  ▎ invert with the mmap lock. Can this cause an AB-BA deadlock? Should
  ▎ this take a reference to the struct file via get_file() and
release it
  ▎ with fput() instead, which defers destruction safely?

  Valid concern. I'll switch to get_file()/fput() which defers the
  destruction safely.

  ▎ Whatever we do verify the VMA this should not be EAGAIN. EINVAL or
  ▎ ENOENT like mfill_get_vma() returns seem more appropriate.

  Agreed, will change to -EINVAL to match mfill_get_vma()'s validation
  failures.

  Will send a v2 with all three fixes later on.

Cheers !