RE: [PATCH v5 1/2] mm, kasan: improve double-free detection

["Luruo, Kuthonuzo" <kuthonuzo.luruo@hpe.com>]

> > Currently, KASAN may fail to detect concurrent deallocations of the same
> > object due to a race in kasan_slab_free(). This patch makes double-free
> > detection more reliable by serializing access to KASAN object metadata.
> > New functions kasan_meta_lock() and kasan_meta_unlock() are provided to
> > lock/unlock per-object metadata. Double-free errors are now reported via
> > kasan_report().
> >
> > Per-object lock concept from suggestion/observations by Dmitry Vyukov.
> Note I've sent out a patch that enables stackdepot support in SLUB.
> I'll probably need to wait till you patch lands and add locking to SLUB as well.

My patch can wait;  It can be rebased and resent for consideration by
maintainers/reviewers after your patch has been reviewed.

> > +void kasan_init_object(struct kmem_cache *cache, void *object)
> > +{
> > +       if (cache->flags & SLAB_KASAN) {
> > +               struct kasan_alloc_meta *allocp = get_alloc_info(cache, object);
> > +               union kasan_shadow_meta *shadow_meta =
> get_shadow_meta(allocp);
> > +
> > +               __memset(allocp, 0, sizeof(*allocp));
> I think we need initialize the lock first, then lock it in order to
> touch *allocp.
> > +               shadow_meta->data = KASAN_KMALLOC_META;
> Shouldn't this be a release store?

Object at this point is being initialized by SLAB off a newly allocated page/slab.
Concurrent access to the embryonic object is unlikely/not expected. I don't
think locking here is of any benefit...

> >                 default:
> > +                       pr_err("invalid allocation state!\n");
> I suggest you also print the object pointer here.

ok.

> > +
> >  struct kasan_alloc_meta {
> > +       u32 alloc_size : 24;
> Why reduce the alloc size?

Thought to save some bits while still accounting for max object size
instrumented by KASAN. But now, with the spectre of OOB writes,
header real estate suddenly seems a lot less valuable ;-). A reset to
u32 won’t be inappropriate. 

> >         if (!(cache->flags & SLAB_KASAN))
> >                 return;
> > -       switch (alloc_info->state) {
> > +       if (info->access_size)
> > +               kasan_meta_lock(alloc_info);
> In which case can info->access_size be zero? Guess even in that case
> we need to lock the metadata.

For a double-free error, access size is zero and lock is already held.

Thank you very much for reviewing the patch!

Kuthonuzo