| Message ID | 20190605155801.GA25165@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | signal: simplify set_user_sigmask/restore_user_sigmask | expand |
On Wed, Jun 5, 2019 at 8:58 AM Oleg Nesterov <oleg@redhat.com> wrote: > > To simplify the review, please see the code with this patch applied. > I am using epoll_pwait() as an example because it looks very simple. I like it. However. I think I'd like it even more if we just said "we don't need restore_saved_sigmask AT ALL". Which would be fairly easy to do with something like the attached... (Yes, this only does x86, which is a problem, but I'm bringing this up as a RFC..) Is it worth another TIF flag? This sure would simplify things, and it really fits the concept too: this really is a do_signal() issue, and fundamentally goes together with TIF_SIGPENDING. Linus arch/x86/entry/common.c | 2 +- arch/x86/include/asm/thread_info.h | 2 ++ kernel/signal.c | 1 + 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c index a986b3c8294c..eb538ecd6603 100644 --- a/arch/x86/entry/common.c +++ b/arch/x86/entry/common.c @@ -160,7 +160,7 @@ static void exit_to_usermode_loop(struct pt_regs *regs, u32 cached_flags) klp_update_patch_state(current); /* deal with pending signal delivery */ - if (cached_flags & _TIF_SIGPENDING) + if (cached_flags & (_TIF_SIGPENDING | _TIF_RESTORE_SIGMASK)) do_signal(regs); if (cached_flags & _TIF_NOTIFY_RESUME) { diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h index f9453536f9bb..d77a9f841455 100644 --- a/arch/x86/include/asm/thread_info.h +++ b/arch/x86/include/asm/thread_info.h @@ -92,6 +92,7 @@ struct thread_info { #define TIF_NOCPUID 15 /* CPUID is not accessible in userland */ #define TIF_NOTSC 16 /* TSC is not accessible in userland */ #define TIF_IA32 17 /* IA32 compatibility process */ +#define TIF_RESTORE_SIGMASK 18 /* Restore saved sigmask on return to user space */ #define TIF_NOHZ 19 /* in adaptive nohz mode */ #define TIF_MEMDIE 20 /* is terminating due to OOM killer */ #define TIF_POLLING_NRFLAG 21 /* idle is polling for TIF_NEED_RESCHED */ @@ -122,6 +123,7 @@ struct thread_info { #define _TIF_NOCPUID (1 << TIF_NOCPUID) #define _TIF_NOTSC (1 << TIF_NOTSC) #define _TIF_IA32 (1 << TIF_IA32) +#define _TIF_RESTORE_SIGMASK (1 << TIF_RESTORE_SIGMASK) #define _TIF_NOHZ (1 << TIF_NOHZ) #define _TIF_POLLING_NRFLAG (1 << TIF_POLLING_NRFLAG) #define _TIF_IO_BITMAP (1 << TIF_IO_BITMAP) diff --git a/kernel/signal.c b/kernel/signal.c index 328a01e1a2f0..a3346da1a4f5 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -2877,6 +2877,7 @@ int set_user_sigmask(const sigset_t __user *usigmask, sigset_t *set, *oldset = current->blocked; set_current_blocked(set); + set_thread_flag(TIF_RESTORE_SIGMASK); return 0; }
From: Linus Torvalds > Sent: 05 June 2019 18:25 > On Wed, Jun 5, 2019 at 8:58 AM Oleg Nesterov <oleg@redhat.com> wrote: > > > > To simplify the review, please see the code with this patch applied. > > I am using epoll_pwait() as an example because it looks very simple. > > I like it. > > However. > > I think I'd like it even more if we just said "we don't need > restore_saved_sigmask AT ALL". > > Which would be fairly easy to do with something like the attached... That would always call the signal handlers even when EINTR wasn't being returned (which I think ought to happen ...). The real purpose of restore_saved_sigmask() is to stop signal handlers that are enabled by the temporary mask being called. If a signal handler is called, I presume that the trampoline calls back into the kernel to get further handlers called and to finally restore the original signal mask? What happens if a signal handler calls something that would normally write to current->saved_sigmask? David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 06/05, Linus Torvalds wrote: > > On Wed, Jun 5, 2019 at 8:58 AM Oleg Nesterov <oleg@redhat.com> wrote: > > > > To simplify the review, please see the code with this patch applied. > > I am using epoll_pwait() as an example because it looks very simple. > > I like it. > > However. > > I think I'd like it even more if we just said "we don't need > restore_saved_sigmask AT ALL". ^^^^^^^^^^^^^^^^^^^^^ Did you mean restore_saved_sigmask_unless() introduced by this patch? If yes: > Which would be fairly easy to do with something like the attached... I don't think so, > --- a/arch/x86/entry/common.c > +++ b/arch/x86/entry/common.c > @@ -160,7 +160,7 @@ static void exit_to_usermode_loop(struct pt_regs *regs, u32 cached_flags) > klp_update_patch_state(current); > > /* deal with pending signal delivery */ > - if (cached_flags & _TIF_SIGPENDING) > + if (cached_flags & (_TIF_SIGPENDING | _TIF_RESTORE_SIGMASK)) > do_signal(regs); ... > --- a/kernel/signal.c > +++ b/kernel/signal.c > @@ -2877,6 +2877,7 @@ int set_user_sigmask(const sigset_t __user *usigmask, sigset_t *set, > > *oldset = current->blocked; > set_current_blocked(set); > + set_thread_flag(TIF_RESTORE_SIGMASK); This will re-introduce the problem fixed by the previous patch. Yes, do_signal() does restore_saved_sigmask() at the end, but only if get_signal() returns false. This means that restore_saved_sigmask()->set_current_blocked(saved_mask) should restore ->blocked (and may be clear TIF_SIGPENDING) before ret-from-syscall. Or I misunderstood? Oleg.
On 06/06, David Laight wrote: > > If a signal handler is called, I presume that the trampoline > calls back into the kernel to get further handlers called > and to finally restore the original signal mask? See sigmask_to_save(), this is what the kernel records in uc.uc_sigmask before the signal handler runs, after that current->saved_sigmask has no meaning. When signal handler returns it does sys_rt_sigreturn() which restores the original mask saved in uc_sigmask. > What happens if a signal handler calls something that > would normally write to current->saved_sigmask? See above. Oleg.
From: Oleg Nesterov > Sent: 06 June 2019 12:05 > On 06/06, David Laight wrote: > > > > If a signal handler is called, I presume that the trampoline > > calls back into the kernel to get further handlers called > > and to finally restore the original signal mask? > > See sigmask_to_save(), this is what the kernel records in uc.uc_sigmask > before the signal handler runs, after that current->saved_sigmask has no > meaning. Some of this code is hard to grep through :-) > When signal handler returns it does sys_rt_sigreturn() which restores > the original mask saved in uc_sigmask. Does that mean that if 2 signals interrupt epoll_wait() only one of the signal handlers is run? David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 06/06, David Laight wrote: > > Some of this code is hard to grep through :-) I'd suggest to simply read the kernel code once and memorise it, after that you will not need to use grep. > > When signal handler returns it does sys_rt_sigreturn() which restores > > the original mask saved in uc_sigmask. > > Does that mean that if 2 signals interrupt epoll_wait() only > one of the signal handlers is run? I'll assume that both signals were blocked before syscall and temporary unblocked by pselect. Quite contrary, they both will be delivered exactly because original mask won't be restored until the 1st handler returns. Unless, of course, the sigaction->sa_mask of the 1st signal blocks another one. Didn't I say you do not read my emails? I have already explained this to you in this thread ;) Oleg.
From: Oleg Nesterov > Sent: 06 June 2019 13:41 > On 06/06, David Laight wrote: > > > > Some of this code is hard to grep through :-) > > I'd suggest to simply read the kernel code once and memorise it, after > that you will not need to use grep. Unfortunately all the available buffer space is full of the SYSV and NetBSD kernels, there isn't any room for the Linux one :-) David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On top of signal-remove-the-wrong-signal_pending-check-in-restore_user_sigmask.patch Let me repeat, every file touched by this patch needs more cleanups, fs/aio.c looks wrong with or without the recent changes. Lets discuss this later. To simplify the review, please see the code with this patch applied. I am using epoll_pwait() as an example because it looks very simple. Note that this patch moves WARN_ON(!TIF_SIGPENDING) from set_restore_sigmask() to restore_unless(). int set_user_sigmask(const sigset_t __user *umask, size_t sigsetsize) { sigset_t *kmask; if (!umask) return 0; if (sigsetsize != sizeof(sigset_t)) return -EINVAL; if (copy_from_user(kmask, umask, sizeof(sigset_t))) return -EFAULT; set_restore_sigmask(); current->saved_sigmask = current->blocked; set_current_blocked(kmask); return 0; } static inline void restore_saved_sigmask_unless(bool interrupted) { if (interrupted) WARN_ON(!test_thread_flag(TIF_SIGPENDING)); else restore_saved_sigmask(); } SYSCALL_DEFINE6(epoll_pwait, int, epfd, struct epoll_event __user *, events, int, maxevents, int, timeout, const sigset_t __user *, sigmask, size_t, sigsetsize) { int error; /* * If the caller wants a certain signal mask to be set during the wait, * we apply it here. */ error = set_user_sigmask(sigmask, sigsetsize); if (error) return error; error = do_epoll_wait(epfd, events, maxevents, timeout); restore_saved_sigmask_unless(error == -EINTR); return error; } Oleg.