| Message ID | 1330140111-17201-8-git-send-email-wad@chromium.org |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On 02/24, Will Drewry wrote: > > To ensure that SIGSYS delivery occurs on return from the triggering > system call, SIGSYS is added to the SYNCHRONOUS_MASK macro. Hmm. Can't understand... please help. > #define SYNCHRONOUS_MASK \ > (sigmask(SIGSEGV) | sigmask(SIGBUS) | sigmask(SIGILL) | \ > - sigmask(SIGTRAP) | sigmask(SIGFPE)) > + sigmask(SIGTRAP) | sigmask(SIGFPE) | sigmask(SIGSYS)) Why? SYNCHRONOUS_MASK just tells dequeue_signal() "pick them first". This is needed to make sure that the handler for, say SIGSEGV, can use ucontext->ip as a faulting addr. But seccomp adds info->si_call_addr, this looks unneeded. Oleg. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 27, 2012 at 9:22 AM, Oleg Nesterov <oleg@redhat.com> wrote: > SYNCHRONOUS_MASK just tells dequeue_signal() "pick them first". > This is needed to make sure that the handler for, say SIGSEGV, > can use ucontext->ip as a faulting addr. It's desireable to have these signals handled first just so that the thread state that provoked the signal is not obscured by an unrelated asynchronous signal having its handler setup done beforehand. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/27, Roland McGrath wrote: > > On Mon, Feb 27, 2012 at 9:22 AM, Oleg Nesterov <oleg@redhat.com> wrote: > > SYNCHRONOUS_MASK just tells dequeue_signal() "pick them first". > > This is needed to make sure that the handler for, say SIGSEGV, > > can use ucontext->ip as a faulting addr. > > It's desireable to have these signals handled first just so that the thread > state that provoked the signal is not obscured by an unrelated asynchronous > signal having its handler setup done beforehand. OK, then probably it makes sense to update the changelog, "To ensure that SIGSYS delivery occurs on return from the triggering system call" looks confusing imho. Not that I really understand why "setup_rt_frame() first" really matters in this case, siginfo should carry all necessary info. IOW, may be "run this handler first" makes more sense but this change makes the opposite. OK, I won't argue, just I was confused by the changelog. Oleg. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 27, 2012 at 11:22 AM, Oleg Nesterov <oleg@redhat.com> wrote: > On 02/24, Will Drewry wrote: >> >> To ensure that SIGSYS delivery occurs on return from the triggering >> system call, SIGSYS is added to the SYNCHRONOUS_MASK macro. > > Hmm. Can't understand... please help. > >> #define SYNCHRONOUS_MASK \ >> (sigmask(SIGSEGV) | sigmask(SIGBUS) | sigmask(SIGILL) | \ >> - sigmask(SIGTRAP) | sigmask(SIGFPE)) >> + sigmask(SIGTRAP) | sigmask(SIGFPE) | sigmask(SIGSYS)) > > Why? > > SYNCHRONOUS_MASK just tells dequeue_signal() "pick them first". > This is needed to make sure that the handler for, say SIGSEGV, > can use ucontext->ip as a faulting addr. I think that Roland covered this. (Since the syscall_rollback was called it's nice to let our handler get first go.) > But seccomp adds info->si_call_addr, this looks unneeded. True enough. I can drop it. It'd only be useful if the SIGSYS wasn't being forced and the signal was being handled without ucontext_t access. thanks! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/27, Will Drewry wrote: > > On Mon, Feb 27, 2012 at 11:22 AM, Oleg Nesterov <oleg@redhat.com> wrote: > > On 02/24, Will Drewry wrote: > >> > >> To ensure that SIGSYS delivery occurs on return from the triggering > >> system call, SIGSYS is added to the SYNCHRONOUS_MASK macro. > > > > Hmm. Can't understand... please help. > > > >> #define SYNCHRONOUS_MASK \ > >> (sigmask(SIGSEGV) | sigmask(SIGBUS) | sigmask(SIGILL) | \ > >> - sigmask(SIGTRAP) | sigmask(SIGFPE)) > >> + sigmask(SIGTRAP) | sigmask(SIGFPE) | sigmask(SIGSYS)) > > > > Why? > > > > SYNCHRONOUS_MASK just tells dequeue_signal() "pick them first". > > This is needed to make sure that the handler for, say SIGSEGV, > > can use ucontext->ip as a faulting addr. > > I think that Roland covered this. (Since the syscall_rollback was > called it's nice to let our handler get first go.) OK, except I do not really understand the "our handler get first go". Suppose SIGSYS "races" with the pending SIGHUP. With this change the caller for SIGHUP will be called first. But yes, setup_frame() will be called for SIGSYS first. Hopefully this is what you want. > > But seccomp adds info->si_call_addr, this looks unneeded. > > True enough. I can drop it. Hmm. I meant, the change in SYNCHRONOUS_MASK looks unneeded. Please keep ->si_call_addr, it is much more convenient than ucontext_t in userspace. > It'd only be useful if the SIGSYS wasn't > being forced and the signal was being handled without ucontext_t > access. No, force_ doesn't make any difference in this sense... In short, the patch looks fine to me, but if you resend it may be you can update the changelog. Oleg. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 28, 2012 at 10:02 AM, Oleg Nesterov <oleg@redhat.com> wrote: > On 02/27, Will Drewry wrote: >> >> On Mon, Feb 27, 2012 at 11:22 AM, Oleg Nesterov <oleg@redhat.com> wrote: >> > On 02/24, Will Drewry wrote: >> >> >> >> To ensure that SIGSYS delivery occurs on return from the triggering >> >> system call, SIGSYS is added to the SYNCHRONOUS_MASK macro. >> > >> > Hmm. Can't understand... please help. >> > >> >> #define SYNCHRONOUS_MASK \ >> >> (sigmask(SIGSEGV) | sigmask(SIGBUS) | sigmask(SIGILL) | \ >> >> - sigmask(SIGTRAP) | sigmask(SIGFPE)) >> >> + sigmask(SIGTRAP) | sigmask(SIGFPE) | sigmask(SIGSYS)) >> > >> > Why? >> > >> > SYNCHRONOUS_MASK just tells dequeue_signal() "pick them first". >> > This is needed to make sure that the handler for, say SIGSEGV, >> > can use ucontext->ip as a faulting addr. >> >> I think that Roland covered this. (Since the syscall_rollback was >> called it's nice to let our handler get first go.) > > OK, except I do not really understand the "our handler get first go". Err I meant "gets to go first". > Suppose SIGSYS "races" with the pending SIGHUP. With this change > the caller for SIGHUP will be called first. But yes, setup_frame() > will be called for SIGSYS first. Hopefully this is what you want. I believe it is. I just want ucontext_t to be properly populate since the registers were just rolled back for it. >> > But seccomp adds info->si_call_addr, this looks unneeded. >> >> True enough. I can drop it. > > Hmm. I meant, the change in SYNCHRONOUS_MASK looks unneeded. Please > keep ->si_call_addr, it is much more convenient than ucontext_t in > userspace. Sorry for the confusion >> It'd only be useful if the SIGSYS wasn't >> being forced and the signal was being handled without ucontext_t >> access. > > No, force_ doesn't make any difference in this sense... I guess I was thinking about users of signalfd wanting the call site but force_ avoids it being blockable so that seems largely irrelevant at present. > In short, the patch looks fine to me, but if you resend it may be > you can update the changelog. Thanks! I will try to clarify the changelog. will -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c index 6557769..c81d2c7 100644 --- a/arch/x86/ia32/ia32_signal.c +++ b/arch/x86/ia32/ia32_signal.c @@ -73,6 +73,10 @@ int copy_siginfo_to_user32(compat_siginfo_t __user *to, siginfo_t *from) switch (from->si_code >> 16) { case __SI_FAULT >> 16: break; + case __SI_SYS >> 16: + put_user_ex(from->si_syscall, &to->si_syscall); + put_user_ex(from->si_arch, &to->si_arch); + break; case __SI_CHLD >> 16: put_user_ex(from->si_utime, &to->si_utime); put_user_ex(from->si_stime, &to->si_stime); diff --git a/arch/x86/include/asm/ia32.h b/arch/x86/include/asm/ia32.h index 1f7e625..541485f 100644 --- a/arch/x86/include/asm/ia32.h +++ b/arch/x86/include/asm/ia32.h @@ -126,6 +126,12 @@ typedef struct compat_siginfo { int _band; /* POLL_IN, POLL_OUT, POLL_MSG */ int _fd; } _sigpoll; + + struct { + unsigned int _call_addr; /* calling insn */ + int _syscall; /* triggering system call number */ + unsigned int _arch; /* AUDIT_ARCH_* of syscall */ + } _sigsys; } _sifields; } compat_siginfo_t; diff --git a/include/asm-generic/siginfo.h b/include/asm-generic/siginfo.h index 0dd4e87..31306f5 100644 --- a/include/asm-generic/siginfo.h +++ b/include/asm-generic/siginfo.h @@ -90,9 +90,18 @@ typedef struct siginfo { __ARCH_SI_BAND_T _band; /* POLL_IN, POLL_OUT, POLL_MSG */ int _fd; } _sigpoll; + + /* SIGSYS */ + struct { + void __user *_call_addr; /* calling insn */ + int _syscall; /* triggering system call number */ + unsigned int _arch; /* AUDIT_ARCH_* of syscall */ + } _sigsys; } _sifields; } siginfo_t; +/* If the arch shares siginfo, then it has SIGSYS. */ +#define __ARCH_SIGSYS #endif /* @@ -116,6 +125,11 @@ typedef struct siginfo { #define si_addr_lsb _sifields._sigfault._addr_lsb #define si_band _sifields._sigpoll._band #define si_fd _sifields._sigpoll._fd +#ifdef __ARCH_SIGSYS +#define si_call_addr _sifields._sigsys._call_addr +#define si_syscall _sifields._sigsys._syscall +#define si_arch _sifields._sigsys._arch +#endif #ifdef __KERNEL__ #define __SI_MASK 0xffff0000u @@ -126,6 +140,7 @@ typedef struct siginfo { #define __SI_CHLD (4 << 16) #define __SI_RT (5 << 16) #define __SI_MESGQ (6 << 16) +#define __SI_SYS (7 << 16) #define __SI_CODE(T,N) ((T) | ((N) & 0xffff)) #else #define __SI_KILL 0 @@ -135,6 +150,7 @@ typedef struct siginfo { #define __SI_CHLD 0 #define __SI_RT 0 #define __SI_MESGQ 0 +#define __SI_SYS 0 #define __SI_CODE(T,N) (N) #endif @@ -232,6 +248,12 @@ typedef struct siginfo { #define NSIGPOLL 6 /* + * SIGSYS si_codes + */ +#define SYS_SECCOMP (__SI_SYS|1) /* seccomp triggered */ +#define NSIGSYS 1 + +/* * sigevent definitions * * It seems likely that SIGEV_THREAD will have to be handled from diff --git a/kernel/signal.c b/kernel/signal.c index c73c428..15a9e9b 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -160,7 +160,7 @@ void recalc_sigpending(void) #define SYNCHRONOUS_MASK \ (sigmask(SIGSEGV) | sigmask(SIGBUS) | sigmask(SIGILL) | \ - sigmask(SIGTRAP) | sigmask(SIGFPE)) + sigmask(SIGTRAP) | sigmask(SIGFPE) | sigmask(SIGSYS)) int next_signal(struct sigpending *pending, sigset_t *mask) { @@ -2686,6 +2686,13 @@ int copy_siginfo_to_user(siginfo_t __user *to, siginfo_t *from) err |= __put_user(from->si_uid, &to->si_uid); err |= __put_user(from->si_ptr, &to->si_ptr); break; +#ifdef __ARCH_SIGSYS + case __SI_SYS: + err |= __put_user(from->si_call_addr, &to->si_call_addr); + err |= __put_user(from->si_syscall, &to->si_syscall); + err |= __put_user(from->si_arch, &to->si_arch); + break; +#endif default: /* this is just in case for now ... */ err |= __put_user(from->si_pid, &to->si_pid); err |= __put_user(from->si_uid, &to->si_uid);
This change enables SIGSYS, defines _sigfields._sigsys, and adds x86 (compat) arch support. _sigsys defines fields which allow a signal handler to receive the triggering system call number, the relevant AUDIT_ARCH_* value for that number, and the address of the callsite. To ensure that SIGSYS delivery occurs on return from the triggering system call, SIGSYS is added to the SYNCHRONOUS_MASK macro. The first consumer of SIGSYS would be seccomp filter. In particular, a filter program could specify a new return value, SECCOMP_RET_TRAP, which would result in the system call being denied and the calling thread signaled. This also means that implementing arch-specific support can be dependent upon HAVE_ARCH_SECCOMP_FILTER. v11: - fix dropped words in the change description - added fallback copy_siginfo support. - added __ARCH_SIGSYS define to allow stepped arch support. v10: - first version based on suggestion Suggested-by: H. Peter Anvin <hpa@zytor.com> Signed-off-by: Will Drewry <wad@chromium.org> --- arch/x86/ia32/ia32_signal.c | 4 ++++ arch/x86/include/asm/ia32.h | 6 ++++++ include/asm-generic/siginfo.h | 22 ++++++++++++++++++++++ kernel/signal.c | 9 ++++++++- 4 files changed, 40 insertions(+), 1 deletions(-)