Re: [mm/slub] 555b8c8cb3: WARNING:at_lib/stackdepot.c:#stack_depot_fetch

[Hyeonggon Yoo]

On Wed, Apr 06, 2022 at 10:34:07AM +0200, Marco Elver wrote:
> On Wed, Apr 06, 2022 at 07:47AM +0900, Hyeonggon Yoo wrote:
> > On Tue, Apr 05, 2022 at 01:07:53PM +0200, Marco Elver wrote:
> > > On Tue, Apr 05, 2022 at 11:00AM +0900, Hyeonggon Yoo wrote:
> > > > On Mon, Apr 04, 2022 at 05:18:16PM +0200, Marco Elver wrote:
> > > > > On Mon, 4 Apr 2022 at 16:20, Vlastimil Babka <vbabka@xxxxxxx> wrote:
> > > [...]
> > > > > > But here we are in mem_dump_obj() -> kmem_dump_obj() -> kmem_obj_info().
> > > > > > Because kmem_valid_obj() returned true, fooled by folio_test_slab()
> > > > > > returning true because of the /* Set required slab fields. */ code.
> > > > > > Yet the illusion is not perfect and we read garbage instead of a valid
> > > > > > stackdepot handle.
> > > > > >
> > > > > > IMHO we should e.g. add the appropriate is_kfence_address() test into
> > > > > > kmem_valid_obj(), to exclude kfence-allocated objects? Sounds much simpler
> > > > > > than trying to extend the illusion further to make kmem_dump_obj() work?
> > > > > > Instead kfence could add its own specific handler to mem_dump_obj() to print
> > > > > > its debugging data?
> > > > > 
> > > > > I think this explanation makes sense!  Indeed, KFENCE already records
> > > > > allocation stacks internally anyway, so it should be straightforward
> > > > > to convince it to just print that.
> > > > >
> > > > 
> > > > Thank you both! Yeah the explanation makes sense... thats why KASAN/KCSAN couldn't yield anything -- it was not overwritten.
> > > > 
> > > > I'm writing a fix and will test if the bug disappears.
> > > > This may take few days.
> > >
> > 
> > I did check the bug is not reproduced after simple fix. (reproduced 0 of 373)
> > This approach was right.
> > 
> > > The below should fix it -- I'd like to make kmem_obj_info() do something
> > > useful for KFENCE objects.
> > >
> > 
> > Agreed.
> > 
> [...]
> > > +	i = get_stack_skipnr(track->stack_entries, track->num_stack_entries, NULL);
> > > +	for (j = 0; i < track->num_stack_entries && j < KS_ADDRS_COUNT - 1; ++i, ++j)
> > 
> > why KS_ADDRS_COUNT - 1 instead of KS_ADDRS_COUNT?
> 
> For `kp_stack[j] = NULL` because KFENCE's stack_entries does not have a
> NULL-delimiter (we have num_stack_entries). But it seems for kp_stack
> it's only added if `j < KS_ADDR_COUNT`, so I've fixed that.
>

Okay.

> > > +		kp_stack[j] = (void *)track->stack_entries[i];
> > > +	kp_stack[j] = NULL;
> [...]
> > > +	kpp->kp_objp = (void *)meta->addr;
> > > +
> > 
> > no need to take meta->lock here?
> 
> Yes, in case state is KFENCE_OBJECT_FREED there could be a race.
>

dumping object that is freed sounds like a bug?
but seems it's better to avoid a race in that case too.

> > > +	kfence_to_kp_stack(&meta->alloc_track, kpp->kp_stack);
> > > +	if (meta->state == KFENCE_OBJECT_FREED)
> > > +		kfence_to_kp_stack(&meta->free_track, kpp->kp_free_stack);
> > > +	/* get_stack_skipnr() ensures the first entry is outside allocator. */
> > > +	kpp->kp_ret = kpp->kp_stack[0];
> > > +
> > > +	return true;
> > > +}
> > 
> > kfence_kmem_obj_info() does not set kp_data_offset. kp_data_offset
> > may not be zero when e.g.) mem_dump_obj(&rhp->func); in rcutorture case. 
> 
> kp_data_offset is the offset e.g. when SLUB has added a redzone:
> 
> |		objp0 = kasan_reset_tag(object);
> |	#ifdef CONFIG_SLUB_DEBUG
> |		objp = restore_red_left(s, objp0);
> |	#else
> |		objp = objp0;
> |	#endif
> |		objnr = obj_to_index(s, slab, objp);
> |		kpp->kp_data_offset = (unsigned long)((char *)objp0 - (char *)objp);
> 
> In !CONFIG_SLUB_DEBUG and !(s->flags & SLAB_RED_ZONE) cases it's always
> 0, and otherwise it's
> 
> 	`objp0 - restore_red_left(objp0)` ==
> 	`object - (object - s->red_left_pad)` ==
> 	`s->red_left_pad`.
> 
> This matters if kp_objp is not the object start accessible by the user.
> But in the KFENCE case this is always the case so kp_data_offset=0.
>

Ah, right. confused "pointer offset" with "data offset".
it's always 0 in kfence case.

> > BTW, I would prefer implementing something like kfence_obj_info()
> > (called by kmem_dump_obj() and called instead of kmem_obj_info())
> > for better readability.
> 
> Hmm, I guess that saves us from having to fix up both slab.c/slub.c. But
> it makes kmem_obj_info() error-prone to use. What if someone calls
> kmem_obj_info() in future somewhere else? That caller then would have to
> remember to also call kfence_obj_info().
>

Valid point.

> I'd prefer fixing it as close to the root-cause (in kmem_obj_info()) to
> avoid that.
> 
> What do you prefer?

Then what about something like this?

kmem_obj_info(object) {
	if (object is from kfence)
		__kfence_obj_info()
	else
		__kmem_obj_info()
}

> 
> > And when mem_dump_obj() is called, I guess it's for debugging purpose.
> > I think it would be better to let user know the object is allocated
> > from kfence pool. maybe adding if (is_kfence_address(object)) pr_cont(" kfence");
> > in kmem_dump_obj() would be enough?
> 
> We can add that.
> 
> Thanks,
> -- Marco

-- 
Thanks,
Hyeonggon