mm/gup: avoid IS_ERR_OR_NULL
Subsystems (5)
PATCH 1/1 mm/gup: avoid IS_ERR_OR_NULL
From: Arnd Bergmann <[email protected]>
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.
---
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(-)
arch/arm64/kernel/mte.c +2 -2
@@ -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 ; }
include/linux/mm.h +1 -1
@@ -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)) {
kernel/events/uprobes.c +4 -1
@@ -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); ret =verify_opcode (old_page, vaddr, &opcode);
mm/memory.c +1 -1
@@ -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
Signature · 1 line
2.39.2
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.
Quoted 5 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
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.
Quoted 50 lines
Change
get_user_page_vma_remote()to return-EIOwhen 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 <[email protected]>---
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(-)arch/arm64/kernel/mte.c +2
-2@@ -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 ; }include/linux/mm.h +1
-1@@ -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)) {kernel/events/uprobes.c +0
-0
kernel/events/uprobes.c int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, · line 481
Quoted 9 lines
kernel/events/uprobes.c +4
-1@@ -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.
Quoted 16 lines
kernel/events/uprobes.c +0
-0ret =verify_opcode (old_page, vaddr, &opcode);mm/memory.c +1
-2@@ -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 --
Quoted 2 lines
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).
Quoted 2 lines · Lorenzo Stoakes
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.
Quoted 11 lines · Arnd Bergmann
Quoted 5 lines
From: Arnd Bergmann <[email protected]>
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.
Quoted 6 lines
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.
Quoted 3 lines
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
Quoted 7 lines · Arnd Bergmann
Quoted 2 lines · Lorenzo Stoakes
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 :)
Quoted 25 lines
Quoted 11 lines · Arnd Bergmann
Quoted 5 lines
From: Arnd Bergmann <[email protected]>
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.
Quoted 6 lines
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.
Quoted 8 lines
Quoted 3 lines
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
-EIOreturn 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.
Quoted 2 lines
Arnd
Quoted 10 lines · Lorenzo Stoakes
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.
Quoted 5 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
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.
Quoted 7 lines
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
Quoted 19 lines · Jason Gunthorpe
Quoted 10 lines · Lorenzo Stoakes
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.
Quoted 5 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
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.Quoted 7 lines
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.
Quoted 6 lines
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.
Quoted 3 lines
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.
Quoted 4 lines
get_user_pages()never returns 0, soget_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.
Quoted 2 lines
Jason
Quoted 22 lines · Lorenzo Stoakes
Quoted 21 lines · Jason Gunthorpe
Quoted 10 lines · Lorenzo Stoakes
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.
Quoted 5 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
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.Quoted 7 lines
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.
Quoted 7 lines
Quoted 4 lines
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.
Quoted 3 lines
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.
Quoted 2 lines
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.
Quoted 3 lines
Also if we decided at some point to return
-EIOas 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
Quoted 31 lines · Jason Gunthorpe
Quoted 22 lines · Lorenzo Stoakes
Quoted 21 lines · Jason Gunthorpe
Quoted 10 lines · Lorenzo Stoakes
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.
Quoted 5 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
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.Quoted 7 lines
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.
Quoted 7 lines
Quoted 4 lines
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.
Quoted 7 lines
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.
Quoted 6 lines
Quoted 4 lines
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...)
Quoted 6 lines
Quoted 3 lines
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.
Quoted 10 lines
Quoted 4 lines
Also if we decided at some point to return
-EIOas 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.
Quoted 6 lines · Lorenzo Stoakes
Quoted 3 lines
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
-EFAULTon missing VMA and-EPERMon 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
Quoted 10 lines · Jason Gunthorpe
Quoted 6 lines · Lorenzo Stoakes
Quoted 3 lines
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
-EFAULTon missing VMA and-EPERMon 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 :)
Quoted 7 lines
Jason
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Quoted 12 lines · Lorenzo Stoakes
Quoted 10 lines · Jason Gunthorpe
Quoted 6 lines · Lorenzo Stoakes
Quoted 3 lines
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
-EFAULTon missing VMA and-EPERMon 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.
Quoted 6 lines
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
Quoted 7 lines · Jason Gunthorpe
Quoted 6 lines · Lorenzo Stoakes
Quoted 5 lines · Jason Gunthorpe
Quoted 4 lines · Lorenzo Stoakes
Quoted 3 lines
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.
Quoted 8 lines
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
The uprobe path thinks otherwise, but maybe the answer is that we just need to
-EFAULTon missing VMA and-EPERMon 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.
Quoted 8 lines
Quoted 4 lines
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,
Signature · 2 lines
John Hubbard NVIDIA