Re: [RESEND RFC PATCH 4/5] mm/remote_mapping: use a pidfd to access memory belonging to unrelated process

[Oleg Nesterov <oleg@redhat.com>]

On 09/09, Mircea CIRJALIU - MELIU wrote:
>
> > From: Oleg Nesterov <oleg@redhat.com>
> >
> > But why is it safe to drop ->mmap_sem without checking
> > FAULT_FLAG_ALLOW_RETRY/RETRY_NOWAIT ?
> >
> Dropping mmap_sem will have the same effects regardless of FAULT_FLAG_ALLOW_RETRY/RETRY_NOWAIT.
> Another thread can unmap the VMA from underneath us, or remap another one in its place.
> In the end, the VMA has to be revalidated when re-acquiring the mmap_sem.
> Or am I wrong?!

To simplify, lets forget about RETRY_NOWAIT/TRIED.

Again, I can be easily wrong. But iiuc, you simply can't drop mmap_sem
if FAULT_FLAG_ALLOW_RETRY is not set, the caller doesn't expect mmap_sem
can be unlocked.

OTOH, if FAULT_FLAG_ALLOW_RETRY is set and you drop mmap_sem, you can
only return VM_FAULT_RETRY to let the caller know it was dropped.

> > > +	 * If FAULT_FLAG_ALLOW_RETRY is set, the mmap_sem must be
> > released
> > > +	 * before returning VM_FAULT_RETRY only if
> > FAULT_FLAG_RETRY_NOWAIT is
> > > +	 * not set.
> >
> > Well, iiuc FAULT_FLAG_ALLOW_RETRY means that ->fault() _can_ drop
> > mmap_sem and return VM_FAULT_RETRY (unless NOWAIT).
>
> That comment is just copied from elsewhere in the code.
> My interpretation was that the fault handler _should_ return with mmap_sem
> held or not depending on FAULT_FLAG_RETRY_NOWAIT.

Yes, this depends on FAULT_FLAG_RETRY_NOWAIT.

But your comment above looks as if he mmap_sem must be _always_ released
if FAULT_FLAG_ALLOW_RETRY && !FAULT_FLAG_RETRY_NOWAIT. This is not true.


Nevermind. If you ever resend this patch, please CC mm/ experts. I tried
to look at this code again and I feel it has much more problems, but as
I said this is not my area.

And I think you should split this patch, mirror_vm_fault() should come in
a separate patch to simplify the review.

Oleg.