On Fri, Jun 29, 2018 at 11:10:56AM -0600, Jason Gunthorpe wrote:
> On Wed, Jun 27, 2018 at 10:44:27AM +0300, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> >
> > DMA mapping is time consuming operation and doesn't need to be
> > performed with mmap_sem semaphore is held on. Limit the scope of
> > that semaphore for accounting part only.
> >
> > Signed-off-by: Huy Nguyen <huyn@xxxxxxxxxxxx>
> > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > drivers/infiniband/core/umem.c | 17 +++++++++--------
> > 1 file changed, 9 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/umem.c b/drivers/infiniband/core/umem.c
> > index 498f59bb4989..626f3f3b8f44 100644
> > +++ b/drivers/infiniband/core/umem.c
> > @@ -84,7 +84,6 @@ struct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr,
> > struct ib_umem *umem;
> > struct page **page_list;
> > struct vm_area_struct **vma_list;
> > - unsigned long locked;
> > unsigned long lock_limit;
> > unsigned long cur_base;
> > unsigned long npages;
> > @@ -149,15 +148,16 @@ struct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr,
> >
> > npages = ib_umem_num_pages(umem);
> >
> > - down_write(¤t->mm->mmap_sem);
> > -
> > - locked = npages + current->mm->pinned_vm;
> > lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
> >
> > - if ((locked > lock_limit) && !capable(CAP_IPC_LOCK)) {
> > + down_write(¤t->mm->mmap_sem);
> > + current->mm->pinned_vm += npages;
> > + if ((current->mm->pinned_vm > lock_limit) && !capable(CAP_IPC_LOCK)) {
> > + up_write(¤t->mm->mmap_sem);
> > ret = -ENOMEM;
> > goto out;
> > }
> > + up_write(¤t->mm->mmap_sem);
>
> This change removes the lock around get_user_pages_longterm(), and we
> have this calltree:
>
> get_user_pages_longterm()
> get_user_pages()
> __get_user_pages_locked()
> __get_user_pages()
>
> And the comment for __get_user_pages() says:
>
> * Must be called with mmap_sem held. It may be released. See below.
> [..]
> * A caller using such a combination of @nonblocking and @gup_flags
> * must therefore hold the mmap_sem for reading only, and recognize
> * when it's been released. Otherwise, it must be held for either
> * reading or writing and will not be released.
>
> Though, confusingly, I checked a random selection of get_user_pages()
> callers and couldn't obviously find a current->mm->mmap_sem in a few
> cases. I think those are bugs. Will try to find out..
>
> If this is safe without a lock for some tricky reason then please
> respin with a comment explaining why
It was unclear to me too, now looking more deeply, I think that at least
read sem is needed to find VMAs (performed internally in gup). It means
that radeon is wrong:
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/radeon/radeon_ttm.c#L561
and i40w too:
https://elixir.bootlin.com/linux/latest/source/drivers/infiniband/hw/i40iw/i40iw_verbs.c#L1412
>
> Otherwise respin to hold the lock across get_user_pages_longterm, as
> it was before..
>
> Jason
Attachment:
signature.asc
Description: PGP signature
