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

On Tue, Nov 30, 2021 at 10:38 AM Bill Wendling [off-list ref] wrote:

On Tue, Nov 30, 2021 at 10:17 AM Nathan Chancellor [off-list ref] wrote:

quoted

On Tue, Nov 30, 2021 at 10:25:43PM +1100, Michael Ellerman wrote:

quoted

Christophe Leroy [off-list ref] writes:

quoted

Le 29/11/2021 à 23:55, kernel test robot a écrit :

quoted

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@intel.com/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 <redacted>

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:

quoted

quoted

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).

If I remove the casts of "val" the warning doesn't appear. I suspect
that when I wrote that patch I forgot to remove those when checking.
#include "Captain_Picard_facepalm.h"

I'll look into it.

Small retraction. It's the "*(<cast>)&val" that's the issue. (I.e. the "*&")