Re: [PATCH v3 1/2] kasan: catch invalid free before SLUB reinitializes the object

[Jann Horn <jannh@google.com>]

On Fri, Jul 26, 2024 at 2:43 AM Andrey Konovalov <andreyknvl@gmail.com> wrote:
> On Thu, Jul 25, 2024 at 5:32 PM Jann Horn <jannh@google.com> wrote:
> >
> > Currently, when KASAN is combined with init-on-free behavior, the
> > initialization happens before KASAN's "invalid free" checks.
> >
> > More importantly, a subsequent commit will want to use the object metadata
> > region to store an rcu_head, and we should let KASAN check that the object
> > pointer is valid before that. (Otherwise that change will make the existing
> > testcase kmem_cache_invalid_free fail.)
>
> This is not the case since v3, right?

Oh, you're right, this text is now wrong.

> Do we still need this patch?

I just tried removing this patch from the series; without it, the
kmem_cache_invalid_free kunit test fails because the kmem_cache_free()
no longer synchronously notices that the pointer is misaligned. I
guess I could change the testcase like this to make the tests pass
without this patch, but I'd like to hear from you or another KASAN
person whether you think that's a reasonable change:

diff --git a/mm/kasan/kasan_test.c b/mm/kasan/kasan_test.c
index cba782a4b072..f44b0dcb0e84 100644
--- a/mm/kasan/kasan_test.c
+++ b/mm/kasan/kasan_test.c
@@ -981,14 +981,21 @@ static void kmem_cache_invalid_free(struct kunit *test)
        if (!p) {
                kunit_err(test, "Allocation failed: %s\n", __func__);
                kmem_cache_destroy(cache);
                return;
        }

-       /* Trigger invalid free, the object doesn't get freed. */
-       KUNIT_EXPECT_KASAN_FAIL(test, kmem_cache_free(cache, p + 1));
+       /*
+        * Trigger invalid free, the object doesn't get freed.
+        * Note that the invalid free detection may happen asynchronously
+        * under CONFIG_SLUB_RCU_DEBUG.
+        */
+       KUNIT_EXPECT_KASAN_FAIL(test, ({
+               kmem_cache_free(cache, p + 1);
+               rcu_barrier();
+       }));

Being able to get rid of this patch would be a nice simplification, so
if you think asynchronous invalid-free detection for TYPESAFE_BY_RCU
slabs is fine, I'll happily throw it out.