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

On 09/09, Mircea CIRJALIU - MELIU wrote:

quoted

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.

quoted

quoted

+	 * If FAULT_FLAG_ALLOW_RETRY is set, the mmap_sem must be

released

quoted

+	 * before returning VM_FAULT_RETRY only if

FAULT_FLAG_RETRY_NOWAIT is

quoted

+	 * 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.