[suggestion] mm/gup: avoid IS_ERR_OR_NULL

  From: Arnd Bergmann <arnd@arndb.de>

While looking at an unused-variable warning, I noticed a new interface coming
in that requires the use of IS_ERR_OR_NULL(), which tends to indicate bad
interface design and is usually surprising to users.

Change get_user_page_vma_remote() to return -EIO when no pages were
found and adapt the callers to match.

Fixes: eca1a00155df ("mm/gup: remove vmas parameter from get_user_pages_remote()")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
I see the real bug is already fixed, but this seemed worth pointing out still.
Not sure if this is the best way to handle the return types here, but the version
in linux-next doesn't look great either.
---
 arch/arm64/kernel/mte.c | 4 ++--
 include/linux/mm.h      | 2 +-
 kernel/events/uprobes.c | 5 ++++-
 mm/memory.c             | 2 +-
 4 files changed, 8 insertions(+), 5 deletions(-)
  

Given you are sharply criticising the code I authored here, is it too much
to ask for you to cc- me, the author on commentaries like this? Thanks.

On Fri, May 19, 2023 at 11:39:13AM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> While looking at an unused-variable warning, I noticed a new interface coming
> in that requires the use of IS_ERR_OR_NULL(), which tends to indicate bad
> interface design and is usually surprising to users.

I am not sure I understand your reasoning, why does it 'tend to indicate
bad interface design'? You say that as if it is an obvious truth. Not
obvious to me at all.

There are 3 possible outcomes from the function - an error, the function
failing to pin a page, or it succeeding in doing so. For some of the
callers that results in an error, for others it is not an error.

Overloading EIO on the assumption that gup will never, ever return this
indicating an error seems to me a worse solution.

>
> Change get_user_page_vma_remote() to return -EIO when no pages were
> found and adapt the callers to match.
>
> Fixes: eca1a00155df ("mm/gup: remove vmas parameter from get_user_pages_remote()")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> I see the real bug is already fixed, but this seemed worth pointing out still.
> Not sure if this is the best way to handle the return types here, but the version
> in linux-next doesn't look great either.
> ---
>  arch/arm64/kernel/mte.c | 4 ++--
>  include/linux/mm.h      | 2 +-
>  kernel/events/uprobes.c | 5 ++++-
>  mm/memory.c             | 2 +-
>  4 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> index 4c5ef9b20065..6983ba35ce16 100644
> --- a/arch/arm64/kernel/mte.c
> +++ b/arch/arm64/kernel/mte.c
> @@ -434,8 +434,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 42ff3e04c006..4bb172e4818c 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2397,7 +2397,7 @@ static inline struct page *get_user_page_vma_remote(struct mm_struct *mm,
>  	if (got < 0)
>  		return ERR_PTR(got);
>  	if (got == 0)
> -		return NULL;
> +		return ERR_PTR(-EIO);
>
>  	vma = vma_lookup(mm, addr);
>  	if (WARN_ON_ONCE(!vma)) {
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index cac3aef7c6f7..9cf2d4ba760e 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -474,7 +474,10 @@ 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))
> +	if (old_page == ERR_PTR(-EIO))
> +		return 0;
> +
> +	if (IS_ERR(old_page))
>  		return PTR_ERR(old_page);

I hate this, you're now using an error to indicate a non-error state.

Also you have no idea whether get_user_page_vma_remote() has encountered an
error condition returning -EIO rather than not pinning anything so this
could also be broken.

>
>  	ret = verify_opcode(old_page, vaddr, &opcode);
> diff --git a/mm/memory.c b/mm/memory.c
> index 8358f3b853f2..f9a81278e76d 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -5604,7 +5604,7 @@ int __access_remote_vm(struct mm_struct *mm, unsigned long addr, void *buf,
>  		struct page *page = get_user_page_vma_remote(mm, addr,
>  							     gup_flags, &vma);
>
> -		if (IS_ERR_OR_NULL(page)) {
> +		if (IS_ERR(page)) {
>  #ifndef CONFIG_HAVE_IOREMAP_PROT
>  			break;
>  #else
> --
> 2.39.2
>

Not a fan at all of this patch, it doesn't achieve anything useful, is in
service of some theoretical improvement, and actually introduces a new
class of bug (differentiating EIO and failing to pin).
  

On Fri, May 19, 2023, at 16:51, Lorenzo Stoakes wrote:
> Given you are sharply criticising the code I authored here, is it too much
> to ask for you to cc- me, the author on commentaries like this? Thanks.

My mistake, I expected this to get added automatically based on
the "Fixes:" tag, I probably dropped you by accident in the end.

> On Fri, May 19, 2023 at 11:39:13AM +0200, Arnd Bergmann wrote:
>> From: Arnd Bergmann <arnd@arndb.de>
>>
>> While looking at an unused-variable warning, I noticed a new interface coming
>> in that requires the use of IS_ERR_OR_NULL(), which tends to indicate bad
>> interface design and is usually surprising to users.
>
> I am not sure I understand your reasoning, why does it 'tend to indicate
> bad interface design'? You say that as if it is an obvious truth. Not
> obvious to me at all.
>
> There are 3 possible outcomes from the function - an error, the function
> failing to pin a page, or it succeeding in doing so. For some of the
> callers that results in an error, for others it is not an error.
>
> Overloading EIO on the assumption that gup will never, ever return this
> indicating an error seems to me a worse solution.

The problem is that we have inconsistent error handling in functions
that return an object, about half of them use NULL to indicate an error,
and the other half use ERR_PTR(), and users frequently get those
wrong by picking the wrong one. Functions that can return both make
this worse because whichever of the two normal ways a user expects,
they still get it wrong.

> Not a fan at all of this patch, it doesn't achieve anything useful, is in
> service of some theoretical improvement, and actually introduces a new
> class of bug (differentiating EIO and failing to pin).

Having another -EIO return code is a problem, so I agree that
my patch wouldn't be good either. Maybe separating the error return
from the page pointer by passing a 'struct page **p' argument that
gets filled would help?

    Arnd
  

On Fri, May 19, 2023 at 05:09:35PM +0200, Arnd Bergmann wrote:
> On Fri, May 19, 2023, at 16:51, Lorenzo Stoakes wrote:
> > Given you are sharply criticising the code I authored here, is it too much
> > to ask for you to cc- me, the author on commentaries like this? Thanks.
>
> My mistake, I expected this to get added automatically based on
> the "Fixes:" tag, I probably dropped you by accident in the end.
>

OK no worries, it's often the way that something is purely accidental but
seems ruder than intended (or even rude at all) because text is a terrible
format :)

> > On Fri, May 19, 2023 at 11:39:13AM +0200, Arnd Bergmann wrote:
> >> From: Arnd Bergmann <arnd@arndb.de>
> >>
> >> While looking at an unused-variable warning, I noticed a new interface coming
> >> in that requires the use of IS_ERR_OR_NULL(), which tends to indicate bad
> >> interface design and is usually surprising to users.
> >
> > I am not sure I understand your reasoning, why does it 'tend to indicate
> > bad interface design'? You say that as if it is an obvious truth. Not
> > obvious to me at all.
> >
> > There are 3 possible outcomes from the function - an error, the function
> > failing to pin a page, or it succeeding in doing so. For some of the
> > callers that results in an error, for others it is not an error.
> >
> > Overloading EIO on the assumption that gup will never, ever return this
> > indicating an error seems to me a worse solution.
>
> The problem is that we have inconsistent error handling in functions
> that return an object, about half of them use NULL to indicate an error,
> and the other half use ERR_PTR(), and users frequently get those
> wrong by picking the wrong one. Functions that can return both make
> this worse because whichever of the two normal ways a user expects,
> they still get it wrong.
>
> > Not a fan at all of this patch, it doesn't achieve anything useful, is in
> > service of some theoretical improvement, and actually introduces a new
> > class of bug (differentiating EIO and failing to pin).
>
> Having another -EIO return code is a problem, so I agree that
> my patch wouldn't be good either. Maybe separating the error return
> from the page pointer by passing a 'struct page **p' argument that
> gets filled would help?

Yeah I see your point, in the majority of cases failing to pin is an error,
I just wonder if something like adding another parameter wouldn't just add
more noise/confusion here than it saves?

Sadly I think aspects of this are C sucking at dealing with multiple return
values sanely, and there probably isn't a totally nice way of dealing with
this.

>
>     Arnd
  

On Fri, May 19, 2023 at 03:51:51PM +0100, Lorenzo Stoakes wrote:
> Given you are sharply criticising the code I authored here, is it too much
> to ask for you to cc- me, the author on commentaries like this? Thanks.
> 
> On Fri, May 19, 2023 at 11:39:13AM +0200, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > While looking at an unused-variable warning, I noticed a new interface coming
> > in that requires the use of IS_ERR_OR_NULL(), which tends to indicate bad
> > interface design and is usually surprising to users.
> 
> I am not sure I understand your reasoning, why does it 'tend to indicate
> bad interface design'? You say that as if it is an obvious truth. Not
> obvious to me at all.
>
> There are 3 possible outcomes from the function - an error, the function
> failing to pin a page, or it succeeding in doing so. For some of the
> callers that results in an error, for others it is not an error.

No, there really isn't.

Either it pins the page or it doesn't. Returning "NULL" to mean a
specific kind of failure was encountered is crazy.. Especially if we
don't document what that specific failure even was.

IIRC if you look really closely the only time get_user_pages()
actually returns 0 is if the input argument validation fails, which I
think is a bug that should be fixed.

get_user_pages() never returns 0, so get_user_page_vma_remote() never
returns NULL. Until we get there collapsing the 0 to EIO is perfectly
fine.

Jason
  

On Fri, May 19, 2023 at 07:17:41PM -0300, Jason Gunthorpe wrote:
> On Fri, May 19, 2023 at 03:51:51PM +0100, Lorenzo Stoakes wrote:
> > Given you are sharply criticising the code I authored here, is it too much
> > to ask for you to cc- me, the author on commentaries like this? Thanks.
> >
> > On Fri, May 19, 2023 at 11:39:13AM +0200, Arnd Bergmann wrote:
> > > From: Arnd Bergmann <arnd@arndb.de>
> > >
> > > While looking at an unused-variable warning, I noticed a new interface coming
> > > in that requires the use of IS_ERR_OR_NULL(), which tends to indicate bad
> > > interface design and is usually surprising to users.
> >
> > I am not sure I understand your reasoning, why does it 'tend to indicate
> > bad interface design'? You say that as if it is an obvious truth. Not
> > obvious to me at all.
> >
> > There are 3 possible outcomes from the function - an error, the function
> > failing to pin a page, or it succeeding in doing so. For some of the
> > callers that results in an error, for others it is not an error.
>
> No, there really isn't.
>
> Either it pins the page or it doesn't. Returning "NULL" to mean a
> specific kind of failure was encountered is crazy.. Especially if we
> don't document what that specific failure even was.
>

It's not a specific kind of failure, it's literally "I didn't pin any
pages" which a caller may or may not choose to interpret as a failure.

> IIRC if you look really closely the only time get_user_pages()
> actually returns 0 is if the input argument validation fails, which I
> think is a bug that should be fixed.

That can be a reason for gup returning 0 but also if it you look at the
main loop in __get_user_pages_locked(), if it can't find the VMA it will
bail early, OR if the VMA flags are not as expected it'll bail early.

>
> get_user_pages() never returns 0, so get_user_page_vma_remote() never
> returns NULL. Until we get there collapsing the 0 to EIO is perfectly
> fine.

Well no, as shown above actually there is a distinct third state,
i.e. couldn't pin, which if you see there is at least one case where the
caller differentiates between an error and not being able to pin -
uprobe_write_opcode() - which treats failure to pin as a non-error state.

Also if we decided at some point to return -EIO as an error suddenly we
would be treating an error state as not an error state in the proposed code
which sounds like a foot gun.

>
> Jason
  

On Sat, May 20, 2023 at 06:19:37AM +0100, Lorenzo Stoakes wrote:
> On Fri, May 19, 2023 at 07:17:41PM -0300, Jason Gunthorpe wrote:
> > On Fri, May 19, 2023 at 03:51:51PM +0100, Lorenzo Stoakes wrote:
> > > Given you are sharply criticising the code I authored here, is it too much
> > > to ask for you to cc- me, the author on commentaries like this? Thanks.
> > >
> > > On Fri, May 19, 2023 at 11:39:13AM +0200, Arnd Bergmann wrote:
> > > > From: Arnd Bergmann <arnd@arndb.de>
> > > >
> > > > While looking at an unused-variable warning, I noticed a new interface coming
> > > > in that requires the use of IS_ERR_OR_NULL(), which tends to indicate bad
> > > > interface design and is usually surprising to users.
> > >
> > > I am not sure I understand your reasoning, why does it 'tend to indicate
> > > bad interface design'? You say that as if it is an obvious truth. Not
> > > obvious to me at all.
> > >
> > > There are 3 possible outcomes from the function - an error, the function
> > > failing to pin a page, or it succeeding in doing so. For some of the
> > > callers that results in an error, for others it is not an error.
> >
> > No, there really isn't.
> >
> > Either it pins the page or it doesn't. Returning "NULL" to mean a
> > specific kind of failure was encountered is crazy.. Especially if we
> > don't document what that specific failure even was.
> >
> 
> It's not a specific kind of failure, it's literally "I didn't pin any
> pages" which a caller may or may not choose to interpret as a failure.

Any time gup fails it didn't pin any pages, that is the whole
point. All that is happening is some ill defined subset of gup errors
are returning 0 instead of an error code.

If we want to enable callers to ignore certain errors then we need to
return error codes with well defined meanings, have documentation what
errors are included and actually make it sane.

> That can be a reason for gup returning 0 but also if it you look at the
> main loop in __get_user_pages_locked(), if it can't find the VMA it will
> bail early, OR if the VMA flags are not as expected it'll bail early.

And how does that make any sense? Missing VMA should be EFAULT.
 
> caller differentiates between an error and not being able to pin -
> uprobe_write_opcode() - which treats failure to pin as a non-error state.

That looks like a bug since the function returns 0 on success but it
clearly didn't succeed.

> Also if we decided at some point to return -EIO as an error suddenly we
> would be treating an error state as not an error state in the proposed code
> which sounds like a foot gun.

No, this returning 0 on failure is a foot gun. Failing to pin a single
page is always an error, the only question is what sub reason caused
the error to happen. There is no third case where it is not an error.

Jason
  

On Sat, May 20, 2023 at 05:25:52AM -0300, Jason Gunthorpe wrote:
> On Sat, May 20, 2023 at 06:19:37AM +0100, Lorenzo Stoakes wrote:
> > On Fri, May 19, 2023 at 07:17:41PM -0300, Jason Gunthorpe wrote:
> > > On Fri, May 19, 2023 at 03:51:51PM +0100, Lorenzo Stoakes wrote:
> > > > Given you are sharply criticising the code I authored here, is it too much
> > > > to ask for you to cc- me, the author on commentaries like this? Thanks.
> > > >
> > > > On Fri, May 19, 2023 at 11:39:13AM +0200, Arnd Bergmann wrote:
> > > > > From: Arnd Bergmann <arnd@arndb.de>
> > > > >
> > > > > While looking at an unused-variable warning, I noticed a new interface coming
> > > > > in that requires the use of IS_ERR_OR_NULL(), which tends to indicate bad
> > > > > interface design and is usually surprising to users.
> > > >
> > > > I am not sure I understand your reasoning, why does it 'tend to indicate
> > > > bad interface design'? You say that as if it is an obvious truth. Not
> > > > obvious to me at all.
> > > >
> > > > There are 3 possible outcomes from the function - an error, the function
> > > > failing to pin a page, or it succeeding in doing so. For some of the
> > > > callers that results in an error, for others it is not an error.
> > >
> > > No, there really isn't.
> > >
> > > Either it pins the page or it doesn't. Returning "NULL" to mean a
> > > specific kind of failure was encountered is crazy.. Especially if we
> > > don't document what that specific failure even was.
> > >
> >
> > It's not a specific kind of failure, it's literally "I didn't pin any
> > pages" which a caller may or may not choose to interpret as a failure.
>
> Any time gup fails it didn't pin any pages, that is the whole
> point. All that is happening is some ill defined subset of gup errors
> are returning 0 instead of an error code.
>
> If we want to enable callers to ignore certain errors then we need to
> return error codes with well defined meanings, have documentation what
> errors are included and actually make it sane.

Yeah I agree it's not exactly great that a failure to pin can be considered
an ordinary case, but I don't think a wrapper function is where we should
be trying to fix this.

In fact this patch isn't even fixing it, it's treating EIO as a success
case for the (possibly broken) uprobe case.

I think we are at the wrong level of abstraction here, basically.

>
> > That can be a reason for gup returning 0 but also if it you look at the
> > main loop in __get_user_pages_locked(), if it can't find the VMA it will
> > bail early, OR if the VMA flags are not as expected it'll bail early.
>
> And how does that make any sense? Missing VMA should be EFAULT.

Yeah missing VMA doesn't really make sense since we invoke the function
with the mmap lock held (it _could_ make sense if you were calling it via
one of the unlocked functions, speculatively, though how sensible doing
that is another discussion...)

>
> > caller differentiates between an error and not being able to pin -
> > uprobe_write_opcode() - which treats failure to pin as a non-error state.
>
> That looks like a bug since the function returns 0 on success but it
> clearly didn't succeed.

The code is specifically handling a failure-to-pin separately - set_swbp() ->
uprobe_write_opcode() -> install_breakpoint() explicitly does the following:-

	ret = set_swbp(&uprobe->arch, mm, vaddr);
	if (!ret)
		clear_bit(MMF_RECALC_UPROBES, &mm->flags);

So even if this is... questionable, the code literally does want to
differentiate between an error and a failure to pin.

Presumably this is because of the flag check, but yeah we should be
differentiating between sub-cases.

>
> > Also if we decided at some point to return -EIO as an error suddenly we
> > would be treating an error state as not an error state in the proposed code
> > which sounds like a foot gun.
>
> No, this returning 0 on failure is a foot gun. Failing to pin a single
> page is always an error, the only question is what sub reason caused
> the error to happen. There is no third case where it is not an error.
>
> Jason

The uprobe path thinks otherwise, but maybe the answer is that we just need
to -EFAULT on missing VMA and -EPERM on invalid flags.

I could look into a patch that tries to undo this convention, and then we
could revisit this for the wrapper function too.
  

On Sat, May 20, 2023 at 10:12:40AM +0100, Lorenzo Stoakes wrote:
> > No, this returning 0 on failure is a foot gun. Failing to pin a single
> > page is always an error, the only question is what sub reason caused
> > the error to happen. There is no third case where it is not an error.
> 
> The uprobe path thinks otherwise, but maybe the answer is that we just need
> to -EFAULT on missing VMA and -EPERM on invalid flags.

I think uprobe is just broken to think there is a third outcome. Let's
just fix it instead of trying to pretend it makes sense.

Jason
  

On Sat, May 27, 2023 at 06:52:01AM -0300, Jason Gunthorpe wrote:
> On Sat, May 20, 2023 at 10:12:40AM +0100, Lorenzo Stoakes wrote:
> > > No, this returning 0 on failure is a foot gun. Failing to pin a single
> > > page is always an error, the only question is what sub reason caused
> > > the error to happen. There is no third case where it is not an error.
> >
> > The uprobe path thinks otherwise, but maybe the answer is that we just need
> > to -EFAULT on missing VMA and -EPERM on invalid flags.
>
> I think uprobe is just broken to think there is a third outcome. Let's
> just fix it instead of trying to pretend it makes sense.

Sure, will take a look at that if I get a chance. We can at the very least
adjust get_user_page_vma_remote() with this fixed.

Do you feel that a partially successful pinning for other GUP callers
should equally be treated as an error (and pages unpinned -> return error
code)? In that instance we'd need to audit things somewhat.

I have a couple more GUP cleanups saved up, so could add that to my queue
of things to look at between book work :)

>
> Jason
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
  

On Sun, May 28, 2023 at 04:13:44PM +0100, Lorenzo Stoakes wrote:
> On Sat, May 27, 2023 at 06:52:01AM -0300, Jason Gunthorpe wrote:
> > On Sat, May 20, 2023 at 10:12:40AM +0100, Lorenzo Stoakes wrote:
> > > > No, this returning 0 on failure is a foot gun. Failing to pin a single
> > > > page is always an error, the only question is what sub reason caused
> > > > the error to happen. There is no third case where it is not an error.
> > >
> > > The uprobe path thinks otherwise, but maybe the answer is that we just need
> > > to -EFAULT on missing VMA and -EPERM on invalid flags.
> >
> > I think uprobe is just broken to think there is a third outcome. Let's
> > just fix it instead of trying to pretend it makes sense.
> 
> Sure, will take a look at that if I get a chance. We can at the very least
> adjust get_user_page_vma_remote() with this fixed.
> 
> Do you feel that a partially successful pinning for other GUP callers
> should equally be treated as an error (and pages unpinned -> return error
> code)? In that instance we'd need to audit things somewhat.

That seems more deeply ingrained at least, I'm not as keen to change
it as to get rid of the 0 return result.

Jason
  

On 5/28/23 09:22, Jason Gunthorpe wrote:
> On Sun, May 28, 2023 at 04:13:44PM +0100, Lorenzo Stoakes wrote:
>> On Sat, May 27, 2023 at 06:52:01AM -0300, Jason Gunthorpe wrote:
>>> On Sat, May 20, 2023 at 10:12:40AM +0100, Lorenzo Stoakes wrote:
>>>>> No, this returning 0 on failure is a foot gun. Failing to pin a single
>>>>> page is always an error, the only question is what sub reason caused
>>>>> the error to happen. There is no third case where it is not an error.
>>>>
>>>> The uprobe path thinks otherwise, but maybe the answer is that we just need
>>>> to -EFAULT on missing VMA and -EPERM on invalid flags.
>>>
>>> I think uprobe is just broken to think there is a third outcome. Let's
>>> just fix it instead of trying to pretend it makes sense.
>>
>> Sure, will take a look at that if I get a chance. We can at the very least
>> adjust get_user_page_vma_remote() with this fixed.

Great!

We've had previous discussions about getting rid of this pseudo-tristate
errno in gup, so I just wanted to mention that I'm also glad to see the
movement toward, "return some pages, or else a -errno". That's progress.


>>
>> Do you feel that a partially successful pinning for other GUP callers
>> should equally be treated as an error (and pages unpinned -> return error
>> code)? In that instance we'd need to audit things somewhat.
> 
> That seems more deeply ingrained at least, I'm not as keen to change
> it as to get rid of the 0 return result.
> 

Yes. It's not just "audit things somewhat", it's more like, "change
quite a few call sites, some of which actually gather sets of partial
results in a retry loop". So some actual coding changes there.


thanks,