Re: + selftests-proc-add-proc-pid-statm-output-validation.patch added to mm-nonmm-unstable branch

Re: + selftests-proc-add-proc-pid-statm-output-validation.patch added to mm-nonmm-unstable branch

[Alexey Dobriyan]

On Sun, Oct 01, 2023 at 12:37:40PM -0700, Andrew Morton wrote:
>      selftests-proc-add-proc-pid-statm-output-validation.patch

> Add /proc/${pid}/statm validation
> 
> /proc/$(pid)/statm output is expected to be:
>  "0 0 0 * 0 0 0\n"
> Here * can be any value
> 
> Read output of /proc/$(pid)/statm
> and compare length of output is
> equal or greater than expected output

> --- a/tools/testing/selftests/proc/proc-empty-vm.c~selftests-proc-add-proc-pid-statm-output-validation
> +++ a/tools/testing/selftests/proc/proc-empty-vm.c
> @@ -303,6 +303,37 @@ static int test_proc_pid_smaps_rollup(pi
>  	}
>  }
>  
> +static const char g_statm[] = "0 0 0 * 0 0 0\n";

This is both unreliable and incorrect.

4th value is "end_code - start_code" when exec is done which could be
anything not 1-digit number (although unlikely).

Testing for strlen is simply too weak of a test.

> +static int test_proc_pid_statm(pid_t pid)
> +{
> +	char buf[4096];
> +
> +	snprintf(buf, sizeof(buf), "/proc/%u/statm", pid);
> +
> +	int fd = open(buf, O_RDONLY);
> +
> +	if (fd == -1) {
> +		if (errno == ENOENT) {
> +			/*
> +			 * /proc/${pid}/statm is under CONFIG_PROC_PAGE_MONITOR,
> +			 * it doesn't necessarily exist.
> +			 */
> +			return EXIT_SUCCESS;
> +		}
> +		perror("open /proc/${pid}/statm");
> +		return EXIT_FAILURE;
> +	} else {
> +		ssize_t rv = read(fd, buf, sizeof(buf));
> +
> +		close(fd);
> +		size_t len = strlen(g_statm);
> +
> +		assert(rv >= len);
> +		return EXIT_SUCCESS;
> +	}
> +}
> +
>  int main(void)
>  {
>  	int rv = EXIT_SUCCESS;
> @@ -389,11 +420,8 @@ int main(void)
>  		if (rv == EXIT_SUCCESS) {
>  			rv = test_proc_pid_smaps_rollup(pid);
>  		}
> -		/*
> -		 * TODO test /proc/${pid}/statm, task_statm()
> -		 * ->start_code, ->end_code aren't updated by munmap().
> -		 * Output can be "0 0 0 2 0 0 0\n" where "2" can be anything.
> -		 */
> +		if (rv == EXIT_SUCCESS)
> +			rv = test_proc_pid_statm(pid);
>  
>  		/* Cut the rope. */

Re: + selftests-proc-add-proc-pid-statm-output-validation.patch added to mm-nonmm-unstable branch

[swarup]

On Mon, Oct 02, 2023 at 03:38:25PM +0300, Alexey Dobriyan wrote:
> On Sun, Oct 01, 2023 at 12:37:40PM -0700, Andrew Morton wrote:
> >      selftests-proc-add-proc-pid-statm-output-validation.patch
> 
> > Add /proc/${pid}/statm validation
> > 
> > /proc/$(pid)/statm output is expected to be:
> >  "0 0 0 * 0 0 0\n"
> > Here * can be any value
> > 
> > Read output of /proc/$(pid)/statm
> > and compare length of output is
> > equal or greater than expected output
> 
> > --- a/tools/testing/selftests/proc/proc-empty-vm.c~selftests-proc-add-proc-pid-statm-output-validation
> > +++ a/tools/testing/selftests/proc/proc-empty-vm.c
> > @@ -303,6 +303,37 @@ static int test_proc_pid_smaps_rollup(pi
> >  	}
> >  }
> >  
> > +static const char g_statm[] = "0 0 0 * 0 0 0\n";
> 
> This is both unreliable and incorrect.
> 
> 4th value is "end_code - start_code" when exec is done which could be
> anything not 1-digit number (although unlikely).
> 
> Testing for strlen is simply too weak of a test.
> 
> > +static int test_proc_pid_statm(pid_t pid)
> > +{
> > +	char buf[4096];
> > +
> > +	snprintf(buf, sizeof(buf), "/proc/%u/statm", pid);
> > +
> > +	int fd = open(buf, O_RDONLY);
> > +
> > +	if (fd == -1) {
> > +		if (errno == ENOENT) {
> > +			/*
> > +			 * /proc/${pid}/statm is under CONFIG_PROC_PAGE_MONITOR,
> > +			 * it doesn't necessarily exist.
> > +			 */
> > +			return EXIT_SUCCESS;
> > +		}
> > +		perror("open /proc/${pid}/statm");
> > +		return EXIT_FAILURE;
> > +	} else {
> > +		ssize_t rv = read(fd, buf, sizeof(buf));
> > +
> > +		close(fd);
> > +		size_t len = strlen(g_statm);
> > +
> > +		assert(rv >= len);
> > +		return EXIT_SUCCESS;
> > +	}
> > +}
> > +
> >  int main(void)
> >  {
> >  	int rv = EXIT_SUCCESS;
> > @@ -389,11 +420,8 @@ int main(void)
> >  		if (rv == EXIT_SUCCESS) {
> >  			rv = test_proc_pid_smaps_rollup(pid);
> >  		}
> > -		/*
> > -		 * TODO test /proc/${pid}/statm, task_statm()
> > -		 * ->start_code, ->end_code aren't updated by munmap().
> > -		 * Output can be "0 0 0 2 0 0 0\n" where "2" can be anything.
> > -		 */
> > +		if (rv == EXIT_SUCCESS)
> > +			rv = test_proc_pid_statm(pid);
> >  
> >  		/* Cut the rope. */

Hi Alexey,
Thanks for reviewing the changes.

I assume below output of /proc/${procid}/statm
can be assumed as mentioned below:

static const char g_statm[] = "0 0 0 * 0 0 0\n"

If 0 is correct at their places, only issue is *,
whose value will be single digit or could change?

If this assumption is correct, i can change the
validation to handle 4th postion, and remaining
place will validate if it has zero or not,
and will send another patch?

Thanks,
Swarup

[PATCH v2] selftests:proc Add /proc/$(pid)/statm output validation

[Swarup Laxman Kotiaklapudi]

Add /proc/${pid}/statm validation

/proc/$(pid)/statm output is expected to be:
 "0 0 0 * 0 0 0\n"
Here * can be any value

Read output of /proc/$(pid)/statm
and check except for 4th position,
all other positions have value zero.

Signed-off-by: Swarup Laxman Kotiaklapudi <swarupkotikalapudi@gmail.com>
---
Changes in V2:
- 4th field if zero it will assert
- field other than 4th field is checked for zero value
- clean up of function test_proc_pid_statm()

Changes in V1:
- added new function test_proc_pid_statm()

 tools/testing/selftests/proc/proc-empty-vm.c | 57 ++++++++++++++++++--
 1 file changed, 52 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/proc/proc-empty-vm.c b/tools/testing/selftests/proc/proc-empty-vm.c
index b16c13688b88..1b5b48445e0f 100644
--- a/tools/testing/selftests/proc/proc-empty-vm.c
+++ b/tools/testing/selftests/proc/proc-empty-vm.c
@@ -302,6 +302,56 @@ static int test_proc_pid_smaps_rollup(pid_t pid)
 	}
 }
 
+static int test_proc_pid_statm(pid_t pid)
+{
+	char buf[4096];
+	char *tok;
+	char *string;
+	int non_zero_value_indx = 4;
+	int i = 1;
+
+	snprintf(buf, sizeof(buf), "/proc/%u/statm", pid);
+
+	/*
+	 *  Output can be "0 0 0 2 0 0 0\n" where "2" can be anything.
+	 */
+	int fd = open(buf, O_RDONLY);
+
+	if (fd == -1) {
+		if (errno == ENOENT) {
+			/*
+			 * /proc/${pid}/statm is under CONFIG_PROC_PAGE_MONITOR,
+			 * it doesn't necessarily exist.
+			 */
+			return EXIT_SUCCESS;
+		}
+		perror("open /proc/${pid}/statm");
+		return EXIT_FAILURE;
+	} else {
+		ssize_t rv = read(fd, buf, sizeof(buf));
+
+		close(fd);
+		assert(rv);
+		string = buf;
+
+		while ((tok = strsep(&string, " ")) != NULL) {
+			if (i == non_zero_value_indx) {
+				if (!strncmp(tok, "0", 1))
+					goto err_statm;
+			} else {
+				if (strncmp(tok, "0", 1))
+					goto err_statm;
+			}
+			i++;
+		}
+	}
+
+	return EXIT_SUCCESS;
+
+err_statm:
+	assert(0);
+}
+
 int main(void)
 {
 	int rv = EXIT_SUCCESS;
@@ -388,11 +438,8 @@ int main(void)
 		if (rv == EXIT_SUCCESS) {
 			rv = test_proc_pid_smaps_rollup(pid);
 		}
-		/*
-		 * TODO test /proc/${pid}/statm, task_statm()
-		 * ->start_code, ->end_code aren't updated by munmap().
-		 * Output can be "0 0 0 2 0 0 0\n" where "2" can be anything.
-		 */
+		if (rv == EXIT_SUCCESS)
+			rv = test_proc_pid_statm(pid);
 
 		/* Cut the rope. */
 		int wstatus;
-- 
2.34.1

Re: + selftests-proc-add-proc-pid-statm-output-validation.patch added to mm-nonmm-unstable branch

[Alexey Dobriyan]

On Wed, Oct 04, 2023 at 01:17:00PM -0700, Andrew Morton wrote:
> 
> The patch titled
>      Subject: selftests: proc: add /proc/$(pid)/statm output validation
> has been added to the -mm mm-nonmm-unstable branch.  Its filename is
>      selftests-proc-add-proc-pid-statm-output-validation.patch
> 
> This patch will shortly appear at
>      https://git.kernel.org/pub/scm/linux/kernel/git/akpm/25-new.git/tree/patches/selftests-proc-add-proc-pid-statm-output-validation.patch
> 
> This patch will later appear in the mm-nonmm-unstable branch at
>     git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
> 
> Before you just go and hit "reply", please:
>    a) Consider who else should be cc'ed
>    b) Prefer to cc a suitable mailing list as well
>    c) Ideally: find the original patch on the mailing list and do a
>       reply-to-all to that, adding suitable additional cc's
> 
> *** Remember to use Documentation/process/submit-checklist.rst when testing your code ***
> 
> The -mm tree is included into linux-next via the mm-everything
> branch at git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
> and is updated there every 2-3 working days
> 
> ------------------------------------------------------
> From: Swarup Laxman Kotiaklapudi <swarupkotikalapudi@gmail.com>
> Subject: selftests: proc: add /proc/$(pid)/statm output validation
> Date: Wed, 4 Oct 2023 01:13:19 +0530
> 
> Add /proc/${pid}/statm validation
> 
> /proc/$(pid)/statm output is expected to be:
>  "0 0 0 * 0 0 0\n"
> Here * can be any value
> 
> Read output of /proc/$(pid)/statm and check except for 4th position, all
> other positions have value zero.
> 
> Link: https://lkml.kernel.org/r/20231003194319.602646-1-swarupkotikalapudi@gmail.com
> Signed-off-by: Swarup Laxman Kotiaklapudi <swarupkotikalapudi@gmail.com>
> Cc: Alexey Dobriyan <adobriyan@gmail.com>
> Cc: Hugh Dickins <hughd@google.com>
> Cc: Shuah Khan <shuah@kernel.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  tools/testing/selftests/proc/proc-empty-vm.c |   57 +++++++++++++++--
>  1 file changed, 52 insertions(+), 5 deletions(-)
> 
> --- a/tools/testing/selftests/proc/proc-empty-vm.c~selftests-proc-add-proc-pid-statm-output-validation
> +++ a/tools/testing/selftests/proc/proc-empty-vm.c
> @@ -303,6 +303,56 @@ static int test_proc_pid_smaps_rollup(pi
>  	}
>  }
>  
> +static int test_proc_pid_statm(pid_t pid)
> +{
> +	char buf[4096];
> +	char *tok;
> +	char *string;
> +	int non_zero_value_indx = 4;
> +	int i = 1;
> +
> +	snprintf(buf, sizeof(buf), "/proc/%u/statm", pid);
> +
> +	/*
> +	 *  Output can be "0 0 0 2 0 0 0\n" where "2" can be anything.
> +	 */
> +	int fd = open(buf, O_RDONLY);
> +
> +	if (fd == -1) {
> +		if (errno == ENOENT) {
> +			/*
> +			 * /proc/${pid}/statm is under CONFIG_PROC_PAGE_MONITOR,
> +			 * it doesn't necessarily exist.
> +			 */
> +			return EXIT_SUCCESS;
> +		}
> +		perror("open /proc/${pid}/statm");
> +		return EXIT_FAILURE;
> +	} else {
> +		ssize_t rv = read(fd, buf, sizeof(buf));
> +
> +		close(fd);
> +		assert(rv);
> +		string = buf;
> +
> +		while ((tok = strsep(&string, " ")) != NULL) {

This is unreliable too. read() doesn't terminate the buffer so this relies
on termination from

	snprintf(buf, sizeof(buf), "/proc/%u/statm", pid);

Buggy kernel could return a lot of data and overwrite it.

Re: + selftests-proc-add-proc-pid-statm-output-validation.patch added to mm-nonmm-unstable branch

[Alexey Dobriyan]

On Mon, Oct 09, 2023 at 09:14:53AM +0300, Alexey Dobriyan wrote:
> On Wed, Oct 04, 2023 at 01:17:00PM -0700, Andrew Morton wrote:

> > +		if (errno == ENOENT) {
> > +			/*
> > +			 * /proc/${pid}/statm is under CONFIG_PROC_PAGE_MONITOR,
> > +			 * it doesn't necessarily exist.

Oh, and /proc/*/statm is _not_ under CONFIG_PROC_PAGE_MONITOR,
it always exists.

Re: + selftests-proc-add-proc-pid-statm-output-validation.patch added to mm-nonmm-unstable branch

[swarup]

esOn Mon, Oct 09, 2023 at 12:00:04PM +0300, Alexey Dobriyan wrote:
> On Mon, Oct 09, 2023 at 09:14:53AM +0300, Alexey Dobriyan wrote:
> > On Wed, Oct 04, 2023 at 01:17:00PM -0700, Andrew Morton wrote:
> 
> > > +		if (errno == ENOENT) {
> > > +			/*
> > > +			 * /proc/${pid}/statm is under CONFIG_PROC_PAGE_MONITOR,
> > > +			 * it doesn't necessarily exist.
> 
> Oh, and /proc/*/statm is _not_ under CONFIG_PROC_PAGE_MONITOR,
> it always exists.

Hi Alexey Dobriyan,
It is my mistake, i checked the code, yes it always exists.
Thanks,
Swarup

Re: + selftests-proc-add-proc-pid-statm-output-validation.patch added to mm-nonmm-unstable branch

[swarup]

eOn Mon, Oct 09, 2023 at 09:14:53AM +0300, Alexey Dobriyan wrote:
> On Wed, Oct 04, 2023 at 01:17:00PM -0700, Andrew Morton wrote:
> > 
> > The patch titled
> >      Subject: selftests: proc: add /proc/$(pid)/statm output validation
> > has been added to the -mm mm-nonmm-unstable branch.  Its filename is
> >      selftests-proc-add-proc-pid-statm-output-validation.patch
> > 
> > This patch will shortly appear at
> >      https://git.kernel.org/pub/scm/linux/kernel/git/akpm/25-new.git/tree/patches/selftests-proc-add-proc-pid-statm-output-validation.patch
> > 
> > This patch will later appear in the mm-nonmm-unstable branch at
> >     git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
> > 
> > Before you just go and hit "reply", please:
> >    a) Consider who else should be cc'ed
> >    b) Prefer to cc a suitable mailing list as well
> >    c) Ideally: find the original patch on the mailing list and do a
> >       reply-to-all to that, adding suitable additional cc's
> > 
> > *** Remember to use Documentation/process/submit-checklist.rst when testing your code ***
> > 
> > The -mm tree is included into linux-next via the mm-everything
> > branch at git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
> > and is updated there every 2-3 working days
> > 
> > ------------------------------------------------------
> > From: Swarup Laxman Kotiaklapudi <swarupkotikalapudi@gmail.com>
> > Subject: selftests: proc: add /proc/$(pid)/statm output validation
> > Date: Wed, 4 Oct 2023 01:13:19 +0530
> > 
> > Add /proc/${pid}/statm validation
> > 
> > /proc/$(pid)/statm output is expected to be:
> >  "0 0 0 * 0 0 0\n"
> > Here * can be any value
> > 
> > Read output of /proc/$(pid)/statm and check except for 4th position, all
> > other positions have value zero.
> > 
> > Link: https://lkml.kernel.org/r/20231003194319.602646-1-swarupkotikalapudi@gmail.com
> > Signed-off-by: Swarup Laxman Kotiaklapudi <swarupkotikalapudi@gmail.com>
> > Cc: Alexey Dobriyan <adobriyan@gmail.com>
> > Cc: Hugh Dickins <hughd@google.com>
> > Cc: Shuah Khan <shuah@kernel.org>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > ---
> > 
> >  tools/testing/selftests/proc/proc-empty-vm.c |   57 +++++++++++++++--
> >  1 file changed, 52 insertions(+), 5 deletions(-)
> > 
> > --- a/tools/testing/selftests/proc/proc-empty-vm.c~selftests-proc-add-proc-pid-statm-output-validation
> > +++ a/tools/testing/selftests/proc/proc-empty-vm.c
> > @@ -303,6 +303,56 @@ static int test_proc_pid_smaps_rollup(pi
> >  	}
> >  }
> >  
> > +static int test_proc_pid_statm(pid_t pid)
> > +{
> > +	char buf[4096];
> > +	char *tok;
> > +	char *string;
> > +	int non_zero_value_indx = 4;
> > +	int i = 1;
> > +
> > +	snprintf(buf, sizeof(buf), "/proc/%u/statm", pid);
> > +
> > +	/*
> > +	 *  Output can be "0 0 0 2 0 0 0\n" where "2" can be anything.
> > +	 */
> > +	int fd = open(buf, O_RDONLY);
> > +
> > +	if (fd == -1) {
> > +		if (errno == ENOENT) {
> > +			/*
> > +			 * /proc/${pid}/statm is under CONFIG_PROC_PAGE_MONITOR,
> > +			 * it doesn't necessarily exist.
> > +			 */
> > +			return EXIT_SUCCESS;
> > +		}
> > +		perror("open /proc/${pid}/statm");
> > +		return EXIT_FAILURE;
> > +	} else {
> > +		ssize_t rv = read(fd, buf, sizeof(buf));
> > +
> > +		close(fd);
> > +		assert(rv);
> > +		string = buf;
> > +
> > +		while ((tok = strsep(&string, " ")) != NULL) {
> 
> This is unreliable too. read() doesn't terminate the buffer so this relies
> on termination from
> 
> 	snprintf(buf, sizeof(buf), "/proc/%u/statm", pid);
> 
> Buggy kernel could return a lot of data and overwrite it.

Hi Alexey Dobriyan,
I will try to correct read() function.
Thanks,
Swarup