* [PATCH] drbd: do not ignore signals in threads @ 2019-07-29 8:32 Christoph Böhmwalder 2019-07-29 8:50 ` David Laight 0 siblings, 1 reply; 11+ messages in thread From: Christoph Böhmwalder @ 2019-07-29 8:32 UTC (permalink / raw) To: linux-kernel Cc: Eric W . Biederman, Jens Axboe, Christoph Böhmwalder, Philipp Reisner, stable 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); -- 2.22.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* RE: [PATCH] drbd: do not ignore signals in threads 2019-07-29 8:32 [PATCH] drbd: do not ignore signals in threads Christoph Böhmwalder @ 2019-07-29 8:50 ` David Laight 2019-08-05 9:33 ` Christoph Böhmwalder 0 siblings, 1 reply; 11+ messages in thread From: David Laight @ 2019-07-29 8:50 UTC (permalink / raw) To: 'Christoph Böhmwalder', linux-kernel Cc: Eric W . Biederman, Jens Axboe, Philipp Reisner, stable 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) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drbd: do not ignore signals in threads 2019-07-29 8:50 ` David Laight @ 2019-08-05 9:33 ` Christoph Böhmwalder 2019-08-05 9:41 ` David Laight 0 siblings, 1 reply; 11+ messages in thread From: Christoph Böhmwalder @ 2019-08-05 9:33 UTC (permalink / raw) To: David Laight, Jens Axboe Cc: linux-kernel, Philipp Reisner, stable, Eric W . Biederman 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH] drbd: do not ignore signals in threads 2019-08-05 9:33 ` Christoph Böhmwalder @ 2019-08-05 9:41 ` David Laight 2019-08-12 11:52 ` Philipp Reisner 0 siblings, 1 reply; 11+ messages in thread From: David Laight @ 2019-08-05 9:41 UTC (permalink / raw) To: 'Christoph Böhmwalder', Jens Axboe Cc: linux-kernel, Philipp Reisner, stable, Eric W . Biederman 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) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drbd: do not ignore signals in threads 2019-08-05 9:41 ` David Laight @ 2019-08-12 11:52 ` Philipp Reisner 2019-08-12 13:12 ` David Laight 0 siblings, 1 reply; 11+ messages in thread From: Philipp Reisner @ 2019-08-12 11:52 UTC (permalink / raw) To: Jens Axboe Cc: David Laight, 'Christoph Böhmwalder', linux-kernel, stable, Eric W . Biederman 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) -- LINBIT | Keeping The Digital World Running DRBD® and LINBIT® are registered trademarks of LINBIT, Austria. ^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH] drbd: do not ignore signals in threads 2019-08-12 11:52 ` Philipp Reisner @ 2019-08-12 13:12 ` David Laight 2019-08-12 13:28 ` Philipp Reisner 0 siblings, 1 reply; 11+ messages in thread From: David Laight @ 2019-08-12 13:12 UTC (permalink / raw) To: 'Philipp Reisner', Jens Axboe Cc: 'Christoph Böhmwalder', linux-kernel, stable, Eric W . Biederman 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) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drbd: do not ignore signals in threads 2019-08-12 13:12 ` David Laight @ 2019-08-12 13:28 ` Philipp Reisner 2019-08-16 22:19 ` [PATCH] signal: Allow cifs and drbd to receive their terminating signals Eric W. Biederman 0 siblings, 1 reply; 11+ messages in thread From: Philipp Reisner @ 2019-08-12 13:28 UTC (permalink / raw) To: David Laight Cc: Jens Axboe, 'Christoph Böhmwalder', linux-kernel, stable, Eric W . Biederman 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; } } -- LINBIT | Keeping The Digital World Running DRBD® and LINBIT® are registered trademarks of LINBIT, Austria. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] signal: Allow cifs and drbd to receive their terminating signals 2019-08-12 13:28 ` Philipp Reisner @ 2019-08-16 22:19 ` Eric W. Biederman 2019-08-19 8:37 ` Christoph Böhmwalder 0 siblings, 1 reply; 11+ messages in thread From: Eric W. Biederman @ 2019-08-16 22:19 UTC (permalink / raw) To: Philipp Reisner Cc: David Laight, Jens Axboe, 'Christoph Böhmwalder', linux-kernel, stable, Steve French, ronnie sahlberg, Jeff Layton, linux-cifs 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); } -- 2.21.0.dirty Eric ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] signal: Allow cifs and drbd to receive their terminating signals 2019-08-16 22:19 ` [PATCH] signal: Allow cifs and drbd to receive their terminating signals Eric W. Biederman @ 2019-08-19 8:37 ` Christoph Böhmwalder 2019-08-19 22:03 ` [GIT PULL] " Eric W. Biederman 0 siblings, 1 reply; 11+ messages in thread From: Christoph Böhmwalder @ 2019-08-19 8:37 UTC (permalink / raw) To: Eric W. Biederman Cc: Philipp Reisner, David Laight, Jens Axboe, linux-kernel, stable, Steve French, ronnie sahlberg, Jeff Layton, linux-cifs 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [GIT PULL] signal: Allow cifs and drbd to receive their terminating signals 2019-08-19 8:37 ` Christoph Böhmwalder @ 2019-08-19 22:03 ` Eric W. Biederman 2019-08-19 23:35 ` pr-tracker-bot 0 siblings, 1 reply; 11+ messages in thread From: Eric W. Biederman @ 2019-08-19 22:03 UTC (permalink / raw) To: Linus Torvalds Cc: Philipp Reisner, David Laight, Jens Axboe, linux-kernel, stable, Steve French, ronnie sahlberg, Jeff Layton, linux-cifs, Christoph Böhmwalder, Oleg Nesterov 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); } -- ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [GIT PULL] signal: Allow cifs and drbd to receive their terminating signals 2019-08-19 22:03 ` [GIT PULL] " Eric W. Biederman @ 2019-08-19 23:35 ` pr-tracker-bot 0 siblings, 0 replies; 11+ messages in thread From: pr-tracker-bot @ 2019-08-19 23:35 UTC (permalink / raw) To: Eric W. Biederman Cc: Linus Torvalds, Philipp Reisner, David Laight, Jens Axboe, linux-kernel, stable, Steve French, ronnie sahlberg, Jeff Layton, linux-cifs, Christoph Böhmwalder, Oleg Nesterov 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! -- Deet-doot-dot, I am a bot. https://korg.wiki.kernel.org/userdoc/prtracker ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-08-19 23:35 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-29 8:32 [PATCH] drbd: do not ignore signals in threads Christoph Böhmwalder 2019-07-29 8:50 ` David Laight 2019-08-05 9:33 ` Christoph Böhmwalder 2019-08-05 9:41 ` David Laight 2019-08-12 11:52 ` Philipp Reisner 2019-08-12 13:12 ` David Laight 2019-08-12 13:28 ` Philipp Reisner 2019-08-16 22:19 ` [PATCH] signal: Allow cifs and drbd to receive their terminating signals Eric W. Biederman 2019-08-19 8:37 ` Christoph Böhmwalder 2019-08-19 22:03 ` [GIT PULL] " Eric W. Biederman 2019-08-19 23:35 ` pr-tracker-bot
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).