| Message ID | b57d5f6618818f2f781a664039ace025c932074c.1696174961.git.lstoakes@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | various improvements to the GUP interface | expand |
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>
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."
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 >
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 --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.
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(-)