block_all_signals() usage in DRM

block_all_signals() usage in DRM

[Richard Weinberger]

Hi!

drivers/gpu/drm/drm_lock.c is the only remaining user of block_all_signals():
        /* don't set the block all signals on the master process for now
         * really probably not the correct answer but lets us debug xkb
         * xserver for now */
        if (!file_priv->is_master) {
                sigemptyset(&dev->sigmask);
                sigaddset(&dev->sigmask, SIGSTOP);
                sigaddset(&dev->sigmask, SIGTSTP);
                sigaddset(&dev->sigmask, SIGTTIN);
                sigaddset(&dev->sigmask, SIGTTOU);
                dev->sigdata.context = lock->context;
                dev->sigdata.lock = master->lock.hw_lock;
                block_all_signals(drm_notifier, dev, &dev->sigmask);
        }

Is this functionality still in use/needed?
Otherwise we could get rid of block_all_signals() and unpuzzle the signaling
code a bit. :-)

Thanks,
//richard

Re: block_all_signals() usage in DRM

[Oleg Nesterov]

AAAAOn 05/25, Richard Weinberger wrote:
>
> Is this functionality still in use/needed?

All I can say it doesn't work.

> Otherwise we could get rid of block_all_signals() and unpuzzle the signaling
> code a bit. :-)

Yes. I do not even remember when I reported this the first time. Perhaps
more than 10 years ago.

See the last attempt in 2011: https://lkml.org/lkml/2011/7/12/263
I copied this email below.

Dave. Lets finally kill this horror? I am going to send a patch unless
you stop me ;)

Oleg.

-------------------------------------------------------------------------------
I tried many times to ask about the supposed behaviour of
block_all_signals() in drm, but it seems nobody can answer.

So I am going to send the patch which simply removes
block_all_signals() and friends. There are numeruous problems
with this interace, I can't even enumerate them. But I think
that it is enough to mention that block_all_signals() simply
can not work. AT ALL. I am wondering, was it ever tested and
how.

So. ioctl()->drm_lock() "blocks" the stop signals. Probably to
ensure the task can't be stopped until it does DRM_IOCTL_UNLOCK.
And what does this mean? Yes, the task won't stop if it receives,
say, SIGTSTP. But! Instead it will loop forever in kernel mode
until it receives another unblocked/non-ignored signal which
should be numerically less than SIGSTOP.

Why do we need this? Once again. block_all_signals(SIGTSTP)
only means that the caller will burn cpu instead of sleeping
in TASK_STOPPED after ^Z. What is the point?

And once again, there are other problems. For example, even if
block_all_signals() actually blocked SIGSTOP/etc, this can not
help if the caller is multithreaded.

I strongly believe block_all_signals() should die. Given that
it doesn't work, could somebody please explain me what will
be broken?



Just in case... Please look at the debugging patch below. With
this patch,

	$ perl -le 'syscall 157,666 and die $!; sleep 1, print while ++$_'
	1
	2
	3
	^Z

Hang. So it does react to ^Z anyway, just it is looping in the
endless loop in the kernel. It can only look as if ^Z is ignored,
because obviously bash doesn't see it stopped.



Now lets look at drm_notifier(). If it returns 0 it does:

	/* Otherwise, set flag to force call to
	   drmUnlock */

drmUnlock? grep shows nothing...

	do {
		old = s->lock->lock;
		new = old | _DRM_LOCK_CONT;
		prev = cmpxchg(&s->lock->lock, old, new);
	} while (prev != old);
	return 0;

OK. So, if block_all_signals() makes any sense, it seems that this
is only because we add _DRM_LOCK_CONT.

Who checks _DRM_LOCK_CONT? _DRM_LOCK_IS_CONT(), but it has no users.
Hmm. Looks like via_release_futex() is the only user, but it doesn't
look as "force call to drmUnlock" and it is CONFIG_DRM_VIA only.


I am totally confused. But block_all_signals() should die anyway.

We can probably implement something like 'i-am-going-to-stop' or
even 'can-i-stop' per-thread notifiers, although this all looks
like the user-space problem to me (yes, I know absolutely nothing
about drm/etc).

If nothing else. We can change drm_lock/drm_unlock to literally
block/unblock SIGSTOP/etc (or perhaps we only should worry about
the signals from tty?). This is the awful hack and this can't work
with the multithreaded tasks too, but still it is better than what
we have now.

Oleg.

--- a/kernel/sys.c~	2011-06-16 20:12:18.000000000 +0200
+++ b/kernel/sys.c	2011-07-12 16:24:50.000000000 +0200
@@ -1614,6 +1614,11 @@ SYSCALL_DEFINE1(umask, int, mask)
 	return mask;
 }
 
+static int notifier(void *arg)
+{
+	return 0;
+}
+
 SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 		unsigned long, arg4, unsigned long, arg5)
 {
@@ -1627,6 +1632,13 @@ SYSCALL_DEFINE5(prctl, int, option, unsi
 
 	error = 0;
 	switch (option) {
+		case 666: {
+			sigset_t *pmask = kmalloc(sizeof(*pmask), GFP_KERNEL);
+			siginitset(pmask, sigmask(SIGTSTP));
+			block_all_signals(notifier, NULL, pmask);
+			break;
+		}
+
 		case PR_SET_PDEATHSIG:
 			if (!valid_signal(arg2)) {
 				error = -EINVAL;

Re: block_all_signals() usage in DRM

[Richard Weinberger]

Am 25.05.2015 um 18:50 schrieb Oleg Nesterov:
> AAAAOn 05/25, Richard Weinberger wrote:
>>
>> Is this functionality still in use/needed?
> 
> All I can say it doesn't work.
> 
>> Otherwise we could get rid of block_all_signals() and unpuzzle the signaling
>> code a bit. :-)
> 
> Yes. I do not even remember when I reported this the first time. Perhaps
> more than 10 years ago.
> 
> See the last attempt in 2011: https://lkml.org/lkml/2011/7/12/263
> I copied this email below.

Thank you Oleg, this makes sense.
I was actually wondering WTF this function is good for.

Thanks,
//richard

Re: block_all_signals() usage in DRM

[Dave Airlie]

On 26 May 2015 at 00:59, Richard Weinberger <richard@nod.at> wrote:
> Hi!
>
> drivers/gpu/drm/drm_lock.c is the only remaining user of block_all_signals():

It's the only user of it, ever. The API was introduced for the drm locking code.

No other user will ever exist. Just to clear up the an API exists with
one user, we should remove it.


>         /* don't set the block all signals on the master process for now
>          * really probably not the correct answer but lets us debug xkb
>          * xserver for now */
>         if (!file_priv->is_master) {
>                 sigemptyset(&dev->sigmask);
>                 sigaddset(&dev->sigmask, SIGSTOP);
>                 sigaddset(&dev->sigmask, SIGTSTP);
>                 sigaddset(&dev->sigmask, SIGTTIN);
>                 sigaddset(&dev->sigmask, SIGTTOU);
>                 dev->sigdata.context = lock->context;
>                 dev->sigdata.lock = master->lock.hw_lock;
>                 block_all_signals(drm_notifier, dev, &dev->sigmask);
>         }
>
> Is this functionality still in use/needed?
> Otherwise we could get rid of block_all_signals() and unpuzzle the signaling
> code a bit. :-)
>

the functionality is still used, but only on legacy systems, if ABI is
something we care about.

I'll follow up to Oleg mail, just wanted to clarify why there isn't other users.

Dave.

Re: block_all_signals() usage in DRM

[Dave Airlie]

On 26 May 2015 at 02:50, Oleg Nesterov <oleg@redhat.com> wrote:
> AAAAOn 05/25, Richard Weinberger wrote:
>>
>> Is this functionality still in use/needed?
>
> All I can say it doesn't work.
>
>> Otherwise we could get rid of block_all_signals() and unpuzzle the signaling
>> code a bit. :-)
>
> Yes. I do not even remember when I reported this the first time. Perhaps
> more than 10 years ago.
>
> See the last attempt in 2011: https://lkml.org/lkml/2011/7/12/263
> I copied this email below.
>
> Dave. Lets finally kill this horror? I am going to send a patch unless
> you stop me ;)

There were follow up on that thread 4 years ago, but we are probably
at the stage where this thing can die,

I suspect any hw using it has died out, and any new hardware  won't be
doing evil things with drm locks

Dave.