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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <frtb77uu5k6low4zub25h4eqkg5jm4rnul3ncrlniwjrgh7vbb@szwcka2qivp7>
Date: Mon, 13 Jan 2025 11:35:04 -0500
From: "Liam R. Howlett" <Liam.Howlett@...cle.com>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Cc: 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

* 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,
Liam