diff mbox series

[4/4] mm/gup: adapt get_user_page_vma_remote() to never return NULL

Message ID b57d5f6618818f2f781a664039ace025c932074c.1696174961.git.lstoakes@gmail.com
State New
Headers
Series various improvements to the GUP interface |

Commit Message

Lorenzo Stoakes Oct. 1, 2023, 4 p.m. UTC 
  get_user_pages_remote() will never return 0 except in the case of
FOLL_NOWAIT being specified, which we explicitly disallow.

This simplifies error handling for the caller and avoids the awkwardness of
dealing with both errors and failing to pin. Failing to pin here is an
error.

Suggested-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
---
 arch/arm64/kernel/mte.c |  4 ++--
 include/linux/mm.h      | 16 +++++++++++++---
 kernel/events/uprobes.c |  4 ++--
 mm/memory.c             |  3 +--
 4 files changed, 18 insertions(+), 9 deletions(-)

Comments

Catalin Marinas Oct. 2, 2023, 10:16 a.m. UTC | #1 
On Sun, Oct 01, 2023 at 05:00:05PM +0100, Lorenzo Stoakes wrote:
> get_user_pages_remote() will never return 0 except in the case of
> FOLL_NOWAIT being specified, which we explicitly disallow.
> 
> This simplifies error handling for the caller and avoids the awkwardness of
> dealing with both errors and failing to pin. Failing to pin here is an
> error.
> 
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
> ---
>  arch/arm64/kernel/mte.c |  4 ++--

For arm64:

Acked-by: Catalin Marinas <catalin.marinas@arm.com>
David Hildenbrand Oct. 2, 2023, 11:08 a.m. UTC | #2 
On 01.10.23 18:00, Lorenzo Stoakes wrote:
> get_user_pages_remote() will never return 0 except in the case of
> FOLL_NOWAIT being specified, which we explicitly disallow.
> 
> This simplifies error handling for the caller and avoids the awkwardness of
> dealing with both errors and failing to pin. Failing to pin here is an
> error.
> 
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
> ---
>   arch/arm64/kernel/mte.c |  4 ++--
>   include/linux/mm.h      | 16 +++++++++++++---
>   kernel/events/uprobes.c |  4 ++--
>   mm/memory.c             |  3 +--
>   4 files changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> index 4edecaac8f91..8878b392df58 100644
> --- a/arch/arm64/kernel/mte.c
> +++ b/arch/arm64/kernel/mte.c
> @@ -411,8 +411,8 @@ static int __access_remote_tags(struct mm_struct *mm, unsigned long addr,
>   		struct page *page = get_user_page_vma_remote(mm, addr,
>   							     gup_flags, &vma);
>   
> -		if (IS_ERR_OR_NULL(page)) {
> -			err = page == NULL ? -EIO : PTR_ERR(page);
> +		if (IS_ERR(page)) {
> +			err = PTR_ERR(page);
>   			break;
>   		}
>   
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 7b89f7bd420d..da9631683d38 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2425,6 +2425,7 @@ long pin_user_pages_remote(struct mm_struct *mm,
>   			   unsigned int gup_flags, struct page **pages,
>   			   int *locked);
>   
> +/* Either retrieve a single VMA and page, or an error. */
>   static inline struct page *get_user_page_vma_remote(struct mm_struct *mm,
>   						    unsigned long addr,
>   						    int gup_flags,
> @@ -2432,12 +2433,21 @@ static inline struct page *get_user_page_vma_remote(struct mm_struct *mm,
>   {
>   	struct page *page;
>   	struct vm_area_struct *vma;
> -	int got = get_user_pages_remote(mm, addr, 1, gup_flags, &page, NULL);
> +	int got;
> +
> +	if (unlikely(gup_flags & FOLL_NOWAIT))
> +		return ERR_PTR(-EINVAL);
> +

Do we have any callers or do we want to make that official (document it) 
and use WARN_ON_ONCE() instead?

> +	got = get_user_pages_remote(mm, addr, 1, gup_flags, &page, NULL);
>   
>   	if (got < 0)
>   		return ERR_PTR(got);
> -	if (got == 0)
> -		return NULL;
> +
> +	/*
> +	 * get_user_pages_remote() is guaranteed to not return 0 for
> +	 * non-FOLL_NOWAIT contexts, so this should never happen.
> +	 */
> +	VM_WARN_ON(got == 0);

You should probably just drop that. Not worth the comment + code and its 
better checked inside get_user_pages_remote().

Ideally, just document that behavior for get_user_pages_remote() "Will 
never return 0 without FOLL_NOWAIT."
Lorenzo Stoakes Oct. 2, 2023, 10:56 p.m. UTC | #3 
On Mon, Oct 02, 2023 at 01:08:41PM +0200, David Hildenbrand wrote:
> On 01.10.23 18:00, Lorenzo Stoakes wrote:
> > get_user_pages_remote() will never return 0 except in the case of
> > FOLL_NOWAIT being specified, which we explicitly disallow.
> >
> > This simplifies error handling for the caller and avoids the awkwardness of
> > dealing with both errors and failing to pin. Failing to pin here is an
> > error.
> >
> > Suggested-by: Arnd Bergmann <arnd@arndb.de>
> > Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
> > ---
> >   arch/arm64/kernel/mte.c |  4 ++--
> >   include/linux/mm.h      | 16 +++++++++++++---
> >   kernel/events/uprobes.c |  4 ++--
> >   mm/memory.c             |  3 +--
> >   4 files changed, 18 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> > index 4edecaac8f91..8878b392df58 100644
> > --- a/arch/arm64/kernel/mte.c
> > +++ b/arch/arm64/kernel/mte.c
> > @@ -411,8 +411,8 @@ static int __access_remote_tags(struct mm_struct *mm, unsigned long addr,
> >   		struct page *page = get_user_page_vma_remote(mm, addr,
> >   							     gup_flags, &vma);
> > -		if (IS_ERR_OR_NULL(page)) {
> > -			err = page == NULL ? -EIO : PTR_ERR(page);
> > +		if (IS_ERR(page)) {
> > +			err = PTR_ERR(page);
> >   			break;
> >   		}
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 7b89f7bd420d..da9631683d38 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -2425,6 +2425,7 @@ long pin_user_pages_remote(struct mm_struct *mm,
> >   			   unsigned int gup_flags, struct page **pages,
> >   			   int *locked);
> > +/* Either retrieve a single VMA and page, or an error. */
> >   static inline struct page *get_user_page_vma_remote(struct mm_struct *mm,
> >   						    unsigned long addr,
> >   						    int gup_flags,
> > @@ -2432,12 +2433,21 @@ static inline struct page *get_user_page_vma_remote(struct mm_struct *mm,
> >   {
> >   	struct page *page;
> >   	struct vm_area_struct *vma;
> > -	int got = get_user_pages_remote(mm, addr, 1, gup_flags, &page, NULL);
> > +	int got;
> > +
> > +	if (unlikely(gup_flags & FOLL_NOWAIT))
> > +		return ERR_PTR(-EINVAL);
> > +
>
> Do we have any callers or do we want to make that official (document it) and
> use WARN_ON_ONCE() instead?

We have no callers who want to do that, will WARN_ON_ONCE() and update
comment in v2.

>
> > +	got = get_user_pages_remote(mm, addr, 1, gup_flags, &page, NULL);
> >   	if (got < 0)
> >   		return ERR_PTR(got);
> > -	if (got == 0)
> > -		return NULL;
> > +
> > +	/*
> > +	 * get_user_pages_remote() is guaranteed to not return 0 for
> > +	 * non-FOLL_NOWAIT contexts, so this should never happen.
> > +	 */
> > +	VM_WARN_ON(got == 0);
>
> You should probably just drop that. Not worth the comment + code and its
> better checked inside get_user_pages_remote().
>
> Ideally, just document that behavior for get_user_pages_remote() "Will never
> return 0 without FOLL_NOWAIT."

Well you'd need to put it at all the other callers of
__get_user_pages_locked() too :) so I think probably not worth doing that,
at least in this patch series.

In any case, we have explicitly added this check there to ensure that's not
possible so I think we're good, will just strip in v2.

>
> --
> Cheers,
>
> David / dhildenb
>
Jason Gunthorpe Oct. 9, 2023, 10:24 p.m. UTC | #4 
On Sun, Oct 01, 2023 at 05:00:05PM +0100, Lorenzo Stoakes wrote:
> get_user_pages_remote() will never return 0 except in the case of
> FOLL_NOWAIT being specified, which we explicitly disallow.
> 
> This simplifies error handling for the caller and avoids the awkwardness of
> dealing with both errors and failing to pin. Failing to pin here is an
> error.
> 
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
> ---
>  arch/arm64/kernel/mte.c |  4 ++--
>  include/linux/mm.h      | 16 +++++++++++++---
>  kernel/events/uprobes.c |  4 ++--
>  mm/memory.c             |  3 +--
>  4 files changed, 18 insertions(+), 9 deletions(-)

Good riddance to IS_ERR_OR_NULL

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason
diff mbox series

Patch

diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
index 4edecaac8f91..8878b392df58 100644
--- a/arch/arm64/kernel/mte.c
+++ b/arch/arm64/kernel/mte.c
@@ -411,8 +411,8 @@  static int __access_remote_tags(struct mm_struct *mm, unsigned long addr,
 		struct page *page = get_user_page_vma_remote(mm, addr,
 							     gup_flags, &vma);
 
-		if (IS_ERR_OR_NULL(page)) {
-			err = page == NULL ? -EIO : PTR_ERR(page);
+		if (IS_ERR(page)) {
+			err = PTR_ERR(page);
 			break;
 		}
 
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 7b89f7bd420d..da9631683d38 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2425,6 +2425,7 @@  long pin_user_pages_remote(struct mm_struct *mm,
 			   unsigned int gup_flags, struct page **pages,
 			   int *locked);
 
+/* Either retrieve a single VMA and page, or an error. */
 static inline struct page *get_user_page_vma_remote(struct mm_struct *mm,
 						    unsigned long addr,
 						    int gup_flags,
@@ -2432,12 +2433,21 @@  static inline struct page *get_user_page_vma_remote(struct mm_struct *mm,
 {
 	struct page *page;
 	struct vm_area_struct *vma;
-	int got = get_user_pages_remote(mm, addr, 1, gup_flags, &page, NULL);
+	int got;
+
+	if (unlikely(gup_flags & FOLL_NOWAIT))
+		return ERR_PTR(-EINVAL);
+
+	got = get_user_pages_remote(mm, addr, 1, gup_flags, &page, NULL);
 
 	if (got < 0)
 		return ERR_PTR(got);
-	if (got == 0)
-		return NULL;
+
+	/*
+	 * get_user_pages_remote() is guaranteed to not return 0 for
+	 * non-FOLL_NOWAIT contexts, so this should never happen.
+	 */
+	VM_WARN_ON(got == 0);
 
 	vma = vma_lookup(mm, addr);
 	if (WARN_ON_ONCE(!vma)) {
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 3048589e2e85..435aac1d8c27 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -474,8 +474,8 @@  int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm,
 		gup_flags |= FOLL_SPLIT_PMD;
 	/* Read the page with vaddr into memory */
 	old_page = get_user_page_vma_remote(mm, vaddr, gup_flags, &vma);
-	if (IS_ERR_OR_NULL(old_page))
-		return old_page ? PTR_ERR(old_page) : 0;
+	if (IS_ERR(old_page))
+		return PTR_ERR(old_page);
 
 	ret = verify_opcode(old_page, vaddr, &opcode);
 	if (ret <= 0)
diff --git a/mm/memory.c b/mm/memory.c
index e2743aa95b56..f2eef3d1cf58 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -5905,7 +5905,7 @@  static int __access_remote_vm(struct mm_struct *mm, unsigned long addr,
 		struct page *page = get_user_page_vma_remote(mm, addr,
 							     gup_flags, &vma);
 
-		if (IS_ERR_OR_NULL(page)) {
+		if (IS_ERR(page)) {
 			/* We might need to expand the stack to access it */
 			vma = vma_lookup(mm, addr);
 			if (!vma) {
@@ -5919,7 +5919,6 @@  static int __access_remote_vm(struct mm_struct *mm, unsigned long addr,
 				continue;
 			}
 
-
 			/*
 			 * Check if this is a VM_IO | VM_PFNMAP VMA, which
 			 * we can access using slightly different code.