Linux-Fsdevel Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jann Horn <jannh@google.com>
To: Hugh Dickins <hughd@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Christoph Hellwig <hch@lst.de>,
	kernel list <linux-kernel@vger.kernel.org>,
	Linux-MM <linux-mm@kvack.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	"Eric W . Biederman" <ebiederm@xmission.com>,
	Oleg Nesterov <oleg@redhat.com>
Subject: Re: [PATCH v5 7/7] mm: Remove the now-unnecessary mmget_still_valid() hack
Date: Mon, 31 Aug 2020 11:58:47 +0200	[thread overview]
Message-ID: <CAG48ez15Zxbkjv0WRChZZZ6F78pFVXPTu_Bn1Pqaxx=7Gk1gUg@mail.gmail.com> (raw)
In-Reply-To: <alpine.LSU.2.11.2008302225510.1934@eggly.anvils>

On Mon, Aug 31, 2020 at 8:07 AM Hugh Dickins <hughd@google.com> wrote:
> On Thu, 27 Aug 2020, Jann Horn wrote:
>
> > The preceding patches have ensured that core dumping properly takes the
> > mmap_lock. Thanks to that, we can now remove mmget_still_valid() and all
> > its users.
>
> Hi Jann, while the only tears to be shed over losing mmget_still_valid()
> will be tears of joy, I think you need to explain why you believe it's
> safe to remove the instance in mm/khugepaged.c: which you'll have found
> I moved just recently, to cover an extra case (sorry for not Cc'ing you).
>
> > --- a/mm/khugepaged.c
> > +++ b/mm/khugepaged.c
> > @@ -431,7 +431,7 @@ static void insert_to_mm_slots_hash(struct mm_struct *mm,
> >
> >  static inline int khugepaged_test_exit(struct mm_struct *mm)
> >  {
> > -     return atomic_read(&mm->mm_users) == 0 || !mmget_still_valid(mm);
> > +     return atomic_read(&mm->mm_users) == 0;
> >  }
> >
> >  static bool hugepage_vma_check(struct vm_area_struct *vma,
>
> The movement (which you have correctly followed) was in
> bbe98f9cadff ("khugepaged: khugepaged_test_exit() check mmget_still_valid()")
> but the "pmd .. physical page 0" issue is explained better in its parent
> 18e77600f7a1 ("khugepaged: retract_page_tables() remember to test exit")
>
> I think your core dumping is still reading the page tables without
> holding mmap_lock

Where? get_dump_page() takes mmap_lock now:
<https://lore.kernel.org/lkml/20200827114932.3572699-7-jannh@google.com/>

I don't think there should be any paths into __get_user_pages() left
that don't hold the mmap_lock. Actually, we should probably try
sticking mmap_assert_locked() in there now as a follow-up?

> so still vulnerable to that extra issue.  It won't
> be as satisfying as removing all traces of mmget_still_valid(), but
> right now I think you should add an mm->core_state check there instead.
>
> (I do have a better solution in use, but it's a much larger patch, that
> will take a lot more effort to get in: checks in pte_offset_map_lock(),
> perhaps returning NULL when pmd is transitioning, requiring retry.)

Just to clarify: This is an issue only between GUP's software page
table walks when running without mmap_lock and concurrent page table
modifications from hugepage code, correct? Hardware page table walks
and get_user_pages_fast() are fine because they properly load PTEs
atomically and are written to assume that the page tables can change
arbitrarily under them, and the only guarantee is that disabling
interrupts ensures that pages referenced by PTEs can't be freed,
right?

> Or maybe it's me who has missed what you're doing instead.
>
> Hugh

  reply	other threads:[~2020-08-31  9:59 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-27 11:49 [PATCH v5 0/7] Fix ELF / FDPIC ELF core dumping, and use mmap_lock properly in there Jann Horn
2020-08-27 11:49 ` [PATCH v5 1/7] binfmt_elf_fdpic: Stop using dump_emit() on user pointers on !MMU Jann Horn
2020-08-27 11:49 ` [PATCH v5 2/7] coredump: Let dump_emit() bail out on short writes Jann Horn
2020-08-27 11:49 ` [PATCH v5 3/7] coredump: Refactor page range dumping into common helper Jann Horn
2020-08-27 11:49 ` [PATCH v5 4/7] coredump: Rework elf/elf_fdpic vma_dump_size() " Jann Horn
2020-08-27 11:49 ` [PATCH v5 5/7] binfmt_elf, binfmt_elf_fdpic: Use a VMA list snapshot Jann Horn
2020-08-27 11:49 ` [PATCH v5 6/7] mm/gup: Take mmap_lock in get_dump_page() Jann Horn
2020-08-27 17:13   ` Linus Torvalds
2020-08-27 11:49 ` [PATCH v5 7/7] mm: Remove the now-unnecessary mmget_still_valid() hack Jann Horn
2020-08-31  6:06   ` Hugh Dickins
2020-08-31  9:58     ` Jann Horn [this message]
2020-08-31 20:36       ` Hugh Dickins
2020-08-31 21:30       ` Hugh Dickins
2020-08-27 17:15 ` [PATCH v5 0/7] Fix ELF / FDPIC ELF core dumping, and use mmap_lock properly in there Linus Torvalds

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAG48ez15Zxbkjv0WRChZZZ6F78pFVXPTu_Bn1Pqaxx=7Gk1gUg@mail.gmail.com' \
    --to=jannh@google.com \
    --cc=akpm@linux-foundation.org \
    --cc=ebiederm@xmission.com \
    --cc=hch@lst.de \
    --cc=hughd@google.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=oleg@redhat.com \
    --cc=torvalds@linux-foundation.org \
    --cc=viro@zeniv.linux.org.uk \
    --subject='Re: [PATCH v5 7/7] mm: Remove the now-unnecessary mmget_still_valid() hack' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).