linux-kernel - Re: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <497d1646-3fd6-8acc-16f3-7abce672c97a@csgroup.eu>
Date:   Tue, 7 Dec 2021 07:55:43 +0000
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     Nathan Chancellor <nathan@...nel.org>
CC:     Michael Ellerman <mpe@...erman.id.au>,
        Bill Wendling <morbo@...gle.com>,
        kernel test robot <lkp@...el.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        "llvm@...ts.linux.dev" <llvm@...ts.linux.dev>,
        "kbuild-all@...ts.01.org" <kbuild-all@...ts.01.org>,
        "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 5/5] powerpc/inst: Optimise
 copy_inst_from_kernel_nofault()



Le 07/12/2021 à 07:41, Nathan Chancellor a écrit :
> On Tue, Dec 07, 2021 at 05:45:08AM +0000, Christophe Leroy wrote:
>>
>>
>> Le 07/12/2021 à 05:48, Nathan Chancellor a écrit :
>>> On Tue, Dec 07, 2021 at 02:37:26PM +1100, Michael Ellerman wrote:
>>>> Bill Wendling <morbo@...gle.com> writes:
>>>>> On Tue, Nov 30, 2021 at 10:38 AM Bill Wendling <morbo@...gle.com> wrote:
>>>>>> On Tue, Nov 30, 2021 at 10:17 AM Nathan Chancellor <nathan@...nel.org> wrote:
>>>>>>> On Tue, Nov 30, 2021 at 10:25:43PM +1100, Michael Ellerman wrote:
>>>>>>>> Christophe Leroy <christophe.leroy@...roup.eu> writes:
>>>>>>>>> Le 29/11/2021 à 23:55, kernel test robot a écrit :
>>>> ...
>>>>>>>>>> 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.
>>>> ...
>>>>>>>>>
>>>>>>>>> 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?
>>>> ...
>>>>>>>
>>>>>>> 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 "*&")
>>>>
>>>> I guess for now I'll just squash this in as a workaround?
>>>>
>>>>
>>>> diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
>>>> index 631436f3f5c3..5b591c51fec9 100644
>>>> --- a/arch/powerpc/include/asm/inst.h
>>>> +++ b/arch/powerpc/include/asm/inst.h
>>>> @@ -157,6 +157,9 @@ static inline int copy_inst_from_kernel_nofault(ppc_inst_t *inst, u32 *src)
>>>>    	if (unlikely(!is_kernel_addr((unsigned long)src)))
>>>>    		return -ERANGE;
>>>
>>> Could we add a version check to this and a link to our bug tracker:
>>>
>>> /* https://github.com/ClangBuiltLinux/linux/issues/1521 */
>>> #if defined(CONFIG_CC_IS_CLANG) && CONFIG_CLANG_VERSION < 140000
>>
>> The robot reported the problem on:
>>
>> compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project
>> df08b2fe8b35cb63dfb3b49738a3494b9b4e6f8e)
>>
>> Should it be CONFIG_CLANG_VERSION <= 140000 ?
> 
> The robot tests clang from tip of tree, rebuilding every week or so. The
> fix is getting ready to land so it will be released in 14.0.0 final. We
> have always written tip of tree version checks with the expectation that
> if people are testing tip of tree clang, they are frequently rebuilding.
> If that is not true, they need to be using released/stable versions,
> otherwise the model is broken.
> 
> If that is too problematic, we could add a version check to Kconfig
> (cannot think of a great name for the config off the top of my head)
> that checks for this issue and ifdef on that. That might be nice in
> case another instance of this crops up in the future.
> 

It's fine for me. I didn't know robot was using prereleases with the 
same name as the future release.

Christophe