Messages in this thread | | | Date | Sun, 25 Oct 2015 14:27:54 +0100 | From | Oleg Nesterov <> | 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.
| |