* [PATCH 0/2] signals: eventpoll: save/restore_sigmask cleanups
@ 2013-06-25 19:57 Oleg Nesterov
2013-06-25 19:57 ` [PATCH 1/2] signals: eventpoll: do not use sigprocmask() Oleg Nesterov
2013-06-25 19:57 ` [PATCH 2/2] signals: eventpoll: set ->saved_sigmask at the start Oleg Nesterov
0 siblings, 2 replies; 7+ messages in thread
From: Oleg Nesterov @ 2013-06-25 19:57 UTC (permalink / raw)
To: Andrew Morton
Cc: Al Viro, Denys Vlasenko, Eric Wong, Jason Baron, linux-kernel
Hello.
Motivated by the recent patch from Denys. Lets cleanup this code
first, other set_restore_sigmask() users can be changed too.
Perhaps it also makes sense to add the new helper which does
copy_from_user + set saved_sigmask + set_current_blocked() ?
Oleg.
fs/eventpoll.c | 38 +++++++++++++++++---------------------
1 files changed, 17 insertions(+), 21 deletions(-)
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] signals: eventpoll: do not use sigprocmask()
2013-06-25 19:57 [PATCH 0/2] signals: eventpoll: save/restore_sigmask cleanups Oleg Nesterov
@ 2013-06-25 19:57 ` Oleg Nesterov
2013-06-25 19:57 ` [PATCH 2/2] signals: eventpoll: set ->saved_sigmask at the start Oleg Nesterov
1 sibling, 0 replies; 7+ messages in thread
From: Oleg Nesterov @ 2013-06-25 19:57 UTC (permalink / raw)
To: Andrew Morton
Cc: Al Viro, Denys Vlasenko, Eric Wong, Jason Baron, linux-kernel
sigprocmask() should die. None of the current callers actually
need this strange interface.
Change fs/eventpoll.c to use set_current_blocked(). This also
means we should not worry about SIGKILL/SIGSTOP.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
fs/eventpoll.c | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index deecc72..0f0f736 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -1975,8 +1975,8 @@ SYSCALL_DEFINE6(epoll_pwait, int, epfd, struct epoll_event __user *, events,
return -EINVAL;
if (copy_from_user(&ksigmask, sigmask, sizeof(ksigmask)))
return -EFAULT;
- sigdelsetmask(&ksigmask, sigmask(SIGKILL) | sigmask(SIGSTOP));
- sigprocmask(SIG_SETMASK, &ksigmask, &sigsaved);
+ sigsaved = current->blocked;
+ set_current_blocked(&ksigmask);
}
error = sys_epoll_wait(epfd, events, maxevents, timeout);
@@ -1993,7 +1993,7 @@ SYSCALL_DEFINE6(epoll_pwait, int, epfd, struct epoll_event __user *, events,
sizeof(sigsaved));
set_restore_sigmask();
} else
- sigprocmask(SIG_SETMASK, &sigsaved, NULL);
+ set_current_blocked(&sigsaved);
}
return error;
@@ -2020,8 +2020,8 @@ COMPAT_SYSCALL_DEFINE6(epoll_pwait, int, epfd,
if (copy_from_user(&csigmask, sigmask, sizeof(csigmask)))
return -EFAULT;
sigset_from_compat(&ksigmask, &csigmask);
- sigdelsetmask(&ksigmask, sigmask(SIGKILL) | sigmask(SIGSTOP));
- sigprocmask(SIG_SETMASK, &ksigmask, &sigsaved);
+ sigsaved = current->blocked;
+ set_current_blocked(&ksigmask);
}
err = sys_epoll_wait(epfd, events, maxevents, timeout);
@@ -2038,7 +2038,7 @@ COMPAT_SYSCALL_DEFINE6(epoll_pwait, int, epfd,
sizeof(sigsaved));
set_restore_sigmask();
} else
- sigprocmask(SIG_SETMASK, &sigsaved, NULL);
+ set_current_blocked(&sigsaved);
}
return err;
--
1.5.5.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] signals: eventpoll: set ->saved_sigmask at the start
2013-06-25 19:57 [PATCH 0/2] signals: eventpoll: save/restore_sigmask cleanups Oleg Nesterov
2013-06-25 19:57 ` [PATCH 1/2] signals: eventpoll: do not use sigprocmask() Oleg Nesterov
@ 2013-06-25 19:57 ` Oleg Nesterov
2013-06-25 20:23 ` Al Viro
1 sibling, 1 reply; 7+ messages in thread
From: Oleg Nesterov @ 2013-06-25 19:57 UTC (permalink / raw)
To: Andrew Morton
Cc: Al Viro, Denys Vlasenko, Eric Wong, Jason Baron, linux-kernel
task_struct->saved_sigmask has no meaning unless we return with
set_restore_sigmask() and nobody except current can use it.
This means that sys_epoll_pwait() doesn't need to save ->blocked
into the local var and then memcopy it into ->saved_sigmask, we
can simply set ->saved_sigmask right before set_current_blocked().
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
fs/eventpoll.c | 34 +++++++++++++++-------------------
1 files changed, 15 insertions(+), 19 deletions(-)
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 0f0f736..2ea3584 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -1964,23 +1964,23 @@ SYSCALL_DEFINE6(epoll_pwait, int, epfd, struct epoll_event __user *, events,
size_t, sigsetsize)
{
int error;
- sigset_t ksigmask, sigsaved;
-
/*
* If the caller wants a certain signal mask to be set during the wait,
* we apply it here.
*/
if (sigmask) {
+ sigset_t ksigmask;
+
if (sigsetsize != sizeof(sigset_t))
return -EINVAL;
if (copy_from_user(&ksigmask, sigmask, sizeof(ksigmask)))
return -EFAULT;
- sigsaved = current->blocked;
+
+ current->saved_sigmask = current->blocked;
set_current_blocked(&ksigmask);
}
error = sys_epoll_wait(epfd, events, maxevents, timeout);
-
/*
* If we changed the signal mask, we need to restore the original one.
* In case we've got a signal while waiting, we do not restore the
@@ -1988,12 +1988,10 @@ SYSCALL_DEFINE6(epoll_pwait, int, epfd, struct epoll_event __user *, events,
* the way back to userspace, before the signal mask is restored.
*/
if (sigmask) {
- if (error == -EINTR) {
- memcpy(¤t->saved_sigmask, &sigsaved,
- sizeof(sigsaved));
+ if (error == -EINTR)
set_restore_sigmask();
- } else
- set_current_blocked(&sigsaved);
+ else
+ __set_current_blocked(¤t->saved_sigmask);
}
return error;
@@ -2007,25 +2005,25 @@ COMPAT_SYSCALL_DEFINE6(epoll_pwait, int, epfd,
compat_size_t, sigsetsize)
{
long err;
- compat_sigset_t csigmask;
- sigset_t ksigmask, sigsaved;
-
/*
* If the caller wants a certain signal mask to be set during the wait,
* we apply it here.
*/
if (sigmask) {
+ compat_sigset_t csigmask;
+ sigset_t ksigmask;
+
if (sigsetsize != sizeof(compat_sigset_t))
return -EINVAL;
if (copy_from_user(&csigmask, sigmask, sizeof(csigmask)))
return -EFAULT;
sigset_from_compat(&ksigmask, &csigmask);
- sigsaved = current->blocked;
+
+ current->saved_sigmask = current->blocked;
set_current_blocked(&ksigmask);
}
err = sys_epoll_wait(epfd, events, maxevents, timeout);
-
/*
* If we changed the signal mask, we need to restore the original one.
* In case we've got a signal while waiting, we do not restore the
@@ -2033,12 +2031,10 @@ COMPAT_SYSCALL_DEFINE6(epoll_pwait, int, epfd,
* the way back to userspace, before the signal mask is restored.
*/
if (sigmask) {
- if (err == -EINTR) {
- memcpy(¤t->saved_sigmask, &sigsaved,
- sizeof(sigsaved));
+ if (err == -EINTR)
set_restore_sigmask();
- } else
- set_current_blocked(&sigsaved);
+ else
+ __set_current_blocked(¤t->saved_sigmask);
}
return err;
--
1.5.5.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] signals: eventpoll: set ->saved_sigmask at the start
2013-06-25 19:57 ` [PATCH 2/2] signals: eventpoll: set ->saved_sigmask at the start Oleg Nesterov
@ 2013-06-25 20:23 ` Al Viro
2013-06-25 20:32 ` Oleg Nesterov
0 siblings, 1 reply; 7+ messages in thread
From: Al Viro @ 2013-06-25 20:23 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Andrew Morton, Denys Vlasenko, Eric Wong, Jason Baron, linux-kernel
On Tue, Jun 25, 2013 at 09:57:59PM +0200, Oleg Nesterov wrote:
> + current->saved_sigmask = current->blocked;
> set_current_blocked(&ksigmask);
> }
>
> error = sys_epoll_wait(epfd, events, maxevents, timeout);
> -
> /*
> * If we changed the signal mask, we need to restore the original one.
> * In case we've got a signal while waiting, we do not restore the
> @@ -1988,12 +1988,10 @@ SYSCALL_DEFINE6(epoll_pwait, int, epfd, struct epoll_event __user *, events,
> * the way back to userspace, before the signal mask is restored.
> */
> if (sigmask) {
> - if (error == -EINTR) {
> - memcpy(¤t->saved_sigmask, &sigsaved,
> - sizeof(sigsaved));
> + if (error == -EINTR)
> set_restore_sigmask();
> - } else
> - set_current_blocked(&sigsaved);
> + else
> + __set_current_blocked(¤t->saved_sigmask);
I don't like that. If anything, we have
static inline void restore_saved_sigmask(void)
{
if (test_and_clear_restore_sigmask())
__set_current_blocked(¤t->saved_sigmask);
}
which means that the last part can be turned into
set_restore_sigmask();
if (error != -EINTR)
restore_saved_sigmask();
and I'd pulled set_restore_sigmask() call next to setting the sucker.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] signals: eventpoll: set ->saved_sigmask at the start
2013-06-25 20:23 ` Al Viro
@ 2013-06-25 20:32 ` Oleg Nesterov
2013-06-25 20:44 ` Oleg Nesterov
0 siblings, 1 reply; 7+ messages in thread
From: Oleg Nesterov @ 2013-06-25 20:32 UTC (permalink / raw)
To: Al Viro
Cc: Andrew Morton, Denys Vlasenko, Eric Wong, Jason Baron, linux-kernel
On 06/25, Al Viro wrote:
>
> On Tue, Jun 25, 2013 at 09:57:59PM +0200, Oleg Nesterov wrote:
> > + current->saved_sigmask = current->blocked;
> > set_current_blocked(&ksigmask);
> > }
> >
> > error = sys_epoll_wait(epfd, events, maxevents, timeout);
> > -
> > /*
> > * If we changed the signal mask, we need to restore the original one.
> > * In case we've got a signal while waiting, we do not restore the
> > @@ -1988,12 +1988,10 @@ SYSCALL_DEFINE6(epoll_pwait, int, epfd, struct epoll_event __user *, events,
> > * the way back to userspace, before the signal mask is restored.
> > */
> > if (sigmask) {
> > - if (error == -EINTR) {
> > - memcpy(¤t->saved_sigmask, &sigsaved,
> > - sizeof(sigsaved));
> > + if (error == -EINTR)
> > set_restore_sigmask();
> > - } else
> > - set_current_blocked(&sigsaved);
> > + else
> > + __set_current_blocked(¤t->saved_sigmask);
>
> I don't like that. If anything, we have
> static inline void restore_saved_sigmask(void)
> {
> if (test_and_clear_restore_sigmask())
> __set_current_blocked(¤t->saved_sigmask);
> }
> which means that the last part can be turned into
> set_restore_sigmask();
> if (error != -EINTR)
> restore_saved_sigmask();
set_restore_sigmask() does WARN_ON(!TIF_SIGPENDING).
> and I'd pulled set_restore_sigmask() call next to setting the sucker.
Sorry, can't understand...
Anyway, I agree we can make this more clean. From 0/2
Perhaps it also makes sense to add the new helper which does
copy_from_user + set saved_sigmask + set_current_blocked() ?
and perhaps we can add another helper which does set_restore_sigmask()
_or_ set_current_blocked(saved_sigmask), this can simplify more callers.
I think we can do this on top of this change.
Or I misunderstood and you dislike the very fact we rely on the already
initialized ->saved_sigmask ?
Oleg.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] signals: eventpoll: set ->saved_sigmask at the start
2013-06-25 20:32 ` Oleg Nesterov
@ 2013-06-25 20:44 ` Oleg Nesterov
2013-06-26 16:48 ` Oleg Nesterov
0 siblings, 1 reply; 7+ messages in thread
From: Oleg Nesterov @ 2013-06-25 20:44 UTC (permalink / raw)
To: Al Viro
Cc: Andrew Morton, Denys Vlasenko, Eric Wong, Jason Baron, linux-kernel
On 06/25, Oleg Nesterov wrote:
>
> On 06/25, Al Viro wrote:
> >
> > On Tue, Jun 25, 2013 at 09:57:59PM +0200, Oleg Nesterov wrote:
> > > + if (error == -EINTR)
> > > set_restore_sigmask();
> > > - } else
> > > - set_current_blocked(&sigsaved);
> > > + else
> > > + __set_current_blocked(¤t->saved_sigmask);
> >
> > I don't like that. If anything, we have
> > static inline void restore_saved_sigmask(void)
> > {
> > if (test_and_clear_restore_sigmask())
> > __set_current_blocked(¤t->saved_sigmask);
> > }
> > which means that the last part can be turned into
> > set_restore_sigmask();
> > if (error != -EINTR)
> > restore_saved_sigmask();
>
> set_restore_sigmask() does WARN_ON(!TIF_SIGPENDING).
But if we remove this WARN_ON() we can probably change
set_restore_sigmask() to set TS_RESTORE_SIGMASK and
do saved_mask = blocked.
Perhaps it can even acccept "sigset_t *newmask" and do
set_current_blocked().
Then we can move it up and change the code above to simply as you
suggested. But imho this needs a separate change.
Oleg.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] signals: eventpoll: set ->saved_sigmask at the start
2013-06-25 20:44 ` Oleg Nesterov
@ 2013-06-26 16:48 ` Oleg Nesterov
0 siblings, 0 replies; 7+ messages in thread
From: Oleg Nesterov @ 2013-06-26 16:48 UTC (permalink / raw)
To: Al Viro
Cc: Andrew Morton, Denys Vlasenko, Eric Wong, Jason Baron, linux-kernel
On 06/25, Oleg Nesterov wrote:
>
> But if we remove this WARN_ON() we can probably change
> set_restore_sigmask() to set TS_RESTORE_SIGMASK and
> do saved_mask = blocked.
>
> Perhaps it can even acccept "sigset_t *newmask" and do
> set_current_blocked().
So, Al, what do you think if we do something like
--- x/arch/x86/include/asm/thread_info.h
+++ x/arch/x86/include/asm/thread_info.h
@@ -247,7 +247,6 @@ static inline void set_restore_sigmask(v
{
struct thread_info *ti = current_thread_info();
ti->status |= TS_RESTORE_SIGMASK;
- WARN_ON(!test_bit(TIF_SIGPENDING, (unsigned long *)&ti->flags));
}
static inline void clear_restore_sigmask(void)
{
--- x/include/linux/signal.h
+++ x/include/linux/signal.h
@@ -249,6 +249,13 @@ extern void __set_current_blocked(const
extern int show_unhandled_signals;
extern int sigsuspend(sigset_t *);
+static inline set_restore_xxx(sigset_t *mask)
+{
+ set_restore_sigmask();
+ current->saved_sigmask = current->blocked;
+ set_current_blocked(mask);
+}
+
struct sigaction {
#ifndef __ARCH_HAS_IRIX_SIGACTION
__sighandler_t sa_handler;
Then sys_epoll_pwait() (and other users) can simply do
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.
*/
if (sigmask) {
sigset_t ksigmask;
if (sigsetsize != sizeof(sigset_t))
return -EINVAL;
if (copy_from_user(&ksigmask, sigmask, sizeof(ksigmask)))
return -EFAULT;
set_restore_xxx(&ksigmask);
}
error = sys_epoll_wait(epfd, events, maxevents, timeout);
if (error != -EINTR)
restore_saved_sigmask();
return error;
}
Hmm... and when I re-read your original email I am starting to think
that perhaps you proposed exactly this...
But I still think it would be better to do this change on top of the
cleanups I sent (fs/compat.c and fs/select.c should be updated too).
But, perhaps, it also makes sense to add
void restore_saved_sigmask_if(bool cond)
{
if (cond)
restore_saved_sigmask();
else
WARN_ON(TS_RESTORE_SIGMASK && !TIF_SIGPENDING);
}
so that epoll_pwait() could do
restore_saved_sigmask_if(error != -EINTR);
What do you think?
Oleg.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-06-26 16:53 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-06-25 19:57 [PATCH 0/2] signals: eventpoll: save/restore_sigmask cleanups Oleg Nesterov
2013-06-25 19:57 ` [PATCH 1/2] signals: eventpoll: do not use sigprocmask() Oleg Nesterov
2013-06-25 19:57 ` [PATCH 2/2] signals: eventpoll: set ->saved_sigmask at the start Oleg Nesterov
2013-06-25 20:23 ` Al Viro
2013-06-25 20:32 ` Oleg Nesterov
2013-06-25 20:44 ` Oleg Nesterov
2013-06-26 16:48 ` Oleg Nesterov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.