* siginfo pid not populated from ptrace? @ 2018-11-12 17:11 Tycho Andersen 2018-11-12 18:30 ` Eric W. Biederman 0 siblings, 1 reply; 24+ messages in thread From: Tycho Andersen @ 2018-11-12 17:11 UTC (permalink / raw) To: Kees Cook, Oleg Nesterov; +Cc: linux-kernel Hi Oleg, I've been running some tests on my seccomp series, and in one of the tests on v4.20-rc2, I noticed, [ RUN ] global.syscall_restart seccomp_bpf.c:2784:global.syscall_restart:Expected getpid() (1492) == info._sifields._kill.si_pid (0) global.syscall_restart: Test failed at step #22 which seems unrelated to my series (the kernel was stock v4.20 with my patches on top). I've been running a lot of tests, and only seen this once, so it seems like a fairly rare race. I tried to look through the code but didn't see anything obvious. Thoughts? Tycho ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: siginfo pid not populated from ptrace? 2018-11-12 17:11 siginfo pid not populated from ptrace? Tycho Andersen @ 2018-11-12 18:30 ` Eric W. Biederman 2018-11-12 18:55 ` Tycho Andersen 0 siblings, 1 reply; 24+ messages in thread From: Eric W. Biederman @ 2018-11-12 18:30 UTC (permalink / raw) To: Tycho Andersen; +Cc: Kees Cook, Oleg Nesterov, linux-kernel Tycho Andersen <tycho@tycho.ws> writes: > Hi Oleg, > > I've been running some tests on my seccomp series, and in one of the > tests on v4.20-rc2, I noticed, > > [ RUN ] global.syscall_restart > seccomp_bpf.c:2784:global.syscall_restart:Expected getpid() (1492) == info._sifields._kill.si_pid (0) > global.syscall_restart: Test failed at step #22 > > which seems unrelated to my series (the kernel was stock v4.20 with my > patches on top). > > I've been running a lot of tests, and only seen this once, so it seems > like a fairly rare race. I tried to look through the code but didn't > see anything obvious. Thoughts? My guess would be pid namespaces, or stopping for a signal other than SIGSTOP. If you can get this to reproduce at all it would be interesting to see si_signo and si_code. So that we can see just which signal is in info, and how it should be decoded. I see this test at line 2736 in 4.20-rc1 so there are almost 50 lines of change in your version of seccomp_bpf.c. So I hope I am reading the proper test. Eric ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: siginfo pid not populated from ptrace? 2018-11-12 18:30 ` Eric W. Biederman @ 2018-11-12 18:55 ` Tycho Andersen 2018-11-12 19:22 ` Eric W. Biederman 2018-11-12 19:24 ` Tycho Andersen 0 siblings, 2 replies; 24+ messages in thread From: Tycho Andersen @ 2018-11-12 18:55 UTC (permalink / raw) To: Eric W. Biederman; +Cc: Kees Cook, Oleg Nesterov, linux-kernel On Mon, Nov 12, 2018 at 12:30:25PM -0600, Eric W. Biederman wrote: > Tycho Andersen <tycho@tycho.ws> writes: > > > Hi Oleg, > > > > I've been running some tests on my seccomp series, and in one of the > > tests on v4.20-rc2, I noticed, > > > > [ RUN ] global.syscall_restart > > seccomp_bpf.c:2784:global.syscall_restart:Expected getpid() (1492) == info._sifields._kill.si_pid (0) > > global.syscall_restart: Test failed at step #22 > > > > which seems unrelated to my series (the kernel was stock v4.20 with my > > patches on top). > > > > I've been running a lot of tests, and only seen this once, so it seems > > like a fairly rare race. I tried to look through the code but didn't > > see anything obvious. Thoughts? > > My guess would be pid namespaces, or stopping for a signal other than > SIGSTOP. > > If you can get this to reproduce at all it would be interesting to see > si_signo and si_code. So that we can see just which signal is in info, > and how it should be decoded. Sure, here's what I see, seccomp_bpf.c:2784:global.syscall_restart:Expected getpid() (2195) == info._sifields._kill.si_pid (0) seccomp_bpf.c:2785:global.syscall_restart:si_signo: 19 seccomp_bpf.c:2786:global.syscall_restart:si_code: 0 > I see this test at line 2736 in 4.20-rc1 so there are almost 50 lines of > change in your version of seccomp_bpf.c. So I hope I am reading the > proper test. Yes, sorry, that's additional test stuff from my user trap series. I haven't manage to reproduce it on stock v4.20-rc2, unfortunately. It could be that this is some memory corruption introduced by my series, but I'm running these tests with KASAN so hopefully it would complain? Tycho ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: siginfo pid not populated from ptrace? 2018-11-12 18:55 ` Tycho Andersen @ 2018-11-12 19:22 ` Eric W. Biederman 2018-11-12 19:24 ` Tycho Andersen 1 sibling, 0 replies; 24+ messages in thread From: Eric W. Biederman @ 2018-11-12 19:22 UTC (permalink / raw) To: Tycho Andersen; +Cc: Kees Cook, Oleg Nesterov, linux-kernel Tycho Andersen <tycho@tycho.ws> writes: > On Mon, Nov 12, 2018 at 12:30:25PM -0600, Eric W. Biederman wrote: >> Tycho Andersen <tycho@tycho.ws> writes: >> >> > Hi Oleg, >> > >> > I've been running some tests on my seccomp series, and in one of the >> > tests on v4.20-rc2, I noticed, >> > >> > [ RUN ] global.syscall_restart >> > seccomp_bpf.c:2784:global.syscall_restart:Expected getpid() (1492) == info._sifields._kill.si_pid (0) >> > global.syscall_restart: Test failed at step #22 >> > >> > which seems unrelated to my series (the kernel was stock v4.20 with my >> > patches on top). >> > >> > I've been running a lot of tests, and only seen this once, so it seems >> > like a fairly rare race. I tried to look through the code but didn't >> > see anything obvious. Thoughts? >> >> My guess would be pid namespaces, or stopping for a signal other than >> SIGSTOP. >> >> If you can get this to reproduce at all it would be interesting to see >> si_signo and si_code. So that we can see just which signal is in info, >> and how it should be decoded. > > Sure, here's what I see, > > seccomp_bpf.c:2784:global.syscall_restart:Expected getpid() (2195) == info._sifields._kill.si_pid (0) > seccomp_bpf.c:2785:global.syscall_restart:si_signo: 19 > seccomp_bpf.c:2786:global.syscall_restart:si_code: 0 That is SI_USER and SIGSTOP. So as expected. >> I see this test at line 2736 in 4.20-rc1 so there are almost 50 lines of >> change in your version of seccomp_bpf.c. So I hope I am reading the >> proper test. > > Yes, sorry, that's additional test stuff from my user trap series. I > haven't manage to reproduce it on stock v4.20-rc2, unfortunately. It > could be that this is some memory corruption introduced by my series, > but I'm running these tests with KASAN so hopefully it would complain? I don't have a clue what KASAN would do. I think it is mostly concerned with unitialized memory, so this might be slipping through the cracks somewhere. It can be easy to mess up siginfo. That is part of the reason I just reworked things to use helpers most of the time when touching siginfo. Eric ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: siginfo pid not populated from ptrace? 2018-11-12 18:55 ` Tycho Andersen 2018-11-12 19:22 ` Eric W. Biederman @ 2018-11-12 19:24 ` Tycho Andersen 2018-11-27 23:21 ` Tycho Andersen 1 sibling, 1 reply; 24+ messages in thread From: Tycho Andersen @ 2018-11-12 19:24 UTC (permalink / raw) To: Eric W. Biederman; +Cc: Kees Cook, Oleg Nesterov, linux-kernel On Mon, Nov 12, 2018 at 11:55:38AM -0700, Tycho Andersen wrote: > I haven't manage to reproduce it on stock v4.20-rc2, unfortunately. Ok, now I have, seccomp_bpf.c:2736:global.syscall_restart:Expected getpid() (1493) == info._sifields._kill.si_pid (0) global.syscall_restart: Test failed at step #22 That line number should match up with v4.20-rc2, but it's in a different point in the test than the first failure I reported. Tycho ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: siginfo pid not populated from ptrace? 2018-11-12 19:24 ` Tycho Andersen @ 2018-11-27 23:21 ` Tycho Andersen 2018-11-28 0:38 ` Kees Cook 0 siblings, 1 reply; 24+ messages in thread From: Tycho Andersen @ 2018-11-27 23:21 UTC (permalink / raw) To: Eric W. Biederman; +Cc: Kees Cook, Oleg Nesterov, linux-kernel On Mon, Nov 12, 2018 at 12:24:43PM -0700, Tycho Andersen wrote: > On Mon, Nov 12, 2018 at 11:55:38AM -0700, Tycho Andersen wrote: > > I haven't manage to reproduce it on stock v4.20-rc2, unfortunately. > > Ok, now I have, > > seccomp_bpf.c:2736:global.syscall_restart:Expected getpid() (1493) == info._sifields._kill.si_pid (0) > global.syscall_restart: Test failed at step #22 Seems like this is still happening on v4.20-rc4, [ RUN ] global.syscall_restart seccomp_bpf.c:2736:global.syscall_restart:Expected getpid() (1901) == info._sifields._kill.si_pid (0) global.syscall_restart: Test failed at step #22 Tycho ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: siginfo pid not populated from ptrace? 2018-11-27 23:21 ` Tycho Andersen @ 2018-11-28 0:38 ` Kees Cook 2018-11-28 1:17 ` Kees Cook 0 siblings, 1 reply; 24+ messages in thread From: Kees Cook @ 2018-11-28 0:38 UTC (permalink / raw) To: Tycho Andersen; +Cc: Eric W. Biederman, Oleg Nesterov, LKML On Tue, Nov 27, 2018 at 3:21 PM, Tycho Andersen <tycho@tycho.ws> wrote: > On Mon, Nov 12, 2018 at 12:24:43PM -0700, Tycho Andersen wrote: >> On Mon, Nov 12, 2018 at 11:55:38AM -0700, Tycho Andersen wrote: >> > I haven't manage to reproduce it on stock v4.20-rc2, unfortunately. >> >> Ok, now I have, >> >> seccomp_bpf.c:2736:global.syscall_restart:Expected getpid() (1493) == info._sifields._kill.si_pid (0) >> global.syscall_restart: Test failed at step #22 > > Seems like this is still happening on v4.20-rc4, > > [ RUN ] global.syscall_restart > seccomp_bpf.c:2736:global.syscall_restart:Expected getpid() (1901) == info._sifields._kill.si_pid (0) > global.syscall_restart: Test failed at step #22 This fails every time for me -- is it still racey for you? I'm attempting a bisect, hoping it doesn't _become_ racey for me. ;) -- Kees Cook ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: siginfo pid not populated from ptrace? 2018-11-28 0:38 ` Kees Cook @ 2018-11-28 1:17 ` Kees Cook 2018-11-28 4:44 ` Eric W. Biederman 0 siblings, 1 reply; 24+ messages in thread From: Kees Cook @ 2018-11-28 1:17 UTC (permalink / raw) To: Tycho Andersen, Eric W. Biederman, Thomas Gleixner; +Cc: Oleg Nesterov, LKML On Tue, Nov 27, 2018 at 4:38 PM, Kees Cook <keescook@chromium.org> wrote: > On Tue, Nov 27, 2018 at 3:21 PM, Tycho Andersen <tycho@tycho.ws> wrote: >> On Mon, Nov 12, 2018 at 12:24:43PM -0700, Tycho Andersen wrote: >>> On Mon, Nov 12, 2018 at 11:55:38AM -0700, Tycho Andersen wrote: >>> > I haven't manage to reproduce it on stock v4.20-rc2, unfortunately. >>> >>> Ok, now I have, >>> >>> seccomp_bpf.c:2736:global.syscall_restart:Expected getpid() (1493) == info._sifields._kill.si_pid (0) >>> global.syscall_restart: Test failed at step #22 >> >> Seems like this is still happening on v4.20-rc4, >> >> [ RUN ] global.syscall_restart >> seccomp_bpf.c:2736:global.syscall_restart:Expected getpid() (1901) == info._sifields._kill.si_pid (0) >> global.syscall_restart: Test failed at step #22 > > This fails every time for me -- is it still racey for you? > > I'm attempting a bisect, hoping it doesn't _become_ racey for me. ;) This bisect to here for me: commit f149b31557446aff9ca96d4be7e39cc266f6e7cc Author: Eric W. Biederman <ebiederm@xmission.com> Date: Mon Sep 3 09:50:36 2018 +0200 signal: Never allocate siginfo for SIGKILL or SIGSTOP The SIGKILL and SIGSTOP signals are never delivered to userspace so queued siginfo for these signals can never be observed. Therefore remove the chance of failure by never even attempting to allocate siginfo in those cases. Reviewed-by: Thomas Gleixner <tglx@linutronix.de> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> They are certainly visible via seccomp ;) -- Kees Cook ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: siginfo pid not populated from ptrace? 2018-11-28 1:17 ` Kees Cook @ 2018-11-28 4:44 ` Eric W. Biederman 2018-11-29 21:17 ` Kees Cook 0 siblings, 1 reply; 24+ messages in thread From: Eric W. Biederman @ 2018-11-28 4:44 UTC (permalink / raw) To: Kees Cook; +Cc: Tycho Andersen, Thomas Gleixner, Oleg Nesterov, LKML Kees Cook <keescook@chromium.org> writes: > On Tue, Nov 27, 2018 at 4:38 PM, Kees Cook <keescook@chromium.org> wrote: >> On Tue, Nov 27, 2018 at 3:21 PM, Tycho Andersen <tycho@tycho.ws> wrote: >>> On Mon, Nov 12, 2018 at 12:24:43PM -0700, Tycho Andersen wrote: >>>> On Mon, Nov 12, 2018 at 11:55:38AM -0700, Tycho Andersen wrote: >>>> > I haven't manage to reproduce it on stock v4.20-rc2, unfortunately. >>>> >>>> Ok, now I have, >>>> >>>> seccomp_bpf.c:2736:global.syscall_restart:Expected getpid() (1493) == info._sifields._kill.si_pid (0) >>>> global.syscall_restart: Test failed at step #22 >>> >>> Seems like this is still happening on v4.20-rc4, >>> >>> [ RUN ] global.syscall_restart >>> seccomp_bpf.c:2736:global.syscall_restart:Expected getpid() (1901) == info._sifields._kill.si_pid (0) >>> global.syscall_restart: Test failed at step #22 >> >> This fails every time for me -- is it still racey for you? >> >> I'm attempting a bisect, hoping it doesn't _become_ racey for me. ;) > > This bisect to here for me: > > commit f149b31557446aff9ca96d4be7e39cc266f6e7cc > Author: Eric W. Biederman <ebiederm@xmission.com> > Date: Mon Sep 3 09:50:36 2018 +0200 > > signal: Never allocate siginfo for SIGKILL or SIGSTOP > > The SIGKILL and SIGSTOP signals are never delivered to userspace so > queued siginfo for these signals can never be observed. Therefore > remove the chance of failure by never even attempting to allocate > siginfo in those cases. > > Reviewed-by: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> > > They are certainly visible via seccomp ;) Well SIGSTOP is visible via PTRACE_GETSIGINFO. I see what is happening now. Since we don't have queued siginfo we generate some as: /* * Ok, it wasn't in the queue. This must be * a fast-pathed signal or we must have been * out of queue space. So zero out the info. */ clear_siginfo(info); info->si_signo = sig; info->si_errno = 0; info->si_code = SI_USER; info->si_pid = 0; info->si_uid = 0; Which allows last_signfo to be set, so despite not really having any siginfo PTRACE_GET_SIGINFO has something to return so does not return -EINVAL. Reconstructing my context that was part of removing SEND_SIG_FORCED so this looks like it will take a little more than a revert to fix this. This is definitely a change that is visible to user space. The logic in my patch was definitely wrong with respect to SIGSTOP and PTRACE_GETSIGINFO. Is there something in userspace that actually cares? AKA is the idiom that the test seccomp_bpf.c is using something that non-test code does? The change below should restore the old behavior. I am just wondering if this is something we want to do. siginfo is allocated with GFP_ATOMIC so if your maching is under memory pressure there is a real chance the allocation can fail. Which would cause whatever is breaking now to break less deterministically then. If we need to fix this do we need to make siginfo allocation more reliable? Eric diff --git a/kernel/signal.c b/kernel/signal.c index 4fd431ce4f91..5c34c55bfea4 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -1057,10 +1057,10 @@ static int __send_signal(int sig, struct kernel_siginfo *info, struct task_struc result = TRACE_SIGNAL_DELIVERED; /* - * Skip useless siginfo allocation for SIGKILL SIGSTOP, + * Skip useless siginfo allocation for SIGKILL, * and kernel threads. */ - if (sig_kernel_only(sig) || (t->flags & PF_KTHREAD)) + if ((sig == SIGKILL) || (t->flags & PF_KTHREAD)) goto out_set; /* ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: siginfo pid not populated from ptrace? 2018-11-28 4:44 ` Eric W. Biederman @ 2018-11-29 21:17 ` Kees Cook 2018-11-29 23:22 ` Tycho Andersen 2018-12-01 15:04 ` Eric W. Biederman 0 siblings, 2 replies; 24+ messages in thread From: Kees Cook @ 2018-11-29 21:17 UTC (permalink / raw) To: Eric W. Biederman; +Cc: Tycho Andersen, Thomas Gleixner, Oleg Nesterov, LKML On Tue, Nov 27, 2018 at 8:44 PM Eric W. Biederman <ebiederm@xmission.com> wrote: > > Kees Cook <keescook@chromium.org> writes: > > > On Tue, Nov 27, 2018 at 4:38 PM, Kees Cook <keescook@chromium.org> wrote: > >> On Tue, Nov 27, 2018 at 3:21 PM, Tycho Andersen <tycho@tycho.ws> wrote: > >>> On Mon, Nov 12, 2018 at 12:24:43PM -0700, Tycho Andersen wrote: > >>>> On Mon, Nov 12, 2018 at 11:55:38AM -0700, Tycho Andersen wrote: > >>>> > I haven't manage to reproduce it on stock v4.20-rc2, unfortunately. > >>>> > >>>> Ok, now I have, > >>>> > >>>> seccomp_bpf.c:2736:global.syscall_restart:Expected getpid() (1493) == info._sifields._kill.si_pid (0) > >>>> global.syscall_restart: Test failed at step #22 > >>> > >>> Seems like this is still happening on v4.20-rc4, > >>> > >>> [ RUN ] global.syscall_restart > >>> seccomp_bpf.c:2736:global.syscall_restart:Expected getpid() (1901) == info._sifields._kill.si_pid (0) > >>> global.syscall_restart: Test failed at step #22 > >> > >> This fails every time for me -- is it still racey for you? > >> > >> I'm attempting a bisect, hoping it doesn't _become_ racey for me. ;) > > > > This bisect to here for me: > > > > commit f149b31557446aff9ca96d4be7e39cc266f6e7cc > > Author: Eric W. Biederman <ebiederm@xmission.com> > > Date: Mon Sep 3 09:50:36 2018 +0200 > > > > signal: Never allocate siginfo for SIGKILL or SIGSTOP > > > > The SIGKILL and SIGSTOP signals are never delivered to userspace so > > queued siginfo for these signals can never be observed. Therefore > > remove the chance of failure by never even attempting to allocate > > siginfo in those cases. > > > > Reviewed-by: Thomas Gleixner <tglx@linutronix.de> > > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> > > > > They are certainly visible via seccomp ;) > > Well SIGSTOP is visible via PTRACE_GETSIGINFO. > > I see what is happening now. Since we don't have queued siginfo > we generate some as: > /* > * Ok, it wasn't in the queue. This must be > * a fast-pathed signal or we must have been > * out of queue space. So zero out the info. > */ > clear_siginfo(info); > info->si_signo = sig; > info->si_errno = 0; > info->si_code = SI_USER; > info->si_pid = 0; > info->si_uid = 0; > > Which allows last_signfo to be set, > so despite not really having any siginfo PTRACE_GET_SIGINFO > has something to return so does not return -EINVAL. > > Reconstructing my context that was part of removing SEND_SIG_FORCED > so this looks like it will take a little more than a revert to fix > this. > > This is definitely a change that is visible to user space. The logic in > my patch was definitely wrong with respect to SIGSTOP and > PTRACE_GETSIGINFO. Is there something in userspace that actually cares? > AKA is the idiom that the test seccomp_bpf.c is using something that > non-test code does? I think this would be needed by any ptracer that handled multiple threads. It needs to figure out which pid stopped. I think it's worth fixing, yes. > The change below should restore the old behavior. I am just wondering > if this is something we want to do. siginfo is allocated with > GFP_ATOMIC so if your machine is under memory pressure there is a real > chance the allocation can fail. Which would cause whatever is breaking > now to break less deterministically then. I think memory pressure that would block a 128 byte GFP_ATOMIC allocation would mean the system was about to seriously fall over. Given the user-facing behavior change and that an existing test was already checking for this means we need to fix it. > If we need to fix this do we need to make siginfo allocation more > reliable? I don't think so -- we'd already get a WARN() if allocation failed. > Eric > > > diff --git a/kernel/signal.c b/kernel/signal.c > index 4fd431ce4f91..5c34c55bfea4 100644 > --- a/kernel/signal.c > +++ b/kernel/signal.c > @@ -1057,10 +1057,10 @@ static int __send_signal(int sig, struct kernel_siginfo *info, struct task_struc > > result = TRACE_SIGNAL_DELIVERED; > /* > - * Skip useless siginfo allocation for SIGKILL SIGSTOP, > + * Skip useless siginfo allocation for SIGKILL, > * and kernel threads. > */ > - if (sig_kernel_only(sig) || (t->flags & PF_KTHREAD)) > + if ((sig == SIGKILL) || (t->flags & PF_KTHREAD)) > goto out_set; > > /* > This fixes it for me! Reported-by: Tycho Andersen <tycho@tycho.ws> Tested-by: Kees Cook <keescook@chromium.org> Fixes: f149b3155744 ("signal: Never allocate siginfo for SIGKILL or SIGSTOP") Thanks! -- Kees Cook ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: siginfo pid not populated from ptrace? 2018-11-29 21:17 ` Kees Cook @ 2018-11-29 23:22 ` Tycho Andersen 2018-12-01 15:04 ` Eric W. Biederman 1 sibling, 0 replies; 24+ messages in thread From: Tycho Andersen @ 2018-11-29 23:22 UTC (permalink / raw) To: Kees Cook; +Cc: Eric W. Biederman, Thomas Gleixner, Oleg Nesterov, LKML On Thu, Nov 29, 2018 at 01:17:01PM -0800, Kees Cook wrote: > On Tue, Nov 27, 2018 at 8:44 PM Eric W. Biederman <ebiederm@xmission.com> wrote: > > > > Kees Cook <keescook@chromium.org> writes: > > > > > On Tue, Nov 27, 2018 at 4:38 PM, Kees Cook <keescook@chromium.org> wrote: > > >> On Tue, Nov 27, 2018 at 3:21 PM, Tycho Andersen <tycho@tycho.ws> wrote: > > >>> On Mon, Nov 12, 2018 at 12:24:43PM -0700, Tycho Andersen wrote: > > >>>> On Mon, Nov 12, 2018 at 11:55:38AM -0700, Tycho Andersen wrote: > > >>>> > I haven't manage to reproduce it on stock v4.20-rc2, unfortunately. > > >>>> > > >>>> Ok, now I have, > > >>>> > > >>>> seccomp_bpf.c:2736:global.syscall_restart:Expected getpid() (1493) == info._sifields._kill.si_pid (0) > > >>>> global.syscall_restart: Test failed at step #22 > > >>> > > >>> Seems like this is still happening on v4.20-rc4, > > >>> > > >>> [ RUN ] global.syscall_restart > > >>> seccomp_bpf.c:2736:global.syscall_restart:Expected getpid() (1901) == info._sifields._kill.si_pid (0) > > >>> global.syscall_restart: Test failed at step #22 > > >> > > >> This fails every time for me -- is it still racey for you? > > >> > > >> I'm attempting a bisect, hoping it doesn't _become_ racey for me. ;) > > > > > > This bisect to here for me: > > > > > > commit f149b31557446aff9ca96d4be7e39cc266f6e7cc > > > Author: Eric W. Biederman <ebiederm@xmission.com> > > > Date: Mon Sep 3 09:50:36 2018 +0200 > > > > > > signal: Never allocate siginfo for SIGKILL or SIGSTOP > > > > > > The SIGKILL and SIGSTOP signals are never delivered to userspace so > > > queued siginfo for these signals can never be observed. Therefore > > > remove the chance of failure by never even attempting to allocate > > > siginfo in those cases. > > > > > > Reviewed-by: Thomas Gleixner <tglx@linutronix.de> > > > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> > > > > > > They are certainly visible via seccomp ;) > > > > Well SIGSTOP is visible via PTRACE_GETSIGINFO. > > > > I see what is happening now. Since we don't have queued siginfo > > we generate some as: > > /* > > * Ok, it wasn't in the queue. This must be > > * a fast-pathed signal or we must have been > > * out of queue space. So zero out the info. > > */ > > clear_siginfo(info); > > info->si_signo = sig; > > info->si_errno = 0; > > info->si_code = SI_USER; > > info->si_pid = 0; > > info->si_uid = 0; > > > > Which allows last_signfo to be set, > > so despite not really having any siginfo PTRACE_GET_SIGINFO > > has something to return so does not return -EINVAL. > > > > Reconstructing my context that was part of removing SEND_SIG_FORCED > > so this looks like it will take a little more than a revert to fix > > this. > > > > This is definitely a change that is visible to user space. The logic in > > my patch was definitely wrong with respect to SIGSTOP and > > PTRACE_GETSIGINFO. Is there something in userspace that actually cares? > > AKA is the idiom that the test seccomp_bpf.c is using something that > > non-test code does? > > I think this would be needed by any ptracer that handled multiple > threads. It needs to figure out which pid stopped. I think it's worth > fixing, yes. > > > The change below should restore the old behavior. I am just wondering > > if this is something we want to do. siginfo is allocated with > > GFP_ATOMIC so if your machine is under memory pressure there is a real > > chance the allocation can fail. Which would cause whatever is breaking > > now to break less deterministically then. > > I think memory pressure that would block a 128 byte GFP_ATOMIC > allocation would mean the system was about to seriously fall over. > Given the user-facing behavior change and that an existing test was > already checking for this means we need to fix it. > > > If we need to fix this do we need to make siginfo allocation more > > reliable? > > I don't think so -- we'd already get a WARN() if allocation failed. > > > Eric > > > > > > diff --git a/kernel/signal.c b/kernel/signal.c > > index 4fd431ce4f91..5c34c55bfea4 100644 > > --- a/kernel/signal.c > > +++ b/kernel/signal.c > > @@ -1057,10 +1057,10 @@ static int __send_signal(int sig, struct kernel_siginfo *info, struct task_struc > > > > result = TRACE_SIGNAL_DELIVERED; > > /* > > - * Skip useless siginfo allocation for SIGKILL SIGSTOP, > > + * Skip useless siginfo allocation for SIGKILL, > > * and kernel threads. > > */ > > - if (sig_kernel_only(sig) || (t->flags & PF_KTHREAD)) > > + if ((sig == SIGKILL) || (t->flags & PF_KTHREAD)) > > goto out_set; > > > > /* > > > > This fixes it for me! > > Reported-by: Tycho Andersen <tycho@tycho.ws> > Tested-by: Kees Cook <keescook@chromium.org> > Fixes: f149b3155744 ("signal: Never allocate siginfo for SIGKILL or SIGSTOP") Thanks guys, it works for me too. Tested-by: Tycho Andersen <tycho@tycho.ws> Tycho ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: siginfo pid not populated from ptrace? 2018-11-29 21:17 ` Kees Cook 2018-11-29 23:22 ` Tycho Andersen @ 2018-12-01 15:04 ` Eric W. Biederman 2018-12-06 1:00 ` Kees Cook 1 sibling, 1 reply; 24+ messages in thread From: Eric W. Biederman @ 2018-12-01 15:04 UTC (permalink / raw) To: Kees Cook; +Cc: Tycho Andersen, Thomas Gleixner, Oleg Nesterov, LKML Kees Cook <keescook@chromium.org> writes: > On Tue, Nov 27, 2018 at 8:44 PM Eric W. Biederman <ebiederm@xmission.com> wrote: >> >> Kees Cook <keescook@chromium.org> writes: >> >> > On Tue, Nov 27, 2018 at 4:38 PM, Kees Cook <keescook@chromium.org> wrote: >> >> On Tue, Nov 27, 2018 at 3:21 PM, Tycho Andersen <tycho@tycho.ws> wrote: >> >>> On Mon, Nov 12, 2018 at 12:24:43PM -0700, Tycho Andersen wrote: >> >>>> On Mon, Nov 12, 2018 at 11:55:38AM -0700, Tycho Andersen wrote: >> >>>> > I haven't manage to reproduce it on stock v4.20-rc2, unfortunately. >> >>>> >> >>>> Ok, now I have, >> >>>> >> >>>> seccomp_bpf.c:2736:global.syscall_restart:Expected getpid() (1493) == info._sifields._kill.si_pid (0) >> >>>> global.syscall_restart: Test failed at step #22 >> >>> >> >>> Seems like this is still happening on v4.20-rc4, >> >>> >> >>> [ RUN ] global.syscall_restart >> >>> seccomp_bpf.c:2736:global.syscall_restart:Expected getpid() (1901) == info._sifields._kill.si_pid (0) >> >>> global.syscall_restart: Test failed at step #22 >> >> >> >> This fails every time for me -- is it still racey for you? >> >> >> >> I'm attempting a bisect, hoping it doesn't _become_ racey for me. ;) >> > >> > This bisect to here for me: >> > >> > commit f149b31557446aff9ca96d4be7e39cc266f6e7cc >> > Author: Eric W. Biederman <ebiederm@xmission.com> >> > Date: Mon Sep 3 09:50:36 2018 +0200 >> > >> > signal: Never allocate siginfo for SIGKILL or SIGSTOP >> > >> > The SIGKILL and SIGSTOP signals are never delivered to userspace so >> > queued siginfo for these signals can never be observed. Therefore >> > remove the chance of failure by never even attempting to allocate >> > siginfo in those cases. >> > >> > Reviewed-by: Thomas Gleixner <tglx@linutronix.de> >> > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> >> > >> > They are certainly visible via seccomp ;) >> >> Well SIGSTOP is visible via PTRACE_GETSIGINFO. >> >> I see what is happening now. Since we don't have queued siginfo >> we generate some as: >> /* >> * Ok, it wasn't in the queue. This must be >> * a fast-pathed signal or we must have been >> * out of queue space. So zero out the info. >> */ >> clear_siginfo(info); >> info->si_signo = sig; >> info->si_errno = 0; >> info->si_code = SI_USER; >> info->si_pid = 0; >> info->si_uid = 0; >> >> Which allows last_signfo to be set, >> so despite not really having any siginfo PTRACE_GET_SIGINFO >> has something to return so does not return -EINVAL. >> >> Reconstructing my context that was part of removing SEND_SIG_FORCED >> so this looks like it will take a little more than a revert to fix >> this. >> >> This is definitely a change that is visible to user space. The logic in >> my patch was definitely wrong with respect to SIGSTOP and >> PTRACE_GETSIGINFO. Is there something in userspace that actually cares? >> AKA is the idiom that the test seccomp_bpf.c is using something that >> non-test code does? > > I think this would be needed by any ptracer that handled multiple > threads. It needs to figure out which pid stopped. I think it's worth > fixing, yes. > >> The change below should restore the old behavior. I am just wondering >> if this is something we want to do. siginfo is allocated with >> GFP_ATOMIC so if your machine is under memory pressure there is a real >> chance the allocation can fail. Which would cause whatever is breaking >> now to break less deterministically then. > > I think memory pressure that would block a 128 byte GFP_ATOMIC > allocation would mean the system was about to seriously fall over. > Given the user-facing behavior change and that an existing test was > already checking for this means we need to fix it. That sounds good but it is all rubbish. A) It doesn't matter for tracing multiple processes because ptrace only works on a single signal at a time. AKA if by the time you are calling PTRACE_GETSIGINFO you already know which process you are working against. B) Not every signal includes si_pid so even if you didn't know who you were talking to this would be an issue. C) For a non-rt signal we only every try and queue up a signal signal. We don't even attempt to queue siginfo otherwise. So what is the real world use case? The most useful I can think of is the whole check if this tracer was the one who sent SIGSTOP. But that is fundamentally unreliable because we only queue a single signal anyway. We must to that to preserve compatibility for non-rt signals as otherwise there are cases we could queue the same signal twice. So with two simultaneous SIGSTOPs it is a race condition which siginfo is made available. Right now we are on the edge of letting test cases for debug infrastructure prevent obvious optimizations/improvements to the code. So if we can find something other than our seccomp_bpf.c test case that fails I am happy to go with the change I posted. Otherwise I think we just need to fix seccomp_bpf.c. Eric ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: siginfo pid not populated from ptrace? 2018-12-01 15:04 ` Eric W. Biederman @ 2018-12-06 1:00 ` Kees Cook 2018-12-06 14:40 ` Eric W. Biederman 0 siblings, 1 reply; 24+ messages in thread From: Kees Cook @ 2018-12-06 1:00 UTC (permalink / raw) To: Eric W. Biederman, Linus Torvalds Cc: Tycho Andersen, Thomas Gleixner, Oleg Nesterov, LKML On Sat, Dec 1, 2018 at 7:04 AM Eric W. Biederman <ebiederm@xmission.com> wrote: > > Kees Cook <keescook@chromium.org> writes: > > > On Tue, Nov 27, 2018 at 8:44 PM Eric W. Biederman <ebiederm@xmission.com> wrote: > >> > >> Kees Cook <keescook@chromium.org> writes: > >> > >> > On Tue, Nov 27, 2018 at 4:38 PM, Kees Cook <keescook@chromium.org> wrote: > >> >> On Tue, Nov 27, 2018 at 3:21 PM, Tycho Andersen <tycho@tycho.ws> wrote: > >> >>> On Mon, Nov 12, 2018 at 12:24:43PM -0700, Tycho Andersen wrote: > >> >>>> On Mon, Nov 12, 2018 at 11:55:38AM -0700, Tycho Andersen wrote: > >> >>>> > I haven't manage to reproduce it on stock v4.20-rc2, unfortunately. > >> >>>> > >> >>>> Ok, now I have, > >> >>>> > >> >>>> seccomp_bpf.c:2736:global.syscall_restart:Expected getpid() (1493) == info._sifields._kill.si_pid (0) > >> >>>> global.syscall_restart: Test failed at step #22 > >> >>> > >> >>> Seems like this is still happening on v4.20-rc4, > >> >>> > >> >>> [ RUN ] global.syscall_restart > >> >>> seccomp_bpf.c:2736:global.syscall_restart:Expected getpid() (1901) == info._sifields._kill.si_pid (0) > >> >>> global.syscall_restart: Test failed at step #22 > >> >> > >> >> This fails every time for me -- is it still racey for you? > >> >> > >> >> I'm attempting a bisect, hoping it doesn't _become_ racey for me. ;) > >> > > >> > This bisects to here for me: > >> > > >> > commit f149b31557446aff9ca96d4be7e39cc266f6e7cc > >> > Author: Eric W. Biederman <ebiederm@xmission.com> > >> > Date: Mon Sep 3 09:50:36 2018 +0200 > >> > > >> > signal: Never allocate siginfo for SIGKILL or SIGSTOP > >> > > >> > The SIGKILL and SIGSTOP signals are never delivered to userspace so > >> > queued siginfo for these signals can never be observed. Therefore > >> > remove the chance of failure by never even attempting to allocate > >> > siginfo in those cases. > >> > > >> > Reviewed-by: Thomas Gleixner <tglx@linutronix.de> > >> > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> > >> > > >> > They are certainly visible via seccomp ;) > >> > >> Well SIGSTOP is visible via PTRACE_GETSIGINFO. > >> > >> I see what is happening now. Since we don't have queued siginfo > >> we generate some as: > >> /* > >> * Ok, it wasn't in the queue. This must be > >> * a fast-pathed signal or we must have been > >> * out of queue space. So zero out the info. > >> */ > >> clear_siginfo(info); > >> info->si_signo = sig; > >> info->si_errno = 0; > >> info->si_code = SI_USER; > >> info->si_pid = 0; > >> info->si_uid = 0; > >> > >> Which allows last_signfo to be set, > >> so despite not really having any siginfo PTRACE_GET_SIGINFO > >> has something to return so does not return -EINVAL. > >> > >> Reconstructing my context that was part of removing SEND_SIG_FORCED > >> so this looks like it will take a little more than a revert to fix > >> this. > >> > >> This is definitely a change that is visible to user space. The logic in > >> my patch was definitely wrong with respect to SIGSTOP and > >> PTRACE_GETSIGINFO. Is there something in userspace that actually cares? > >> AKA is the idiom that the test seccomp_bpf.c is using something that > >> non-test code does? > > > > I think this would be needed by any ptracer that handled multiple > > threads. It needs to figure out which pid stopped. I think it's worth > > fixing, yes. > > > >> The change below should restore the old behavior. I am just wondering > >> if this is something we want to do. siginfo is allocated with > >> GFP_ATOMIC so if your machine is under memory pressure there is a real > >> chance the allocation can fail. Which would cause whatever is breaking > >> now to break less deterministically then. > > > > I think memory pressure that would block a 128 byte GFP_ATOMIC > > allocation would mean the system was about to seriously fall over. > > Given the user-facing behavior change and that an existing test was > > already checking for this means we need to fix it. > > That sounds good but it is all rubbish. > A) It doesn't matter for tracing multiple processes because ptrace > only works on a single signal at a time. AKA if by the time you > are calling PTRACE_GETSIGINFO you already know which process you are > working against. > B) Not every signal includes si_pid so even if you didn't know who you > were talking to this would be an issue. > C) For a non-rt signal we only every try and queue up a signal signal. > We don't even attempt to queue siginfo otherwise. Fair enough, all true. > So what is the real world use case? A quick search didn't show anything I could find (looking for combinations of PTRACE_GETSIGINFO and si_pid use around a SIGSTOP). That doesn't mean it doesn't exist, though. The reason seccomp's selftests are so extensive is because we've had some very corner-case[1] bugs in real-world software. > The most useful I can think of is the whole check if this tracer was the > one who sent SIGSTOP. But that is fundamentally unreliable because we > only queue a single signal anyway. We must to that to preserve > compatibility for non-rt signals as otherwise there are cases we could > queue the same signal twice. So with two simultaneous SIGSTOPs it is a > race condition which siginfo is made available. Understood. However, the point of the seccomp test is for making sure that the ordering of the controlled test gets us the right pattern of events across a syscall interruption when seccomp is filtering. This is about making sure nothing around the restart changes since this could impact seccomp filters in the wild. > Right now we are on the edge of letting test cases for debug > infrastructure prevent obvious optimizations/improvements to the code. > So if we can find something other than our seccomp_bpf.c test case that > fails I am happy to go with the change I posted. Otherwise I think we > just need to fix seccomp_bpf.c. I wouldn't call this "debug infrastructure": they are behavioral test cases for seccomp, syscall, and ptrace interaction, and even this one has found bugs (see the "FIXME" note a few lines down, that remains unsolved[2] in arm32). If this actually doesn't count as breaking userspace, then okay, sure, I'll drop the sanity check in the seccomp selftest. But it feels wrong to me. -Kees [1] 485a252a5559 ("seccomp: Fix tracer exit notifications during fatal signals") [2] http://lkml.kernel.org/r/CAGXu5jKzif=vp6gn5ZtrTx-JTN367qFphobnt9s=awbaafwoUw@mail.gmail.com -- Kees Cook ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: siginfo pid not populated from ptrace? 2018-12-06 1:00 ` Kees Cook @ 2018-12-06 14:40 ` Eric W. Biederman 2018-12-06 18:48 ` Linus Torvalds 0 siblings, 1 reply; 24+ messages in thread From: Eric W. Biederman @ 2018-12-06 14:40 UTC (permalink / raw) To: Kees Cook Cc: Linus Torvalds, Tycho Andersen, Thomas Gleixner, Oleg Nesterov, LKML Kees Cook <keescook@chromium.org> writes: > On Sat, Dec 1, 2018 at 7:04 AM Eric W. Biederman <ebiederm@xmission.com> wrote: >> >> Kees Cook <keescook@chromium.org> writes: >> >> > On Tue, Nov 27, 2018 at 8:44 PM Eric W. Biederman <ebiederm@xmission.com> wrote: >> >> >> >> Kees Cook <keescook@chromium.org> writes: >> >> >> >> > On Tue, Nov 27, 2018 at 4:38 PM, Kees Cook <keescook@chromium.org> wrote: >> >> >> On Tue, Nov 27, 2018 at 3:21 PM, Tycho Andersen <tycho@tycho.ws> wrote: >> >> >>> On Mon, Nov 12, 2018 at 12:24:43PM -0700, Tycho Andersen wrote: >> >> >>>> On Mon, Nov 12, 2018 at 11:55:38AM -0700, Tycho Andersen wrote: >> >> >>>> > I haven't manage to reproduce it on stock v4.20-rc2, unfortunately. >> >> >>>> >> >> >>>> Ok, now I have, >> >> >>>> >> >> >>>> seccomp_bpf.c:2736:global.syscall_restart:Expected getpid() (1493) == info._sifields._kill.si_pid (0) >> >> >>>> global.syscall_restart: Test failed at step #22 >> >> >>> >> >> >>> Seems like this is still happening on v4.20-rc4, >> >> >>> >> >> >>> [ RUN ] global.syscall_restart >> >> >>> seccomp_bpf.c:2736:global.syscall_restart:Expected getpid() (1901) == info._sifields._kill.si_pid (0) >> >> >>> global.syscall_restart: Test failed at step #22 >> >> >> >> >> >> This fails every time for me -- is it still racey for you? >> >> >> >> >> >> I'm attempting a bisect, hoping it doesn't _become_ racey for me. ;) >> >> > >> >> > This bisects to here for me: >> >> > >> >> > commit f149b31557446aff9ca96d4be7e39cc266f6e7cc >> >> > Author: Eric W. Biederman <ebiederm@xmission.com> >> >> > Date: Mon Sep 3 09:50:36 2018 +0200 >> >> > >> >> > signal: Never allocate siginfo for SIGKILL or SIGSTOP >> >> > >> >> > The SIGKILL and SIGSTOP signals are never delivered to userspace so >> >> > queued siginfo for these signals can never be observed. Therefore >> >> > remove the chance of failure by never even attempting to allocate >> >> > siginfo in those cases. >> >> > >> >> > Reviewed-by: Thomas Gleixner <tglx@linutronix.de> >> >> > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> >> >> > >> >> > They are certainly visible via seccomp ;) >> >> >> >> Well SIGSTOP is visible via PTRACE_GETSIGINFO. >> >> >> >> I see what is happening now. Since we don't have queued siginfo >> >> we generate some as: >> >> /* >> >> * Ok, it wasn't in the queue. This must be >> >> * a fast-pathed signal or we must have been >> >> * out of queue space. So zero out the info. >> >> */ >> >> clear_siginfo(info); >> >> info->si_signo = sig; >> >> info->si_errno = 0; >> >> info->si_code = SI_USER; >> >> info->si_pid = 0; >> >> info->si_uid = 0; >> >> >> >> Which allows last_signfo to be set, >> >> so despite not really having any siginfo PTRACE_GET_SIGINFO >> >> has something to return so does not return -EINVAL. >> >> >> >> Reconstructing my context that was part of removing SEND_SIG_FORCED >> >> so this looks like it will take a little more than a revert to fix >> >> this. >> >> >> >> This is definitely a change that is visible to user space. The logic in >> >> my patch was definitely wrong with respect to SIGSTOP and >> >> PTRACE_GETSIGINFO. Is there something in userspace that actually cares? >> >> AKA is the idiom that the test seccomp_bpf.c is using something that >> >> non-test code does? >> > >> > I think this would be needed by any ptracer that handled multiple >> > threads. It needs to figure out which pid stopped. I think it's worth >> > fixing, yes. >> > >> >> The change below should restore the old behavior. I am just wondering >> >> if this is something we want to do. siginfo is allocated with >> >> GFP_ATOMIC so if your machine is under memory pressure there is a real >> >> chance the allocation can fail. Which would cause whatever is breaking >> >> now to break less deterministically then. >> > >> > I think memory pressure that would block a 128 byte GFP_ATOMIC >> > allocation would mean the system was about to seriously fall over. >> > Given the user-facing behavior change and that an existing test was >> > already checking for this means we need to fix it. >> >> That sounds good but it is all rubbish. >> A) It doesn't matter for tracing multiple processes because ptrace >> only works on a single signal at a time. AKA if by the time you >> are calling PTRACE_GETSIGINFO you already know which process you are >> working against. >> B) Not every signal includes si_pid so even if you didn't know who you >> were talking to this would be an issue. >> C) For a non-rt signal we only every try and queue up a signal signal. >> We don't even attempt to queue siginfo otherwise. > > Fair enough, all true. > >> So what is the real world use case? > > A quick search didn't show anything I could find (looking for > combinations of PTRACE_GETSIGINFO and si_pid use around a SIGSTOP). > That doesn't mean it doesn't exist, though. The reason seccomp's > selftests are so extensive is because we've had some very > corner-case[1] bugs in real-world software. I support that. >> The most useful I can think of is the whole check if this tracer was the >> one who sent SIGSTOP. But that is fundamentally unreliable because we >> only queue a single signal anyway. We must to that to preserve >> compatibility for non-rt signals as otherwise there are cases we could >> queue the same signal twice. So with two simultaneous SIGSTOPs it is a >> race condition which siginfo is made available. > > Understood. However, the point of the seccomp test is for making sure > that the ordering of the controlled test gets us the right pattern of > events across a syscall interruption when seccomp is filtering. This > is about making sure nothing around the restart changes since this > could impact seccomp filters in the wild. That is fair. Does the test fundamentally need to check the sender of SIGSTOP to know everything you are sending is working for seccomp? >> Right now we are on the edge of letting test cases for debug >> infrastructure prevent obvious optimizations/improvements to the code. >> So if we can find something other than our seccomp_bpf.c test case that >> fails I am happy to go with the change I posted. Otherwise I think we >> just need to fix seccomp_bpf.c. > > I wouldn't call this "debug infrastructure": they are behavioral test > cases for seccomp, syscall, and ptrace interaction, and even this one > has found bugs (see the "FIXME" note a few lines down, that remains > unsolved[2] in arm32). I would call ptrace "debug infrastructure". My concern is that there were a number of signal senders that were setting SEND_SIG_FORCED. They were setting SEND_SIG_FORCED to cause the signal delivery process to work differently. Ensuring the pid namespace init was killed, to not allocate siginfo, etc. This was very fragile code internal to the kernel and required much to much knowledge from the signal senders of the signals. So I updated the code to apply the transformations whenever reasonably possible so senders of those signals would not need to think about it. Reducing bugs overall and making the kernel more consistent. The logic was that the siginfo from SIGSTOP and SIGKILL could not be observed. Except for PTRACE_GETSIGINFO for SIGSTOP is actually true. > If this actually doesn't count as breaking userspace, then okay, sure, > I'll drop the sanity check in the seccomp selftest. But it feels wrong > to me. With just a test showing this, this doesn't yet count as breaking userspace. At the same time this is on a funny edge. If something that is not a test truly depends on this it would definitely count as breaking userspace. Equally if there is a case where it can be shown that userspace can reasonably depend on this behavior to achieve something userspace needs to do. Then sure. We have in the past had ptrace users that weren't just about debugging so I don't know that it is fair to just dismiss it as debugging infrastructure. Right now I am trying to understand why seccomp bpf cares. So that I can figure out if I need to do something. The optimization of not allocating siginfo for SIGSTOP is very reasonable except when the process is being ptraced. It even has the possibility of making SIGSTOP working better for the common case. So I am reluctant to give up a kernel improvement until I understand what reasonable case in userspace I am breaking. What I am seeing is a case where for a multi-threaded process the ptracer has to win the multi-threaded signal lottery to see the siginfo. Further no one else but the ptracer can send SIGSTOP in the same window and generate an alternate SIGSTOP. So my best analysis is that except under very controlled circumstances si_pid for SIGSTOP will be unreliable. Except in a test I don't know how you could guarantee those controlled circumstances. But please if you can see a scenario that I don't please let me know. Eric ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: siginfo pid not populated from ptrace? 2018-12-06 14:40 ` Eric W. Biederman @ 2018-12-06 18:48 ` Linus Torvalds 2018-12-06 19:20 ` Tycho Andersen 0 siblings, 1 reply; 24+ messages in thread From: Linus Torvalds @ 2018-12-06 18:48 UTC (permalink / raw) To: Eric W. Biederman Cc: Kees Cook, Tycho Andersen, Thomas Gleixner, Oleg Nesterov, Linux List Kernel Mailing On Thu, Dec 6, 2018 at 6:40 AM Eric W. Biederman <ebiederm@xmission.com> wrote: > > We have in the past had ptrace users that weren't just about debugging > so I don't know that it is fair to just dismiss it as debugging > infrastructure. Absolutely. Some uses are more than just debug. People occasionally use ptrace because it's the only way to do what they want, so you'll find people who do it for sandboxing, for example. It's not necessarily designed for that, or particularly fast or well-suited for it, but I've definitely seen it used that way. So I don't think the behavioral test breakage like this is necessarily a huge deal, and until some "real use" actually shows that it cares it might be something we dismiss as "just test", but it very much has the potential to hit real uses. The fact that a behavioral test broke is definitely interesting. And maybe some of the siginfo allocations could depend on whether the signal is actually ever caught or not. For example, a terminal signal (or one that is ignored) might not need siginfo. But if the process is ptraced, maybe that terminal signal isn't actually terminal? So we might have situations where we want to simply check "is the signal target being ptraced".. Linus ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: siginfo pid not populated from ptrace? 2018-12-06 18:48 ` Linus Torvalds @ 2018-12-06 19:20 ` Tycho Andersen 2018-12-06 21:11 ` Eric W. Biederman 2018-12-10 14:57 ` Oleg Nesterov 0 siblings, 2 replies; 24+ messages in thread From: Tycho Andersen @ 2018-12-06 19:20 UTC (permalink / raw) To: Linus Torvalds Cc: Eric W. Biederman, Kees Cook, Thomas Gleixner, Oleg Nesterov, Linux List Kernel Mailing On Thu, Dec 06, 2018 at 10:48:39AM -0800, Linus Torvalds wrote: > On Thu, Dec 6, 2018 at 6:40 AM Eric W. Biederman <ebiederm@xmission.com> wrote: > > > > We have in the past had ptrace users that weren't just about debugging > > so I don't know that it is fair to just dismiss it as debugging > > infrastructure. > > Absolutely. > > Some uses are more than just debug. People occasionally use ptrace > because it's the only way to do what they want, so you'll find people > who do it for sandboxing, for example. It's not necessarily designed > for that, or particularly fast or well-suited for it, but I've > definitely seen it used that way. > > So I don't think the behavioral test breakage like this is necessarily > a huge deal, and until some "real use" actually shows that it cares it > might be something we dismiss as "just test", but it very much has the > potential to hit real uses. > > The fact that a behavioral test broke is definitely interesting. > > And maybe some of the siginfo allocations could depend on whether the > signal is actually ever caught or not. > > For example, a terminal signal (or one that is ignored) might not need > siginfo. But if the process is ptraced, maybe that terminal signal > isn't actually terminal? So we might have situations where we want to > simply check "is the signal target being ptraced".. Yes, something like this, I suppose? It works for me. Tycho From 3bcaadd56ebb532ab4d481556fcc0826d65efc43 Mon Sep 17 00:00:00 2001 From: Tycho Andersen <tycho@tycho.ws> Date: Thu, 6 Dec 2018 12:15:22 -0700 Subject: [PATCH] signal: allocate siginfo when a traced task gets SIGSTOP Tracers can view SIGSTOP: https://lore.kernel.org/lkml/87zhtthkuy.fsf@xmission.com/T/#u so let's allocate a siginfo for SIGSTOP when a task is traced. Signed-off-by: Tycho Andersen <tycho@tycho.ws> --- kernel/signal.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/kernel/signal.c b/kernel/signal.c index 9a32bc2088c9..ab4ba00119f4 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -1056,11 +1056,14 @@ static int __send_signal(int sig, struct kernel_siginfo *info, struct task_struc goto ret; result = TRACE_SIGNAL_DELIVERED; + /* - * Skip useless siginfo allocation for SIGKILL SIGSTOP, - * and kernel threads. + * Skip useless siginfo allocation for SIGKILL and kernel threads. + * SIGSTOP is visible to tracers, so only skip allocation when the task + * is not traced. */ - if (sig_kernel_only(sig) || (t->flags & PF_KTHREAD)) + if ((sig == SIGKILL) || (!task_is_traced(t) && sig == SIGSTOP) || + (t->flags & PF_KTHREAD)) goto out_set; /* -- 2.19.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: siginfo pid not populated from ptrace? 2018-12-06 19:20 ` Tycho Andersen @ 2018-12-06 21:11 ` Eric W. Biederman 2018-12-06 21:34 ` Kees Cook 2018-12-10 15:37 ` Oleg Nesterov 2018-12-10 14:57 ` Oleg Nesterov 1 sibling, 2 replies; 24+ messages in thread From: Eric W. Biederman @ 2018-12-06 21:11 UTC (permalink / raw) To: Tycho Andersen Cc: Linus Torvalds, Kees Cook, Thomas Gleixner, Oleg Nesterov, Linux List Kernel Mailing Tycho Andersen <tycho@tycho.ws> writes: > On Thu, Dec 06, 2018 at 10:48:39AM -0800, Linus Torvalds wrote: >> On Thu, Dec 6, 2018 at 6:40 AM Eric W. Biederman <ebiederm@xmission.com> wrote: >> > >> > We have in the past had ptrace users that weren't just about debugging >> > so I don't know that it is fair to just dismiss it as debugging >> > infrastructure. >> >> Absolutely. >> >> Some uses are more than just debug. People occasionally use ptrace >> because it's the only way to do what they want, so you'll find people >> who do it for sandboxing, for example. It's not necessarily designed >> for that, or particularly fast or well-suited for it, but I've >> definitely seen it used that way. >> >> So I don't think the behavioral test breakage like this is necessarily >> a huge deal, and until some "real use" actually shows that it cares it >> might be something we dismiss as "just test", but it very much has the >> potential to hit real uses. >> >> The fact that a behavioral test broke is definitely interesting. >> >> And maybe some of the siginfo allocations could depend on whether the >> signal is actually ever caught or not. >> >> For example, a terminal signal (or one that is ignored) might not need >> siginfo. But if the process is ptraced, maybe that terminal signal >> isn't actually terminal? So we might have situations where we want to >> simply check "is the signal target being ptraced".. > > Yes, something like this, I suppose? It works for me. The challenge is that we could be delivering this to a zombie signal group leader. At which point we won't deliver it to the target task. Sigh it is probably time that I dig in and figure out how to avoid that case which we need to fix anyway because we can get the permission checks wrong for multi-threaded processes that call setuid and friends. Once that is sorted your small change will at least be safe. Eric > From 3bcaadd56ebb532ab4d481556fcc0826d65efc43 Mon Sep 17 00:00:00 2001 > From: Tycho Andersen <tycho@tycho.ws> > Date: Thu, 6 Dec 2018 12:15:22 -0700 > Subject: [PATCH] signal: allocate siginfo when a traced task gets SIGSTOP > > Tracers can view SIGSTOP: > > https://lore.kernel.org/lkml/87zhtthkuy.fsf@xmission.com/T/#u > > so let's allocate a siginfo for SIGSTOP when a task is traced. > > Signed-off-by: Tycho Andersen <tycho@tycho.ws> > --- > kernel/signal.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/kernel/signal.c b/kernel/signal.c > index 9a32bc2088c9..ab4ba00119f4 100644 > --- a/kernel/signal.c > +++ b/kernel/signal.c > @@ -1056,11 +1056,14 @@ static int __send_signal(int sig, struct kernel_siginfo *info, struct task_struc > goto ret; > > result = TRACE_SIGNAL_DELIVERED; > + > /* > - * Skip useless siginfo allocation for SIGKILL SIGSTOP, > - * and kernel threads. > + * Skip useless siginfo allocation for SIGKILL and kernel threads. > + * SIGSTOP is visible to tracers, so only skip allocation when the task > + * is not traced. > */ > - if (sig_kernel_only(sig) || (t->flags & PF_KTHREAD)) > + if ((sig == SIGKILL) || (!task_is_traced(t) && sig == SIGSTOP) || > + (t->flags & PF_KTHREAD)) > goto out_set; > > /* ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: siginfo pid not populated from ptrace? 2018-12-06 21:11 ` Eric W. Biederman @ 2018-12-06 21:34 ` Kees Cook 2018-12-06 22:43 ` Eric W. Biederman 2018-12-10 15:37 ` Oleg Nesterov 1 sibling, 1 reply; 24+ messages in thread From: Kees Cook @ 2018-12-06 21:34 UTC (permalink / raw) To: Eric W. Biederman Cc: Tycho Andersen, Linus Torvalds, Thomas Gleixner, Oleg Nesterov, LKML On Thu, Dec 6, 2018 at 1:11 PM Eric W. Biederman <ebiederm@xmission.com> wrote: > > Tycho Andersen <tycho@tycho.ws> writes: > > > On Thu, Dec 06, 2018 at 10:48:39AM -0800, Linus Torvalds wrote: > >> On Thu, Dec 6, 2018 at 6:40 AM Eric W. Biederman <ebiederm@xmission.com> wrote: > >> > > >> > We have in the past had ptrace users that weren't just about debugging > >> > so I don't know that it is fair to just dismiss it as debugging > >> > infrastructure. > >> > >> Absolutely. > >> > >> Some uses are more than just debug. People occasionally use ptrace > >> because it's the only way to do what they want, so you'll find people > >> who do it for sandboxing, for example. It's not necessarily designed > >> for that, or particularly fast or well-suited for it, but I've > >> definitely seen it used that way. > >> > >> So I don't think the behavioral test breakage like this is necessarily > >> a huge deal, and until some "real use" actually shows that it cares it > >> might be something we dismiss as "just test", but it very much has the > >> potential to hit real uses. > >> > >> The fact that a behavioral test broke is definitely interesting. > >> > >> And maybe some of the siginfo allocations could depend on whether the > >> signal is actually ever caught or not. > >> > >> For example, a terminal signal (or one that is ignored) might not need > >> siginfo. But if the process is ptraced, maybe that terminal signal > >> isn't actually terminal? So we might have situations where we want to > >> simply check "is the signal target being ptraced".. > > > > Yes, something like this, I suppose? It works for me. > > The challenge is that we could be delivering this to a zombie signal > group leader. At which point we won't deliver it to the target task. > > Sigh it is probably time that I dig in and figure out how to avoid that > case which we need to fix anyway because we can get the permission > checks wrong for multi-threaded processes that call setuid and friends. > > Once that is sorted your small change will at least be safe. > > Eric > > > From 3bcaadd56ebb532ab4d481556fcc0826d65efc43 Mon Sep 17 00:00:00 2001 > > From: Tycho Andersen <tycho@tycho.ws> > > Date: Thu, 6 Dec 2018 12:15:22 -0700 > > Subject: [PATCH] signal: allocate siginfo when a traced task gets SIGSTOP > > > > Tracers can view SIGSTOP: > > > > https://lore.kernel.org/lkml/87zhtthkuy.fsf@xmission.com/T/#u > > > > so let's allocate a siginfo for SIGSTOP when a task is traced. > > > > Signed-off-by: Tycho Andersen <tycho@tycho.ws> > > --- > > kernel/signal.c | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/kernel/signal.c b/kernel/signal.c > > index 9a32bc2088c9..ab4ba00119f4 100644 > > --- a/kernel/signal.c > > +++ b/kernel/signal.c > > @@ -1056,11 +1056,14 @@ static int __send_signal(int sig, struct kernel_siginfo *info, struct task_struc > > goto ret; > > > > result = TRACE_SIGNAL_DELIVERED; > > + > > /* > > - * Skip useless siginfo allocation for SIGKILL SIGSTOP, > > - * and kernel threads. > > + * Skip useless siginfo allocation for SIGKILL and kernel threads. > > + * SIGSTOP is visible to tracers, so only skip allocation when the task > > + * is not traced. > > */ > > - if (sig_kernel_only(sig) || (t->flags & PF_KTHREAD)) > > + if ((sig == SIGKILL) || (!task_is_traced(t) && sig == SIGSTOP) || > > + (t->flags & PF_KTHREAD)) > > goto out_set; > > > > /* What should we do for v4.20? I need to have the selftests actually passing. :) -- Kees Cook ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: siginfo pid not populated from ptrace? 2018-12-06 21:34 ` Kees Cook @ 2018-12-06 22:43 ` Eric W. Biederman 2018-12-06 22:55 ` Kees Cook 0 siblings, 1 reply; 24+ messages in thread From: Eric W. Biederman @ 2018-12-06 22:43 UTC (permalink / raw) To: Kees Cook Cc: Tycho Andersen, Linus Torvalds, Thomas Gleixner, Oleg Nesterov, LKML Kees Cook <keescook@chromium.org> writes: > On Thu, Dec 6, 2018 at 1:11 PM Eric W. Biederman <ebiederm@xmission.com> wrote: >> >> Tycho Andersen <tycho@tycho.ws> writes: >> >> > On Thu, Dec 06, 2018 at 10:48:39AM -0800, Linus Torvalds wrote: >> >> On Thu, Dec 6, 2018 at 6:40 AM Eric W. Biederman <ebiederm@xmission.com> wrote: >> >> > >> >> > We have in the past had ptrace users that weren't just about debugging >> >> > so I don't know that it is fair to just dismiss it as debugging >> >> > infrastructure. >> >> >> >> Absolutely. >> >> >> >> Some uses are more than just debug. People occasionally use ptrace >> >> because it's the only way to do what they want, so you'll find people >> >> who do it for sandboxing, for example. It's not necessarily designed >> >> for that, or particularly fast or well-suited for it, but I've >> >> definitely seen it used that way. >> >> >> >> So I don't think the behavioral test breakage like this is necessarily >> >> a huge deal, and until some "real use" actually shows that it cares it >> >> might be something we dismiss as "just test", but it very much has the >> >> potential to hit real uses. >> >> >> >> The fact that a behavioral test broke is definitely interesting. >> >> >> >> And maybe some of the siginfo allocations could depend on whether the >> >> signal is actually ever caught or not. >> >> >> >> For example, a terminal signal (or one that is ignored) might not need >> >> siginfo. But if the process is ptraced, maybe that terminal signal >> >> isn't actually terminal? So we might have situations where we want to >> >> simply check "is the signal target being ptraced".. >> > >> > Yes, something like this, I suppose? It works for me. >> >> The challenge is that we could be delivering this to a zombie signal >> group leader. At which point we won't deliver it to the target task. >> >> Sigh it is probably time that I dig in and figure out how to avoid that >> case which we need to fix anyway because we can get the permission >> checks wrong for multi-threaded processes that call setuid and friends. >> >> Once that is sorted your small change will at least be safe. >> >> Eric >> >> > From 3bcaadd56ebb532ab4d481556fcc0826d65efc43 Mon Sep 17 00:00:00 2001 >> > From: Tycho Andersen <tycho@tycho.ws> >> > Date: Thu, 6 Dec 2018 12:15:22 -0700 >> > Subject: [PATCH] signal: allocate siginfo when a traced task gets SIGSTOP >> > >> > Tracers can view SIGSTOP: >> > >> > https://lore.kernel.org/lkml/87zhtthkuy.fsf@xmission.com/T/#u >> > >> > so let's allocate a siginfo for SIGSTOP when a task is traced. >> > >> > Signed-off-by: Tycho Andersen <tycho@tycho.ws> >> > --- >> > kernel/signal.c | 9 ++++++--- >> > 1 file changed, 6 insertions(+), 3 deletions(-) >> > >> > diff --git a/kernel/signal.c b/kernel/signal.c >> > index 9a32bc2088c9..ab4ba00119f4 100644 >> > --- a/kernel/signal.c >> > +++ b/kernel/signal.c >> > @@ -1056,11 +1056,14 @@ static int __send_signal(int sig, struct kernel_siginfo *info, struct task_struc >> > goto ret; >> > >> > result = TRACE_SIGNAL_DELIVERED; >> > + >> > /* >> > - * Skip useless siginfo allocation for SIGKILL SIGSTOP, >> > - * and kernel threads. >> > + * Skip useless siginfo allocation for SIGKILL and kernel threads. >> > + * SIGSTOP is visible to tracers, so only skip allocation when the task >> > + * is not traced. >> > */ >> > - if (sig_kernel_only(sig) || (t->flags & PF_KTHREAD)) >> > + if ((sig == SIGKILL) || (!task_is_traced(t) && sig == SIGSTOP) || >> > + (t->flags & PF_KTHREAD)) >> > goto out_set; >> > >> > /* > > What should we do for v4.20? I need to have the selftests actually > passing. :) For v4.20 we need to do one of two things. 1) Present a plausible case that someone will could care about, we document it in the commit we can perform my earlier partial revert. 2) Remove the sanity check seccomp_bpf.c I really just want to ensure we have clear reasoning here. Eric ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: siginfo pid not populated from ptrace? 2018-12-06 22:43 ` Eric W. Biederman @ 2018-12-06 22:55 ` Kees Cook 0 siblings, 0 replies; 24+ messages in thread From: Kees Cook @ 2018-12-06 22:55 UTC (permalink / raw) To: Eric W. Biederman Cc: Tycho Andersen, Linus Torvalds, Thomas Gleixner, Oleg Nesterov, LKML On Thu, Dec 6, 2018 at 2:43 PM Eric W. Biederman <ebiederm@xmission.com> wrote: > > Kees Cook <keescook@chromium.org> writes: > > What should we do for v4.20? I need to have the selftests actually > > passing. :) > > For v4.20 we need to do one of two things. > 1) Present a plausible case that someone will could care about, > we document it in the commit we can perform my earlier partial revert. If SIGSTOP si_pid can't be used to determine who sent the signal reliably even before, then I'm guessing we'll never see a real-world case where this matters. > 2) Remove the sanity check seccomp_bpf.c > > I really just want to ensure we have clear reasoning here. I'll remove it for now and add a link to this conversation, in case anyone else goes looking. -Kees -- Kees Cook ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: siginfo pid not populated from ptrace? 2018-12-06 21:11 ` Eric W. Biederman 2018-12-06 21:34 ` Kees Cook @ 2018-12-10 15:37 ` Oleg Nesterov 2018-12-10 15:44 ` Tycho Andersen 2018-12-10 17:36 ` Eric W. Biederman 1 sibling, 2 replies; 24+ messages in thread From: Oleg Nesterov @ 2018-12-10 15:37 UTC (permalink / raw) To: Eric W. Biederman Cc: Tycho Andersen, Linus Torvalds, Kees Cook, Thomas Gleixner, Linux List Kernel Mailing On 12/06, Eric W. Biederman wrote: > > The challenge is that we could be delivering this to a zombie signal > group leader. ... > Sigh it is probably time that I dig in and figure out how to avoid that > case which we need to fix anyway because we can get the permission > checks wrong for multi-threaded processes that call setuid and friends. this is another issue... I am sure we have already discussed this, but I failed to find any link to the previous discussion. > Once that is sorted your small change will at least be safe. I don't think so, any sub-thread can dequeue SIGSTOP unless type == PIDTYPE_PID, this has nothing to do with the problems connected to zombie leader, or I misunderstood you. Oleg. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: siginfo pid not populated from ptrace? 2018-12-10 15:37 ` Oleg Nesterov @ 2018-12-10 15:44 ` Tycho Andersen 2018-12-10 17:36 ` Eric W. Biederman 1 sibling, 0 replies; 24+ messages in thread From: Tycho Andersen @ 2018-12-10 15:44 UTC (permalink / raw) To: Oleg Nesterov Cc: Eric W. Biederman, Linus Torvalds, Kees Cook, Thomas Gleixner, Linux List Kernel Mailing Hi Oleg, On Mon, Dec 10, 2018 at 04:37:18PM +0100, Oleg Nesterov wrote: > On 12/06, Eric W. Biederman wrote: > > > > The challenge is that we could be delivering this to a zombie signal > > group leader. > > ... > > > Sigh it is probably time that I dig in and figure out how to avoid that > > case which we need to fix anyway because we can get the permission > > checks wrong for multi-threaded processes that call setuid and friends. > > this is another issue... I am sure we have already discussed this, but I > failed to find any link to the previous discussion. > > > Once that is sorted your small change will at least be safe. > > I don't think so, any sub-thread can dequeue SIGSTOP unless type == PIDTYPE_PID, > this has nothing to do with the problems connected to zombie leader, or I > misunderstood you. I think the conclusion about this bug is that we're just not going to fix it, Kees sent a patch to remove the check from the test: https://lore.kernel.org/lkml/20181206235038.GA18273@beast/T/#u So I will drop my patch. Cheers, Tycho ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: siginfo pid not populated from ptrace? 2018-12-10 15:37 ` Oleg Nesterov 2018-12-10 15:44 ` Tycho Andersen @ 2018-12-10 17:36 ` Eric W. Biederman 1 sibling, 0 replies; 24+ messages in thread From: Eric W. Biederman @ 2018-12-10 17:36 UTC (permalink / raw) To: Oleg Nesterov Cc: Tycho Andersen, Linus Torvalds, Kees Cook, Thomas Gleixner, Linux List Kernel Mailing Oleg Nesterov <oleg@redhat.com> writes: > On 12/06, Eric W. Biederman wrote: >> >> The challenge is that we could be delivering this to a zombie signal >> group leader. > > ... > >> Sigh it is probably time that I dig in and figure out how to avoid that >> case which we need to fix anyway because we can get the permission >> checks wrong for multi-threaded processes that call setuid and friends. > > this is another issue... I am sure we have already discussed this, but I > failed to find any link to the previous discussion. Now that we have PIDTYPE_TGID I think we are closer to being able to solve that issue. You are absolutely right it is another issue. >> Once that is sorted your small change will at least be safe. > > I don't think so, any sub-thread can dequeue SIGSTOP unless type == PIDTYPE_PID, > this has nothing to do with the problems connected to zombie leader, or I > misunderstood you. I forgot to check what wants_signal does in this case. I thought SIGSTOP was like SIGKILL and being unblockable would always be delivered to the thread we are aiming at. With a zombie leader being the exception. Having reread wants_signal you are absolutely correct. SIGSTOP can be delivered to any thread so this won't help. I don't understand why for SIGSTOP we don't treat SIGSTOP like SIGKILL, but that is also another conversation. It feels like the differences between SIGSTOP and SIGKILL in wants_signal are silly. I don't see them leading to incorrect behavior. Eric ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: siginfo pid not populated from ptrace? 2018-12-06 19:20 ` Tycho Andersen 2018-12-06 21:11 ` Eric W. Biederman @ 2018-12-10 14:57 ` Oleg Nesterov 1 sibling, 0 replies; 24+ messages in thread From: Oleg Nesterov @ 2018-12-10 14:57 UTC (permalink / raw) To: Tycho Andersen Cc: Linus Torvalds, Eric W. Biederman, Kees Cook, Thomas Gleixner, Linux List Kernel Mailing On 12/06, Tycho Andersen wrote: > > --- a/kernel/signal.c > +++ b/kernel/signal.c > @@ -1056,11 +1056,14 @@ static int __send_signal(int sig, struct kernel_siginfo *info, struct task_struc > goto ret; > > result = TRACE_SIGNAL_DELIVERED; > + > /* > - * Skip useless siginfo allocation for SIGKILL SIGSTOP, > - * and kernel threads. > + * Skip useless siginfo allocation for SIGKILL and kernel threads. > + * SIGSTOP is visible to tracers, so only skip allocation when the task > + * is not traced. > */ > - if (sig_kernel_only(sig) || (t->flags & PF_KTHREAD)) > + if ((sig == SIGKILL) || (!task_is_traced(t) && sig == SIGSTOP) || ^^^^^^^^^^^^^^ task_is_traced() checks task->state, probably you meant t->ptrace != 0. However, in multithreaded case t->ptrace won't help too, unless the signal is private you do not know which thread will actually dequeue this signal and possibly report to debugger. Oleg. ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2018-12-10 17:37 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-12 17:11 siginfo pid not populated from ptrace? Tycho Andersen 2018-11-12 18:30 ` Eric W. Biederman 2018-11-12 18:55 ` Tycho Andersen 2018-11-12 19:22 ` Eric W. Biederman 2018-11-12 19:24 ` Tycho Andersen 2018-11-27 23:21 ` Tycho Andersen 2018-11-28 0:38 ` Kees Cook 2018-11-28 1:17 ` Kees Cook 2018-11-28 4:44 ` Eric W. Biederman 2018-11-29 21:17 ` Kees Cook 2018-11-29 23:22 ` Tycho Andersen 2018-12-01 15:04 ` Eric W. Biederman 2018-12-06 1:00 ` Kees Cook 2018-12-06 14:40 ` Eric W. Biederman 2018-12-06 18:48 ` Linus Torvalds 2018-12-06 19:20 ` Tycho Andersen 2018-12-06 21:11 ` Eric W. Biederman 2018-12-06 21:34 ` Kees Cook 2018-12-06 22:43 ` Eric W. Biederman 2018-12-06 22:55 ` Kees Cook 2018-12-10 15:37 ` Oleg Nesterov 2018-12-10 15:44 ` Tycho Andersen 2018-12-10 17:36 ` Eric W. Biederman 2018-12-10 14:57 ` Oleg Nesterov
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).