[PATCH] afs: fix no return statement in function returning non-void

[PATCH] afs: fix no return statement in function returning non-void

[David Howells]

From: Zheng Zengkai <zhengzengkai@huawei.com>

Add missing return to fix following compilation issue:

fs/afs/dir.c: In function ‘afs_dir_set_page_dirty’:
fs/afs/dir.c:51:1: error: no return statement in function
returning non-void [-Werror=return-type]

Fixes: f3ddee8dc4e2 ("afs: Fix directory handling")
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Zheng Zengkai <zhengzengkai@huawei.com>
Acked-by: Randy Dunlap <rdunlap@infradead.org>
cc: Tom Rix <trix@redhat.com>
cc: linux-afs@lists.infradead.org
Link: https://lore.kernel.org/r/20210327121624.194639-1-zhengzengkai@huawei.com/
---

 fs/afs/dir.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/afs/dir.c b/fs/afs/dir.c
index 78719f2f567e..c31c21afd2e1 100644
--- a/fs/afs/dir.c
+++ b/fs/afs/dir.c
@@ -48,6 +48,7 @@ static void afs_dir_invalidatepage(struct page *page, unsigned int offset,
 static int afs_dir_set_page_dirty(struct page *page)
 {
 	BUG(); /* This should never happen. */
+	return 0;
 }
 
 const struct file_operations afs_dir_file_operations = {

Re: [PATCH] afs: fix no return statement in function returning non-void

[David Howells]

Hi Linus,

Note that this isn't really a fix, so can wait to the next merge window if you
prefer.

David

Re: [PATCH] afs: fix no return statement in function returning non-void

[Linus Torvalds]

On Tue, Jun 15, 2021 at 4:55 AM David Howells <dhowells@redhat.com> wrote:
>
> From: Zheng Zengkai <zhengzengkai@huawei.com>
>
> Add missing return to fix following compilation issue:
>
> fs/afs/dir.c: In function ‘afs_dir_set_page_dirty’:
> fs/afs/dir.c:51:1: error: no return statement in function
> returning non-void [-Werror=return-type]

This warning is actively wrong, and the patch is the wrong thing to do.

What compiler / architecture / config?

Because BUG() should have an "unreachable()", and the compiler should
know that a return statement isn't needed (and adding it shouldn't
make any difference).

And it's not warning for me when I build that code. So I really think
the real bug is entirely somewhere else, and this patch is papering
over the real problem.

               Linus

Re: [PATCH] afs: fix no return statement in function returning non-void

[Randy Dunlap]

On 6/15/21 7:49 AM, Linus Torvalds wrote:
> On Tue, Jun 15, 2021 at 4:55 AM David Howells <dhowells@redhat.com> wrote:
>>
>> From: Zheng Zengkai <zhengzengkai@huawei.com>
>>
>> Add missing return to fix following compilation issue:
>>
>> fs/afs/dir.c: In function ‘afs_dir_set_page_dirty’:
>> fs/afs/dir.c:51:1: error: no return statement in function
>> returning non-void [-Werror=return-type]
> 
> This warning is actively wrong, and the patch is the wrong thing to do.
> 
> What compiler / architecture / config?
> 
> Because BUG() should have an "unreachable()", and the compiler should
> know that a return statement isn't needed (and adding it shouldn't
> make any difference).
> 
> And it's not warning for me when I build that code. So I really think
> the real bug is entirely somewhere else, and this patch is papering
> over the real problem.

Hi,

Some implementations of BUG() are macros, not functions, so "unreachable"
is not applicable AFAIK.


-- 
~Randy

Re: [PATCH] afs: fix no return statement in function returning non-void

[Linus Torvalds]

On Tue, Jun 15, 2021 at 4:58 PM Randy Dunlap <rdunlap@infradead.org> wrote:
>
> Some implementations of BUG() are macros, not functions,

Not "some", I think. Most.

> so "unreachable" is not applicable AFAIK.

Sure it is. One common pattern is the x86 one:

  #define BUG()                                                   \
  do {                                                            \
          instrumentation_begin();                                \
          _BUG_FLAGS(ASM_UD2, 0);                                 \
          unreachable();                                          \
  } while (0)

and that "unreachable()" is exactly what I'm talking about.

So I repeat: what completely broken compiler / config / architecture
is it that needs that "return 0" after a BUG() statement?

Because that environment is broken, and the warning is bogus and wrong.

It might not be the compiler. It might be some architecture that does
this wrong. It might be some very particular configuration that does
something bad and makes the "unreachable()" not work (or not exist).

But *that* is the bug that should be fixed. Not adding a pointless and
incorrect line that makes no sense, just to hide the real bug.

               Linus

                 Linus

Re: [PATCH] afs: fix no return statement in function returning non-void

[Randy Dunlap]

On 6/15/21 5:32 PM, Linus Torvalds wrote:
> On Tue, Jun 15, 2021 at 4:58 PM Randy Dunlap <rdunlap@infradead.org> wrote:
>>
>> Some implementations of BUG() are macros, not functions,
> 
> Not "some", I think. Most.
> 
>> so "unreachable" is not applicable AFAIK.
> 
> Sure it is. One common pattern is the x86 one:
> 
>   #define BUG()                                                   \
>   do {                                                            \
>           instrumentation_begin();                                \
>           _BUG_FLAGS(ASM_UD2, 0);                                 \
>           unreachable();                                          \
>   } while (0)

duh.

> and that "unreachable()" is exactly what I'm talking about.
> 
> So I repeat: what completely broken compiler / config / architecture
> is it that needs that "return 0" after a BUG() statement?

I have seen it on ia64 -- most likely GCC 9.3.0, but I'll have to
double check that.

> Because that environment is broken, and the warning is bogus and wrong.
> 
> It might not be the compiler. It might be some architecture that does
> this wrong. It might be some very particular configuration that does
> something bad and makes the "unreachable()" not work (or not exist).
> 
> But *that* is the bug that should be fixed. Not adding a pointless and
> incorrect line that makes no sense, just to hide the real bug.

Re: [PATCH] afs: fix no return statement in function returning non-void

[Randy Dunlap]

On 6/15/21 6:38 PM, Randy Dunlap wrote:
> On 6/15/21 5:32 PM, Linus Torvalds wrote:
>> On Tue, Jun 15, 2021 at 4:58 PM Randy Dunlap <rdunlap@infradead.org> wrote:
>>>
>>> Some implementations of BUG() are macros, not functions,
>>
>> Not "some", I think. Most.
>>
>>> so "unreachable" is not applicable AFAIK.
>>
>> Sure it is. One common pattern is the x86 one:
>>
>>   #define BUG()                                                   \
>>   do {                                                            \
>>           instrumentation_begin();                                \
>>           _BUG_FLAGS(ASM_UD2, 0);                                 \
>>           unreachable();                                          \
>>   } while (0)
> 
> duh.
> 
>> and that "unreachable()" is exactly what I'm talking about.
>>
>> So I repeat: what completely broken compiler / config / architecture
>> is it that needs that "return 0" after a BUG() statement?
> 
> I have seen it on ia64 -- most likely GCC 9.3.0, but I'll have to
> double check that.

Nope, I cannot repro that now. I'll check a few more arches...

>> Because that environment is broken, and the warning is bogus and wrong.
>>
>> It might not be the compiler. It might be some architecture that does
>> this wrong. It might be some very particular configuration that does
>> something bad and makes the "unreachable()" not work (or not exist).
>>
>> But *that* is the bug that should be fixed. Not adding a pointless and
>> incorrect line that makes no sense, just to hide the real bug.


-- 
~Randy

Re: [PATCH] afs: fix no return statement in function returning non-void

[Zheng Zengkai]

Oops, Sorry for the late reply and missing the compilation details.

> On 6/15/21 5:32 PM, Linus Torvalds wrote:
>> On Tue, Jun 15, 2021 at 4:58 PM Randy Dunlap <rdunlap@infradead.org> wrote:
>>> Some implementations of BUG() are macros, not functions,
>> Not "some", I think. Most.
>>
>>> so "unreachable" is not applicable AFAIK.
>> Sure it is. One common pattern is the x86 one:
>>
>>    #define BUG()                                                   \
>>    do {                                                            \
>>            instrumentation_begin();                                \
>>            _BUG_FLAGS(ASM_UD2, 0);                                 \
>>            unreachable();                                          \
>>    } while (0)
> duh.
>
>> and that "unreachable()" is exactly what I'm talking about.
>>
>> So I repeat: what completely broken compiler / config / architecture
>> is it that needs that "return 0" after a BUG() statement?
> I have seen it on ia64 -- most likely GCC 9.3.0, but I'll have to
> double check that.

Actually we build the kernel with the following compiler, config and 
architecture :

powerpc64-linux-gnu-gcc --version
powerpc64-linux-gnu-gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

CONFIG_AFS_FS=y
# CONFIG_AFS_DEBUG is not set
CONFIG_AFS_DEBUG_CURSOR=y

make ARCH=powerpc CROSS_COMPILE=powerpc64-linux-gnu- -j64

...

fs/afs/dir.c: In function ‘afs_dir_set_page_dirty’:
fs/afs/dir.c:51:1: error: no return statement in function returning 
non-void [-Werror=return-type]
    51 | }
       | ^
cc1: some warnings being treated as errors

>> Because that environment is broken, and the warning is bogus and wrong.
>>
>> It might not be the compiler. It might be some architecture that does
>> this wrong. It might be some very particular configuration that does
>> something bad and makes the "unreachable()" not work (or not exist).
>>
>> But *that* is the bug that should be fixed. Not adding a pointless and
>> incorrect line that makes no sense, just to hide the real bug.
> .
>

Re: [PATCH] afs: fix no return statement in function returning non-void

[Tom Rix]

On 6/15/21 8:15 PM, Zheng Zengkai wrote:
> Oops, Sorry for the late reply and missing the compilation details.
>
>> On 6/15/21 5:32 PM, Linus Torvalds wrote:
>>> On Tue, Jun 15, 2021 at 4:58 PM Randy Dunlap <rdunlap@infradead.org> 
>>> wrote:
>>>> Some implementations of BUG() are macros, not functions,
>>> Not "some", I think. Most.
>>>
>>>> so "unreachable" is not applicable AFAIK.
>>> Sure it is. One common pattern is the x86 one:
>>>
>>>    #define BUG()                                                   \
>>>    do {                                                            \
>>> instrumentation_begin();                                \
>>>            _BUG_FLAGS(ASM_UD2, 0);                                 \
>>> unreachable();                                          \
>>>    } while (0)
>> duh.
>>
>>> and that "unreachable()" is exactly what I'm talking about.
>>>
>>> So I repeat: what completely broken compiler / config / architecture
>>> is it that needs that "return 0" after a BUG() statement?
>> I have seen it on ia64 -- most likely GCC 9.3.0, but I'll have to
>> double check that.
>
> Actually we build the kernel with the following compiler, config and 
> architecture :
>
> powerpc64-linux-gnu-gcc --version
> powerpc64-linux-gnu-gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0
> Copyright (C) 2019 Free Software Foundation, Inc.
> This is free software; see the source for copying conditions. There is NO
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR 
> PURPOSE.
>
> CONFIG_AFS_FS=y
> # CONFIG_AFS_DEBUG is not set
> CONFIG_AFS_DEBUG_CURSOR=y
>
> make ARCH=powerpc CROSS_COMPILE=powerpc64-linux-gnu- -j64
>
> ...
>
> fs/afs/dir.c: In function ‘afs_dir_set_page_dirty’:
> fs/afs/dir.c:51:1: error: no return statement in function returning 
> non-void [-Werror=return-type]
>    51 | }
>       | ^
> cc1: some warnings being treated as errors
>
powerpc64 gcc 10.3.1 is what I used to find this problem.

A fix is to use the __noreturn attribute on this function and not add a 
return like this

-static int afs_dir_set_page_dirty(struct page *page)
+static int __noreturn afs_dir_set_page_dirty(struct page *page)

and to the set of ~300 similar functions in these files.

$ grep -r -P "^\tBUG\(\)" .

If folks are ok with this, I'll get that set started.

Tom

>>> Because that environment is broken, and the warning is bogus and wrong.
>>>
>>> It might not be the compiler. It might be some architecture that does
>>> this wrong. It might be some very particular configuration that does
>>> something bad and makes the "unreachable()" not work (or not exist).
>>>
>>> But *that* is the bug that should be fixed. Not adding a pointless and
>>> incorrect line that makes no sense, just to hide the real bug.
>> .
>>
>

Re: [PATCH] afs: fix no return statement in function returning non-void

[Linus Torvalds]

On Wed, Jun 16, 2021 at 5:56 AM Tom Rix <trix@redhat.com> wrote:
>
> A fix is to use the __noreturn attribute on this function

That's certainly a better thing. It would be better yet to figure out
why BUG() didn't do it automatically.

Without CONFIG_BUG, it looks like powerpc picks up

  #ifndef HAVE_ARCH_BUG
  #define BUG() do {} while (1)

which should still make it pointless to have the return.  But I might
have missed something.

             Linus

Re: [PATCH] afs: fix no return statement in function returning non-void

[Tom Rix]

On 6/16/21 7:34 AM, Linus Torvalds wrote:
> On Wed, Jun 16, 2021 at 5:56 AM Tom Rix <trix@redhat.com> wrote:
>> A fix is to use the __noreturn attribute on this function
> That's certainly a better thing. It would be better yet to figure out
> why BUG() didn't do it automatically.
>
> Without CONFIG_BUG, it looks like powerpc picks up
>
>    #ifndef HAVE_ARCH_BUG
>    #define BUG() do {} while (1)
>
> which should still make it pointless to have the return.  But I might
> have missed something.

This looks like a problem the generic BUG().

with CONFIG_BUG=y, the *.i is

static int afs_dir_set_page_dirty(struct page *page)
{
  do { __asm__ __volatile__( "1:    " "twi 31, 0, 0" "\n" ".section 
__bug_table,\"aw\"\n" "2:\t.4byte 1b - 2b, %0 - 2b\n" "\t.short %1, 
%2\n" ".org 2b+%3\n" ".previous\n" : : "i" ("fs/afs/dir.c"), "i" (50), 
"i" (0), "i" (sizeof(struct bug_entry))); do { ; asm volatile(""); 
__builtin_unreachable(); } while (0); } while (0);
}
BUG() expanded from
#define BUG() do {                        \
     BUG_ENTRY("twi 31, 0, 0", 0);                \
     unreachable();                        \
} while (0)


with CONFIG_BUG=n, the *.i is

static int afs_dir_set_page_dirty(struct page *page)
{
  do {} while (1);
}

BUG() expanded from
  do {} while (1)

to fix, add an unreachable() to the generic BUG()

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index f152b9bb916f..b250e06d7de2 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -177,7 +177,10 @@ void __warn(const char *file, int line, void 
*caller, unsigned taint,

  #else /* !CONFIG_BUG */
  #ifndef HAVE_ARCH_BUG
-#define BUG() do {} while (1)
+#define BUG() do {                                             \
+               do {} while (1);                                \
+               unreachable();                                  \
+       } while (0)
  #endif

the new *.i

static int afs_dir_set_page_dirty(struct page *page)
{
  do { do {} while (1); do { ; asm volatile(""); 
__builtin_unreachable(); } while (0); } while (0);
}

The assembly is unchanged.

The key being the unreachable builtin

ref: gcc docs https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html

" ... without the __builtin_unreachable, GCC issues a
   warning that control reaches the end of a non-void function."

Tom

>
>               Linus
>

Re: [PATCH] afs: fix no return statement in function returning non-void

[Linus Torvalds]

On Wed, Jun 16, 2021 at 9:22 AM Tom Rix <trix@redhat.com> wrote:
>
> to fix, add an unreachable() to the generic BUG()
>
> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
> index f152b9bb916f..b250e06d7de2 100644
> --- a/include/asm-generic/bug.h
> +++ b/include/asm-generic/bug.h
> @@ -177,7 +177,10 @@ void __warn(const char *file, int line, void
> *caller, unsigned taint,
>
>   #else /* !CONFIG_BUG */
>   #ifndef HAVE_ARCH_BUG
> -#define BUG() do {} while (1)
> +#define BUG() do {                                             \
> +               do {} while (1);                                \
> +               unreachable();                                  \
> +       } while (0)
>   #endif

I'm a bit surprised that the compiler doesn't make that code after an
infinite loop automatically be marked "unreachable". But at the same I
can imagine the compiler doing some checks without doing real flow
analysis, and doing "oh, that conditional branch is unconditional".

So this patch at least makes sense to me and I have no objections to
it, even if it makes me go "silly compiler, we shouldn't have to tell
you this".

So Ack from me on this.

           Linus

Re: [PATCH] afs: fix no return statement in function returning non-void

[Arnd Bergmann]

On Thu, Jun 17, 2021 at 12:51 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Wed, Jun 16, 2021 at 9:22 AM Tom Rix <trix@redhat.com> wrote:
> >
> > to fix, add an unreachable() to the generic BUG()
> >
> > diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
> > index f152b9bb916f..b250e06d7de2 100644
> > --- a/include/asm-generic/bug.h
> > +++ b/include/asm-generic/bug.h
> > @@ -177,7 +177,10 @@ void __warn(const char *file, int line, void
> > *caller, unsigned taint,
> >
> >   #else /* !CONFIG_BUG */
> >   #ifndef HAVE_ARCH_BUG
> > -#define BUG() do {} while (1)
> > +#define BUG() do {                                             \
> > +               do {} while (1);                                \
> > +               unreachable();                                  \
> > +       } while (0)
> >   #endif
>
> I'm a bit surprised that the compiler doesn't make that code after an
> infinite loop automatically be marked "unreachable". But at the same I
> can imagine the compiler doing some checks without doing real flow
> analysis, and doing "oh, that conditional branch is unconditional".
>
> So this patch at least makes sense to me and I have no objections to
> it, even if it makes me go "silly compiler, we shouldn't have to tell
> you this".
>
> So Ack from me on this.

I've tried to figure out what the compiler is trying to do here, and it's
still weird. When I saw the patch posted, I misread it as having just
unreachable() without the loop, and that would have been bad
because it triggers undefined behavior.

What I found is a minimal test case of

static int f(void)
{
   do {} while (1);
}

to trigger the warning with any version of gcc (not clang), but none of
these other variations cause a warning:

 // not static -> no warning!
int f(void)
{
   do {} while (1);
}

// some return statement anywhere in the function, no warning
static int f(int i)
{
  if (i)
      return 0;
   do {} while (1);
}

// annotated as never returning, as discussed in this thread
static int __attribute__((noreturn)) f(void)
{
   do {} while (1);
}

// unreachable annotation, as suggested by Tom
static int f(void)
{
   do {} while (1);
   __builtin_unreachable();
}

The last three are obviously intentional, as the warning is only for functions
that can *never* return but lack an annotation. I have no idea why the warning
is only for static functions though.

All my randconfig builds for arm/arm64/x86 missed this problem since those
architectures have a custom BUG() implementation with an inline asm.
I've taken them out now and found only two other instances of the issue so far:
arbitrary_virt_to_machine() and ppc64 get_hugepd_cache_index(). My preference
would be to annotate these as __noreturn, but change to the asm-generic
BUG() is probably better.

        Arnd

Re: [PATCH] afs: fix no return statement in function returning non-void

[David Howells]

Tom Rix <trix@redhat.com> wrote:

> A fix is to use the __noreturn attribute on this function and not add a return
> like this
> 
> -static int afs_dir_set_page_dirty(struct page *page)
> +static int __noreturn afs_dir_set_page_dirty(struct page *page)
> 
> and to the set of ~300 similar functions in these files.

BUG() really ought to handle it.  Do you have CONFIG_BUG=y?

David