On Thu, Feb 6, 2025 at 1:58 PM Maciej Wieczor-Retman <maciej.wieczor-retman@xxxxxxxxx> wrote: > > >Is there a reason these definitions are added to > >include/linux/kasan.h? At least within this patch, they are only used > >within mm/kasan, so let's keep them in mm/kasan/kasan.h. > > Parts of x86 arch use these later (minimal slab alignment, kasan shadow start > address) so I thought it was convenient to already have it in place here? AFAICT, KASAN_SHADOW_START only relies on KASAN_SHADOW_SCALE_SHIFT, which is defined arch/x86/include/asm/kasan.h anyway. And ARCH_SLAB_MINALIGN is defined in asm headers, so the definitions from include/linux/kasan.h shouldn't be visible to it? I think that we need to do is to define KASAN_GRANULE_SHIFT next to KASAN_SHADOW_SCALE_SHIFT for x86 and then use it in mm/kasan/kasan.h to define KASAN_GRANULE_SIZE for SW_TAGS. (Similarly as with arm64, where ARCH_SLAB_MINALIGN depends on either KASAN_SHADOW_SCALE_SHIFT or MTE_GRANULE_SIZE, both of which are defined in arm64 asm headers.) Btw, I think ARCH_SLAB_MINALIGN needs to be defined in include/asm/cache.h: at least all other architectures have it there. > Since I'll be reordering patches I can just move these changes together. Otherwise, if you need to expose something new in include/linux/kasan.h, please do it together with the change that uses it. Or you can even put it into a separate patch with an explanation of why it's required - at least from the review perspective having separate smaller patches is often better. In general, if something doesn't need to get exposed to the rest of the kernel, keep it in mm/kasan/kasan.h. > >I think this should also depend on KASAN_OUTLINE: Clang/GCC aren't > >aware of the dense mode. > > I wasn't sure I fully understood how inline/outline interacts with clang/gcc on > x86 (especially that I think some parts are still missing in x86 clang for > tag-based KASAN). So I understand that compiling with inline doesn't do > anything? If so, is it not doing anything because of missing compiler code or > something in the kernel? With inline instrumentation, the compiler directly embeds the instructions to calculate the shadow address and check the shadow value. Since the compiler assumes that one shadow byte corresponds to 16 bytes of memory and not 32, the generated instructions won't be compatible with the dense mode. With outline instrumentation, the compiler just adds function calls and thus all the shadow calculations are performed by the C code. Or did the dense mode work for you with KASAN_INLINE enabled? I would expect this not to work. Or maybe the inline instrumentation somehow got auto-disabled... > >Would it be possible to move this part to kasan_poison_last_granule()? > >That functions seems to be serving a similar purpose but for the > >Generic mode. > > > >It might also be cleaner to add a kasan_poison_first_granule() that > >contains the if (addr64 % KASAN_SHADOW_SCALE_SIZE) check. > ... > sure, I'll try to move these checks to kasan_poison_first/last_granule. For kasan_poison_last_granule(), I think the change makes sense. For kasan_poison_first_granule(), please check whether it gives any readability benefit - if kasan_poison() is the only caller, maybe adding another function is not worth it.
