Re: [PATCH 3/3] kmsan: fix metadata loss during HugeTLB struct page reinitialization

[Chris Down <chris@chrisdown.name>]

Hi Muchun,

I can reproduce the crash and the series fixes it, and I think page_ext is
the right place for these pointers. I do have a few comments, though.

Muchun Song writes:
>-	struct page *kmsan_shadow;
>-	struct page *kmsan_origin;

This takes struct page from 80 bytes to 64 on a KMSAN kernel, which means
that hugetlb_vmemmap_optimizable_size() no longer returns 0 and HVO can be
used there for the first time. From a quick check with
hugetlb_free_vmemmap=on it seems to work, but nobody would guess that from
the subject, so I think the changelog should mention it.

Documentation/dev-tools/kmsan.rst also still describes the two pointers as
members of struct page, under "Metadata allocation", so that needs
updating as well.

>+ifdef CONFIG_KMSAN
>+KMSAN_SANITIZE_page_ext.o	:= n
>+KCOV_INSTRUMENT_page_ext.o	:= n
>+UBSAN_SANITIZE_page_ext.o	:= n
>+CFLAGS_page_ext.o		+= -DDISABLE_BRANCH_PROFILING
>+CFLAGS_REMOVE_page_ext.o	= $(CC_FLAGS_FTRACE)
>+endif

This takes the instrumentation away from all of page_ext.c, when the only
thing that needs it is the lookup. page_ext_size is already visible in
page_ext.h for page_ext_next(), so I think page_ext_lookup_raw() could be
a static inline there instead. That way it gets compiled into
mm/kmsan/shadow.c with the right flags already and needs none of the
finagling above, and it saves a call on a very hot path as well.

>+	preempt_disable_notrace();
>+	page_ext = page_ext_lookup_raw(page_to_pfn(page));
>+	if (page_ext)
>+		meta = *(struct page_metadata *)page_ext_data(page_ext, &kmsan_ops);
>+	preempt_enable_no_resched_notrace();

This is quite a lot more work than the two loads from struct page that it
replaces, and kmsan_get_shadow_origin_ptr() has to go through it twice
for every instrumented access to direct-mapped memory, because it calls
kmsan_get_metadata() once for the shadow and then again for the origin.
You might think that's a bit academic, but it does add up. For example, on
a 2 CPU VM, perf bench sched pipe -l 30000 goes from about 1.37s to about
1.58s with the series applied, perf bench syscall basic -l 300000 from
about 0.66s to about 0.75s, and a dd of 300000 blocks of 512 bytes from
/dev/zero to /dev/null from about 1.63s to about 1.90s.

1/3 says that keeping the pair together lets callers avoid duplicate
lookups, but the caller that matters most doesn't make use of that yet.
 From a quick hack that has kmsan_get_shadow_origin_ptr() look the page up
once and take both pointers from the result, the same three come out at
about 1.38s, 0.62s and 1.54s, which is no worse than before the series.
Could you do something like that here?

Thanks!

Chris