From: Christian Brauner <christian@brauner.io>
To: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Daniel Colascione <dancol@google.com>,
linux-kernel <linux-kernel@vger.kernel.org>,
Linux API <linux-api@vger.kernel.org>,
Andy Lutomirski <luto@kernel.org>, Arnd Bergmann <arnd@arndb.de>,
"Serge E. Hallyn" <serge@hallyn.com>,
Jann Horn <jannh@google.com>,
Andrew Morton <akpm@linux-foundation.org>,
Oleg Nesterov <oleg@redhat.com>, Aleksa Sarai <cyphar@cyphar.com>,
Al Viro <viro@zeniv.linux.org.uk>,
Linux FS Devel <linux-fsdevel@vger.kernel.org>,
Tim Murray <timmurray@google.com>,
linux-man <linux-man@vger.kernel.org>,
Kees Cook <keescook@chromium.org>,
Florian Weimer <fweimer@redhat.com>,
tglx@linutronix.de, x86@kernel.org
Subject: Re: [PATCH v4] signal: add taskfd_send_signal() syscall
Date: Thu, 6 Dec 2018 18:41:31 +0100 [thread overview]
Message-ID: <20181206174129.taakmwekysbkaosu@brauner.io> (raw)
In-Reply-To: <878t12efg3.fsf@xmission.com>
On Thu, Dec 06, 2018 at 11:24:28AM -0600, Eric W. Biederman wrote:
> Daniel Colascione <dancol@google.com> writes:
>
> > On Thu, Dec 6, 2018 at 7:02 AM Eric W. Biederman <ebiederm@xmission.com> wrote:
> >>
> >> Christian Brauner <christian@brauner.io> writes:
> >>
> >> > The kill() syscall operates on process identifiers (pid). After a process
> >> > has exited its pid can be reused by another process. If a caller sends a
> >> > signal to a reused pid it will end up signaling the wrong process. This
> >> > issue has often surfaced and there has been a push [1] to address this
> >> > problem.
> >> >
> >> > This patch uses file descriptors (fd) from proc/<pid> as stable handles on
> >> > struct pid. Even if a pid is recycled the handle will not change. The fd
> >> > can be used to send signals to the process it refers to.
> >> > Thus, the new syscall taskfd_send_signal() is introduced to solve this
> >> > problem. Instead of pids it operates on process fds (taskfd).
> >>
> >> I am not yet thrilled with the taskfd naming.
> >
> > Both the old and new names were fine. Do you want to suggest a name at
> > this point? You can't just say "I don't like this. Guess again"
> > forever.
>
> Both names suck, as neither name actually describes what the function is
> designed to do.
>
> Most issues happen at the interface between abstractions. A name that
> confuses your users will just make that confusion more likely. So it is
> important that we do the very best with the name that we can do.
>
> We are already having questions about what happens when you perform the
> non-sense operation of sending a signal to a zombie. It comes up
> because there are races when a process may die and you are not expecting
> it. That is an issue with the existing signal sending API, that has
> caused confusion. That isn't half as confusing as the naming issue.
>
> A task in linux is a single thread. A process is all of the threads.
> If we are going to support both cases it doesn't make sense to hard code
> a single case in the name.
>
> I would be inclined to simplify things and call the syscall something
> like "fdkill(int fd, struct siginfo *info, int flags)". Or perhaps
No, definitely nothing with "kill" will be used because that's
absolutely not expressing what this syscall is doing.
> just "fd_send_signal(int fd, struct siginfo *info, int flags)".
>
> Then we are not overspecifying what the system call does in the name.
I feel changing the name around by a single persons preferences is not
really a nice thing to do community-wise. So I'd like to hear other
people chime in first before I make that change.
> Plus it makes it clear that the fd specifies where the signal goes.
> Something I see that by your reply below that you were confused about.
>
> >> Is there any plan to support sesssions and process groups?
> >
> > Such a thing could be added with flags in the future. Why complicate
> > this patch?
>
> Actually that isn't the way this is designed. You would have to have
> another kind of file descriptor. I am asking because it goes to the
> question of naming and what we are trying to do here.
>
> We don't need to implement that but we have already looked into this
> kind of extensibility. If we want the extensibility we should make
> room for it, or just close the door. Having the door half open and a
> confusing interface is a problem for users.
>
> >> I am concerned about using kill_pid_info. It does this:
> >>
> >>
> >> rcu_read_lock();
> >> p = pid_task(pid, PIDTYPE_PID);
> >> if (p)
> >> error = group_send_sig_info(sig, info, p, PIDTYPE_TGID);
> >> rcu_read_unlock();
> >>
> >> That pid_task(PIDTYPE_PID) is fine for existing callers that need bug
> >> compatibility. For new interfaces I would strongly prefer pid_task(PIDTYPE_TGID).
> >
> > What is the bug that PIDTYPE_PID preserves?
>
> I am not 100% certain all of the bits for this to matter have been
> merged yet but we are close enough that it would not be hard to make it
> matter.
>
> There are two strange behaviours of ordinary kill on the linux kernel
> that I am aware of.
>
> 1) kill(thread_id,...) where the thread_id is not the id of the first
> thread and the thread_id thus the pid of the process sends the signal
> to the entire process. Something that arguably should not happen.
>
> 2) kill(pid,...) where the original thread has exited performs the
> permission checks against the dead signal group leader. Which means
> that the permission checks for sending a signal are very likely wrong
> for a multi-threaded processes that calls a function like setuid.
>
> To fix the second case we are going to have to perform the permission
> checks on a non-zombie thread. That is something that is straight
> forward to make work with PIDTYPE_TGID. It is not so easy to make work
> with PIDTYPE_PID.
>
> I looked and it doesn't look like I have merged the logic of having
> PIDTYPE_TGID point to a living thread when the signal group leader
> exits and becomes a zombie. It isn't hard but it does require some very
> delicate surgery on the code, so that we don't break some of the
> historic confusion of threads and process groups.
Then this seems irrelevant to the current patch. It seems we can simply
switch to PIDTYPE_PGID once your new logic lands but not right now.
next prev parent reply other threads:[~2018-12-06 17:41 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-06 12:18 [PATCH v4] signal: add taskfd_send_signal() syscall Christian Brauner
2018-12-06 12:30 ` Florian Weimer
2018-12-06 12:45 ` Jürg Billeter
2018-12-06 13:12 ` Florian Weimer
2018-12-06 13:18 ` Jürg Billeter
2018-12-06 13:20 ` Florian Weimer
2018-12-06 13:40 ` Eric W. Biederman
2018-12-06 13:44 ` Florian Weimer
2018-12-06 14:27 ` Aleksa Sarai
2018-12-06 14:46 ` Eric W. Biederman
2018-12-06 12:53 ` Christian Brauner
2018-12-06 13:17 ` Florian Weimer
2018-12-06 15:01 ` Eric W. Biederman
2018-12-06 16:17 ` Daniel Colascione
2018-12-06 17:24 ` Eric W. Biederman
2018-12-06 17:41 ` Christian Brauner [this message]
2018-12-06 18:30 ` Kees Cook
2018-12-06 22:27 ` Serge E. Hallyn
2018-12-06 17:14 ` Christian Brauner
2018-12-06 19:17 ` Eric W. Biederman
2018-12-06 19:30 ` Christian Brauner
2018-12-06 20:29 ` Eric W. Biederman
2018-12-06 20:37 ` Daniel Colascione
2018-12-06 22:22 ` Eric W. Biederman
2018-12-06 22:43 ` Daniel Colascione
2018-12-06 21:31 ` Christian Brauner
2018-12-06 21:46 ` Eric W. Biederman
2018-12-06 22:01 ` Daniel Colascione
2018-12-06 22:39 ` Christian Brauner
2018-12-06 23:17 ` Christian Brauner
2018-12-07 0:31 ` Serge E. Hallyn
2018-12-07 0:34 ` Daniel Colascione
2018-12-07 0:59 ` Serge E. Hallyn
2018-12-07 1:39 ` Daniel Colascione
2018-12-07 1:54 ` Christian Brauner
2018-12-07 16:49 ` Serge E. Hallyn
2018-12-07 16:47 ` Serge E. Hallyn
2018-12-08 21:46 ` kbuild test robot
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=20181206174129.taakmwekysbkaosu@brauner.io \
--to=christian@brauner.io \
--cc=akpm@linux-foundation.org \
--cc=arnd@arndb.de \
--cc=cyphar@cyphar.com \
--cc=dancol@google.com \
--cc=ebiederm@xmission.com \
--cc=fweimer@redhat.com \
--cc=jannh@google.com \
--cc=keescook@chromium.org \
--cc=linux-api@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-man@vger.kernel.org \
--cc=luto@kernel.org \
--cc=oleg@redhat.com \
--cc=serge@hallyn.com \
--cc=tglx@linutronix.de \
--cc=timmurray@google.com \
--cc=viro@zeniv.linux.org.uk \
--cc=x86@kernel.org \
/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).