[LTP] [PATCH 1/3] lib: add .min_cpus in tst_test struct

[LTP] [PATCH 1/3] lib: add .min_cpus in tst_test struct

[Li Wang]

As the name implies, this new field is mainly to set minimum numbers
of CPU for LTP testcase. If system online CPUs are less than .min_cpus,
test will be exit with TCONF.

Note: I wouldn't add more description in test-writing-guidelines.txt,
because it is easy to understand the usage from the name.

Signed-off-by: Li Wang <liwang@redhat.com>
---
 include/tst_test.h | 3 +++
 lib/tst_test.c     | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/include/tst_test.h b/include/tst_test.h
index c91d3f18a..64a0934dd 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -157,6 +157,9 @@ struct tst_test {
 	 */
 	int all_filesystems:1;
 
+	/* Minimal numbers of CPU online required by the test */
+	unsigned long min_cpus;
+
 	/*
 	 * If set non-zero number of request_hugepages, test will try to reserve the
 	 * expected number of hugepage for testing in setup phase. If system does not
diff --git a/lib/tst_test.c b/lib/tst_test.c
index 535c0ff4c..90b69789c 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -918,6 +918,9 @@ static void do_setup(int argc, char *argv[])
 	if (tst_test->all_filesystems)
 		tst_test->needs_device = 1;
 
+	if (tst_test->min_cpus > tst_ncpus())
+		tst_brk(TCONF, "Test needs %lu CPUs online", tst_test->min_cpus);
+
 	if (tst_test->request_hugepages)
 		tst_request_hugepages(tst_test->request_hugepages);
 
-- 
2.21.3

[LTP] [PATCH 2/3] syscalls: take use of .min_cpus

[Li Wang]

To set the minimal numbers of online CPU for test.

[From Martin Doucha]
af_alg07 requires 2 CPUs, otherwise it'll report false positives.
The test will pass only if fchownat() hits a half-closed socket and
returns error. But IIRC the half-closed socket will be destroyed during
reschedule which means there's no race window to hit anymore. But it
would be better to put the TCONF condition into the test itself.

getcwd04: as itself requirements.

Signed-off-by: Li Wang <liwang@redhat.com>
---
 testcases/kernel/crypto/af_alg07.c          | 1 +
 testcases/kernel/syscalls/getcwd/getcwd04.c | 6 ++----
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/testcases/kernel/crypto/af_alg07.c b/testcases/kernel/crypto/af_alg07.c
index 6ad86f4f3..539ebee11 100644
--- a/testcases/kernel/crypto/af_alg07.c
+++ b/testcases/kernel/crypto/af_alg07.c
@@ -110,6 +110,7 @@ static struct tst_test test = {
 	.setup = setup,
 	.cleanup = cleanup,
 	.min_kver = "4.10.0",
+	.min_cpus = 2,
 	.taint_check = TST_TAINT_W | TST_TAINT_D,
 	.tags = (const struct tst_tag[]) {
 		{"linux-git", "9060cb719e61"},
diff --git a/testcases/kernel/syscalls/getcwd/getcwd04.c b/testcases/kernel/syscalls/getcwd/getcwd04.c
index 2fa65fb2c..2e07e675f 100644
--- a/testcases/kernel/syscalls/getcwd/getcwd04.c
+++ b/testcases/kernel/syscalls/getcwd/getcwd04.c
@@ -68,9 +68,6 @@ static void verify_getcwd(void)
 
 static void setup(void)
 {
-	if (tst_ncpus() == 1)
-		tst_brk(TCONF, "This test needs two cpus at least");
-
 	SAFE_SIGNAL(SIGALRM, sigproc);
 
 	alarm(TIMEOUT);
@@ -101,5 +98,6 @@ static struct tst_test test = {
 	.setup = setup,
 	.test_all = verify_getcwd,
 	.needs_tmpdir = 1,
-	.forks_child = 1
+	.forks_child = 1,
+	.min_cpus = 2
 };
-- 
2.21.3

[LTP] [PATCH 3/3] af_alg07: add dynamic bias for ARM

[Li Wang]

We observed that af_alg07 sporadically failed on hpe-moonshot(aarch64) with
fixed kernel, but after trying manually we can NOT reproduce it again, one
possibility is that the FAIL is caused by an unaligned race window between
two threads, so here add dynamic bias adjustment to see if it helps.

Point of view from Martin Doucha:
1) fchownat() returns 0 => fchownat() was called too early or the kernel is vulnerable
2) fchwonat() fails with ENOENT => kernel is fixed, print TPASS and exit
3) fchownat() fails with EBADF => fchownat() was called too late

Signed-off-by: Li Wang <liwang@redhat.com>
CC: Richard Palethorpe <rpalethorpe@suse.com>
CC: Martin Doucha <mdoucha@suse.cz>
CC: Joerg Vehlow <lkml@jv-coder.de>
CC: Chunyu Hu <chuhu@redhat.com>
CC: Cyril Hrubis <chrubis@suse.cz>
---
 testcases/kernel/crypto/af_alg07.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/testcases/kernel/crypto/af_alg07.c b/testcases/kernel/crypto/af_alg07.c
index 539ebee11..ee48c2edb 100644
--- a/testcases/kernel/crypto/af_alg07.c
+++ b/testcases/kernel/crypto/af_alg07.c
@@ -76,6 +76,16 @@ static void run(void)
 			return;
 		}
 
+		if (TST_RET == 0) {
+			tst_fzsync_pair_add_bias(&fzsync_pair, -1);
+			continue;
+		}
+
+		if (TST_RET == -1 && TST_ERR == EBADF) {
+			tst_fzsync_pair_add_bias(&fzsync_pair, 1);
+			continue;
+		}
+
 		if (TST_RET == -1 && TST_ERR == ENOENT) {
 			tst_res(TPASS | TTERRNO,
 				"fchownat() failed successfully");
-- 
2.21.3

[LTP] [PATCH 3/3] af_alg07: add dynamic bias for ARM

[Richard Palethorpe]

Hello,

Li Wang <liwang@redhat.com> writes:

> We observed that af_alg07 sporadically failed on hpe-moonshot(aarch64) with
> fixed kernel, but after trying manually we can NOT reproduce it again, one
> possibility is that the FAIL is caused by an unaligned race window between
> two threads, so here add dynamic bias adjustment to see if it helps.
>
> Point of view from Martin Doucha:
> 1) fchownat() returns 0 => fchownat() was called too early or the kernel is vulnerable
> 2) fchwonat() fails with ENOENT => kernel is fixed, print TPASS and exit
> 3) fchownat() fails with EBADF => fchownat() was called too late

Just FYI, what really prevents fzsync from working is if the temporal
behaviour (time to execute) of a syscall changes dramatically depending
on the outcome of a race[1].

For example if fchownat returns very quickly after EBADF, but takes a
long time otherwise, then the delay will be too small. I guess most
syscalls will check the FD very early so this is likely to be the
problem.

>
> Signed-off-by: Li Wang <liwang@redhat.com>
> CC: Richard Palethorpe <rpalethorpe@suse.com>
> CC: Martin Doucha <mdoucha@suse.cz>
> CC: Joerg Vehlow <lkml@jv-coder.de>
> CC: Chunyu Hu <chuhu@redhat.com>
> CC: Cyril Hrubis <chrubis@suse.cz>
> ---
>  testcases/kernel/crypto/af_alg07.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/testcases/kernel/crypto/af_alg07.c b/testcases/kernel/crypto/af_alg07.c
> index 539ebee11..ee48c2edb 100644
> --- a/testcases/kernel/crypto/af_alg07.c
> +++ b/testcases/kernel/crypto/af_alg07.c
> @@ -76,6 +76,16 @@ static void run(void)
>  			return;
>  		}
>  
> +		if (TST_RET == 0) {
> +			tst_fzsync_pair_add_bias(&fzsync_pair, -1);
> +			continue;
> +		}

It may actually be better to remove this and only increase the delay if
EBADF?

> +
> +		if (TST_RET == -1 && TST_ERR == EBADF) {
> +			tst_fzsync_pair_add_bias(&fzsync_pair, 1);
> +			continue;
> +		}
> +
>  		if (TST_RET == -1 && TST_ERR == ENOENT) {
>  			tst_res(TPASS | TTERRNO,
>  				"fchownat() failed successfully");

[1] Not necessarily the race which causes the bug.

-- 
Thank you,
Richard.

[LTP] [PATCH 3/3] af_alg07: add dynamic bias for ARM

[Martin Doucha]

On 03. 12. 20 13:32, Richard Palethorpe wrote:
> Hello,
> 
> Li Wang <liwang@redhat.com> writes:
> 
>> We observed that af_alg07 sporadically failed on hpe-moonshot(aarch64) with
>> fixed kernel, but after trying manually we can NOT reproduce it again, one
>> possibility is that the FAIL is caused by an unaligned race window between
>> two threads, so here add dynamic bias adjustment to see if it helps.
>>
>> Point of view from Martin Doucha:
>> 1) fchownat() returns 0 => fchownat() was called too early or the kernel is vulnerable
>> 2) fchwonat() fails with ENOENT => kernel is fixed, print TPASS and exit
>> 3) fchownat() fails with EBADF => fchownat() was called too late
> 
> Just FYI, what really prevents fzsync from working is if the temporal
> behaviour (time to execute) of a syscall changes dramatically depending
> on the outcome of a race[1].
> 
> For example if fchownat returns very quickly after EBADF, but takes a
> long time otherwise, then the delay will be too small. I guess most
> syscalls will check the FD very early so this is likely to be the
> problem.
> 
>>
>> Signed-off-by: Li Wang <liwang@redhat.com>
>> CC: Richard Palethorpe <rpalethorpe@suse.com>
>> CC: Martin Doucha <mdoucha@suse.cz>
>> CC: Joerg Vehlow <lkml@jv-coder.de>
>> CC: Chunyu Hu <chuhu@redhat.com>
>> CC: Cyril Hrubis <chrubis@suse.cz>
>> ---
>>  testcases/kernel/crypto/af_alg07.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/testcases/kernel/crypto/af_alg07.c b/testcases/kernel/crypto/af_alg07.c
>> index 539ebee11..ee48c2edb 100644
>> --- a/testcases/kernel/crypto/af_alg07.c
>> +++ b/testcases/kernel/crypto/af_alg07.c
>> @@ -76,6 +76,16 @@ static void run(void)
>>  			return;
>>  		}
>>  
>> +		if (TST_RET == 0) {
>> +			tst_fzsync_pair_add_bias(&fzsync_pair, -1);
>> +			continue;
>> +		}
> 
> It may actually be better to remove this and only increase the delay if
> EBADF?

Agreed. Adjusting delay in both directions would be acceptable if the
test takes too much time or doesn't work at all without it. Otherwise
it's preferable to adjust delay only when syscalls return an error state
that clearly falls outside the race window.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

[LTP] [PATCH 1/3] lib: add .min_cpus in tst_test struct

[Richard Palethorpe]

Hello li,

Only some minor nits.

Li Wang <liwang@redhat.com> writes:

> As the name implies, this new field is mainly to set minimum numbers
> of CPU for LTP testcase. If system online CPUs are less than .min_cpus,
> test will be exit with TCONF.
>
> Note: I wouldn't add more description in test-writing-guidelines.txt,
> because it is easy to understand the usage from the name.
>
> Signed-off-by: Li Wang <liwang@redhat.com>
> ---
>  include/tst_test.h | 3 +++
>  lib/tst_test.c     | 3 +++
>  2 files changed, 6 insertions(+)
>
> diff --git a/include/tst_test.h b/include/tst_test.h
> index c91d3f18a..64a0934dd 100644
> --- a/include/tst_test.h
> +++ b/include/tst_test.h
> @@ -157,6 +157,9 @@ struct tst_test {
>  	 */
>  	int all_filesystems:1;
>  
> +	/* Minimal numbers of CPU online required by the test */

*Mininum number of online CPUs required by the test*

> +	unsigned long min_cpus;
> +
>  	/*
>  	 * If set non-zero number of request_hugepages, test will try to reserve the
>  	 * expected number of hugepage for testing in setup phase. If system does not
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index 535c0ff4c..90b69789c 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -918,6 +918,9 @@ static void do_setup(int argc, char *argv[])
>  	if (tst_test->all_filesystems)
>  		tst_test->needs_device = 1;
>  
> +	if (tst_test->min_cpus > tst_ncpus())
> +		tst_brk(TCONF, "Test needs %lu CPUs online", tst_test->min_cpus);
                                          ^at least 

> +
>  	if (tst_test->request_hugepages)
>  		tst_request_hugepages(tst_test->request_hugepages);
>  
> -- 
> 2.21.3


-- 
Thank you,
Richard.

[LTP] [PATCH v2 1/3] lib: add .min_cpus in tst_test struct

[Li Wang]

As the name implies, this new field is mainly to set minimum numbers
of CPU for LTP testcase. If system online CPUs are less than .min_cpus,
test will be exit with TCONF.

Note: I wouldn't add more description in test-writing-guidelines.txt,
because it is easy to understand the usage from the name.

Signed-off-by: Li Wang <liwang@redhat.com>
---
 include/tst_test.h | 3 +++
 lib/tst_test.c     | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/include/tst_test.h b/include/tst_test.h
index c91d3f18a..b4fc4db47 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -157,6 +157,9 @@ struct tst_test {
 	 */
 	int all_filesystems:1;
 
+	/* Minimum numbers of online CPU required by the test */
+	unsigned long min_cpus;
+
 	/*
 	 * If set non-zero number of request_hugepages, test will try to reserve the
 	 * expected number of hugepage for testing in setup phase. If system does not
diff --git a/lib/tst_test.c b/lib/tst_test.c
index 535c0ff4c..c4e04eb4a 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -918,6 +918,9 @@ static void do_setup(int argc, char *argv[])
 	if (tst_test->all_filesystems)
 		tst_test->needs_device = 1;
 
+	if (tst_test->min_cpus > tst_ncpus())
+		tst_brk(TCONF, "Test needs at least %lu CPUs online", tst_test->min_cpus);
+
 	if (tst_test->request_hugepages)
 		tst_request_hugepages(tst_test->request_hugepages);
 
-- 
2.21.3

[LTP] [PATCH v2 2/3] syscalls: make use of .min_cpus

[Li Wang]

To set the minimum numbers of online CPU for test.

From Martin Doucha:

af_alg07 requires 2 CPUs, otherwise it'll report false positives.
The test will pass only if fchownat() hits a half-closed socket and
returns error. But IIRC the half-closed socket will be destroyed during
reschedule which means there's no race window to hit anymore. But it
would be better to put the TCONF condition into the test itself.

getcwd04: as itself requirements.

Signed-off-by: Li Wang <liwang@redhat.com>
---
 testcases/kernel/crypto/af_alg07.c          | 1 +
 testcases/kernel/syscalls/getcwd/getcwd04.c | 6 ++----
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/testcases/kernel/crypto/af_alg07.c b/testcases/kernel/crypto/af_alg07.c
index 6ad86f4f3..539ebee11 100644
--- a/testcases/kernel/crypto/af_alg07.c
+++ b/testcases/kernel/crypto/af_alg07.c
@@ -110,6 +110,7 @@ static struct tst_test test = {
 	.setup = setup,
 	.cleanup = cleanup,
 	.min_kver = "4.10.0",
+	.min_cpus = 2,
 	.taint_check = TST_TAINT_W | TST_TAINT_D,
 	.tags = (const struct tst_tag[]) {
 		{"linux-git", "9060cb719e61"},
diff --git a/testcases/kernel/syscalls/getcwd/getcwd04.c b/testcases/kernel/syscalls/getcwd/getcwd04.c
index 2fa65fb2c..2e07e675f 100644
--- a/testcases/kernel/syscalls/getcwd/getcwd04.c
+++ b/testcases/kernel/syscalls/getcwd/getcwd04.c
@@ -68,9 +68,6 @@ static void verify_getcwd(void)
 
 static void setup(void)
 {
-	if (tst_ncpus() == 1)
-		tst_brk(TCONF, "This test needs two cpus at least");
-
 	SAFE_SIGNAL(SIGALRM, sigproc);
 
 	alarm(TIMEOUT);
@@ -101,5 +98,6 @@ static struct tst_test test = {
 	.setup = setup,
 	.test_all = verify_getcwd,
 	.needs_tmpdir = 1,
-	.forks_child = 1
+	.forks_child = 1,
+	.min_cpus = 2
 };
-- 
2.21.3

[LTP] [PATCH v2 2/3] syscalls: make use of .min_cpus

[Cyril Hrubis]

Hi!
Pushed, thanks.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 3/3] af_alg07: add dynamic bias for ARM

[Li Wang]

We observed that af_alg07 sporadically failed on hpe-moonshot(aarch64) with
fixed kernel, but after trying manually we can NOT reproduce it again, one
possibility is that the FAIL is caused by an unaligned race window between
two threads, so here add dynamic bias adjustment to see if it helps.

From Martin Doucha:

1) fchownat() returns 0 => fchownat() was called too early or the kernel is vulnerable
2) fchwonat() fails with ENOENT => kernel is fixed, print TPASS and exit
3) fchownat() fails with EBADF => fchownat() was called too late

Considering the test does NOT takes too time in 1) so we only adjust
delay when syscalls return an error state that clearly falls outside
the race window.

Signed-off-by: Li Wang <liwang@redhat.com>
CC: Richard Palethorpe <rpalethorpe@suse.com>
CC: Martin Doucha <mdoucha@suse.cz>
CC: Joerg Vehlow <lkml@jv-coder.de>
CC: Chunyu Hu <chuhu@redhat.com>
CC: Cyril Hrubis <chrubis@suse.cz>
---
 testcases/kernel/crypto/af_alg07.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/testcases/kernel/crypto/af_alg07.c b/testcases/kernel/crypto/af_alg07.c
index 539ebee11..c94bb292f 100644
--- a/testcases/kernel/crypto/af_alg07.c
+++ b/testcases/kernel/crypto/af_alg07.c
@@ -76,6 +76,11 @@ static void run(void)
 			return;
 		}
 
+		if (TST_RET == -1 && TST_ERR == EBADF) {
+			tst_fzsync_pair_add_bias(&fzsync_pair, 1);
+			continue;
+		}
+
 		if (TST_RET == -1 && TST_ERR == ENOENT) {
 			tst_res(TPASS | TTERRNO,
 				"fchownat() failed successfully");
-- 
2.21.3

[LTP] [PATCH v2 3/3] af_alg07: add dynamic bias for ARM

[Li Wang]

Hi all,

Any objection to merging this one? or comments?

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20201209/9a2111b5/attachment.htm>

[LTP] [PATCH v2 3/3] af_alg07: add dynamic bias for ARM

[Richard Palethorpe]

Hello Li,

Li Wang <liwang@redhat.com> writes:

> Hi all,
>
> Any objection to merging this one? or comments?

LGTM!

-- 
Thank you,
Richard.

[LTP] [PATCH v2 3/3] af_alg07: add dynamic bias for ARM

[Martin Doucha]

On 04. 12. 20 7:33, Li Wang wrote:
> We observed that af_alg07 sporadically failed on hpe-moonshot(aarch64) with
> fixed kernel, but after trying manually we can NOT reproduce it again, one
> possibility is that the FAIL is caused by an unaligned race window between
> two threads, so here add dynamic bias adjustment to see if it helps.
> 
> From Martin Doucha:
> 
> 1) fchownat() returns 0 => fchownat() was called too early or the kernel is vulnerable
> 2) fchwonat() fails with ENOENT => kernel is fixed, print TPASS and exit
> 3) fchownat() fails with EBADF => fchownat() was called too late
> 
> Considering the test does NOT takes too time in 1) so we only adjust
> delay when syscalls return an error state that clearly falls outside
> the race window.
> 
> Signed-off-by: Li Wang <liwang@redhat.com>
> CC: Richard Palethorpe <rpalethorpe@suse.com>
> CC: Martin Doucha <mdoucha@suse.cz>
> CC: Joerg Vehlow <lkml@jv-coder.de>
> CC: Chunyu Hu <chuhu@redhat.com>
> CC: Cyril Hrubis <chrubis@suse.cz>

Reviewed-by: Martin Doucha <mdoucha@suse.cz>

> ---
>  testcases/kernel/crypto/af_alg07.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/testcases/kernel/crypto/af_alg07.c b/testcases/kernel/crypto/af_alg07.c
> index 539ebee11..c94bb292f 100644
> --- a/testcases/kernel/crypto/af_alg07.c
> +++ b/testcases/kernel/crypto/af_alg07.c
> @@ -76,6 +76,11 @@ static void run(void)
>  			return;
>  		}
>  
> +		if (TST_RET == -1 && TST_ERR == EBADF) {
> +			tst_fzsync_pair_add_bias(&fzsync_pair, 1);
> +			continue;
> +		}
> +
>  		if (TST_RET == -1 && TST_ERR == ENOENT) {
>  			tst_res(TPASS | TTERRNO,
>  				"fchownat() failed successfully");
> 


-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

[LTP] [PATCH v2 3/3] af_alg07: add dynamic bias for ARM

[Li Wang]

> Reviewed-by: Martin Doucha <mdoucha@suse.cz>
>

Pushed, thanks Martin and Richard for the review.

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20201209/f4af3ab7/attachment.htm>

[LTP] [PATCH v2 1/3] lib: add .min_cpus in tst_test struct

[Cyril Hrubis]

Hi!
Pushed with two fixes, thanks.

> +	/* Minimum numbers of online CPU required by the test */
                      ^
		      number
> +	unsigned long min_cpus;
> +
>  	/*
>  	 * If set non-zero number of request_hugepages, test will try to reserve the
>  	 * expected number of hugepage for testing in setup phase. If system does not
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index 535c0ff4c..c4e04eb4a 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -918,6 +918,9 @@ static void do_setup(int argc, char *argv[])
>  	if (tst_test->all_filesystems)
>  		tst_test->needs_device = 1;
>  
> +	if (tst_test->min_cpus > tst_ncpus())
                                   ^
				   Added (unsigned long) cast to silence
				   warnings.
> +		tst_brk(TCONF, "Test needs at least %lu CPUs online", tst_test->min_cpus);
> +
>  	if (tst_test->request_hugepages)
>  		tst_request_hugepages(tst_test->request_hugepages);
>  
> -- 
> 2.21.3
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz