Re: [PATCH] drbd: do not ignore signals in threads

[Philipp Reisner <philipp.reisner@linbit.com>]

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.