Re: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault()

[Nathan Chancellor]

On Tue, Nov 30, 2021 at 10:25:43PM +1100, Michael Ellerman wrote:
> Christophe Leroy <christophe.leroy@xxxxxxxxxx> writes:
> > Le 29/11/2021 à 23:55, kernel test robot a écrit :
> >> Hi Christophe,
> >> 
> >> I love your patch! Perhaps something to improve:
> >> 
> >> [auto build test WARNING on powerpc/next]
> >> [also build test WARNING on v5.16-rc3 next-20211129]
> >> [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]
> >> 
> >> url:    https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
> >> base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
> >> config: powerpc-randconfig-r023-20211129 (https://download.01.org/0day-ci/archive/20211130/202111300652.0yDBNvyJ-lkp@xxxxxxxxx/config)
> >> compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project df08b2fe8b35cb63dfb3b49738a3494b9b4e6f8e)
> >> 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
> >>          # install powerpc cross compiling tool for clang build
> >>          # apt-get install binutils-powerpc-linux-gnu
> >>          # https://github.com/0day-ci/linux/commit/fb7bff30cc0efc7e4df1b48bb69de1f325eee826
> >>          git remote add linux-review https://github.com/0day-ci/linux
> >>          git fetch --no-tags linux-review Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
> >>          git checkout fb7bff30cc0efc7e4df1b48bb69de1f325eee826
> >>          # save the config file to linux build tree
> >>          mkdir build_dir
> >>          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc prepare
> >> 
> >> If you fix the issue, kindly add following tag as appropriate
> >> Reported-by: kernel test robot <lkp@xxxxxxxxx>
> >> 
> >> All warnings (new ones prefixed by >>):
> >> 
> >>     In file included from arch/powerpc/kernel/asm-offsets.c:71:
> >>     In file included from arch/powerpc/kernel/../xmon/xmon_bpts.h:7:
> >>>> arch/powerpc/include/asm/inst.h:165:20: warning: variable 'val' is uninitialized when used here [-Wuninitialized]
> >>                     *inst = ppc_inst(val);
> >>                                      ^~~
> >>     arch/powerpc/include/asm/inst.h:53:22: note: expanded from macro 'ppc_inst'
> >>     #define ppc_inst(x) (x)
> >>                          ^
> >>     arch/powerpc/include/asm/inst.h:155:18: note: initialize the variable 'val' to silence this warning
> >>             unsigned int val, suffix;
> >>                             ^
> >>                              = 0
> >
> > I can't understand what's wrong here.
> >
> > We have
> >
> > 	__get_kernel_nofault(&val, src, u32, Efault);
> > 	if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) {
> > 		__get_kernel_nofault(&suffix, src + 1, u32, Efault);
> > 		*inst = ppc_inst_prefix(val, suffix);
> > 	} else {
> > 		*inst = ppc_inst(val);
> > 	}
> >
> > With
> >
> > #define __get_kernel_nofault(dst, src, type, err_label)			\
> > 	__get_user_size_goto(*((type *)(dst)),				\
> > 		(__force type __user *)(src), sizeof(type), err_label)
> >
> >
> > And
> >
> > #define __get_user_size_goto(x, ptr, size, label)				\
> > do {										\
> > 	BUILD_BUG_ON(size > sizeof(x));						\
> > 	switch (size) {								\
> > 	case 1: __get_user_asm_goto(x, (u8 __user *)ptr, label, "lbz"); break;	\
> > 	case 2: __get_user_asm_goto(x, (u16 __user *)ptr, label, "lhz"); break;	\
> > 	case 4: __get_user_asm_goto(x, (u32 __user *)ptr, label, "lwz"); break;	\
> > 	case 8: __get_user_asm2_goto(x, (u64 __user *)ptr, label);  break;	\
> > 	default: x = 0; BUILD_BUG();						\
> > 	}									\
> > } while (0)
> >
> > And
> >
> > #define __get_user_asm_goto(x, addr, label, op)			\
> > 	asm_volatile_goto(					\
> > 		"1:	"op"%U1%X1 %0, %1	# get_user\n"	\
> > 		EX_TABLE(1b, %l2)				\
> > 		: "=r" (x)					\
> > 		: "m<>" (*addr)				\
> > 		:						\
> > 		: label)
> >
> >
> > I see no possibility, no alternative path where val wouldn't be set. The 
> > asm clearly has *addr as an output param so it is always set.
> 
> I guess clang can't convince itself of that?

A simplified reproducer:

$ cat test.c
static inline int copy_inst_from_kernel_nofault(unsigned int *inst,
                                                unsigned int *src)
{
        unsigned int val;

        asm goto("1: lwz %U1%X1 %0, %1   # get_user\n"
                 ".section __ex_table,\"a\";"
                 ".balign 4;"
                 ".long (1b) - . ;"
                 ".long (%l2) - . ;"
                 ".previous"
                 : "=r" (*(unsigned int *)(&val))
                 : "m<>" (*(unsigned int *)(src))
                 :
                 : Efault);

        *inst = val;
        return 0;

Efault:
        return -14; /* -EFAULT */
}

$ clang --target=powerpc-linux-gnu -Wuninitialized -fsyntax-only test.c
test.c:17:10: warning: variable 'val' is uninitialized when used here [-Wuninitialized]
        *inst = val;
                ^~~
test.c:4:18: note: initialize the variable 'val' to silence this warning
        unsigned int val;
                        ^
                         = 0
1 warning generated.

It certainly looks like there is something wrong with how clang is
tracking the initialization of the variable because it looks to me like
val is only used in the fallthrough path, which happens after it is
initialized via lwz.  Perhaps something is wrong with the logic of
https://reviews.llvm.org/D71314?  I've added Bill to CC (LLVM issues are
being migrated from Bugzilla to GitHub Issues right now so I cannot file
this upstream at the moment).

Cheers,
Nathan