linux-kernel - Re: [PATCH V2] f2fs: fix atgc bug on issue in 32bits platform

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Date:   Mon, 14 Nov 2022 11:17:48 +0100
From:   "Arnd Bergmann" <arnd@...db.de>
To:     "Zhiguo Niu" <niuzhiguo84@...il.com>
Cc:     "kernel test robot" <lkp@...el.com>,
        "zhiguo.niu" <zhiguo.niu@...soc.com>, jaegeuk@...nel.org,
        chao@...nel.org, linux-f2fs-devel@...ts.sourceforge.net,
        linux-kernel@...r.kernel.org, llvm@...ts.linux.dev,
        oe-kbuild-all@...ts.linux.dev
Subject: Re: [PATCH V2] f2fs: fix atgc bug on issue in 32bits platform

On Mon, Nov 14, 2022, at 10:23, Zhiguo Niu wrote:
> Arnd Bergmann <arnd@...db.de> 于2022年11月11日周五 17:57写道：

> so I just modify struct victim_entry as your suggestion:
> diff --git a/fs/f2fs/gc.h b/fs/f2fs/gc.h
> index 19b956c2d697..e2f25b8fd865 100644
> --- a/fs/f2fs/gc.h
> +++ b/fs/f2fs/gc.h
> @@ -56,16 +56,16 @@ struct gc_inode_list {
>  };
> 
>  struct victim_info {
> - unsigned long long mtime; /* mtime of section */
> - unsigned int segno; /* section No. */
> + unsigned long long mtime __packed; /* mtime of section */
> + unsigned int segno; /* section No. */
>  };
> 
>  struct victim_entry {
>   struct rb_node rb_node; /* rb node located in rb-tree */
>   union {
>   struct {
> - unsigned long long mtime; /* mtime of section */
> - unsigned int segno; /* segment No. */
> + unsigned long long mtime __packed; /* mtime of section */
> + unsigned int segno; /* segment No. */
>   };
>   struct victim_info vi; /* victim info */
>   };

Right, this should work. I'm still unsure where you need
a union inside of victim_entry rather than just having the
'victim_info' portion in there by itself, but that is not
a matter of correctness.

> There is no problem with functional verification in both 64bit and 
> 32bit platforms, 
> sorry I don't have the environment to verify is  build warnig reported 
> by the kernel test robot still there.

I had a bit trouble reproducing this as well. It looks like this
only happens when -Wunaligned-access is enabled for a config, but
that requires two things:

- building with CC=clang for a target architecture that does
  not support unaligned access natively, such as ARMv5.
  ARMv7 is interesting because it disables the warning, though
  it only supports unaligned load/store on 32-bit and 16-bit
  words but not 64-bit words using the ldrd/strd instructions.

- Even on architectures with no unaligned load/store, the
  warning is disabled by default unless you use "make W=1" to
  enable extra warnings.

Alternatively, you can enable the warning manually by passing
"CFLAGS_MODULE=-Wunaligned-access" to make, which should trigger
the warning on any 32-bit architecture.

      Arnd