On Wed, May 12, 2021 at 01:09PM -0700, Peter Collingbourne wrote:
[...]
> +void kasan_alloc_pages(struct page *page, unsigned int order, gfp_t flags);
> +void kasan_free_pages(struct page *page, unsigned int order);
> +
> #else /* CONFIG_KASAN_HW_TAGS */
>
> static inline bool kasan_enabled(void)
> {
> +#ifdef CONFIG_KASAN
> return true;
> +#else
> + return false;
> +#endif
> }
Just
return IS_ENABLED(CONFIG_KASAN);
> 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.
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?
The variants here would simply return kasan_has_integrated_init().
That way, there'd be no need for the BUILD_BUG()s and the interface
becomes harder to misuse by design.
Also, given that kasan_{alloc,free}_pages() initializes memory, this is
an opportunity to just give them a better name. Perhaps
/* Returns true if KASAN took care of initialization, false otherwise. */
bool __must_check kasan_alloc_pages_try_init(struct page *page, unsigned int order, gfp_t flags);
bool __must_check kasan_free_pages_try_init(struct page *page, unsigned int order);
[...]
> - init = want_init_on_free();
> - if (init && !kasan_has_integrated_init())
> - kernel_init_free_pages(page, 1 << order);
> - kasan_free_nondeferred_pages(page, order, init, fpi_flags);
> + if (kasan_has_integrated_init()) {
> + if (!skip_kasan_poison)
> + kasan_free_pages(page, order);
I think kasan_free_pages() could return a bool, and this would become
if (skip_kasan_poison || !kasan_free_pages(...)) {
...
> + } else {
> + bool init = want_init_on_free();
> +
> + if (init)
> + kernel_init_free_pages(page, 1 << order);
> + if (!skip_kasan_poison)
> + kasan_poison_pages(page, order, init);
> + }
>
> /*
> * arch_free_page() can make the page's contents inaccessible. s390
> @@ -2324,8 +2324,6 @@ static bool check_new_pages(struct page *page, unsigned int order)
> inline void post_alloc_hook(struct page *page, unsigned int order,
> gfp_t gfp_flags)
> {
> - bool init;
> -
> set_page_private(page, 0);
> set_page_refcounted(page);
>
> @@ -2344,10 +2342,16 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
> * kasan_alloc_pages and kernel_init_free_pages must be
> * kept together to avoid discrepancies in behavior.
> */
> - init = !want_init_on_free() && want_init_on_alloc(gfp_flags);
> - kasan_alloc_pages(page, order, init);
> - if (init && !kasan_has_integrated_init())
> - kernel_init_free_pages(page, 1 << order);
> + if (kasan_has_integrated_init()) {
> + kasan_alloc_pages(page, order, gfp_flags);
It looks to me that kasan_alloc_pages() could return a bool, and this
would become
if (!kasan_alloc_pages(...)) {
...
> + } else {
> + bool init =
> + !want_init_on_free() && want_init_on_alloc(gfp_flags);
> +
[ No need for line-break (for cases like this the kernel is fine with up
to 100 cols if it improves readability). ]
> + kasan_unpoison_pages(page, order, init);
> + if (init)
> + kernel_init_free_pages(page, 1 << order);
> + }
Thoughts?
Thanks,
-- Marco
