From: Daniel Colascione <email@example.com> To: "Eric W. Biederman" <firstname.lastname@example.org> Cc: Christian Brauner <email@example.com>, linux-kernel <firstname.lastname@example.org>, Linux API <email@example.com>, Andy Lutomirski <firstname.lastname@example.org>, Arnd Bergmann <email@example.com>, "Serge E. Hallyn" <firstname.lastname@example.org>, Jann Horn <email@example.com>, Andrew Morton <firstname.lastname@example.org>, Oleg Nesterov <email@example.com>, Aleksa Sarai <firstname.lastname@example.org>, Al Viro <email@example.com>, Linux FS Devel <firstname.lastname@example.org>, Tim Murray <email@example.com>, linux-man <firstname.lastname@example.org>, Kees Cook <email@example.com>, Florian Weimer <firstname.lastname@example.org>, email@example.com, firstname.lastname@example.org Subject: Re: [PATCH v4] signal: add taskfd_send_signal() syscall Date: Thu, 6 Dec 2018 14:43:27 -0800 Message-ID: <CAKOZuevqbmww0Ep+3gM73bm6H=1kUp1od8--QctuardSCaJxvw@mail.gmail.com> (raw) In-Reply-To: <email@example.com> On Thu, Dec 6, 2018 at 2:22 PM Eric W. Biederman <firstname.lastname@example.org> wrote: > > Daniel Colascione <email@example.com> writes: > > > On Thu, Dec 6, 2018 at 12:29 PM Eric W. Biederman <firstname.lastname@example.org> wrote: > >> Christian Brauner <email@example.com> writes: > >> > >> > : You cannot replicate certain aspects of kill *yet*. We have > >> > established this before. If we want process group support later we do > >> > have the flags argument to extend the sycall. > >> > >> Then you have horrible contradiction in the API. > >> > >> Either the grouping is a property of your file descriptor or the > >> grouping comes from the flags argument. > >> > >> If the grouping is specified in the flags argument then pidfd is the > >> proper name for your file descriptors, and the appropriate prefix > >> for your system call. > > > > Yes and no. "taskfd" is fine, since even if we do add a > > kill-process-group capability, the general facility under discussion > > is still *about* tasks in general, so "taskfd" still tells you in a > > general sense what the thing does. "pidfd" would be wrong, and for the > > same reason that the kernel's "struct pid" is badly-named: the object > > being named is a *task*, and signaling a particular task instead of > > whatever task happens to be labeled with a particular numeric PID at > > the time of all is the whole point of this change. > > No. The object being named by struct pid is an identifier. > > The same identifier can label processes, the same identifier can > label threads, the same identifier can label process groups the > same identifier can label sessions. > > That is fundamental to how that identifier works. > > Now the new file descriptor can either be a handle to the struct pid > the general purpose identifier, or the file descriptor can be a handle > to a process. Other file descriptor types would be handles to different > kinds of processes. If you open /proc/pid, you get a file descriptor of type A. If you pass that file descriptor to taskfd_signal, you signal the process. If you open a file descriptor to /proc/pid/task/tid, you get a file descriptor of type B, and if you pass that file descriptor to taskfd_signal, you signal the thread. A and B are *already* distinct. The only reason you'd want to require a flag for case B is to reduce the probability of error. The flag wouldn't let you open /proc/pid, yielding an A, and then pass it to taskfd_signal with a flag saying "interpret as B". I understand that it'd be inconsistent to later add a flag saying "interpret this A as a reference to its whole process group", because with respect to the difference between threads and processes, it's the file descriptor type that controls the operation invoked, not the flags. We can solve that by introducing a new FD type for a process group or by just allowing the "casting" via flag, but we're not at the point of having to decide that just yet, and I don't think the API is particularly bad either way. If you don't want to require a flag value when passing a B, then fine. It'll work either with or without the flag. If, instead, you want the flag to allow "casting" an A to a B or vice versa, that's fine too. > When people tell me the new interface can handle process groups or > sessions by just adding a flag. I hear they want to have an handle > to struct pid then general purpose identifier. Because it doesn't make > any sense at all to add a flag to when you need to add a new file > descriptor type to keep the API consistent. > > Later when I hear people say that they want an identifier for a process > and not have a handle to a general purpose identifier I think they need > to think about differnt types of file descriptors for the different > purposes. The flags argument does not help at all there. > > >> If the grouping is a property of your file descriptor it does not make > >> sense to talk about using the flags argument later. > >> > >> Your intention is to add the thread case to support pthreads once the > >> process case is sorted out. So this is something that needs to be made > >> clear. Did I miss how you plan to handle threads? > >> > >> And this fundamentally and definitely gets into all of my concerns about > >> proper handling of pid_task and PIDTYPE_TGID etc. > > > > To the extent that it's possible, this system call should mimic the > > behavior of a signal-send to a positive numeric PID (i.e., a specific > > task), so if we change one, we should change both. > > There is a special case in kill(2) where we treat an identifier that > only identifies a thread as an identifier of a process. We should > not copy that. Sure.
next prev parent reply index Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-12-06 12:18 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 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 [this message] 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='CAKOZuevqbmww0Ep+3gM73bm6H=1kUp1od8--QctuardSCaJxvw@mail.gmail.com' \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ /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
LKML Archive on lore.kernel.org Archives are clonable: git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \ firstname.lastname@example.org public-inbox-index lkml Example config snippet for mirrors Newsgroup available over NNTP: nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel AGPL code for this site: git clone https://public-inbox.org/public-inbox.git