* [PATCH AUTOSEL 4.9 149/251] signal/cifs: Fix cifs_put_tcp_session to call send_sig instead of force_sig
[not found] <20200116173641.22137-1-sashal@kernel.org>
@ 2020-01-16 17:34 ` Sasha Levin
2020-01-16 17:35 ` [PATCH AUTOSEL 4.9 177/251] cifs: fix rmmod regression in cifs.ko caused by force_sig changes Sasha Levin
2020-01-16 17:35 ` [PATCH AUTOSEL 4.9 183/251] signal: Allow cifs and drbd to receive their terminating signals Sasha Levin
2 siblings, 0 replies; 4+ messages in thread
From: Sasha Levin @ 2020-01-16 17:34 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Eric W. Biederman, Namjae Jeon, Jeff Layton, Steve French,
Sasha Levin, linux-cifs, samba-technical
From: "Eric W. Biederman" <ebiederm@xmission.com>
[ Upstream commit 72abe3bcf0911d69b46c1e8bdb5612675e0ac42c ]
The locking in force_sig_info is not prepared to deal with a task that
exits or execs (as sighand may change). The is not a locking problem
in force_sig as force_sig is only built to handle synchronous
exceptions.
Further the function force_sig_info changes the signal state if the
signal is ignored, or blocked or if SIGNAL_UNKILLABLE will prevent the
delivery of the signal. The signal SIGKILL can not be ignored and can
not be blocked and SIGNAL_UNKILLABLE won't prevent it from being
delivered.
So using force_sig rather than send_sig for SIGKILL is confusing
and pointless.
Because it won't impact the sending of the signal and and because
using force_sig is wrong, replace force_sig with send_sig.
Cc: Namjae Jeon <namjae.jeon@samsung.com>
Cc: Jeff Layton <jlayton@primarydata.com>
Cc: Steve French <smfrench@gmail.com>
Fixes: a5c3e1c725af ("Revert "cifs: No need to send SIGKILL to demux_thread during umount"")
Fixes: e7ddee9037e7 ("cifs: disable sharing session and tcon and add new TCP sharing code")
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
fs/cifs/connect.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index e43ba6db2bdd..110febd69737 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -2221,7 +2221,7 @@ cifs_put_tcp_session(struct TCP_Server_Info *server, int from_reconnect)
task = xchg(&server->tsk, NULL);
if (task)
- force_sig(SIGKILL, task);
+ send_sig(SIGKILL, task, 1);
}
static struct TCP_Server_Info *
--
2.20.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH AUTOSEL 4.9 177/251] cifs: fix rmmod regression in cifs.ko caused by force_sig changes
[not found] <20200116173641.22137-1-sashal@kernel.org>
2020-01-16 17:34 ` [PATCH AUTOSEL 4.9 149/251] signal/cifs: Fix cifs_put_tcp_session to call send_sig instead of force_sig Sasha Levin
@ 2020-01-16 17:35 ` Sasha Levin
2020-01-16 19:11 ` [EXTERNAL] " Steven French
2020-01-16 17:35 ` [PATCH AUTOSEL 4.9 183/251] signal: Allow cifs and drbd to receive their terminating signals Sasha Levin
2 siblings, 1 reply; 4+ messages in thread
From: Sasha Levin @ 2020-01-16 17:35 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Steve French, Ronnie Sahlberg, Eric W . Biederman, Sasha Levin,
linux-cifs, samba-technical
From: Steve French <stfrench@microsoft.com>
[ Upstream commit 247bc9470b1eeefc7b58cdf2c39f2866ba651509 ]
Fixes: 72abe3bcf091 ("signal/cifs: Fix cifs_put_tcp_session to call send_sig instead of force_sig")
The global change from force_sig caused module unloading of cifs.ko
to fail (since the cifsd process could not be killed, "rmmod cifs"
now would always fail)
Signed-off-by: Steve French <stfrench@microsoft.com>
Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
CC: Eric W. Biederman <ebiederm@xmission.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
fs/cifs/connect.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 110febd69737..7d46025d5e89 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -885,6 +885,7 @@ cifs_demultiplex_thread(void *p)
mempool_resize(cifs_req_poolp, length + cifs_min_rcv);
set_freezable();
+ allow_signal(SIGKILL);
while (server->tcpStatus != CifsExiting) {
if (try_to_freeze())
continue;
--
2.20.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH AUTOSEL 4.9 183/251] signal: Allow cifs and drbd to receive their terminating signals
[not found] <20200116173641.22137-1-sashal@kernel.org>
2020-01-16 17:34 ` [PATCH AUTOSEL 4.9 149/251] signal/cifs: Fix cifs_put_tcp_session to call send_sig instead of force_sig Sasha Levin
2020-01-16 17:35 ` [PATCH AUTOSEL 4.9 177/251] cifs: fix rmmod regression in cifs.ko caused by force_sig changes Sasha Levin
@ 2020-01-16 17:35 ` Sasha Levin
2 siblings, 0 replies; 4+ messages in thread
From: Sasha Levin @ 2020-01-16 17:35 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Eric W. Biederman, ronnie sahlberg, Christoph Böhmwalder,
Steve French, Philipp Reisner, David Laight, Sasha Levin,
drbd-dev, linux-block, linux-cifs, samba-technical
From: "Eric W. Biederman" <ebiederm@xmission.com>
[ Upstream commit 33da8e7c814f77310250bb54a9db36a44c5de784 ]
My recent to change to only use force_sig for a synchronous events
wound up breaking signal reception cifs and drbd. I had overlooked
the fact that by default kthreads start out with all signals set to
SIG_IGN. So a change I thought was safe turned out to have made it
impossible for those kernel thread to catch their signals.
Reverting the work on force_sig is a bad idea because what the code
was doing was very much a misuse of force_sig. As the way force_sig
ultimately allowed the signal to happen was to change the signal
handler to SIG_DFL. Which after the first signal will allow userspace
to send signals to these kernel threads. At least for
wake_ack_receiver in drbd that does not appear actively wrong.
So correct this problem by adding allow_kernel_signal that will allow
signals whose siginfo reports they were sent by the kernel through,
but will not allow userspace generated signals, and update cifs and
drbd to call allow_kernel_signal in an appropriate place so that their
thread can receive this signal.
Fixing things this way ensures that userspace won't be able to send
signals and cause problems, that it is clear which signals the
threads are expecting to receive, and it guarantees that nothing
else in the system will be affected.
This change was partly inspired by similar cifs and drbd patches that
added allow_signal.
Reported-by: ronnie sahlberg <ronniesahlberg@gmail.com>
Reported-by: Christoph Böhmwalder <christoph.boehmwalder@linbit.com>
Tested-by: Christoph Böhmwalder <christoph.boehmwalder@linbit.com>
Cc: Steve French <smfrench@gmail.com>
Cc: Philipp Reisner <philipp.reisner@linbit.com>
Cc: David Laight <David.Laight@ACULAB.COM>
Fixes: 247bc9470b1e ("cifs: fix rmmod regression in cifs.ko caused by force_sig changes")
Fixes: 72abe3bcf091 ("signal/cifs: Fix cifs_put_tcp_session to call send_sig instead of force_sig")
Fixes: fee109901f39 ("signal/drbd: Use send_sig not force_sig")
Fixes: 3cf5d076fb4d ("signal: Remove task parameter from force_sig")
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/block/drbd/drbd_main.c | 2 ++
fs/cifs/connect.c | 2 +-
include/linux/signal.h | 15 ++++++++++++++-
kernel/signal.c | 5 +++++
4 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c
index f5c24459fc5c..daa9cef96ec6 100644
--- a/drivers/block/drbd/drbd_main.c
+++ b/drivers/block/drbd/drbd_main.c
@@ -332,6 +332,8 @@ static int drbd_thread_setup(void *arg)
thi->name[0],
resource->name);
+ allow_kernel_signal(DRBD_SIGKILL);
+ allow_kernel_signal(SIGXCPU);
restart:
retval = thi->function(thi);
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 7d46025d5e89..751bdde6515d 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -885,7 +885,7 @@ cifs_demultiplex_thread(void *p)
mempool_resize(cifs_req_poolp, length + cifs_min_rcv);
set_freezable();
- allow_signal(SIGKILL);
+ allow_kernel_signal(SIGKILL);
while (server->tcpStatus != CifsExiting) {
if (try_to_freeze())
continue;
diff --git a/include/linux/signal.h b/include/linux/signal.h
index 5308304993be..ffa58ff53e22 100644
--- a/include/linux/signal.h
+++ b/include/linux/signal.h
@@ -313,6 +313,9 @@ extern void signal_setup_done(int failed, struct ksignal *ksig, int stepping);
extern void exit_signals(struct task_struct *tsk);
extern void kernel_sigaction(int, __sighandler_t);
+#define SIG_KTHREAD ((__force __sighandler_t)2)
+#define SIG_KTHREAD_KERNEL ((__force __sighandler_t)3)
+
static inline void allow_signal(int sig)
{
/*
@@ -320,7 +323,17 @@ static inline void allow_signal(int sig)
* know it'll be handled, so that they don't get converted to
* SIGKILL or just silently dropped.
*/
- kernel_sigaction(sig, (__force __sighandler_t)2);
+ kernel_sigaction(sig, SIG_KTHREAD);
+}
+
+static inline void allow_kernel_signal(int sig)
+{
+ /*
+ * Kernel threads handle their own signals. Let the signal code
+ * know signals sent by the kernel will be handled, so that they
+ * don't get silently dropped.
+ */
+ kernel_sigaction(sig, SIG_KTHREAD_KERNEL);
}
static inline void disallow_signal(int sig)
diff --git a/kernel/signal.c b/kernel/signal.c
index 30914b3c76b2..57fadbe69c2e 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -79,6 +79,11 @@ static int sig_task_ignored(struct task_struct *t, int sig, bool force)
handler == SIG_DFL && !(force && sig_kernel_only(sig)))
return 1;
+ /* Only allow kernel generated signals to this kthread */
+ if (unlikely((t->flags & PF_KTHREAD) &&
+ (handler == SIG_KTHREAD_KERNEL) && !force))
+ return true;
+
return sig_handler_ignored(handler, sig);
}
--
2.20.1
^ permalink raw reply related [flat|nested] 4+ messages in thread