[PATCH 0/2] signals: eventpoll: save/restore_sigmask cleanups

[PATCH 0/2] signals: eventpoll: save/restore_sigmask cleanups

[Oleg Nesterov]

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(-)

[PATCH 1/2] signals: eventpoll: do not use sigprocmask()

[Oleg Nesterov]

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

[PATCH 2/2] signals: eventpoll: set ->saved_sigmask at the start

[Oleg Nesterov]

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(&current->saved_sigmask, &sigsaved,
-			       sizeof(sigsaved));
+		if (error == -EINTR)
 			set_restore_sigmask();
-		} else
-			set_current_blocked(&sigsaved);
+		else
+			__set_current_blocked(&current->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(&current->saved_sigmask, &sigsaved,
-			       sizeof(sigsaved));
+		if (err == -EINTR)
 			set_restore_sigmask();
-		} else
-			set_current_blocked(&sigsaved);
+		else
+			__set_current_blocked(&current->saved_sigmask);
 	}
 
 	return err;
-- 
1.5.5.1

Re: [PATCH 2/2] signals: eventpoll: set ->saved_sigmask at the start

[Al Viro]

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(&current->saved_sigmask, &sigsaved,
> -			       sizeof(sigsaved));
> +		if (error == -EINTR)
>  			set_restore_sigmask();
> -		} else
> -			set_current_blocked(&sigsaved);
> +		else
> +			__set_current_blocked(&current->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(&current->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.

Re: [PATCH 2/2] signals: eventpoll: set ->saved_sigmask at the start

[Oleg Nesterov]

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(&current->saved_sigmask, &sigsaved,
> > -			       sizeof(sigsaved));
> > +		if (error == -EINTR)
> >  			set_restore_sigmask();
> > -		} else
> > -			set_current_blocked(&sigsaved);
> > +		else
> > +			__set_current_blocked(&current->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(&current->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.

Re: [PATCH 2/2] signals: eventpoll: set ->saved_sigmask at the start

[Oleg Nesterov]

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(&current->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(&current->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.

Re: [PATCH 2/2] signals: eventpoll: set ->saved_sigmask at the start

[Oleg Nesterov]

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.