linux-kernel - Re: [GIT PULL] LoongArch changes for v6.6

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: <CAAhV-H64=ZWBnzFmtS-FMB83nn21DSm20adrXdsrYkoka_oyjw@mail.gmail.com>
Date:   Sat, 9 Sep 2023 16:05:44 +0800
From:   Huacai Chen <chenhuacai@...nel.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Huacai Chen <chenhuacai@...ngson.cn>,
        Arnd Bergmann <arnd@...db.de>, loongarch@...ts.linux.dev,
        linux-arch@...r.kernel.org, linux-kernel@...r.kernel.org,
        Guo Ren <guoren@...nel.org>, Xuerui Wang <kernel@...0n.name>,
        Jiaxun Yang <jiaxun.yang@...goat.com>
Subject: Re: [GIT PULL] LoongArch changes for v6.6

Hi, Linus,

On Sat, Sep 9, 2023 at 3:48 AM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> On Fri, 8 Sept 2023 at 04:11, Huacai Chen <chenhuacai@...ngson.cn> wrote:
> >
> > 7, Add KASAN (Kernel Address Sanitizer) support
>
> I have pulled this, but please at least consider
>
>  (a) don't use the stupid and random __HAVE_ARCH_xyz defines
>
> Yes, yes, we have historical uses of it. That doesn't make it good.
> Instead of making up new random symbol names, just *USE* the names you
> are defining per architecture.
>
> IOW, instead of doing
>
>   #define __HAVE_ARCH_SHADOW_MAP
>
> and defining your own helper replacement functions for
> kasan_mem_to_shadow() etc, just use those names as-is.
>
> So if you have an architecture that has its own version of
> "kasan_mem_to_shadow()", then use *THAT* name for the #ifdef too.
> Don't make up another name entirely of the form "__HAVE_ARCH_xyz".
>
> Example: architectures can override the default generic versions of
> "arch_atomic_xyz()" operations, and the way you do that is (for
> example)
>
>   static __always_inline int arch_atomic_add_return(int i, atomic_t *v)
>   {
>         return i + xadd(&v->counter, i);
>   }
>   #define arch_atomic_add_return arch_atomic_add_return
>
> note how the define to let you know the name exists is the name itself
> (and because the implementation is an inline function, not the macro,
> the marker is then just a "define the name to itself").
>
> End result: no made-up secondary names for other things. When you grep
> for the thing that is used, you find both the implementation and the
> marker for "look, I am overriding it".
>
> And also
>
>  (b) do you really want to inline those kasan_mem_to_shadow() and
> kasan_shadow_to_mem() things?
>
> Yes, the caller is addr_has_metadata(), and that one is
> "__always_inline",. because otherwise objtool would complain about
> doing function calls in SMAP-enabled regions.
>
> HOWEVER:
>
>  - on LoongArch, I don't think you have that objtool / SMAP issue
>
>  - and if  you did, the function should be __always_inline, not just
> plain inline anyway
>
> so I get the feeling that the inline is simply wrong either way. Maybe
> that function should just be declared, with the implementation being
> out-of-line? It seems unnecessarily big to be an inline function, and
> it doesn't seem performance-critical?
>
> Neither of the above issues are critical, and the second one in
> particular really is just a "did you really mean to do that" kind of
> reaction, but since I was looking at this, I decided to write up my
> reactions.
Thank you for your suggestions, I will make cleanup patches for the
two issues before v6.6 is released.

Huacai
>
>               Linus
>