Linux-api Archive on lore.kernel.org
 help / color / Atom feed
From: Sargun Dhillon <sargun@sargun.me>
To: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: LKML <linux-kernel@vger.kernel.org>,
	Linux Containers <containers@lists.linux-foundation.org>,
	Linux API <linux-api@vger.kernel.org>,
	Christian Brauner <christian.brauner@ubuntu.com>,
	Tycho Andersen <tycho@tycho.ws>,
	Kees Cook <keescook@chromium.org>,
	Aleksa Sarai <cyphar@cyphar.com>
Subject: Re: [PATCH] seccomp: Add group_leader pid to seccomp_notif
Date: Fri, 22 May 2020 10:54:55 -0700
Message-ID: <CAMp4zn_v-D=gyzdWO7D2KrdZ_vct87dV_0pM5HVNE_3zDG7k8Q@mail.gmail.com> (raw)
In-Reply-To: <87h7wc4zac.fsf@x220.int.ebiederm.org>

On Mon, May 18, 2020 at 4:11 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> Sargun Dhillon <sargun@sargun.me> writes:
>
> > This includes the thread group leader ID in the seccomp_notif. This is
> > immediately useful for opening up a pidfd for the group leader, as
> > pidfds only work on group leaders.
>
> The code looks fine (except for the name of the test), but can you
> please talk and think about this as something other than the
> group leader?
>
> The initial thread in a thread group can die, and the tgid is still
> valid for the entire group.  Because the initial thread of a
> process/thread group can die (but rarely does) that tends to result in
> kernel code that fails when thread_group_leader dies.
>
> To remove that class of bugs I am slowy working to remove the
> thread_group_leader from the kernel entirely.
>
> Looking at the names of the fields in the structure it looks like
> there is another class of bugs to be removed by renaming PIDTYPE_PID
> to PIDTYPE_TID in the kernel as well.  Just skimming the example code
> it looks very simple to get confused.
>
> Is there any chance some can modify struct seccomp_notify to do
> {
>         ...
>         union {
>                 __u32 pid;
>                 __u32 tid;
>         };
>         ...
> }
>
> Just to reduce the chance of confusion between the userspace pid and the
> in kernel pid names?
>
> Eric
Our use cases would be unaffected by this. I think this would be a wonderful
way to move forward, but I don't know if it could break userspace.

I believe Christian's team is the biggest user of this feature in OSS right now,
so he might know.

In addition, I'm not sure where you would want the thread's ID versus the
process's ID, unless you wanted to do something like SIGSTOP, and freeze
the thread to prevent it from making more progress, or being interrupted
while you go do notifier work.

Christian & Kees,
Thoughts?

      reply index

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-15 23:40 Sargun Dhillon
2020-05-17  7:17 ` Kees Cook
2020-05-17 10:47   ` Christian Brauner
2020-05-17 11:21     ` Aleksa Sarai
2020-05-17 14:23       ` Tycho Andersen
2020-05-17 14:33         ` Christian Brauner
2020-05-17 14:35           ` Christian Brauner
2020-05-17 14:46           ` Tycho Andersen
2020-05-17 15:02             ` Tycho Andersen
2020-05-17 21:30               ` Kees Cook
2020-05-18  8:32                 ` Sargun Dhillon
2020-05-18 12:45                   ` Christian Brauner
2020-05-18 13:23                     ` Tycho Andersen
2020-05-18 14:00                       ` Christian Brauner
2020-05-18 12:05                 ` Christian Brauner
2020-05-18 21:10                 ` Kees Cook
2020-05-18 12:53               ` Christian Brauner
2020-05-18 13:20                 ` Tycho Andersen
2020-05-18 21:37                 ` Sargun Dhillon
2020-05-17 11:17   ` Sargun Dhillon
2020-05-18 23:08 ` Eric W. Biederman
2020-05-22 17:54   ` Sargun Dhillon [this message]

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='CAMp4zn_v-D=gyzdWO7D2KrdZ_vct87dV_0pM5HVNE_3zDG7k8Q@mail.gmail.com' \
    --to=sargun@sargun.me \
    --cc=christian.brauner@ubuntu.com \
    --cc=containers@lists.linux-foundation.org \
    --cc=cyphar@cyphar.com \
    --cc=ebiederm@xmission.com \
    --cc=keescook@chromium.org \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=tycho@tycho.ws \
    /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

Linux-api Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-api/0 linux-api/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-api linux-api/ https://lore.kernel.org/linux-api \
		linux-api@vger.kernel.org
	public-inbox-index linux-api

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-api


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git