Hi, Linus,
On Sat, Sep 9, 2023 at 3:48 AM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Fri, 8 Sept 2023 at 04:11, Huacai Chen <chenhuacai@xxxxxxxxxxx> 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
>
