Re: [PATCH v3 03/14] x86: Add arch specific kasan functions

[Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>]

On 2025-04-04 at 09:06:51 -0700, Dave Hansen wrote:
>On 4/4/25 06:14, Maciej Wieczor-Retman wrote:
>> +static inline const void *__tag_set(const void *addr, u8 tag)
>> +{
>> +	u64 __addr = (u64)addr & ~__tag_shifted(KASAN_TAG_KERNEL);
>> +	return (const void *)(__addr | __tag_shifted(tag));
>> +}
>
>This becomes a lot clearer to read if you separate out the casting from
>the logical bit manipulation. For instance:
>
>static inline const void *__tag_set(const void *__addr, u8 tag)
>{
>	u64 addr = (u64)__addr;
>
>	addr &= ~__tag_shifted(KASAN_TAG_KERNEL);
>	addr |= __tag_shifted(tag);
>
>	return (const void *)addr;
>}
>
>Also, unless there's a good reason for it, you might as well limit the
>places you need to use "__".

Thanks, the above looks better :)

>
>Now that we can read this, I think it's potentially buggy. If someone
>went and changed:
>
>#define KASAN_TAG_KERNEL	0xFF
>
>to, say:
>
>#define KASAN_TAG_KERNEL	0xAB
>zo
>the '&' would miss clearing bits. It works fine in the arm64 implementation:
>
>	u64 __addr = (u64)addr & ~__tag_shifted(0xff);
>
>because they've hard-coded 0xff. I _think_ that's what you actually want
>here. You don't want to mask out KASAN_TAG_KERNEL, you actually want to
>mask out *ANYTHING* in those bits.
>
>So the best thing is probably to define a KASAN_TAG_MASK that makes it
>clear which are the tag bits.

Okay, that makes more sense. KASAN_TAG_MASK already exist ((1 << TAG_WIDTH) - 1)
in include/linux/mmzone.h. I'll move it to include/linux/kasan-tags.h so it can
be included.

-- 
Kind regards
Maciej Wieczór-Retman