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.
Also you broke the vma tests, go run make in tools/testing/vma/...
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.
>
> Signed-off-by: Pasha Tatashin <pasha.tatashin@xxxxxxxxxx>
> ---
> fs/proc/task_mmu.c | 61 ----------------------------------------------
> include/linux/fs.h | 3 +++
> mm/vma.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 63 insertions(+), 61 deletions(-)
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index e52bd96137a6..b28c42b7a591 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -240,67 +240,6 @@ static int do_maps_open(struct inode *inode, struct file *file,
> sizeof(struct proc_maps_private));
> }
>
> -static void get_vma_name(struct vm_area_struct *vma,
> - const struct path **path,
> - const char **name,
> - const char **name_fmt)
> -{
> - struct anon_vma_name *anon_name = vma->vm_mm ? anon_vma_name(vma) : NULL;
> -
> - *name = NULL;
> - *path = NULL;
> - *name_fmt = NULL;
> -
> - /*
> - * Print the dentry name for named mappings, and a
> - * special [heap] marker for the heap:
> - */
> - if (vma->vm_file) {
> - /*
> - * If user named this anon shared memory via
> - * prctl(PR_SET_VMA ..., use the provided name.
> - */
> - if (anon_name) {
> - *name_fmt = "[anon_shmem:%s]";
> - *name = anon_name->name;
> - } else {
> - *path = file_user_path(vma->vm_file);
> - }
> - return;
> - }
> -
> - if (vma->vm_ops && vma->vm_ops->name) {
> - *name = vma->vm_ops->name(vma);
> - if (*name)
> - return;
> - }
> -
> - *name = arch_vma_name(vma);
> - if (*name)
> - return;
> -
> - if (!vma->vm_mm) {
> - *name = "[vdso]";
> - return;
> - }
> -
> - if (vma_is_initial_heap(vma)) {
> - *name = "[heap]";
> - return;
> - }
> -
> - if (vma_is_initial_stack(vma)) {
> - *name = "[stack]";
> - return;
> - }
> -
> - if (anon_name) {
> - *name_fmt = "[anon:%s]";
> - *name = anon_name->name;
> - return;
> - }
> -}
> -
> static void show_vma_header_prefix(struct seq_file *m,
> unsigned long start, unsigned long end,
> vm_flags_t flags, unsigned long long pgoff,
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 3559446279c1..a25b72397af5 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -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?
> static inline bool vma_is_dax(const struct vm_area_struct *vma)
> {
> return vma->vm_file && IS_DAX(vma->vm_file->f_mapping->host);
> diff --git a/mm/vma.c b/mm/vma.c
> index 7621384d64cf..1bd589fbc3c7 100644
> --- a/mm/vma.c
> +++ b/mm/vma.c
> @@ -2069,3 +2069,63 @@ void mm_drop_all_locks(struct mm_struct *mm)
>
> mutex_unlock(&mm_all_locks_mutex);
> }
> +
> +void get_vma_name(struct vm_area_struct *vma, const struct path **path,
> + const char **name, const char **name_fmt)
> +{
> + struct anon_vma_name *anon_name = vma->vm_mm ? anon_vma_name(vma) : NULL;
If we were to export this (I'm very dubious about that) I'd want to assert
some lock state and that the vma exists too.
Because we're just assuming the VMA won't disappear from under us and now
the driver will too, and also assuming we won't be passed NULL's...
But in general I'm not in favour of having this exported.
> +
> + *name = NULL;
> + *path = NULL;
> + *name_fmt = NULL;
> +
> + /*
> + * Print the dentry name for named mappings, and a
> + * special [heap] marker for the heap:
> + */
> + if (vma->vm_file) {
> + /*
> + * If user named this anon shared memory via
> + * prctl(PR_SET_VMA ..., use the provided name.
> + */
> + if (anon_name) {
> + *name_fmt = "[anon_shmem:%s]";
> + *name = anon_name->name;
> + } else {
> + *path = file_user_path(vma->vm_file);
> + }
> + return;
> + }
> +
> + if (vma->vm_ops && vma->vm_ops->name) {
> + *name = vma->vm_ops->name(vma);
> + if (*name)
> + return;
> + }
> +
> + *name = arch_vma_name(vma);
> + if (*name)
> + return;
> +
> + if (!vma->vm_mm) {
> + *name = "[vdso]";
> + return;
> + }
> +
> + if (vma_is_initial_heap(vma)) {
> + *name = "[heap]";
> + return;
> + }
> +
> + if (vma_is_initial_stack(vma)) {
> + *name = "[stack]";
> + return;
> + }
> +
> + if (anon_name) {
> + *name_fmt = "[anon:%s]";
> + *name = anon_name->name;
> + return;
> + }
> +}
> +EXPORT_SYMBOL_GPL(get_vma_name);
> --
> 2.47.0.338.g60cca15819-goog
>
