[PATCH v3 08/23] vfio, mm: fix get_user_pages_remote() and FOLL_LONGTERM

[John Hubbard]

On 11/12/19 2:43 PM, Dan Williams wrote:
... 
> Ah, sorry. This was the first time I had looked at this series and
> jumped in without reading the background.
> 
> Your patch as is looks ok, I assume you've removed the FOLL_LONGTERM
> warning in get_user_pages_remote in another patch?
> 

Actually, I haven't gone quite that far. Actually this patch is the last
change to that function. Therefore, at the end of this patchset, 
get_user_pages_remote() ends up with this check in it which
is a less-restrictive version of the warning:

	/*
	 * Current FOLL_LONGTERM behavior is incompatible with
	 * FAULT_FLAG_ALLOW_RETRY because of the FS DAX check requirement on
	 * vmas. However, this only comes up if locked is set, and there are
	 * callers that do request FOLL_LONGTERM, but do not set locked. So,
	 * allow what we can.
	 */
	if (gup_flags & FOLL_LONGTERM) {
		if (WARN_ON_ONCE(locked))
			return -EINVAL;
	}

Is that OK, or did you want to go further (possibly in a follow-up
patchset, as I'm hoping to get this one in soon)?  

...
>>> I think check_vma_flags() should do the ((FOLL_LONGTERM | FOLL_GET) &&
>>> vma_is_fsdax()) check and that would also remove the need for
>>> __gup_longterm_locked.
>>>
>>
>> Good idea, but there is still the call to check_and_migrate_cma_pages(),
>> inside __gup_longterm_locked().  So it's a little more involved and
>> we can't trivially delete __gup_longterm_locked() yet, right?
> 
> [ add Aneesh ]
> 
> Yes, you're right. I had overlooked that had snuck in there. That to
> me similarly needs to be pushed down into the core with its own FOLL
> flag, or it needs to be an explicit fixup that each caller does after
> get_user_pages. The fact that migration silently happens as a side
> effect of gup is too magical for my taste.
> 

Yes. It's an intrusive side effect that is surprising, and not in a 
"happy surprise" way. :) .   Fixing up the CMA pages by splitting that
functionality into separate function calls sounds like an improvement
worth exploring.


thanks,
-- 
John Hubbard
NVIDIA