Received-SPF: pass (google.com: domain of linux-kernel+bounces-206108-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 7 Jun 2024 14:12:21 +0100
From: Mark Brown <broonie@kernel.org>
To: Dev Jain <dev.jain@arm.com>
Cc: shuah@kernel.org, oleg@redhat.com, stsp2@yandex.ru, mingo@kernel.org,
	tglx@linutronix.de, mark.rutland@arm.com, ryan.roberts@arm.com,
	suzuki.poulose@arm.com, Anshuman.Khandual@arm.com,
	DeepakKumar.Mishra@arm.com, AneeshKumar.KizhakeVeetil@arm.com,
	linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] selftests: Add a test mangling with uc_sigmask
Message-ID: <ZmMHNZcYfNMW1Ft7@finisterre.sirena.org.uk>
References: <20240607122319.768640-1-dev.jain@arm.com>
 <20240607122319.768640-3-dev.jain@arm.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="cF6vW8T0ysx4GnzU"
Content-Disposition: inline
In-Reply-To: <20240607122319.768640-3-dev.jain@arm.com>


--cF6vW8T0ysx4GnzU
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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.

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

> +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.

> +	/* 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.

--cF6vW8T0ysx4GnzU
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmZjBzIACgkQJNaLcl1U
h9BGwAf+NAq/bq240V4/kOR1sft3cqiATc7NHxEpjjk0MCkAsSzcxKDaMbPujgVx
i8iNT1QYF43Rip16x/x0NKxttjm1X/EWoytGmOjXHSN8J4xl68LZX97wCfqwlTUB
gW+L4K8T8i68m/bAnr31VkTAP/tBoX60r2KsuV8YgPA1+yHsUkA75KS9GmDfT5Bi
R/Rwe7gVzCezAQF7A1pX5O3RoFq4dBpDxiAc/rSuTR9pqlWdn4VY1VCdWGRZse5B
YAXYthSgs7DnYHHLT8uyLq/qtYLrsWZJlWWtvTGgegM3aQoGklL/0XmiX2OfVNJG
DxntSU9FL/FD5tKdhHGqL13yXE3GCQ==
=ZvFX
-----END PGP SIGNATURE-----

--cF6vW8T0ysx4GnzU--