linux-arch.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: ebiederm@xmission.com (Eric W. Biederman)
To: David Laight <David.Laight@ACULAB.COM>
Cc: 'Oleg Nesterov' <oleg@redhat.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Deepa Dinamani <deepa.kernel@gmail.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"arnd@arndb.de" <arnd@arndb.de>,
	"dbueso@suse.de" <dbueso@suse.de>,
	"axboe@kernel.dk" <axboe@kernel.dk>,
	"dave@stgolabs.net" <dave@stgolabs.net>,
	"e@80x24.org" <e@80x24.org>,
	"jbaron@akamai.com" <jbaron@akamai.com>,
	"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
	"linux-aio@kvack.org" <linux-aio@kvack.org>,
	"omar.kilani@gmail.com" <omar.kilani@gmail.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	Al Viro <viro@ZenIV.linux.org.uk>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	"linux-arch@vger.kernel.org" <linux-arch@vger.kernel.org>
Subject: Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask
Date: Wed, 12 Jun 2019 08:09:17 -0500	[thread overview]
Message-ID: <874l4v0x36.fsf@xmission.com> (raw)
In-Reply-To: <fd2aab3d26754becbb0efe4ae65c32ac@AcuMS.aculab.com> (David Laight's message of "Wed, 12 Jun 2019 08:39:53 +0000")

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

> From: Oleg Nesterov [mailto:oleg@redhat.com]
>> Sent: 11 June 2019 19:56
>> On 06/10, Eric W. Biederman wrote:
>> >
>> > Personally I don't think anyone sane would intentionally depend on this
>> > and I don't think there is a sufficiently reliable way to depend on this
>> > by accident that people would actually be depending on it.
>> 
>> Agreed.
>> 
>> As I said I like these changes and I see nothing wrong. To me they fix the
>> current behaviour, or at least make it more consistent.
>> 
>> But perhaps this should be documented in the changelog? To make it clear
>> that this change was intentional.
>
> What happens if you run the test program I posted yesterday after the changes?
>
> It looks like pselect() and epoll_pwait() operated completely differently.
> pselect() would always calls the signal handlers.
> epoll_pwait() only calls them when EINTR is returned.
> So changing epoll_pwait() and pselect() to work the same way
> is bound to break some applications.

That is not the change we are discussing.  We are looking at making
pselect and epoll_pwait act a little more like sigtimedwait.

In particular we are discussiong signals whose handler is SIG_DFL and
whose default disposition is to kill the process, such as SIGINT.

When those signals are delivered and they are not blocked, we take
an optimized path in complete_signal and start the process tear down.

That early start of process tear down does not happen if the signal is
blocked or it happens to be in real_blocked (from sigtimedwait).

This matters is the small time window where the signal is received while
the process has temporarily unblocked the signal, and the signal is not
detected by the code and blocked and oridinarily would not be delivered
until next time because of restore_sigmask_unless.

If the tear down has started early.  Even if we would not have returned
the process normally the signal can kill the process.  AKA epoll_pwait
can today result in it's caller being killed without -EINTR being
returned.

My change fixes that behavior and disables the early process tear down
for those signals, by having real_blocked match the set of signals
that are normally blocked by the process.  The result should be
the signal will have to wait for the next call that temporarily
unblocked the process.

The real benefit is that that the code becomes more comprehensible.

It is my patch that titled: "signal: Always keep real_blocked in sync
with blocked" that causes that to happen.

Eric

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

WARNING: multiple messages have this Message-ID (diff)
From: ebiederm@xmission.com (Eric W. Biederman)
To: David Laight <David.Laight@ACULAB.COM>
Cc: 'Oleg Nesterov' <oleg@redhat.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Deepa Dinamani <deepa.kernel@gmail.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"arnd@arndb.de" <arnd@arndb.de>,
	"dbueso@suse.de" <dbueso@suse.de>,
	"axboe@kernel.dk" <axboe@kernel.dk>,
	"dave@stgolabs.net" <dave@stgolabs.net>,
	"e@80x24.org" <e@80x24.org>,
	"jbaron@akamai.com" <jbaron@akamai.com>,
	"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
	"linux-aio@kvack.org" <linux-aio@kvack.org>,
	"omar.kilani@gmail.com" <omar.kilani@gmail.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	Al Viro <viro@ZenIV.linux.org.uk>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	"linux-arch@vger.kernel.org" <linux-arch@vger.kernel.org>
Subject: Re: [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask
Date: Wed, 12 Jun 2019 08:09:17 -0500	[thread overview]
Message-ID: <874l4v0x36.fsf@xmission.com> (raw)
Message-ID: <20190612130917.NvpMSZ55CcZ1jKgGNbt2QegdNz1RmEjU4FJBVjbv4CU@z> (raw)
In-Reply-To: <fd2aab3d26754becbb0efe4ae65c32ac@AcuMS.aculab.com> (David Laight's message of "Wed, 12 Jun 2019 08:39:53 +0000")

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

> From: Oleg Nesterov [mailto:oleg@redhat.com]
>> Sent: 11 June 2019 19:56
>> On 06/10, Eric W. Biederman wrote:
>> >
>> > Personally I don't think anyone sane would intentionally depend on this
>> > and I don't think there is a sufficiently reliable way to depend on this
>> > by accident that people would actually be depending on it.
>> 
>> Agreed.
>> 
>> As I said I like these changes and I see nothing wrong. To me they fix the
>> current behaviour, or at least make it more consistent.
>> 
>> But perhaps this should be documented in the changelog? To make it clear
>> that this change was intentional.
>
> What happens if you run the test program I posted yesterday after the changes?
>
> It looks like pselect() and epoll_pwait() operated completely differently.
> pselect() would always calls the signal handlers.
> epoll_pwait() only calls them when EINTR is returned.
> So changing epoll_pwait() and pselect() to work the same way
> is bound to break some applications.

That is not the change we are discussing.  We are looking at making
pselect and epoll_pwait act a little more like sigtimedwait.

In particular we are discussiong signals whose handler is SIG_DFL and
whose default disposition is to kill the process, such as SIGINT.

When those signals are delivered and they are not blocked, we take
an optimized path in complete_signal and start the process tear down.

That early start of process tear down does not happen if the signal is
blocked or it happens to be in real_blocked (from sigtimedwait).

This matters is the small time window where the signal is received while
the process has temporarily unblocked the signal, and the signal is not
detected by the code and blocked and oridinarily would not be delivered
until next time because of restore_sigmask_unless.

If the tear down has started early.  Even if we would not have returned
the process normally the signal can kill the process.  AKA epoll_pwait
can today result in it's caller being killed without -EINTR being
returned.

My change fixes that behavior and disables the early process tear down
for those signals, by having real_blocked match the set of signals
that are normally blocked by the process.  The result should be
the signal will have to wait for the next call that temporarily
unblocked the process.

The real benefit is that that the code becomes more comprehensible.

It is my patch that titled: "signal: Always keep real_blocked in sync
with blocked" that causes that to happen.

Eric

  parent reply	other threads:[~2019-06-12 13:09 UTC|newest]

Thread overview: 60+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20190522032144.10995-1-deepa.kernel@gmail.com>
     [not found] ` <20190529161157.GA27659@redhat.com>
     [not found]   ` <20190604134117.GA29963@redhat.com>
     [not found]     ` <20190606140814.GA13440@redhat.com>
2019-06-07 21:39       ` [RFC PATCH 0/5]: Removing saved_sigmask Eric W. Biederman
2019-06-07 21:39         ` Eric W. Biederman
2019-06-07 21:41         ` [RFC PATCH 1/5] signal: Teach sigsuspend to use set_user_sigmask Eric W. Biederman
2019-06-07 21:41           ` Eric W. Biederman
2019-06-07 22:07           ` Linus Torvalds
2019-06-07 22:07             ` Linus Torvalds
2019-06-10 16:22           ` Oleg Nesterov
2019-06-10 16:22             ` Oleg Nesterov
2019-06-10 21:20             ` Eric W. Biederman
2019-06-10 21:20               ` Eric W. Biederman
2019-06-11  9:52               ` David Laight
2019-06-11  9:52                 ` David Laight
2019-06-11 11:14                 ` David Laight
2019-06-11 11:14                   ` David Laight
2019-06-12 12:55                   ` Eric W. Biederman
2019-06-12 12:55                     ` Eric W. Biederman
2019-06-12 13:24                     ` David Laight
2019-06-12 13:24                       ` David Laight
2019-06-12 13:35                       ` Oleg Nesterov
2019-06-12 13:35                         ` Oleg Nesterov
2019-06-12 13:39                         ` David Laight
2019-06-12 13:39                           ` David Laight
2019-06-11 15:46                 ` David Laight
2019-06-11 15:46                   ` David Laight
2019-06-12 12:40                   ` Eric W. Biederman
2019-06-12 12:40                     ` Eric W. Biederman
2019-06-12 13:45                 ` Oleg Nesterov
2019-06-12 13:45                   ` Oleg Nesterov
2019-06-12 14:18                   ` David Laight
2019-06-12 14:18                     ` David Laight
2019-06-12 15:11                     ` Eric W. Biederman
2019-06-12 15:11                       ` Eric W. Biederman
2019-06-12 15:37                       ` Oleg Nesterov
2019-06-12 15:37                         ` Oleg Nesterov
2019-06-13  8:48                     ` David Laight
2019-06-13  8:48                       ` David Laight
2019-06-13  9:43                       ` Oleg Nesterov
2019-06-13  9:43                         ` Oleg Nesterov
2019-06-13 10:56                         ` David Laight
2019-06-13 10:56                           ` David Laight
2019-06-13 12:43                           ` Oleg Nesterov
2019-06-13 12:43                             ` Oleg Nesterov
2019-06-11 18:55               ` Oleg Nesterov
2019-06-11 18:55                 ` Oleg Nesterov
2019-06-11 19:02                 ` Eric W. Biederman
2019-06-11 19:02                   ` Eric W. Biederman
2019-06-12  8:39                 ` David Laight
2019-06-12  8:39                   ` David Laight
2019-06-12 13:09                   ` Eric W. Biederman [this message]
2019-06-12 13:09                     ` Eric W. Biederman
2019-06-07 21:41         ` [RFC PATCH 2/5] signal/kvm: Stop using sigprocmask in kvm_sigset_(activate|deactivate) Eric W. Biederman
2019-06-07 21:41           ` Eric W. Biederman
2019-06-07 21:42         ` [RFC PATCH 3/5] signal: Always keep real_blocked in sync with blocked Eric W. Biederman
2019-06-07 21:42           ` Eric W. Biederman
2019-06-07 21:43         ` [RFC PATCH 4/5] signal: Remove saved_sigmask Eric W. Biederman
2019-06-07 21:43           ` Eric W. Biederman
2019-06-07 21:44         ` [RFC PATCH 5/5] signal: Remove the unnecessary restore_sigmask flag Eric W. Biederman
2019-06-07 21:44           ` Eric W. Biederman
2019-06-11 18:58         ` [RFC PATCH 0/5]: Removing saved_sigmask Oleg Nesterov
2019-06-11 18:58           ` Oleg Nesterov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=874l4v0x36.fsf@xmission.com \
    --to=ebiederm@xmission.com \
    --cc=David.Laight@ACULAB.COM \
    --cc=akpm@linux-foundation.org \
    --cc=arnd@arndb.de \
    --cc=axboe@kernel.dk \
    --cc=dave@stgolabs.net \
    --cc=dbueso@suse.de \
    --cc=deepa.kernel@gmail.com \
    --cc=e@80x24.org \
    --cc=jbaron@akamai.com \
    --cc=linux-aio@kvack.org \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=oleg@redhat.com \
    --cc=omar.kilani@gmail.com \
    --cc=tglx@linutronix.de \
    --cc=torvalds@linux-foundation.org \
    --cc=viro@ZenIV.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).