Linux-kselftest Archive on lore.kernel.org
 help / color / Atom feed
From: Cristian Marussi <cristian.marussi@arm.com>
To: Dave Martin <Dave.Martin@arm.com>
Cc: linux-kselftest@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v3 02/11] kselftest: arm64: adds first test and common utils
Date: Fri, 9 Aug 2019 13:20:45 +0100
Message-ID: <4a73fcdf-911e-b44a-ce6b-f9bcde34eec8@arm.com> (raw)
In-Reply-To: <20190809111635.GL10425@arm.com>


Hi

On 8/9/19 12:16 PM, Dave Martin wrote:
> On Fri, Aug 09, 2019 at 11:54:06AM +0100, Cristian Marussi wrote:
>> Hi
>>
>> On 8/2/19 6:02 PM, Cristian Marussi wrote:
>>> Added some arm64/signal specific boilerplate and utility code to help
>>> further testcase development.
>>>
>>> A simple testcase and related helpers are also introduced in this commit:
>>> mangle_pstate_invalid_compat_toggle is a simple mangle testcase which
>>> messes with the ucontext_t from within the sig_handler, trying to toggle
>>> PSTATE state bits to switch the system between 32bit/64bit execution state.
>>> Expects SIGSEGV on test PASS.
>>>
>>> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
>>> ---
>>> A few fixes:
>>> - test_arm64_signals.sh runner script generation has been reviewed in order to
>>>    be safe against the .gitignore
>>> - using kselftest.h officially provided defines for tests' return values
>>> - removed SAFE_WRITE()/dump_uc()
>>> - looking for si_code==SEGV_ACCERR on SEGV test cases to better understand if
>>>    the sigfault had been directly triggered by Kernel
>>> ---
>>>   tools/testing/selftests/arm64/Makefile        |   2 +-
>>>   .../testing/selftests/arm64/signal/.gitignore |   6 +
>>>   tools/testing/selftests/arm64/signal/Makefile |  88 ++++++
>>>   tools/testing/selftests/arm64/signal/README   |  59 ++++
>>>   .../arm64/signal/test_arm64_signals.src_shell |  55 ++++
>>>   .../selftests/arm64/signal/test_signals.c     |  26 ++
>>>   .../selftests/arm64/signal/test_signals.h     | 137 +++++++++
>>>   .../arm64/signal/test_signals_utils.c         | 261 ++++++++++++++++++
>>>   .../arm64/signal/test_signals_utils.h         |  13 +
>>>   .../arm64/signal/testcases/.gitignore         |   1 +
>>>   .../mangle_pstate_invalid_compat_toggle.c     |  25 ++
>>>   .../arm64/signal/testcases/testcases.c        | 150 ++++++++++
>>>   .../arm64/signal/testcases/testcases.h        |  83 ++++++
>>>   13 files changed, 905 insertions(+), 1 deletion(-)
>>>   create mode 100644 tools/testing/selftests/arm64/signal/.gitignore
>>>   create mode 100644 tools/testing/selftests/arm64/signal/Makefile
>>>   create mode 100644 tools/testing/selftests/arm64/signal/README
>>>   create mode 100755 tools/testing/selftests/arm64/signal/test_arm64_signals.src_shell
>>>   create mode 100644 tools/testing/selftests/arm64/signal/test_signals.c
>>>   create mode 100644 tools/testing/selftests/arm64/signal/test_signals.h
>>>   create mode 100644 tools/testing/selftests/arm64/signal/test_signals_utils.c
>>>   create mode 100644 tools/testing/selftests/arm64/signal/test_signals_utils.h
>>>   create mode 100644 tools/testing/selftests/arm64/signal/testcases/.gitignore
>>>   create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_compat_toggle.c
>>>   create mode 100644 tools/testing/selftests/arm64/signal/testcases/testcases.c
>>>   create mode 100644 tools/testing/selftests/arm64/signal/testcases/testcases.h
>>>
>>
>> A few more compilation warnings triggered by GCC-8 ONLY when compiling via the top kselftest Makefile/target
>> (due to some additional -W passed down and an awkward use of snprintf on my side...)
>>
>>
>> test_signals_utils.c: In function ‘feats_to_string’:
>> test_signals_utils.c:38:13: warning: passing argument 1 to restrict-qualified parameter aliases with argument 4 [-Wrestrict]
>>      snprintf(feats_string, MAX_FEATS_SZ - 1, "%s %s ",
>>               ^~~~~~~~~~~~
>> test_signals_utils.c: In function ‘default_handler’:
>> test_signals_utils.c:192:19: warning: format ‘%p’ expects argument of type ‘void *’, but argument 3 has type ‘long long unsigned int’ [-Wformat=]
>>      "SIG_OK -- SP:%p  si_addr@:0x%p  si_code:%d  token@:0x%p  offset:%ld\n",
>>                    ~^
>>
>> will be fixed in V4 as:
>>
>>
>> diff --git a/tools/testing/selftests/arm64/signal/test_signals_utils.c b/tools/testing/selftests/arm64/signal/test_signals_utils.c
>> index 31788a1d33a4..c0f3cd1b560a 100644
>> --- a/tools/testing/selftests/arm64/signal/test_signals_utils.c
>> +++ b/tools/testing/selftests/arm64/signal/test_signals_utils.c
>> @@ -23,21 +23,25 @@ extern struct tdescr *current;
>>   static int sig_copyctx = SIGTRAP;
>>   static char *feats_store[FMAX_END] = {
>> -       "SSBS",
>> -       "PAN",
>> -       "UAO",
>> +       " SSBS ",
>> +       " PAN ",
>> +       " UAO ",
>>   };
>>   #define MAX_FEATS_SZ   128
>> +static char feats_string[MAX_FEATS_SZ];
>> +
>>   static inline char *feats_to_string(unsigned long feats)
>>   {
>> -       static char feats_string[MAX_FEATS_SZ];
>> +       for (int i = 0; i < FMAX_END; i++) {
>> +               size_t tlen = 0;
>> -       for (int i = 0; i < FMAX_END && feats_store[i][0]; i++) {
>> -               if (feats & 1UL << i)
>> -                       snprintf(feats_string, MAX_FEATS_SZ - 1, "%s %s ",
>> -                                feats_string, feats_store[i]);
>> +               if (feats & 1UL << i) {
>> +                       strncat(feats_string, feats_store[i],
> 
> Should this be feats_string + tlen?
>

strncat appends to the end of a NULL terminated string overwriting the NULL itself and
appending its own NULL (as long as dest and src do not overlap and fits the max size param),
so it must be fed the start of the dest string to which we are appending
  
>> +                               MAX_FEATS_SZ - 1 - tlen);
> 
> An assert(tlen <= MAX_FEATS_SZ - 1) is probably a good idea here,
> in case more features are added to feats_store[] someday.
> 

Yes in fact...if not it would be simply truncated silently

>> +                       tlen += strlen(feats_store[i]);
>> +               }
> 
> Don't we need to initialise tlen outside the loop?  Otherwise we just
> zero it again after the +=.

..and that's a bug :<

> 
>>          }
>>          return feats_string;
>> @@ -190,7 +194,7 @@ static void default_handler(int signum, siginfo_t *si, void *uc)
>>                  /* it's a bug in the test code when this assert fail */
>>                  assert(!current->sig_trig || current->triggered);
>>                  fprintf(stderr,
>> -                       "SIG_OK -- SP:%p  si_addr@:0x%p  si_code:%d  token@:0x%p  offset:%ld\n",
>> +                       "SIG_OK -- SP:%llX  si_addr@:0x%p  si_code:%d  token@:0x%p  offset:%ld\n",
> 
> For consistency, can we have a "0x" prefix?
> 
> I think %p usually generates a "0x" prefix by itself, so 0x%p might give
> a double prefix.
> 

Yes you are right.

Moreover I'm in doubt what to do generally with all these stderr output, because I optionally discard to null
testing standalone, but this is not what the KSFT framework runner script does, so arm64/signal tests
end up being overly verbose when run from the framework (even if tests use anyway the KSFT exit codes
conventions so all the results are correctly reported); but I suppose I'll receive a clear indication on this matter
from the maintainers at the end...
  
Cheers

Cristian

> [...]
> 
> Cheers
> ---Dave
> 

  reply index

Thread overview: 56+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-02 17:02 [PATCH v3 00/11] Add arm64/signal initial kselftest support Cristian Marussi
2019-08-02 17:02 ` [PATCH v3 01/11] kselftest: arm64: introduce new boilerplate code Cristian Marussi
2019-08-13 16:23   ` Dave Martin
2019-08-27 12:14     ` Cristian Marussi
2019-08-02 17:02 ` [PATCH v3 02/11] kselftest: arm64: adds first test and common utils Cristian Marussi
2019-08-06 15:50   ` Cristian Marussi
2019-08-07 15:42   ` Cristian Marussi
2019-08-09 10:54   ` Cristian Marussi
2019-08-09 11:16     ` Dave Martin
2019-08-09 12:20       ` Cristian Marussi [this message]
2019-08-09 12:32         ` Dave Martin
2019-08-12 12:43   ` Amit Kachhap
2019-08-13 13:22     ` Cristian Marussi
2019-08-14 10:22       ` Amit Kachhap
2019-08-27 14:24         ` Cristian Marussi
2019-08-13 16:24   ` Dave Martin
2019-08-28 17:34     ` Cristian Marussi
2019-09-03 15:34       ` Dave Martin
2019-09-03 16:08         ` Cristian Marussi
2019-08-02 17:02 ` [PATCH v3 03/11] kselftest: arm64: mangle_pstate_invalid_daif_bits Cristian Marussi
2019-08-13 16:24   ` Dave Martin
2019-08-29 10:19     ` Cristian Marussi
2019-08-02 17:02 ` [PATCH v3 04/11] kselftest: arm64: mangle_pstate_invalid_mode_el Cristian Marussi
2019-08-13 16:24   ` Dave Martin
2019-08-29 11:50     ` Cristian Marussi
2019-08-02 17:02 ` [PATCH v3 05/11] kselftest: arm64: mangle_pstate_ssbs_regs Cristian Marussi
2019-08-13 16:25   ` Dave Martin
2019-08-29 15:35     ` Cristian Marussi
2019-08-02 17:02 ` [PATCH v3 06/11] kselftest: arm64: fake_sigreturn_bad_magic Cristian Marussi
2019-08-13 16:25   ` Dave Martin
2019-08-30 14:29     ` Cristian Marussi
2019-09-04 10:05       ` Dave Martin
2019-09-04 10:37         ` Cristian Marussi
2019-09-04 10:47           ` Dave Martin
2019-08-02 17:02 ` [PATCH v3 07/11] kselftest: arm64: fake_sigreturn_bad_size_for_magic0 Cristian Marussi
2019-08-13 16:25   ` Dave Martin
2019-08-30 14:49     ` Cristian Marussi
2019-08-02 17:02 ` [PATCH v3 08/11] kselftest: arm64: fake_sigreturn_missing_fpsimd Cristian Marussi
2019-08-13 16:26   ` Dave Martin
2019-08-30 14:55     ` Cristian Marussi
2019-08-02 17:02 ` [PATCH v3 09/11] kselftest: arm64: fake_sigreturn_duplicated_fpsimd Cristian Marussi
2019-08-13 16:26   ` Dave Martin
2019-08-30 15:11     ` Cristian Marussi
2019-08-02 17:02 ` [PATCH v3 10/11] kselftest: arm64: fake_sigreturn_bad_size Cristian Marussi
2019-08-13 16:26   ` Dave Martin
2019-08-30 15:21     ` Cristian Marussi
2019-08-02 17:03 ` [PATCH v3 11/11] kselftest: arm64: fake_sigreturn_misaligned_sp Cristian Marussi
2019-08-07 16:04   ` Cristian Marussi
2019-08-13 16:28     ` Dave Martin
2019-08-30 15:22       ` Cristian Marussi
2019-08-13 16:27   ` Dave Martin
2019-08-30 16:33     ` Cristian Marussi
2019-08-13 16:22 ` [PATCH v3 00/11] Add arm64/signal initial kselftest support Dave Martin
2019-08-30 16:40   ` Cristian Marussi
2019-09-02 10:53     ` Dave Martin
2019-09-02 11:30       ` Cristian Marussi

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4a73fcdf-911e-b44a-ce6b-f9bcde34eec8@arm.com \
    --to=cristian.marussi@arm.com \
    --cc=Dave.Martin@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kselftest@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-kselftest Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-kselftest/0 linux-kselftest/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-kselftest linux-kselftest/ https://lore.kernel.org/linux-kselftest \
		linux-kselftest@vger.kernel.org linux-kselftest@archiver.kernel.org
	public-inbox-index linux-kselftest

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kselftest


AGPL code for this site: git clone https://public-inbox.org/ public-inbox