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

[Oleg Nesterov <oleg@xxxxxxxxxx>]

it seems that nobody is going to review this patch ;)

So I tried to read mirror_vm_fault() and the usage of mmap_sem doesn't
look right to me. But let me repeat, this is not my area I can be easily
wrong, please correct me.

On 09/04, Adalbert Lazăr wrote:
>
> +static vm_fault_t mirror_vm_fault(struct vm_fault *vmf)
> +{
> +	struct vm_area_struct *vma = vmf->vma;
> +	struct mm_struct *mm = vma->vm_mm;
> +	struct remote_vma_context *ctx = vma->vm_private_data;
> +	struct remote_view *view = ctx->view;
> +	struct file *file = vma->vm_file;
> +	struct remote_file_context *fctx = file->private_data;
> +	unsigned long req_addr;
> +	unsigned int gup_flags;
> +	struct page *req_page;
> +	vm_fault_t result = VM_FAULT_SIGBUS;
> +	struct mm_struct *src_mm = fctx->mm;
> +	unsigned long seq;
> +	int idx;
> +
> +fault_retry:
> +	seq = mmu_interval_read_begin(&view->mmin);
> +
> +	idx = srcu_read_lock(&fctx->fault_srcu);
> +
> +	/* check if view was invalidated */
> +	if (unlikely(!READ_ONCE(view->valid))) {
> +		pr_debug("%s: region [%lx-%lx) was invalidated!!\n", __func__,
> +			view->offset, view->offset + view->size);
> +		goto out_invalid;		/* VM_FAULT_SIGBUS */
> +	}
> +
> +	/* drop current mm semapchore */
> +	up_read(&current->mm->mmap_sem);

Please use mmap_read_lock/unlock(mm) instead of down/up_read(mmap_sem).

But why is it safe to drop ->mmap_sem without checking
FAULT_FLAG_ALLOW_RETRY/RETRY_NOWAIT ?

> +	/* take remote mm semaphore */
> +	if (vmf->flags & FAULT_FLAG_ALLOW_RETRY) {
> +		if (!down_read_trylock(&src_mm->mmap_sem)) {

I don't understand this... perhaps you meant FAULT_FLAG_RETRY_NOWAIT ?

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

> +	if (vmf->flags & (FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_TRIED)) {
> +		if (!(vmf->flags & FAULT_FLAG_KILLABLE))
> +			if (current && fatal_signal_pending(current)) {
> +				up_read(&current->mm->mmap_sem);
> +				return VM_FAULT_RETRY;

I fail to understand this. But in any case, you do not need to check
current != NULL and up_read() looks wrong if RETRY_NOWAIT?

Oleg.