signals: eventpoll: save/restore_sigmask cleanups

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

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.

---
 fs/eventpoll.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

index deecc72..0f0f736 100644
@@ -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

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

---
 fs/eventpoll.c |   34 +++++++++++++++-------------------
 1 files changed, 15 insertions(+), 19 deletions(-)

index 0f0f736..2ea3584 100644
@@ -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

set_restore_sigmask() does WARN_ON(!TIF_SIGPENDING).

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.

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.

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.