On Mon, Nov 04, 2019 at 12:57:59PM -0800, John Hubbard wrote:
On 11/4/19 12:37 PM, Jason Gunthorpe wrote:
On Mon, Nov 04, 2019 at 03:31:53PM -0500, Jerome Glisse wrote:
Note for Jason: the (a) or (b) items are talking about the vfio case, which is one of the two call sites that now use pin_longterm_pages_remote(), and the other one is infiniband:
drivers/infiniband/core/umem_odp.c:646: npages = pin_longterm_pages_remote(owning_process, owning_mm, drivers/vfio/vfio_iommu_type1.c:353: ret = pin_longterm_pages_remote(NULL, mm, vaddr, 1,
vfio should be reverted until it can be properly implemented. The issue is that when you fix the implementation you might break vfio existing user and thus regress the kernel from user point of view. So i rather have the change to vfio reverted, i believe it was not well understood when it got upstream, between in my 5.4 tree it is still gup_remote not longterm.
It is clearly a bug, vfio must use LONGTERM, and does right above this remote call:
if (mm == current->mm) { ret = get_user_pages(vaddr, 1, flags | FOLL_LONGTERM, page, vmas); } else { ret = get_user_pages_remote(NULL, mm, vaddr, 1, flags, page, vmas, NULL);I'm not even sure that it really makes any sense to build a 'if' like that, surely just always call remote??
Right, and I thought about this when converting, and realized that the above code is working around the current gup.c limitations, which are "cannot support gup remote with FOLL_LONGTERM".
But AFAICT it doesn't have a problem, the protection test is just too strict, and I guess the control flow needs a bit of fixing..
The issue is this:
static __always_inline long __get_user_pages_locked(): { if (locked) { /* if VM_FAULT_RETRY can be returned, vmas become invalid */ BUG_ON(vmas); /* check caller initialized locked */ BUG_ON(*locked != 1); }
so remote could be written as:
if (gup_flags & FOLL_LONGTERM) { if (WARN_ON_ONCE(locked)) return -EINVAL; return __gup_longterm_locked(...) }
return __get_user_pages_locked(...)
??
Jason