Re: [PATCH v16 1/5] mm: add VM_DROPPABLE for designating always lazily freeable mappings

[Jason A. Donenfeld]

On Fri, May 31, 2024 at 03:00:26PM +0200, Jann Horn wrote:
> On Fri, May 31, 2024 at 2:13 PM Jason A. Donenfeld <Jason@xxxxxxxxx> wrote:
> > On Fri, May 31, 2024 at 12:48:58PM +0200, Jann Horn wrote:
> > > On Tue, May 28, 2024 at 2:24 PM Jason A. Donenfeld <Jason@xxxxxxxxx> wrote:
> > > > c) If there's not enough memory to service a page fault, it's not fatal.
> > > [...]
> > > > @@ -5689,6 +5689,10 @@ vm_fault_t handle_mm_fault(struct vm_area_struct *vma, unsigned long address,
> > > >
> > > >         lru_gen_exit_fault();
> > > >
> > > > +       /* If the mapping is droppable, then errors due to OOM aren't fatal. */
> > > > +       if (vma->vm_flags & VM_DROPPABLE)
> > > > +               ret &= ~VM_FAULT_OOM;
> > >
> > > Can you remind me how this is supposed to work? If we get an OOM
> > > error, and the error is not fatal, does that mean we'll just keep
> > > hitting the same fault handler over and over again (until we happen to
> > > have memory available again I guess)?
> >
> > Right, it'll just keep retrying. I agree this isn't great, which is why
> > in the 2023 patchset, I had additional code to simply skip the faulting
> > instruction, and then the userspace code would notice the inconsistency
> > and fallback to the syscall. This worked pretty well. But it meant
> > decoding the instruction and in general skipping instructions is weird,
> > and that made this patchset very very contentious. Since the skipping
> > behavior isn't actually required by the /security goals/ of this, I
> > figured I'd just drop that. And maybe we can all revisit it together
> > sometime down the line. But for now I'm hoping for something a little
> > easier to swallow.
> 
> In that case, since we need to be able to populate this memory to make
> forward progress, would it make sense to remove the parts of the patch
> that treat the allocation as if it was allowed to silently fail (the
> "__GFP_NOWARN | __GFP_NORETRY" and the "ret &= ~VM_FAULT_OOM")? I
> think that would also simplify this a bit by making this type of
> memory a little less special.

The whole point, though, is that it needs to not fail or warn. It's
memory that can be dropped/zeroed at any moment, and the code is
deliberately robust to that.

Jason