Re: [PATCH v3 5/7] proc/task_mmu: change proc_get_vma() to stop returning gate VMA at the end

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Fri, Sep 11, 2026 at 12:18:41PM -0700, Suren Baghdasaryan wrote:
> On Fri, Sep 11, 2026 at 12:13 PM Lorenzo Stoakes (ARM) <ljs@kernel.org> wrote:
> >
> > On Fri, Sep 11, 2026 at 12:11:55PM -0700, Suren Baghdasaryan wrote:
> > > On Fri, Sep 11, 2026 at 12:03 PM Lorenzo Stoakes (ARM) <ljs@kernel.org> wrote:
> > > >
> > > > On Fri, Sep 11, 2026 at 07:26:49PM +0100, Lorenzo Stoakes (ARM) wrote:
> > > > > On Thu, Sep 10, 2026 at 04:47:35PM -0700, Suren Baghdasaryan wrote:
> > > > > > proc_get_vma() returning gate VMA at the end is desirable for the its
> > > > > > current m_start/m_next callers, as they need to report a gate VMA at the
> > > > > > end of the address space. This behavior is very specific to these callers
> > > > > > and makes proc_get_vma() hard to use for other purposes.
> > > > > >
> > > > > > Move this usage-specific behavior into the callers themselves so that
> > > > > > proc_get_vma() returns either a valid VMA, an error or a NULL when no
> > > > > > more VMAs are available. This makes it more generic, simpler and usable
> > > > > > in the later patches.
> > > > > >
> > > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > > >
> > > > > Yes, very good change, thanks!
> > > > >
> > > > > Reviewed-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
> > > > >
> > > > > > ---
> > > > > >  fs/proc/task_mmu.c | 23 ++++++++++++++++++-----
> > > > > >  1 file changed, 18 insertions(+), 5 deletions(-)
> > > > > >
> > > > > > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > > > > > index ecce7ce116cb..9a3c996c1d61 100644
> > > > > > --- a/fs/proc/task_mmu.c
> > > > > > +++ b/fs/proc/task_mmu.c
> > > > > > @@ -236,9 +236,6 @@ static struct vm_area_struct *proc_get_vma(struct seq_file *m, loff_t *ppos)
> > > > > >              * found the extended vma with the same vm_start.
> > > > > >              */
> > > > > >             *ppos = vma->vm_end;
> > > > > > -   } else {
> > > > > > -           *ppos = SENTINEL_VMA_GATE;
> > > > > > -           vma = get_gate_vma(priv->lock_ctx.mm);
> > > > >
> > > > > Yeah this is just so confusing as-was.
> > > > >
> > > > > >     }
> > > > > >
> > > > > >     return vma;
> > > > > > @@ -248,6 +245,7 @@ static void *m_start(struct seq_file *m, loff_t *ppos)
> > > > > >  {
> > > > > >     struct proc_maps_private *priv = m->private;
> > > > > >     struct proc_maps_locking_ctx *lock_ctx;
> > > > > > +   struct vm_area_struct *vma;
> > > > > >     loff_t last_addr = *ppos;
> > > > > >     struct mm_struct *mm;
> > > > > >
> > > > > > @@ -280,16 +278,31 @@ static void *m_start(struct seq_file *m, loff_t *ppos)
> > > > > >     if (last_addr == SENTINEL_VMA_GATE)
> > > > > >             return get_gate_vma(mm);
> > > > > >
> > > > > > -   return proc_get_vma(m, ppos);
> > > > > > +   vma = proc_get_vma(m, ppos);
> > > > > > +   if (vma)
> > > > > > +           return vma;
> > > > > > +
> > > > > > +   /* Return gate VMA at the end */
> > > > > > +   *ppos = SENTINEL_VMA_GATE;
> > > > > > +   return get_gate_vma(mm);
> > > > > >  }
> > > > > >
> > > > > >  static void *m_next(struct seq_file *m, void *v, loff_t *ppos)
> > > > > >  {
> > > > > > +   struct proc_maps_private *priv = m->private;
> > > > > > +   struct vm_area_struct *vma;
> > > > > > +
> > > > > >     if (*ppos == SENTINEL_VMA_GATE) {
> > > > > >             *ppos = SENTINEL_VMA_END;
> > > > > >             return NULL;
> > > > > >     }
> > > > > > -   return proc_get_vma(m, ppos);
> > > > > > +   vma = proc_get_vma(m, ppos);
> > > > > > +   if (vma)
> > > > > > +           return vma;
> > > > >
> > > > > OK so I guess the logic is, iterate through every VMA, then once you run out,
> > > > > report the gate VMA. Makes sense.
> > > >
> > > > Hmm one thing on this though - m_start() still has:
> > > >
> > > >         if (last_addr == SENTINEL_VMA_GATE)
> > > >                 return get_gate_vma(mm);
> > > >
> > > > As well as:
> > > >
> > > >         vma = proc_get_vma(m, ppos);
> > > >         if (vma)
> > > >                 return vma;
> > > >
> > > >         /* Return gate VMA at the end */
> > > >         *ppos = SENTINEL_VMA_GATE;
> > > >         return get_gate_vma(mm);
> > > >
> > > > Now at the end.
> > > >
> > > > Is this correct? Is it maybe duplicated now?
> > >
> > > Yeah, I noticed that too but I it's not duplication. The first check
> > > handles the case when right after m_next() hit the end of the address
> > > space and set *ppos = SENTINEL_VMA_GATE, we ran out of page space and
> > > had to flush its content. Once that's done, m_start will be called and
> > > last_addr will be set to SENTINEL_VMA_GATE. In that case we should
> > > return get_gate_vma() and avoid calling proc_get_vma(). That's what
> > > the first check for sentinel is doing. The second one handles the case
> > > when m_start() itself readches the end of the address space and has to
> > > return SENTINEL_VMA_GATE.
> > >
> > > It's possible this can be refactored a bit and made cleaner but I
> > > would keep that as a separate change.
> >
> > Maybe just add a comment to the first one to explain when it'll trigger? That
> > should suffice, cleanups can be separate yes.
>
> Will do. Appreciate the reviews!
>
> I'll post the update today because there is some urgency to backport
> these patches and I want to have a public link when backporting but I
> don't expect anyone to burn the midnight oil to review the final
> version. Have a nice weekend!

Well you have tags on most things from me now so it'll only be one patch I
think? That'll be missing mine anyway.

>
> >
> > >
> > > >
> > > > >
> > > > > > +
> > > > > > +   /* Return gate VMA at the end */
> > > > > > +   *ppos = SENTINEL_VMA_GATE;
> > > > > > +   return get_gate_vma(priv->lock_ctx.mm);
> > > > > >  }
> > > > > >
> > > > > >  static void m_stop(struct seq_file *m, void *v)
> > > > > > --
> > > > > > 2.55.0.1007.g17ff1f9808-goog
> > > > > >
> > > > >
> > > > > --
> > > > > Cheers, Lorenzo
> > > >
> > > > --
> > > > Cheers, Lorenzo
> >
> > --
> > Cheers, Lorenzo

--
Cheers, Lorenzo