Re: [PATCH] mm: move get_vma_name() from procfs to mm tree

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

I'm sorry to be difficult, but for me this is a no :(. I explain below.


On Sun, Sep 29, 2024 at 05:32:12PM GMT, qiwu.chen wrote:
> Commit acd4b2ecf3bb2 ("fs/procfs: extract logic for getting VMA name
> constituents") introduces a generic helper to get VMA name constituents.
> Currently, it's statically defined and referenced in fs/proc/task_mmu.c,
> which is not opened to users who want to query VMA name more efficiently.

I don't know what you mean by 'efficiently'? What is inefficient? This
seems more like a convenience function for debugging?

>
> This patch moves get_vma_name() from procfs to mm/mmap.c, and export it
> for modules usage.
>
> There should be no functional changes.

I'd call exporting an arbitrary VMA operation a functional change :) but
guess it's up for debate.


I'm not in favour of this patch as:

The VMA needs to be locked correctly for this to work and we plan to adjust
how that locking works over time, in fact relatively soon.

Exporting this as a module-available function means we are more constrained
in how we can proceed with such changes, even if officially we do not
guarantee internal kernel API/ABI stability.

Also, an 'exportable' version of this function would be one that asserted
the locking for you and did various more checks to make sure the
input/output parameters weren't trash, but again I don't think the
maintenance burden of making that available is really worth it.

Ideally I'd love VMAs to be an entirely internal implementation
detail. Sadly things like fault handlers for file systems prevent that. But
it very much minds me against exporting things any more than that.

So yeah, sorry to be difficult on something seemingly rather trivial, but
it's a no.

>
> Signed-off-by: qiwu.chen <qiwu.chen@transsion.com>

An aside, but I don't feel hugely comfortable with an email being sent from
one account and signed off by another... I'm not sure on our policy on that
though.

> ---
>  fs/proc/task_mmu.c | 61 --------------------------------------
>  include/linux/mm.h |  2 ++
>  mm/mmap.c          | 74 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 76 insertions(+), 61 deletions(-)
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 72f14fd59c2d..7d414c39367a 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/mm.h b/include/linux/mm.h
> index ecf63d2b0582..31abb857e11e 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -3418,6 +3418,8 @@ int expand_downwards(struct vm_area_struct *vma, unsigned long address);
>  extern struct vm_area_struct * find_vma(struct mm_struct * mm, unsigned long addr);
>  extern struct vm_area_struct * find_vma_prev(struct mm_struct * mm, unsigned long addr,
>  					     struct vm_area_struct **pprev);
> +extern void get_vma_name(struct vm_area_struct *vma, const struct path **path,
> +			const char **name, const char **name_fmt);

A nit (especially as I'm not in favour of this change) and you have no
reason why you'd be aware of this so it's fine but - generally when we add
functions here we don't include the extern keyword as it's unnecessary.

>
>  /*
>   * Look up the first VMA which intersects the interval [start_addr, end_addr)
> diff --git a/mm/mmap.c b/mm/mmap.c
> index ee8f91eaadb9..d6ca383fc302 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -995,6 +995,80 @@ find_vma_prev(struct mm_struct *mm, unsigned long addr,
>  	return vma;
>  }
>
> +/**
> + * get_vma_name() - get VMA name with relevant pieces of data.
> + * @vma: The vma to check
> + * @path: The file path given to the vma
> + * @name: The vma name
> + * @name_fmt: The formatted vma name
> + *
> + * Extract generic logic to fetch relevant pieces of data to describe
> + * VMA name. This could be just some string (either special constant or
> + * user-provided), or a string with some formatted wrapping text (e.g.,
> + * "[anon_shmem:<something>]"), or, commonly, file path.
> + */
> +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;
> +	}
> +}
> +EXPORT_SYMBOL(get_vma_name);

It's a convenience thing, so I'd have preferred an EXPORT_SYMBOL_GPL()
anyway. I have no desire to provide this kind of thing to binary blobs.

> +
>  /*
>   * Verify that the stack growth is acceptable and
>   * update accounting. This is shared with both the
> --
> 2.25.1
>
>