linux-kernel - Re: [RFCv1 1/6] mm: Make get_vma

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: <CA+CK2bBqi7+RExARBq5m91kaxC+w+nLYnLf4wyM_MJjaxr2rAw@mail.gmail.com>
Date: Mon, 18 Nov 2024 15:40:57 -0500
From: Pasha Tatashin <pasha.tatashin@...een.com>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org, 
	linux-doc@...r.kernel.org, linux-fsdevel@...r.kernel.org, 
	cgroups@...r.kernel.org, linux-kselftest@...r.kernel.org, 
	akpm@...ux-foundation.org, corbet@....net, derek.kiernan@....com, 
	dragan.cvetic@....com, arnd@...db.de, gregkh@...uxfoundation.org, 
	viro@...iv.linux.org.uk, brauner@...nel.org, jack@...e.cz, tj@...nel.org, 
	hannes@...xchg.org, mhocko@...nel.org, roman.gushchin@...ux.dev, 
	shakeel.butt@...ux.dev, muchun.song@...ux.dev, Liam.Howlett@...cle.com, 
	vbabka@...e.cz, jannh@...gle.com, shuah@...nel.org, vegard.nossum@...cle.com, 
	vattunuru@...vell.com, schalla@...vell.com, david@...hat.com, 
	willy@...radead.org, osalvador@...e.de, usama.anjum@...labora.com, 
	andrii@...nel.org, ryan.roberts@....com, peterx@...hat.com, oleg@...hat.com, 
	tandersen@...flix.com, rientjes@...gle.com, gthelen@...gle.com
Subject: Re: [RFCv1 1/6] mm: Make get_vma_name() function public

On Mon, Nov 18, 2024 at 5:27 AM Lorenzo Stoakes
<lorenzo.stoakes@...cle.com> wrote:
>
> On Sat, Nov 16, 2024 at 05:59:17PM +0000, Pasha Tatashin wrote:
> > Page Detective will be using get_vma_name() that is currently used by
> > fs/proc to show names of VMAs in /proc/<pid>/smaps for example.
> >
> > Move this function to mm/vma.c, and make it accessible by modules.
>
> This is incorrect.
>
> mm/vma.c is for internal VMA implementation details, whose interface is
> explicitly mm/vma.h. This is so we can maintain the internal mechanism
> separate from interfaces and, importantly, are able to userland unit test
> VMA functionality.
>
> I think this _should_ be in mm/vma.c, but if it were to be exported it
> would need to be via a wrapper function in mm/mmap.c or somewhere like
> this.

Ok, I can do that in the next version.

>
> Also you broke the vma tests, go run make in tools/testing/vma/...

Hm interesting, I will take a look, this is surprising, as this patch
should not really change the behavior of anything. I guess it would be
because of the out of kernel vma.c build?

>
> Your patch also does not apply against Andrew's tree and the mm-unstable
> branch (i.e. against 6.13 in other words) which is what new mm patches
> should be based upon.
>
> Maybe I'll comment on the cover letter, but I don't agree you should be
> doing mm implementation details in a driver.
>
> The core of this should be in mm rather than exporting a bunch of stuff and
> have a driver do it. You're exposing internal implementation details
> unnecessarily.

This is not a problem, I will convert Page Detective to be in core mm.

> > @@ -3474,6 +3474,9 @@ void setattr_copy(struct mnt_idmap *, struct inode *inode,
> >
> >  extern int file_update_time(struct file *file);
> >
> > +void get_vma_name(struct vm_area_struct *vma, const struct path **path,
> > +               const char **name, const char **name_fmt);
> > +
>
> You're putting something in an mm/ C-file and the header in fs.h? Eh?

This is done so we do not have to include struct path into vma.h. fs.h
already has some vma functions like: vma_is_dax() and vma_is_fsdax().