* [PATCH] debugfs: Check return value of debugfs_real_fops() for NULL @ 2018-03-27 23:55 Matthias Kaehlcke 2018-03-28 6:16 ` Greg Kroah-Hartman 0 siblings, 1 reply; 10+ messages in thread From: Matthias Kaehlcke @ 2018-03-27 23:55 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: linux-kernel, Josh Poimboeuf, Manoj Gupta, Matthias Kaehlcke debugfs_real_fops() returns a NULL pointer when it is invoked without a prior call to debugfs_file_get(). In code paths including this call it is not strictly necessary to check the return value of debugfs_real_fops(). However clang inlines debugfs_real_fops(), detects the invalid dereferencing of the NULL pointer and drops the code path. This leads to a bunch of objtool warnings when building with clang and CONFIG_UNWINDER_ORC=y: fs/debugfs/file.o: warning: objtool: full_proxy_llseek() falls through to next function full_proxy_read() fs/debugfs/file.o: warning: objtool: full_proxy_read() falls through to next function full_proxy_write() fs/debugfs/file.o: warning: objtool: full_proxy_write() falls through to next function full_proxy_poll() fs/debugfs/file.o: warning: objtool: full_proxy_poll() falls through to next function full_proxy_unlocked_ioctl() fs/debugfs/file.o: warning: objtool: full_proxy_unlocked_ioctl() falls through to next function fops_u8_open() Check the pointer returned by debugfs_real_fops() in all code paths to make clang and objtool happy. Debugged-by: Josh Poimboeuf <jpoimboe@redhat.com> Debugged-by: Manoj Gupta <manojgupta@chromium.org> Signed-off-by: Matthias Kaehlcke <mka@chromium.org> --- fs/debugfs/file.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c index cd12e6576b48..427e185ecc8f 100644 --- a/fs/debugfs/file.c +++ b/fs/debugfs/file.c @@ -183,7 +183,8 @@ static ret_type full_proxy_ ## name(proto) \ if (unlikely(r)) \ return r; \ real_fops = debugfs_real_fops(filp); \ - r = real_fops->name(args); \ + if (real_fops) \ + r = real_fops->name(args); \ debugfs_file_put(dentry); \ return r; \ } @@ -217,7 +218,14 @@ static unsigned int full_proxy_poll(struct file *filp, return POLLHUP; real_fops = debugfs_real_fops(filp); + if (!real_fops) { + r = -ENXIO; + goto out; + } + r = real_fops->poll(filp, wait); + +out: debugfs_file_put(dentry); return r; } -- 2.17.0.rc1.321.gba9d0f2565-goog ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] debugfs: Check return value of debugfs_real_fops() for NULL 2018-03-27 23:55 [PATCH] debugfs: Check return value of debugfs_real_fops() for NULL Matthias Kaehlcke @ 2018-03-28 6:16 ` Greg Kroah-Hartman 2018-03-28 14:47 ` Manoj Gupta 0 siblings, 1 reply; 10+ messages in thread From: Greg Kroah-Hartman @ 2018-03-28 6:16 UTC (permalink / raw) To: Matthias Kaehlcke; +Cc: linux-kernel, Josh Poimboeuf, Manoj Gupta On Tue, Mar 27, 2018 at 04:55:53PM -0700, Matthias Kaehlcke wrote: > debugfs_real_fops() returns a NULL pointer when it is invoked without a > prior call to debugfs_file_get(). In code paths including this call it > is not strictly necessary to check the return value of > debugfs_real_fops(). However clang inlines debugfs_real_fops(), detects > the invalid dereferencing of the NULL pointer and drops the code path. Wait, what? Why would it do that, because it thinks dereferencing NULL is undefined behaviour and it can just do whatever it wants to? That feels crazy, as for these calls we "know" it will never be NULL because the previous call to debugfs_file_get() will always ensure it will be correct. So this is a case of the compiler trying to be smarter than it really is, and getting things totally wrong :( Has anyone reported this to the clang developers? Papering over compiler foolishness is not something I like to do in kernel code if at all possible... thanks, greg k-h ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] debugfs: Check return value of debugfs_real_fops() for NULL 2018-03-28 6:16 ` Greg Kroah-Hartman @ 2018-03-28 14:47 ` Manoj Gupta 2018-03-28 15:30 ` Josh Poimboeuf 2018-03-28 18:05 ` Greg Kroah-Hartman 0 siblings, 2 replies; 10+ messages in thread From: Manoj Gupta @ 2018-03-28 14:47 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: Matthias Kaehlcke, linux-kernel, Josh Poimboeuf Please note that there is nothing wrong in the generated code, just that it confuses objtool. Clang has simply omitted the statement where NULL is returned since the pointer was always dereferenced post inlining. Note that GCC will also remove the NULL pointers if it knows that the pointer is dereferenced. Here is an example. void null_check(int *P) { int deref = *P; if (P == 0) // GCC won't check the condition. return; *P = 4; } Compiling with gcc -O2 gives: movl $4, (%rdi) ret Thanks, Manoj On Tue, Mar 27, 2018 at 11:16 PM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Tue, Mar 27, 2018 at 04:55:53PM -0700, Matthias Kaehlcke wrote: >> debugfs_real_fops() returns a NULL pointer when it is invoked without a >> prior call to debugfs_file_get(). In code paths including this call it >> is not strictly necessary to check the return value of >> debugfs_real_fops(). However clang inlines debugfs_real_fops(), detects >> the invalid dereferencing of the NULL pointer and drops the code path. > > Wait, what? Why would it do that, because it thinks dereferencing NULL > is undefined behaviour and it can just do whatever it wants to? > > That feels crazy, as for these calls we "know" it will never be NULL > because the previous call to debugfs_file_get() will always ensure it > will be correct. > > So this is a case of the compiler trying to be smarter than it really > is, and getting things totally wrong :( > > Has anyone reported this to the clang developers? > > Papering over compiler foolishness is not something I like to do in > kernel code if at all possible... > > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] debugfs: Check return value of debugfs_real_fops() for NULL 2018-03-28 14:47 ` Manoj Gupta @ 2018-03-28 15:30 ` Josh Poimboeuf 2018-03-28 15:34 ` Josh Poimboeuf 2018-03-28 18:05 ` Greg Kroah-Hartman 1 sibling, 1 reply; 10+ messages in thread From: Josh Poimboeuf @ 2018-03-28 15:30 UTC (permalink / raw) To: Manoj Gupta; +Cc: Greg Kroah-Hartman, Matthias Kaehlcke, linux-kernel On Wed, Mar 28, 2018 at 07:47:53AM -0700, Manoj Gupta wrote: > Please note that there is nothing wrong in the generated code, just > that it confuses objtool. > Clang has simply omitted the statement where NULL is returned since > the pointer was always dereferenced post inlining. ... but returning NULL would be far more sane than falling through to the next function. > Note that GCC will also remove the NULL pointers if it knows that the > pointer is dereferenced. > Here is an example. > > void null_check(int *P) { > int deref = *P; > if (P == 0) // GCC won't check the condition. > return; > *P = 4; > } > > Compiling with gcc -O2 gives: > movl $4, (%rdi) > ret This is why we use -fno-delete-null-pointer-checks. -- Josh ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] debugfs: Check return value of debugfs_real_fops() for NULL 2018-03-28 15:30 ` Josh Poimboeuf @ 2018-03-28 15:34 ` Josh Poimboeuf 0 siblings, 0 replies; 10+ messages in thread From: Josh Poimboeuf @ 2018-03-28 15:34 UTC (permalink / raw) To: Manoj Gupta; +Cc: Greg Kroah-Hartman, Matthias Kaehlcke, linux-kernel On Wed, Mar 28, 2018 at 10:30:51AM -0500, Josh Poimboeuf wrote: > On Wed, Mar 28, 2018 at 07:47:53AM -0700, Manoj Gupta wrote: > > Please note that there is nothing wrong in the generated code, just > > that it confuses objtool. > > Clang has simply omitted the statement where NULL is returned since > > the pointer was always dereferenced post inlining. > > ... but returning NULL would be far more sane than falling through to > the next function. Or, as the case may be, oopsing at the point of failure. > > Note that GCC will also remove the NULL pointers if it knows that the > > pointer is dereferenced. > > Here is an example. > > > > void null_check(int *P) { > > int deref = *P; > > if (P == 0) // GCC won't check the condition. > > return; > > *P = 4; > > } > > > > Compiling with gcc -O2 gives: > > movl $4, (%rdi) > > ret > > This is why we use -fno-delete-null-pointer-checks. -- Josh ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] debugfs: Check return value of debugfs_real_fops() for NULL 2018-03-28 14:47 ` Manoj Gupta 2018-03-28 15:30 ` Josh Poimboeuf @ 2018-03-28 18:05 ` Greg Kroah-Hartman 2018-03-28 18:14 ` Matthias Kaehlcke 1 sibling, 1 reply; 10+ messages in thread From: Greg Kroah-Hartman @ 2018-03-28 18:05 UTC (permalink / raw) To: Manoj Gupta; +Cc: Matthias Kaehlcke, linux-kernel, Josh Poimboeuf A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? A: Top-posting. Q: What is the most annoying thing in e-mail? A: No. Q: Should I include quotations after my reply? http://daringfireball.net/2007/07/on_top On Wed, Mar 28, 2018 at 07:47:53AM -0700, Manoj Gupta wrote: > Please note that there is nothing wrong in the generated code, just > that it confuses objtool. Then fix the tool, the C code is correct :) > Clang has simply omitted the statement where NULL is returned since > the pointer was always dereferenced post inlining. Then tell clang not to do that, like we tell gcc not to do that as that is a foolish thing for a compiler to do when building the kernel. thanks, greg k-h ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] debugfs: Check return value of debugfs_real_fops() for NULL 2018-03-28 18:05 ` Greg Kroah-Hartman @ 2018-03-28 18:14 ` Matthias Kaehlcke 2018-03-28 18:19 ` Greg Kroah-Hartman 0 siblings, 1 reply; 10+ messages in thread From: Matthias Kaehlcke @ 2018-03-28 18:14 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: Manoj Gupta, linux-kernel, Josh Poimboeuf El Wed, Mar 28, 2018 at 08:05:56PM +0200 Greg Kroah-Hartman ha dit: > > A: Because it messes up the order in which people normally read text. > Q: Why is top-posting such a bad thing? > A: Top-posting. > Q: What is the most annoying thing in e-mail? > > A: No. > Q: Should I include quotations after my reply? > > http://daringfireball.net/2007/07/on_top > > On Wed, Mar 28, 2018 at 07:47:53AM -0700, Manoj Gupta wrote: > > Please note that there is nothing wrong in the generated code, just > > that it confuses objtool. > > Then fix the tool, the C code is correct :) > > > Clang has simply omitted the statement where NULL is returned since > > the pointer was always dereferenced post inlining. > > Then tell clang not to do that, like we tell gcc not to do that as that > is a foolish thing for a compiler to do when building the kernel. Thanks all for your input, we'll try to get -fno-delete-null-pointer-checks or a similar flag to be added to clang. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] debugfs: Check return value of debugfs_real_fops() for NULL 2018-03-28 18:14 ` Matthias Kaehlcke @ 2018-03-28 18:19 ` Greg Kroah-Hartman 2018-03-28 18:50 ` Matthias Kaehlcke 0 siblings, 1 reply; 10+ messages in thread From: Greg Kroah-Hartman @ 2018-03-28 18:19 UTC (permalink / raw) To: Matthias Kaehlcke; +Cc: Manoj Gupta, linux-kernel, Josh Poimboeuf On Wed, Mar 28, 2018 at 11:14:56AM -0700, Matthias Kaehlcke wrote: > El Wed, Mar 28, 2018 at 08:05:56PM +0200 Greg Kroah-Hartman ha dit: > > > > > A: Because it messes up the order in which people normally read text. > > Q: Why is top-posting such a bad thing? > > A: Top-posting. > > Q: What is the most annoying thing in e-mail? > > > > A: No. > > Q: Should I include quotations after my reply? > > > > http://daringfireball.net/2007/07/on_top > > > > On Wed, Mar 28, 2018 at 07:47:53AM -0700, Manoj Gupta wrote: > > > Please note that there is nothing wrong in the generated code, just > > > that it confuses objtool. > > > > Then fix the tool, the C code is correct :) > > > > > Clang has simply omitted the statement where NULL is returned since > > > the pointer was always dereferenced post inlining. > > > > Then tell clang not to do that, like we tell gcc not to do that as that > > is a foolish thing for a compiler to do when building the kernel. > > Thanks all for your input, we'll try to get > -fno-delete-null-pointer-checks or a similar flag to be added to > clang. Wait, clang does not have that? That's crazy, how has this not been hit yet when building the kernel? confused, greg k-h ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] debugfs: Check return value of debugfs_real_fops() for NULL 2018-03-28 18:19 ` Greg Kroah-Hartman @ 2018-03-28 18:50 ` Matthias Kaehlcke 2018-03-28 19:22 ` Greg Kroah-Hartman 0 siblings, 1 reply; 10+ messages in thread From: Matthias Kaehlcke @ 2018-03-28 18:50 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Manoj Gupta, linux-kernel, Josh Poimboeuf, Alexander Potapenko El Wed, Mar 28, 2018 at 08:19:36PM +0200 Greg Kroah-Hartman ha dit: > On Wed, Mar 28, 2018 at 11:14:56AM -0700, Matthias Kaehlcke wrote: > > El Wed, Mar 28, 2018 at 08:05:56PM +0200 Greg Kroah-Hartman ha dit: > > > > > > > > A: Because it messes up the order in which people normally read text. > > > Q: Why is top-posting such a bad thing? > > > A: Top-posting. > > > Q: What is the most annoying thing in e-mail? > > > > > > A: No. > > > Q: Should I include quotations after my reply? > > > > > > http://daringfireball.net/2007/07/on_top > > > > > > On Wed, Mar 28, 2018 at 07:47:53AM -0700, Manoj Gupta wrote: > > > > Please note that there is nothing wrong in the generated code, just > > > > that it confuses objtool. > > > > > > Then fix the tool, the C code is correct :) > > > > > > > Clang has simply omitted the statement where NULL is returned since > > > > the pointer was always dereferenced post inlining. > > > > > > Then tell clang not to do that, like we tell gcc not to do that as that > > > is a foolish thing for a compiler to do when building the kernel. > > > > Thanks all for your input, we'll try to get > > -fno-delete-null-pointer-checks or a similar flag to be added to > > clang. > > Wait, clang does not have that? Nope, clang doesn't currently have such a flag. > That's crazy, how has this not been hit yet when building the > kernel? IIRC this patch was needed to work around the lack of the flag: commit beaec533fc2701a28a4d667f67c9f59c6e4e0d13 Author: Alexander Potapenko <glider@google.com> Date: Wed Jul 19 20:27:30 2017 +0200 llist: clang: introduce member_address_is_nonnull() Currently llist_for_each_entry() and llist_for_each_entry_safe() iterate until &pos->member != NULL. But when building the kernel with Clang, the compiler assumes &pos->member cannot be NULL if the member's offset is greater than 0 (which would be equivalent to the object being non-contiguous in memory). Therefore the loop condition is always true, and the loops become infinite. To work around this, introduce the member_address_is_nonnull() macro, which casts object pointer to uintptr_t, thus letting the member pointer to be NULL. Signed-off-by: Alexander Potapenko <glider@google.com> Tested-by: Sodagudi Prasad <psodagud@codeaurora.org> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> Other than that I am not aware of any known issues. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] debugfs: Check return value of debugfs_real_fops() for NULL 2018-03-28 18:50 ` Matthias Kaehlcke @ 2018-03-28 19:22 ` Greg Kroah-Hartman 0 siblings, 0 replies; 10+ messages in thread From: Greg Kroah-Hartman @ 2018-03-28 19:22 UTC (permalink / raw) To: Matthias Kaehlcke Cc: Manoj Gupta, linux-kernel, Josh Poimboeuf, Alexander Potapenko On Wed, Mar 28, 2018 at 11:50:09AM -0700, Matthias Kaehlcke wrote: > El Wed, Mar 28, 2018 at 08:19:36PM +0200 Greg Kroah-Hartman ha dit: > > > On Wed, Mar 28, 2018 at 11:14:56AM -0700, Matthias Kaehlcke wrote: > > > El Wed, Mar 28, 2018 at 08:05:56PM +0200 Greg Kroah-Hartman ha dit: > > > > > > > > > > > A: Because it messes up the order in which people normally read text. > > > > Q: Why is top-posting such a bad thing? > > > > A: Top-posting. > > > > Q: What is the most annoying thing in e-mail? > > > > > > > > A: No. > > > > Q: Should I include quotations after my reply? > > > > > > > > http://daringfireball.net/2007/07/on_top > > > > > > > > On Wed, Mar 28, 2018 at 07:47:53AM -0700, Manoj Gupta wrote: > > > > > Please note that there is nothing wrong in the generated code, just > > > > > that it confuses objtool. > > > > > > > > Then fix the tool, the C code is correct :) > > > > > > > > > Clang has simply omitted the statement where NULL is returned since > > > > > the pointer was always dereferenced post inlining. > > > > > > > > Then tell clang not to do that, like we tell gcc not to do that as that > > > > is a foolish thing for a compiler to do when building the kernel. > > > > > > Thanks all for your input, we'll try to get > > > -fno-delete-null-pointer-checks or a similar flag to be added to > > > clang. > > > > Wait, clang does not have that? > > Nope, clang doesn't currently have such a flag. > > > That's crazy, how has this not been hit yet when building the > > kernel? > > IIRC this patch was needed to work around the lack of the flag: > > commit beaec533fc2701a28a4d667f67c9f59c6e4e0d13 > Author: Alexander Potapenko <glider@google.com> > Date: Wed Jul 19 20:27:30 2017 +0200 > > llist: clang: introduce member_address_is_nonnull() > > Currently llist_for_each_entry() and llist_for_each_entry_safe() iterate > until &pos->member != NULL. But when building the kernel with Clang, > the compiler assumes &pos->member cannot be NULL if the member's offset > is greater than 0 (which would be equivalent to the object being > non-contiguous in memory). Therefore the loop condition is always true, > and the loops become infinite. > > To work around this, introduce the member_address_is_nonnull() macro, > which casts object pointer to uintptr_t, thus letting the member pointer > to be NULL. > > Signed-off-by: Alexander Potapenko <glider@google.com> > Tested-by: Sodagudi Prasad <psodagud@codeaurora.org> > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> > > > Other than that I am not aware of any known issues. I think you have gotten lucky :) greg k-h ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-03-28 19:22 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-03-27 23:55 [PATCH] debugfs: Check return value of debugfs_real_fops() for NULL Matthias Kaehlcke 2018-03-28 6:16 ` Greg Kroah-Hartman 2018-03-28 14:47 ` Manoj Gupta 2018-03-28 15:30 ` Josh Poimboeuf 2018-03-28 15:34 ` Josh Poimboeuf 2018-03-28 18:05 ` Greg Kroah-Hartman 2018-03-28 18:14 ` Matthias Kaehlcke 2018-03-28 18:19 ` Greg Kroah-Hartman 2018-03-28 18:50 ` Matthias Kaehlcke 2018-03-28 19:22 ` Greg Kroah-Hartman
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.