Re: [LTP] [PATCH] syscalls/prctl10: Add basic test for PR_SET/GET_TSC

[Richard Palethorpe <rpalethorpe@suse.de>]

Hello,

"xuyang2018.jy@fujitsu.com" <xuyang2018.jy@fujitsu.com> writes:

> Hi All
>
> Any comment?

Ah, yes, better late than never.

>
> Best Regards
> Yang Xu
>> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
>> ---
>>   include/lapi/prctl.h                       |   7 ++
>>   runtest/syscalls                           |   1 +
>>   testcases/kernel/syscalls/prctl/.gitignore |   1 +
>>   testcases/kernel/syscalls/prctl/prctl10.c  | 112 +++++++++++++++++++++
>>   4 files changed, 121 insertions(+)
>>   create mode 100644 testcases/kernel/syscalls/prctl/prctl10.c
>> 
>> diff --git a/include/lapi/prctl.h b/include/lapi/prctl.h
>> index fa5922231..8d3ef5c32 100644
>> --- a/include/lapi/prctl.h
>> +++ b/include/lapi/prctl.h
>> @@ -19,6 +19,13 @@
>>   # define PR_SET_SECCOMP  22
>>   #endif
>>   
>> +#ifndef PR_SET_TSC
>> +# define PR_GET_TSC 25
>> +# define PR_SET_TSC 26
>> +# define PR_TSC_ENABLE  1
>> +# define PR_TSC_SIGSEGV 2
>> +#endif
>> +
>>   #ifndef PR_SET_TIMERSLACK
>>   # define PR_SET_TIMERSLACK 29
>>   # define PR_GET_TIMERSLACK 30
>> diff --git a/runtest/syscalls b/runtest/syscalls
>> index 36fc50aeb..a0935821a 100644
>> --- a/runtest/syscalls
>> +++ b/runtest/syscalls
>> @@ -1004,6 +1004,7 @@ prctl06 prctl06
>>   prctl07 prctl07
>>   prctl08 prctl08
>>   prctl09 prctl09
>> +prctl10 prctl10
>>   
>>   pread01 pread01
>>   pread01_64 pread01_64
>> diff --git a/testcases/kernel/syscalls/prctl/.gitignore b/testcases/kernel/syscalls/prctl/.gitignore
>> index 0f2c9b194..50ee4bf60 100644
>> --- a/testcases/kernel/syscalls/prctl/.gitignore
>> +++ b/testcases/kernel/syscalls/prctl/.gitignore
>> @@ -8,3 +8,4 @@
>>   /prctl07
>>   /prctl08
>>   /prctl09
>> +/prctl10
>> diff --git a/testcases/kernel/syscalls/prctl/prctl10.c b/testcases/kernel/syscalls/prctl/prctl10.c
>> new file mode 100644
>> index 000000000..1b6791679
>> --- /dev/null
>> +++ b/testcases/kernel/syscalls/prctl/prctl10.c
>> @@ -0,0 +1,112 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (c) 2022 FUJITSU LIMITED. All rights reserved.
>> + * Author: Yang Xu <xuyang2018.jy@fujitsu.com>
>> + */
>> +
>> +/*\
>> + * [Description]
>> + *
>> + * Basic test to test behaviour of PR_GET_TSC and PR_SET_TSC.
>> + *
>> + * Set the state of the flag determining whether the timestamp counter can
>> + * be read by the process.
>> + *
>> + * - Pass PR_TSC_ENABLE to arg2 to allow it to be read.
>> + * - Pass PR_TSC_SIGSEGV to arg2 to generate a SIGSEGV when reading TSC.
>> + */
>> +
>> +#include <sys/prctl.h>
>> +#include <string.h>
>> +#include <stdio.h>
>> +#include <setjmp.h>
>> +#include "tst_test.h"
>> +#include "lapi/prctl.h"
>> +
>> +#define TCASE_ENTRY(tsc_read_stat) { .name = #tsc_read_stat, .read_stat = tsc_read_stat}
>> +
>> +static int pass;
>> +static sigjmp_buf env;
>> +
>> +static const char *tsc_read_stat_names[] = {
>> +	[0] = "[not set]",
>> +	[PR_TSC_ENABLE] = "PR_TSC_ENABLE",
>> +	[PR_TSC_SIGSEGV] = "PR_TSC_SIGSEGV",
>> +};
>> +
>> +static struct tcase {
>> +	char *name;
>> +	int read_stat;
>> +} tcases[] = {
>> +	TCASE_ENTRY(PR_TSC_ENABLE),
>> +	TCASE_ENTRY(PR_TSC_SIGSEGV)
>> +};
>> +
>> +static void sighandler(int sig LTP_ATTRIBUTE_UNUSED)
>> +{
>> +	pass = 1;
>> +	TST_EXP_PASS_SILENT(prctl(PR_SET_TSC, PR_TSC_ENABLE));
>> +	siglongjmp(env, 1);
>> +}
>> +
>> +static uint64_t rdtsc(void)
>> +{
>> +	uint32_t lo, hi;
>> +	/* We cannot use "=A", since this would use %rax on x86_64 */
>> +	__asm__ __volatile__ ("rdtsc" : "=a" (lo), "=d" (hi));
>> +	return (uint64_t)hi << 32 | lo;
>> +}
>> +
>> +static void verify_prctl(unsigned int n)
>> +{
>> +	struct tcase *tc = &tcases[n];
>> +	unsigned long long time1, time2;
>> +	int tsc_val = 0;
>> +
>> +	TST_EXP_PASS_SILENT(prctl(PR_SET_TSC, tc->read_stat));
>> +	TST_EXP_PASS_SILENT(prctl(PR_GET_TSC, &tsc_val));
>> +	if (tsc_val == tc->read_stat)
>> +		tst_res(TPASS, "current state is %s(%d)",
>> +				tc->name, tc->read_stat);
>> +	else
>> +		tst_res(TFAIL, "current state is %s(%d), expect %s(%d)",
>> +				tsc_read_stat_names[tsc_val], tsc_val, tc->name, tc->read_stat);
>> +
>> +	if (tc->read_stat == PR_TSC_SIGSEGV) {
>> +		if (sigsetjmp(env, 1) == 0)
>> +			rdtsc();

I think that because rdtsc is volatile, the load of 'pass' won't be
moved before it by the compiler. OTOH pass itself is not marked volatile and we
are relying on the compiler infering that it is volatile from the signal
handler instead of assuming it is 0.

To be on the safe side we could mark pass as volatile or use the atomic
functions. However see below.

>> +
>> +		if (pass)
>> +			tst_res(TPASS,
>> +				"get SIGSEGV signal under PR_TSC_SIGSEGV situation");
>> +		else
>> +			tst_res(TFAIL,
>> +				"don't get SIGSEGV signal under PR_TSC_SIGSEGV situation");
>> +		pass = 0;
>> +	}
>> +
>> +	time1 = rdtsc();
>> +	time2 = rdtsc();
>> +	if (time2 > time1)
>> +		tst_res(TPASS, "rdtsc works correctly, %lld ->%lld",
>> +			time1, time2);
>> +	else
>> +		tst_res(TFAIL, "rdtsc works incorrectly, %lld ->%lld",
>> +			time1, time2);
>> +}
>> +
>> +static void setup(void)
>> +{
>> +	SAFE_SIGNAL(SIGSEGV, sighandler);

So if we segfault for any other reason some wierd stuff could
happen. Wouldn't it be easier to fork a child process and check if it is
killed by SIGSEGV?

It would be easier for me to reason about at least.

>> +}
>> +
>> +static struct tst_test test = {
>> +	.setup = setup,
>> +	.test = verify_prctl,
>> +	.tcnt = ARRAY_SIZE(tcases),
>> +	.supported_archs = (const char *const []) {
>> +		"x86",
>> +		"x86_64",
>> +		NULL
>> +	},
>> +};


-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp