LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: ebiederm@xmission.com (Eric W. Biederman)
To: Dave Martin <Dave.Martin@arm.com>
Cc: "linux-kernel\@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Linux Containers <containers@lists.linux-foundation.org>,
	Oleg Nesterov <oleg@redhat.com>,
	"linux-arch\@vger.kernel.org" <linux-arch@vger.kernel.org>,
	James Morse <James.Morse@arm.com>,
	Will Deacon <Will.Deacon@arm.com>
Subject: Re: [REVIEW][PATCH 03/26] signal/arm64: Use force_sig not force_sig_fault for SIGKILL
Date: Thu, 23 May 2019 09:53:06 -0500	[thread overview]
Message-ID: <87r28pgr3h.fsf@xmission.com> (raw)
In-Reply-To: <20190523102101.GW28398@e103592.cambridge.arm.com> (Dave Martin's message of "Thu, 23 May 2019 11:21:04 +0100")

Dave Martin <Dave.Martin@arm.com> writes:

> On Thu, May 23, 2019 at 01:38:53AM +0100, Eric W. Biederman wrote:
>> It really only matters to debuggers but the SIGKILL does not have any
>> si_codes that use the fault member of the siginfo union.  Correct this
>> the simple way and call force_sig instead of force_sig_fault when the
>> signal is SIGKILL.
>
> I haven't fully understood the context for this, but why does it matter
> what's in siginfo for SIGKILL?  My understanding is that userspace
> (including ptrace) never gets to see it anyway for the SIGKILL case.

Yes.  In practice I think it would take tracing or something very
exotic to notice anything going wrong because the task will be killed.

> Here it feels like SIGKILL is logically a synchronous, thread-targeted
> fault: we must ensure that no subsequent insn in current executes (just
> like other fault signal).  In this case, I thought we fall back to
> SIGKILL not because there is no fault, but because we failed to
> properly diagnose or report the type of fault that occurred.
>
> So maybe handling it consistently with other faults signals makes
> sense.  The fact that delivery of this signal destroys the process
> before anyone can look at the resulting siginfo feels like a
> side-effect rather than something obviously wrong.
>
> The siginfo is potentially useful diagnostic information, that we could
> subsequently provide a means to access post-mortem.
>
> I just dived in on this single patch, so I may be missing something more
> fundamental, or just being pedantic...

Not really.  I was working on another cleanup and this usage of SIGKILL
came up.

A synchronous thread synchronous fault gets us as far as the forc_sig
family of functions.  That only leaves the question of which union
member in struct siginfo we are using.  The union members are _kill,
_fault, _timer, _rt, _sigchld, _sigfault, _sigpoll, and _sigsys.

As it has prove quite error prone for people to fill out struct siginfo
in the past by hand, I have provided a couple of helper functions for
the common cases that come up such as: force_sig_fault,
force_sig_mceerr, force_sig_bnderr, force_sig_pkuerr.  Each of those
helper functions takes the information needed to fill out the union
member of struct siginfo that kind of fault corresponds to.

For the SIGKILL case the only si_code I see being passed SI_KERNEL.
The SI_KERNEL si_code corresponds to the _kill union member while
force_sig_fault fills in fields for the _fault union member.

Because of the mismatch of which union member SIGKILL should be using
and the union member force_sig_fault applies alarm bells ring in my head
when I read the current arm64 kernel code.  Somewhat doubly so because
the other fields in passed to force_sig_fault appear to be somewhat
random when SIGKILL is the signal.

So I figured let's preserve the usage of SIGKILL as a synchronous
exception.  That seems legitimate and other folks do that as well but
let's use force_sig instead of force_sig_fault instead.  I don't know if
userspace will notice but at the very least we won't be providing a bad
example for other kernel code to follow and we won't wind up be making
assumptions that are true today and false tomorrow when some
implementation detail changes.

For imformation on what signals and si_codes correspond to which
union members you can look at siginfo_layout.  That function
is the keeper of the magic decoder key.  Currently the only two
si_codes defined for SIGKILL are SI_KERNEL and SI_USER both of which
correspond to a _kill union member.

Eric


> Cheers
> ---Dave
>
>> Cc: stable@vger.kernel.org
>> Cc: Dave Martin <Dave.Martin@arm.com>
>> Cc: James Morse <james.morse@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Fixes: af40ff687bc9 ("arm64: signal: Ensure si_code is valid for all fault signals")
>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> ---
>>  arch/arm64/kernel/traps.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>> 
>> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
>> index ade32046f3fe..0feb17bdcaa0 100644
>> --- a/arch/arm64/kernel/traps.c
>> +++ b/arch/arm64/kernel/traps.c
>> @@ -282,6 +282,11 @@ void arm64_notify_die(const char *str, struct pt_regs *regs,
>>  		current->thread.fault_address = 0;
>>  		current->thread.fault_code = err;
>>  
>> +		if (signo == SIGKILL) {
>> +			arm64_show_signal(signo, str);
>> +			force_sig(signo, current);
>> +			return;
>> +		}
>>  		arm64_force_sig_fault(signo, sicode, addr, str);
>>  	} else {
>>  		die(str, regs, err);
>> -- 
>> 2.21.0
>> 

  reply	other threads:[~2019-05-23 14:53 UTC|newest]

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-23  0:38 [REVIEW][PATCH 00/26] signal: Remove task argument from force_sig_info Eric W. Biederman
2019-05-23  0:38 ` [REVIEW][PATCH 01/26] signal: Correct namespace fixups of si_pid and si_uid Eric W. Biederman
     [not found]   ` <20190529131503.F2AC221871@mail.kernel.org>
2019-05-29 15:18     ` Eric W. Biederman
2019-05-23  0:38 ` [REVIEW][PATCH 02/26] signal/ptrace: Simplify and fix PTRACE_KILL Eric W. Biederman
2019-05-29 14:35   ` Eric W. Biederman
2019-05-23  0:38 ` [REVIEW][PATCH 03/26] signal/arm64: Use force_sig not force_sig_fault for SIGKILL Eric W. Biederman
2019-05-23 10:17   ` Will Deacon
2019-05-23 14:59     ` Eric W. Biederman
2019-05-23 16:11     ` [REVIEW][PATCHv2 " Eric W. Biederman
2019-05-23 16:15       ` Will Deacon
2019-05-23 20:59         ` Eric W. Biederman
2019-05-24 10:00           ` Will Deacon
2019-05-24 22:36             ` Eric W. Biederman
2019-05-29 15:12               ` Will Deacon
2019-05-29 15:34                 ` Eric W. Biederman
2019-05-23 10:21   ` [REVIEW][PATCH " Dave Martin
2019-05-23 14:53     ` Eric W. Biederman [this message]
2019-05-23 16:12       ` Dave P Martin
2019-05-23 21:00         ` Eric W. Biederman
2019-05-23  0:38 ` [REVIEW][PATCH 04/26] signal/drbd: Use send_sig not force_sig Eric W. Biederman
2019-05-23  0:38 ` [REVIEW][PATCH 05/26] signal/bpfilter: Fix bpfilter_kernl to use " Eric W. Biederman
2019-05-23  0:38 ` [REVIEW][PATCH 06/26] signal/pid_namespace: Fix reboot_pid_ns " Eric W. Biederman
2019-05-23  0:38 ` [REVIEW][PATCH 07/26] signal/cifs: Fix cifs_put_tcp_session to call send_sig instead of force_sig Eric W. Biederman
2019-05-23  0:38 ` [REVIEW][PATCH 08/26] signal: Remove task parameter from force_sigsegv Eric W. Biederman
2019-05-23  0:38 ` [REVIEW][PATCH 09/26] signal: Remove task parameter from force_sig Eric W. Biederman
2019-05-23  0:39 ` [REVIEW][PATCH 10/26] signal: Remove task parameter from force_sig_mceerr Eric W. Biederman
2019-05-23  0:39 ` [REVIEW][PATCH 11/26] signal/x86: Remove task parameter from send_sigtrap Eric W. Biederman
2019-05-28 18:18   ` Thomas Gleixner
2019-05-23  0:39 ` [REVIEW][PATCH 12/26] signal/um: " Eric W. Biederman
2019-05-23  0:39 ` [REVIEW][PATCH 13/26] signal/sh: Remove tsk parameter from force_sig_info_fault Eric W. Biederman
2019-05-23  0:39 ` [REVIEW][PATCH 14/26] signal/riscv: Remove tsk parameter from do_trap Eric W. Biederman
2019-05-23  0:39 ` [REVIEW][PATCH 15/26] signal/nds32: Remove tsk parameter from send_sigtrap Eric W. Biederman
2019-05-23  0:39 ` [REVIEW][PATCH 16/26] signal/arm: Remove tsk parameter from ptrace_break Eric W. Biederman
2019-05-23  0:39 ` [REVIEW][PATCH 17/26] signal/arm: Remove tsk parameter from __do_user_fault Eric W. Biederman
2019-05-23  0:39 ` [REVIEW][PATCH 18/26] signal/unicore32: " Eric W. Biederman
2019-05-23  0:39 ` [REVIEW][PATCH 19/26] signal: Explicitly call force_sig_fault on current Eric W. Biederman
2019-05-23  0:39 ` [REVIEW][PATCH 20/26] signal: Use force_sig_fault_to_task for the two calls that don't deliver to current Eric W. Biederman
2019-05-23  0:39 ` [REVIEW][PATCH 21/26] signal: Remove the task parameter from force_sig_fault Eric W. Biederman
2019-05-23  0:39 ` [REVIEW][PATCH 22/26] signal: Properly set TRACE_SIGNAL_LOSE_INFO in __send_signal Eric W. Biederman
2019-05-23  0:39 ` [REVIEW][PATCH 23/26] signal: Move the computation of force into send_signal and correct it Eric W. Biederman
2019-05-23  0:39 ` [REVIEW][PATCH 24/26] signal: Generate the siginfo in force_sig Eric W. Biederman
2019-05-23  0:39 ` [REVIEW][PATCH 25/26] signal: Factor force_sig_info_to_task out of force_sig_info Eric W. Biederman
2019-05-23  0:39 ` [REVIEW][PATCH 26/26] signal: Remove the signal number and task parameters from force_sig_info Eric W. Biederman
2019-05-24 23:35 ` [REVIEW][PATCH 00/26] signal: Remove task argument " Eric W. Biederman
2019-05-29 15:37 ` Eric W. Biederman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87r28pgr3h.fsf@xmission.com \
    --to=ebiederm@xmission.com \
    --cc=Dave.Martin@arm.com \
    --cc=James.Morse@arm.com \
    --cc=Will.Deacon@arm.com \
    --cc=containers@lists.linux-foundation.org \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=oleg@redhat.com \
    --subject='Re: [REVIEW][PATCH 03/26] signal/arm64: Use force_sig not force_sig_fault for SIGKILL' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).