Re: + signal-turn-dequeue_signal_lock-into-kernel_dequeue_signal.patch added to -mm tree

Re: + signal-turn-dequeue_signal_lock-into-kernel_dequeue_signal.patch added to -mm tree

[Oleg Nesterov]

Hi Markus,

s/mm-commits/lkml/

On 10/24, Markus Pargmann wrote:
>
> On Mon, Oct 05, 2015 at 02:19:27PM -0700, akpm@linux-foundation.org wrote:
> >
> > Subject: signal: turn dequeue_signal_lock() into kernel_dequeue_signal()
> >
> > 1. Rename dequeue_signal_lock() to kernel_dequeue_signal(). This
> >    matches another "for kthreads only" kernel_sigaction() helper.
> >
> > 2. Remove the "tsk" and "mask" arguments, they are always current
> >    and current->blocked. And it is simply wrong if tsk != current.
> >
> > 3. We could also remove the 3rd "siginfo_t *info" arg but it looks
> >    potentially useful. However we can simplify the callers if we
> >    change kernel_dequeue_signal() to accept info => NULL.
> >
> > 4. Remove _irqsave, it is never called from atomic context.
>
> I just realised that this patch will conflict with a fixup patch for nbd
> that will be included in rc7.
>
> dcc909d90ccd (nbd: Add locking for tasks)
>
> I think there is basically one new instance of dequeue_signal_lock() that
> needs to be replaced with kernel_dequeue_signal().

Thanks! I'll send *-fix.patch to Andrew.

But you know, dcc909d90ccd (nbd: Add locking for tasks) doesn't look exactly
right at first glance, although I need to re-check tomorrow...

One question, can sock_xmit() be called from user space? Or it is only called
by kthreads?

Oleg.

Re: + signal-turn-dequeue_signal_lock-into-kernel_dequeue_signal.patch added to -mm tree

[Markus Pargmann]

[-- Attachment #1: Type: text/plain, Size: 2143 bytes --]

Hi Oleg,

On Sat, Oct 24, 2015 at 09:48:26PM +0200, Oleg Nesterov wrote:
> Hi Markus,
> 
> s/mm-commits/lkml/
> 
> On 10/24, Markus Pargmann wrote:
> >
> > On Mon, Oct 05, 2015 at 02:19:27PM -0700, akpm@linux-foundation.org wrote:
> > >
> > > Subject: signal: turn dequeue_signal_lock() into kernel_dequeue_signal()
> > >
> > > 1. Rename dequeue_signal_lock() to kernel_dequeue_signal(). This
> > >    matches another "for kthreads only" kernel_sigaction() helper.
> > >
> > > 2. Remove the "tsk" and "mask" arguments, they are always current
> > >    and current->blocked. And it is simply wrong if tsk != current.
> > >
> > > 3. We could also remove the 3rd "siginfo_t *info" arg but it looks
> > >    potentially useful. However we can simplify the callers if we
> > >    change kernel_dequeue_signal() to accept info => NULL.
> > >
> > > 4. Remove _irqsave, it is never called from atomic context.
> >
> > I just realised that this patch will conflict with a fixup patch for nbd
> > that will be included in rc7.
> >
> > dcc909d90ccd (nbd: Add locking for tasks)
> >
> > I think there is basically one new instance of dequeue_signal_lock() that
> > needs to be replaced with kernel_dequeue_signal().
> 
> Thanks! I'll send *-fix.patch to Andrew.
> 
> 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?

> 
> 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.

Best Regards,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: + signal-turn-dequeue_signal_lock-into-kernel_dequeue_signal.patch added to -mm tree

[Oleg Nesterov]

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.