On Wed, 29 Jan 2020, Eric W. Biederman wrote:
Sasha Levin <sashal@xxxxxxxxxx> writes:
On Wed, Jan 29, 2020 at 12:36:43PM +0100, Greg Kroah-Hartman wrote:
On Wed, Jan 29, 2020 at 12:10:47PM +0100, Thomas Voegtle wrote:
On Tue, 28 Jan 2020, Greg Kroah-Hartman wrote:
From: Eric W. Biederman <ebiederm@xxxxxxxxxxxx>
[ 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@xxxxxxxxx>
Reported-by: Christoph Böhmwalder <christoph.boehmwalder@xxxxxxxxxx>
Tested-by: Christoph Böhmwalder <christoph.boehmwalder@xxxxxxxxxx>
Cc: Steve French <smfrench@xxxxxxxxx>
Cc: Philipp Reisner <philipp.reisner@xxxxxxxxxx>
Cc: David Laight <David.Laight@xxxxxxxxxx>
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")
These two commits come with that release, but...
Fixes: fee109901f39 ("signal/drbd: Use send_sig not force_sig")
Fixes: 3cf5d076fb4d ("signal: Remove task parameter from force_sig")
...these two commits not and were never added to 4.9.y.
Are these both really not needed?
I don't think so, do you feel otherwise?
Both of those commits read as a cleanup to me. I've actually slightly
modified to patch to not need those commits (they were less than trivial
to backport as is).
All of these changes were cleanup. Which is why I didn't tag any of
them for stable.
Not to say that there weren't real problems using force_sig instead
of send_sig. force_sig does nothing to ensure the task it is sending
signals to won't, and hasn't gone away. Which is why it is a bad
idea to use force_sig on anything but current. As I recall drbd used
force_sig on a kernel_thread which didn't go away.
When fixing the force_sig vs send_sig confusion I didn't realize that
some places were using force_sig because they had not enabled receiving
the signals they depended on. Which is where allow_kernel_signal comes
from. But while using force_sig allow_kernel_signal is not necessary.
Eric
Thanks for clarification.
Thomas
