linux-kernel - Re: [PATCH v3 1/3] vdso: Switch get/put unaligned from packed struct to memcpy

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>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAP-5=fWeK4RnL8=BQm3o3u0KoONYEptwEYFBC5_DkJTbgpbx9g@mail.gmail.com>
Date: Tue, 22 Jul 2025 10:00:51 -0700
From: Ian Rogers <irogers@...gle.com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: kernel test robot <lkp@...el.com>, Eric Biggers <ebiggers@...gle.com>, Yuzhuo Jing <yuzhuo@...gle.com>, 
	Andy Lutomirski <luto@...nel.org>, Vincenzo Frascino <vincenzo.frascino@....com>, 
	Arnaldo Carvalho de Melo <acme@...hat.com>, Al Viro <viro@...iv.linux.org.uk>, 
	Christophe Leroy <christophe.leroy@...roup.eu>, "Jason A. Donenfeld" <Jason@...c4.com>, 
	linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org, 
	llvm@...ts.linux.dev, oe-kbuild-all@...ts.linux.dev
Subject: Re: [PATCH v3 1/3] vdso: Switch get/put unaligned from packed struct
 to memcpy

On Tue, Jul 22, 2025 at 9:44 AM Ian Rogers <irogers@...gle.com> wrote:
>
> On Tue, Jul 22, 2025 at 8:56 AM Thomas Gleixner <tglx@...utronix.de> wrote:
> >
> > Ian!
> >
> > On Sat, Jul 05 2025 at 08:05, kernel test robot wrote:
> > > kernel test robot noticed the following build warnings:
> > >
> > > [auto build test WARNING on linus/master]
> > > [also build test WARNING on tip/timers/vdso v6.16-rc4 next-20250704]
> > > [cannot apply to acme/perf/core]
> > > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > > And when submitting patch, we suggest to use '--base' as documented in
> > > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> > >
> > > url:    https://github.com/intel-lab-lkp/linux/commits/Ian-Rogers/vdso-Switch-get-put-unaligned-from-packed-struct-to-memcpy/20250626-135005
> > > base:   linus/master
> > > patch link:    https://lore.kernel.org/r/20250626054826.433453-2-irogers%40google.com
> > > patch subject: [PATCH v3 1/3] vdso: Switch get/put unaligned from packed struct to memcpy
> > > config: s390-randconfig-002-20250705 (https://download.01.org/0day-ci/archive/20250705/202507050736.b4hX0Xks-lkp@intel.com/config)
> > > compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project 61529d9e36fa86782a2458e6bdeedf7f376ef4b5)
> > > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250705/202507050736.b4hX0Xks-lkp@intel.com/reproduce)
> > >
> > > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > > the same patch/commit), kindly add following tags
> > > | Reported-by: kernel test robot <lkp@...el.com>
> > > | Closes: https://lore.kernel.org/oe-kbuild-all/202507050736.b4hX0Xks-lkp@intel.com/
> > >
> > > All warnings (new ones prefixed by >>):
> > >
> > >    In file included from arch/s390/boot/decompressor.c:48:
> > >    In file included from arch/s390/include/uapi/../../../../lib/decompress_unlz4.c:10:
> > >    In file included from arch/s390/include/uapi/../../../../lib/lz4/lz4_decompress.c:36:
> > >>> arch/s390/include/uapi/../../../../lib/lz4/lz4defs.h:109:9: warning: default initialization of an object of type 'typeof (*((const U16 *)ptr))' (aka 'const unsigned short') leaves the object uninitialized [-Wdefault-const-init-var-unsafe]
> > >      109 |         return get_unaligned((const U16 *)ptr);
> > >          |                ^
> >
> > Any update on this one?
>
> Hi Thomas!
>
> Thanks for the interest. I'm not sure how to resolve this.
> Basically the story in the code is:
>
>   get_unaligned((const U16 *)ptr);
>
> is being expanded by:
>
>   #define get_unaligned(ptr)      __get_unaligned_t(typeof(*(ptr)), (ptr))
>   #define __get_unaligned_t(type, ptr) ({
>           \
>        type __get_unaligned_ctrl_type __always_unused;                 \
>        __unqual_scalar_typeof(__get_unaligned_ctrl_type) __get_unaligned_val; \
>        __builtin_memcpy(&__get_unaligned_val, (void *)(ptr),           \
>                         sizeof(__get_unaligned_val));                  \
>        __get_unaligned_val;                                            \
>   })
>
> to:
>
>   ({
>   const U16 __get_unaligned_ctrl_type __always_unused;
>   __unqual_scalar_typeof(__get_unaligned_ctrl_type) __get_unaligned_val;
>   __builtin_memcpy(&__get_unaligned_val, (void *)(ptr),
>    sizeof(__get_unaligned_val));
>   __get_unaligned_val;
>   })
>
> which is then expanded to:
>
>   ({
>   const U16 __get_unaligned_ctrl_type __always_unused;
>   U16 __get_unaligned_val;
>   __builtin_memcpy(&__get_unaligned_val, (void *)(ptr),
>    sizeof(__get_unaligned_val));
>   __get_unaligned_val;
>   })
>
> so the analysis is correct that __get_unaligned_ctrl_type is
> unused, however, I've also put __always_unused on the variable.
> The purpose of that variable is for __unqual_scalar_typeof that
> passes the value to _Generic. I truly don't care about that
> variable but I care about it causing _Generic to get the correct
> type for __get_unaligned_val (that value can't be const as it is
> the destination of the memcpy). Why even declare
> __get_unaligned_ctrl_type? Well we want the result of __get_unaligned_t to
> match its type argument which needn't match the dereference of
> its pointer argument (*ptr) - I accept that in many cases it
> will. The value passed to _Generic is an expression and not a
> type, so declaring the variable and using the variable for
> _Generic allows this.
>
> My feeling is that my patch is correct and I'm not clear how to
> improve upon it. I believe it is a weakness in the analysis that
> the __always_unused isn't having an effect.
>
> Should adding new warnings for analysis on the code base be
> allowed for this patch? Well the upside to the patch is getting
> closer to not requiring -fno-strict-aliasing, or not introducing
> requiring that flag for things in the tools directory. I think
> the upside is good, I don't know how to mitigate the downside
> with poor analysis by certain tools.

Oh, the actual warning is "leaves the object uninitialized". It is
possible to silence this by changing:

  const U16 __get_unaligned_ctrl_type __always_unused;

to something like:

  const U16 __get_unaligned_ctrl_type __always_unused = 0;

You then get complained at that the code is using 0 instead of NULL
when instead of U16 the type of the __get_unaligned_t is a pointer.
Basically I've entered into an analysis tool wac-a-mole and I don't
have a combination to make them all happy.

Thanks,
Ian