On Wed, Aug 6, 2025 at 11:04 AM Lorenzo Stoakes lorenzo.stoakes@oracle.com 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@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.
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