[PATCH] proc: nommu: fix empty /proc/<pid>/maps

[PATCH] proc: nommu: fix empty /proc/<pid>/maps

[Ben Wolsieffer]

On no-MMU, /proc/<pid>/maps reads as an empty file. This happens because
find_vma(mm, 0) always returns NULL (assuming no vma actually contains
the zero address, which is normally the case).

To fix this bug and improve the maintainability in the future, this
patch makes the no-MMU implementation as similar as possible to the MMU
implementation.

The only remaining differences are the lack of
hold/release_task_mempolicy and the extra code to shoehorn the gate vma
into the iterator.

This has been tested on top of 6.5.3 on an STM32F746.

Fixes: 0c563f148043 ("proc: remove VMA rbtree use from nommu")
Signed-off-by: Ben Wolsieffer <ben.wolsieffer@hefring.com>
---
 fs/proc/internal.h   |  2 --
 fs/proc/task_nommu.c | 37 ++++++++++++++++++++++---------------
 2 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index 9dda7e54b2d0..9a8f32f21ff5 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -289,9 +289,7 @@ struct proc_maps_private {
 	struct inode *inode;
 	struct task_struct *task;
 	struct mm_struct *mm;
-#ifdef CONFIG_MMU
 	struct vma_iterator iter;
-#endif
 #ifdef CONFIG_NUMA
 	struct mempolicy *task_mempolicy;
 #endif
diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
index 061bd3f82756..d3e19080df4a 100644
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -188,15 +188,28 @@ static int show_map(struct seq_file *m, void *_p)
 	return nommu_vma_show(m, _p);
 }
 
-static void *m_start(struct seq_file *m, loff_t *pos)
+static struct vm_area_struct *proc_get_vma(struct proc_maps_private *priv,
+						loff_t *ppos)
+{
+	struct vm_area_struct *vma = vma_next(&priv->iter);
+
+	if (vma) {
+		*ppos = vma->vm_start;
+	} else {
+		*ppos = -1UL;
+	}
+
+	return vma;
+}
+
+static void *m_start(struct seq_file *m, loff_t *ppos)
 {
 	struct proc_maps_private *priv = m->private;
+	unsigned long last_addr = *ppos;
 	struct mm_struct *mm;
-	struct vm_area_struct *vma;
-	unsigned long addr = *pos;
 
-	/* See m_next(). Zero at the start or after lseek. */
-	if (addr == -1UL)
+	/* See proc_get_vma(). Zero at the start or after lseek. */
+	if (last_addr == -1UL)
 		return NULL;
 
 	/* pin the task and mm whilst we play with them */
@@ -218,12 +231,9 @@ static void *m_start(struct seq_file *m, loff_t *pos)
 		return ERR_PTR(-EINTR);
 	}
 
-	/* start the next element from addr */
-	vma = find_vma(mm, addr);
-	if (vma)
-		return vma;
+	vma_iter_init(&priv->iter, mm, last_addr);
 
-	return NULL;
+	return proc_get_vma(priv, ppos);
 }
 
 static void m_stop(struct seq_file *m, void *v)
@@ -240,12 +250,9 @@ static void m_stop(struct seq_file *m, void *v)
 	priv->task = NULL;
 }
 
-static void *m_next(struct seq_file *m, void *_p, loff_t *pos)
+static void *m_next(struct seq_file *m, void *_p, loff_t *ppos)
 {
-	struct vm_area_struct *vma = _p;
-
-	*pos = vma->vm_end;
-	return find_vma(vma->vm_mm, vma->vm_end);
+	return proc_get_vma(m->private, ppos);
 }
 
 static const struct seq_operations proc_pid_maps_ops = {
-- 
2.42.0

Re: [PATCH] proc: nommu: fix empty /proc/<pid>/maps

[Liam R. Howlett]

* Ben Wolsieffer <ben.wolsieffer@hefring.com> [230915 12:05]:
> On no-MMU, /proc/<pid>/maps reads as an empty file. This happens because
> find_vma(mm, 0) always returns NULL (assuming no vma actually contains
> the zero address, which is normally the case).
> 
> To fix this bug and improve the maintainability in the future, this
> patch makes the no-MMU implementation as similar as possible to the MMU
> implementation.

The confusing find_vma() interface is even more confusing when nommu and
mmu versions have different meanings.  Perhaps the nommu should be made
the same?  This is almost certainly the source of the bug in the first
place.


Note that the nommu version uses addr in find_vma(mm, addr) as the start
address (and not the precise number) and searches upwards from there.


> 
> The only remaining differences are the lack of
> hold/release_task_mempolicy and the extra code to shoehorn the gate vma
> into the iterator.
> 
> This has been tested on top of 6.5.3 on an STM32F746.
> 
> Fixes: 0c563f148043 ("proc: remove VMA rbtree use from nommu")
> Signed-off-by: Ben Wolsieffer <ben.wolsieffer@hefring.com>
> ---
>  fs/proc/internal.h   |  2 --
>  fs/proc/task_nommu.c | 37 ++++++++++++++++++++++---------------
>  2 files changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> index 9dda7e54b2d0..9a8f32f21ff5 100644
> --- a/fs/proc/internal.h
> +++ b/fs/proc/internal.h
> @@ -289,9 +289,7 @@ struct proc_maps_private {
>  	struct inode *inode;
>  	struct task_struct *task;
>  	struct mm_struct *mm;
> -#ifdef CONFIG_MMU
>  	struct vma_iterator iter;
> -#endif
>  #ifdef CONFIG_NUMA
>  	struct mempolicy *task_mempolicy;
>  #endif
> diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
> index 061bd3f82756..d3e19080df4a 100644
> --- a/fs/proc/task_nommu.c
> +++ b/fs/proc/task_nommu.c
> @@ -188,15 +188,28 @@ static int show_map(struct seq_file *m, void *_p)
>  	return nommu_vma_show(m, _p);
>  }
>  
> -static void *m_start(struct seq_file *m, loff_t *pos)
> +static struct vm_area_struct *proc_get_vma(struct proc_maps_private *priv,
> +						loff_t *ppos)
> +{
> +	struct vm_area_struct *vma = vma_next(&priv->iter);
> +
> +	if (vma) {
> +		*ppos = vma->vm_start;
> +	} else {
> +		*ppos = -1UL;
> +	}
> +
> +	return vma;
> +}
> +
> +static void *m_start(struct seq_file *m, loff_t *ppos)
>  {
>  	struct proc_maps_private *priv = m->private;
> +	unsigned long last_addr = *ppos;
>  	struct mm_struct *mm;
> -	struct vm_area_struct *vma;
> -	unsigned long addr = *pos;
>  
> -	/* See m_next(). Zero at the start or after lseek. */
> -	if (addr == -1UL)
> +	/* See proc_get_vma(). Zero at the start or after lseek. */
> +	if (last_addr == -1UL)
>  		return NULL;
>  
>  	/* pin the task and mm whilst we play with them */
> @@ -218,12 +231,9 @@ static void *m_start(struct seq_file *m, loff_t *pos)
>  		return ERR_PTR(-EINTR);
>  	}
>  
> -	/* start the next element from addr */
> -	vma = find_vma(mm, addr);
> -	if (vma)
> -		return vma;
> +	vma_iter_init(&priv->iter, mm, last_addr);
>  
> -	return NULL;
> +	return proc_get_vma(priv, ppos);
>  }
>  
>  static void m_stop(struct seq_file *m, void *v)
> @@ -240,12 +250,9 @@ static void m_stop(struct seq_file *m, void *v)
>  	priv->task = NULL;
>  }
>  
> -static void *m_next(struct seq_file *m, void *_p, loff_t *pos)
> +static void *m_next(struct seq_file *m, void *_p, loff_t *ppos)
>  {
> -	struct vm_area_struct *vma = _p;
> -
> -	*pos = vma->vm_end;
> -	return find_vma(vma->vm_mm, vma->vm_end);
> +	return proc_get_vma(m->private, ppos);
>  }
>  
>  static const struct seq_operations proc_pid_maps_ops = {
> -- 
> 2.42.0
> 

Re: [PATCH] proc: nommu: fix empty /proc/<pid>/maps

[Matthew Wilcox]

On Fri, Sep 15, 2023 at 12:00:56PM -0400, Ben Wolsieffer wrote:
> On no-MMU, /proc/<pid>/maps reads as an empty file. This happens because
> find_vma(mm, 0) always returns NULL (assuming no vma actually contains
> the zero address, which is normally the case).

Your patch is correct, but this is a deeper problem.  find_vma() on
MMU architectures returns the first VMA which is >= addr.

 * Returns: The VMA associated with addr, or the next VMA.
 * May return %NULL in the case of no VMA at addr or above.

But that's not how find_vma() behaves on nommu!  And I'd be tempted to
blame the maple tree conversion, but this is how it looked before the
maple tree:

-       /* trawl the list (there may be multiple mappings in which addr
-        * resides) */
-       for (vma = mm->mmap; vma; vma = vma->vm_next) {
-               if (vma->vm_start > addr)
-                       return NULL;
-               if (vma->vm_end > addr) {
-                       vmacache_update(addr, vma);
-                       return vma;
-               }
-       }

So calling find_vma(0) always returned NULL.  Unless there was a VMA
at 0, which there probably wasn't.

Why does nommu behave differently?  Dave, you introduced it back in 2005
(yes, I had to go to the git history tree for this one)