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@google.com Reviewed-by: Vlastimil Babka vbabka@suse.cz

---

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.

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?

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

On Wed, Aug 06, 2025 at 08:59:04AM -0700, Suren Baghdasaryan wrote:

Utilize per-vma locks to stabilize vma after lookup without taking mmap_lock during PROCMAP_QUERY ioctl execution. If vma lock is contended, we fall back to mmap_lock but take it only momentarily to lock the vma and release the mmap_lock. In a very unlikely case of vm_refcnt overflow, this fall back path will fail and ioctl is done under mmap_lock protection.

This change is designed to reduce mmap_lock contention and prevent PROCMAP_QUERY ioctl calls from blocking address space updates.

Signed-off-by: Suren Baghdasaryan surenb@google.com

A lot of nits but nothing's really standing out as broken, AFAICT...

---

fs/proc/task_mmu.c | 84 +++++++++++++++++++++++++++++++++++++--------- 1 file changed, 68 insertions(+), 16 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c index 45134335e086..0396315dfaee 100644 --- a/fs/proc/task_mmu.c +++ b/fs/proc/task_mmu.c @@ -517,28 +517,81 @@ static int pid_maps_open(struct inode *inode, struct file *file) PROCMAP_QUERY_VMA_FLAGS \ )

-static int query_vma_setup(struct mm_struct *mm) +#ifdef CONFIG_PER_VMA_LOCK

+static int query_vma_setup(struct proc_maps_locking_ctx *lock_ctx) {

• return mmap_read_lock_killable(mm);

• lock_ctx->locked_vma = NULL;
• lock_ctx->mmap_locked = false;

We also do this in lock_vma_range(), seems sensible to factor out? E.g.:

static void ctx_clear_vma(struct proc_maps_locking_ctx *lock_ctx) { lock_ctx->locked_vma = NULL; lock_ctx->mmap_locked = false; }

• return 0;

}

-static void query_vma_teardown(struct mm_struct *mm, struct vm_area_struct *vma) +static void query_vma_teardown(struct proc_maps_locking_ctx *lock_ctx) {

• mmap_read_unlock(mm);

• if (lock_ctx->mmap_locked)

• mmap_read_unlock(lock_ctx->mm);
  

Maybe worth a comment as to why we leave lock_ctx->mmap_locked set here?

• else

• unlock_vma(lock_ctx);
  

Should have said on 2/3, but I wonder if we should prefix with ctx_, as 'unlock_vma()' and 'lock_vma()' seem like core functions... esp. since we have vma_start_read/write() rather than functions that reference locking.

So - ctx_unlock_vma() and ctx_lock_vma() or unlock_ctx_vma() / lock_ctx_vma()?

+}

+static struct vm_area_struct *query_vma_find_by_addr(struct proc_maps_locking_ctx *lock_ctx,

• 				     unsigned long addr)
  

+{

• struct vm_area_struct *vma;
• struct vma_iterator vmi;
• if (lock_ctx->mmap_locked)

• return find_vma(lock_ctx->mm, addr);
  

• unlock_vma(lock_ctx);
• rcu_read_lock();
• vma_iter_init(&vmi, lock_ctx->mm, addr);
• vma = lock_next_vma(lock_ctx->mm, &vmi, addr);
• rcu_read_unlock();

I think a comment at the top of this block would be useful, something like 'We unlock any previously locked VMA and find the next under RCU'.

• if (!IS_ERR_OR_NULL(vma)) {

Is the NULL bit here really necessary? presumably lock_ctx->locked_vma is expected to be NULL already, so we're not overwriting anything here.

In fact we could get rid of the horrid if/else here with a guard clause like:

if (!IS_ERR(vma) || PTR_ERR(vma) != -EAGAIN) return vma;

(the !IS_ERR() bit is probably a bit redundant but makes things clearer vs. just the PTR_ERR() thing)

Then do the rest below.

• lock_ctx->locked_vma = vma;
  

• } else if (PTR_ERR(vma) == -EAGAIN) {

• /* Fallback to mmap_lock on vma->vm_refcnt overflow */
  

• mmap_read_lock(lock_ctx->mm);
  

• vma = find_vma(lock_ctx->mm, addr);
  

• lock_ctx->mmap_locked = true;
  

Kinda sucks we have two separate ways of doing fallback now, this open-coded thing and fallback_to_mmap_lock().

Sort of hard to combine since we have subtly diffrent semantics - the RCU read lock is being held in the /proc/$pid/maps case, but here we've released it already.

• }
• return vma;

+}

+#else /* CONFIG_PER_VMA_LOCK */

+static int query_vma_setup(struct proc_maps_locking_ctx *lock_ctx) +{

• return mmap_read_lock_killable(lock_ctx->mm);

}

-static struct vm_area_struct *query_vma_find_by_addr(struct mm_struct *mm, unsigned long addr) +static void query_vma_teardown(struct proc_maps_locking_ctx *lock_ctx) {

• return find_vma(mm, addr);

• mmap_read_unlock(lock_ctx->mm);

}

-static struct vm_area_struct *query_matching_vma(struct mm_struct *mm, +static struct vm_area_struct *query_vma_find_by_addr(struct proc_maps_locking_ctx *lock_ctx,

• 				     unsigned long addr)
  

+{

• return find_vma(lock_ctx->mm, addr);

+}

+#endif /* CONFIG_PER_VMA_LOCK */

+static struct vm_area_struct *query_matching_vma(struct proc_maps_locking_ctx *lock_ctx, unsigned long addr, u32 flags) { struct vm_area_struct *vma;

next_vma:

• vma = query_vma_find_by_addr(mm, addr);

• vma = query_vma_find_by_addr(lock_ctx, addr);
• if (IS_ERR(vma))

• return vma;
  

• if (!vma) goto no_vma;

@@ -579,11 +632,11 @@ static struct vm_area_struct *query_matching_vma(struct mm_struct *mm, return ERR_PTR(-ENOENT); }

-static int do_procmap_query(struct proc_maps_private *priv, void __user *uarg) +static int do_procmap_query(struct mm_struct *mm, void __user *uarg) {

• struct proc_maps_locking_ctx lock_ctx = { .mm = mm };

struct procmap_query karg; struct vm_area_struct *vma;

• struct mm_struct *mm; const char *name = NULL; char build_id_buf[BUILD_ID_SIZE_MAX], *name_buf = NULL; __u64 usize;

@@ -610,17 +663,16 @@ 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->lock_ctx.mm; if (!mm || !mmget_not_zero(mm)) return -ESRCH;

• err = query_vma_setup(mm);

• err = query_vma_setup(&lock_ctx); if (err) { mmput(mm); return err; }

• vma = query_matching_vma(mm, karg.query_addr, karg.query_flags);

• vma = query_matching_vma(&lock_ctx, karg.query_addr, karg.query_flags); if (IS_ERR(vma)) { err = PTR_ERR(vma); vma = NULL;

@@ -705,7 +757,7 @@ static int do_procmap_query(struct proc_maps_private *priv, void __user *uarg) }

/* unlock vma or mmap_lock, and put mm_struct before copying data to user */

• query_vma_teardown(mm, vma);

• query_vma_teardown(&lock_ctx); mmput(mm);

  if (karg.vma_name_size && copy_to_user(u64_to_user_ptr(karg.vma_name_addr),

@@ -725,7 +777,7 @@ static int do_procmap_query(struct proc_maps_private *priv, void __user *uarg) return 0;

out:

• query_vma_teardown(mm, vma);

• query_vma_teardown(&lock_ctx); mmput(mm); kfree(name_buf); return err;

@@ -738,7 +790,7 @@ static long procfs_procmap_ioctl(struct file *file, unsigned int cmd, unsigned l

switch (cmd) { case PROCMAP_QUERY:

• return do_procmap_query(priv, (void __user *)arg);
  

• return do_procmap_query(priv->lock_ctx.mm, (void __user *)arg);
  

OK this confused me until I worked it through.

We set priv->lock_ctx.mm in:

pid_maps_open() -> do_maps_open() -> proc_maps_open()

Which it gets from proc_mem_open() which figures out the mm.

Maybe one for 2/3, but it'd be nice to have a comment saying something about how this is set, since it being part of lock_ctx makes it seem like it's something that would be set elsewhere.

Since we have fallback stuff and want to thread through this new lokc context type I guess it makes sense to put it here but given that's the case, let's maybe just add a comment here to clarify.

default: return -ENOIOCTLCMD; } -- 2.50.1.565.gc32cd1483b-goog

Cheers, Lorenzo

On Wed, Aug 06, 2025 at 02:46:00PM -0700, Suren Baghdasaryan wrote:

On Wed, Aug 6, 2025 at 12:03 PM Lorenzo Stoakes lorenzo.stoakes@oracle.com wrote:

...

On Wed, Aug 06, 2025 at 08:59:04AM -0700, Suren Baghdasaryan wrote:

...

Utilize per-vma locks to stabilize vma after lookup without taking mmap_lock during PROCMAP_QUERY ioctl execution. If vma lock is contended, we fall back to mmap_lock but take it only momentarily to lock the vma and release the mmap_lock. In a very unlikely case of vm_refcnt overflow, this fall back path will fail and ioctl is done under mmap_lock protection.

This change is designed to reduce mmap_lock contention and prevent PROCMAP_QUERY ioctl calls from blocking address space updates.

Signed-off-by: Suren Baghdasaryan surenb@google.com

A lot of nits but nothing's really standing out as broken, AFAICT...

...

---

fs/proc/task_mmu.c | 84 +++++++++++++++++++++++++++++++++++++--------- 1 file changed, 68 insertions(+), 16 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c index 45134335e086..0396315dfaee 100644 --- a/fs/proc/task_mmu.c +++ b/fs/proc/task_mmu.c @@ -517,28 +517,81 @@ static int pid_maps_open(struct inode *inode, struct file *file) PROCMAP_QUERY_VMA_FLAGS \ )

-static int query_vma_setup(struct mm_struct *mm) +#ifdef CONFIG_PER_VMA_LOCK

+static int query_vma_setup(struct proc_maps_locking_ctx *lock_ctx) {

• return mmap_read_lock_killable(mm);
  

• lock_ctx->locked_vma = NULL;
  

• lock_ctx->mmap_locked = false;
  

We also do this in lock_vma_range(), seems sensible to factor out? E.g.:

static void ctx_clear_vma(struct proc_maps_locking_ctx *lock_ctx)

That name really confused me :) Maybe lock_vma_ctx_init() or something along these lines. If we can't think of a good name I would prefer to keep it as is, given it's only two lines and used only in two places.

Yeah you're right, that name isn't great (it's hard to get naming right isn't it? :P)

I think it's worth separating out just because I find this:

helper_struct->field1 = X; helper_struct->field2 = Y;

Open-coding fiddly and prone to error, what if you add a new field later etc.

It's also semantically useful to know that updating one field impliles the update of another.

...

{ lock_ctx->locked_vma = NULL; lock_ctx->mmap_locked = false; }

...

• return 0;
  

}

-static void query_vma_teardown(struct mm_struct *mm, struct vm_area_struct *vma) +static void query_vma_teardown(struct proc_maps_locking_ctx *lock_ctx) {

• mmap_read_unlock(mm);
  

• if (lock_ctx->mmap_locked)
  

•         mmap_read_unlock(lock_ctx->mm);
  

Maybe worth a comment as to why we leave lock_ctx->mmap_locked set here?

Sure. The reason is that this is a teardown stage and lock_ctx won't be used anymore. I guess I could reset it just to leave lock_ctx consistent instead of adding a comment. Will do that.

Thanks makes sense.

...

...

• else
  

•         unlock_vma(lock_ctx);
  

Should have said on 2/3, but I wonder if we should prefix with ctx_, as 'unlock_vma()' and 'lock_vma()' seem like core functions... esp. since we have vma_start_read/write() rather than functions that reference locking.

So - ctx_unlock_vma() and ctx_lock_vma() or unlock_ctx_vma() / lock_ctx_vma()?

unlock_ctx_vma() / lock_ctx_vma() sounds good to me.

...

...

+}

+static struct vm_area_struct *query_vma_find_by_addr(struct proc_maps_locking_ctx *lock_ctx,

•                                              unsigned long addr)
  

+{

• struct vm_area_struct *vma;
  

• struct vma_iterator vmi;
  

• if (lock_ctx->mmap_locked)
  

•         return find_vma(lock_ctx->mm, addr);
  

• unlock_vma(lock_ctx);
  

• rcu_read_lock();
  

• vma_iter_init(&vmi, lock_ctx->mm, addr);
  

• vma = lock_next_vma(lock_ctx->mm, &vmi, addr);
  

• rcu_read_unlock();
  

I think a comment at the top of this block would be useful, something like 'We unlock any previously locked VMA and find the next under RCU'.

Ack.

...

...

• if (!IS_ERR_OR_NULL(vma)) {
  

Is the NULL bit here really necessary? presumably lock_ctx->locked_vma is expected to be NULL already, so we're not overwriting anything here.

In fact we could get rid of the horrid if/else here with a guard clause like:

    if (!IS_ERR(vma) || PTR_ERR(vma) != -EAGAIN)
            return vma;

We still need to assign lock_ctx->locked_vma when !IS_ERR(vma) before we return the vma, so the lines about would not be correct. I can change it to:

if (!vma) return NULL;

if (!IS_ERR(vma)) { lock_ctx->locked_vma = vma; return vma; }

if (PTR_ERR(vma) == -EAGAIN) { /* Fallback to mmap_lock on vma->vm_refcnt overflow */ ... } return vma;

I think that would be the equivalent of what I currently have. Would you prefer that?

Yeah sorry, sort of sketching this out quickly here.

Yeah what you suggest looks good thanks!

...

(the !IS_ERR() bit is probably a bit redundant but makes things clearer vs. just the PTR_ERR() thing)

Then do the rest below.

...

•         lock_ctx->locked_vma = vma;
  

• } else if (PTR_ERR(vma) == -EAGAIN) {
  

•         /* Fallback to mmap_lock on vma->vm_refcnt overflow */
  

•         mmap_read_lock(lock_ctx->mm);
  

•         vma = find_vma(lock_ctx->mm, addr);
  

•         lock_ctx->mmap_locked = true;
  

Kinda sucks we have two separate ways of doing fallback now, this open-coded thing and fallback_to_mmap_lock().

Sort of hard to combine since we have subtly diffrent semantics - the RCU read lock is being held in the /proc/$pid/maps case, but here we've released it already.

Yeah, plus that one uses iterators and this one doesn't... I don't think it's worth trying to shoehorn them together given that the code is quite short.

Yeah right.

I sort of wish we could have things be a little more consistent across the two, but I think that would need to be part of a refactoring of this code in general, so is not really relevant here.

So leave it as-is for now that's fine!

...

...

• }
  

• return vma;
  

+}

+#else /* CONFIG_PER_VMA_LOCK */

+static int query_vma_setup(struct proc_maps_locking_ctx *lock_ctx) +{

• return mmap_read_lock_killable(lock_ctx->mm);
  

}

-static struct vm_area_struct *query_vma_find_by_addr(struct mm_struct *mm, unsigned long addr) +static void query_vma_teardown(struct proc_maps_locking_ctx *lock_ctx) {

• return find_vma(mm, addr);
  

• mmap_read_unlock(lock_ctx->mm);
  

}

-static struct vm_area_struct *query_matching_vma(struct mm_struct *mm, +static struct vm_area_struct *query_vma_find_by_addr(struct proc_maps_locking_ctx *lock_ctx,

•                                              unsigned long addr)
  

+{

• return find_vma(lock_ctx->mm, addr);
  

+}

+#endif /* CONFIG_PER_VMA_LOCK */

+static struct vm_area_struct *query_matching_vma(struct proc_maps_locking_ctx *lock_ctx, unsigned long addr, u32 flags) { struct vm_area_struct *vma;

next_vma:

• vma = query_vma_find_by_addr(mm, addr);
  

• vma = query_vma_find_by_addr(lock_ctx, addr);
  

• if (IS_ERR(vma))
  

•         return vma;
  

• if (!vma)
          goto no_vma;
  
  

@@ -579,11 +632,11 @@ static struct vm_area_struct *query_matching_vma(struct mm_struct *mm, return ERR_PTR(-ENOENT); }

-static int do_procmap_query(struct proc_maps_private *priv, void __user *uarg) +static int do_procmap_query(struct mm_struct *mm, void __user *uarg) {

• struct proc_maps_locking_ctx lock_ctx = { .mm = mm };
  

...

  struct procmap_query karg;
  struct vm_area_struct *vma;

• struct mm_struct *mm;
  const char *name = NULL;
  char build_id_buf[BUILD_ID_SIZE_MAX], *name_buf = NULL;
  __u64 usize;
  

@@ -610,17 +663,16 @@ 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->lock_ctx.mm;
  if (!mm || !mmget_not_zero(mm))
          return -ESRCH;
  
  

• err = query_vma_setup(mm);
  

• err = query_vma_setup(&lock_ctx);
  if (err) {
          mmput(mm);
          return err;
  }
  
  

• vma = query_matching_vma(mm, karg.query_addr, karg.query_flags);
  

• vma = query_matching_vma(&lock_ctx, karg.query_addr, karg.query_flags);
  if (IS_ERR(vma)) {
          err = PTR_ERR(vma);
          vma = NULL;
  

@@ -705,7 +757,7 @@ static int do_procmap_query(struct proc_maps_private *priv, void __user *uarg) }

  /* unlock vma or mmap_lock, and put mm_struct before copying data to user */

• query_vma_teardown(mm, vma);
  

• query_vma_teardown(&lock_ctx);
  mmput(mm);
  
  if (karg.vma_name_size && copy_to_user(u64_to_user_ptr(karg.vma_name_addr),
  

@@ -725,7 +777,7 @@ static int do_procmap_query(struct proc_maps_private *priv, void __user *uarg) return 0;

out:

• query_vma_teardown(mm, vma);
  

• query_vma_teardown(&lock_ctx);
  mmput(mm);
  kfree(name_buf);
  return err;
  

@@ -738,7 +790,7 @@ static long procfs_procmap_ioctl(struct file *file, unsigned int cmd, unsigned l

  switch (cmd) {
  case PROCMAP_QUERY:

•         return do_procmap_query(priv, (void __user *)arg);
  

•         return do_procmap_query(priv->lock_ctx.mm, (void __user *)arg);
  

OK this confused me until I worked it through.

We set priv->lock_ctx.mm in:

pid_maps_open() -> do_maps_open() -> proc_maps_open()

Which it gets from proc_mem_open() which figures out the mm.

Maybe one for 2/3, but it'd be nice to have a comment saying something about how this is set, since it being part of lock_ctx makes it seem like it's something that would be set elsewhere.

Since we have fallback stuff and want to thread through this new lokc context type I guess it makes sense to put it here but given that's the case, let's maybe just add a comment here to clarify.

Ok, something like "lock_ctx.mm is set during file open operation" ?

Yeah that's fine thanks!

...

...

  default:
          return -ENOIOCTLCMD;
  }

-- 2.50.1.565.gc32cd1483b-goog

Cheers, Lorenzo