Re: [PATCH 10/11] kasan: fix bug detection via ksize for HW_TAGS mode

On Tue, 12 Jan 2021 at 22:16, Andrey Konovalov [off-list ref] wrote:

On Tue, Jan 12, 2021 at 3:32 PM Marco Elver [off-list ref] wrote:

quoted

quoted

+/*
+ * Unlike kasan_check_read/write(), kasan_check_byte() is performed even for
+ * the hardware tag-based mode that doesn't rely on compiler instrumentation.
+ */

We have too many check-functions, and the name needs to be more precise.
Intuitively, I would have thought this should have access-type, i.e.
read or write, effectively mirroring a normal access.

Would kasan_check_byte_read() be better (and just not have a 'write'
variant because we do not need it)? This would restore ksize() closest
to what it was before (assuming reporting behaviour is fixed, too).

quoted

quoted

 void kasan_poison(const void *address, size_t size, u8 value);
 void kasan_unpoison(const void *address, size_t size);
-bool kasan_check_invalid_free(void *addr);
+bool kasan_check(const void *addr);

Definitely prefer shorted names, but we're in the unfortunate situation
of having numerous kasan_check-functions, so we probably need to be more
precise.

kasan_check() makes me think this also does reporting, but it does not
(it seems to only check the metadata for validity).

The internal function could therefore be kasan_check_allocated() (it's
now the inverse of kasan_check_invalid_free()).

Re: kasan_check_byte():

I think the _read suffix is only making the name longer. ksize() isn't
checking that the memory is readable (or writable), it's checking that
it's addressable. At least that's the intention of the annotation, so
it makes sense to name it correspondingly despite the implementation.

Having all kasan_check_*() functions both checking and reporting makes
sense, so let's keep the kasan_check_ prefix.

What isn't obvious from the name is that this function is present for
every kasan mode. Maybe kasan_check_byte_always()? Although it also
seems too long.

But I'm OK with keeping kasan_check_byte().

This is fine.

Re kasan_check():

Here we can use Andrew's suggestion about the name being related to
what the function returns. And also drop the kasan_check_ prefix as
this function only does the checking.

Let's use kasan_byte_accessible() instead of kasan_check().

Sounds reasonable to me.

Thanks,
-- Marco

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