[PATCH 1/2] selftests/powerpc: Allocate base registers

Some ptrace selftests are passing input operands using a constraint that
can allocate any register for the operand, and using these registers on
load/store operations.

If the register allocated by the compiler happens to be zero (r0), it might
cause an invalid memory address access, since load and store operations
consider the content of 0x0 address if the base register is r0, instead
of the content of the r0 register. For example:

	r1 := 0xdeadbeef
	r0 := 0xdeadbeef
	
	ld r2, 0(r1) /* will load into r2 the content of r1 address */
	ld r2, 0(r0) /* will load into r2 the context of 0x0 */

In order to avoid this possible problem, the inline assembly constraint
should be aware that these registers will be used as a base register, thus,
r0 should not be alocated.

Other than that, this patch removes inline assembly operands that are not used
by the tests.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 tools/testing/selftests/powerpc/ptrace/ptrace-gpr.c        | 2 +-
 tools/testing/selftests/powerpc/ptrace/ptrace-tm-gpr.c     | 4 ++--
 tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-tar.c | 2 +-
 tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-vsx.c | 3 +--
 tools/testing/selftests/powerpc/ptrace/ptrace-tm-spr.c     | 2 +-
 tools/testing/selftests/powerpc/ptrace/ptrace-tm-tar.c     | 2 +-
 tools/testing/selftests/powerpc/ptrace/ptrace-tm-vsx.c     | 3 +--
 7 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/tools/testing/selftests/powerpc/ptrace/ptrace-gpr.c b/tools/testing/selftests/powerpc/ptrace/ptrace-gpr.c
index 0b4ebcc2f485..ca29fafeed5d 100644
--- a/tools/testing/selftests/powerpc/ptrace/ptrace-gpr.c
+++ b/tools/testing/selftests/powerpc/ptrace/ptrace-gpr.c

@@ -31,7 +31,7 @@ void gpr(void)
 		ASM_LOAD_GPR_IMMED(gpr_1)
 		ASM_LOAD_FPR_SINGLE_PRECISION(flt_1)
 		:
-		: [gpr_1]"i"(GPR_1), [flt_1] "r" (&a)
+		: [gpr_1]"i"(GPR_1), [flt_1] "b" (&a)
 		: "memory", "r6", "r7", "r8", "r9", "r10",
 		"r11", "r12", "r13", "r14", "r15", "r16", "r17",
 		"r18", "r19", "r20", "r21", "r22", "r23", "r24",

diff --git a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-gpr.c b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-gpr.c
index 59206b96e98a..a08a91594dbe 100644
--- a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-gpr.c
+++ b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-gpr.c

@@ -59,8 +59,8 @@ void tm_gpr(void)
 		"3: ;"
 		: [res] "=r" (result), [texasr] "=r" (texasr)
 		: [gpr_1]"i"(GPR_1), [gpr_2]"i"(GPR_2),
-		[sprn_texasr] "i" (SPRN_TEXASR), [flt_1] "r" (&a),
-		[flt_2] "r" (&b), [cptr1] "r" (&cptr[1])
+		[sprn_texasr] "i" (SPRN_TEXASR), [flt_1] "b" (&a),
+		[flt_2] "b" (&b), [cptr1] "b" (&cptr[1])
 		: "memory", "r7", "r8", "r9", "r10",
 		"r11", "r12", "r13", "r14", "r15", "r16",
 		"r17", "r18", "r19", "r20", "r21", "r22",

diff --git a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-tar.c b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-tar.c
index b3c061dc9512..f47174746231 100644
--- a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-tar.c
+++ b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-tar.c

@@ -72,7 +72,7 @@ void tm_spd_tar(void)
 		"3: ;"
 
 		: [res] "=r" (result), [texasr] "=r" (texasr)
-		: [val] "r" (cptr[1]), [sprn_dscr]"i"(SPRN_DSCR),
+		: [sprn_dscr]"i"(SPRN_DSCR),
 		[sprn_tar]"i"(SPRN_TAR), [sprn_ppr]"i"(SPRN_PPR),
 		[sprn_texasr]"i"(SPRN_TEXASR), [tar_1]"i"(TAR_1),
 		[dscr_1]"i"(DSCR_1), [tar_2]"i"(TAR_2), [dscr_2]"i"(DSCR_2),

diff --git a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-vsx.c b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-vsx.c
index 277dade1b382..18a685bf6a09 100644
--- a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-vsx.c
+++ b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-vsx.c

@@ -77,8 +77,7 @@ void tm_spd_vsx(void)
 
 		"3: ;"
 		: [res] "=r" (result), [texasr] "=r" (texasr)
-		: [fp_load] "r" (fp_load), [fp_load_ckpt] "r" (fp_load_ckpt),
-		[sprn_texasr] "i"  (SPRN_TEXASR)
+		: [sprn_texasr] "i"  (SPRN_TEXASR)
 		: "memory", "r0", "r1", "r3", "r4",
 		"r7", "r8", "r9", "r10", "r11"
 		);

diff --git a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spr.c b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spr.c
index 51427a2465f6..ba04999254e3 100644
--- a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spr.c
+++ b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spr.c

@@ -74,7 +74,7 @@ void tm_spr(void)
 
 		"3: ;"
 		: [tfhar] "=r" (tfhar), [res] "=r" (result),
-		[texasr] "=r" (texasr), [cptr1] "=r" (cptr1)
+		[texasr] "=r" (texasr), [cptr1] "=b" (cptr1)
 		: [sprn_texasr] "i"  (SPRN_TEXASR)
 		: "memory", "r0", "r8", "r31"
 		);

diff --git a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-tar.c b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-tar.c
index 48b462f75023..f70023b25e6e 100644
--- a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-tar.c
+++ b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-tar.c

@@ -65,7 +65,7 @@ void tm_tar(void)
 		: [sprn_dscr]"i"(SPRN_DSCR), [sprn_tar]"i"(SPRN_TAR),
 		[sprn_ppr]"i"(SPRN_PPR), [sprn_texasr]"i"(SPRN_TEXASR),
 		[tar_1]"i"(TAR_1), [dscr_1]"i"(DSCR_1), [tar_2]"i"(TAR_2),
-		[dscr_2]"i"(DSCR_2), [cptr1] "r" (&cptr[1])
+		[dscr_2]"i"(DSCR_2), [cptr1] "b" (&cptr[1])
 		: "memory", "r0", "r1", "r3", "r4", "r5", "r6"
 		);
 

diff --git a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-vsx.c b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-vsx.c
index 17c23cabac3e..dfba80058977 100644
--- a/tools/testing/selftests/powerpc/ptrace/ptrace-tm-vsx.c
+++ b/tools/testing/selftests/powerpc/ptrace/ptrace-tm-vsx.c

@@ -65,8 +65,7 @@ void tm_vsx(void)
 
 		"3: ;"
 		: [res] "=r" (result), [texasr] "=r" (texasr)
-		: [fp_load] "r" (fp_load), [fp_load_ckpt] "r" (fp_load_ckpt),
-		[sprn_texasr] "i"  (SPRN_TEXASR), [cptr1] "r" (&cptr[1])
+		: [sprn_texasr] "i"  (SPRN_TEXASR), [cptr1] "b" (&cptr[1])
 		: "memory", "r0", "r1", "r3", "r4",
 		"r7", "r8", "r9", "r10", "r11"
 		);

-- 
2.19.0

Current core-pkey selftest fails if the test runs without privileges to
write into the core pattern file (/proc/sys/kernel/core_pattern). This
causes the test to fail and give the impression that the subsystem being
tested is broken, when, in fact, the test is being executed without the
proper privileges. This is the current error:

	test: core_pkey
	tags: git_version:v4.19-3-g9e3363be9bce-dirty
	Error writing to core_pattern file: Permission denied
	failure: core_pkey

This patch simply skips this test if it runs without the proper privileges,
avoiding this undesired failure.

CC: Thiago Jung Bauermann <redacted>
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 tools/testing/selftests/powerpc/ptrace/core-pkey.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/powerpc/ptrace/core-pkey.c b/tools/testing/selftests/powerpc/ptrace/core-pkey.c
index e23e2e199eb4..e07949120fc8 100644
--- a/tools/testing/selftests/powerpc/ptrace/core-pkey.c
+++ b/tools/testing/selftests/powerpc/ptrace/core-pkey.c

@@ -352,17 +352,11 @@ static int write_core_pattern(const char *core_pattern)
 	FILE *f;
 
 	f = fopen(core_pattern_file, "w");
-	if (!f) {
-		perror("Error writing to core_pattern file");
-		return TEST_FAIL;
-	}
+	SKIP_IF(!f);
 
 	ret = fwrite(core_pattern, 1, len, f);
 	fclose(f);
-	if (ret != len) {
-		perror("Error writing to core_pattern file");
-		return TEST_FAIL;
-	}
+	SKIP_IF(ret != len);
 
 	return TEST_PASS;
 }

-- 
2.19.0

On 10/23/2018 01:23 PM, Breno Leitao wrote:

If we don't have proper privileges we should fail on the open, right? So wouldn't we still want to fail on the write if something goes wrong?

-Tyrel

 	return TEST_PASS;
 }

On Tue, Oct 23, 2018 at 05:23:16PM -0300, Breno Leitao wrote:

Some ptrace selftests are passing input operands using a constraint that
can allocate any register for the operand, and using these registers on
load/store operations.

If the register allocated by the compiler happens to be zero (r0), it might
cause an invalid memory address access, since load and store operations
consider the content of 0x0 address if the base register is r0, instead
of the content of the r0 register. For example:

	r1 := 0xdeadbeef
	r0 := 0xdeadbeef
	
	ld r2, 0(r1) /* will load into r2 the content of r1 address */
	ld r2, 0(r0) /* will load into r2 the context of 0x0 */

That isn't valid syntax: you have to write
	ld r2, 0(0)

(s/context/content/ btw)

In order to avoid this possible problem, the inline assembly constraint
should be aware that these registers will be used as a base register, thus,
r0 should not be alocated.

(allocated)

The patch looks fine :-)

Reviewed-by: Segher Boessenkool <redacted>


Segher

Hi Tyrel,

On 10/23/2018 05:41 PM, Tyrel Datwyler wrote:

quoted

--- a/tools/testing/selftests/powerpc/ptrace/core-pkey.c
+++ b/tools/testing/selftests/powerpc/ptrace/core-pkey.c

@@ -352,17 +352,11 @@ static int write_core_pattern(const char *core_pattern)
 	FILE *f;

 	f = fopen(core_pattern_file, "w");
-	if (!f) {
-		perror("Error writing to core_pattern file");
-		return TEST_FAIL;
-	}
+	SKIP_IF(!f);

 	ret = fwrite(core_pattern, 1, len, f);
 	fclose(f);
-	if (ret != len) {
-		perror("Error writing to core_pattern file");
-		return TEST_FAIL;
-	}
+	SKIP_IF(ret != len);

If we don't have proper privileges we should fail on the open, right?
So wouldn't we still want to fail on the write if something goes
wrong?

That is a good point. Should the test fail or skip if it is not possible
to create the infrastructure to run the core test?

Trying to find the answer in the current test sets, I find tests where
the self test skips if the test environment is not able to be set up, as
for example, when a memory allocation fails.

File: tools/testing/selftests/powerpc/alignment/alignment_handler.c

        ci1 = mmap(NULL, bufsize, PROT_WRITE, MAP_SHARED,
                   fd, bufsize);
        if ((ci0 == MAP_FAILED) || (ci1 == MAP_FAILED)) {
                printf("\n");
                perror("mmap failed");
                SKIP_IF(1);
        }

Breno Leitao [off-list ref] writes:

Hi Tyrel,

On 10/23/2018 05:41 PM, Tyrel Datwyler wrote:

quoted

quoted

--- a/tools/testing/selftests/powerpc/ptrace/core-pkey.c
+++ b/tools/testing/selftests/powerpc/ptrace/core-pkey.c

@@ -352,17 +352,11 @@ static int write_core_pattern(const char *core_pattern)
 	FILE *f;

 	f = fopen(core_pattern_file, "w");
-	if (!f) {
-		perror("Error writing to core_pattern file");
-		return TEST_FAIL;
-	}
+	SKIP_IF(!f);

 	ret = fwrite(core_pattern, 1, len, f);
 	fclose(f);
-	if (ret != len) {
-		perror("Error writing to core_pattern file");
-		return TEST_FAIL;
-	}
+	SKIP_IF(ret != len);

quoted

If we don't have proper privileges we should fail on the open, right?
So wouldn't we still want to fail on the write if something goes
wrong?

That is a good point. Should the test fail or skip if it is not possible
to create the infrastructure to run the core test?

Trying to find the answer in the current test sets, I find tests where
the self test skips if the test environment is not able to be set up, as
for example, when a memory allocation fails.

File: tools/testing/selftests/powerpc/alignment/alignment_handler.c

        ci1 = mmap(NULL, bufsize, PROT_WRITE, MAP_SHARED,
                   fd, bufsize);
        if ((ci0 == MAP_FAILED) || (ci1 == MAP_FAILED)) {
                printf("\n");
                perror("mmap failed");
                SKIP_IF(1);
        }

I think TEST_FAIL means the test was able to exercise the feature
and found a problem with it. In this case, the test wasn't able to
exercise the feature so it's not appropriate.

Ideally, there should be a TEST_ERROR result for a case like this where
an unexpected problem prevented the testcase from exercising the
feature.

If we're to use the an existing result then I vote for SKIP_IF.

For reference, here are the test results that DejaGnu supports (it is
the test harness used by some GNU projects):

https://www.gnu.org/software/dejagnu/manual/Output-States.html

I would say that SKIP_IF corresponds to UNSUPPORTED in DejaGnu.

--
Thiago Jung Bauermann
IBM Linux Technology Center

Thiago Jung Bauermann [off-list ref] writes:

Breno Leitao [off-list ref] writes:

quoted

Hi Tyrel,

On 10/23/2018 05:41 PM, Tyrel Datwyler wrote:

quoted

quoted

--- a/tools/testing/selftests/powerpc/ptrace/core-pkey.c
+++ b/tools/testing/selftests/powerpc/ptrace/core-pkey.c

@@ -352,17 +352,11 @@ static int write_core_pattern(const char *core_pattern)
 	FILE *f;

 	f = fopen(core_pattern_file, "w");
-	if (!f) {
-		perror("Error writing to core_pattern file");
-		return TEST_FAIL;
-	}
+	SKIP_IF(!f);

 	ret = fwrite(core_pattern, 1, len, f);
 	fclose(f);
-	if (ret != len) {
-		perror("Error writing to core_pattern file");
-		return TEST_FAIL;
-	}
+	SKIP_IF(ret != len);

quoted

If we don't have proper privileges we should fail on the open, right?
So wouldn't we still want to fail on the write if something goes
wrong?

That is a good point. Should the test fail or skip if it is not possible
to create the infrastructure to run the core test?

Trying to find the answer in the current test sets, I find tests where
the self test skips if the test environment is not able to be set up, as
for example, when a memory allocation fails.

File: tools/testing/selftests/powerpc/alignment/alignment_handler.c

        ci1 = mmap(NULL, bufsize, PROT_WRITE, MAP_SHARED,
                   fd, bufsize);
        if ((ci0 == MAP_FAILED) || (ci1 == MAP_FAILED)) {
                printf("\n");
                perror("mmap failed");
                SKIP_IF(1);
        }

I think TEST_FAIL means the test was able to exercise the feature
and found a problem with it. In this case, the test wasn't able to
exercise the feature so it's not appropriate.

Ideally, there should be a TEST_ERROR result for a case like this where
an unexpected problem prevented the testcase from exercising the
feature.

If we're to use the an existing result then I vote for SKIP_IF.

Yeah I agree.

See for example some of the TM tests, which skip if TM is not available.
Or the alignment test which skips if it can't open /dev/fb0.

In this case it should print "you need to be root to run this" and then
skip.

cheers

On 10/30/2018 08:16 AM, Michael Ellerman wrote:

Thiago Jung Bauermann [off-list ref] writes:

quoted

Breno Leitao [off-list ref] writes:

quoted

Hi Tyrel,

On 10/23/2018 05:41 PM, Tyrel Datwyler wrote:

quoted

quoted

--- a/tools/testing/selftests/powerpc/ptrace/core-pkey.c
+++ b/tools/testing/selftests/powerpc/ptrace/core-pkey.c

@@ -352,17 +352,11 @@ static int write_core_pattern(const char *core_pattern)
 	FILE *f;

 	f = fopen(core_pattern_file, "w");
-	if (!f) {
-		perror("Error writing to core_pattern file");
-		return TEST_FAIL;
-	}
+	SKIP_IF(!f);

 	ret = fwrite(core_pattern, 1, len, f);
 	fclose(f);
-	if (ret != len) {
-		perror("Error writing to core_pattern file");
-		return TEST_FAIL;
-	}
+	SKIP_IF(ret != len);

quoted

If we don't have proper privileges we should fail on the open, right?
So wouldn't we still want to fail on the write if something goes
wrong?

That is a good point. Should the test fail or skip if it is not possible
to create the infrastructure to run the core test?

Trying to find the answer in the current test sets, I find tests where
the self test skips if the test environment is not able to be set up, as
for example, when a memory allocation fails.

File: tools/testing/selftests/powerpc/alignment/alignment_handler.c

        ci1 = mmap(NULL, bufsize, PROT_WRITE, MAP_SHARED,
                   fd, bufsize);
        if ((ci0 == MAP_FAILED) || (ci1 == MAP_FAILED)) {
                printf("\n");
                perror("mmap failed");
                SKIP_IF(1);
        }

I think TEST_FAIL means the test was able to exercise the feature
and found a problem with it. In this case, the test wasn't able to
exercise the feature so it's not appropriate.

Ideally, there should be a TEST_ERROR result for a case like this where
an unexpected problem prevented the testcase from exercising the
feature.

If we're to use the an existing result then I vote for SKIP_IF.

Yeah I agree.

See for example some of the TM tests, which skip if TM is not available.
Or the alignment test which skips if it can't open /dev/fb0.

In this case it should print "you need to be root to run this" and then
skip.

Agreed that there should be some indicator of why we are skipping the test. My original point I was trying to make was that I thought skipping a failed open was okay because we are likely not root. However, I wasn't sure that a failed write was okay to skip as that could be an indicator that something has actually been broken.

-Tyrel

cheers

Tyrel Datwyler [off-list ref] writes:

On 10/23/2018 01:23 PM, Breno Leitao wrote:

quoted

Current core-pkey selftest fails if the test runs without privileges to
write into the core pattern file (/proc/sys/kernel/core_pattern). This
causes the test to fail and give the impression that the subsystem being
tested is broken, when, in fact, the test is being executed without the
proper privileges. This is the current error:

	test: core_pkey
	tags: git_version:v4.19-3-g9e3363be9bce-dirty
	Error writing to core_pattern file: Permission denied
	failure: core_pkey

This patch simply skips this test if it runs without the proper privileges,
avoiding this undesired failure.

CC: Thiago Jung Bauermann <redacted>
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 tools/testing/selftests/powerpc/ptrace/core-pkey.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/powerpc/ptrace/core-pkey.c b/tools/testing/selftests/powerpc/ptrace/core-pkey.c
index e23e2e199eb4..e07949120fc8 100644
--- a/tools/testing/selftests/powerpc/ptrace/core-pkey.c
+++ b/tools/testing/selftests/powerpc/ptrace/core-pkey.c

@@ -352,17 +352,11 @@ static int write_core_pattern(const char *core_pattern)
 	FILE *f;

 	f = fopen(core_pattern_file, "w");
-	if (!f) {
-		perror("Error writing to core_pattern file");
-		return TEST_FAIL;
-	}
+	SKIP_IF(!f);

 	ret = fwrite(core_pattern, 1, len, f);
 	fclose(f);
-	if (ret != len) {
-		perror("Error writing to core_pattern file");
-		return TEST_FAIL;
-	}
+	SKIP_IF(ret != len);

If we don't have proper privileges we should fail on the open, right?
So wouldn't we still want to fail on the write if something goes
wrong?

Yes you're right. If we don't have permission then the open should have
failed, and we skip then.

But if the open succeeded and the write fails then we don't know what's
going on and the test should fail.

cheers

Tyrel Datwyler [off-list ref] writes:

On 10/30/2018 08:16 AM, Michael Ellerman wrote:

quoted

Thiago Jung Bauermann [off-list ref] writes:

quoted

Breno Leitao [off-list ref] writes:

quoted

Hi Tyrel,

On 10/23/2018 05:41 PM, Tyrel Datwyler wrote:

quoted

quoted

--- a/tools/testing/selftests/powerpc/ptrace/core-pkey.c
+++ b/tools/testing/selftests/powerpc/ptrace/core-pkey.c

@@ -352,17 +352,11 @@ static int write_core_pattern(const char *core_pattern)
 	FILE *f;

 	f = fopen(core_pattern_file, "w");
-	if (!f) {
-		perror("Error writing to core_pattern file");
-		return TEST_FAIL;
-	}
+	SKIP_IF(!f);

 	ret = fwrite(core_pattern, 1, len, f);
 	fclose(f);
-	if (ret != len) {
-		perror("Error writing to core_pattern file");
-		return TEST_FAIL;
-	}
+	SKIP_IF(ret != len);

quoted

If we don't have proper privileges we should fail on the open, right?
So wouldn't we still want to fail on the write if something goes
wrong?

That is a good point. Should the test fail or skip if it is not possible
to create the infrastructure to run the core test?

Trying to find the answer in the current test sets, I find tests where
the self test skips if the test environment is not able to be set up, as
for example, when a memory allocation fails.

File: tools/testing/selftests/powerpc/alignment/alignment_handler.c

        ci1 = mmap(NULL, bufsize, PROT_WRITE, MAP_SHARED,
                   fd, bufsize);
        if ((ci0 == MAP_FAILED) || (ci1 == MAP_FAILED)) {
                printf("\n");
                perror("mmap failed");
                SKIP_IF(1);
        }

I think TEST_FAIL means the test was able to exercise the feature
and found a problem with it. In this case, the test wasn't able to
exercise the feature so it's not appropriate.

Ideally, there should be a TEST_ERROR result for a case like this where
an unexpected problem prevented the testcase from exercising the
feature.

If we're to use the an existing result then I vote for SKIP_IF.

Yeah I agree.

See for example some of the TM tests, which skip if TM is not available.
Or the alignment test which skips if it can't open /dev/fb0.

In this case it should print "you need to be root to run this" and then
skip.

Agreed that there should be some indicator of why we are skipping the
test. My original point I was trying to make was that I thought
skipping a failed open was okay because we are likely not root.
However, I wasn't sure that a failed write was okay to skip as that
could be an indicator that something has actually been broken.

Yes I agree. I didn't read your mail closely enough. Have just replied
to it.

cheers