On Wed, Aug 6, 2025 at 11:04 AM Lorenzo Stoakes
<lorenzo.stoakes@xxxxxxxxxx> wrote:
>
> On Wed, Aug 06, 2025 at 08:59:03AM -0700, Suren Baghdasaryan wrote:
> > Refactor struct proc_maps_private so that the fields used by PROCMAP_QUERY
> > ioctl are moved into a separate structure. In the next patch this allows
> > ioctl to reuse some of the functions used for reading /proc/pid/maps
> > without using file->private_data. This prevents concurrent modification
> > of file->private_data members by ioctl and /proc/pid/maps readers.
> >
> > The change is pure code refactoring and has no functional changes.
> >
> > Signed-off-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>
> > Reviewed-by: Vlastimil Babka <vbabka@xxxxxxx>
> > ---
> > fs/proc/internal.h | 15 ++++++----
> > fs/proc/task_mmu.c | 70 ++++++++++++++++++++++----------------------
> > fs/proc/task_nommu.c | 14 ++++-----
> > 3 files changed, 52 insertions(+), 47 deletions(-)
> >
> > diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> > index e737401d7383..d1598576506c 100644
> > --- a/fs/proc/internal.h
> > +++ b/fs/proc/internal.h
> > @@ -378,16 +378,21 @@ extern void proc_self_init(void);
> > * task_[no]mmu.c
> > */
> > struct mem_size_stats;
> > -struct proc_maps_private {
> > - struct inode *inode;
> > - struct task_struct *task;
> > +
> > +struct proc_maps_locking_ctx {
>
> Decent name :)
>
> > struct mm_struct *mm;
> > - struct vma_iterator iter;
> > - loff_t last_pos;
> > #ifdef CONFIG_PER_VMA_LOCK
> > bool mmap_locked;
> > struct vm_area_struct *locked_vma;
> > #endif
> > +};
> > +
> > +struct proc_maps_private {
> > + struct inode *inode;
> > + struct task_struct *task;
> > + struct vma_iterator iter;
> > + loff_t last_pos;
> > + struct proc_maps_locking_ctx lock_ctx;
> > #ifdef CONFIG_NUMA
> > struct mempolicy *task_mempolicy;
> > #endif
>
> I was going to ask why we have these in internal.h, but then noticed we have to
> have a nommu version of the task_mmu stuff for museum pieces and
> why-do-they-exist arches, sigh.
>
> > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > index ee1e4ccd33bd..45134335e086 100644
> > --- a/fs/proc/task_mmu.c
> > +++ b/fs/proc/task_mmu.c
> > @@ -132,11 +132,11 @@ static void release_task_mempolicy(struct proc_maps_private *priv)
> >
> > #ifdef CONFIG_PER_VMA_LOCK
> >
> > -static void unlock_vma(struct proc_maps_private *priv)
> > +static void unlock_vma(struct proc_maps_locking_ctx *lock_ctx)
> > {
> > - if (priv->locked_vma) {
> > - vma_end_read(priv->locked_vma);
> > - priv->locked_vma = NULL;
> > + if (lock_ctx->locked_vma) {
> > + vma_end_read(lock_ctx->locked_vma);
> > + lock_ctx->locked_vma = NULL;
> > }
> > }
> >
> > @@ -151,14 +151,14 @@ static inline bool lock_vma_range(struct seq_file *m,
> > * walking the vma tree under rcu read protection.
> > */
> > if (m->op != &proc_pid_maps_op) {
> > - if (mmap_read_lock_killable(priv->mm))
> > + if (mmap_read_lock_killable(priv->lock_ctx.mm))
> > return false;
> >
> > - priv->mmap_locked = true;
> > + priv->lock_ctx.mmap_locked = true;
> > } else {
> > rcu_read_lock();
> > - priv->locked_vma = NULL;
> > - priv->mmap_locked = false;
> > + priv->lock_ctx.locked_vma = NULL;
> > + priv->lock_ctx.mmap_locked = false;
> > }
> >
> > return true;
> > @@ -166,10 +166,10 @@ static inline bool lock_vma_range(struct seq_file *m,
> >
> > static inline void unlock_vma_range(struct proc_maps_private *priv)
> > {
>
> Not sure why we have unlock_vma() parameterised by proc_maps_locking_ctx but
> this is parameerised by proc_maps_private?
>
> Seems more consistent to have both parameterised by proc_maps_locking_ctx.
True, we can pass just proc_maps_locking_ctx to both lock_vma_range()
and unlock_vma_range(). Will update.
>
> Maybe we'd want lock() forms this way too for consistency?
>
> > - if (priv->mmap_locked) {
> > - mmap_read_unlock(priv->mm);
> > + if (priv->lock_ctx.mmap_locked) {
> > + mmap_read_unlock(priv->lock_ctx.mm);
> > } else {
> > - unlock_vma(priv);
> > + unlock_vma(&priv->lock_ctx);
> > rcu_read_unlock();
> > }
> > }
> > @@ -179,13 +179,13 @@ static struct vm_area_struct *get_next_vma(struct proc_maps_private *priv,
> > {
> > struct vm_area_struct *vma;
> >
>
> We reference priv->lock_ctx 3 times here, either extract as helper var or pass
> in direct perhaps?
>
> > - if (priv->mmap_locked)
> > + if (priv->lock_ctx.mmap_locked)
> > return vma_next(&priv->iter);
> >
> > - unlock_vma(priv);
> > - vma = lock_next_vma(priv->mm, &priv->iter, last_pos);
> > + unlock_vma(&priv->lock_ctx);
> > + vma = lock_next_vma(priv->lock_ctx.mm, &priv->iter, last_pos);
> > if (!IS_ERR_OR_NULL(vma))
> > - priv->locked_vma = vma;
> > + priv->lock_ctx.locked_vma = vma;
> >
> > return vma;
> > }
> > @@ -193,14 +193,14 @@ static struct vm_area_struct *get_next_vma(struct proc_maps_private *priv,
> > static inline bool fallback_to_mmap_lock(struct proc_maps_private *priv,
> > loff_t pos)
> > {
>
> (Also)
>
> We reference priv->lock_ctx 3 times here, either extract as helper var or pass
> in direct perhaps?
>
> > - if (priv->mmap_locked)
> > + if (priv->lock_ctx.mmap_locked)
> > return false;
> >
> > rcu_read_unlock();
> > - mmap_read_lock(priv->mm);
> > + mmap_read_lock(priv->lock_ctx.mm);
> > /* Reinitialize the iterator after taking mmap_lock */
> > vma_iter_set(&priv->iter, pos);
> > - priv->mmap_locked = true;
> > + priv->lock_ctx.mmap_locked = true;
> >
> > return true;
> > }
> > @@ -210,12 +210,12 @@ static inline bool fallback_to_mmap_lock(struct proc_maps_private *priv,
> > static inline bool lock_vma_range(struct seq_file *m,
> > struct proc_maps_private *priv)
> > {
> > - return mmap_read_lock_killable(priv->mm) == 0;
> > + return mmap_read_lock_killable(priv->lock_ctx.mm) == 0;
> > }
> >
> > static inline void unlock_vma_range(struct proc_maps_private *priv)
> > {
> > - mmap_read_unlock(priv->mm);
> > + mmap_read_unlock(priv->lock_ctx.mm);
> > }
> >
> > static struct vm_area_struct *get_next_vma(struct proc_maps_private *priv,
> > @@ -258,7 +258,7 @@ static struct vm_area_struct *proc_get_vma(struct seq_file *m, loff_t *ppos)
> > *ppos = vma->vm_end;
> > } else {
> > *ppos = SENTINEL_VMA_GATE;
> > - vma = get_gate_vma(priv->mm);
> > + vma = get_gate_vma(priv->lock_ctx.mm);
> > }
> >
> > return vma;
> > @@ -278,7 +278,7 @@ static void *m_start(struct seq_file *m, loff_t *ppos)
> > if (!priv->task)
> > return ERR_PTR(-ESRCH);
> >
> > - mm = priv->mm;
> > + mm = priv->lock_ctx.mm;
> > if (!mm || !mmget_not_zero(mm)) {
> > put_task_struct(priv->task);
> > priv->task = NULL;
> > @@ -318,7 +318,7 @@ static void *m_next(struct seq_file *m, void *v, loff_t *ppos)
> > static void m_stop(struct seq_file *m, void *v)
> > {
> > struct proc_maps_private *priv = m->private;
> > - struct mm_struct *mm = priv->mm;
> > + struct mm_struct *mm = priv->lock_ctx.mm;
> >
> > if (!priv->task)
> > return;
> > @@ -339,9 +339,9 @@ static int proc_maps_open(struct inode *inode, struct file *file,
> > return -ENOMEM;
> >
> > priv->inode = inode;
> > - priv->mm = proc_mem_open(inode, PTRACE_MODE_READ);
> > - if (IS_ERR_OR_NULL(priv->mm)) {
> > - int err = priv->mm ? PTR_ERR(priv->mm) : -ESRCH;
> > + priv->lock_ctx.mm = proc_mem_open(inode, PTRACE_MODE_READ);
> > + if (IS_ERR_OR_NULL(priv->lock_ctx.mm)) {
> > + int err = priv->lock_ctx.mm ? PTR_ERR(priv->lock_ctx.mm) : -ESRCH;
> >
> > seq_release_private(inode, file);
> > return err;
> > @@ -355,8 +355,8 @@ static int proc_map_release(struct inode *inode, struct file *file)
> > struct seq_file *seq = file->private_data;
> > struct proc_maps_private *priv = seq->private;
> >
> > - if (priv->mm)
> > - mmdrop(priv->mm);
> > + if (priv->lock_ctx.mm)
> > + mmdrop(priv->lock_ctx.mm);
> >
> > return seq_release_private(inode, file);
> > }
> > @@ -610,7 +610,7 @@ static int do_procmap_query(struct proc_maps_private *priv, void __user *uarg)
> > if (!!karg.build_id_size != !!karg.build_id_addr)
> > return -EINVAL;
> >
> > - mm = priv->mm;
> > + mm = priv->lock_ctx.mm;
> > if (!mm || !mmget_not_zero(mm))
> > return -ESRCH;
> >
> > @@ -1311,7 +1311,7 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
> > {
> > struct proc_maps_private *priv = m->private;
> > struct mem_size_stats mss = {};
> > - struct mm_struct *mm = priv->mm;
> > + struct mm_struct *mm = priv->lock_ctx.mm;
>
> Nit, but maybe add a
>
> struct proc_maps_locking_ctx *lock_ctx = priv->lock_ctx;
>
> Here to reduce 'priv->lock_ctx' stuff?
Yep, will do that in all the places. Thanks!
>
> > struct vm_area_struct *vma;
> > unsigned long vma_start = 0, last_vma_end = 0;
> > int ret = 0;
> > @@ -1456,9 +1456,9 @@ static int smaps_rollup_open(struct inode *inode, struct file *file)
> > goto out_free;
> >
> > priv->inode = inode;
> > - priv->mm = proc_mem_open(inode, PTRACE_MODE_READ);
> > - if (IS_ERR_OR_NULL(priv->mm)) {
> > - ret = priv->mm ? PTR_ERR(priv->mm) : -ESRCH;
> > + priv->lock_ctx.mm = proc_mem_open(inode, PTRACE_MODE_READ);
> > + if (IS_ERR_OR_NULL(priv->lock_ctx.mm)) {
> > + ret = priv->lock_ctx.mm ? PTR_ERR(priv->lock_ctx.mm) : -ESRCH;
> >
> > single_release(inode, file);
> > goto out_free;
> > @@ -1476,8 +1476,8 @@ static int smaps_rollup_release(struct inode *inode, struct file *file)
> > struct seq_file *seq = file->private_data;
> > struct proc_maps_private *priv = seq->private;
> >
> > - if (priv->mm)
> > - mmdrop(priv->mm);
> > + if (priv->lock_ctx.mm)
> > + mmdrop(priv->lock_ctx.mm);
> >
> > kfree(priv);
> > return single_release(inode, file);
> > diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
> > index 59bfd61d653a..d362919f4f68 100644
> > --- a/fs/proc/task_nommu.c
> > +++ b/fs/proc/task_nommu.c
> > @@ -204,7 +204,7 @@ static void *m_start(struct seq_file *m, loff_t *ppos)
> > if (!priv->task)
> > return ERR_PTR(-ESRCH);
> >
> > - mm = priv->mm;
> > + mm = priv->lock_ctx.mm;
> > if (!mm || !mmget_not_zero(mm)) {
> > put_task_struct(priv->task);
> > priv->task = NULL;
> > @@ -226,7 +226,7 @@ static void *m_start(struct seq_file *m, loff_t *ppos)
> > static void m_stop(struct seq_file *m, void *v)
> > {
> > struct proc_maps_private *priv = m->private;
> > - struct mm_struct *mm = priv->mm;
>
> (same as above, I reviewed this upsidedown :P)
>
> NIT, but seems sensible to have a
>
> struct proc_maps_locking_ctx *lock_ctx = priv->lock_ctx;
>
> Here so we can avoid the ugly 'priv->lock_ctx' stuff below.
>
> > + struct mm_struct *mm = priv->lock_ctx.mm;
> >
> > if (!priv->task)
> > return;
> > @@ -259,9 +259,9 @@ static int maps_open(struct inode *inode, struct file *file,
> > return -ENOMEM;
> >
> > priv->inode = inode;
> > - priv->mm = proc_mem_open(inode, PTRACE_MODE_READ);
> > - if (IS_ERR_OR_NULL(priv->mm)) {
> > - int err = priv->mm ? PTR_ERR(priv->mm) : -ESRCH;
> > + priv->lock_ctx.mm = proc_mem_open(inode, PTRACE_MODE_READ);
> > + if (IS_ERR_OR_NULL(priv->lock_ctx.mm)) {
> > + int err = priv->lock_ctx.mm ? PTR_ERR(priv->lock_ctx.mm) : -ESRCH;
>
> >
> > seq_release_private(inode, file);
> > return err;
> > @@ -276,8 +276,8 @@ static int map_release(struct inode *inode, struct file *file)
> > struct seq_file *seq = file->private_data;
> > struct proc_maps_private *priv = seq->private;
> >
> > - if (priv->mm)
> > - mmdrop(priv->mm);
> > + if (priv->lock_ctx.mm)
> > + mmdrop(priv->lock_ctx.mm);
> >
> > return seq_release_private(inode, file);
> > }
> > --
> > 2.50.1.565.gc32cd1483b-goog
> >
