Re: [PATCH v4] signal: add taskfd_send_signal() syscall

[Eric W. Biederman]

Daniel Colascione <dancol@xxxxxxxxxx> writes:

> On Thu, Dec 6, 2018 at 7:02 AM Eric W. Biederman <ebiederm@xxxxxxxxxxxx> wrote:
>>
>> Christian Brauner <christian@xxxxxxxxxx> 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
just "fd_send_signal(int fd, struct siginfo *info, int flags)".

Then we are not overspecifying what the system call does in the name.
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.

Eric