[PATCH] kcsan: fix debugfs initcall return type

[PATCH] kcsan: fix debugfs initcall return type

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

clang points out that an initcall funciton should return an 'int':

kernel/kcsan/debugfs.c:274:15: error: returning 'void' from a function with incompatible result type 'int'
late_initcall(kcsan_debugfs_init);
~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
include/linux/init.h:292:46: note: expanded from macro 'late_initcall'
 #define late_initcall(fn)               __define_initcall(fn, 7)

Fixes: e36299efe7d7 ("kcsan, debugfs: Move debugfs file creation out of early init")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 kernel/kcsan/debugfs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/kcsan/debugfs.c b/kernel/kcsan/debugfs.c
index c1dd02f3be8b..e65de172ccf7 100644
--- a/kernel/kcsan/debugfs.c
+++ b/kernel/kcsan/debugfs.c
@@ -266,9 +266,10 @@ static const struct file_operations debugfs_ops =
 	.release = single_release
 };
 
-static void __init kcsan_debugfs_init(void)
+static int __init kcsan_debugfs_init(void)
 {
 	debugfs_create_file("kcsan", 0644, NULL, NULL, &debugfs_ops);
+	return 0;
 }
 
 late_initcall(kcsan_debugfs_init);
-- 
2.29.2

Re: [PATCH] kcsan: fix debugfs initcall return type

[Greg Kroah-Hartman]

On Fri, May 14, 2021 at 04:00:08PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> clang points out that an initcall funciton should return an 'int':
> 
> kernel/kcsan/debugfs.c:274:15: error: returning 'void' from a function with incompatible result type 'int'
> late_initcall(kcsan_debugfs_init);
> ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
> include/linux/init.h:292:46: note: expanded from macro 'late_initcall'
>  #define late_initcall(fn)               __define_initcall(fn, 7)
> 
> Fixes: e36299efe7d7 ("kcsan, debugfs: Move debugfs file creation out of early init")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  kernel/kcsan/debugfs.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/kcsan/debugfs.c b/kernel/kcsan/debugfs.c
> index c1dd02f3be8b..e65de172ccf7 100644
> --- a/kernel/kcsan/debugfs.c
> +++ b/kernel/kcsan/debugfs.c
> @@ -266,9 +266,10 @@ static const struct file_operations debugfs_ops =
>  	.release = single_release
>  };
>  
> -static void __init kcsan_debugfs_init(void)
> +static int __init kcsan_debugfs_init(void)
>  {
>  	debugfs_create_file("kcsan", 0644, NULL, NULL, &debugfs_ops);
> +	return 0;
>  }
>  
>  late_initcall(kcsan_debugfs_init);
> -- 
> 2.29.2
> 
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH] kcsan: fix debugfs initcall return type

[Marco Elver]

On Fri, 14 May 2021 at 16:10, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Fri, May 14, 2021 at 04:00:08PM +0200, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > clang points out that an initcall funciton should return an 'int':
> >
> > kernel/kcsan/debugfs.c:274:15: error: returning 'void' from a function with incompatible result type 'int'
> > late_initcall(kcsan_debugfs_init);
> > ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
> > include/linux/init.h:292:46: note: expanded from macro 'late_initcall'
> >  #define late_initcall(fn)               __define_initcall(fn, 7)
> >
> > Fixes: e36299efe7d7 ("kcsan, debugfs: Move debugfs file creation out of early init")
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
[...]
> >
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Reviewed-by: Marco Elver <elver@google.com>

Thanks for catching this -- it boggles my mind why gcc nor clang
wouldn't warn about this by default...
Is this a new clang?

Paul, please also add a "Cc: stable <stable@vger.kernel.org>" because
e36299efe7d7 is, too.

Thanks,
-- Marco

Re: [PATCH] kcsan: fix debugfs initcall return type

[Arnd Bergmann]

On Fri, May 14, 2021 at 4:45 PM 'Marco Elver' via Clang Built Linux
<clang-built-linux@googlegroups.com> wrote:
> On Fri, 14 May 2021 at 16:10, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Fri, May 14, 2021 at 04:00:08PM +0200, Arnd Bergmann wrote:
> > > From: Arnd Bergmann <arnd@arndb.de>
> > >
> > > clang points out that an initcall funciton should return an 'int':
> > >
> > > kernel/kcsan/debugfs.c:274:15: error: returning 'void' from a function with incompatible result type 'int'
> > > late_initcall(kcsan_debugfs_init);
> > > ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
> > > include/linux/init.h:292:46: note: expanded from macro 'late_initcall'
> > >  #define late_initcall(fn)               __define_initcall(fn, 7)
> > >
> > > Fixes: e36299efe7d7 ("kcsan, debugfs: Move debugfs file creation out of early init")
> > > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> [...]
> > >
> > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> Reviewed-by: Marco Elver <elver@google.com>
>
> Thanks for catching this -- it boggles my mind why gcc nor clang
> wouldn't warn about this by default...
> Is this a new clang?

It was clang-13, not sure if that made a difference.

         Arnd

Re: [PATCH] kcsan: fix debugfs initcall return type

[Nathan Chancellor]

On 5/14/2021 7:45 AM, Marco Elver wrote:
> On Fri, 14 May 2021 at 16:10, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>> On Fri, May 14, 2021 at 04:00:08PM +0200, Arnd Bergmann wrote:
>>> From: Arnd Bergmann <arnd@arndb.de>
>>>
>>> clang points out that an initcall funciton should return an 'int':
>>>
>>> kernel/kcsan/debugfs.c:274:15: error: returning 'void' from a function with incompatible result type 'int'
>>> late_initcall(kcsan_debugfs_init);
>>> ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
>>> include/linux/init.h:292:46: note: expanded from macro 'late_initcall'
>>>   #define late_initcall(fn)               __define_initcall(fn, 7)
>>>
>>> Fixes: e36299efe7d7 ("kcsan, debugfs: Move debugfs file creation out of early init")
>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> [...]
>>>
>> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> Reviewed-by: Marco Elver <elver@google.com>
> 
> Thanks for catching this -- it boggles my mind why gcc nor clang
> wouldn't warn about this by default...
> Is this a new clang?

KCSAN appears to only support x86_64, which also selects 
HAVE_ARCH_PREL32_RELOCATIONS, meaning that the initcalls never have 
their types validated because there is no assignment:

https://elixir.bootlin.com/linux/v5.12.4/source/include/linux/init.h#L240

In the case of CONFIG_LTO_CLANG, the initcall function is called in the 
stub function, resulting in the error that we see here.

Hopefully that makes sense :)

Cheers,
Nathan

Re: [PATCH] kcsan: fix debugfs initcall return type

[Nathan Chancellor]

On 5/14/2021 7:00 AM, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> clang points out that an initcall funciton should return an 'int':
> 
> kernel/kcsan/debugfs.c:274:15: error: returning 'void' from a function with incompatible result type 'int'
> late_initcall(kcsan_debugfs_init);
> ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
> include/linux/init.h:292:46: note: expanded from macro 'late_initcall'
>   #define late_initcall(fn)               __define_initcall(fn, 7)
> 
> Fixes: e36299efe7d7 ("kcsan, debugfs: Move debugfs file creation out of early init")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

For the record, this requires CONFIG_LTO_CLANG to be visible.

Reviewed-by: Nathan Chancellor <nathan@kernel.org>

> ---
>   kernel/kcsan/debugfs.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/kcsan/debugfs.c b/kernel/kcsan/debugfs.c
> index c1dd02f3be8b..e65de172ccf7 100644
> --- a/kernel/kcsan/debugfs.c
> +++ b/kernel/kcsan/debugfs.c
> @@ -266,9 +266,10 @@ static const struct file_operations debugfs_ops =
>   	.release = single_release
>   };
>   
> -static void __init kcsan_debugfs_init(void)
> +static int __init kcsan_debugfs_init(void)
>   {
>   	debugfs_create_file("kcsan", 0644, NULL, NULL, &debugfs_ops);
> +	return 0;
>   }
>   
>   late_initcall(kcsan_debugfs_init);
> 

Re: [PATCH] kcsan: fix debugfs initcall return type

[Paul E. McKenney]

On Fri, May 14, 2021 at 11:29:18AM -0700, Nathan Chancellor wrote:
> On 5/14/2021 7:00 AM, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> > 
> > clang points out that an initcall funciton should return an 'int':
> > 
> > kernel/kcsan/debugfs.c:274:15: error: returning 'void' from a function with incompatible result type 'int'
> > late_initcall(kcsan_debugfs_init);
> > ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
> > include/linux/init.h:292:46: note: expanded from macro 'late_initcall'
> >   #define late_initcall(fn)               __define_initcall(fn, 7)
> > 
> > Fixes: e36299efe7d7 ("kcsan, debugfs: Move debugfs file creation out of early init")
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
> For the record, this requires CONFIG_LTO_CLANG to be visible.
> 
> Reviewed-by: Nathan Chancellor <nathan@kernel.org>

Queued with the three Reviewed-by tags, thank you all!

Nathan, I lost the thread on exactly what it is that requires that
CONFIG_LTO_CLANG be visible.  A naive reader might conclude that the
compiler diagnostic does not appear unless CONFIG_LTO_CLANG=y, but
that would be surprising (and yes, I have been surprised many times).
If you are suggesting that the commit log be upgraded, could you please
supply suggested wording?

Once this is nailed down (or by Wednesday if I hear no more), I will
rebase it to the bottom of the current kcsan stack, let it soak in -next
for a couple of days, then send to Linus as a fix for a regression.
Hopefully some time next week.

							Thanx, Paul

> > ---
> >   kernel/kcsan/debugfs.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/kcsan/debugfs.c b/kernel/kcsan/debugfs.c
> > index c1dd02f3be8b..e65de172ccf7 100644
> > --- a/kernel/kcsan/debugfs.c
> > +++ b/kernel/kcsan/debugfs.c
> > @@ -266,9 +266,10 @@ static const struct file_operations debugfs_ops =
> >   	.release = single_release
> >   };
> > -static void __init kcsan_debugfs_init(void)
> > +static int __init kcsan_debugfs_init(void)
> >   {
> >   	debugfs_create_file("kcsan", 0644, NULL, NULL, &debugfs_ops);
> > +	return 0;
> >   }
> >   late_initcall(kcsan_debugfs_init);
> > 
> 

Re: [PATCH] kcsan: fix debugfs initcall return type

[Nathan Chancellor]

Hi Paul,

On 5/14/2021 12:36 PM, Paul E. McKenney wrote:
> On Fri, May 14, 2021 at 11:29:18AM -0700, Nathan Chancellor wrote:
>> On 5/14/2021 7:00 AM, Arnd Bergmann wrote:
>>> From: Arnd Bergmann <arnd@arndb.de>
>>>
>>> clang points out that an initcall funciton should return an 'int':
>>>
>>> kernel/kcsan/debugfs.c:274:15: error: returning 'void' from a function with incompatible result type 'int'
>>> late_initcall(kcsan_debugfs_init);
>>> ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
>>> include/linux/init.h:292:46: note: expanded from macro 'late_initcall'
>>>    #define late_initcall(fn)               __define_initcall(fn, 7)
>>>
>>> Fixes: e36299efe7d7 ("kcsan, debugfs: Move debugfs file creation out of early init")
>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>>
>> For the record, this requires CONFIG_LTO_CLANG to be visible.
>>
>> Reviewed-by: Nathan Chancellor <nathan@kernel.org>
> 
> Queued with the three Reviewed-by tags, thank you all!
> 
> Nathan, I lost the thread on exactly what it is that requires that
> CONFIG_LTO_CLANG be visible.  A naive reader might conclude that the
> compiler diagnostic does not appear unless CONFIG_LTO_CLANG=y, but
> that would be surprising (and yes, I have been surprised many times).
> If you are suggesting that the commit log be upgraded, could you please
> supply suggested wording?

You can see my response to Marco here:

https://lore.kernel.org/r/ad7fa126-f371-5a24-1d80-27fe8f655b05@kernel.org/

Maybe some improved wording might look like

clang with CONFIG_LTO_CLANG points out that an initcall function should 
return an 'int' due to the changes made to the initcall macros in commit 
3578ad11f3fb ("init: lto: fix PREL32 relocations"):

...

Arnd, do you have any objections?

Cheers,
Nathan

Re: [PATCH] kcsan: fix debugfs initcall return type

[Paul E. McKenney]

On Fri, May 14, 2021 at 01:11:05PM -0700, Nathan Chancellor wrote:
> Hi Paul,
> 
> On 5/14/2021 12:36 PM, Paul E. McKenney wrote:
> > On Fri, May 14, 2021 at 11:29:18AM -0700, Nathan Chancellor wrote:
> > > On 5/14/2021 7:00 AM, Arnd Bergmann wrote:
> > > > From: Arnd Bergmann <arnd@arndb.de>
> > > > 
> > > > clang points out that an initcall funciton should return an 'int':
> > > > 
> > > > kernel/kcsan/debugfs.c:274:15: error: returning 'void' from a function with incompatible result type 'int'
> > > > late_initcall(kcsan_debugfs_init);
> > > > ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
> > > > include/linux/init.h:292:46: note: expanded from macro 'late_initcall'
> > > >    #define late_initcall(fn)               __define_initcall(fn, 7)
> > > > 
> > > > Fixes: e36299efe7d7 ("kcsan, debugfs: Move debugfs file creation out of early init")
> > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > > 
> > > For the record, this requires CONFIG_LTO_CLANG to be visible.
> > > 
> > > Reviewed-by: Nathan Chancellor <nathan@kernel.org>
> > 
> > Queued with the three Reviewed-by tags, thank you all!
> > 
> > Nathan, I lost the thread on exactly what it is that requires that
> > CONFIG_LTO_CLANG be visible.  A naive reader might conclude that the
> > compiler diagnostic does not appear unless CONFIG_LTO_CLANG=y, but
> > that would be surprising (and yes, I have been surprised many times).
> > If you are suggesting that the commit log be upgraded, could you please
> > supply suggested wording?
> 
> You can see my response to Marco here:
> 
> https://lore.kernel.org/r/ad7fa126-f371-5a24-1d80-27fe8f655b05@kernel.org/
> 
> Maybe some improved wording might look like
> 
> clang with CONFIG_LTO_CLANG points out that an initcall function should
> return an 'int' due to the changes made to the initcall macros in commit
> 3578ad11f3fb ("init: lto: fix PREL32 relocations"):

OK, so the naive reading was correct, thank you!

> ...
> 
> Arnd, do you have any objections?

In the meantime, here is what I have.  Please let me know of any needed
updates.

							Thanx, Paul

------------------------------------------------------------------------

commit fe1f4e1b099797d06bd8c66681eed4024c3cad67
Author: Arnd Bergmann <arnd@arndb.de>
Date:   Fri May 14 16:00:08 2021 +0200

    kcsan: Fix debugfs initcall return type
    
    clang with CONFIG_LTO_CLANG points out that an initcall function should
    return an 'int' due to the changes made to the initcall macros in commit
    3578ad11f3fb ("init: lto: fix PREL32 relocations"):
    
    kernel/kcsan/debugfs.c:274:15: error: returning 'void' from a function with incompatible result type 'int'
    late_initcall(kcsan_debugfs_init);
    ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
    include/linux/init.h:292:46: note: expanded from macro 'late_initcall'
     #define late_initcall(fn)               __define_initcall(fn, 7)
    
    Fixes: e36299efe7d7 ("kcsan, debugfs: Move debugfs file creation out of early init")
    Cc: stable <stable@vger.kernel.org>
    Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    Reviewed-by: Marco Elver <elver@google.com>
    Reviewed-by: Nathan Chancellor <nathan@kernel.org>
    Signed-off-by: Arnd Bergmann <arnd@arndb.de>
    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>

diff --git a/kernel/kcsan/debugfs.c b/kernel/kcsan/debugfs.c
index c1dd02f3be8b..e65de172ccf7 100644
--- a/kernel/kcsan/debugfs.c
+++ b/kernel/kcsan/debugfs.c
@@ -266,9 +266,10 @@ static const struct file_operations debugfs_ops =
 	.release = single_release
 };
 
-static void __init kcsan_debugfs_init(void)
+static int __init kcsan_debugfs_init(void)
 {
 	debugfs_create_file("kcsan", 0644, NULL, NULL, &debugfs_ops);
+	return 0;
 }
 
 late_initcall(kcsan_debugfs_init);

Re: [PATCH] kcsan: fix debugfs initcall return type

[Arnd Bergmann]

On Fri, May 14, 2021 at 10:18 PM Paul E. McKenney <paulmck@kernel.org> wrote:
> On Fri, May 14, 2021 at 01:11:05PM -0700, Nathan Chancellor wrote:

> > You can see my response to Marco here:
> >
> > https://lore.kernel.org/r/ad7fa126-f371-5a24-1d80-27fe8f655b05@kernel.org/
> >
> > Maybe some improved wording might look like
> >
> > clang with CONFIG_LTO_CLANG points out that an initcall function should
> > return an 'int' due to the changes made to the initcall macros in commit
> > 3578ad11f3fb ("init: lto: fix PREL32 relocations"):
>
> OK, so the naive reading was correct, thank you!
>
> > ...
> >
> > Arnd, do you have any objections?
>
> In the meantime, here is what I have.  Please let me know of any needed
> updates.
>

Looks good to me, thanks for the improvements!

          Arnd

Re: [PATCH] kcsan: fix debugfs initcall return type

[Marco Elver]

On Fri, May 14, 2021 at 11:16PM +0200, Arnd Bergmann wrote:
> On Fri, May 14, 2021 at 10:18 PM Paul E. McKenney <paulmck@kernel.org> wrote:
> > On Fri, May 14, 2021 at 01:11:05PM -0700, Nathan Chancellor wrote:
> 
> > > You can see my response to Marco here:
> > >
> > > https://lore.kernel.org/r/ad7fa126-f371-5a24-1d80-27fe8f655b05@kernel.org/
> > >
> > > Maybe some improved wording might look like
> > >
> > > clang with CONFIG_LTO_CLANG points out that an initcall function should
> > > return an 'int' due to the changes made to the initcall macros in commit
> > > 3578ad11f3fb ("init: lto: fix PREL32 relocations"):
> >
> > OK, so the naive reading was correct, thank you!
> >
> > > ...
> > >
> > > Arnd, do you have any objections?
> >
> > In the meantime, here is what I have.  Please let me know of any needed
> > updates.
> >
> 
> Looks good to me, thanks for the improvements!

FWIW, this prompted me to see if I can convince the compiler to complain
in all configs. The below is what I came up with and will send once the
fix here has landed. Need to check a few other config+arch combinations
(allyesconfig with gcc on x86_64 is good).

Thanks,
-- Marco

------ >8 ------

From 96c1c4e9902e96485268909d5ea8f91b9595e187 Mon Sep 17 00:00:00 2001
From: Marco Elver <elver@google.com>
Date: Fri, 14 May 2021 21:08:50 +0200
Subject: [PATCH] init: verify that function is initcall_t at compile-time

In the spirit of making it hard to misuse an interface, add a
compile-time assertion in the CONFIG_HAVE_ARCH_PREL32_RELOCATIONS case
to verify the initcall function matches initcall_t, because the inline
asm bypasses any type-checking the compiler would otherwise do. This
will help developers catch incorrect API use in all configurations.

A recent example of this is:
https://lkml.kernel.org/r/20210514140015.2944744-1-arnd@kernel.org

Signed-off-by: Marco Elver <elver@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Joe Perches <joe@perches.com>
Cc: Masahiro Yamada <masahiroy@kernel.org>
Cc: Miguel Ojeda <ojeda@kernel.org>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: "Paul E. McKenney" <paulmck@kernel.org>
---
 include/linux/init.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/init.h b/include/linux/init.h
index 045ad1650ed1..d82b4b2e1d25 100644
--- a/include/linux/init.h
+++ b/include/linux/init.h
@@ -242,7 +242,8 @@ extern bool initcall_debug;
 	asm(".section	\"" __sec "\", \"a\"		\n"	\
 	    __stringify(__name) ":			\n"	\
 	    ".long	" __stringify(__stub) " - .	\n"	\
-	    ".previous					\n");
+	    ".previous					\n");	\
+	static_assert(__same_type(initcall_t, &fn));
 #else
 #define ____define_initcall(fn, __unused, __name, __sec)	\
 	static initcall_t __name __used 			\
-- 
2.31.1.751.gd2f1c929bd-goog

Re: [PATCH] kcsan: fix debugfs initcall return type

[Paul E. McKenney]

On Sat, May 15, 2021 at 01:01:31AM +0200, Marco Elver wrote:
> On Fri, May 14, 2021 at 11:16PM +0200, Arnd Bergmann wrote:
> > On Fri, May 14, 2021 at 10:18 PM Paul E. McKenney <paulmck@kernel.org> wrote:
> > > On Fri, May 14, 2021 at 01:11:05PM -0700, Nathan Chancellor wrote:
> > 
> > > > You can see my response to Marco here:
> > > >
> > > > https://lore.kernel.org/r/ad7fa126-f371-5a24-1d80-27fe8f655b05@kernel.org/
> > > >
> > > > Maybe some improved wording might look like
> > > >
> > > > clang with CONFIG_LTO_CLANG points out that an initcall function should
> > > > return an 'int' due to the changes made to the initcall macros in commit
> > > > 3578ad11f3fb ("init: lto: fix PREL32 relocations"):
> > >
> > > OK, so the naive reading was correct, thank you!
> > >
> > > > ...
> > > >
> > > > Arnd, do you have any objections?
> > >
> > > In the meantime, here is what I have.  Please let me know of any needed
> > > updates.
> > >
> > 
> > Looks good to me, thanks for the improvements!
> 
> FWIW, this prompted me to see if I can convince the compiler to complain
> in all configs. The below is what I came up with and will send once the
> fix here has landed. Need to check a few other config+arch combinations
> (allyesconfig with gcc on x86_64 is good).

Cool!

If I have not sent the pull request for Arnd's fix by Wednesday, please
remind me.

							Thanx, Paul

> Thanks,
> -- Marco
> 
> ------ >8 ------
> 
> >From 96c1c4e9902e96485268909d5ea8f91b9595e187 Mon Sep 17 00:00:00 2001
> From: Marco Elver <elver@google.com>
> Date: Fri, 14 May 2021 21:08:50 +0200
> Subject: [PATCH] init: verify that function is initcall_t at compile-time
> 
> In the spirit of making it hard to misuse an interface, add a
> compile-time assertion in the CONFIG_HAVE_ARCH_PREL32_RELOCATIONS case
> to verify the initcall function matches initcall_t, because the inline
> asm bypasses any type-checking the compiler would otherwise do. This
> will help developers catch incorrect API use in all configurations.
> 
> A recent example of this is:
> https://lkml.kernel.org/r/20210514140015.2944744-1-arnd@kernel.org
> 
> Signed-off-by: Marco Elver <elver@google.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Joe Perches <joe@perches.com>
> Cc: Masahiro Yamada <masahiroy@kernel.org>
> Cc: Miguel Ojeda <ojeda@kernel.org>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> ---
>  include/linux/init.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/init.h b/include/linux/init.h
> index 045ad1650ed1..d82b4b2e1d25 100644
> --- a/include/linux/init.h
> +++ b/include/linux/init.h
> @@ -242,7 +242,8 @@ extern bool initcall_debug;
>  	asm(".section	\"" __sec "\", \"a\"		\n"	\
>  	    __stringify(__name) ":			\n"	\
>  	    ".long	" __stringify(__stub) " - .	\n"	\
> -	    ".previous					\n");
> +	    ".previous					\n");	\
> +	static_assert(__same_type(initcall_t, &fn));
>  #else
>  #define ____define_initcall(fn, __unused, __name, __sec)	\
>  	static initcall_t __name __used 			\
> -- 
> 2.31.1.751.gd2f1c929bd-goog
> 

Re: [PATCH] kcsan: fix debugfs initcall return type

[Paul E. McKenney]

On Fri, May 14, 2021 at 05:55:50PM -0700, Paul E. McKenney wrote:
> On Sat, May 15, 2021 at 01:01:31AM +0200, Marco Elver wrote:
> > On Fri, May 14, 2021 at 11:16PM +0200, Arnd Bergmann wrote:
> > > On Fri, May 14, 2021 at 10:18 PM Paul E. McKenney <paulmck@kernel.org> wrote:
> > > > On Fri, May 14, 2021 at 01:11:05PM -0700, Nathan Chancellor wrote:
> > > 
> > > > > You can see my response to Marco here:
> > > > >
> > > > > https://lore.kernel.org/r/ad7fa126-f371-5a24-1d80-27fe8f655b05@kernel.org/
> > > > >
> > > > > Maybe some improved wording might look like
> > > > >
> > > > > clang with CONFIG_LTO_CLANG points out that an initcall function should
> > > > > return an 'int' due to the changes made to the initcall macros in commit
> > > > > 3578ad11f3fb ("init: lto: fix PREL32 relocations"):
> > > >
> > > > OK, so the naive reading was correct, thank you!
> > > >
> > > > > ...
> > > > >
> > > > > Arnd, do you have any objections?
> > > >
> > > > In the meantime, here is what I have.  Please let me know of any needed
> > > > updates.
> > > >
> > > 
> > > Looks good to me, thanks for the improvements!
> > 
> > FWIW, this prompted me to see if I can convince the compiler to complain
> > in all configs. The below is what I came up with and will send once the
> > fix here has landed. Need to check a few other config+arch combinations
> > (allyesconfig with gcc on x86_64 is good).
> 
> Cool!
> 
> If I have not sent the pull request for Arnd's fix by Wednesday, please
> remind me.

Except that I was slow getting Miguel Ojeda's Reviewed-by applied.
I need to wait for -next to incorporate this change (hopefully by
tomorrow, Pacific Time), and then test this.  With luck, I will send
this Thursday, Pacific Time.

							Thanx, Paul

Re: [PATCH] kcsan: fix debugfs initcall return type

[Marco Elver]

On Wed, 19 May 2021 at 01:20, Paul E. McKenney <paulmck@kernel.org> wrote:
[...]
> > If I have not sent the pull request for Arnd's fix by Wednesday, please
> > remind me.
>
> Except that I was slow getting Miguel Ojeda's Reviewed-by applied.
> I need to wait for -next to incorporate this change (hopefully by
> tomorrow, Pacific Time), and then test this.  With luck, I will send
> this Thursday, Pacific Time.

Thank you! If I don't see anything by end of this week, I'll do the
reminder then.

Thanks,
-- Marco

Re: [PATCH] kcsan: fix debugfs initcall return type

[Miguel Ojeda]

On Sat, May 15, 2021 at 1:01 AM 'Marco Elver' via Clang Built Linux
<clang-built-linux@googlegroups.com> wrote:
>
> FWIW, this prompted me to see if I can convince the compiler to complain
> in all configs. The below is what I came up with and will send once the
> fix here has landed. Need to check a few other config+arch combinations
> (allyesconfig with gcc on x86_64 is good).

+1 Works for LLVM=1 too (x86_64, small config).

Reviewed-by: Miguel Ojeda <ojeda@kernel.org>

Cheers,
Miguel

Re: [PATCH] kcsan: fix debugfs initcall return type

[Paul E. McKenney]

On Sat, May 15, 2021 at 04:19:45PM +0200, Miguel Ojeda wrote:
> On Sat, May 15, 2021 at 1:01 AM 'Marco Elver' via Clang Built Linux
> <clang-built-linux@googlegroups.com> wrote:
> >
> > FWIW, this prompted me to see if I can convince the compiler to complain
> > in all configs. The below is what I came up with and will send once the
> > fix here has landed. Need to check a few other config+arch combinations
> > (allyesconfig with gcc on x86_64 is good).
> 
> +1 Works for LLVM=1 too (x86_64, small config).
> 
> Reviewed-by: Miguel Ojeda <ojeda@kernel.org>

I will applyon the next rebase, thank you!

							Thanx, Paul

Re: [PATCH] kcsan: fix debugfs initcall return type

[Nathan Chancellor]

Hi Marco,

On Sat, May 15, 2021 at 01:01:31AM +0200, Marco Elver wrote:
> FWIW, this prompted me to see if I can convince the compiler to complain
> in all configs. The below is what I came up with and will send once the
> fix here has landed. Need to check a few other config+arch combinations
> (allyesconfig with gcc on x86_64 is good).
> 
> Thanks,
> -- Marco
> 
> ------ >8 ------
> 
> >From 96c1c4e9902e96485268909d5ea8f91b9595e187 Mon Sep 17 00:00:00 2001
> From: Marco Elver <elver@google.com>
> Date: Fri, 14 May 2021 21:08:50 +0200
> Subject: [PATCH] init: verify that function is initcall_t at compile-time
> 
> In the spirit of making it hard to misuse an interface, add a
> compile-time assertion in the CONFIG_HAVE_ARCH_PREL32_RELOCATIONS case
> to verify the initcall function matches initcall_t, because the inline
> asm bypasses any type-checking the compiler would otherwise do. This
> will help developers catch incorrect API use in all configurations.
> 
> A recent example of this is:
> https://lkml.kernel.org/r/20210514140015.2944744-1-arnd@kernel.org
> 
> Signed-off-by: Marco Elver <elver@google.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Joe Perches <joe@perches.com>
> Cc: Masahiro Yamada <masahiroy@kernel.org>
> Cc: Miguel Ojeda <ojeda@kernel.org>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>

Hi Marco,

I verified that I see an error without Arnd's patch with all supported
KCSAN compilers when I apply this patch.

clang-11: https://builds.tuxbuild.com/1sYcyUZoCS7hFS3qZMZsJgsA5bp/build.log
clang-12: https://builds.tuxbuild.com/1sYcyRDtvvkaQQbGX435X8FUb6o/build.log
clang-13: https://builds.tuxbuild.com/1sYcyPubVREo7Dl05zCKRRNh6RB/build.log

gcc-11 had to be done locally as TuxSuite appears not to support gcc-11
so no nifty link:

In file included from /home/nathan/cbl/src/korg-linux/include/asm-generic/atomic-instrumented.h:20,
                 from /home/nathan/cbl/src/korg-linux/include/linux/atomic.h:82,
                 from /home/nathan/cbl/src/korg-linux/kernel/kcsan/debugfs.c:10:
/home/nathan/cbl/src/korg-linux/include/linux/build_bug.h:78:41: error: static assertion failed: "__same_type(initcall_t, &kcsan_debugfs_init)"
   78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
      |                                         ^~~~~~~~~~~~~~
/home/nathan/cbl/src/korg-linux/include/linux/build_bug.h:77:34: note: in expansion of macro '__static_assert'
   77 | #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
      |                                  ^~~~~~~~~~~~~~~
/home/nathan/cbl/src/korg-linux/include/linux/init.h:246:9: note: in expansion of macro 'static_assert'
  246 |         static_assert(__same_type(initcall_t, &fn));
      |         ^~~~~~~~~~~~~
/home/nathan/cbl/src/korg-linux/include/linux/init.h:254:9: note: in expansion of macro '____define_initcall'
  254 |         ____define_initcall(fn,                                 \
      |         ^~~~~~~~~~~~~~~~~~~
/home/nathan/cbl/src/korg-linux/include/linux/init.h:260:9: note: in expansion of macro '__unique_initcall'
  260 |         __unique_initcall(fn, id, __sec, __initcall_id(fn))
      |         ^~~~~~~~~~~~~~~~~
/home/nathan/cbl/src/korg-linux/include/linux/init.h:262:35: note: in expansion of macro '___define_initcall'
  262 | #define __define_initcall(fn, id) ___define_initcall(fn, id, .initcall##id)
      |                                   ^~~~~~~~~~~~~~~~~~
/home/nathan/cbl/src/korg-linux/include/linux/init.h:293:41: note: in expansion of macro '__define_initcall'
  293 | #define late_initcall(fn)               __define_initcall(fn, 7)
      |                                         ^~~~~~~~~~~~~~~~~
/home/nathan/cbl/src/korg-linux/kernel/kcsan/debugfs.c:274:1: note: in expansion of macro 'late_initcall'
  274 | late_initcall(kcsan_debugfs_init);
      | ^~~~~~~~~~~~~
make[3]: *** [/home/nathan/cbl/src/korg-linux/scripts/Makefile.build:273: kernel/kcsan/debugfs.o] Error 1

I did a series of builds against next-20210514 with gcc 8 through 10 and
clang 11 through 13 targeting arm, arm64, i386, powerpc, s390, and
x86_64 defconfig and allmodconfig with no errors with this patch on top
of Arnd's. Repo and TuxSuite configuration below in case anyone cares :)

https://git.kernel.org/pub/scm/linux/kernel/git/nathan/linux.git/log/?h=tuxsuite/initcall-static-assert
https://gist.github.com/nathanchance/eb71e1c2287561a0de79ef28c3c521384

When you formally send it, please feel free to add:

Reviewed-by: Nathan Chancellor <nathan@kernel.org>
Tested-by: Nathan Chancellor <nathan@kernel.org>

Cheers,
Nathan

> ---
>  include/linux/init.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/init.h b/include/linux/init.h
> index 045ad1650ed1..d82b4b2e1d25 100644
> --- a/include/linux/init.h
> +++ b/include/linux/init.h
> @@ -242,7 +242,8 @@ extern bool initcall_debug;
>  	asm(".section	\"" __sec "\", \"a\"		\n"	\
>  	    __stringify(__name) ":			\n"	\
>  	    ".long	" __stringify(__stub) " - .	\n"	\
> -	    ".previous					\n");
> +	    ".previous					\n");	\
> +	static_assert(__same_type(initcall_t, &fn));
>  #else
>  #define ____define_initcall(fn, __unused, __name, __sec)	\
>  	static initcall_t __name __used 			\
> -- 
> 2.31.1.751.gd2f1c929bd-goog
>