From:   Vitaly Kuznetsov <vkuznets@redhat.com>
To:     Andra Paraschiv <andraprs@amazon.com>,
        linux-kernel <linux-kernel@vger.kernel.org>
Cc:     Alexandru Ciobotaru <alcioa@amazon.com>,
        Greg KH <gregkh@linuxfoundation.org>,
        Alexandru Vasile <lexnv@amazon.com>,
        Marcelo Cerri <marcelo.cerri@canonical.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Stefano Garzarella <sgarzare@redhat.com>,
        Stefan Hajnoczi <stefanha@redhat.com>,
        Tim Gardner <tim.gardner@canonical.com>,
        kvm <kvm@vger.kernel.org>,
        ne-devel-upstream <ne-devel-upstream@amazon.com>,
        stable <stable@vger.kernel.org>,
        Andra Paraschiv <andraprs@amazon.com>
Subject: Re: [PATCH =?utf-8?Q?v1=C2=A0=5D?= nitro_enclaves: Add
 mmap_read_lock() for the
 get_user_pages() call
In-Reply-To: <20211218103525.26739-1-andraprs@amazon.com>
References: <20211218103525.26739-1-andraprs@amazon.com>
Date:   Mon, 20 Dec 2021 10:23:35 +0100
Message-ID: <87o85btyso.fsf@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Andra Paraschiv <andraprs@amazon.com> 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@amazon.com>
> Cc: stable@vger.kernel.org

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*()")

> ---
>  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.

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

-- 
Vitaly