lkml.org 
[lkml]   [2018]   [Dec]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v4] signal: add taskfd_send_signal() syscall
    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
    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

    \
     
     \ /
      Last update: 2018-12-06 18:25    [W:2.839 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site