linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Serge E. Hallyn" <serge@hallyn.com>
To: Daniel Colascione <dancol@google.com>
Cc: "Serge E. Hallyn" <serge@hallyn.com>,
	Christian Brauner <christian@brauner.io>,
	"Eric W. Biederman" <ebiederm@xmission.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>,
	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:59:17 -0600	[thread overview]
Message-ID: <20181207005917.GA11302@mail.hallyn.com> (raw)
In-Reply-To: <CAKOZuevgF7ObKcVvu-iuNK3xLMX2Ze_S+adBEGNZCdfXRibh1A@mail.gmail.com>

On Thu, Dec 06, 2018 at 04:34:54PM -0800, Daniel Colascione wrote:
> On Thu, Dec 6, 2018 at 4:31 PM Serge E. Hallyn <serge@hallyn.com> wrote:
> >
> > On Fri, Dec 07, 2018 at 12:17:45AM +0100, Christian Brauner wrote:
> > > On Thu, Dec 06, 2018 at 11:39:48PM +0100, Christian Brauner wrote:
> > > > On Thu, Dec 06, 2018 at 03:46:53PM -0600, Eric W. Biederman wrote:
> > > > > Christian Brauner <christian@brauner.io> writes:
> > > > >
> > > > > >> 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?
> > > > > >
> > > > > > Yeah, maybe you missed it in the commit message [2] which is based on a
> > > > > > discussion with Andy [3] and Arnd [4]:
> > > > >
> > > > > Looking at your references I haven't missed it.  You are not deciding
> > > > > anything as of yet to keep it simple.  Except you are returning
> > > > > EOPNOTSUPP.  You are very much intending to do something.
> > > >
> > > > That was clear all along and was pointed at every occassion in the
> > > > threads. I even went through the hazzle to give you all of the
> > > > references when there's lore.kernel.org.
> > > >
> > > > >
> > > > > Decide.  Do you use the flags parameter or is the width of the
> > > > > target depending on the flags.
> > >
> > > Ok, let's try to be constructive. I understand the general concern for
> > > the future so let's put a contract into the commit message stating that
> > > the width of the target aka *what is signaled* will be based on a flag
> > > parameter if we ever extend it:
> > >
> > > taskfd_send_signal(fd, SIGSTOP, NULL, TASKFD_PGID);
> > > taskfd_send_signal(fd, SIGSTOP, NULL, TASKFD_TID);
> > >
> > > with the current default being
> > >
> > > taskfd_send_signal(fd, SIGSTOP, NULL, TASKFD_PID);
> > >
> > > This seems to me the cleanest solution as we only use one type of file
> > > descriptor. Can everyone be on board with this? If so I'm going to send
> > > out a new version of the patch.
> > >
> > > Christian
> >
> > I'm on board with this, but I think you need to also clarify what exactly
> > the fd stands for.  I think that (a) userspace should not have to care
> > about the struct pid implementation, and so (b) the procfd should stand
> > for all the pids.  So when taskfd_send_signal(fd, SIGSTOP, NULL, TASKFD_PGID)
> > becomes implemented, then open(/proc/5) will pin all three pids, as will
> > open(/proc/5/task/6).
> 
> This change doesn't "pin" any PID, and it makes no sense to make a
> process FD stand for all its threads. What does that even mean?

Currently the patch relies on the procfd inode saving a copy to the PIDTYPE_PID
pid.  I'm not sure offhand, can it go to the PIDTYPE_PGID from that after the
task has died, or not?   I didn't think so.  If it can then great.

The point is (a) these are details which should not have to bother userspace,
and (b) how to decide who we're sending the signal to (tid/pid/pgid) should
be specified in precisely one way.  So either a flag, or comign from the type
of fd that was opened.

-serge

  reply	other threads:[~2018-12-07  0:59 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
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 [this message]
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=20181207005917.GA11302@mail.hallyn.com \
    --to=serge@hallyn.com \
    --cc=akpm@linux-foundation.org \
    --cc=arnd@arndb.de \
    --cc=christian@brauner.io \
    --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=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).