Received-SPF: pass (google.com: domain of linux-kernel+bounces-206133-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <5d26ac17-a50a-46c4-8fcb-68eaa6d0ce2a@arm.com>
Date: Fri, 7 Jun 2024 18:53:27 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 2/2] selftests: Add a test mangling with uc_sigmask
To: Mark Brown <broonie@kernel.org>
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
References: <20240607122319.768640-1-dev.jain@arm.com>
 <20240607122319.768640-3-dev.jain@arm.com>
 <ZmMHNZcYfNMW1Ft7@finisterre.sirena.org.uk>
Content-Language: en-US
From: Dev Jain <dev.jain@arm.com>
In-Reply-To: <ZmMHNZcYfNMW1Ft7@finisterre.sirena.org.uk>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


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.