[PATCH 0/3] use vma locks for proc/pid/{smaps|numa_maps} reads

[PATCH 0/3] use vma locks for proc/pid/{smaps|numa_maps} reads

[Suren Baghdasaryan]

Use per-vma locks when reading /proc/pid/smaps and /proc/pid/numa_maps
similar to /proc/pid/maps to reduce contention on central mmap_lock. One
major difference between maps and smaps/numa_maps reading is that the
latter executes page table walk which can't be done under RCU due to a
possibility of sleeping. Therefore we drop RCU read lock before this walk
while keeping the VMA locked. After the walk we retake RCU read lock,
reset VMA iterator and proceed with the next VMA.

The last two patches extend /proc/pid/maps test to cover /proc/pid/smaps
reading during concurrent address space modification.

Applies over mm-unstable.

Suren Baghdasaryan (3):
  fs/proc/task_mmu: read proc/pid/{smaps|numa_maps} under per-vma lock
  selftests/proc: ensure the test is performed at the right page
    boundary
  selftests/proc: add /proc/pid/smaps tearing tests

 fs/proc/task_mmu.c                            | 193 ++++++++++---
 tools/testing/selftests/proc/proc-maps-race.c | 254 ++++++++++++++----
 2 files changed, 354 insertions(+), 93 deletions(-)


base-commit: a22b29c0b1a2a245ef5f782d7456de3c76eb135f
-- 
2.54.0.545.g6539524ca2-goog

[PATCH 1/3] fs/proc/task_mmu: read proc/pid/{smaps|numa_maps} under per-vma lock

[Suren Baghdasaryan]

proc/pid/{smaps|numa_maps} can be read using the combination of RCU and
VMA read locks, similar to proc/pid/maps. RCU is required to safely
traverse the VMA tree and VMA lock stabilizes the VMA being processed
and the pagetable walk.

Signed-off-by: Suren Baghdasaryan <surenb@google.com>
---
 fs/proc/task_mmu.c | 193 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 154 insertions(+), 39 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 751b9ba160fb..96cfea252db6 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -132,6 +132,22 @@ static void release_task_mempolicy(struct proc_maps_private *priv)
 
 #ifdef CONFIG_PER_VMA_LOCK
 
+static inline int lock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
+{
+	int ret = mmap_read_lock_killable(lock_ctx->mm);
+
+	if (!ret)
+		lock_ctx->mmap_locked = true;
+
+	return ret;
+}
+
+static inline void unlock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
+{
+	mmap_read_unlock(lock_ctx->mm);
+	lock_ctx->mmap_locked = false;
+}
+
 static void reset_lock_ctx(struct proc_maps_locking_ctx *lock_ctx)
 {
 	lock_ctx->locked_vma = NULL;
@@ -146,25 +162,11 @@ static void unlock_ctx_vma(struct proc_maps_locking_ctx *lock_ctx)
 	}
 }
 
-static const struct seq_operations proc_pid_maps_op;
-
 static inline bool lock_vma_range(struct seq_file *m,
 				  struct proc_maps_locking_ctx *lock_ctx)
 {
-	/*
-	 * smaps and numa_maps perform page table walk, therefore require
-	 * mmap_lock but maps can be read with locking just the vma and
-	 * walking the vma tree under rcu read protection.
-	 */
-	if (m->op != &proc_pid_maps_op) {
-		if (mmap_read_lock_killable(lock_ctx->mm))
-			return false;
-
-		lock_ctx->mmap_locked = true;
-	} else {
-		rcu_read_lock();
-		reset_lock_ctx(lock_ctx);
-	}
+	rcu_read_lock();
+	reset_lock_ctx(lock_ctx);
 
 	return true;
 }
@@ -172,7 +174,7 @@ static inline bool lock_vma_range(struct seq_file *m,
 static inline void unlock_vma_range(struct proc_maps_locking_ctx *lock_ctx)
 {
 	if (lock_ctx->mmap_locked) {
-		mmap_read_unlock(lock_ctx->mm);
+		unlock_ctx_mm(lock_ctx);
 	} else {
 		unlock_ctx_vma(lock_ctx);
 		rcu_read_unlock();
@@ -213,17 +215,45 @@ static inline bool fallback_to_mmap_lock(struct proc_maps_private *priv,
 	return true;
 }
 
+static inline void drop_rcu(struct proc_maps_private *priv)
+{
+	if (priv->lock_ctx.mmap_locked)
+		return;
+
+	rcu_read_unlock();
+}
+
+static inline void reacquire_rcu(struct proc_maps_private *priv)
+{
+	if (priv->lock_ctx.mmap_locked)
+		return;
+
+	rcu_read_lock();
+	/* Reinitialize the iterator. */
+	vma_iter_set(&priv->iter, priv->lock_ctx.locked_vma->vm_end);
+}
+
 #else /* CONFIG_PER_VMA_LOCK */
 
+static inline int lock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
+{
+	return mmap_read_lock_killable(lock_ctx->mm);
+}
+
+static inline void unlock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
+{
+	mmap_read_unlock(lock_ctx->mm);
+}
+
 static inline bool lock_vma_range(struct seq_file *m,
 				  struct proc_maps_locking_ctx *lock_ctx)
 {
-	return mmap_read_lock_killable(lock_ctx->mm) == 0;
+	return lock_ctx_mm(lock_ctx) == 0;
 }
 
 static inline void unlock_vma_range(struct proc_maps_locking_ctx *lock_ctx)
 {
-	mmap_read_unlock(lock_ctx->mm);
+	unlock_ctx_mm(lock_ctx);
 }
 
 static struct vm_area_struct *get_next_vma(struct proc_maps_private *priv,
@@ -238,6 +268,9 @@ static inline bool fallback_to_mmap_lock(struct proc_maps_private *priv,
 	return false;
 }
 
+static inline void drop_rcu(struct proc_maps_private *priv) {}
+static inline void reacquire_rcu(struct proc_maps_private *priv) {}
+
 #endif /* CONFIG_PER_VMA_LOCK */
 
 static struct vm_area_struct *proc_get_vma(struct seq_file *m, loff_t *ppos)
@@ -538,12 +571,10 @@ static int query_vma_setup(struct proc_maps_locking_ctx *lock_ctx)
 
 static void query_vma_teardown(struct proc_maps_locking_ctx *lock_ctx)
 {
-	if (lock_ctx->mmap_locked) {
-		mmap_read_unlock(lock_ctx->mm);
-		lock_ctx->mmap_locked = false;
-	} else {
+	if (lock_ctx->mmap_locked)
+		unlock_ctx_mm(lock_ctx);
+	else
 		unlock_ctx_vma(lock_ctx);
-	}
 }
 
 static struct vm_area_struct *query_vma_find_by_addr(struct proc_maps_locking_ctx *lock_ctx,
@@ -1280,16 +1311,64 @@ static const struct mm_walk_ops smaps_shmem_walk_ops = {
 	.walk_lock		= PGWALK_RDLOCK,
 };
 
+#ifdef CONFIG_PER_VMA_LOCK
+
+static const struct mm_walk_ops smaps_walk_vma_lock_ops = {
+	.pmd_entry		= smaps_pte_range,
+	.hugetlb_entry		= smaps_hugetlb_range,
+	.walk_lock		= PGWALK_VMA_RDLOCK_VERIFY,
+};
+
+static const struct mm_walk_ops smaps_shmem_walk_vma_lock_ops = {
+	.pmd_entry		= smaps_pte_range,
+	.hugetlb_entry		= smaps_hugetlb_range,
+	.pte_hole		= smaps_pte_hole,
+	.walk_lock		= PGWALK_VMA_RDLOCK_VERIFY,
+};
+
+static inline const struct mm_walk_ops *
+get_smaps_walk_ops(struct proc_maps_private *priv)
+{
+	if (priv->lock_ctx.mmap_locked)
+		return &smaps_walk_ops;
+	return &smaps_walk_vma_lock_ops;
+}
+
+static inline const struct mm_walk_ops *
+get_smaps_shmem_walk_ops(struct proc_maps_private *priv)
+{
+	if (priv->lock_ctx.mmap_locked)
+		return  &smaps_shmem_walk_ops;
+	return &smaps_shmem_walk_vma_lock_ops;
+}
+
+#else /* CONFIG_PER_VMA_LOCK */
+
+static inline const struct mm_walk_ops *
+get_smaps_walk_ops(struct proc_maps_private *priv)
+{
+	return &smaps_walk_ops;
+}
+
+static inline const struct mm_walk_ops *
+get_smaps_shmem_walk_ops(struct proc_maps_private *priv)
+{
+	return &smaps_shmem_walk_ops;
+}
+
+#endif /* CONFIG_PER_VMA_LOCK */
+
 /*
  * Gather mem stats from @vma with the indicated beginning
  * address @start, and keep them in @mss.
  *
  * Use vm_start of @vma as the beginning address if @start is 0.
  */
-static void smap_gather_stats(struct vm_area_struct *vma,
-		struct mem_size_stats *mss, unsigned long start)
+static void smap_gather_stats(struct proc_maps_private *priv,
+			      struct vm_area_struct *vma,
+			      struct mem_size_stats *mss, unsigned long start)
 {
-	const struct mm_walk_ops *ops = &smaps_walk_ops;
+	const struct mm_walk_ops *ops = get_smaps_walk_ops(priv);
 
 	/* Invalid start */
 	if (start >= vma->vm_end)
@@ -1312,15 +1391,24 @@ static void smap_gather_stats(struct vm_area_struct *vma,
 					!(vma->vm_flags & VM_WRITE))) {
 			mss->swap += shmem_swapped;
 		} else {
-			ops = &smaps_shmem_walk_ops;
+			ops = get_smaps_shmem_walk_ops(priv);
 		}
 	}
 
-	/* mmap_lock is held in m_start */
+	/* Skip walking pages if gate VMA */
+	if (vma == get_gate_vma(priv->lock_ctx.mm))
+		return;
+
+	/*
+	 * Need to drop RCU read lock before the walk due to possibility of sleep.
+	 * Note that the VMA is still locked.
+	 */
+	drop_rcu(priv);
 	if (!start)
 		walk_page_vma(vma, ops, mss);
 	else
 		walk_page_range(vma->vm_mm, start, vma->vm_end, ops, mss);
+	reacquire_rcu(priv);
 }
 
 #define SEQ_PUT_DEC(str, val) \
@@ -1369,10 +1457,11 @@ static void __show_smap(struct seq_file *m, const struct mem_size_stats *mss,
 
 static int show_smap(struct seq_file *m, void *v)
 {
+	struct proc_maps_private *priv = m->private;
 	struct vm_area_struct *vma = v;
 	struct mem_size_stats mss = {};
 
-	smap_gather_stats(vma, &mss, 0);
+	smap_gather_stats(priv, vma, &mss, 0);
 
 	show_map_vma(m, vma);
 
@@ -1413,7 +1502,7 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
 		goto out_put_task;
 	}
 
-	ret = mmap_read_lock_killable(mm);
+	ret = lock_ctx_mm(&priv->lock_ctx);
 	if (ret)
 		goto out_put_mm;
 
@@ -1425,7 +1514,7 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
 
 	vma_start = vma->vm_start;
 	do {
-		smap_gather_stats(vma, &mss, 0);
+		smap_gather_stats(priv, vma, &mss, 0);
 		last_vma_end = vma->vm_end;
 
 		/*
@@ -1434,8 +1523,8 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
 		 */
 		if (mmap_lock_is_contended(mm)) {
 			vma_iter_invalidate(&vmi);
-			mmap_read_unlock(mm);
-			ret = mmap_read_lock_killable(mm);
+			unlock_ctx_mm(&priv->lock_ctx);
+			ret = lock_ctx_mm(&priv->lock_ctx);
 			if (ret) {
 				release_task_mempolicy(priv);
 				goto out_put_mm;
@@ -1484,14 +1573,14 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
 
 			/* Case 1 and 2 above */
 			if (vma->vm_start >= last_vma_end) {
-				smap_gather_stats(vma, &mss, 0);
+				smap_gather_stats(priv, vma, &mss, 0);
 				last_vma_end = vma->vm_end;
 				continue;
 			}
 
 			/* Case 4 above */
 			if (vma->vm_end > last_vma_end) {
-				smap_gather_stats(vma, &mss, last_vma_end);
+				smap_gather_stats(priv, vma, &mss, last_vma_end);
 				last_vma_end = vma->vm_end;
 			}
 		}
@@ -1505,7 +1594,7 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
 	__show_smap(m, &mss, true);
 
 	release_task_mempolicy(priv);
-	mmap_read_unlock(mm);
+	unlock_ctx_mm(&priv->lock_ctx);
 
 out_put_mm:
 	mmput(mm);
@@ -3291,6 +3380,31 @@ static const struct mm_walk_ops show_numa_ops = {
 	.walk_lock = PGWALK_RDLOCK,
 };
 
+#ifdef CONFIG_PER_VMA_LOCK
+static const struct mm_walk_ops show_numa_vma_lock_ops = {
+	.hugetlb_entry = gather_hugetlb_stats,
+	.pmd_entry = gather_pte_stats,
+	.walk_lock = PGWALK_VMA_RDLOCK_VERIFY,
+};
+
+static inline const struct mm_walk_ops *
+get_show_numa_ops(struct proc_maps_private *priv)
+{
+	if (priv->lock_ctx.mmap_locked)
+		return &show_numa_ops;
+	return &show_numa_vma_lock_ops;
+}
+
+#else /* CONFIG_PER_VMA_LOCK */
+
+static inline const struct mm_walk_ops *
+get_show_numa_ops(struct proc_maps_private *priv)
+{
+	return &show_numa_ops;
+}
+
+#endif /* CONFIG_PER_VMA_LOCK */
+
 /*
  * Display pages allocated per node and memory policy via /proc.
  */
@@ -3335,8 +3449,9 @@ static int show_numa_map(struct seq_file *m, void *v)
 	if (is_vm_hugetlb_page(vma))
 		seq_puts(m, " huge");
 
-	/* mmap_lock is held by m_start */
-	walk_page_vma(vma, &show_numa_ops, md);
+	drop_rcu(proc_priv);
+	walk_page_vma(vma, get_show_numa_ops(proc_priv), md);
+	reacquire_rcu(proc_priv);
 
 	if (!md->pages)
 		goto out;
-- 
2.54.0.545.g6539524ca2-goog

[PATCH 2/3] selftests/proc: ensure the test is performed at the right page boundary

[Suren Baghdasaryan]

When running tearing tests we need to ensure the pages we use include
VMAs that were mapped by the child process for this test. Currently we
always use the first two pages, checking VMAs at their boundaries and
this works, however once we add tests for /proc/pid/smaps, the first two
pages might not contain the VMAs that child modifies.
Locate the page that contains the first VMA mapped by the child and use
that and the next page for the test.

Signed-off-by: Suren Baghdasaryan <surenb@google.com>
---
 tools/testing/selftests/proc/proc-maps-race.c | 78 ++++++++++++++++---
 1 file changed, 68 insertions(+), 10 deletions(-)

diff --git a/tools/testing/selftests/proc/proc-maps-race.c b/tools/testing/selftests/proc/proc-maps-race.c
index a734553718da..c5031b0593b7 100644
--- a/tools/testing/selftests/proc/proc-maps-race.c
+++ b/tools/testing/selftests/proc/proc-maps-race.c
@@ -39,6 +39,13 @@
 #include <sys/types.h>
 #include <sys/wait.h>
 
+#define min(a, b) \
+	({ \
+		typeof(a) _a = (a); \
+		typeof(b) _b = (b); \
+		_a < _b ? _a : _b; \
+	})
+
 /* /proc/pid/maps parsing routines */
 struct page_content {
 	char *data;
@@ -77,6 +84,7 @@ FIXTURE(proc_maps_race)
 	struct line_content first_line;
 	unsigned long duration_sec;
 	int shared_mem_size;
+	int skip_pages;
 	int page_size;
 	int vma_count;
 	bool verbose;
@@ -105,27 +113,68 @@ struct vma_modifier_info {
 	void *child_mapped_addr[];
 };
 
-
-static bool read_two_pages(FIXTURE_DATA(proc_maps_race) *self)
+static bool read_page(FIXTURE_DATA(proc_maps_race) *self,
+		      struct page_content *page)
 {
 	ssize_t  bytes_read;
 
-	if (lseek(self->maps_fd, 0, SEEK_SET) < 0)
+	bytes_read = read(self->maps_fd, page->data, self->page_size);
+	if (bytes_read <= 0)
 		return false;
 
-	bytes_read = read(self->maps_fd, self->page1.data, self->page_size);
-	if (bytes_read <= 0)
+	/* Make sure data always ends with a newline character. */
+	if (page->data[bytes_read - 1] != '\n')
 		return false;
 
-	self->page1.size = bytes_read;
+	page->size = bytes_read;
 
-	bytes_read = read(self->maps_fd, self->page2.data, self->page_size);
-	if (bytes_read <= 0)
+	return true;
+}
+
+static int locate_containing_page(FIXTURE_DATA(proc_maps_race) *self,
+				  unsigned long addr, unsigned long size)
+{
+	unsigned long start, end;
+	int page = 0;
+
+	if (lseek(self->maps_fd, 0, SEEK_SET) < 0)
+		return -1;
+
+	while (true) {
+		char *curr_pos;
+		char *end_pos;
+
+		if (!read_page(self, &self->page1))
+			return -1;
+
+		curr_pos = self->page1.data;
+		end_pos = self->page1.data + self->page1.size;
+		while (curr_pos < end_pos) {
+			if (sscanf(curr_pos, "%lx-%lx", &start, &end) == 2 &&
+			    start == addr && end == addr + size)
+				return page;
+
+			curr_pos = strchr(curr_pos, '\n');
+			if (!curr_pos)
+				break;
+			curr_pos++;
+		}
+		page++;
+	}
+
+	return 0;
+}
+
+static bool read_two_pages(FIXTURE_DATA(proc_maps_race) *self)
+{
+	if (lseek(self->maps_fd, 0, SEEK_SET) < 0)
 		return false;
 
-	self->page2.size = bytes_read;
+	for (int i = 0; i < self->skip_pages; i++)
+		if (!read_page(self, &self->page1))
+			return false;
 
-	return true;
+	return read_page(self, &self->page1) && read_page(self, &self->page2);
 }
 
 static void copy_first_line(struct page_content *page, char *first_line)
@@ -418,6 +467,8 @@ FIXTURE_SETUP(proc_maps_race)
 	struct vma_modifier_info *mod_info;
 	pthread_mutexattr_t mutex_attr;
 	pthread_condattr_t cond_attr;
+	unsigned long first_map_addr;
+	unsigned long last_map_addr;
 	unsigned long duration_sec;
 	char fname[32];
 
@@ -502,6 +553,13 @@ FIXTURE_SETUP(proc_maps_race)
 	self->page2.data = malloc(self->page_size);
 	ASSERT_NE(self->page2.data, NULL);
 
+	first_map_addr = (unsigned long)mod_info->child_mapped_addr[0];
+	last_map_addr = (unsigned long)mod_info->child_mapped_addr[mod_info->vma_count - 1];
+
+	self->skip_pages = locate_containing_page(self,
+					min(first_map_addr, last_map_addr),
+					self->page_size * 3);
+	ASSERT_NE(self->skip_pages, -1);
 	ASSERT_TRUE(read_boundary_lines(self, &self->last_line, &self->first_line));
 
 	/*
-- 
2.54.0.545.g6539524ca2-goog

[PATCH 3/3] selftests/proc: add /proc/pid/smaps tearing tests

[Suren Baghdasaryan]

Add MAPS_FILE environment variable to select whether the tests to be run
for /proc/pid/maps or /proc/pid/smaps. Add support for testing smaps file
by reusing the same logic as with maps file but skipping all the data
except for the VMA addresses, which are the only part relevant for the
tearing tests. Skip PROCMAP_QUERY parts of the tests because smaps does
not implement that ioctl.

Signed-off-by: Suren Baghdasaryan <surenb@google.com>
---
 tools/testing/selftests/proc/proc-maps-race.c | 176 +++++++++++++-----
 1 file changed, 132 insertions(+), 44 deletions(-)

diff --git a/tools/testing/selftests/proc/proc-maps-race.c b/tools/testing/selftests/proc/proc-maps-race.c
index c5031b0593b7..b677bc23cab9 100644
--- a/tools/testing/selftests/proc/proc-maps-race.c
+++ b/tools/testing/selftests/proc/proc-maps-race.c
@@ -17,8 +17,8 @@
  */
 /*
  * Fork a child that concurrently modifies address space while the main
- * process is reading /proc/$PID/maps and verifying the results. Address
- * space modifications include:
+ * process is reading /proc/$PID/maps and /proc/$PID/smaps, verifying the
+ * results. Address space modifications include:
  *     VMA splitting and merging
  *
  */
@@ -73,6 +73,11 @@ enum test_state {
 	TEST_DONE,
 };
 
+enum maps_file {
+	MAPS,
+	SMAPS,
+};
+
 struct vma_modifier_info;
 
 FIXTURE(proc_maps_race)
@@ -83,6 +88,7 @@ FIXTURE(proc_maps_race)
 	struct line_content last_line;
 	struct line_content first_line;
 	unsigned long duration_sec;
+	enum maps_file maps_file;
 	int shared_mem_size;
 	int skip_pages;
 	int page_size;
@@ -199,6 +205,57 @@ static void copy_last_line(struct page_content *page, char *last_line)
 	last_line[end - pos] = '\0';
 }
 
+static bool copy_first_entry(struct page_content *page, char *first_line)
+{
+	const char *start_pos = page->data;
+
+	while (start_pos < page->data + page->size) {
+		unsigned long start_addr;
+		unsigned long end_addr;
+		char *end_pos;
+
+		end_pos = strchr(start_pos, '\n');
+		if (!end_pos)
+			break;
+
+		if (sscanf(start_pos, "%lx-%lx", &start_addr, &end_addr) == 2) {
+			strncpy(first_line, start_pos, end_pos - start_pos);
+			first_line[end_pos - start_pos] = '\0';
+			return true;
+		}
+
+		start_pos = end_pos + 1;
+	}
+
+	return false;
+}
+
+static bool copy_last_entry(struct page_content *page, char *last_line)
+{
+	const char *end_pos = page->data + page->size - 1;
+	const char *start_pos;
+
+	while (end_pos > page->data) {
+		unsigned long start_addr;
+		unsigned long end_addr;
+
+		/* skip last newline */
+		start_pos = end_pos - 1;
+		/* search previous newline */
+		while (start_pos > page->data && start_pos[-1] != '\n')
+			start_pos--;
+		if (sscanf(start_pos, "%lx-%lx", &start_addr, &end_addr) == 2) {
+			strncpy(last_line, start_pos, end_pos - start_pos);
+			last_line[end_pos - start_pos] = '\0';
+			return true;
+		}
+
+		end_pos = start_pos - 1;
+	}
+
+	return false;
+}
+
 /* Read the last line of the first page and the first line of the second page */
 static bool read_boundary_lines(FIXTURE_DATA(proc_maps_race) *self,
 				struct line_content *last_line,
@@ -207,8 +264,16 @@ static bool read_boundary_lines(FIXTURE_DATA(proc_maps_race) *self,
 	if (!read_two_pages(self))
 		return false;
 
-	copy_last_line(&self->page1, last_line->text);
-	copy_first_line(&self->page2, first_line->text);
+	if (self->maps_file == MAPS) {
+		copy_last_line(&self->page1, last_line->text);
+		copy_first_line(&self->page2, first_line->text);
+	} else if (self->maps_file == SMAPS) {
+		if (!copy_last_entry(&self->page1, last_line->text) ||
+		    !copy_first_entry(&self->page2, first_line->text))
+			return false;
+	} else {
+		return false;
+	}
 
 	return sscanf(last_line->text, "%lx-%lx", &last_line->start_addr,
 		      &last_line->end_addr) == 2 &&
@@ -464,6 +529,7 @@ FIXTURE_SETUP(proc_maps_race)
 {
 	const char *verbose = getenv("VERBOSE");
 	const char *duration = getenv("DURATION");
+	const char *maps_file = getenv("MAPS_FILE");
 	struct vma_modifier_info *mod_info;
 	pthread_mutexattr_t mutex_attr;
 	pthread_condattr_t cond_attr;
@@ -474,6 +540,18 @@ FIXTURE_SETUP(proc_maps_race)
 
 	self->page_size = (unsigned long)sysconf(_SC_PAGESIZE);
 	self->verbose = verbose && !strncmp(verbose, "1", 1);
+	if (maps_file) {
+		if (!strcmp(maps_file, "maps")) {
+			self->maps_file = MAPS;
+		} else if (!strcmp(maps_file, "smaps")) {
+			self->maps_file = SMAPS;
+		} else {
+			ksft_print_msg("Unknown maps file %s\n", maps_file);
+			ksft_exit_fail();
+		}
+	} else {
+		self->maps_file = MAPS;
+	}
 	duration_sec = duration ? atol(duration) : 0;
 	self->duration_sec = duration_sec ? duration_sec : 5UL;
 
@@ -540,7 +618,16 @@ FIXTURE_SETUP(proc_maps_race)
 		exit(0);
 	}
 
-	sprintf(fname, "/proc/%d/maps", self->pid);
+	switch (self->maps_file) {
+	case MAPS:
+		sprintf(fname, "/proc/%d/maps", self->pid);
+		break;
+	case SMAPS:
+		sprintf(fname, "/proc/%d/smaps", self->pid);
+		break;
+	default:
+		ksft_exit_fail();
+	}
 	self->maps_fd = open(fname, O_RDONLY);
 	ASSERT_NE(self->maps_fd, -1);
 
@@ -675,20 +762,20 @@ TEST_F(proc_maps_race, test_maps_tearing_from_split)
 		last_line_changed = strcmp(new_last_line.text, self->last_line.text) != 0;
 		first_line_changed = strcmp(new_first_line.text, self->first_line.text) != 0;
 		ASSERT_EQ(last_line_changed, first_line_changed);
-
-		/* Check if PROCMAP_QUERY ioclt() finds the right VMA */
-		ASSERT_TRUE(query_addr_at(self->maps_fd, mod_info->addr + self->page_size,
-					  &vma_start, &vma_end));
-		/*
-		 * The vma at the split address can be either the same as
-		 * original one (if read before the split) or the same as the
-		 * first line in the second page (if read after the split).
-		 */
-		ASSERT_TRUE((vma_start == self->last_line.start_addr &&
-			     vma_end == self->last_line.end_addr) ||
-			    (vma_start == split_first_line.start_addr &&
-			     vma_end == split_first_line.end_addr));
-
+		if (self->maps_file == MAPS) {
+			/* Check if PROCMAP_QUERY ioclt() finds the right VMA */
+			ASSERT_TRUE(query_addr_at(self->maps_fd, mod_info->addr + self->page_size,
+						  &vma_start, &vma_end));
+			/*
+			 * The vma at the split address can be either the same as
+			 * original one (if read before the split) or the same as the
+			 * first line in the second page (if read after the split).
+			 */
+			ASSERT_TRUE((vma_start == self->last_line.start_addr &&
+				     vma_end == self->last_line.end_addr) ||
+				    (vma_start == split_first_line.start_addr &&
+				     vma_end == split_first_line.end_addr));
+		}
 		clock_gettime(CLOCK_MONOTONIC_COARSE, &end_ts);
 		end_test_iteration(&end_ts, self->verbose);
 	} while (end_ts.tv_sec - start_ts.tv_sec < self->duration_sec);
@@ -758,17 +845,18 @@ TEST_F(proc_maps_race, test_maps_tearing_from_resize)
 					strcmp(new_first_line.text, restored_first_line.text),
 					"Expand result invalid", self));
 		}
-
-		/* Check if PROCMAP_QUERY ioclt() finds the right VMA */
-		ASSERT_TRUE(query_addr_at(self->maps_fd, mod_info->addr, &vma_start, &vma_end));
-		/*
-		 * The vma should stay at the same address and have either the
-		 * original size of 3 pages or 1 page if read after shrinking.
-		 */
-		ASSERT_TRUE(vma_start == self->last_line.start_addr &&
-			    (vma_end - vma_start == self->page_size * 3 ||
-			     vma_end - vma_start == self->page_size));
-
+		if (self->maps_file == MAPS) {
+			/* Check if PROCMAP_QUERY ioclt() finds the right VMA */
+			ASSERT_TRUE(query_addr_at(self->maps_fd, mod_info->addr,
+						  &vma_start, &vma_end));
+			/*
+			 * The vma should stay at the same address and have either the
+			 * original size of 3 pages or 1 page if read after shrinking.
+			 */
+			ASSERT_TRUE(vma_start == self->last_line.start_addr &&
+				    (vma_end - vma_start == self->page_size * 3 ||
+				     vma_end - vma_start == self->page_size));
+		}
 		clock_gettime(CLOCK_MONOTONIC_COARSE, &end_ts);
 		end_test_iteration(&end_ts, self->verbose);
 	} while (end_ts.tv_sec - start_ts.tv_sec < self->duration_sec);
@@ -838,20 +926,20 @@ TEST_F(proc_maps_race, test_maps_tearing_from_remap)
 					strcmp(new_first_line.text, restored_first_line.text),
 					"Remap restore result invalid", self));
 		}
-
-		/* Check if PROCMAP_QUERY ioclt() finds the right VMA */
-		ASSERT_TRUE(query_addr_at(self->maps_fd, mod_info->addr + self->page_size,
-					  &vma_start, &vma_end));
-		/*
-		 * The vma should either stay at the same address and have the
-		 * original size of 3 pages or we should find the remapped vma
-		 * at the remap destination address with size of 1 page.
-		 */
-		ASSERT_TRUE((vma_start == self->last_line.start_addr &&
-			     vma_end - vma_start == self->page_size * 3) ||
-			    (vma_start == self->last_line.start_addr + self->page_size &&
-			     vma_end - vma_start == self->page_size));
-
+		if (self->maps_file == MAPS) {
+			/* Check if PROCMAP_QUERY ioclt() finds the right VMA */
+			ASSERT_TRUE(query_addr_at(self->maps_fd, mod_info->addr + self->page_size,
+						  &vma_start, &vma_end));
+			/*
+			 * The vma should either stay at the same address and have the
+			 * original size of 3 pages or we should find the remapped vma
+			 * at the remap destination address with size of 1 page.
+			 */
+			ASSERT_TRUE((vma_start == self->last_line.start_addr &&
+				     vma_end - vma_start == self->page_size * 3) ||
+				    (vma_start == self->last_line.start_addr + self->page_size &&
+				     vma_end - vma_start == self->page_size));
+		}
 		clock_gettime(CLOCK_MONOTONIC_COARSE, &end_ts);
 		end_test_iteration(&end_ts, self->verbose);
 	} while (end_ts.tv_sec - start_ts.tv_sec < self->duration_sec);
-- 
2.54.0.545.g6539524ca2-goog

Re: [PATCH 1/3] fs/proc/task_mmu: read proc/pid/{smaps|numa_maps} under per-vma lock

[Liam R. Howlett]

On 26/04/24 12:02AM, Suren Baghdasaryan wrote:
> proc/pid/{smaps|numa_maps} can be read using the combination of RCU and
> VMA read locks, similar to proc/pid/maps. RCU is required to safely
> traverse the VMA tree and VMA lock stabilizes the VMA being processed
> and the pagetable walk.
> 
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>

Reviewed-by: Liam R. Howlett <liam@infradead.org>

> ---
>  fs/proc/task_mmu.c | 193 ++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 154 insertions(+), 39 deletions(-)
> 
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 751b9ba160fb..96cfea252db6 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -132,6 +132,22 @@ static void release_task_mempolicy(struct proc_maps_private *priv)
>  
>  #ifdef CONFIG_PER_VMA_LOCK
>  
> +static inline int lock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> +{
> +	int ret = mmap_read_lock_killable(lock_ctx->mm);
> +
> +	if (!ret)
> +		lock_ctx->mmap_locked = true;
> +
> +	return ret;
> +}
> +
> +static inline void unlock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> +{
> +	mmap_read_unlock(lock_ctx->mm);
> +	lock_ctx->mmap_locked = false;
> +}
> +
>  static void reset_lock_ctx(struct proc_maps_locking_ctx *lock_ctx)
>  {
>  	lock_ctx->locked_vma = NULL;
> @@ -146,25 +162,11 @@ static void unlock_ctx_vma(struct proc_maps_locking_ctx *lock_ctx)
>  	}
>  }
>  
> -static const struct seq_operations proc_pid_maps_op;
> -
>  static inline bool lock_vma_range(struct seq_file *m,
>  				  struct proc_maps_locking_ctx *lock_ctx)
>  {
> -	/*
> -	 * smaps and numa_maps perform page table walk, therefore require
> -	 * mmap_lock but maps can be read with locking just the vma and
> -	 * walking the vma tree under rcu read protection.
> -	 */
> -	if (m->op != &proc_pid_maps_op) {
> -		if (mmap_read_lock_killable(lock_ctx->mm))
> -			return false;
> -
> -		lock_ctx->mmap_locked = true;
> -	} else {
> -		rcu_read_lock();
> -		reset_lock_ctx(lock_ctx);
> -	}
> +	rcu_read_lock();
> +	reset_lock_ctx(lock_ctx);
>  
>  	return true;
>  }
> @@ -172,7 +174,7 @@ static inline bool lock_vma_range(struct seq_file *m,
>  static inline void unlock_vma_range(struct proc_maps_locking_ctx *lock_ctx)
>  {
>  	if (lock_ctx->mmap_locked) {
> -		mmap_read_unlock(lock_ctx->mm);
> +		unlock_ctx_mm(lock_ctx);
>  	} else {
>  		unlock_ctx_vma(lock_ctx);
>  		rcu_read_unlock();
> @@ -213,17 +215,45 @@ static inline bool fallback_to_mmap_lock(struct proc_maps_private *priv,
>  	return true;
>  }
>  
> +static inline void drop_rcu(struct proc_maps_private *priv)
> +{
> +	if (priv->lock_ctx.mmap_locked)
> +		return;
> +
> +	rcu_read_unlock();
> +}
> +
> +static inline void reacquire_rcu(struct proc_maps_private *priv)
> +{
> +	if (priv->lock_ctx.mmap_locked)
> +		return;
> +
> +	rcu_read_lock();
> +	/* Reinitialize the iterator. */
> +	vma_iter_set(&priv->iter, priv->lock_ctx.locked_vma->vm_end);
> +}
> +
>  #else /* CONFIG_PER_VMA_LOCK */
>  
> +static inline int lock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> +{
> +	return mmap_read_lock_killable(lock_ctx->mm);
> +}
> +
> +static inline void unlock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> +{
> +	mmap_read_unlock(lock_ctx->mm);
> +}
> +
>  static inline bool lock_vma_range(struct seq_file *m,
>  				  struct proc_maps_locking_ctx *lock_ctx)
>  {
> -	return mmap_read_lock_killable(lock_ctx->mm) == 0;
> +	return lock_ctx_mm(lock_ctx) == 0;
>  }
>  
>  static inline void unlock_vma_range(struct proc_maps_locking_ctx *lock_ctx)
>  {
> -	mmap_read_unlock(lock_ctx->mm);
> +	unlock_ctx_mm(lock_ctx);
>  }
>  
>  static struct vm_area_struct *get_next_vma(struct proc_maps_private *priv,
> @@ -238,6 +268,9 @@ static inline bool fallback_to_mmap_lock(struct proc_maps_private *priv,
>  	return false;
>  }
>  
> +static inline void drop_rcu(struct proc_maps_private *priv) {}
> +static inline void reacquire_rcu(struct proc_maps_private *priv) {}
> +
>  #endif /* CONFIG_PER_VMA_LOCK */
>  
>  static struct vm_area_struct *proc_get_vma(struct seq_file *m, loff_t *ppos)
> @@ -538,12 +571,10 @@ static int query_vma_setup(struct proc_maps_locking_ctx *lock_ctx)
>  
>  static void query_vma_teardown(struct proc_maps_locking_ctx *lock_ctx)
>  {
> -	if (lock_ctx->mmap_locked) {
> -		mmap_read_unlock(lock_ctx->mm);
> -		lock_ctx->mmap_locked = false;
> -	} else {
> +	if (lock_ctx->mmap_locked)
> +		unlock_ctx_mm(lock_ctx);
> +	else
>  		unlock_ctx_vma(lock_ctx);
> -	}
>  }
>  
>  static struct vm_area_struct *query_vma_find_by_addr(struct proc_maps_locking_ctx *lock_ctx,
> @@ -1280,16 +1311,64 @@ static const struct mm_walk_ops smaps_shmem_walk_ops = {
>  	.walk_lock		= PGWALK_RDLOCK,
>  };
>  
> +#ifdef CONFIG_PER_VMA_LOCK
> +
> +static const struct mm_walk_ops smaps_walk_vma_lock_ops = {
> +	.pmd_entry		= smaps_pte_range,
> +	.hugetlb_entry		= smaps_hugetlb_range,
> +	.walk_lock		= PGWALK_VMA_RDLOCK_VERIFY,
> +};
> +
> +static const struct mm_walk_ops smaps_shmem_walk_vma_lock_ops = {
> +	.pmd_entry		= smaps_pte_range,
> +	.hugetlb_entry		= smaps_hugetlb_range,
> +	.pte_hole		= smaps_pte_hole,
> +	.walk_lock		= PGWALK_VMA_RDLOCK_VERIFY,
> +};
> +
> +static inline const struct mm_walk_ops *
> +get_smaps_walk_ops(struct proc_maps_private *priv)
> +{
> +	if (priv->lock_ctx.mmap_locked)
> +		return &smaps_walk_ops;
> +	return &smaps_walk_vma_lock_ops;
> +}
> +
> +static inline const struct mm_walk_ops *
> +get_smaps_shmem_walk_ops(struct proc_maps_private *priv)
> +{
> +	if (priv->lock_ctx.mmap_locked)
> +		return  &smaps_shmem_walk_ops;
> +	return &smaps_shmem_walk_vma_lock_ops;
> +}
> +
> +#else /* CONFIG_PER_VMA_LOCK */
> +
> +static inline const struct mm_walk_ops *
> +get_smaps_walk_ops(struct proc_maps_private *priv)
> +{
> +	return &smaps_walk_ops;
> +}
> +
> +static inline const struct mm_walk_ops *
> +get_smaps_shmem_walk_ops(struct proc_maps_private *priv)
> +{
> +	return &smaps_shmem_walk_ops;
> +}
> +
> +#endif /* CONFIG_PER_VMA_LOCK */
> +
>  /*
>   * Gather mem stats from @vma with the indicated beginning
>   * address @start, and keep them in @mss.
>   *
>   * Use vm_start of @vma as the beginning address if @start is 0.
>   */
> -static void smap_gather_stats(struct vm_area_struct *vma,
> -		struct mem_size_stats *mss, unsigned long start)
> +static void smap_gather_stats(struct proc_maps_private *priv,
> +			      struct vm_area_struct *vma,
> +			      struct mem_size_stats *mss, unsigned long start)
>  {
> -	const struct mm_walk_ops *ops = &smaps_walk_ops;
> +	const struct mm_walk_ops *ops = get_smaps_walk_ops(priv);
>  
>  	/* Invalid start */
>  	if (start >= vma->vm_end)
> @@ -1312,15 +1391,24 @@ static void smap_gather_stats(struct vm_area_struct *vma,
>  					!(vma->vm_flags & VM_WRITE))) {
>  			mss->swap += shmem_swapped;
>  		} else {
> -			ops = &smaps_shmem_walk_ops;
> +			ops = get_smaps_shmem_walk_ops(priv);
>  		}
>  	}
>  
> -	/* mmap_lock is held in m_start */
> +	/* Skip walking pages if gate VMA */
> +	if (vma == get_gate_vma(priv->lock_ctx.mm))
> +		return;
> +
> +	/*
> +	 * Need to drop RCU read lock before the walk due to possibility of sleep.
> +	 * Note that the VMA is still locked.
> +	 */
> +	drop_rcu(priv);
>  	if (!start)
>  		walk_page_vma(vma, ops, mss);
>  	else
>  		walk_page_range(vma->vm_mm, start, vma->vm_end, ops, mss);
> +	reacquire_rcu(priv);
>  }
>  
>  #define SEQ_PUT_DEC(str, val) \
> @@ -1369,10 +1457,11 @@ static void __show_smap(struct seq_file *m, const struct mem_size_stats *mss,
>  
>  static int show_smap(struct seq_file *m, void *v)
>  {
> +	struct proc_maps_private *priv = m->private;
>  	struct vm_area_struct *vma = v;
>  	struct mem_size_stats mss = {};
>  
> -	smap_gather_stats(vma, &mss, 0);
> +	smap_gather_stats(priv, vma, &mss, 0);
>  
>  	show_map_vma(m, vma);
>  
> @@ -1413,7 +1502,7 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
>  		goto out_put_task;
>  	}
>  
> -	ret = mmap_read_lock_killable(mm);
> +	ret = lock_ctx_mm(&priv->lock_ctx);
>  	if (ret)
>  		goto out_put_mm;
>  
> @@ -1425,7 +1514,7 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
>  
>  	vma_start = vma->vm_start;
>  	do {
> -		smap_gather_stats(vma, &mss, 0);
> +		smap_gather_stats(priv, vma, &mss, 0);
>  		last_vma_end = vma->vm_end;
>  
>  		/*
> @@ -1434,8 +1523,8 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
>  		 */
>  		if (mmap_lock_is_contended(mm)) {
>  			vma_iter_invalidate(&vmi);
> -			mmap_read_unlock(mm);
> -			ret = mmap_read_lock_killable(mm);
> +			unlock_ctx_mm(&priv->lock_ctx);
> +			ret = lock_ctx_mm(&priv->lock_ctx);
>  			if (ret) {
>  				release_task_mempolicy(priv);
>  				goto out_put_mm;
> @@ -1484,14 +1573,14 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
>  
>  			/* Case 1 and 2 above */
>  			if (vma->vm_start >= last_vma_end) {
> -				smap_gather_stats(vma, &mss, 0);
> +				smap_gather_stats(priv, vma, &mss, 0);
>  				last_vma_end = vma->vm_end;
>  				continue;
>  			}
>  
>  			/* Case 4 above */
>  			if (vma->vm_end > last_vma_end) {
> -				smap_gather_stats(vma, &mss, last_vma_end);
> +				smap_gather_stats(priv, vma, &mss, last_vma_end);
>  				last_vma_end = vma->vm_end;
>  			}
>  		}
> @@ -1505,7 +1594,7 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
>  	__show_smap(m, &mss, true);
>  
>  	release_task_mempolicy(priv);
> -	mmap_read_unlock(mm);
> +	unlock_ctx_mm(&priv->lock_ctx);
>  
>  out_put_mm:
>  	mmput(mm);
> @@ -3291,6 +3380,31 @@ static const struct mm_walk_ops show_numa_ops = {
>  	.walk_lock = PGWALK_RDLOCK,
>  };
>  
> +#ifdef CONFIG_PER_VMA_LOCK
> +static const struct mm_walk_ops show_numa_vma_lock_ops = {
> +	.hugetlb_entry = gather_hugetlb_stats,
> +	.pmd_entry = gather_pte_stats,
> +	.walk_lock = PGWALK_VMA_RDLOCK_VERIFY,
> +};
> +
> +static inline const struct mm_walk_ops *
> +get_show_numa_ops(struct proc_maps_private *priv)
> +{
> +	if (priv->lock_ctx.mmap_locked)
> +		return &show_numa_ops;
> +	return &show_numa_vma_lock_ops;
> +}
> +
> +#else /* CONFIG_PER_VMA_LOCK */
> +
> +static inline const struct mm_walk_ops *
> +get_show_numa_ops(struct proc_maps_private *priv)
> +{
> +	return &show_numa_ops;
> +}
> +
> +#endif /* CONFIG_PER_VMA_LOCK */
> +
>  /*
>   * Display pages allocated per node and memory policy via /proc.
>   */
> @@ -3335,8 +3449,9 @@ static int show_numa_map(struct seq_file *m, void *v)
>  	if (is_vm_hugetlb_page(vma))
>  		seq_puts(m, " huge");
>  
> -	/* mmap_lock is held by m_start */
> -	walk_page_vma(vma, &show_numa_ops, md);
> +	drop_rcu(proc_priv);
> +	walk_page_vma(vma, get_show_numa_ops(proc_priv), md);
> +	reacquire_rcu(proc_priv);
>  
>  	if (!md->pages)
>  		goto out;
> -- 
> 2.54.0.545.g6539524ca2-goog
> 

Re: [PATCH 2/3] selftests/proc: ensure the test is performed at the right page boundary

[Liam R. Howlett]

On 26/04/24 12:02AM, Suren Baghdasaryan wrote:
> When running tearing tests we need to ensure the pages we use include
> VMAs that were mapped by the child process for this test. Currently we
> always use the first two pages, checking VMAs at their boundaries and
> this works, however once we add tests for /proc/pid/smaps, the first two
> pages might not contain the VMAs that child modifies.
> Locate the page that contains the first VMA mapped by the child and use
> that and the next page for the test.
> 
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>

Reviewed-by: Liam R. Howlett <liam@infradead.org>

> ---
>  tools/testing/selftests/proc/proc-maps-race.c | 78 ++++++++++++++++---
>  1 file changed, 68 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/testing/selftests/proc/proc-maps-race.c b/tools/testing/selftests/proc/proc-maps-race.c
> index a734553718da..c5031b0593b7 100644
> --- a/tools/testing/selftests/proc/proc-maps-race.c
> +++ b/tools/testing/selftests/proc/proc-maps-race.c
> @@ -39,6 +39,13 @@
>  #include <sys/types.h>
>  #include <sys/wait.h>
>  
> +#define min(a, b) \
> +	({ \
> +		typeof(a) _a = (a); \
> +		typeof(b) _b = (b); \
> +		_a < _b ? _a : _b; \
> +	})
> +

Another implementation of min in the selftests.  I guess it's somewhat
unavoidable.

>  /* /proc/pid/maps parsing routines */
>  struct page_content {
>  	char *data;
> @@ -77,6 +84,7 @@ FIXTURE(proc_maps_race)
>  	struct line_content first_line;
>  	unsigned long duration_sec;
>  	int shared_mem_size;
> +	int skip_pages;
>  	int page_size;
>  	int vma_count;
>  	bool verbose;
> @@ -105,27 +113,68 @@ struct vma_modifier_info {
>  	void *child_mapped_addr[];
>  };
>  
> -
> -static bool read_two_pages(FIXTURE_DATA(proc_maps_race) *self)
> +static bool read_page(FIXTURE_DATA(proc_maps_race) *self,
> +		      struct page_content *page)
>  {
>  	ssize_t  bytes_read;
>  
> -	if (lseek(self->maps_fd, 0, SEEK_SET) < 0)
> +	bytes_read = read(self->maps_fd, page->data, self->page_size);
> +	if (bytes_read <= 0)
>  		return false;
>  
> -	bytes_read = read(self->maps_fd, self->page1.data, self->page_size);
> -	if (bytes_read <= 0)
> +	/* Make sure data always ends with a newline character. */
> +	if (page->data[bytes_read - 1] != '\n')
>  		return false;
>  
> -	self->page1.size = bytes_read;
> +	page->size = bytes_read;
>  
> -	bytes_read = read(self->maps_fd, self->page2.data, self->page_size);
> -	if (bytes_read <= 0)
> +	return true;
> +}
> +
> +static int locate_containing_page(FIXTURE_DATA(proc_maps_race) *self,
> +				  unsigned long addr, unsigned long size)
> +{
> +	unsigned long start, end;
> +	int page = 0;
> +
> +	if (lseek(self->maps_fd, 0, SEEK_SET) < 0)
> +		return -1;
> +
> +	while (true) {
> +		char *curr_pos;
> +		char *end_pos;
> +
> +		if (!read_page(self, &self->page1))
> +			return -1;
> +
> +		curr_pos = self->page1.data;
> +		end_pos = self->page1.data + self->page1.size;
> +		while (curr_pos < end_pos) {
> +			if (sscanf(curr_pos, "%lx-%lx", &start, &end) == 2 &&
> +			    start == addr && end == addr + size)
> +				return page;
> +
> +			curr_pos = strchr(curr_pos, '\n');
> +			if (!curr_pos)
> +				break;
> +			curr_pos++;
> +		}
> +		page++;
> +	}
> +
> +	return 0;
> +}
> +
> +static bool read_two_pages(FIXTURE_DATA(proc_maps_race) *self)
> +{
> +	if (lseek(self->maps_fd, 0, SEEK_SET) < 0)
>  		return false;
>  
> -	self->page2.size = bytes_read;
> +	for (int i = 0; i < self->skip_pages; i++)
> +		if (!read_page(self, &self->page1))
> +			return false;
>  
> -	return true;
> +	return read_page(self, &self->page1) && read_page(self, &self->page2);
>  }
>  
>  static void copy_first_line(struct page_content *page, char *first_line)
> @@ -418,6 +467,8 @@ FIXTURE_SETUP(proc_maps_race)
>  	struct vma_modifier_info *mod_info;
>  	pthread_mutexattr_t mutex_attr;
>  	pthread_condattr_t cond_attr;
> +	unsigned long first_map_addr;
> +	unsigned long last_map_addr;
>  	unsigned long duration_sec;
>  	char fname[32];
>  
> @@ -502,6 +553,13 @@ FIXTURE_SETUP(proc_maps_race)
>  	self->page2.data = malloc(self->page_size);
>  	ASSERT_NE(self->page2.data, NULL);
>  
> +	first_map_addr = (unsigned long)mod_info->child_mapped_addr[0];
> +	last_map_addr = (unsigned long)mod_info->child_mapped_addr[mod_info->vma_count - 1];
> +
> +	self->skip_pages = locate_containing_page(self,
> +					min(first_map_addr, last_map_addr),
> +					self->page_size * 3);
> +	ASSERT_NE(self->skip_pages, -1);
>  	ASSERT_TRUE(read_boundary_lines(self, &self->last_line, &self->first_line));
>  
>  	/*
> -- 
> 2.54.0.545.g6539524ca2-goog
> 

Re: [PATCH 3/3] selftests/proc: add /proc/pid/smaps tearing tests

[Liam R. Howlett]

On 26/04/24 12:02AM, Suren Baghdasaryan wrote:
> Add MAPS_FILE environment variable to select whether the tests to be run
> for /proc/pid/maps or /proc/pid/smaps. Add support for testing smaps file
> by reusing the same logic as with maps file but skipping all the data
> except for the VMA addresses, which are the only part relevant for the
> tearing tests. Skip PROCMAP_QUERY parts of the tests because smaps does
> not implement that ioctl.
> 
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>

One comment question, otherwise..

Reviewed-by: Liam R. Howlett <liam@infradead.org>

> ---
>  tools/testing/selftests/proc/proc-maps-race.c | 176 +++++++++++++-----
>  1 file changed, 132 insertions(+), 44 deletions(-)
> 
> diff --git a/tools/testing/selftests/proc/proc-maps-race.c b/tools/testing/selftests/proc/proc-maps-race.c
> index c5031b0593b7..b677bc23cab9 100644
> --- a/tools/testing/selftests/proc/proc-maps-race.c
> +++ b/tools/testing/selftests/proc/proc-maps-race.c
> @@ -17,8 +17,8 @@
>   */
>  /*
>   * Fork a child that concurrently modifies address space while the main
> - * process is reading /proc/$PID/maps and verifying the results. Address
> - * space modifications include:
> + * process is reading /proc/$PID/maps and /proc/$PID/smaps, verifying the
                                         ^^^ and? Should this be or?  It
                                         doesn't match your commit
                                         message.


> + * results. Address space modifications include:
>   *     VMA splitting and merging
>   *
>   */
> @@ -73,6 +73,11 @@ enum test_state {
>  	TEST_DONE,
>  };
>  
> +enum maps_file {
> +	MAPS,
> +	SMAPS,
> +};
> +
>  struct vma_modifier_info;
>  
>  FIXTURE(proc_maps_race)
> @@ -83,6 +88,7 @@ FIXTURE(proc_maps_race)
>  	struct line_content last_line;
>  	struct line_content first_line;
>  	unsigned long duration_sec;
> +	enum maps_file maps_file;
>  	int shared_mem_size;
>  	int skip_pages;
>  	int page_size;
> @@ -199,6 +205,57 @@ static void copy_last_line(struct page_content *page, char *last_line)
>  	last_line[end - pos] = '\0';
>  }
>  
> +static bool copy_first_entry(struct page_content *page, char *first_line)
> +{
> +	const char *start_pos = page->data;
> +
> +	while (start_pos < page->data + page->size) {
> +		unsigned long start_addr;
> +		unsigned long end_addr;
> +		char *end_pos;
> +
> +		end_pos = strchr(start_pos, '\n');
> +		if (!end_pos)
> +			break;
> +
> +		if (sscanf(start_pos, "%lx-%lx", &start_addr, &end_addr) == 2) {
> +			strncpy(first_line, start_pos, end_pos - start_pos);
> +			first_line[end_pos - start_pos] = '\0';
> +			return true;
> +		}
> +
> +		start_pos = end_pos + 1;
> +	}
> +
> +	return false;
> +}
> +
> +static bool copy_last_entry(struct page_content *page, char *last_line)
> +{
> +	const char *end_pos = page->data + page->size - 1;
> +	const char *start_pos;
> +
> +	while (end_pos > page->data) {
> +		unsigned long start_addr;
> +		unsigned long end_addr;
> +
> +		/* skip last newline */
> +		start_pos = end_pos - 1;
> +		/* search previous newline */
> +		while (start_pos > page->data && start_pos[-1] != '\n')
> +			start_pos--;
> +		if (sscanf(start_pos, "%lx-%lx", &start_addr, &end_addr) == 2) {
> +			strncpy(last_line, start_pos, end_pos - start_pos);
> +			last_line[end_pos - start_pos] = '\0';
> +			return true;
> +		}
> +
> +		end_pos = start_pos - 1;
> +	}
> +
> +	return false;
> +}
> +
>  /* Read the last line of the first page and the first line of the second page */
>  static bool read_boundary_lines(FIXTURE_DATA(proc_maps_race) *self,
>  				struct line_content *last_line,
> @@ -207,8 +264,16 @@ static bool read_boundary_lines(FIXTURE_DATA(proc_maps_race) *self,
>  	if (!read_two_pages(self))
>  		return false;
>  
> -	copy_last_line(&self->page1, last_line->text);
> -	copy_first_line(&self->page2, first_line->text);
> +	if (self->maps_file == MAPS) {
> +		copy_last_line(&self->page1, last_line->text);
> +		copy_first_line(&self->page2, first_line->text);
> +	} else if (self->maps_file == SMAPS) {
> +		if (!copy_last_entry(&self->page1, last_line->text) ||
> +		    !copy_first_entry(&self->page2, first_line->text))
> +			return false;
> +	} else {
> +		return false;
> +	}
>  
>  	return sscanf(last_line->text, "%lx-%lx", &last_line->start_addr,
>  		      &last_line->end_addr) == 2 &&
> @@ -464,6 +529,7 @@ FIXTURE_SETUP(proc_maps_race)
>  {
>  	const char *verbose = getenv("VERBOSE");
>  	const char *duration = getenv("DURATION");
> +	const char *maps_file = getenv("MAPS_FILE");
>  	struct vma_modifier_info *mod_info;
>  	pthread_mutexattr_t mutex_attr;
>  	pthread_condattr_t cond_attr;
> @@ -474,6 +540,18 @@ FIXTURE_SETUP(proc_maps_race)
>  
>  	self->page_size = (unsigned long)sysconf(_SC_PAGESIZE);
>  	self->verbose = verbose && !strncmp(verbose, "1", 1);
> +	if (maps_file) {
> +		if (!strcmp(maps_file, "maps")) {
> +			self->maps_file = MAPS;
> +		} else if (!strcmp(maps_file, "smaps")) {
> +			self->maps_file = SMAPS;
> +		} else {
> +			ksft_print_msg("Unknown maps file %s\n", maps_file);
> +			ksft_exit_fail();
> +		}
> +	} else {
> +		self->maps_file = MAPS;
> +	}
>  	duration_sec = duration ? atol(duration) : 0;
>  	self->duration_sec = duration_sec ? duration_sec : 5UL;
>  
> @@ -540,7 +618,16 @@ FIXTURE_SETUP(proc_maps_race)
>  		exit(0);
>  	}
>  
> -	sprintf(fname, "/proc/%d/maps", self->pid);
> +	switch (self->maps_file) {
> +	case MAPS:
> +		sprintf(fname, "/proc/%d/maps", self->pid);
> +		break;
> +	case SMAPS:
> +		sprintf(fname, "/proc/%d/smaps", self->pid);
> +		break;
> +	default:
> +		ksft_exit_fail();
> +	}
>  	self->maps_fd = open(fname, O_RDONLY);
>  	ASSERT_NE(self->maps_fd, -1);
>  
> @@ -675,20 +762,20 @@ TEST_F(proc_maps_race, test_maps_tearing_from_split)
>  		last_line_changed = strcmp(new_last_line.text, self->last_line.text) != 0;
>  		first_line_changed = strcmp(new_first_line.text, self->first_line.text) != 0;
>  		ASSERT_EQ(last_line_changed, first_line_changed);
> -
> -		/* Check if PROCMAP_QUERY ioclt() finds the right VMA */
> -		ASSERT_TRUE(query_addr_at(self->maps_fd, mod_info->addr + self->page_size,
> -					  &vma_start, &vma_end));
> -		/*
> -		 * The vma at the split address can be either the same as
> -		 * original one (if read before the split) or the same as the
> -		 * first line in the second page (if read after the split).
> -		 */
> -		ASSERT_TRUE((vma_start == self->last_line.start_addr &&
> -			     vma_end == self->last_line.end_addr) ||
> -			    (vma_start == split_first_line.start_addr &&
> -			     vma_end == split_first_line.end_addr));
> -
> +		if (self->maps_file == MAPS) {
> +			/* Check if PROCMAP_QUERY ioclt() finds the right VMA */
> +			ASSERT_TRUE(query_addr_at(self->maps_fd, mod_info->addr + self->page_size,
> +						  &vma_start, &vma_end));
> +			/*
> +			 * The vma at the split address can be either the same as
> +			 * original one (if read before the split) or the same as the
> +			 * first line in the second page (if read after the split).
> +			 */
> +			ASSERT_TRUE((vma_start == self->last_line.start_addr &&
> +				     vma_end == self->last_line.end_addr) ||
> +				    (vma_start == split_first_line.start_addr &&
> +				     vma_end == split_first_line.end_addr));
> +		}
>  		clock_gettime(CLOCK_MONOTONIC_COARSE, &end_ts);
>  		end_test_iteration(&end_ts, self->verbose);
>  	} while (end_ts.tv_sec - start_ts.tv_sec < self->duration_sec);
> @@ -758,17 +845,18 @@ TEST_F(proc_maps_race, test_maps_tearing_from_resize)
>  					strcmp(new_first_line.text, restored_first_line.text),
>  					"Expand result invalid", self));
>  		}
> -
> -		/* Check if PROCMAP_QUERY ioclt() finds the right VMA */
> -		ASSERT_TRUE(query_addr_at(self->maps_fd, mod_info->addr, &vma_start, &vma_end));
> -		/*
> -		 * The vma should stay at the same address and have either the
> -		 * original size of 3 pages or 1 page if read after shrinking.
> -		 */
> -		ASSERT_TRUE(vma_start == self->last_line.start_addr &&
> -			    (vma_end - vma_start == self->page_size * 3 ||
> -			     vma_end - vma_start == self->page_size));
> -
> +		if (self->maps_file == MAPS) {
> +			/* Check if PROCMAP_QUERY ioclt() finds the right VMA */
> +			ASSERT_TRUE(query_addr_at(self->maps_fd, mod_info->addr,
> +						  &vma_start, &vma_end));
> +			/*
> +			 * The vma should stay at the same address and have either the
> +			 * original size of 3 pages or 1 page if read after shrinking.
> +			 */
> +			ASSERT_TRUE(vma_start == self->last_line.start_addr &&
> +				    (vma_end - vma_start == self->page_size * 3 ||
> +				     vma_end - vma_start == self->page_size));
> +		}
>  		clock_gettime(CLOCK_MONOTONIC_COARSE, &end_ts);
>  		end_test_iteration(&end_ts, self->verbose);
>  	} while (end_ts.tv_sec - start_ts.tv_sec < self->duration_sec);
> @@ -838,20 +926,20 @@ TEST_F(proc_maps_race, test_maps_tearing_from_remap)
>  					strcmp(new_first_line.text, restored_first_line.text),
>  					"Remap restore result invalid", self));
>  		}
> -
> -		/* Check if PROCMAP_QUERY ioclt() finds the right VMA */
> -		ASSERT_TRUE(query_addr_at(self->maps_fd, mod_info->addr + self->page_size,
> -					  &vma_start, &vma_end));
> -		/*
> -		 * The vma should either stay at the same address and have the
> -		 * original size of 3 pages or we should find the remapped vma
> -		 * at the remap destination address with size of 1 page.
> -		 */
> -		ASSERT_TRUE((vma_start == self->last_line.start_addr &&
> -			     vma_end - vma_start == self->page_size * 3) ||
> -			    (vma_start == self->last_line.start_addr + self->page_size &&
> -			     vma_end - vma_start == self->page_size));
> -
> +		if (self->maps_file == MAPS) {
> +			/* Check if PROCMAP_QUERY ioclt() finds the right VMA */
> +			ASSERT_TRUE(query_addr_at(self->maps_fd, mod_info->addr + self->page_size,
> +						  &vma_start, &vma_end));
> +			/*
> +			 * The vma should either stay at the same address and have the
> +			 * original size of 3 pages or we should find the remapped vma
> +			 * at the remap destination address with size of 1 page.
> +			 */
> +			ASSERT_TRUE((vma_start == self->last_line.start_addr &&
> +				     vma_end - vma_start == self->page_size * 3) ||
> +				    (vma_start == self->last_line.start_addr + self->page_size &&
> +				     vma_end - vma_start == self->page_size));
> +		}
>  		clock_gettime(CLOCK_MONOTONIC_COARSE, &end_ts);
>  		end_test_iteration(&end_ts, self->verbose);
>  	} while (end_ts.tv_sec - start_ts.tv_sec < self->duration_sec);
> -- 
> 2.54.0.545.g6539524ca2-goog
> 

Re: [PATCH 2/3] selftests/proc: ensure the test is performed at the right page boundary

[Suren Baghdasaryan]

On Fri, Apr 24, 2026 at 11:21 AM Liam R. Howlett <liam@infradead.org> wrote:
>
> On 26/04/24 12:02AM, Suren Baghdasaryan wrote:
> > When running tearing tests we need to ensure the pages we use include
> > VMAs that were mapped by the child process for this test. Currently we
> > always use the first two pages, checking VMAs at their boundaries and
> > this works, however once we add tests for /proc/pid/smaps, the first two
> > pages might not contain the VMAs that child modifies.
> > Locate the page that contains the first VMA mapped by the child and use
> > that and the next page for the test.
> >
> > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
>
> Reviewed-by: Liam R. Howlett <liam@infradead.org>

Thanks!

>
> > ---
> >  tools/testing/selftests/proc/proc-maps-race.c | 78 ++++++++++++++++---
> >  1 file changed, 68 insertions(+), 10 deletions(-)
> >
> > diff --git a/tools/testing/selftests/proc/proc-maps-race.c b/tools/testing/selftests/proc/proc-maps-race.c
> > index a734553718da..c5031b0593b7 100644
> > --- a/tools/testing/selftests/proc/proc-maps-race.c
> > +++ b/tools/testing/selftests/proc/proc-maps-race.c
> > @@ -39,6 +39,13 @@
> >  #include <sys/types.h>
> >  #include <sys/wait.h>
> >
> > +#define min(a, b) \
> > +     ({ \
> > +             typeof(a) _a = (a); \
> > +             typeof(b) _b = (b); \
> > +             _a < _b ? _a : _b; \
> > +     })
> > +
>
> Another implementation of min in the selftests.  I guess it's somewhat
> unavoidable.

Yeah, I was looking for a good place to put it and reuse in other
tests but the only common part it seems is the harness and it didn't
feel like the right place to put it.

>
> >  /* /proc/pid/maps parsing routines */
> >  struct page_content {
> >       char *data;
> > @@ -77,6 +84,7 @@ FIXTURE(proc_maps_race)
> >       struct line_content first_line;
> >       unsigned long duration_sec;
> >       int shared_mem_size;
> > +     int skip_pages;
> >       int page_size;
> >       int vma_count;
> >       bool verbose;
> > @@ -105,27 +113,68 @@ struct vma_modifier_info {
> >       void *child_mapped_addr[];
> >  };
> >
> > -
> > -static bool read_two_pages(FIXTURE_DATA(proc_maps_race) *self)
> > +static bool read_page(FIXTURE_DATA(proc_maps_race) *self,
> > +                   struct page_content *page)
> >  {
> >       ssize_t  bytes_read;
> >
> > -     if (lseek(self->maps_fd, 0, SEEK_SET) < 0)
> > +     bytes_read = read(self->maps_fd, page->data, self->page_size);
> > +     if (bytes_read <= 0)
> >               return false;
> >
> > -     bytes_read = read(self->maps_fd, self->page1.data, self->page_size);
> > -     if (bytes_read <= 0)
> > +     /* Make sure data always ends with a newline character. */
> > +     if (page->data[bytes_read - 1] != '\n')
> >               return false;
> >
> > -     self->page1.size = bytes_read;
> > +     page->size = bytes_read;
> >
> > -     bytes_read = read(self->maps_fd, self->page2.data, self->page_size);
> > -     if (bytes_read <= 0)
> > +     return true;
> > +}
> > +
> > +static int locate_containing_page(FIXTURE_DATA(proc_maps_race) *self,
> > +                               unsigned long addr, unsigned long size)
> > +{
> > +     unsigned long start, end;
> > +     int page = 0;
> > +
> > +     if (lseek(self->maps_fd, 0, SEEK_SET) < 0)
> > +             return -1;
> > +
> > +     while (true) {
> > +             char *curr_pos;
> > +             char *end_pos;
> > +
> > +             if (!read_page(self, &self->page1))
> > +                     return -1;
> > +
> > +             curr_pos = self->page1.data;
> > +             end_pos = self->page1.data + self->page1.size;
> > +             while (curr_pos < end_pos) {
> > +                     if (sscanf(curr_pos, "%lx-%lx", &start, &end) == 2 &&
> > +                         start == addr && end == addr + size)
> > +                             return page;
> > +
> > +                     curr_pos = strchr(curr_pos, '\n');
> > +                     if (!curr_pos)
> > +                             break;
> > +                     curr_pos++;
> > +             }
> > +             page++;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static bool read_two_pages(FIXTURE_DATA(proc_maps_race) *self)
> > +{
> > +     if (lseek(self->maps_fd, 0, SEEK_SET) < 0)
> >               return false;
> >
> > -     self->page2.size = bytes_read;
> > +     for (int i = 0; i < self->skip_pages; i++)
> > +             if (!read_page(self, &self->page1))
> > +                     return false;
> >
> > -     return true;
> > +     return read_page(self, &self->page1) && read_page(self, &self->page2);
> >  }
> >
> >  static void copy_first_line(struct page_content *page, char *first_line)
> > @@ -418,6 +467,8 @@ FIXTURE_SETUP(proc_maps_race)
> >       struct vma_modifier_info *mod_info;
> >       pthread_mutexattr_t mutex_attr;
> >       pthread_condattr_t cond_attr;
> > +     unsigned long first_map_addr;
> > +     unsigned long last_map_addr;
> >       unsigned long duration_sec;
> >       char fname[32];
> >
> > @@ -502,6 +553,13 @@ FIXTURE_SETUP(proc_maps_race)
> >       self->page2.data = malloc(self->page_size);
> >       ASSERT_NE(self->page2.data, NULL);
> >
> > +     first_map_addr = (unsigned long)mod_info->child_mapped_addr[0];
> > +     last_map_addr = (unsigned long)mod_info->child_mapped_addr[mod_info->vma_count - 1];
> > +
> > +     self->skip_pages = locate_containing_page(self,
> > +                                     min(first_map_addr, last_map_addr),
> > +                                     self->page_size * 3);
> > +     ASSERT_NE(self->skip_pages, -1);
> >       ASSERT_TRUE(read_boundary_lines(self, &self->last_line, &self->first_line));
> >
> >       /*
> > --
> > 2.54.0.545.g6539524ca2-goog
> >

Re: [PATCH 3/3] selftests/proc: add /proc/pid/smaps tearing tests

[Suren Baghdasaryan]

On Fri, Apr 24, 2026 at 11:23 AM Liam R. Howlett <liam@infradead.org> wrote:
>
> On 26/04/24 12:02AM, Suren Baghdasaryan wrote:
> > Add MAPS_FILE environment variable to select whether the tests to be run
> > for /proc/pid/maps or /proc/pid/smaps. Add support for testing smaps file
> > by reusing the same logic as with maps file but skipping all the data
> > except for the VMA addresses, which are the only part relevant for the
> > tearing tests. Skip PROCMAP_QUERY parts of the tests because smaps does
> > not implement that ioctl.
> >
> > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
>
> One comment question, otherwise..
>
> Reviewed-by: Liam R. Howlett <liam@infradead.org>
>
> > ---
> >  tools/testing/selftests/proc/proc-maps-race.c | 176 +++++++++++++-----
> >  1 file changed, 132 insertions(+), 44 deletions(-)
> >
> > diff --git a/tools/testing/selftests/proc/proc-maps-race.c b/tools/testing/selftests/proc/proc-maps-race.c
> > index c5031b0593b7..b677bc23cab9 100644
> > --- a/tools/testing/selftests/proc/proc-maps-race.c
> > +++ b/tools/testing/selftests/proc/proc-maps-race.c
> > @@ -17,8 +17,8 @@
> >   */
> >  /*
> >   * Fork a child that concurrently modifies address space while the main
> > - * process is reading /proc/$PID/maps and verifying the results. Address
> > - * space modifications include:
> > + * process is reading /proc/$PID/maps and /proc/$PID/smaps, verifying the
>                                          ^^^ and? Should this be or?  It
>                                          doesn't match your commit
>                                          message.

Quite right. I'll fix it. Need to post a new version anyway since
Sashiko found a couple of issues.

>
>
> > + * results. Address space modifications include:
> >   *     VMA splitting and merging
> >   *
> >   */
> > @@ -73,6 +73,11 @@ enum test_state {
> >       TEST_DONE,
> >  };
> >
> > +enum maps_file {
> > +     MAPS,
> > +     SMAPS,
> > +};
> > +
> >  struct vma_modifier_info;
> >
> >  FIXTURE(proc_maps_race)
> > @@ -83,6 +88,7 @@ FIXTURE(proc_maps_race)
> >       struct line_content last_line;
> >       struct line_content first_line;
> >       unsigned long duration_sec;
> > +     enum maps_file maps_file;
> >       int shared_mem_size;
> >       int skip_pages;
> >       int page_size;
> > @@ -199,6 +205,57 @@ static void copy_last_line(struct page_content *page, char *last_line)
> >       last_line[end - pos] = '\0';
> >  }
> >
> > +static bool copy_first_entry(struct page_content *page, char *first_line)
> > +{
> > +     const char *start_pos = page->data;
> > +
> > +     while (start_pos < page->data + page->size) {
> > +             unsigned long start_addr;
> > +             unsigned long end_addr;
> > +             char *end_pos;
> > +
> > +             end_pos = strchr(start_pos, '\n');
> > +             if (!end_pos)
> > +                     break;
> > +
> > +             if (sscanf(start_pos, "%lx-%lx", &start_addr, &end_addr) == 2) {
> > +                     strncpy(first_line, start_pos, end_pos - start_pos);
> > +                     first_line[end_pos - start_pos] = '\0';
> > +                     return true;
> > +             }
> > +
> > +             start_pos = end_pos + 1;
> > +     }
> > +
> > +     return false;
> > +}
> > +
> > +static bool copy_last_entry(struct page_content *page, char *last_line)
> > +{
> > +     const char *end_pos = page->data + page->size - 1;
> > +     const char *start_pos;
> > +
> > +     while (end_pos > page->data) {
> > +             unsigned long start_addr;
> > +             unsigned long end_addr;
> > +
> > +             /* skip last newline */
> > +             start_pos = end_pos - 1;
> > +             /* search previous newline */
> > +             while (start_pos > page->data && start_pos[-1] != '\n')
> > +                     start_pos--;
> > +             if (sscanf(start_pos, "%lx-%lx", &start_addr, &end_addr) == 2) {
> > +                     strncpy(last_line, start_pos, end_pos - start_pos);
> > +                     last_line[end_pos - start_pos] = '\0';
> > +                     return true;
> > +             }
> > +
> > +             end_pos = start_pos - 1;
> > +     }
> > +
> > +     return false;
> > +}
> > +
> >  /* Read the last line of the first page and the first line of the second page */
> >  static bool read_boundary_lines(FIXTURE_DATA(proc_maps_race) *self,
> >                               struct line_content *last_line,
> > @@ -207,8 +264,16 @@ static bool read_boundary_lines(FIXTURE_DATA(proc_maps_race) *self,
> >       if (!read_two_pages(self))
> >               return false;
> >
> > -     copy_last_line(&self->page1, last_line->text);
> > -     copy_first_line(&self->page2, first_line->text);
> > +     if (self->maps_file == MAPS) {
> > +             copy_last_line(&self->page1, last_line->text);
> > +             copy_first_line(&self->page2, first_line->text);
> > +     } else if (self->maps_file == SMAPS) {
> > +             if (!copy_last_entry(&self->page1, last_line->text) ||
> > +                 !copy_first_entry(&self->page2, first_line->text))
> > +                     return false;
> > +     } else {
> > +             return false;
> > +     }
> >
> >       return sscanf(last_line->text, "%lx-%lx", &last_line->start_addr,
> >                     &last_line->end_addr) == 2 &&
> > @@ -464,6 +529,7 @@ FIXTURE_SETUP(proc_maps_race)
> >  {
> >       const char *verbose = getenv("VERBOSE");
> >       const char *duration = getenv("DURATION");
> > +     const char *maps_file = getenv("MAPS_FILE");
> >       struct vma_modifier_info *mod_info;
> >       pthread_mutexattr_t mutex_attr;
> >       pthread_condattr_t cond_attr;
> > @@ -474,6 +540,18 @@ FIXTURE_SETUP(proc_maps_race)
> >
> >       self->page_size = (unsigned long)sysconf(_SC_PAGESIZE);
> >       self->verbose = verbose && !strncmp(verbose, "1", 1);
> > +     if (maps_file) {
> > +             if (!strcmp(maps_file, "maps")) {
> > +                     self->maps_file = MAPS;
> > +             } else if (!strcmp(maps_file, "smaps")) {
> > +                     self->maps_file = SMAPS;
> > +             } else {
> > +                     ksft_print_msg("Unknown maps file %s\n", maps_file);
> > +                     ksft_exit_fail();
> > +             }
> > +     } else {
> > +             self->maps_file = MAPS;
> > +     }
> >       duration_sec = duration ? atol(duration) : 0;
> >       self->duration_sec = duration_sec ? duration_sec : 5UL;
> >
> > @@ -540,7 +618,16 @@ FIXTURE_SETUP(proc_maps_race)
> >               exit(0);
> >       }
> >
> > -     sprintf(fname, "/proc/%d/maps", self->pid);
> > +     switch (self->maps_file) {
> > +     case MAPS:
> > +             sprintf(fname, "/proc/%d/maps", self->pid);
> > +             break;
> > +     case SMAPS:
> > +             sprintf(fname, "/proc/%d/smaps", self->pid);
> > +             break;
> > +     default:
> > +             ksft_exit_fail();
> > +     }
> >       self->maps_fd = open(fname, O_RDONLY);
> >       ASSERT_NE(self->maps_fd, -1);
> >
> > @@ -675,20 +762,20 @@ TEST_F(proc_maps_race, test_maps_tearing_from_split)
> >               last_line_changed = strcmp(new_last_line.text, self->last_line.text) != 0;
> >               first_line_changed = strcmp(new_first_line.text, self->first_line.text) != 0;
> >               ASSERT_EQ(last_line_changed, first_line_changed);
> > -
> > -             /* Check if PROCMAP_QUERY ioclt() finds the right VMA */
> > -             ASSERT_TRUE(query_addr_at(self->maps_fd, mod_info->addr + self->page_size,
> > -                                       &vma_start, &vma_end));
> > -             /*
> > -              * The vma at the split address can be either the same as
> > -              * original one (if read before the split) or the same as the
> > -              * first line in the second page (if read after the split).
> > -              */
> > -             ASSERT_TRUE((vma_start == self->last_line.start_addr &&
> > -                          vma_end == self->last_line.end_addr) ||
> > -                         (vma_start == split_first_line.start_addr &&
> > -                          vma_end == split_first_line.end_addr));
> > -
> > +             if (self->maps_file == MAPS) {
> > +                     /* Check if PROCMAP_QUERY ioclt() finds the right VMA */
> > +                     ASSERT_TRUE(query_addr_at(self->maps_fd, mod_info->addr + self->page_size,
> > +                                               &vma_start, &vma_end));
> > +                     /*
> > +                      * The vma at the split address can be either the same as
> > +                      * original one (if read before the split) or the same as the
> > +                      * first line in the second page (if read after the split).
> > +                      */
> > +                     ASSERT_TRUE((vma_start == self->last_line.start_addr &&
> > +                                  vma_end == self->last_line.end_addr) ||
> > +                                 (vma_start == split_first_line.start_addr &&
> > +                                  vma_end == split_first_line.end_addr));
> > +             }
> >               clock_gettime(CLOCK_MONOTONIC_COARSE, &end_ts);
> >               end_test_iteration(&end_ts, self->verbose);
> >       } while (end_ts.tv_sec - start_ts.tv_sec < self->duration_sec);
> > @@ -758,17 +845,18 @@ TEST_F(proc_maps_race, test_maps_tearing_from_resize)
> >                                       strcmp(new_first_line.text, restored_first_line.text),
> >                                       "Expand result invalid", self));
> >               }
> > -
> > -             /* Check if PROCMAP_QUERY ioclt() finds the right VMA */
> > -             ASSERT_TRUE(query_addr_at(self->maps_fd, mod_info->addr, &vma_start, &vma_end));
> > -             /*
> > -              * The vma should stay at the same address and have either the
> > -              * original size of 3 pages or 1 page if read after shrinking.
> > -              */
> > -             ASSERT_TRUE(vma_start == self->last_line.start_addr &&
> > -                         (vma_end - vma_start == self->page_size * 3 ||
> > -                          vma_end - vma_start == self->page_size));
> > -
> > +             if (self->maps_file == MAPS) {
> > +                     /* Check if PROCMAP_QUERY ioclt() finds the right VMA */
> > +                     ASSERT_TRUE(query_addr_at(self->maps_fd, mod_info->addr,
> > +                                               &vma_start, &vma_end));
> > +                     /*
> > +                      * The vma should stay at the same address and have either the
> > +                      * original size of 3 pages or 1 page if read after shrinking.
> > +                      */
> > +                     ASSERT_TRUE(vma_start == self->last_line.start_addr &&
> > +                                 (vma_end - vma_start == self->page_size * 3 ||
> > +                                  vma_end - vma_start == self->page_size));
> > +             }
> >               clock_gettime(CLOCK_MONOTONIC_COARSE, &end_ts);
> >               end_test_iteration(&end_ts, self->verbose);
> >       } while (end_ts.tv_sec - start_ts.tv_sec < self->duration_sec);
> > @@ -838,20 +926,20 @@ TEST_F(proc_maps_race, test_maps_tearing_from_remap)
> >                                       strcmp(new_first_line.text, restored_first_line.text),
> >                                       "Remap restore result invalid", self));
> >               }
> > -
> > -             /* Check if PROCMAP_QUERY ioclt() finds the right VMA */
> > -             ASSERT_TRUE(query_addr_at(self->maps_fd, mod_info->addr + self->page_size,
> > -                                       &vma_start, &vma_end));
> > -             /*
> > -              * The vma should either stay at the same address and have the
> > -              * original size of 3 pages or we should find the remapped vma
> > -              * at the remap destination address with size of 1 page.
> > -              */
> > -             ASSERT_TRUE((vma_start == self->last_line.start_addr &&
> > -                          vma_end - vma_start == self->page_size * 3) ||
> > -                         (vma_start == self->last_line.start_addr + self->page_size &&
> > -                          vma_end - vma_start == self->page_size));
> > -
> > +             if (self->maps_file == MAPS) {
> > +                     /* Check if PROCMAP_QUERY ioclt() finds the right VMA */
> > +                     ASSERT_TRUE(query_addr_at(self->maps_fd, mod_info->addr + self->page_size,
> > +                                               &vma_start, &vma_end));
> > +                     /*
> > +                      * The vma should either stay at the same address and have the
> > +                      * original size of 3 pages or we should find the remapped vma
> > +                      * at the remap destination address with size of 1 page.
> > +                      */
> > +                     ASSERT_TRUE((vma_start == self->last_line.start_addr &&
> > +                                  vma_end - vma_start == self->page_size * 3) ||
> > +                                 (vma_start == self->last_line.start_addr + self->page_size &&
> > +                                  vma_end - vma_start == self->page_size));
> > +             }
> >               clock_gettime(CLOCK_MONOTONIC_COARSE, &end_ts);
> >               end_test_iteration(&end_ts, self->verbose);
> >       } while (end_ts.tv_sec - start_ts.tv_sec < self->duration_sec);
> > --
> > 2.54.0.545.g6539524ca2-goog
> >

Re: [PATCH 3/3] selftests/proc: add /proc/pid/smaps tearing tests

[Suren Baghdasaryan]

On Fri, Apr 24, 2026 at 12:59 PM Suren Baghdasaryan <surenb@google.com> wrote:
>
> On Fri, Apr 24, 2026 at 11:23 AM Liam R. Howlett <liam@infradead.org> wrote:
> >
> > On 26/04/24 12:02AM, Suren Baghdasaryan wrote:
> > > Add MAPS_FILE environment variable to select whether the tests to be run
> > > for /proc/pid/maps or /proc/pid/smaps. Add support for testing smaps file
> > > by reusing the same logic as with maps file but skipping all the data
> > > except for the VMA addresses, which are the only part relevant for the
> > > tearing tests. Skip PROCMAP_QUERY parts of the tests because smaps does
> > > not implement that ioctl.
> > >
> > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> >
> > One comment question, otherwise..
> >
> > Reviewed-by: Liam R. Howlett <liam@infradead.org>
> >
> > > ---
> > >  tools/testing/selftests/proc/proc-maps-race.c | 176 +++++++++++++-----
> > >  1 file changed, 132 insertions(+), 44 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/proc/proc-maps-race.c b/tools/testing/selftests/proc/proc-maps-race.c
> > > index c5031b0593b7..b677bc23cab9 100644
> > > --- a/tools/testing/selftests/proc/proc-maps-race.c
> > > +++ b/tools/testing/selftests/proc/proc-maps-race.c
> > > @@ -17,8 +17,8 @@
> > >   */
> > >  /*
> > >   * Fork a child that concurrently modifies address space while the main
> > > - * process is reading /proc/$PID/maps and verifying the results. Address
> > > - * space modifications include:
> > > + * process is reading /proc/$PID/maps and /proc/$PID/smaps, verifying the
> >                                          ^^^ and? Should this be or?  It
> >                                          doesn't match your commit
> >                                          message.
>
> Quite right. I'll fix it. Need to post a new version anyway since
> Sashiko found a couple of issues.

Actually, I realized that I can use FIXTURE_VARIANT to easily run
tests for both smaps and maps in the same run. I think that's a better
option.
v2 has a number of fixups for issues flagged by Sashiko but they are
quite straight-forward, so I think I can keep your Reviewed-by.
Will post an update shortly after I verify that Sashiko is fine with it.

>
> >
> >
> > > + * results. Address space modifications include:
> > >   *     VMA splitting and merging
> > >   *
> > >   */
> > > @@ -73,6 +73,11 @@ enum test_state {
> > >       TEST_DONE,
> > >  };
> > >
> > > +enum maps_file {
> > > +     MAPS,
> > > +     SMAPS,
> > > +};
> > > +
> > >  struct vma_modifier_info;
> > >
> > >  FIXTURE(proc_maps_race)
> > > @@ -83,6 +88,7 @@ FIXTURE(proc_maps_race)
> > >       struct line_content last_line;
> > >       struct line_content first_line;
> > >       unsigned long duration_sec;
> > > +     enum maps_file maps_file;
> > >       int shared_mem_size;
> > >       int skip_pages;
> > >       int page_size;
> > > @@ -199,6 +205,57 @@ static void copy_last_line(struct page_content *page, char *last_line)
> > >       last_line[end - pos] = '\0';
> > >  }
> > >
> > > +static bool copy_first_entry(struct page_content *page, char *first_line)
> > > +{
> > > +     const char *start_pos = page->data;
> > > +
> > > +     while (start_pos < page->data + page->size) {
> > > +             unsigned long start_addr;
> > > +             unsigned long end_addr;
> > > +             char *end_pos;
> > > +
> > > +             end_pos = strchr(start_pos, '\n');
> > > +             if (!end_pos)
> > > +                     break;
> > > +
> > > +             if (sscanf(start_pos, "%lx-%lx", &start_addr, &end_addr) == 2) {
> > > +                     strncpy(first_line, start_pos, end_pos - start_pos);
> > > +                     first_line[end_pos - start_pos] = '\0';
> > > +                     return true;
> > > +             }
> > > +
> > > +             start_pos = end_pos + 1;
> > > +     }
> > > +
> > > +     return false;
> > > +}
> > > +
> > > +static bool copy_last_entry(struct page_content *page, char *last_line)
> > > +{
> > > +     const char *end_pos = page->data + page->size - 1;
> > > +     const char *start_pos;
> > > +
> > > +     while (end_pos > page->data) {
> > > +             unsigned long start_addr;
> > > +             unsigned long end_addr;
> > > +
> > > +             /* skip last newline */
> > > +             start_pos = end_pos - 1;
> > > +             /* search previous newline */
> > > +             while (start_pos > page->data && start_pos[-1] != '\n')
> > > +                     start_pos--;
> > > +             if (sscanf(start_pos, "%lx-%lx", &start_addr, &end_addr) == 2) {
> > > +                     strncpy(last_line, start_pos, end_pos - start_pos);
> > > +                     last_line[end_pos - start_pos] = '\0';
> > > +                     return true;
> > > +             }
> > > +
> > > +             end_pos = start_pos - 1;
> > > +     }
> > > +
> > > +     return false;
> > > +}
> > > +
> > >  /* Read the last line of the first page and the first line of the second page */
> > >  static bool read_boundary_lines(FIXTURE_DATA(proc_maps_race) *self,
> > >                               struct line_content *last_line,
> > > @@ -207,8 +264,16 @@ static bool read_boundary_lines(FIXTURE_DATA(proc_maps_race) *self,
> > >       if (!read_two_pages(self))
> > >               return false;
> > >
> > > -     copy_last_line(&self->page1, last_line->text);
> > > -     copy_first_line(&self->page2, first_line->text);
> > > +     if (self->maps_file == MAPS) {
> > > +             copy_last_line(&self->page1, last_line->text);
> > > +             copy_first_line(&self->page2, first_line->text);
> > > +     } else if (self->maps_file == SMAPS) {
> > > +             if (!copy_last_entry(&self->page1, last_line->text) ||
> > > +                 !copy_first_entry(&self->page2, first_line->text))
> > > +                     return false;
> > > +     } else {
> > > +             return false;
> > > +     }
> > >
> > >       return sscanf(last_line->text, "%lx-%lx", &last_line->start_addr,
> > >                     &last_line->end_addr) == 2 &&
> > > @@ -464,6 +529,7 @@ FIXTURE_SETUP(proc_maps_race)
> > >  {
> > >       const char *verbose = getenv("VERBOSE");
> > >       const char *duration = getenv("DURATION");
> > > +     const char *maps_file = getenv("MAPS_FILE");
> > >       struct vma_modifier_info *mod_info;
> > >       pthread_mutexattr_t mutex_attr;
> > >       pthread_condattr_t cond_attr;
> > > @@ -474,6 +540,18 @@ FIXTURE_SETUP(proc_maps_race)
> > >
> > >       self->page_size = (unsigned long)sysconf(_SC_PAGESIZE);
> > >       self->verbose = verbose && !strncmp(verbose, "1", 1);
> > > +     if (maps_file) {
> > > +             if (!strcmp(maps_file, "maps")) {
> > > +                     self->maps_file = MAPS;
> > > +             } else if (!strcmp(maps_file, "smaps")) {
> > > +                     self->maps_file = SMAPS;
> > > +             } else {
> > > +                     ksft_print_msg("Unknown maps file %s\n", maps_file);
> > > +                     ksft_exit_fail();
> > > +             }
> > > +     } else {
> > > +             self->maps_file = MAPS;
> > > +     }
> > >       duration_sec = duration ? atol(duration) : 0;
> > >       self->duration_sec = duration_sec ? duration_sec : 5UL;
> > >
> > > @@ -540,7 +618,16 @@ FIXTURE_SETUP(proc_maps_race)
> > >               exit(0);
> > >       }
> > >
> > > -     sprintf(fname, "/proc/%d/maps", self->pid);
> > > +     switch (self->maps_file) {
> > > +     case MAPS:
> > > +             sprintf(fname, "/proc/%d/maps", self->pid);
> > > +             break;
> > > +     case SMAPS:
> > > +             sprintf(fname, "/proc/%d/smaps", self->pid);
> > > +             break;
> > > +     default:
> > > +             ksft_exit_fail();
> > > +     }
> > >       self->maps_fd = open(fname, O_RDONLY);
> > >       ASSERT_NE(self->maps_fd, -1);
> > >
> > > @@ -675,20 +762,20 @@ TEST_F(proc_maps_race, test_maps_tearing_from_split)
> > >               last_line_changed = strcmp(new_last_line.text, self->last_line.text) != 0;
> > >               first_line_changed = strcmp(new_first_line.text, self->first_line.text) != 0;
> > >               ASSERT_EQ(last_line_changed, first_line_changed);
> > > -
> > > -             /* Check if PROCMAP_QUERY ioclt() finds the right VMA */
> > > -             ASSERT_TRUE(query_addr_at(self->maps_fd, mod_info->addr + self->page_size,
> > > -                                       &vma_start, &vma_end));
> > > -             /*
> > > -              * The vma at the split address can be either the same as
> > > -              * original one (if read before the split) or the same as the
> > > -              * first line in the second page (if read after the split).
> > > -              */
> > > -             ASSERT_TRUE((vma_start == self->last_line.start_addr &&
> > > -                          vma_end == self->last_line.end_addr) ||
> > > -                         (vma_start == split_first_line.start_addr &&
> > > -                          vma_end == split_first_line.end_addr));
> > > -
> > > +             if (self->maps_file == MAPS) {
> > > +                     /* Check if PROCMAP_QUERY ioclt() finds the right VMA */
> > > +                     ASSERT_TRUE(query_addr_at(self->maps_fd, mod_info->addr + self->page_size,
> > > +                                               &vma_start, &vma_end));
> > > +                     /*
> > > +                      * The vma at the split address can be either the same as
> > > +                      * original one (if read before the split) or the same as the
> > > +                      * first line in the second page (if read after the split).
> > > +                      */
> > > +                     ASSERT_TRUE((vma_start == self->last_line.start_addr &&
> > > +                                  vma_end == self->last_line.end_addr) ||
> > > +                                 (vma_start == split_first_line.start_addr &&
> > > +                                  vma_end == split_first_line.end_addr));
> > > +             }
> > >               clock_gettime(CLOCK_MONOTONIC_COARSE, &end_ts);
> > >               end_test_iteration(&end_ts, self->verbose);
> > >       } while (end_ts.tv_sec - start_ts.tv_sec < self->duration_sec);
> > > @@ -758,17 +845,18 @@ TEST_F(proc_maps_race, test_maps_tearing_from_resize)
> > >                                       strcmp(new_first_line.text, restored_first_line.text),
> > >                                       "Expand result invalid", self));
> > >               }
> > > -
> > > -             /* Check if PROCMAP_QUERY ioclt() finds the right VMA */
> > > -             ASSERT_TRUE(query_addr_at(self->maps_fd, mod_info->addr, &vma_start, &vma_end));
> > > -             /*
> > > -              * The vma should stay at the same address and have either the
> > > -              * original size of 3 pages or 1 page if read after shrinking.
> > > -              */
> > > -             ASSERT_TRUE(vma_start == self->last_line.start_addr &&
> > > -                         (vma_end - vma_start == self->page_size * 3 ||
> > > -                          vma_end - vma_start == self->page_size));
> > > -
> > > +             if (self->maps_file == MAPS) {
> > > +                     /* Check if PROCMAP_QUERY ioclt() finds the right VMA */
> > > +                     ASSERT_TRUE(query_addr_at(self->maps_fd, mod_info->addr,
> > > +                                               &vma_start, &vma_end));
> > > +                     /*
> > > +                      * The vma should stay at the same address and have either the
> > > +                      * original size of 3 pages or 1 page if read after shrinking.
> > > +                      */
> > > +                     ASSERT_TRUE(vma_start == self->last_line.start_addr &&
> > > +                                 (vma_end - vma_start == self->page_size * 3 ||
> > > +                                  vma_end - vma_start == self->page_size));
> > > +             }
> > >               clock_gettime(CLOCK_MONOTONIC_COARSE, &end_ts);
> > >               end_test_iteration(&end_ts, self->verbose);
> > >       } while (end_ts.tv_sec - start_ts.tv_sec < self->duration_sec);
> > > @@ -838,20 +926,20 @@ TEST_F(proc_maps_race, test_maps_tearing_from_remap)
> > >                                       strcmp(new_first_line.text, restored_first_line.text),
> > >                                       "Remap restore result invalid", self));
> > >               }
> > > -
> > > -             /* Check if PROCMAP_QUERY ioclt() finds the right VMA */
> > > -             ASSERT_TRUE(query_addr_at(self->maps_fd, mod_info->addr + self->page_size,
> > > -                                       &vma_start, &vma_end));
> > > -             /*
> > > -              * The vma should either stay at the same address and have the
> > > -              * original size of 3 pages or we should find the remapped vma
> > > -              * at the remap destination address with size of 1 page.
> > > -              */
> > > -             ASSERT_TRUE((vma_start == self->last_line.start_addr &&
> > > -                          vma_end - vma_start == self->page_size * 3) ||
> > > -                         (vma_start == self->last_line.start_addr + self->page_size &&
> > > -                          vma_end - vma_start == self->page_size));
> > > -
> > > +             if (self->maps_file == MAPS) {
> > > +                     /* Check if PROCMAP_QUERY ioclt() finds the right VMA */
> > > +                     ASSERT_TRUE(query_addr_at(self->maps_fd, mod_info->addr + self->page_size,
> > > +                                               &vma_start, &vma_end));
> > > +                     /*
> > > +                      * The vma should either stay at the same address and have the
> > > +                      * original size of 3 pages or we should find the remapped vma
> > > +                      * at the remap destination address with size of 1 page.
> > > +                      */
> > > +                     ASSERT_TRUE((vma_start == self->last_line.start_addr &&
> > > +                                  vma_end - vma_start == self->page_size * 3) ||
> > > +                                 (vma_start == self->last_line.start_addr + self->page_size &&
> > > +                                  vma_end - vma_start == self->page_size));
> > > +             }
> > >               clock_gettime(CLOCK_MONOTONIC_COARSE, &end_ts);
> > >               end_test_iteration(&end_ts, self->verbose);
> > >       } while (end_ts.tv_sec - start_ts.tv_sec < self->duration_sec);
> > > --
> > > 2.54.0.545.g6539524ca2-goog
> > >