Re: [PATCH v1 ] nitro_enclaves: Add mmap_read_lock() for the get_user_pages() call

[Paraschiv, Andra-Irina]

On 20.12.2021 11:23, Vitaly Kuznetsov wrote:
> Andra Paraschiv <andraprs@xxxxxxxxxx> writes:
>
> > After commit 5b78ed24e8ec (mm/pagemap: add mmap_assert_locked()
> > annotations to find_vma*()), the call to get_user_pages() will trigger
> > the mmap assert.
> >
> > static inline void mmap_assert_locked(struct mm_struct *mm)
> > {
> >        lockdep_assert_held(&mm->mmap_lock);
> >        VM_BUG_ON_MM(!rwsem_is_locked(&mm->mmap_lock), mm);
> > }
> >
> > [   62.521410] kernel BUG at include/linux/mmap_lock.h:156!
> > ...........................................................
> > [   62.538938] RIP: 0010:find_vma+0x32/0x80
> > ...........................................................
> > [   62.605889] Call Trace:
> > [   62.608502]  <TASK>
> > [   62.610956]  ? lock_timer_base+0x61/0x80
> > [   62.614106]  find_extend_vma+0x19/0x80
> > [   62.617195]  __get_user_pages+0x9b/0x6a0
> > [   62.620356]  __gup_longterm_locked+0x42d/0x450
> > [   62.623721]  ? finish_wait+0x41/0x80
> > [   62.626748]  ? __kmalloc+0x178/0x2f0
> > [   62.629768]  ne_set_user_memory_region_ioctl.isra.0+0x225/0x6a0 [nitro_enclaves]
> > [   62.635776]  ne_enclave_ioctl+0x1cf/0x6d7 [nitro_enclaves]
> > [   62.639541]  __x64_sys_ioctl+0x82/0xb0
> > [   62.642620]  do_syscall_64+0x3b/0x90
> > [   62.645642]  entry_SYSCALL_64_after_hwframe+0x44/0xae
> >
> > Add mmap_read_lock() for the get_user_pages() call when setting the
> > enclave memory regions.
> >
> > Signed-off-by: Andra Paraschiv <andraprs@xxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
>
> In case commit 5b78ed24e8ec broke Nitro Enclaves driver, we need to
> explicitly state this:
>
> Fixes: 5b78ed24e8ec ("mm/pagemap: add mmap_assert_locked() annotations to find_vma*()")

Added to the commit message.

> > ---
> >   drivers/virt/nitro_enclaves/ne_misc_dev.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> > index 8939612ee0e0..6c51ff024036 100644
> > --- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
> > +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> > @@ -886,8 +886,13 @@ static int ne_set_user_memory_region_ioctl(struct ne_enclave *ne_enclave,
> >                        goto put_pages;
> >                }
> >
> > +             mmap_read_lock(current->mm);
> > +
> >                gup_rc = get_user_pages(mem_region.userspace_addr + memory_size, 1, FOLL_GET,
> >                                        ne_mem_region->pages + i, NULL);
> > +
> > +             mmap_read_unlock(current->mm);
> > +
>
> This looks very much like get_user_pages_unlocked(), I think we can use
> it instead of open-coding it.

Indeed, it's mentioned as well in the comment for 
get_user_pages_unlocked() this pattern of mmap_read_lock() / 
get_user_pages() / mmap_read_unlock().

Thank you for the feedback, Vitaly. I sent v2 that includes the 
mentioned updates.

Thanks,
Andra


> >                if (gup_rc < 0) {
> >                        rc = gup_rc;
>
> --
> Vitaly



Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.