[2/2] module: fix building stats for 32-bit targets

  From: Arnd Bergmann <arnd@arndb.de>

The new module statistics code mixes 64-bit types and wordsized 'long'
variables, which leads to build failures on 32-bit architectures:

kernel/module/stats.c: In function 'read_file_mod_stats':
kernel/module/stats.c:291:29: error: passing argument 1 of 'atomic64_read' from incompatible pointer type [-Werror=incompatible-pointer-types]
  291 |  total_size = atomic64_read(&total_mod_size);
x86_64-linux-ld: kernel/module/stats.o: in function `read_file_mod_stats':
stats.c:(.text+0x2b2): undefined reference to `__udivdi3'

To fix this, the code has to use one of the two types consistently.

Change them all to fixed-size 64-bit ones here.

Fixes: 0d4ab68ce983 ("module: add debug stats to help identify memory pressure")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
I have no idea if there is a risk of these variables actually
overflowing 'long' on 32-bit machines. If they provably can't, it
would be better to do the opposite patch.
---
 kernel/module/internal.h | 14 +++++------
 kernel/module/main.c     | 10 ++++----
 kernel/module/stats.c    | 50 ++++++++++++++++++++--------------------
 3 files changed, 37 insertions(+), 37 deletions(-)
  

On Tue, Apr 18, 2023 at 12:02:47AM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The new module statistics code mixes 64-bit types and wordsized 'long'
> variables, which leads to build failures on 32-bit architectures:

Doh. 0-day had not complained!

> kernel/module/stats.c: In function 'read_file_mod_stats':
> kernel/module/stats.c:291:29: error: passing argument 1 of 'atomic64_read' from incompatible pointer type [-Werror=incompatible-pointer-types]
>   291 |  total_size = atomic64_read(&total_mod_size);
> x86_64-linux-ld: kernel/module/stats.o: in function `read_file_mod_stats':
> stats.c:(.text+0x2b2): undefined reference to `__udivdi3'
> 
> To fix this, the code has to use one of the two types consistently.
> 
> Change them all to fixed-size 64-bit ones here.
> 
> Fixes: 0d4ab68ce983 ("module: add debug stats to help identify memory pressure")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> I have no idea if there is a risk of these variables actually
> overflowing 'long' on 32-bit machines. If they provably can't, it
> would be better to do the opposite patch.

I had originally used atomic64_t and added a debugfs knob for it but
Linus had advised against it because its not a stat we care too much
on 32-bit and atomic64 is nasty on 32-bit [0].

So I went with atomic_long and the cast becuase we're just reading.
 
Is there a way to fix this without doing the fully jump? If not oh well.

[0] https://lkml.kernel.org/r/CAHk-=whH+OsAY+9qLc9Hz+-W8u=dvD3NLWHemOQpZPcgZa52fA@mail.gmail.com

  Luis

> ---
>  kernel/module/internal.h | 14 +++++------
>  kernel/module/main.c     | 10 ++++----
>  kernel/module/stats.c    | 50 ++++++++++++++++++++--------------------
>  3 files changed, 37 insertions(+), 37 deletions(-)
> 
> diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> index 9ba5f8df15bc..c1710b74027c 100644
> --- a/kernel/module/internal.h
> +++ b/kernel/module/internal.h
> @@ -175,13 +175,13 @@ enum fail_dup_mod_reason {
>  
>  #ifdef CONFIG_MODULE_STATS
>  
> -#define mod_stat_add_long(count, var) atomic_long_add(count, var)
> +#define mod_stat_add_64(count, var) atomic64_add(count, var)
>  #define mod_stat_inc(name) atomic_inc(name)
>  
> -extern atomic_long_t total_mod_size;
> -extern atomic_long_t total_text_size;
> -extern atomic_long_t invalid_kread_bytes;
> -extern atomic_long_t invalid_decompress_bytes;
> +extern atomic64_t total_mod_size;
> +extern atomic64_t total_text_size;
> +extern atomic64_t invalid_kread_bytes;
> +extern atomic64_t invalid_decompress_bytes;
>  
>  extern atomic_t modcount;
>  extern atomic_t failed_kreads;
> @@ -189,7 +189,7 @@ extern atomic_t failed_decompress;
>  struct mod_fail_load {
>  	struct list_head list;
>  	char name[MODULE_NAME_LEN];
> -	atomic_long_t count;
> +	atomic64_t count;
>  	unsigned long dup_fail_mask;
>  };
>  
> @@ -199,7 +199,7 @@ void mod_stat_bump_becoming(struct load_info *info, int flags);
>  
>  #else
>  
> -#define mod_stat_add_long(name, var)
> +#define mod_stat_add_64(name, var)
>  #define mod_stat_inc(name)
>  
>  static inline int try_add_failed_module(const char *name, size_t len,
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 1ed373145278..d1b213310e4b 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -2600,8 +2600,8 @@ static noinline int do_init_module(struct module *mod)
>  	mutex_unlock(&module_mutex);
>  	wake_up_all(&module_wq);
>  
> -	mod_stat_add_long(text_size, &total_text_size);
> -	mod_stat_add_long(total_size, &total_mod_size);
> +	mod_stat_add_64(text_size, &total_text_size);
> +	mod_stat_add_64(total_size, &total_mod_size);
>  
>  	mod_stat_inc(&modcount);
>  
> @@ -3052,7 +3052,7 @@ SYSCALL_DEFINE3(init_module, void __user *, umod,
>  	err = copy_module_from_user(umod, len, &info);
>  	if (err) {
>  		mod_stat_inc(&failed_kreads);
> -		mod_stat_add_long(len, &invalid_kread_bytes);
> +		mod_stat_add_64(len, &invalid_kread_bytes);
>  		return err;
>  	}
>  
> @@ -3081,7 +3081,7 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs, int, flags)
>  				       READING_MODULE);
>  	if (len < 0) {
>  		mod_stat_inc(&failed_kreads);
> -		mod_stat_add_long(len, &invalid_kread_bytes);
> +		mod_stat_add_64(len, &invalid_kread_bytes);
>  		return len;
>  	}
>  
> @@ -3090,7 +3090,7 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs, int, flags)
>  		vfree(buf); /* compressed data is no longer needed */
>  		if (err) {
>  			mod_stat_inc(&failed_decompress);
> -			mod_stat_add_long(len, &invalid_decompress_bytes);
> +			mod_stat_add_64(len, &invalid_decompress_bytes);
>  			return err;
>  		}
>  	} else {
> diff --git a/kernel/module/stats.c b/kernel/module/stats.c
> index bbf90190a3fe..c4db7f64cdae 100644
> --- a/kernel/module/stats.c
> +++ b/kernel/module/stats.c
> @@ -196,12 +196,12 @@ static LIST_HEAD(dup_failed_modules);
>   *    a separate module request was being issued for each CPU on a system.
>   */
>  
> -atomic_long_t total_mod_size;
> -atomic_long_t total_text_size;
> -atomic_long_t invalid_kread_bytes;
> -atomic_long_t invalid_decompress_bytes;
> -static atomic_long_t invalid_becoming_bytes;
> -static atomic_long_t invalid_mod_bytes;
> +atomic64_t total_mod_size;
> +atomic64_t total_text_size;
> +atomic64_t invalid_kread_bytes;
> +atomic64_t invalid_decompress_bytes;
> +static atomic64_t invalid_becoming_bytes;
> +static atomic64_t invalid_mod_bytes;
>  atomic_t modcount;
>  atomic_t failed_kreads;
>  atomic_t failed_decompress;
> @@ -222,21 +222,21 @@ static const char *mod_fail_to_str(struct mod_fail_load *mod_fail)
>  
>  void mod_stat_bump_invalid(struct load_info *info, int flags)
>  {
> -	atomic_long_add(info->len * 2, &invalid_mod_bytes);
> +	atomic64_add(info->len * 2, &invalid_mod_bytes);
>  	atomic_inc(&failed_load_modules);
>  #if defined(CONFIG_MODULE_DECOMPRESS)
>  	if (flags & MODULE_INIT_COMPRESSED_FILE)
> -		atomic_long_add(info->compressed_len, &invalid_mod_byte);
> +		atomic64_add(info->compressed_len, &invalid_mod_bytes);
>  #endif
>  }
>  
>  void mod_stat_bump_becoming(struct load_info *info, int flags)
>  {
>  	atomic_inc(&failed_becoming);
> -	atomic_long_add(info->len, &invalid_becoming_bytes);
> +	atomic64_add(info->len, &invalid_becoming_bytes);
>  #if defined(CONFIG_MODULE_DECOMPRESS)
>  	if (flags & MODULE_INIT_COMPRESSED_FILE)
> -		atomic_long_add(info->compressed_len, &invalid_becoming_bytes);
> +		atomic64_add(info->compressed_len, &invalid_becoming_bytes);
>  #endif
>  }
>  
> @@ -247,7 +247,7 @@ int try_add_failed_module(const char *name, size_t len, enum fail_dup_mod_reason
>  	list_for_each_entry_rcu(mod_fail, &dup_failed_modules, list,
>  				lockdep_is_held(&module_mutex)) {
>  		if (strlen(mod_fail->name) == len && !memcmp(mod_fail->name, name, len)) {
> -                        atomic_long_inc(&mod_fail->count);
> +                        atomic64_inc(&mod_fail->count);
>  			__set_bit(reason, &mod_fail->dup_fail_mask);
>                          goto out;
>                  }
> @@ -258,7 +258,7 @@ int try_add_failed_module(const char *name, size_t len, enum fail_dup_mod_reason
>  		return -ENOMEM;
>  	memcpy(mod_fail->name, name, len);
>  	__set_bit(reason, &mod_fail->dup_fail_mask);
> -        atomic_long_inc(&mod_fail->count);
> +        atomic64_inc(&mod_fail->count);
>          list_add_rcu(&mod_fail->list, &dup_failed_modules);
>  out:
>  	return 0;
> @@ -331,12 +331,12 @@ static ssize_t read_file_mod_stats(struct file *file, char __user *user_buf,
>  
>  	if (live_mod_count && total_size) {
>  		len += scnprintf(buf + len, size - len, "%25s\t%llu\n", "Average mod size",
> -				 DIV_ROUND_UP(total_size, live_mod_count));
> +				 DIV64_U64_ROUND_UP(total_size, live_mod_count));
>  	}
>  
>  	if (live_mod_count && text_size) {
>  		len += scnprintf(buf + len, size - len, "%25s\t%llu\n", "Average mod text size",
> -				 DIV_ROUND_UP(text_size, live_mod_count));
> +				 DIV64_U64_ROUND_UP(text_size, live_mod_count));
>  	}
>  
>  	/*
> @@ -349,25 +349,25 @@ static ssize_t read_file_mod_stats(struct file *file, char __user *user_buf,
>  	WARN_ON_ONCE(ikread_bytes && !fkreads);
>  	if (fkreads && ikread_bytes) {
>  		len += scnprintf(buf + len, size - len, "%25s\t%llu\n", "Avg fail kread bytes",
> -				 DIV_ROUND_UP(ikread_bytes, fkreads));
> +				 DIV64_U64_ROUND_UP(ikread_bytes, fkreads));
>  	}
>  
>  	WARN_ON_ONCE(ibecoming_bytes && !fbecoming);
>  	if (fbecoming && ibecoming_bytes) {
>  		len += scnprintf(buf + len, size - len, "%25s\t%llu\n", "Avg fail becoming bytes",
> -				 DIV_ROUND_UP(ibecoming_bytes, fbecoming));
> +				 DIV64_U64_ROUND_UP(ibecoming_bytes, fbecoming));
>  	}
>  
>  	WARN_ON_ONCE(idecompress_bytes && !fdecompress);
>  	if (fdecompress && idecompress_bytes) {
>  		len += scnprintf(buf + len, size - len, "%25s\t%llu\n", "Avg fail decomp bytes",
> -				 DIV_ROUND_UP(idecompress_bytes, fdecompress));
> +				 DIV64_U64_ROUND_UP(idecompress_bytes, fdecompress));
>  	}
>  
>  	WARN_ON_ONCE(imod_bytes && !floads);
>  	if (floads && imod_bytes) {
>  		len += scnprintf(buf + len, size - len, "%25s\t%llu\n", "Average fail load bytes",
> -				 DIV_ROUND_UP(imod_bytes, floads));
> +				 DIV64_U64_ROUND_UP(imod_bytes, floads));
>  	}
>  
>  	/* End of our debug preamble header. */
> @@ -407,16 +407,16 @@ static const struct file_operations fops_mod_stats = {
>  	.llseek = default_llseek,
>  };
>  
> -#define mod_debug_add_ulong(name) debugfs_create_ulong(#name, 0400, mod_debugfs_root, (unsigned long *) &name.counter)
> +#define mod_debug_add_u64(name) debugfs_create_u64(#name, 0400, mod_debugfs_root, (s64 *)&name.counter)
>  #define mod_debug_add_atomic(name) debugfs_create_atomic_t(#name, 0400, mod_debugfs_root, &name)
>  static int __init module_stats_init(void)
>  {
> -	mod_debug_add_ulong(total_mod_size);
> -	mod_debug_add_ulong(total_text_size);
> -	mod_debug_add_ulong(invalid_kread_bytes);
> -	mod_debug_add_ulong(invalid_decompress_bytes);
> -	mod_debug_add_ulong(invalid_becoming_bytes);
> -	mod_debug_add_ulong(invalid_mod_bytes);
> +	mod_debug_add_u64(total_mod_size);
> +	mod_debug_add_u64(total_text_size);
> +	mod_debug_add_u64(invalid_kread_bytes);
> +	mod_debug_add_u64(invalid_decompress_bytes);
> +	mod_debug_add_u64(invalid_becoming_bytes);
> +	mod_debug_add_u64(invalid_mod_bytes);
>  
>  	mod_debug_add_atomic(modcount);
>  	mod_debug_add_atomic(failed_kreads);
> -- 
> 2.39.2
>
  

On Tue, Apr 18, 2023, at 00:15, Luis Chamberlain wrote:
> On Tue, Apr 18, 2023 at 12:02:47AM +0200, Arnd Bergmann wrote:
>> I have no idea if there is a risk of these variables actually
>> overflowing 'long' on 32-bit machines. If they provably can't, it
>> would be better to do the opposite patch.
>
> I had originally used atomic64_t and added a debugfs knob for it but
> Linus had advised against it because its not a stat we care too much
> on 32-bit and atomic64 is nasty on 32-bit [0].
>
> So I went with atomic_long and the cast becuase we're just reading.
> 
> Is there a way to fix this without doing the fully jump? If not oh well.

I've sent a v2 now that does it the other way round, which is
clearly much more efficient. Have only done minimal build testing
so far, but it passes the randconfigs that failed before.

     Arnd
  

On Tue, Apr 18, 2023 at 12:49:29AM +0200, Arnd Bergmann wrote:
> On Tue, Apr 18, 2023, at 00:15, Luis Chamberlain wrote:
> > On Tue, Apr 18, 2023 at 12:02:47AM +0200, Arnd Bergmann wrote:
> >> I have no idea if there is a risk of these variables actually
> >> overflowing 'long' on 32-bit machines. If they provably can't, it
> >> would be better to do the opposite patch.
> >
> > I had originally used atomic64_t and added a debugfs knob for it but
> > Linus had advised against it because its not a stat we care too much
> > on 32-bit and atomic64 is nasty on 32-bit [0].
> >
> > So I went with atomic_long and the cast becuase we're just reading.
> > 
> > Is there a way to fix this without doing the fully jump? If not oh well.
> 
> I've sent a v2 now that does it the other way round, which is
> clearly much more efficient. Have only done minimal build testing
> so far, but it passes the randconfigs that failed before.

Thanks! You're too fast.

  Luis