[PATCH] debugfs: Check return value of debugfs_real_fops() for NULL

[PATCH] debugfs: Check return value of debugfs_real_fops() for NULL

[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

Re: [PATCH] debugfs: Check return value of debugfs_real_fops() for NULL

[Greg Kroah-Hartman]

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

Re: [PATCH] debugfs: Check return value of debugfs_real_fops() for NULL

[Manoj Gupta]

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

Re: [PATCH] debugfs: Check return value of debugfs_real_fops() for NULL

[Josh Poimboeuf]

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

Re: [PATCH] debugfs: Check return value of debugfs_real_fops() for NULL

[Josh Poimboeuf]

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

Re: [PATCH] debugfs: Check return value of debugfs_real_fops() for NULL

[Greg Kroah-Hartman]

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

Re: [PATCH] debugfs: Check return value of debugfs_real_fops() for NULL

[Matthias Kaehlcke]

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.

Re: [PATCH] debugfs: Check return value of debugfs_real_fops() for NULL

[Greg Kroah-Hartman]

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

Re: [PATCH] debugfs: Check return value of debugfs_real_fops() for NULL

[Matthias Kaehlcke]

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.

Re: [PATCH] debugfs: Check return value of debugfs_real_fops() for NULL

[Greg Kroah-Hartman]

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