Re: [PATCH -mm 0/1] signal: simplify set_user_sigmask/restore_user_sigmask

From: Linus Torvalds <torvalds@linux-foundation.org>
To: Oleg Nesterov <oleg@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Deepa Dinamani <deepa.kernel@gmail.com>,
	Linux List Kernel Mailing <linux-kernel@vger.kernel.org>,
	Arnd Bergmann <arnd@arndb.de>, Davidlohr Bueso <dbueso@suse.de>,
	Jens Axboe <axboe@kernel.dk>, Davidlohr Bueso <dave@stgolabs.net>,
	e@80x24.org, Jason Baron <jbaron@akamai.com>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	linux-aio@kvack.org, omar.kilani@gmail.com,
	Thomas Gleixner <tglx@linutronix.de>,
	stable <stable@vger.kernel.org>,
	Al Viro <viro@zeniv.linux.org.uk>,
	"Eric W. Biederman" <ebiederm@xmission.com>,
	David Laight <David.Laight@aculab.com>
Subject: Re: [PATCH -mm 0/1] signal: simplify set_user_sigmask/restore_user_sigmask
Date: Wed, 5 Jun 2019 10:24:47 -0700	[thread overview]
Message-ID: <CAHk-=wjkNx8u4Mcm5dfSQKYQmLQAv1Z1yGLDZvty7BVSj4eqBA@mail.gmail.com> (raw)
In-Reply-To: <20190605155801.GA25165@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 695 bytes --]

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

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 2079 bytes --]

 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;
 }
