Re: [PATCH 2/2] selftests: Add a test mangling with uc_sigmask

[Dev Jain <dev.jain@arm.com>]

On 6/7/24 18:42, Mark Brown wrote:
> On Fri, Jun 07, 2024 at 05:53:19PM +0530, Dev Jain wrote:
>> This test asserts the relation between blocked signal, delivered signal,
>> and ucontext. The ucontext is mangled with, by adding a signal mask to
>> it; on return from the handler, the thread must block the corresponding
>> signal.
>> @@ -1,2 +1,3 @@
>>   # SPDX-License-Identifier: GPL-2.0-only
>>   sigaltstack
>> +mangle_uc_sigmask
> Please keep these build files sorted alphabetically, this reduces
> spurioius conflicts between serieses.


Sure.

>
>> + * Author: Dev Jain <dev.jain@arm.com>
>> + *
>> + * Test describing a clear distinction between signal states - delivered and
>> + * blocked, and their relation with ucontext.
> This would be clearer if it said more positiviely what the relationship
> between these things is actually expected to be and how they're tested.
> Right now it's a bit hard to tell what the test is actually verifying.


I thought I had described that quite well in the code comments.

Anyways, I shall incorporate some detail into the initial test

description too.

>
>> +void handler_verify_ucontext(int signo, siginfo_t *info, void *uc)
>> +{
>> +	int ret;
>> +
>> +	/* Kernel dumps ucontext with USR2 blocked */
>> +	ret = sigismember(&(((ucontext_t *)uc)->uc_sigmask), SIGUSR2);
>> +	ksft_test_result(ret == 1, "USR2 in ucontext\n");
> "USR2 blocked in ucontext".
>
>> +
>> +	raise(SIGUSR2);
>> +}
> A comment explaining that we're verifying that the signal is blocked
> might be good (I think that's what this is doing?).  We're also not
> checking the return value of raise() anywhere in the program, this would
> be a useful diagnostic.


Sure.

>
>> +	/* SEGV blocked during handler execution, delivered on return */
>> +	raise(SIGPIPE);
>> +	ksft_print_msg("SEGV bypassed successfully\n");
> SIGPIPE or SIGEGV?
>
>> +	/* SIGPIPE has been blocked in sa_mask, but ucontext is invariant */
>> +	ret = sigismember(&(((ucontext_t *)uc)->uc_sigmask), SIGPIPE);
>> +	ksft_test_result(ret == 0, "USR1 not in ucontext\n");
> The relationship between the comment and test are not clear here, nor is
> that between the sigismembber() call and the test name we print?
>
>> +	/* SIGUSR1 has been blocked, but ucontext is invariant */
>> +	ret = sigismember(&(((ucontext_t *)uc)->uc_sigmask), SIGUSR1);
>> +	ksft_test_result(ret == 0, "SEGV not in ucontext\n");
> Similarly here.
>
>> +	/* add SEGV to blocked mask */
>> +	if (sigemptyset(&act.sa_mask) || sigaddset(&act.sa_mask, SIGPIPE)
>> +	    || (sigismember(&act.sa_mask, SIGPIPE) != 1))
>> +		ksft_exit_fail_msg("Cannot add SEGV to blocked mask\n");
> SIGPIPE vs SIGSEGV.


Ah sorry, I was testing out something else, and then I

did something and it partially changed it back to SEGV.

I shall revert all mentions of PIPE with SEGV. Please read

all mentions of pipe, or PIPE, as segv and SEGV.