Re: [PATCH v2 3/5] mm: madvise: implement lightweight guard page mechanism

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Tue, Oct 22, 2024 at 09:08:53PM +0200, Jann Horn wrote:
> On Mon, Oct 21, 2024 at 10:46 PM Vlastimil Babka <vbabka@suse.cz> wrote:
> > On 10/21/24 22:27, Lorenzo Stoakes wrote:
> > > On Mon, Oct 21, 2024 at 10:11:29PM +0200, Vlastimil Babka wrote:
> > >> On 10/20/24 18:20, Lorenzo Stoakes wrote:
> > >> > +  while (true) {
> > >> > +          /* Returns < 0 on error, == 0 if success, > 0 if zap needed. */
> > >> > +          err = walk_page_range_mm(vma->vm_mm, start, end,
> > >> > +                                   &guard_poison_walk_ops, NULL);
> > >> > +          if (err <= 0)
> > >> > +                  return err;
> > >> > +
> > >> > +          /*
> > >> > +           * OK some of the range have non-guard pages mapped, zap
> > >> > +           * them. This leaves existing guard pages in place.
> > >> > +           */
> > >> > +          zap_page_range_single(vma, start, end - start, NULL);
> > >>
> > >> ... however the potentially endless loop doesn't seem great. Could a
> > >> malicious program keep refaulting the range (ignoring any segfaults if it
> > >> loses a race) with one thread while failing to make progress here with
> > >> another thread? Is that ok because it would only punish itself?
> > >
> > > Sigh. Again, I don't think you've read the previous series have you? Or
> > > even the changelog... I added this as Jann asked for it. Originally we'd
> > > -EAGAIN if we got raced. See the discussion over in v1 for details.
> > >
> > > I did it that way specifically to avoid such things, but Jann didn't appear
> > > to think it was a problem.
> >
> > If Jann is fine with this then it must be secure enough.
>
> My thinking there was:
>
> We can legitimately race with adjacent faults populating the area
> we're operating on with THP pages; as long as the zapping and
> poison-marker-setting are separate, *someone* will have to do the
> retry. Either we do it in the kernel, or we tell userspace to handle
> it, but having the kernel take care of it is preferable because it
> makes the stable UAPI less messy.
>
> One easy way to do it in the kernel would be to return -ERESTARTNOINTR
> after the zap_page_range_single() instead of jumping back up, which in
> terms of locking and signal handling and such would be equivalent to
> looping in userspace (because really that's what -ERESTARTNOINTR does
> - it returns out to userspace and moves the instruction pointer back
> to restart the syscall). Though if we do that immediately, it might
> make MADV_POISON unnecessarily slow, so we should probably retry once
> before doing that. The other easy way is to just loop here.

Yes we should definitely retry probably a few times to cover the rare
situation of a THP race as you describe under non-abusive circumstances.

>
> The cond_resched() and pending fatal signal check mean that (except on
> CONFIG_PREEMPT_NONE) the only differences between the current
> implementation and looping in userspace are that we don't handle
> non-fatal signals in between iterations and that we keep hogging the
> mmap_lock in read mode. We do already have a bunch of codepaths that
> retry on concurrent page table changes, like when zap_pte_range()
> encounters a pte_offset_map_lock() failure; though I guess the
> difference is that the retry on those is just a couple instructions,
> which would be harder to race consistently, while here we redo walks
> across the entire range, which should be fairly easy to race
> repeatedly.
>
> So I guess you have a point that this might be the easiest way to
> stall other tasks that are trying to take mmap_lock for an extended
> amount of time, I did not fully consider that... and then I guess you
> could use that to slow down usercopy fault handling (once the lock
> switches to handoff mode because of a stalled writer?) or slow down
> other processes trying to read /proc/$pid/cmdline?

Hm does that need a write lock?

>
> You can already indefinitely hog the mmap_lock with FUSE, though that
> requires that you can mount a FUSE filesystem (which you wouldn't be
> able in reasonably sandboxed code) and that you can find something
> like a pin_user_pages() call that can't drop the mmap lock in between,
> and there aren't actually that many of those...
>
> So I guess you have a point and the -ERESTARTNOINTR approach would be
> a little bit nicer, as long as it's easy to implement.

I can go ahead and do it that way if nobody objects, with a few loops
before we do it... which hopefully covers off all the concerns?

Thanks