This patch series is motivated by the following observation:
Raise a signal, jump to signal handler. The ucontext_t structure dumped by kernel to userspace has a uc_sigmask field having the mask of blocked signals. If you run a fresh minimalistic program doing this, this field is empty, even if you block some signals while registering the handler with sigaction().
Here is what the man-pages have to say:
sigaction(2): "sa_mask specifies a mask of signals which should be blocked (i.e., added to the signal mask of the thread in which the signal handler is invoked) during execution of the signal handler. In addition, the signal which triggered the handler will be blocked, unless the SA_NODEFER flag is used."
signal(7): Under "Execution of signal handlers", (1.3) implies:
"The thread's current signal mask is accessible via the ucontext_t object that is pointed to by the third argument of the signal handler."
But, (1.4) states:
"Any signals specified in act->sa_mask when registering the handler with sigprocmask(2) are added to the thread's signal mask. The signal being delivered is also added to the signal mask, unless SA_NODEFER was specified when registering the handler. These signals are thus blocked while the handler executes."
There clearly is no distinction being made in the man pages between "Thread's signal mask" and ucontext_t; this logically should imply that a signal blocked by populating struct sigaction should be visible in ucontext_t.
Here is what the kernel code does (for Aarch64):
do_signal() -> handle_signal() -> sigmask_to_save(), which returns ¤t->blocked, is passed to setup_rt_frame() -> setup_sigframe() -> __copy_to_user(). Hence, ¤t->blocked is copied to ucontext_t exposed to userspace. Returning back to handle_signal(), signal_setup_done() -> signal_delivered() -> sigorsets() and set_current_blocked() are responsible for using information from struct ksignal ksig, which was populated through the sigaction() system call in kernel/signal.c: copy_from_user(&new_sa.sa, act, sizeof(new_sa.sa)), to update ¤t->blocked; hence, the set of blocked signals for the current thread is updated AFTER the kernel dumps ucontext_t to userspace.
Assuming that the above is indeed the intended behaviour, because it semantically makes sense, since the signals blocked using sigaction() remain blocked only till the execution of the handler, and not in the context present before jumping to the handler (but nothing can be confirmed from the man-pages), the series introduces a test for mangling with uc_sigmask. I will send a separate series to fix the man-pages.
The proposed selftest has been tested out on Aarch32, Aarch64 and x86_64.
v5->v6: - Drop renaming of sas.c - Include the explanation from the cover letter in the changelog for the second patch
v4->v5: - Remove a redundant print statement
v3->v4: - Allocate sigsets as automatic variables to avoid malloc()
v2->v3: - ucontext describes current state -> ucontext describes interrupted context - Add a comment for blockage of USR2 even after return from handler - Describe blockage of signals in a better way
v1->v2: - Replace all occurrences of SIGPIPE with SIGSEGV - Fixed a mismatch between code comment and ksft log - Add a testcase: Raise the same signal again; it must not be queued - Remove unneeded <assert.h>, <unistd.h> - Give a detailed test description in the comments; also describe the exact meaning of delivered and blocked - Handle errors for all libc functions/syscalls - Mention tests in Makefile and .gitignore in alphabetical order
v1: - https://lore.kernel.org/all/20240607122319.768640-1-dev.jain@arm.com/
Dev Jain (2): selftests: Rename sigaltstack to generic signal selftests: Add a test mangling with uc_sigmask
tools/testing/selftests/Makefile | 2 +- .../{sigaltstack => signal}/.gitignore | 1 + .../{sigaltstack => signal}/Makefile | 3 +- .../current_stack_pointer.h | 0 .../selftests/signal/mangle_uc_sigmask.c | 184 ++++++++++++++++++ .../selftests/{sigaltstack => signal}/sas.c | 0 6 files changed, 188 insertions(+), 2 deletions(-) rename tools/testing/selftests/{sigaltstack => signal}/.gitignore (70%) rename tools/testing/selftests/{sigaltstack => signal}/Makefile (56%) rename tools/testing/selftests/{sigaltstack => signal}/current_stack_pointer.h (100%) create mode 100644 tools/testing/selftests/signal/mangle_uc_sigmask.c rename tools/testing/selftests/{sigaltstack => signal}/sas.c (100%)
Rename sigaltstack to generic signal directory, to allow adding more signal tests in the future.
Signed-off-by: Dev Jain dev.jain@arm.com Reviewed-by: Mark Brown broonie@kernel.org --- tools/testing/selftests/Makefile | 2 +- tools/testing/selftests/{sigaltstack => signal}/.gitignore | 0 tools/testing/selftests/{sigaltstack => signal}/Makefile | 0 .../selftests/{sigaltstack => signal}/current_stack_pointer.h | 0 tools/testing/selftests/{sigaltstack => signal}/sas.c | 0 5 files changed, 1 insertion(+), 1 deletion(-) rename tools/testing/selftests/{sigaltstack => signal}/.gitignore (100%) rename tools/testing/selftests/{sigaltstack => signal}/Makefile (100%) rename tools/testing/selftests/{sigaltstack => signal}/current_stack_pointer.h (100%) rename tools/testing/selftests/{sigaltstack => signal}/sas.c (100%)
diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile index bc8fe9e8f7f2..edbe30fb3304 100644 --- a/tools/testing/selftests/Makefile +++ b/tools/testing/selftests/Makefile @@ -87,7 +87,7 @@ TARGETS += rtc TARGETS += rust TARGETS += seccomp TARGETS += sgx -TARGETS += sigaltstack +TARGETS += signal TARGETS += size TARGETS += sparc64 TARGETS += splice diff --git a/tools/testing/selftests/sigaltstack/.gitignore b/tools/testing/selftests/signal/.gitignore similarity index 100% rename from tools/testing/selftests/sigaltstack/.gitignore rename to tools/testing/selftests/signal/.gitignore diff --git a/tools/testing/selftests/sigaltstack/Makefile b/tools/testing/selftests/signal/Makefile similarity index 100% rename from tools/testing/selftests/sigaltstack/Makefile rename to tools/testing/selftests/signal/Makefile diff --git a/tools/testing/selftests/sigaltstack/current_stack_pointer.h b/tools/testing/selftests/signal/current_stack_pointer.h similarity index 100% rename from tools/testing/selftests/sigaltstack/current_stack_pointer.h rename to tools/testing/selftests/signal/current_stack_pointer.h diff --git a/tools/testing/selftests/sigaltstack/sas.c b/tools/testing/selftests/signal/sas.c similarity index 100% rename from tools/testing/selftests/sigaltstack/sas.c rename to tools/testing/selftests/signal/sas.c
On 8/22/24 06:14, Dev Jain wrote:
Rename sigaltstack to generic signal directory, to allow adding more signal tests in the future.
Sorry - I think I mentioned I don't like this test renamed. Why are you sending this rename still included in the patch series?
thanks, -- Shuah
On 8/27/24 17:14, Shuah Khan wrote:
On 8/22/24 06:14, Dev Jain wrote:
Rename sigaltstack to generic signal directory, to allow adding more signal tests in the future.
Sorry - I think I mentioned I don't like this test renamed. Why are you sending this rename still included in the patch series?
I am not renaming the test, just the directory. The directory name is changed to signal, and I have retained the name of the test - sas.c.
thanks, -- Shuah
On 8/27/24 17:16, Dev Jain wrote:
On 8/27/24 17:14, Shuah Khan wrote:
On 8/22/24 06:14, Dev Jain wrote:
Rename sigaltstack to generic signal directory, to allow adding more signal tests in the future.
Sorry - I think I mentioned I don't like this test renamed. Why are you sending this rename still included in the patch series?
I am not renaming the test, just the directory. The directory name is changed to signal, and I have retained the name of the test - sas.c.
Gentle ping: I guess there was a misunderstanding; in v5, I was also changing the name of the test, to which you objected, and I agreed. But, we need to change the name of the directory since the new test has no relation to the current directory name, "sigaltstack". The patch description explains that the directory should be generically named.
thanks, -- Shuah
On 8/30/24 10:29, Dev Jain wrote:
On 8/27/24 17:16, Dev Jain wrote:
On 8/27/24 17:14, Shuah Khan wrote:
On 8/22/24 06:14, Dev Jain wrote:
Rename sigaltstack to generic signal directory, to allow adding more signal tests in the future.
Sorry - I think I mentioned I don't like this test renamed. Why are you sending this rename still included in the patch series?
I am not renaming the test, just the directory. The directory name is changed to signal, and I have retained the name of the test - sas.c.
Gentle ping: I guess there was a misunderstanding; in v5, I was also changing the name of the test, to which you objected, and I agreed. But, we need to change the name of the directory since the new test has no relation to the current directory name, "sigaltstack". The patch description explains that the directory should be generically named.
Right. You are no longer changing the test name. You are still changing the directory name. The problem I mentioned stays the same. Any fixes to the existing tests in this directory can no longer auto applied to stables releases.
Other than the desire to rename the directory to generic, what other value does this change bring?
thanks, -- Shuah
On 9/4/24 03:14, Shuah Khan wrote:
On 8/30/24 10:29, Dev Jain wrote:
On 8/27/24 17:16, Dev Jain wrote:
On 8/27/24 17:14, Shuah Khan wrote:
On 8/22/24 06:14, Dev Jain wrote:
Rename sigaltstack to generic signal directory, to allow adding more signal tests in the future.
Sorry - I think I mentioned I don't like this test renamed. Why are you sending this rename still included in the patch series?
I am not renaming the test, just the directory. The directory name is changed to signal, and I have retained the name of the test - sas.c.
Gentle ping: I guess there was a misunderstanding; in v5, I was also changing the name of the test, to which you objected, and I agreed. But, we need to change the name of the directory since the new test has no relation to the current directory name, "sigaltstack". The patch description explains that the directory should be generically named.
Right. You are no longer changing the test name. You are still changing the directory name. The problem I mentioned stays the same. Any fixes to the existing tests in this directory can no longer auto applied to stables releases.
I understand your point, but commit baa489fabd01 (selftests/vm: rename selftests/vm to selftests/mm) is also present. That was a lot bigger change; sigaltstack contains just one test currently, whose fixes possibly would have to be backported, so I guess it should not be that much of a big problem?
Other than the desire to rename the directory to generic, what other value does this change bring?
Do you have an alternative suggestion as to where I should put my new test then; I do not see what is the value of creating another directory to just include my test. This will unnecessarily clutter the selftests/ directory with directories containing single tests. And, putting this in "sigaltstack" is just wrong since this test has no relation with sigaltstack.
On 9/3/24 22:52, Dev Jain wrote:
On 9/4/24 03:14, Shuah Khan wrote:
On 8/30/24 10:29, Dev Jain wrote:
On 8/27/24 17:16, Dev Jain wrote:
On 8/27/24 17:14, Shuah Khan wrote:
On 8/22/24 06:14, Dev Jain wrote:
Rename sigaltstack to generic signal directory, to allow adding more signal tests in the future.
Sorry - I think I mentioned I don't like this test renamed. Why are you sending this rename still included in the patch series?
I am not renaming the test, just the directory. The directory name is changed to signal, and I have retained the name of the test - sas.c.
Gentle ping: I guess there was a misunderstanding; in v5, I was also changing the name of the test, to which you objected, and I agreed. But, we need to change the name of the directory since the new test has no relation to the current directory name, "sigaltstack". The patch description explains that the directory should be generically named.
Right. You are no longer changing the test name. You are still changing the directory name. The problem I mentioned stays the same. Any fixes to the existing tests in this directory can no longer auto applied to stables releases.
I understand your point, but commit baa489fabd01 (selftests/vm: rename selftests/vm to selftests/mm) is also present. That was a lot bigger change; sigaltstack contains just one test currently, whose fixes possibly would have to be backported, so I guess it should not be that much of a big problem?
So who does the backports whenevenr something changes? You are adding work where as the automated process would just work without this change. It doesn't matter if there is another test that changed the name.
Other than the desire to rename the directory to generic, what other value does this change bring?
Do you have an alternative suggestion as to where I should put my new test then; I do not see what is the value of creating another directory to just include my test. This will unnecessarily clutter the selftests/ directory with directories containing single tests. And, putting this in "sigaltstack" is just wrong since this test has no relation with sigaltstack.
If this new test has no relation to sigaltstack, then why are you changing and renaming the sigaltstack directory? Adding a new directory is much better than going down a path that is more confusing and adding backport overhead.
Two options: -- Add a new directory or add a note and keep it under sigaltstack -- Do you foresee this new growing?
thanks, -- Shuah
On 9/4/24 22:35, Shuah Khan wrote:
On 9/3/24 22:52, Dev Jain wrote:
On 9/4/24 03:14, Shuah Khan wrote:
On 8/30/24 10:29, Dev Jain wrote:
On 8/27/24 17:16, Dev Jain wrote:
On 8/27/24 17:14, Shuah Khan wrote:
On 8/22/24 06:14, Dev Jain wrote: > Rename sigaltstack to generic signal directory, to allow adding > more > signal tests in the future.
Sorry - I think I mentioned I don't like this test renamed. Why are you sending this rename still included in the patch series?
I am not renaming the test, just the directory. The directory name is changed to signal, and I have retained the name of the test - sas.c.
Gentle ping: I guess there was a misunderstanding; in v5, I was also changing the name of the test, to which you objected, and I agreed. But, we need to change the name of the directory since the new test has no relation to the current directory name, "sigaltstack". The patch description explains that the directory should be generically named.
Right. You are no longer changing the test name. You are still changing the directory name. The problem I mentioned stays the same. Any fixes to the existing tests in this directory can no longer auto applied to stables releases.
I understand your point, but commit baa489fabd01 (selftests/vm: rename selftests/vm to selftests/mm) is also present. That was a lot bigger change; sigaltstack contains just one test currently, whose fixes possibly would have to be backported, so I guess it should not be that much of a big problem?
So who does the backports whenevenr something changes? You are adding work where as the automated process would just work without this change. It doesn't matter if there is another test that changed the name.
Other than the desire to rename the directory to generic, what other value does this change bring?
Do you have an alternative suggestion as to where I should put my new test then; I do not see what is the value of creating another directory to just include my test. This will unnecessarily clutter the selftests/ directory with directories containing single tests. And, putting this in "sigaltstack" is just wrong since this test has no relation with sigaltstack.
If this new test has no relation to sigaltstack, then why are you changing and renaming the sigaltstack directory?
Because the functionality I am testing is of signals, and signals are a superset of sigaltstack. Still, I can think of a compromise, if semantically you want to consider the new test as not testing signals, but a specific syscall "sigaction" and its interaction with blocking of signals, how about naming the new directory "sigaction"?
Adding a new directory is much better than going down a path that is more confusing and adding backport overhead.
Two options: -- Add a new directory or add a note and keep it under sigaltstack -- Do you foresee this new growing?
thanks, -- Shuah
On Thu, Sep 05, 2024 at 11:26:02AM +0530, Dev Jain wrote:
On 9/4/24 22:35, Shuah Khan wrote:
So who does the backports whenevenr something changes? You are adding work where as the automated process would just work without this change. It doesn't matter if there is another test that changed the name.
I thought git was supposed to have some ability to try to cope with renames, though heuristic based? It does seem to work sometimes. TBH I'm also not sure how frequent an issue backporting fixes to this one test is going to be, it's had a couple of minor fixes for warnings in the past few years and the last substantial update was in 2021.
If this new test has no relation to sigaltstack, then why are you changing and renaming the sigaltstack directory?
Because the functionality I am testing is of signals, and signals are a superset of sigaltstack. Still, I can think of a compromise, if semantically you want
I do tend to agree here, it seems neater to merge things and from the point of view of running the tests in CI it's nice to not have too many tiny suites, they create runtime overhead.
to consider the new test as not testing signals, but a specific syscall "sigaction" and its interaction with blocking of signals, how about naming the new directory "sigaction"?
That's not going to scale amazingly if we test any other aspects of signals... I'd just call it "signal" and if it's not possible to get the merge done just leave the sigaltstack suite as it is.
On 9/4/24 23:56, Dev Jain wrote:
On 9/4/24 22:35, Shuah Khan wrote:
On 9/3/24 22:52, Dev Jain wrote:
On 9/4/24 03:14, Shuah Khan wrote:
On 8/30/24 10:29, Dev Jain wrote:
On 8/27/24 17:16, Dev Jain wrote:
On 8/27/24 17:14, Shuah Khan wrote: > On 8/22/24 06:14, Dev Jain wrote: >> Rename sigaltstack to generic signal directory, to allow adding more >> signal tests in the future. > > Sorry - I think I mentioned I don't like this test renamed. Why are you sending > this rename still included in the patch series?
I am not renaming the test, just the directory. The directory name is changed to signal, and I have retained the name of the test - sas.c.
Gentle ping: I guess there was a misunderstanding; in v5, I was also changing the name of the test, to which you objected, and I agreed. But, we need to change the name of the directory since the new test has no relation to the current directory name, "sigaltstack". The patch description explains that the directory should be generically named.
Right. You are no longer changing the test name. You are still changing the directory name. The problem I mentioned stays the same. Any fixes to the existing tests in this directory can no longer auto applied to stables releases.
I understand your point, but commit baa489fabd01 (selftests/vm: rename selftests/vm to selftests/mm) is also present. That was a lot bigger change; sigaltstack contains just one test currently, whose fixes possibly would have to be backported, so I guess it should not be that much of a big problem?
So who does the backports whenevenr something changes? You are adding work where as the automated process would just work without this change. It doesn't matter if there is another test that changed the name.
Other than the desire to rename the directory to generic, what other value does this change bring?
Do you have an alternative suggestion as to where I should put my new test then; I do not see what is the value of creating another directory to just include my test. This will unnecessarily clutter the selftests/ directory with directories containing single tests. And, putting this in "sigaltstack" is just wrong since this test has no relation with sigaltstack.
If this new test has no relation to sigaltstack, then why are you changing and renaming the sigaltstack directory?
Because the functionality I am testing is of signals, and signals are a superset of sigaltstack. Still, I can think of a compromise, if semantically you want to consider the new test as not testing signals, but a specific syscall "sigaction" and its interaction with blocking of signals, how about naming the new directory "sigaction"?
Adding a new directory is much better than going down a path that is more confusing and adding backport overhead.
Okay - they are related except that you view signalstack as a subset of signals. I saw Mark's response as well saying sigaction isn't a good name for this.
Rename usually wipe out git history as well based on what have seen in the past.
My main concern is backports. Considering sigstack hasn't changed 2021 (as Mark's email), let's rename it.
I am reluctantly agreeing to the rename as it seems to make sense in this case.
thanks, -- Shuah
On 9/7/24 01:29, Shuah Khan wrote:
On 9/4/24 23:56, Dev Jain wrote:
On 9/4/24 22:35, Shuah Khan wrote:
On 9/3/24 22:52, Dev Jain wrote:
On 9/4/24 03:14, Shuah Khan wrote:
On 8/30/24 10:29, Dev Jain wrote:
On 8/27/24 17:16, Dev Jain wrote: > > On 8/27/24 17:14, Shuah Khan wrote: >> On 8/22/24 06:14, Dev Jain wrote: >>> Rename sigaltstack to generic signal directory, to allow >>> adding more >>> signal tests in the future. >> >> Sorry - I think I mentioned I don't like this test renamed. Why >> are you sending >> this rename still included in the patch series? > > I am not renaming the test, just the directory. The directory name > is changed to signal, and I have retained the name of the test - > sas.c.
Gentle ping: I guess there was a misunderstanding; in v5, I was also changing the name of the test, to which you objected, and I agreed. But, we need to change the name of the directory since the new test has no relation to the current directory name, "sigaltstack". The patch description explains that the directory should be generically named.
Right. You are no longer changing the test name. You are still changing the directory name. The problem I mentioned stays the same. Any fixes to the existing tests in this directory can no longer auto applied to stables releases.
I understand your point, but commit baa489fabd01 (selftests/vm: rename selftests/vm to selftests/mm) is also present. That was a lot bigger change; sigaltstack contains just one test currently, whose fixes possibly would have to be backported, so I guess it should not be that much of a big problem?
So who does the backports whenevenr something changes? You are adding work where as the automated process would just work without this change. It doesn't matter if there is another test that changed the name.
Other than the desire to rename the directory to generic, what other value does this change bring?
Do you have an alternative suggestion as to where I should put my new test then; I do not see what is the value of creating another directory to just include my test. This will unnecessarily clutter the selftests/ directory with directories containing single tests. And, putting this in "sigaltstack" is just wrong since this test has no relation with sigaltstack.
If this new test has no relation to sigaltstack, then why are you changing and renaming the sigaltstack directory?
Because the functionality I am testing is of signals, and signals are a superset of sigaltstack. Still, I can think of a compromise, if semantically you want to consider the new test as not testing signals, but a specific syscall "sigaction" and its interaction with blocking of signals, how about naming the new directory "sigaction"?
Adding a new directory is much better than going down a path that is more confusing and adding backport overhead.
Okay - they are related except that you view signalstack as a subset of signals. I saw Mark's response as well saying sigaction isn't a good name for this.
Rename usually wipe out git history as well based on what have seen in the past.
My main concern is backports. Considering sigstack hasn't changed 2021 (as Mark's email), let's rename it.
I am reluctantly agreeing to the rename as it seems to make sense in this case.
Thanks! I guess there is no update required from my side, and you can pull this series?
thanks, -- Shuah
On 9/8/24 23:16, Dev Jain wrote:
On 9/7/24 01:29, Shuah Khan wrote:
On 9/4/24 23:56, Dev Jain wrote:
On 9/4/24 22:35, Shuah Khan wrote:
On 9/3/24 22:52, Dev Jain wrote:
On 9/4/24 03:14, Shuah Khan wrote:
On 8/30/24 10:29, Dev Jain wrote: > > On 8/27/24 17:16, Dev Jain wrote: >> >> On 8/27/24 17:14, Shuah Khan wrote: >>> On 8/22/24 06:14, Dev Jain wrote: >>>> Rename sigaltstack to generic signal directory, to allow adding more >>>> signal tests in the future. >>> >>> Sorry - I think I mentioned I don't like this test renamed. Why are you sending >>> this rename still included in the patch series? >> >> I am not renaming the test, just the directory. The directory name >> is changed to signal, and I have retained the name of the test - >> sas.c. > > Gentle ping: I guess there was a misunderstanding; in v5, I was > also changing the name of the test, to which you objected, and > I agreed. But, we need to change the name of the directory since > the new test has no relation to the current directory name, > "sigaltstack". The patch description explains that the directory > should be generically named. >
Right. You are no longer changing the test name. You are still changing the directory name. The problem I mentioned stays the same. Any fixes to the existing tests in this directory can no longer auto applied to stables releases.
I understand your point, but commit baa489fabd01 (selftests/vm: rename selftests/vm to selftests/mm) is also present. That was a lot bigger change; sigaltstack contains just one test currently, whose fixes possibly would have to be backported, so I guess it should not be that much of a big problem?
So who does the backports whenevenr something changes? You are adding work where as the automated process would just work without this change. It doesn't matter if there is another test that changed the name.
Other than the desire to rename the directory to generic, what other value does this change bring?
Do you have an alternative suggestion as to where I should put my new test then; I do not see what is the value of creating another directory to just include my test. This will unnecessarily clutter the selftests/ directory with directories containing single tests. And, putting this in "sigaltstack" is just wrong since this test has no relation with sigaltstack.
If this new test has no relation to sigaltstack, then why are you changing and renaming the sigaltstack directory?
Because the functionality I am testing is of signals, and signals are a superset of sigaltstack. Still, I can think of a compromise, if semantically you want to consider the new test as not testing signals, but a specific syscall "sigaction" and its interaction with blocking of signals, how about naming the new directory "sigaction"?
Adding a new directory is much better than going down a path that is more confusing and adding backport overhead.
Okay - they are related except that you view signalstack as a subset of signals. I saw Mark's response as well saying sigaction isn't a good name for this.
Rename usually wipe out git history as well based on what have seen in the past.
My main concern is backports. Considering sigstack hasn't changed 2021 (as Mark's email), let's rename it.
I am reluctantly agreeing to the rename as it seems to make sense in this case.
Thanks! I guess there is no update required from my side, and you can pull this series?
I can pull this with x86v maintainer ack.
Or to go through x86 tree:
Acked-by: Shuah Khan skhan@linuxfoundation.org
thanks, -- Shuah
On 9/9/24 23:24, Shuah Khan wrote:
On 9/8/24 23:16, Dev Jain wrote:
On 9/7/24 01:29, Shuah Khan wrote:
On 9/4/24 23:56, Dev Jain wrote:
On 9/4/24 22:35, Shuah Khan wrote:
On 9/3/24 22:52, Dev Jain wrote:
On 9/4/24 03:14, Shuah Khan wrote: > On 8/30/24 10:29, Dev Jain wrote: >> >> On 8/27/24 17:16, Dev Jain wrote: >>> >>> On 8/27/24 17:14, Shuah Khan wrote: >>>> On 8/22/24 06:14, Dev Jain wrote: >>>>> Rename sigaltstack to generic signal directory, to allow >>>>> adding more >>>>> signal tests in the future. >>>> >>>> Sorry - I think I mentioned I don't like this test renamed. >>>> Why are you sending >>>> this rename still included in the patch series? >>> >>> I am not renaming the test, just the directory. The directory >>> name >>> is changed to signal, and I have retained the name of the test - >>> sas.c. >> >> Gentle ping: I guess there was a misunderstanding; in v5, I was >> also changing the name of the test, to which you objected, and >> I agreed. But, we need to change the name of the directory since >> the new test has no relation to the current directory name, >> "sigaltstack". The patch description explains that the directory >> should be generically named. >> > > Right. You are no longer changing the test name. You are still > changing the directory name. The problem I mentioned stays the > same. Any fixes to the existing tests in this directory can no > longer auto applied to stables releases.
I understand your point, but commit baa489fabd01 (selftests/vm: rename selftests/vm to selftests/mm) is also present. That was a lot bigger change; sigaltstack contains just one test currently, whose fixes possibly would have to be backported, so I guess it should not be that much of a big problem?
>
So who does the backports whenevenr something changes? You are adding work where as the automated process would just work without this change. It doesn't matter if there is another test that changed the name.
> Other than the desire to rename the directory to generic, what > other value does this change bring?
Do you have an alternative suggestion as to where I should put my new test then; I do not see what is the value of creating another directory to just include my test. This will unnecessarily clutter the selftests/ directory with directories containing single tests. And, putting this in "sigaltstack" is just wrong since this test has no relation with sigaltstack.
If this new test has no relation to sigaltstack, then why are you changing and renaming the sigaltstack directory?
Because the functionality I am testing is of signals, and signals are a superset of sigaltstack. Still, I can think of a compromise, if semantically you want to consider the new test as not testing signals, but a specific syscall "sigaction" and its interaction with blocking of signals, how about naming the new directory "sigaction"?
Adding a new directory is much better than going down a path that is more confusing and adding backport overhead.
Okay - they are related except that you view signalstack as a subset of signals. I saw Mark's response as well saying sigaction isn't a good name for this.
Rename usually wipe out git history as well based on what have seen in the past.
My main concern is backports. Considering sigstack hasn't changed 2021 (as Mark's email), let's rename it.
I am reluctantly agreeing to the rename as it seems to make sense in this case.
Thanks! I guess there is no update required from my side, and you can pull this series?
I can pull this with x86v maintainer ack.
Or to go through x86 tree:
Acked-by: Shuah Khan skhan@linuxfoundation.org
Gentle ping, adding all x86 maintainers and the x86 list, in case they missed.
On 9/16/24 09:28, Dev Jain wrote:
On 9/9/24 23:24, Shuah Khan wrote:
On 9/8/24 23:16, Dev Jain wrote:
On 9/7/24 01:29, Shuah Khan wrote:
On 9/4/24 23:56, Dev Jain wrote:
On 9/4/24 22:35, Shuah Khan wrote:
On 9/3/24 22:52, Dev Jain wrote: > > On 9/4/24 03:14, Shuah Khan wrote: >> On 8/30/24 10:29, Dev Jain wrote: >>> >>> On 8/27/24 17:16, Dev Jain wrote: >>>> >>>> On 8/27/24 17:14, Shuah Khan wrote: >>>>> On 8/22/24 06:14, Dev Jain wrote: >>>>>> Rename sigaltstack to generic signal directory, to allow >>>>>> adding more >>>>>> signal tests in the future. >>>>> >>>>> Sorry - I think I mentioned I don't like this test renamed. >>>>> Why are you sending >>>>> this rename still included in the patch series? >>>> >>>> I am not renaming the test, just the directory. The directory >>>> name >>>> is changed to signal, and I have retained the name of the test - >>>> sas.c. >>> >>> Gentle ping: I guess there was a misunderstanding; in v5, I was >>> also changing the name of the test, to which you objected, and >>> I agreed. But, we need to change the name of the directory since >>> the new test has no relation to the current directory name, >>> "sigaltstack". The patch description explains that the directory >>> should be generically named. >>> >> >> Right. You are no longer changing the test name. You are still >> changing the directory name. The problem I mentioned stays the >> same. Any fixes to the existing tests in this directory can no >> longer auto applied to stables releases. > > I understand your point, but commit baa489fabd01 (selftests/vm: > rename > selftests/vm to selftests/mm) is also present. That was a lot > bigger change; > sigaltstack contains just one test currently, whose fixes > possibly would have > to be backported, so I guess it should not be that much of a big > problem? > >>
So who does the backports whenevenr something changes? You are adding work where as the automated process would just work without this change. It doesn't matter if there is another test that changed the name.
>> Other than the desire to rename the directory to generic, what >> other value does this change bring? > > Do you have an alternative suggestion as to where I should put > my new test then; > I do not see what is the value of creating another directory to > just include > my test. This will unnecessarily clutter the selftests/ > directory with > directories containing single tests. And, putting this in > "sigaltstack" is just > wrong since this test has no relation with sigaltstack. >
If this new test has no relation to sigaltstack, then why are you changing and renaming the sigaltstack directory?
Because the functionality I am testing is of signals, and signals are a superset of sigaltstack. Still, I can think of a compromise, if semantically you want to consider the new test as not testing signals, but a specific syscall "sigaction" and its interaction with blocking of signals, how about naming the new directory "sigaction"?
Adding a new directory is much better than going down a path that is more confusing and adding backport overhead.
Okay - they are related except that you view signalstack as a subset of signals. I saw Mark's response as well saying sigaction isn't a good name for this.
Rename usually wipe out git history as well based on what have seen in the past.
My main concern is backports. Considering sigstack hasn't changed 2021 (as Mark's email), let's rename it.
I am reluctantly agreeing to the rename as it seems to make sense in this case.
Thanks! I guess there is no update required from my side, and you can pull this series?
I can pull this with x86v maintainer ack.
Or to go through x86 tree:
Acked-by: Shuah Khan skhan@linuxfoundation.org
Gentle ping, adding all x86 maintainers and the x86 list, in case they missed.
Gentle ping
On Mon, Oct 07, 2024 at 10:07:24AM +0530, Dev Jain wrote:
On 9/16/24 09:28, Dev Jain wrote:
Gentle ping, adding all x86 maintainers and the x86 list, in case they missed.
Gentle ping
Given that this was posted prior to the merge window you should probably resend it at this point.
The test is motivated by the following observation:
Raise a signal, jump to signal handler. The ucontext_t structure dumped by kernel to userspace has a uc_sigmask field having the mask of blocked signals. If you run a fresh minimalistic program doing this, this field is empty, even if you block some signals while registering the handler with sigaction().
Here is what the man-pages have to say:
sigaction(2): "sa_mask specifies a mask of signals which should be blocked (i.e., added to the signal mask of the thread in which the signal handler is invoked) during execution of the signal handler. In addition, the signal which triggered the handler will be blocked, unless the SA_NODEFER flag is used."
signal(7): Under "Execution of signal handlers", (1.3) implies:
"The thread's current signal mask is accessible via the ucontext_t object that is pointed to by the third argument of the signal handler."
But, (1.4) states:
"Any signals specified in act->sa_mask when registering the handler with sigprocmask(2) are added to the thread's signal mask. The signal being delivered is also added to the signal mask, unless SA_NODEFER was specified when registering the handler. These signals are thus blocked while the handler executes."
There clearly is no distinction being made in the man pages between "Thread's signal mask" and ucontext_t; this logically should imply that a signal blocked by populating struct sigaction should be visible in ucontext_t.
Here is what the kernel code does (for Aarch64):
do_signal() -> handle_signal() -> sigmask_to_save(), which returns ¤t->blocked, is passed to setup_rt_frame() -> setup_sigframe() -> __copy_to_user(). Hence, ¤t->blocked is copied to ucontext_t exposed to userspace. Returning back to handle_signal(), signal_setup_done() -> signal_delivered() -> sigorsets() and set_current_blocked() are responsible for using information from struct ksignal ksig, which was populated through the sigaction() system call in kernel/signal.c: copy_from_user(&new_sa.sa, act, sizeof(new_sa.sa)), to update ¤t->blocked; hence, the set of blocked signals for the current thread is updated AFTER the kernel dumps ucontext_t to userspace.
Assuming that the above is indeed the intended behaviour, because it semantically makes sense, since the signals blocked using sigaction() remain blocked only till the execution of the handler, and not in the context present before jumping to the handler (but nothing can be confirmed from the man-pages), this patch introduces a test for mangling with uc_sigmask.
The 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.
In the test description, I have also described signal delivery and blockage, for ease of understanding what the test does.
Signed-off-by: Dev Jain dev.jain@arm.com Reviewed-by: Mark Brown broonie@kernel.org --- tools/testing/selftests/signal/.gitignore | 1 + tools/testing/selftests/signal/Makefile | 3 +- .../selftests/signal/mangle_uc_sigmask.c | 184 ++++++++++++++++++ 3 files changed, 187 insertions(+), 1 deletion(-) create mode 100644 tools/testing/selftests/signal/mangle_uc_sigmask.c
diff --git a/tools/testing/selftests/signal/.gitignore b/tools/testing/selftests/signal/.gitignore index 50a19a8888ce..3f339865a3b6 100644 --- a/tools/testing/selftests/signal/.gitignore +++ b/tools/testing/selftests/signal/.gitignore @@ -1,2 +1,3 @@ # SPDX-License-Identifier: GPL-2.0-only +mangle_uc_sigmask sas diff --git a/tools/testing/selftests/signal/Makefile b/tools/testing/selftests/signal/Makefile index 3e96d5d47036..e0bf7058d19c 100644 --- a/tools/testing/selftests/signal/Makefile +++ b/tools/testing/selftests/signal/Makefile @@ -1,6 +1,7 @@ # SPDX-License-Identifier: GPL-2.0-only CFLAGS = -Wall -TEST_GEN_PROGS = sas +TEST_GEN_PROGS = mangle_uc_sigmask +TEST_GEN_PROGS += sas
include ../lib.mk
diff --git a/tools/testing/selftests/signal/mangle_uc_sigmask.c b/tools/testing/selftests/signal/mangle_uc_sigmask.c new file mode 100644 index 000000000000..b79ab92178a8 --- /dev/null +++ b/tools/testing/selftests/signal/mangle_uc_sigmask.c @@ -0,0 +1,184 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (C) 2024 ARM Ltd. + * + * Author: Dev Jain dev.jain@arm.com + * + * Test describing a clear distinction between signal states - delivered and + * blocked, and their relation with ucontext. + * + * A process can request blocking of a signal by masking it into its set of + * blocked signals; such a signal, when sent to the process by the kernel, + * will get blocked by the process and it may later unblock it and take an + * action. At that point, the signal will be delivered. + * + * We test the following functionalities of the kernel: + * + * ucontext_t describes the interrupted context of the thread; this implies + * that, in case of registering a handler and catching the corresponding + * signal, that state is before what was jumping into the handler. + * + * The thread's mask of blocked signals can be permanently changed, i.e, not + * just during the execution of the handler, by mangling with uc_sigmask + * from inside the handler. + * + * Assume that we block the set of signals, S1, by sigaction(), and say, the + * signal for which the handler was installed, is S2. When S2 is sent to the + * program, it will be considered "delivered", since we will act on the + * signal and jump to the handler. Any instances of S1 or S2 raised, while the + * program is executing inside the handler, will be blocked; they will be + * delivered immediately upon termination of the handler. + * + * For standard signals (also see real-time signals in the man page), multiple + * blocked instances of the same signal are not queued; such a signal will + * be delivered just once. + */ + +#include <stdio.h> +#include <stdlib.h> +#include <signal.h> +#include <ucontext.h> + +#include "../kselftest.h" + +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 blocked in ucontext\n"); + + /* + * USR2 is blocked; can be delivered neither here, nor after + * exit from handler + */ + if (raise(SIGUSR2)) + ksft_exit_fail_perror("raise"); +} + +void handler_segv(int signo, siginfo_t *info, void *uc) +{ + /* + * Three cases possible: + * 1. Program already terminated due to segmentation fault. + * 2. SEGV was blocked even after returning from handler_usr. + * 3. SEGV was delivered on returning from handler_usr. + * The last option must happen. + */ + ksft_test_result_pass("SEGV delivered\n"); +} + +static int cnt; + +void handler_usr(int signo, siginfo_t *info, void *uc) +{ + int ret; + + /* + * Break out of infinite recursion caused by raise(SIGUSR1) invoked + * from inside the handler + */ + ++cnt; + if (cnt > 1) + return; + + /* SEGV blocked during handler execution, delivered on return */ + if (raise(SIGSEGV)) + ksft_exit_fail_perror("raise"); + + ksft_print_msg("SEGV bypassed successfully\n"); + + /* + * Signal responsible for handler invocation is blocked by default; + * delivered on return, leading to recursion + */ + if (raise(SIGUSR1)) + ksft_exit_fail_perror("raise"); + + ksft_test_result(cnt == 1, + "USR1 is blocked, cannot invoke handler right now\n"); + + /* Raise USR1 again; only one instance must be delivered upon exit */ + if (raise(SIGUSR1)) + ksft_exit_fail_perror("raise"); + + /* SEGV has been blocked in sa_mask, but ucontext is empty */ + ret = sigismember(&(((ucontext_t *)uc)->uc_sigmask), SIGSEGV); + ksft_test_result(ret == 0, "SEGV not blocked in ucontext\n"); + + /* USR1 has been blocked, but ucontext is empty */ + ret = sigismember(&(((ucontext_t *)uc)->uc_sigmask), SIGUSR1); + ksft_test_result(ret == 0, "USR1 not blocked in ucontext\n"); + + /* + * Mangle ucontext; this will be copied back into ¤t->blocked + * on return from the handler. + */ + if (sigaddset(&((ucontext_t *)uc)->uc_sigmask, SIGUSR2)) + ksft_exit_fail_perror("sigaddset"); +} + +int main(int argc, char *argv[]) +{ + struct sigaction act, act2; + sigset_t set, oldset; + + ksft_print_header(); + ksft_set_plan(7); + + act.sa_flags = SA_SIGINFO; + act.sa_sigaction = &handler_usr; + + /* Add SEGV to blocked mask */ + if (sigemptyset(&act.sa_mask) || sigaddset(&act.sa_mask, SIGSEGV) + || (sigismember(&act.sa_mask, SIGSEGV) != 1)) + ksft_exit_fail_msg("Cannot add SEGV to blocked mask\n"); + + if (sigaction(SIGUSR1, &act, NULL)) + ksft_exit_fail_perror("Cannot install handler"); + + act2.sa_flags = SA_SIGINFO; + act2.sa_sigaction = &handler_segv; + + if (sigaction(SIGSEGV, &act2, NULL)) + ksft_exit_fail_perror("Cannot install handler"); + + /* Invoke handler */ + if (raise(SIGUSR1)) + ksft_exit_fail_perror("raise"); + + /* USR1 must not be queued */ + ksft_test_result(cnt == 2, "handler invoked only twice\n"); + + /* Mangled ucontext implies USR2 is blocked for current thread */ + if (raise(SIGUSR2)) + ksft_exit_fail_perror("raise"); + + ksft_print_msg("USR2 bypassed successfully\n"); + + act.sa_sigaction = &handler_verify_ucontext; + if (sigaction(SIGUSR1, &act, NULL)) + ksft_exit_fail_perror("Cannot install handler"); + + if (raise(SIGUSR1)) + ksft_exit_fail_perror("raise"); + + /* + * Raising USR2 in handler_verify_ucontext is redundant since it + * is blocked + */ + ksft_print_msg("USR2 still blocked on return from handler\n"); + + /* Confirm USR2 blockage by sigprocmask() too */ + if (sigemptyset(&set)) + ksft_exit_fail_perror("sigemptyset"); + + if (sigprocmask(SIG_BLOCK, &set, &oldset)) + ksft_exit_fail_perror("sigprocmask"); + + ksft_test_result(sigismember(&oldset, SIGUSR2) == 1, + "USR2 present in ¤t->blocked\n"); + + ksft_finished(); +}
linux-kselftest-mirror@lists.linaro.org