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
