linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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).