Fix a regression introduced by upstream commit fee109901f39 ('signal/drbd: Use send_sig not force_sig').
Currently, when a thread is initialized, all signals are set to be ignored by default. DRBD uses SIGHUP to end its threads, which means it is now no longer possible to bring down a DRBD resource because the signals do not make it through to the thread in question.
This circumstance was previously hidden by the fact that DRBD used force_sig() to kill its threads. The aforementioned upstream commit changed this to send_sig(), which means the effects of the signals being ignored by default are now becoming visible.
Thus, issue an allow_signal() at the start of the thread to explicitly allow the desired signals.
Signed-off-by: Christoph Böhmwalder christoph.boehmwalder@linbit.com Signed-off-by: Philipp Reisner philipp.reisner@linbit.com Fixes: fee109901f39 ("signal/drbd: Use send_sig not force_sig") Cc: stable@vger.kernel.org --- drivers/block/drbd/drbd_main.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c index 9bd4ddd12b25..b8b986df6814 100644 --- a/drivers/block/drbd/drbd_main.c +++ b/drivers/block/drbd/drbd_main.c @@ -318,6 +318,9 @@ static int drbd_thread_setup(void *arg) unsigned long flags; int retval;
+ allow_signal(DRBD_SIGKILL); + allow_signal(SIGXCPU); + snprintf(current->comm, sizeof(current->comm), "drbd_%c_%s", thi->name[0], resource->name);
From: Christoph Böhmwalder
Sent: 29 July 2019 09:33 Fix a regression introduced by upstream commit fee109901f39 ('signal/drbd: Use send_sig not force_sig').
Currently, when a thread is initialized, all signals are set to be ignored by default. DRBD uses SIGHUP to end its threads, which means it is now no longer possible to bring down a DRBD resource because the signals do not make it through to the thread in question.
This circumstance was previously hidden by the fact that DRBD used force_sig() to kill its threads. The aforementioned upstream commit changed this to send_sig(), which means the effects of the signals being ignored by default are now becoming visible.
Thus, issue an allow_signal() at the start of the thread to explicitly allow the desired signals.
Doesn't unmasking the signals and using send_sig() instead of force_sig() have the (probably unwanted) side effect of allowing userspace to send the signal?
I've certainly got some driver code that uses force_sig() on a kthread that it doesn't (ever) want userspace to signal.
The origina1 commit says:
Further force_sig is for delivering synchronous signals (aka exceptions). The locking in force_sig is not prepared to deal with running processes, as tsk->sighand may change during exec for a running process.
I think a lot of code has assumed that the only real difference between send_sig() and force_sig() is that the latter ignores the signal mask.
If you need to unblock a kernel thread (eg one blocked in kernel_accept()) in order to unload a driver, then you really don't want it to be possible for anything else to signal the kthread.
David
- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 29.07.19 10:50, David Laight wrote:
Doesn't unmasking the signals and using send_sig() instead of force_sig() have the (probably unwanted) side effect of allowing userspace to send the signal?
I have ran some tests, and it does look like it is now possible to send signals to the DRBD kthread from userspace. However, ...
I've certainly got some driver code that uses force_sig() on a kthread that it doesn't (ever) want userspace to signal.
... we don't feel that it is absolutely necessary for userspace to be unable to send a signal to our kthreads. This is because the DRBD thread independently checks its own state, and (for example) only exits as a result of a signal if its thread state was already "EXITING" to begin with.
As such, our priority here is to get the main issue -- DRBD hanging upon exit -- resolved. I agree that it is not exactly desirable to have userspace send random signals to kthreads; not for DRBD and certainly not in general. However, we feel like it is more important to have DRBD actually work again in 5.3.
That said, there should probably still be a way to be able to send a signal to a kthread from the kernel, but not from userspace. I think the author of the original patch (Eric) might have some ideas here.
Jens, could you take a look and decide whether or not it's appropriate for you to funnel this through the linux-block tree to Linus for rc4?
The origina1 commit says:
Further force_sig is for delivering synchronous signals (aka exceptions). The locking in force_sig is not prepared to deal with running processes, as tsk->sighand may change during exec for a running process.
I think a lot of code has assumed that the only real difference between send_sig() and force_sig() is that the latter ignores the signal mask.
If you need to unblock a kernel thread (eg one blocked in kernel_accept()) in order to unload a driver, then you really don't want it to be possible for anything else to signal the kthread.
David
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
-- Christoph Böhmwalder LINBIT | Keeping the Digital World Running DRBD HA — Disaster Recovery — Software defined Storage
From: Christoph Böhmwalder
Sent: 05 August 2019 10:33
On 29.07.19 10:50, David Laight wrote:
Doesn't unmasking the signals and using send_sig() instead of force_sig() have the (probably unwanted) side effect of allowing userspace to send the signal?
I have ran some tests, and it does look like it is now possible to send signals to the DRBD kthread from userspace. However, ...
I've certainly got some driver code that uses force_sig() on a kthread that it doesn't (ever) want userspace to signal.
... we don't feel that it is absolutely necessary for userspace to be unable to send a signal to our kthreads. This is because the DRBD thread independently checks its own state, and (for example) only exits as a result of a signal if its thread state was already "EXITING" to begin with.
In must 'clear' the signal - otherwise it won't block again.
I've also got this horrid code fragment:
init_waitqueue_entry(&w, current);
/* Tell scheduler we are going to sleep... */ if (signal_pending(current) && !interruptible) /* We don't want waking immediately (again) */ sleep_state = TASK_UNINTERRUPTIBLE; else sleep_state = TASK_INTERRUPTIBLE; set_current_state(sleep_state);
/* Connect to condition variable ... */ add_wait_queue(cvp, &w); mutex_unlock(mtxp); /* Release mutex */
where we want to sleep TASK_UNINTERRUPTIBLE but that f*cks up the 'load average', so sleep TASK_INTERRUPTIBLE unless there is a signal pending that we want to ignore.
David
- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Hi Jens,
Please have a look.
With fee109901f392 Eric W. Biederman changed drbd to use send_sig() instead of force_sig(). That was part of a series that did this change in multiple call sites tree wide. Which, by accident broke drbd, since the signals are _not_ allowed by default. That got released with v5.2.
On July 29 Christoph Böhmwalder sent a patch that adds two allow_signal()s to fix drbd.
Then David Laight points out that he has code that can not deal with the send_sig() instead of force_sig() because allowed signals can be sent from user-space as well. I assume that David is referring to out of tree code, so I fear it is up to him to fix that to work with upstream, or initiate a revert of Eric's change.
Jens, please consider sending Christoph's path to Linus for merge in this cycle, or let us know how you think we should proceed.
best regards, Phil
Am Montag, 5. August 2019, 11:41:06 CEST schrieb David Laight:
From: Christoph Böhmwalder
Sent: 05 August 2019 10:33
On 29.07.19 10:50, David Laight wrote:
Doesn't unmasking the signals and using send_sig() instead of force_sig() have the (probably unwanted) side effect of allowing userspace to send the signal?
I have ran some tests, and it does look like it is now possible to send signals to the DRBD kthread from userspace. However, ...
I've certainly got some driver code that uses force_sig() on a kthread that it doesn't (ever) want userspace to signal.
... we don't feel that it is absolutely necessary for userspace to be unable to send a signal to our kthreads. This is because the DRBD thread independently checks its own state, and (for example) only exits as a result of a signal if its thread state was already "EXITING" to begin with.
In must 'clear' the signal - otherwise it won't block again.
I've also got this horrid code fragment:
init_waitqueue_entry(&w, current); /* Tell scheduler we are going to sleep... */ if (signal_pending(current) && !interruptible) /* We don't want waking immediately (again) */ sleep_state = TASK_UNINTERRUPTIBLE; else sleep_state = TASK_INTERRUPTIBLE; set_current_state(sleep_state); /* Connect to condition variable ... */ add_wait_queue(cvp, &w); mutex_unlock(mtxp); /* Release mutex */where we want to sleep TASK_UNINTERRUPTIBLE but that f*cks up the 'load average',
so sleep TASK_INTERRUPTIBLE unless there is a signal pending
that we want to ignore.
David
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
From: Philipp Reisner
Sent: 12 August 2019 12:53 Hi Jens,
Please have a look.
With fee109901f392 Eric W. Biederman changed drbd to use send_sig() instead of force_sig(). That was part of a series that did this change in multiple call sites tree wide. Which, by accident broke drbd, since the signals are _not_ allowed by default. That got released with v5.2.
On July 29 Christoph Böhmwalder sent a patch that adds two allow_signal()s to fix drbd.
Then David Laight points out that he has code that can not deal with the send_sig() instead of force_sig() because allowed signals can be sent from user-space as well. I assume that David is referring to out of tree code, so I fear it is up to him to fix that to work with upstream, or initiate a revert of Eric's change.
While our code is 'out of tree' (you really don't want it - and since it still uses force_sig() is fine) I suspect that the 'drdb' code (with Christoph's allow_signal() patch) now loops in kernel if a user sends it a signal.
If the driver (eg drdb) is using (say) SIGINT to break a thread out of (say) a blocking kernel_accept() call then it can detect the unexpected signal (maybe double-checking with signal_pending()) but I don't think it can clear down the pending signal so that kernel_accept() blocks again.
Jens, please consider sending Christoph's path to Linus for merge in this cycle, or let us know how you think we should proceed.
I'm not sure what the 'correct' solution is.
David
- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Hi David,
[...]
While our code is 'out of tree' (you really don't want it - and since it still uses force_sig() is fine) I suspect that the 'drdb' code (with Christoph's allow_signal() patch) now loops in kernel if a user sends it a signal.
I am not asking for that out of tree code. But you are welcome to learn from the drbd code that is in the upstream kernel. It does not loop if a root sends a signal, it receives it and ignores it.
If the driver (eg drdb) is using (say) SIGINT to break a thread out of (say) a blocking kernel_accept() call then it can detect the unexpected signal (maybe double-checking with signal_pending()) but I don't think it can clear down the pending signal so that kernel_accept() blocks again.
You do that with flush_signals(current)
What we have do is, somewhere in the main loop:
if (signal_pending(current)) { flush_signals(current); if (!terminate_condition()) { warn(connection, "Ignoring an unexpected signal\n"); continue; } break; } }
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 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 --- 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(-)
Folks my apolgies for this mess and for taking so long to suggest an improvement. I needed a good nights sleep to think about this and with a new baby at home that has been a challenge to get.
Unless someone has an objection or sees a problem with this patch I will send this to Linus in the next couple of days.
I think adding allow_kernel_signal is better because it makes it clear that userspace does not mess with these signals. I would love it if we could avoid signals all together but that appears tricky in the presence of kernel threads making blocking network requests.
diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c index 9bd4ddd12b25..5b248763a672 100644 --- a/drivers/block/drbd/drbd_main.c +++ b/drivers/block/drbd/drbd_main.c @@ -322,6 +322,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 a15a6e738eb5..1795e80cbdf7 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -1113,7 +1113,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 b5d99482d3fe..703fa20c06f5 100644 --- a/include/linux/signal.h +++ b/include/linux/signal.h @@ -282,6 +282,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) { /* @@ -289,7 +292,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 + * kwown 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 e667be6907d7..534fec266a33 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -90,6 +90,11 @@ static bool sig_task_ignored(struct task_struct *t, int sig, bool force) handler == SIG_DFL && !(force && sig_kernel_only(sig))) return true;
+ /* 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); }
On Fri, Aug 16, 2019 at 05:19:38PM -0500, Eric W. Biederman wrote:
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 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
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(-)
Just tested this patch, and I can confirm that it makes DRBD work as intended again.
Tested-by: Christoph Böhmwalder christoph.boehmwalder@linbit.com
-- Christoph Böhmwalder LINBIT | Keeping the Digital World Running DRBD HA — Disaster Recovery — Software defined Storage
Linus,
Please pull the siginfo-linus branch from the git tree:
git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git siginfo-linus
HEAD: 33da8e7c814f77310250bb54a9db36a44c5de784 signal: Allow cifs and drbd to receive their terminating signals
I overlooked the fact that kernel threads are created with all signals set to SIG_IGN, and accidentally caused a regression in cifs and drbd when replacing force_sig with send_sig.
This pull request is my fix for that regression. I add a new function allow_kernel_signal which allows kernel threads to receive signals sent from the kernel, but continues to ignore all signals sent from userspace. This ensures the user space interface for cifs and drbd remain the same.
These kernel threads depend on blocking networking calls which block until something is received or a signal is pending. Making receiving of signals somewhat necessary for these kernel threads. Perhaps someday we can cleanup those interfaces and remove allow_kernel_signal. If not allow_kernel_signal is pretty trivial and clearly documents what is going on so I don't think we will mind carrying it.
Eric W. Biederman (1): signal: Allow cifs and drbd to receive their terminating signals
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 9bd4ddd12b25..5b248763a672 100644 --- a/drivers/block/drbd/drbd_main.c +++ b/drivers/block/drbd/drbd_main.c @@ -322,6 +322,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 a15a6e738eb5..1795e80cbdf7 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -1113,7 +1113,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 b5d99482d3fe..1a5f88316b08 100644 --- a/include/linux/signal.h +++ b/include/linux/signal.h @@ -282,6 +282,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) { /* @@ -289,7 +292,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 e667be6907d7..534fec266a33 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -90,6 +90,11 @@ static bool sig_task_ignored(struct task_struct *t, int sig, bool force) handler == SIG_DFL && !(force && sig_kernel_only(sig))) return true;
+ /* 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); }
The pull request you sent on Mon, 19 Aug 2019 17:03:01 -0500:
git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git siginfo-linus
has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/287c55ed7df531c30f7a5093120339193dc7f166
Thank you!
linux-stable-mirror@lists.linaro.org
-
Christoph Böhmwalder -
David Laight -
ebiederm＠xmission.com -
Philipp Reisner -
pr-tracker-bot＠kernel.org