On Wed, 26 May 2021 at 21:28, Peter Collingbourne [off-list ref] wrote:
[...]

quoted

quoted

 static inline bool kasan_has_integrated_init(void)

@@ -113,8 +113,30 @@ static inline bool kasan_has_integrated_init(void)
      return false;
 }

+static __always_inline void kasan_alloc_pages(struct page *page,
+                                           unsigned int order, gfp_t flags)
+{
+     /* Only available for integrated init. */
+     BUILD_BUG();
+}
+
+static __always_inline void kasan_free_pages(struct page *page,
+                                          unsigned int order)
+{
+     /* Only available for integrated init. */
+     BUILD_BUG();
+}

This *should* always work, as long as the compiler optimizes everything
like we expect.

Yeah, as I mentioned to Catalin on an earlier revision I'm not a fan
of relying on the compiler optimizing this away, but it looks like
we're already relying on this elsewhere in the kernel.

That's true, and it's also how BUILD_BUG() works underneath (it calls
a  __attribute__((error(msg))) function guarded by a condition, or in
this case without a condition...  new code should usually use
static_assert() but that's obviously not possible here). In fact, if
the kernel is built without optimizations, BUILD_BUG() turns into
no-ops.

And just in case, I do not mind the BUILD_BUG(), because it should always work.

quoted

But: In this case, I think this is sign that the interface design can be
improved. Can we just make kasan_{alloc,free}_pages() return a 'bool
__must_check' to indicate if kasan takes care of init?

I considered a number of different approaches including something like
that before settling on the one in this patch. One consideration was
that we should avoid involving KASAN in normal execution as much as
possible, in order to make the normal code path as comprehensible as
possible. With an approach where alloc/free return a bool the reader
needs to understand what the KASAN alloc/free functions do in the
normal case. Whereas with an approach where an "accessor" function on
the KASAN side returns a bool, it's more obvious that the code has a
"normal path" and a "KASAN path", and readers who only care about the
normal path can ignore the KASAN path.

Does that make sense? I don't feel too strongly so I can change
alloc/free to return a bool if you don't agree.

If this had been considered, then that's fair. I just wanted to point
it out in case it hadn't.

Let's leave as-is.

I also just noticed that we also pass 'init' to kasan_poison_pages(..,
init) in the !kasan_has_integrated_init() case which might be
confusing.

Thanks,
-- Marco

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel