linux-kernel - Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <87v9xayh27.fsf@xmission.com>
Date:   Wed, 12 Jun 2019 10:11:28 -0500
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     David Laight <David.Laight@...LAB.COM>
Cc:     'Oleg Nesterov' <oleg@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Deepa Dinamani <deepa.kernel@...il.com>,
        "linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>,
        "arnd\@arndb.de" <arnd@...db.de>,
        "dbueso\@suse.de" <dbueso@...e.de>,
        "axboe\@kernel.dk" <axboe@...nel.dk>,
        "dave\@stgolabs.net" <dave@...olabs.net>,
        "e\@80x24.org" <e@...24.org>,
        "jbaron\@akamai.com" <jbaron@...mai.com>,
        "linux-fsdevel\@vger.kernel.org" <linux-fsdevel@...r.kernel.org>,
        "linux-aio\@kvack.org" <linux-aio@...ck.org>,
        "omar.kilani\@gmail.com" <omar.kilani@...il.com>,
        "tglx\@linutronix.de" <tglx@...utronix.de>,
        Al Viro <viro@...IV.linux.org.uk>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        "linux-arch\@vger.kernel.org" <linux-arch@...r.kernel.org>
Subject: Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask

David Laight <David.Laight@...LAB.COM> writes:

> From: Oleg Nesterov
>> Sent: 12 June 2019 14:46
>> On 06/11, David Laight wrote:
>> >
>> > If I have an application that has a loop with a pselect call that
>> > enables SIGINT (without a handler) and, for whatever reason,
>> > one of the fd is always 'ready' then I'd expect a SIGINT
>> > (from ^C) to terminate the program.

I think this gets into a quality of implementation.

I suspect that set_user_sigmask should do:
if (signal_pending())
	return -ERESTARNOSIGHAND; /* -EINTR that restarts if nothing was pending */

Which should be safe as nothing has blocked yet to consume any of the
timeouts, and it should ensure that none of the routines miss a signal.

>> This was never true.
>> 
>> Before Eric's patches SIGINT can kill a process or not, depending on timing.
>> In particular, if SIGINT was already pending before pselect() and it finds
>> an already ready fd, the program won't terminate.
>
> Which matches what I see on a very old Linux system.
>
>> After the Eric's patches SIGINT will only kill the program if pselect() does
>> not find a ready fd.
>> 
>> And this is much more consistent. Now we can simply say that the signal will
>> be delivered only if pselect() fails and returns -EINTR. If it doesn't have
>> a handler the process will be killed, otherwise the handler will be called.
>
> But is it what the standards mandate?
> Can anyone check how Solaris and any of the BSDs behave?
> I don't have access to any solaris systems (I doubt I'll get the disk to
> spin on the one in my garage).
> I can check NetBSD when I get home.
>
> The ToG page for pselect() http://pubs.opengroup.org/onlinepubs/9699919799/functions/pselect.html says:
>     "If sigmask is not a null pointer, then the pselect() function shall replace
>     the signal mask of the caller by the set of signals pointed to by sigmask
>     before examining the descriptors, and shall restore the signal mask of the
>     calling thread before returning."
> Note that it says 'before examining the descriptors' not 'before blocking'.
> Under the general description about signals it also says that the signal handler
> will be called (or other action happen) when a pending signal is unblocked.
> So unblocking SIGINT (set to SIG_DFL) prior to examining the descriptors
> should be enough to cause the process to exit.
> The fact that signal handlers are not called until 'return to user'
> is really an implementation choice - but (IMHO) it should appear as if they
> were called at the time they became unmasked.
>
> If nothing else the man pages need a note about the standards and portability.

I think the entire point is so that signals can be added to an event
loop in a race free way.  *cough* signalfd *cough*.  I think if you are
setting SIGINT to SIG_DFL you would leave SIGINT unblocked so it can
happen anywhere.

I can see more utility in having a SIGINT handler and you block SIGINT
except in your polling loop so you can do something deterministic when
SIGINT comes in.

Which makes it independent of my patches, but still worth fixing.

Eric