On Wed, Apr 27, 2022 at 09:33:52AM +0200, Marco Elver wrote: > On Wed, 27 Apr 2022 at 09:11, <cgel.zte@xxxxxxxxx> wrote: > > > > From: xu xin <xu.xin16@xxxxxxxxxx> > > > > In __kfence_free(), the returned 'meta' from addr_to_metadata() > > might be NULL just as the implementation of addr_to_metadata() > > shows. > > > > Let's add a check of the pointer 'meta' to avoid NULL pointer > > dereference. The patch brings three changes: > > > > 1. Add checks in both kfence_free() and __kfence_free(); > > 2. kfence_free is not inline function any longer and new inline > > function '__try_free_kfence_meta' is introduced. > > This is very bad for performance (see below). > > > 3. The check of is_kfence_address() is not required for > > __kfence_free() now because __kfence_free has done the check in > > addr_to_metadata(); > > > > Reported-by: Zeal Robot <zealci@xxxxxxxxxx> > > Is this a static analysis robot? Please show a real stack trace with > an actual NULL-deref. > > Nack - please see: > https://lore.kernel.org/all/CANpmjNO5-o1B9r2eYS_482RBVJSyPoHSnV2t+M8fJdFzBf6d2A@xxxxxxxxxxxxxx/ > Thanks for your reply. It's from static analysis indeed and no actual NULL-deref event happened yet. I'm just worried that what if address at the edge of __kfence_pool and thus addr_to_metadata() returns NULL. Is is just a guess, I'm not sure. But if __kfence_free make sure that the given address never is at the edge of __kfence_pool, then the calculation and check in addr_to_metadata() is extra performance consumption: "index = (addr - (unsigned long)__kfence_pool) / (PAGE_SIZE * 2) - 1; if (index < 0 || index >= CONFIG_KFENCE_NUM_OBJECTS) 240 return NULL;" > >Signed-off-by: xu xin <xu.xin16@xxxxxxxxxx> > > --- > > include/linux/kfence.h | 10 ++-------- > > mm/kfence/core.c | 30 +++++++++++++++++++++++++++--- > > 2 files changed, 29 insertions(+), 11 deletions(-) > > > > diff --git a/include/linux/kfence.h b/include/linux/kfence.h > > index 726857a4b680..fbf6391ab53c 100644 > > --- a/include/linux/kfence.h > > +++ b/include/linux/kfence.h > > @@ -160,7 +160,7 @@ void *kfence_object_start(const void *addr); > > * __kfence_free() - release a KFENCE heap object to KFENCE pool > > * @addr: object to be freed > > * > > - * Requires: is_kfence_address(addr) > > + * Requires: is_kfence_address(addr), but now it's unnecessary > > (As an aside, something can't be required and be unnecessary at the same time.) Oh, I'm sorry for this. In my opinion, inner addr_to_metadata(), is_kfence_address is executed for the second time, so not necessary here.
