On Wed, 26 May 2021 at 21:28, Peter Collingbourne <pcc@xxxxxxxxxx> wrote:
[...]
> > > 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.
> > 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
