linux-kernel - RE: [PATCH] signal: remove the wrong signal_pending() check in restore_user

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 for Android: free password hash cracker in your pocket

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

Message-ID: <263d0e478ee447d9aa10baab0d8673a5@AcuMS.aculab.com>
Date:   Wed, 5 Jun 2019 09:02:54 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Linus Torvalds' <torvalds@...ux-foundation.org>,
        Oleg Nesterov <oleg@...hat.com>
CC:     Andrew Morton <akpm@...ux-foundation.org>,
        Deepa Dinamani <deepa.kernel@...il.com>,
        Linux List Kernel Mailing <linux-kernel@...r.kernel.org>,
        Arnd Bergmann <arnd@...db.de>,
        "Davidlohr Bueso" <dbueso@...e.de>, Jens Axboe <axboe@...nel.dk>,
        Davidlohr Bueso <dave@...olabs.net>,
        "e@...24.org" <e@...24.org>, Jason Baron <jbaron@...mai.com>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        "linux-aio@...ck.org" <linux-aio@...ck.org>,
        "omar.kilani@...il.com" <omar.kilani@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        stable <stable@...r.kernel.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        "Eric W. Biederman" <ebiederm@...ssion.com>
Subject: RE: [PATCH] signal: remove the wrong signal_pending() check in
 restore_user_sigmask()

From: Linus Torvalds
> Sent: 04 June 2019 22:27
> Ugh. I htink this is correct, but I wish we had a better and more
> intuitive interface.
> 
> In particular, since restore_user_sigmask() basically wants to check
> for "signal_pending()" anyway (to decide if the mask should be
> restored by signal handling or by that function), I really get the
> feeling that a lot of these patterns like
> 
> > -       restore_user_sigmask(ksig.sigmask, &sigsaved);
> > -       if (signal_pending(current) && !ret)
> > +
> > +       interrupted = signal_pending(current);
> > +       restore_user_sigmask(ksig.sigmask, &sigsaved, interrupted);
> > +       if (interrupted && !ret)
> >                 ret = -ERESTARTNOHAND;
> 
> are wrong to begin with, and we really should aim for an interface
> which says "tell me whether you completed the system call, and I'll
> give you an error return if not".
> 
> How about we make restore_user_sigmask() take two return codes: the
> 'ret' we already have, and the return we would get if there is a
> signal pending and w're currently returning zero.
> 
> IOW, I think the above could become
> 
>         ret = restore_user_sigmask(ksig.sigmask, &sigsaved, ret, -ERESTARTHAND);
> 
> instead if we just made the right interface decision.

I think we should tell restore_user_sigmask() whether it should
cause any signal handles to be run (using the current mask)
and it should tell us whether it would run any (ie if it deferred
restoring the mask to syscall exit).

So the above would (probably) be:
	if (restore_user_sigmask(ksig.sigmask, &sigsaved, true) && !ret)
		ret = -ERESTARTNOHAND;

epoll() would have:
	if (restore_user_sigmask(xxx.sigmask, &sigsaved, !ret || ret == -EINTR))
		ret = -EINTR;

I also think it could be simplified if code that loaded the 'user sigmask'
saved the old one in 'current->saved_sigmask' (and saved that it had done it).
You'd not need 'sigsaved' nor pass the user sigmask address into
the restore function.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)