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@xxxxxxxxxx>

One comment question, otherwise..

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

> ---
>  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
>