[hnaz-mm:master 272/379] lib/vsprintf.c:991:13: warning: variable 'modbuildid' set but not used

[hnaz-mm:master 272/379] lib/vsprintf.c:991:13: warning: variable 'modbuildid' set but not used

[kernel test robot]

tree:   https://github.com/hnaz/linux-mm master
head:   a46912c14343fd3269cc133494988af90b377d9f
commit: b314f622e664eb263ea03ef7f4580e37146f123f [272/379] kallsyms: enhance %pS/s/b printing when KALLSYSMS is disabled
config: arm-eseries_pxa_defconfig (https://download.01.org/0day-ci/archive/20220301/202203012040.uFWGm3My-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/hnaz/linux-mm/commit/b314f622e664eb263ea03ef7f4580e37146f123f
        git remote add hnaz-mm https://github.com/hnaz/linux-mm
        git fetch --no-tags hnaz-mm master
        git checkout b314f622e664eb263ea03ef7f4580e37146f123f
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arm SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   lib/vsprintf.c: In function 'sprint_module_info':
>> lib/vsprintf.c:991:13: warning: variable 'modbuildid' set but not used [-Wunused-but-set-variable]
     991 |         int modbuildid = 0;
         |             ^~~~~~~~~~
   lib/vsprintf.c: In function 'va_format':
   lib/vsprintf.c:1759:9: warning: function 'va_format' might be a candidate for 'gnu_printf' format attribute [-Wsuggest-attribute=format]
    1759 |         buf += vsnprintf(buf, end > buf ? end - buf : 0, va_fmt->fmt, va);
         |         ^~~


vim +/modbuildid +991 lib/vsprintf.c

   981	
   982	#if !defined(CONFIG_KALLSYMS) && defined(CONFIG_MODULES)
   983	static int sprint_module_info(char *buf, char *end, unsigned long value,
   984				     const char *fmt)
   985	{
   986		struct module *mod;
   987		unsigned long offset = 1;
   988		unsigned long base;
   989		int ret = 0;
   990		const char *modname;
 > 991		int modbuildid = 0;
   992		int len;
   993	#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID)
   994		const unsigned char *buildid = NULL;
   995	#endif
   996	
   997		if (is_ksym_addr(value))
   998			return 0;
   999	
  1000		if (*fmt == 'B' && fmt[1] == 'b')
  1001			modbuildid = 1;
  1002		else if (*fmt == 'S' && (fmt[1] == 'b' || (fmt[1] == 'R' && fmt[2] == 'b')))
  1003			modbuildid = 1;
  1004		else if (*fmt != 's') {
  1005			/*
  1006			 * do nothing.
  1007			 */
  1008		} else
  1009			offset = 0;
  1010	
  1011		preempt_disable();
  1012		mod = __module_address(value);
  1013		if (mod) {
  1014			ret = 1;
  1015			modname = mod->name;
  1016	#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID)
  1017			if (modbuildid)
  1018				buildid = mod->build_id;
  1019	#endif
  1020			if (offset) {
  1021				base = (unsigned long)mod->core_layout.base;
  1022				offset = value - base;
  1023			}
  1024		}
  1025	
  1026		preempt_enable();
  1027		if (!ret)
  1028			return 0;
  1029	
  1030		/* address belongs to module */
  1031		if (offset)
  1032			len = sprintf(buf, "0x%lx+0x%lx", base, offset);
  1033		else
  1034			len = sprintf(buf, "0x%lx", value);
  1035	
  1036		len += sprintf(buf + len, " [%s", modname);
  1037	#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID)
  1038		if (modbuildid && buildid) {
  1039			/* build ID should match length of sprintf */
  1040			static_assert(sizeof(typeof_member(struct module, build_id)) == 20);
  1041			len += sprintf(buf + len, " %20phN", buildid);
  1042		}
  1043	#endif
  1044		len += sprintf(buf + len, "]");
  1045	
  1046		return len;
  1047	}
  1048	#else
  1049	static inline int sprint_module_info(char *buf, char *end, unsigned long value,
  1050				     const char *fmt)
  1051	{
  1052		return 0;
  1053	}
  1054	#endif
  1055	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [hnaz-mm:master 272/379] lib/vsprintf.c:991:13: warning: variable 'modbuildid' set but not used

[Andrew Morton]

On Tue, 1 Mar 2022 20:11:04 +0800 kernel test robot <lkp@intel.com> wrote:

> tree:   https://github.com/hnaz/linux-mm master
> head:   a46912c14343fd3269cc133494988af90b377d9f
> commit: b314f622e664eb263ea03ef7f4580e37146f123f [272/379] kallsyms: enhance %pS/s/b printing when KALLSYSMS is disabled
> config: arm-eseries_pxa_defconfig (https://download.01.org/0day-ci/archive/20220301/202203012040.uFWGm3My-lkp@intel.com/config)
> compiler: arm-linux-gnueabi-gcc (GCC) 11.2.0
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # https://github.com/hnaz/linux-mm/commit/b314f622e664eb263ea03ef7f4580e37146f123f
>         git remote add hnaz-mm https://github.com/hnaz/linux-mm
>         git fetch --no-tags hnaz-mm master
>         git checkout b314f622e664eb263ea03ef7f4580e37146f123f
>         # save the config file to linux build tree
>         mkdir build_dir
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arm SHELL=/bin/bash
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All warnings (new ones prefixed by >>):
> 
>    lib/vsprintf.c: In function 'sprint_module_info':
> >> lib/vsprintf.c:991:13: warning: variable 'modbuildid' set but not used [-Wunused-but-set-variable]
>      991 |         int modbuildid = 0;
>          |             ^~~~~~~~~~

Do we care about this?  [-Wunused-but-set-variable isn't normally set. 
Under what circumstances does it get set in your setup?

I did this:

--- a/lib/vsprintf.c~kallsyms-enhance-%ps-s-b-printing-when-kallsysms-is-disabled-fix
+++ a/lib/vsprintf.c
@@ -988,7 +988,7 @@ static int sprint_module_info(char *buf,
 	unsigned long base;
 	int ret = 0;
 	const char *modname;
-	int modbuildid = 0;
+	int modbuildid __maybe_unused = 0;
 	int len;
 #if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID)
 	const unsigned char *buildid = NULL;
_


>    lib/vsprintf.c: In function 'va_format':
>    lib/vsprintf.c:1759:9: warning: function 'va_format' might be a candidate for 'gnu_printf' format attribute [-Wsuggest-attribute=format]
>     1759 |         buf += vsnprintf(buf, end > buf ? end - buf : 0, va_fmt->fmt, va);
>          |         ^~~

I wonder what this means.

Re: [hnaz-mm:master 272/379] lib/vsprintf.c:991:13: warning: variable 'modbuildid' set but not used

[Matthew Wilcox]

On Tue, Mar 01, 2022 at 10:24:48AM -0800, Andrew Morton wrote:
> >    lib/vsprintf.c: In function 'va_format':
> >    lib/vsprintf.c:1759:9: warning: function 'va_format' might be a candidate for 'gnu_printf' format attribute [-Wsuggest-attribute=format]
> >     1759 |         buf += vsnprintf(buf, end > buf ? end - buf : 0, va_fmt->fmt, va);
> >          |         ^~~
> 
> I wonder what this means.

It means the compiler thinks we might want to add:

__attribute__((format(gnu_printf, x, y))) to the function declaration so it
can type-check the arguments.

'format (ARCHETYPE, STRING-INDEX, FIRST-TO-CHECK)'
     The 'format' attribute specifies that a function takes 'printf',
     'scanf', 'strftime' or 'strfmon' style arguments that should be
     type-checked against a format string.  For example, the
     declaration:

          extern int
          my_printf (void *my_object, const char *my_format, ...)
                __attribute__ ((format (printf, 2, 3)));

     causes the compiler to check the arguments in calls to 'my_printf'
     for consistency with the 'printf' style format string argument
     'my_format'.


I haven't looked into this at all and have no idea if we should.

Re: [hnaz-mm:master 272/379] lib/vsprintf.c:991:13: warning: variable 'modbuildid' set but not used

[John Ogness]

On 2022-03-01, Matthew Wilcox <willy@infradead.org> wrote:
>> >    lib/vsprintf.c: In function 'va_format':
>> >    lib/vsprintf.c:1759:9: warning: function 'va_format' might be a candidate for 'gnu_printf' format attribute [-Wsuggest-attribute=format]
>> >     1759 |         buf += vsnprintf(buf, end > buf ? end - buf : 0, va_fmt->fmt, va);
>> >          |         ^~~
>> 
>> I wonder what this means.
>
> It means the compiler thinks we might want to add:
>
> __attribute__((format(gnu_printf, x, y))) to the function declaration so it
> can type-check the arguments.
>
> 'format (ARCHETYPE, STRING-INDEX, FIRST-TO-CHECK)'
>      The 'format' attribute specifies that a function takes 'printf',
>      'scanf', 'strftime' or 'strfmon' style arguments that should be
>      type-checked against a format string.  For example, the
>      declaration:
>
>           extern int
>           my_printf (void *my_object, const char *my_format, ...)
>                 __attribute__ ((format (printf, 2, 3)));
>
>      causes the compiler to check the arguments in calls to 'my_printf'
>      for consistency with the 'printf' style format string argument
>      'my_format'.
>
>
> I haven't looked into this at all and have no idea if we should.

AFAICT it is not possible to use the gnu_printf format attribute for
this because the va_list to check is a field within the passed in struct
pointer @va_fmt.

John Ogness

Re: [hnaz-mm:master 272/379] lib/vsprintf.c:991:13: warning: variable 'modbuildid' set but not used

[Petr Mladek]

On Tue 2022-03-01 19:22:46, Matthew Wilcox wrote:
> On Tue, Mar 01, 2022 at 10:24:48AM -0800, Andrew Morton wrote:
> > >    lib/vsprintf.c: In function 'va_format':
> > >    lib/vsprintf.c:1759:9: warning: function 'va_format' might be a candidate for 'gnu_printf' format attribute [-Wsuggest-attribute=format]
> > >     1759 |         buf += vsnprintf(buf, end > buf ? end - buf : 0, va_fmt->fmt, va);
> > >          |         ^~~
> > 
> > I wonder what this means.
> 
> It means the compiler thinks we might want to add:
> 
> __attribute__((format(gnu_printf, x, y))) to the function declaration so it
> can type-check the arguments.
> 
> 'format (ARCHETYPE, STRING-INDEX, FIRST-TO-CHECK)'
>      The 'format' attribute specifies that a function takes 'printf',
>      'scanf', 'strftime' or 'strfmon' style arguments that should be
>      type-checked against a format string.  For example, the
>      declaration:
> 
>           extern int
>           my_printf (void *my_object, const char *my_format, ...)
>                 __attribute__ ((format (printf, 2, 3)));
> 
>      causes the compiler to check the arguments in calls to 'my_printf'
>      for consistency with the 'printf' style format string argument
>      'my_format'.
> 
> 
> I haven't looked into this at all and have no idea if we should.

There is the macro __printf(x, y). This particular warning can be
fixed by:

--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1672,6 +1672,7 @@ char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 	return buf;
 }
 
+__printf(5, 0)
 static char *va_format(char *buf, char *end, struct va_format *va_fmt,
 		       struct printf_spec spec, const char *fmt)
 {


But it seems to be can of worms. I get more warnings after fixing this
one. The following patch calmed down the warnings in vsprintf.o. But
it triggered another warning elsewhere, for example:

kernel/trace/bpf_trace.c: In function ‘____bpf_trace_printk’:
kernel/trace/bpf_trace.c:383:2: warning: function ‘____bpf_trace_printk’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
  ret = bstr_printf(buf, sizeof(buf), fmt, bin_args);
  ^~~
kernel/trace/bpf_trace.c: In function ‘____bpf_trace_vprintk’:
kernel/trace/bpf_trace.c:439:2: warning: function ‘____bpf_trace_vprintk’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
  ret = bstr_printf(buf, sizeof(buf), fmt, bin_args);
  ^~~


From 66f6166e968d8c7e752260e7ee7e1c0414cd2cce Mon Sep 17 00:00:00 2001
From: Petr Mladek <pmladek@suse.com>
Date: Wed, 2 Mar 2022 10:03:14 +0100
Subject: [PATCH] vsprintf: Fix warnings about missing gnu_printf attribute in vsprintf.o
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The following warning is produced by: make W=1 lib/vsprintf.o

lib/vsprintf.c:1770:1: error: multiple storage classes in declaration specifiers
 static char *va_format(char *buf, char *end, struct va_format *va_fmt,
 ^~~~~~
lib/vsprintf.c:1770:14: warning: no previous prototype for ‘va_format’ [-Wmissing-prototypes]
 static char *va_format(char *buf, char *end, struct va_format *va_fmt,
              ^~~~~~~~~

More similar warnings are printed after fixing the 1st one:

lib/vsprintf.c: In function ‘pointer’:
lib/vsprintf.c:2522:3: warning: function ‘pointer’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
   return va_format(buf, end, ptr, spec, fmt);
   ^~~~~~
lib/vsprintf.c: In function ‘vbin_printf’:
lib/vsprintf.c:3213:12: warning: function ‘vbin_printf’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
            spec);
            ^~~~
lib/vsprintf.c: In function ‘bstr_printf’:
lib/vsprintf.c:3398:5: warning: function ‘bstr_printf’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
     str = pointer(fmt, str, end, get_arg(void *), spec);
     ^~~

Add the proposed annotation.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 include/linux/string.h | 3 +++
 lib/vsprintf.c         | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index b6572aeca2f5..e37eaecb7906 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -196,8 +196,11 @@ int __sysfs_match_string(const char * const *array, size_t n, const char *s);
 #define sysfs_match_string(_a, _s) __sysfs_match_string(_a, ARRAY_SIZE(_a), _s)
 
 #ifdef CONFIG_BINARY_PRINTF
+__printf(3, 0)
 int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args);
+__printf(3, 0)
 int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf);
+__printf(3, 4)
 int bprintf(u32 *bin_buf, size_t size, const char *fmt, ...) __printf(3, 4);
 #endif
 
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3b8129dd374c..981c71da5e3d 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1672,6 +1672,7 @@ char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 	return buf;
 }
 
+__printf(5, 0)
 static char *va_format(char *buf, char *end, struct va_format *va_fmt,
 		       struct printf_spec spec, const char *fmt)
 {
@@ -2498,6 +2499,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
  * @precision: precision of a number
  * @qualifier: qualifier of a number (long, size_t, ...)
  */
+__printf(1, 0)
 static noinline_for_stack
 int format_decode(const char *fmt, struct printf_spec *spec)
 {
-- 
2.26.2

Re: [hnaz-mm:master 272/379] lib/vsprintf.c:991:13: warning: variable 'modbuildid' set but not used

[Petr Mladek]

On Wed 2022-03-02 10:58:49, John Ogness wrote:
> On 2022-03-01, Matthew Wilcox <willy@infradead.org> wrote:
> >> >    lib/vsprintf.c: In function 'va_format':
> >> >    lib/vsprintf.c:1759:9: warning: function 'va_format' might be a candidate for 'gnu_printf' format attribute [-Wsuggest-attribute=format]
> >> >     1759 |         buf += vsnprintf(buf, end > buf ? end - buf : 0, va_fmt->fmt, va);
> >> >          |         ^~~
> >> 
> >> I wonder what this means.
> >
> > It means the compiler thinks we might want to add:
> >
> > __attribute__((format(gnu_printf, x, y))) to the function declaration so it
> > can type-check the arguments.
> >
> > 'format (ARCHETYPE, STRING-INDEX, FIRST-TO-CHECK)'
> >      The 'format' attribute specifies that a function takes 'printf',
> >      'scanf', 'strftime' or 'strfmon' style arguments that should be
> >      type-checked against a format string.  For example, the
> >      declaration:
> >
> >           extern int
> >           my_printf (void *my_object, const char *my_format, ...)
> >                 __attribute__ ((format (printf, 2, 3)));
> >
> >      causes the compiler to check the arguments in calls to 'my_printf'
> >      for consistency with the 'printf' style format string argument
> >      'my_format'.
> >
> >
> > I haven't looked into this at all and have no idea if we should.
> 
> AFAICT it is not possible to use the gnu_printf format attribute for
> this because the va_list to check is a field within the passed in struct
> pointer @va_fmt.

My understanding is that it can be handled by passing '0' as the
FIRST-TO-CHECK parameter:

<paste>
format (archetype, string-index, first-to-check)
The format attribute specifies that a function takes printf, scanf,
strftime or strfmon style arguments that should be type-checked
against a format string. For example, the declaration:

[...]

"For functions where the arguments are not available to be checked
(such as vprintf), specify the third parameter as zero."
<paste>

, cut&pasted from
https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes


Well, this particular function va_format() is never used with
open-coded @arg parameter. It always just passes @arg from
the caller. So that the check is not important.

Best Regards,
Petr

Re: [hnaz-mm:master 272/379] lib/vsprintf.c:991:13: warning: variable 'modbuildid' set but not used

[Petr Mladek]

On Tue 2022-03-01 10:24:48, Andrew Morton wrote:
> On Tue, 1 Mar 2022 20:11:04 +0800 kernel test robot <lkp@intel.com> wrote:
> 
> > tree:   https://github.com/hnaz/linux-mm master
> > head:   a46912c14343fd3269cc133494988af90b377d9f
> > commit: b314f622e664eb263ea03ef7f4580e37146f123f [272/379] kallsyms: enhance %pS/s/b printing when KALLSYSMS is disabled
> > config: arm-eseries_pxa_defconfig (https://download.01.org/0day-ci/archive/20220301/202203012040.uFWGm3My-lkp@intel.com/config)
> > compiler: arm-linux-gnueabi-gcc (GCC) 11.2.0
> > reproduce (this is a W=1 build):
> >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >         chmod +x ~/bin/make.cross
> >         # https://github.com/hnaz/linux-mm/commit/b314f622e664eb263ea03ef7f4580e37146f123f
> >         git remote add hnaz-mm https://github.com/hnaz/linux-mm
> >         git fetch --no-tags hnaz-mm master
> >         git checkout b314f622e664eb263ea03ef7f4580e37146f123f
> >         # save the config file to linux build tree
> >         mkdir build_dir
> >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arm SHELL=/bin/bash
> > 
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot <lkp@intel.com>
> > 
> > All warnings (new ones prefixed by >>):
> > 
> >    lib/vsprintf.c: In function 'sprint_module_info':
> > >> lib/vsprintf.c:991:13: warning: variable 'modbuildid' set but not used [-Wunused-but-set-variable]
> >      991 |         int modbuildid = 0;
> >          |             ^~~~~~~~~~
> 
> Do we care about this?  [-Wunused-but-set-variable isn't normally set. 
> Under what circumstances does it get set in your setup?
> 
> I did this:
> 
> --- a/lib/vsprintf.c~kallsyms-enhance-%ps-s-b-printing-when-kallsysms-is-disabled-fix
> +++ a/lib/vsprintf.c
> @@ -988,7 +988,7 @@ static int sprint_module_info(char *buf,
>  	unsigned long base;
>  	int ret = 0;
>  	const char *modname;
> -	int modbuildid = 0;
> +	int modbuildid __maybe_unused = 0;
>  	int len;
>  #if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID)
>  	const unsigned char *buildid = NULL;

Looks good. I would fix this. I am not sure but I guess that there
are people that try to fix W=1 warnings.

That said, I would prefer to disable the patch "kallsyms: enhance
%pS/s/b printing when KALLSYSMS is disabled" for now. There are
two problems with it:

   + It discloses the base address of loaded modules. I am not sure
     if it is acceptable from the security point of view.

   + It duplicates a lot of code from kallsyms.c. I would like to
     avoid it.

I have pointed out both problems at
https://lore.kernel.org/r/YhzywNowPiQm3IN4@alley
I have to admit that they are hidden between less important comments.

Best Regards,
Petr