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

[Dev Jain]

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@xxxxxxx>
> > + *
> > + * 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.