Re: [PATCH 4/5] alloc_tag: Track fixed vs dynamic sized kmalloc calls

[Kees Cook <kees@kernel.org>]

On Thu, Aug 29, 2024 at 09:00:37AM -0700, Suren Baghdasaryan wrote:
> On Fri, Aug 9, 2024 at 12:33 AM Kees Cook <kees@kernel.org> wrote:
> [...]
> > -#define kmem_cache_alloc(...)                  alloc_hooks(kmem_cache_alloc_noprof(__VA_ARGS__))
> > +#define kmem_cache_alloc(...)          alloc_hooks(kmem_cache_alloc_noprof(__VA_ARGS__))
> 
> nit: seems like an unnecessary churn.

Whoops, yes. This was left over from an earlier pass and I failed to get
the whitespace correctly restored. I will fix this this.

> > diff --git a/lib/alloc_tag.c b/lib/alloc_tag.c
> > index 81e5f9a70f22..6d2cb72bf269 100644
> > --- a/lib/alloc_tag.c
> > +++ b/lib/alloc_tag.c
> > @@ -78,6 +78,14 @@ static void alloc_tag_to_text(struct seq_buf *out, struct codetag *ct)
> >
> >         seq_buf_printf(out, "%12lli %8llu ", bytes, counter.calls);
> >         codetag_to_text(out, ct);
> > +#ifdef CONFIG_SLAB_PER_SITE
> > +       seq_buf_putc(out, ' ');
> > +       seq_buf_printf(out, "size:%s(%zu) slab:%s",
> > +                               tag->meta.sized == 0 ? "non-slab" :
> 
> "non-slab" term sounds overly specific and we might extend this to
> some other allocations as well in the future. I would suggest
> "unknown" instead.

Heh, yeah. I went back and forth on the name for this and went with
non-slab because we do know what it isn't. It's not some kind of
unexpected state. Maybe "untracked", or "unsized", though both seem
inaccurate from certain perspectives.

> 
> > +                                       tag->meta.sized == SIZE_MAX ? "dynamic" : "fixed",
> > +                               tag->meta.sized == SIZE_MAX ? 0 : tag->meta.sized,
> > +                               tag->meta.cache ? "ready" : "unused");
> 
> I don't see "struct alloc_meta" having a "cache" member...

Oops, yes, as you found this should have been associated with the next
patch that adds "cache".

> Since you are changing the format of this file, you want to also bump
> up the file version inside print_allocinfo_header().

Okay, yeah. In that case I'll probably split the report into a separate
patch after "cache" is added so there's only a single bump in allocinfo
versioning.

-- 
Kees Cook