[PATCH 00/43] Add KernelMemorySanitizer infrastructure

quoted

Just to make sure I don't misunderstand - for example for "kmsan: mm:
call KMSAN hooks from SLUB code", would it be better to pull the code
in mm/kmsan/core.c implementing kmsan_slab_alloc() and
kmsan_slab_free() into that patch?

Yes.

quoted

I thought maintainers would prefer to have patches to their code
separated from KMSAN code, but if it's not true, I can surely fix
that.

As a maintainer, I want to know what the function call that you just
added to my subsystem to call does.  Wouldn't you?  Put it all in the
same patch.

Ok, will be done in v2, thanks!

Think about submitting a patch series as telling a story.  You need to
show the progression forward of the feature so that everyone can
understand what is going on.  To just throw tiny snippets at us is
impossible to follow along with what your goal is.

You want reviewers to be able to easily see if the things you describe
being done in the changelog actually are implemented in the diff.
Dividing stuff up by files does not show that at all.

thanks,

greg k-h

-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Liana Sebastian
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Diese E-Mail ist vertraulich. Falls Sie diese fälschlicherweise
erhalten haben sollten, leiten Sie diese bitte nicht an jemand anderes
weiter, löschen Sie alle Kopien und Anhänge davon und lassen Sie mich
bitte wissen, dass die E-Mail an die falsche Person gesendet wurde.


This e-mail is confidential. If you received this communication by
mistake, please don't forward it to anyone else, please erase all
copies and attachments, and please let me know that it has gone to the
wrong person.

quoted

 static inline void *get_freepointer(struct kmem_cache *s, void *object)
 {
      object = kasan_reset_tag(object);
-     return freelist_dereference(s, object + s->offset);
+     return kmsan_init(freelist_dereference(s, object + s->offset));

... but I don't see why it applies to get_freepointer() too? What am I missing?

Agreed, kmsan_init() is not needed here.

quoted

 }

 static void prefetch_freepointer(const struct kmem_cache *s, void *object)

@@ -357,18 +361,28 @@ static void prefetch_freepointer(const struct kmem_cache *s, void *object)
      prefetchw(object + s->offset);
 }

+/*
+ * When running under KMSAN, get_freepointer_safe() may return an uninitialized
+ * pointer value in the case the current thread loses the race for the next
+ * memory chunk in the freelist. In that case this_cpu_cmpxchg_double() in
+ * slab_alloc_node() will fail, so the uninitialized value won't be used, but
+ * KMSAN will still check all arguments of cmpxchg because of imperfect
+ * handling of inline assembly.
+ * To work around this problem, use kmsan_init() to force initialize the
+ * return value of get_freepointer_safe().
+ */
 static inline void *get_freepointer_safe(struct kmem_cache *s, void *object)
 {
      unsigned long freepointer_addr;
      void *p;

      if (!debug_pagealloc_enabled_static())
-             return get_freepointer(s, object);
+             return kmsan_init(get_freepointer(s, object));

So here kmsan_init() is done twice?

Yeah, removing it from get_freepointer() does not introduce new
errors. I'll fix this in v2.