linux-kernel - Re: + signal-turn-dequeue_signal_lock-into-kernel_dequeue

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Sun, 25 Oct 2015 14:27:54 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Markus Pargmann <mpa@...gutronix.de>
Cc:	akpm@...ux-foundation.org, balbi@...com, dwmw2@...radead.org,
	tj@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: +
	signal-turn-dequeue_signal_lock-into-kernel_dequeue_signal.patch
	added to -mm tree

On 10/24, Markus Pargmann wrote:
>
> Hi Oleg,
>
> On Sat, Oct 24, 2015 at 09:48:26PM +0200, Oleg Nesterov wrote:
> >
> > Thanks! I'll send *-fix.patch to Andrew.

I'll send it in a minute, could you please review?

> > But you know, dcc909d90ccd (nbd: Add locking for tasks) doesn't look exactly
> > right at first glance, although I need to re-check tomorrow...
>
> In which regard? Is the locking incorrect or am I doing something wrong
> with the signal handling?

I'll probably write another email...

But lets start with force_sig(SIGKILL, nbd->task_send) and nbd_thread_send().
Why do we need force_sig(p) ? Note that it assumes that p == current (yes, it
has other buggy users iirc). That is why it doesn't use lock_task_signand().
Note also that it clears SIGNAL_UNKILLABLE, this is not what we want. Although
I guess this doesn't really matters in this particular case, but still this
doesn't look right.

So why do we need force_sig() ? May be because we want to wake ->task_send up
even if ignores SIGKILL because it is a kernel thread? In this case, shouldn't
we change nbd_thread_send() to simply do allow_signal(SIGKILL) at the start
and change nbd_xmit_timeout() to do send_sig_info(SIGKILL)?

Or this can not work because we do not want to react to SIGKILL from user-
space?

Also. dcc909d90ccd adds /* Clear maybe pending signals */ at the end,

	if (signal_pending(current)) {
		dequeue_signal_lock(...);
	}

for what? This kthread is going to exit, the pending signal is fine.

Finally. kthread_run() + kthread_stop() looks "obviously racy", but perhaps
I missed something... I'll send another patch, kthread_get_run() can have
other users.

Now lets look at nbd_thread_recv(). This one is called by ioctl() and
thus (I think) from user-space, right? This means that we do not need
force_sig(nbd->task_recv), a user-mode task can't block/ignore SIGKILL
so send_sig_info() should work just fine. But this is minor.

Note that nbd_thread_recv() dequeues and throws out the "random" signal
before it returns, this can not be right (again, if called by a user-
mode task).

> > One question, can sock_xmit() be called from user space? Or it is only called
> > by kthreads?
>
> sock_xmit() can be called by a thread that entered from userspace. In
> general the idea is that there are no pending signals when it leaves
> into userspace again.

OK, thanks. This means that at least the comment is wrong. We can not
really block SIGSTOP if the caller is multithreaded. Hmm, git blame shows
be0ef957 (nbd.c: sock_xmit: cleanup signal related code) from me ;) but
that commit didn't change the behaviour.

I'll try to send a couple of cleanups today, but it seems that this
code needs more, or I am totally confused.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/