linux-kernel - Re: [PATCH v9 13/17] mm/debug: print vm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJuCfpG=4b31T85Wer+s7vJF4WAO7Sd0TCXsDRzETL76AW5muA@mail.gmail.com>
Date: Mon, 13 Jan 2025 09:57:54 -0800
From: Suren Baghdasaryan <surenb@...gle.com>
To: "Liam R. Howlett" <Liam.Howlett@...cle.com>, Lorenzo Stoakes <lorenzo.stoakes@...cle.com>, 
	Suren Baghdasaryan <surenb@...gle.com>, akpm@...ux-foundation.org, peterz@...radead.org, 
	willy@...radead.org, david.laight.linux@...il.com, mhocko@...e.com, 
	vbabka@...e.cz, hannes@...xchg.org, mjguzik@...il.com, oliver.sang@...el.com, 
	mgorman@...hsingularity.net, david@...hat.com, peterx@...hat.com, 
	oleg@...hat.com, dave@...olabs.net, paulmck@...nel.org, brauner@...nel.org, 
	dhowells@...hat.com, hdanton@...a.com, hughd@...gle.com, 
	lokeshgidra@...gle.com, minchan@...gle.com, jannh@...gle.com, 
	shakeel.butt@...ux.dev, souravpanda@...gle.com, pasha.tatashin@...een.com, 
	klarasmodin@...il.com, richard.weiyang@...il.com, corbet@....net, 
	linux-doc@...r.kernel.org, linux-mm@...ck.org, linux-kernel@...r.kernel.org, 
	kernel-team@...roid.com
Subject: Re: [PATCH v9 13/17] mm/debug: print vm_refcnt state when dumping the vma

On Mon, Jan 13, 2025 at 8:35 AM Liam R. Howlett <Liam.Howlett@...cle.com> wrote:
>
> * Lorenzo Stoakes <lorenzo.stoakes@...cle.com> [250113 11:21]:
> > On Fri, Jan 10, 2025 at 08:26:00PM -0800, Suren Baghdasaryan wrote:
> > > vm_refcnt encodes a number of useful states:
> > > - whether vma is attached or detached
> > > - the number of current vma readers
> > > - presence of a vma writer
> > > Let's include it in the vma dump.
> > >
> > > Signed-off-by: Suren Baghdasaryan <surenb@...gle.com>
> > > Acked-by: Vlastimil Babka <vbabka@...e.cz>
> > > ---
> > >  mm/debug.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > >
> > > diff --git a/mm/debug.c b/mm/debug.c
> > > index 8d2acf432385..325d7bf22038 100644
> > > --- a/mm/debug.c
> > > +++ b/mm/debug.c
> > > @@ -178,6 +178,17 @@ EXPORT_SYMBOL(dump_page);
> > >
> > >  void dump_vma(const struct vm_area_struct *vma)
> > >  {
> > > +#ifdef CONFIG_PER_VMA_LOCK
> > > +   pr_emerg("vma %px start %px end %px mm %px\n"
> > > +           "prot %lx anon_vma %px vm_ops %px\n"
> > > +           "pgoff %lx file %px private_data %px\n"
> > > +           "flags: %#lx(%pGv) refcnt %x\n",
> > > +           vma, (void *)vma->vm_start, (void *)vma->vm_end, vma->vm_mm,
> > > +           (unsigned long)pgprot_val(vma->vm_page_prot),
> > > +           vma->anon_vma, vma->vm_ops, vma->vm_pgoff,
> > > +           vma->vm_file, vma->vm_private_data,
> > > +           vma->vm_flags, &vma->vm_flags, refcount_read(&vma->vm_refcnt));
> > > +#else
> > >     pr_emerg("vma %px start %px end %px mm %px\n"
> > >             "prot %lx anon_vma %px vm_ops %px\n"
> > >             "pgoff %lx file %px private_data %px\n"
> > > @@ -187,6 +198,7 @@ void dump_vma(const struct vm_area_struct *vma)
> > >             vma->anon_vma, vma->vm_ops, vma->vm_pgoff,
> > >             vma->vm_file, vma->vm_private_data,
> > >             vma->vm_flags, &vma->vm_flags);
> > > +#endif
> > >  }
> >
> > This is pretty horribly duplicative and not in line with how this kind of
> > thing is done in the rest of the file. You're just adding one entry, so why
> > not:
> >
> > void dump_vma(const struct vm_area_struct *vma)
> > {
> >       pr_emerg("vma %px start %px end %px mm %px\n"
> >               "prot %lx anon_vma %px vm_ops %px\n"
> >               "pgoff %lx file %px private_data %px\n"
> > #ifdef CONFIG_PER_VMA_LOCK
> >               "refcnt %x\n"
> > #endif
> >               "flags: %#lx(%pGv)\n",
> >               vma, (void *)vma->vm_start, (void *)vma->vm_end, vma->vm_mm,
> >               (unsigned long)pgprot_val(vma->vm_page_prot),
> >               vma->anon_vma, vma->vm_ops, vma->vm_pgoff,
> >               vma->vm_file, vma->vm_private_data,
> >               vma->vm_flags,
> > #ifdef CONFIG_PER_VMA_LOCK
> >               refcount_read(&vma->vm_refcnt),
> > #endif
> >               &vma->vm_flags);
> > }
>
> right, I had an issue with this as well.
>
> Another option would be:
>
>         pr_emerg("vma %px start %px end %px mm %px\n"
>                 "prot %lx anon_vma %px vm_ops %px\n"
>                 "pgoff %lx file %px private_data %px\n",
>                 <big mess here>);
>         dump_vma_refcnt();
>         pr_emerg("flags:...", vma_vm_flags);
>
>
> Then dump_vma_refcnt() either dumps the refcnt or does nothing,
> depending on the config option.
>
> Either way is good with me.  Lorenzo's suggestion is in line with the
> file and it's clear as to why the refcnt might be missing, but I don't
> really see this being an issue in practice.

Thanks for clarifying! Lorenzo's suggestion LGTM too. I'll adopt it. Thanks!

>
> Thanks,
> Liam
>