linux-kernel - Re: [RFC PATCH 0/3] introduce PIDFD

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <1d19f18c-5a60-44b5-a96f-9d0e74f2b02c@lucifer.local>
Date: Tue, 1 Oct 2024 15:31:52 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Christian Brauner <brauner@...nel.org>
Cc: Oleg Nesterov <oleg@...hat.com>, Aleksa Sarai <cyphar@...har.com>,
        Florian Weimer <fweimer@...hat.com>,
        Christian Brauner <christian@...uner.io>,
        Shuah Khan <shuah@...nel.org>,
        "Liam R . Howlett" <Liam.Howlett@...cle.com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Vlastimil Babka <vbabka@...e.cz>, pedro.falcato@...il.com,
        linux-kselftest@...r.kernel.org, linux-mm@...ck.org,
        linux-fsdevel@...r.kernel.org, linux-api@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 0/3] introduce PIDFD_SELF

On Tue, Oct 01, 2024 at 12:21:32PM GMT, Christian Brauner wrote:
> On Mon, Sep 30, 2024 at 03:32:25PM GMT, Lorenzo Stoakes wrote:
> > On Mon, Sep 30, 2024 at 04:21:23PM GMT, Aleksa Sarai wrote:

[snip]

> > > Sorry to bike-shed, but to match /proc/self and /proc/thread-self, maybe
> > > they should be called PIDFD_SELF (for tgid) and PIDFD_THREAD_SELF (for
> > > current's tid)? In principle I guess users might use PIDFD_SELF by
> > > accident but if we mirror the naming with /proc/{,thread-}self that
> > > might not be that big of an issue?
> >
> > Lol, you know I wasn't even aware /proc/thread-self existed...
>
> Wait until you learn that /proc/$TID thread entries exist but aren't
> shown when you do ls -al /proc, only when you explicitly access them.

My God... You're right, that's crazy... :)

>
> >
> > Yeah, that actually makes sense and is consistent, though obviously the
> > concern is people will reflexively use PIDFD_SELF and end up with
> > potentially confusing results.
> >
> > I will obviously be doing a manpage patch for this so we can spell it out
> > there very clearly and also in the header - so I'd actually lean towards
> > doing this myself.
> >
> > Christian, Florian? Thoughts?
>
> I think adding both would be potentially useful. How about:
>
> #define PIDFD_SELF		-100
> #define PIDFD_THREAD_GROUP	-200

Sure, makes sense to add both.

>
> This will make PIDFD_SELF mean current and PIDFD_THREAD_GROUP mean
> current->pid_links[PIDTYPE_TGID]. I don't think we need to or should
> mirror procfs in any way. pidfds are intended to be usable without
> procfs at all.

Yeah, I think it's important to ensure the _default_ choice, so in this
case PIDFD_SELF clearly, is one that will be least surprising.

The proc thing is sort of pleasing from an aesthetic point of view, but if
you followed it you'd have to _clearly_ document PIDFD_THREAD_SELF as the
default.

Happy to go along with this. PIDFD_THREAD_GROUP is also clearer as it is
distinct from PIDFD_SELF (doesn't reference 'self' at all).

>
> I want to leave one comment on a bit of quirkiness in the api when we
> add this. I don't consider it a big deal but it should be pointed out.
>
> It can be useful to allow PIDFD_SELF or PIDFD_THREAD_GROUP to refer to
> the calling thread even for pidfd_open() to avoid an additional getpid()
> system call:
>
> (1) pidfd_open(PIDFD_SELF, PIDFD_THREAD)
> (2) pidfd_open(PIDFD_SELF, 0)
>

Hm, this is a bit weird, as these are pid_t's and PIDFD_SELF and
PIDFD_THREAD_GROUP are otherwise (pid)fd's.

Being dummy values sort of allows us to put them into service here also,
but it is just weird, we pass what is usually a pidfd to receive a pidfd,
only this time it's an actually concrete one?

I'm not sure I like this, even though as you say it avoids a getpid().

If we did this I'd prefer it to be a separate name, even if it has the same
numeric value (I guess we also might want to check for anything that uses a
negative pid_t to refer to an error or something else too).

Perhaps PID_SELF and PID_THREAD_GROUP?

> So if we allow this (Should we allow it?) then (1) is just redundant but
> whathever. But (2) is at least worth discussing: Should we reject (2) on
> the grounds of contradictory requests or allow it and document that it's
> equivalent to pidfd_open(getpid(), PIDFD_THREAD)? It feels like the
> latter would be ok.
>
> Similar for pidfd_send_signal():
>
> // redundant but ok:
> (1) pidfd_send_signal(PIDFD_SELF,         PIDFD_SIGNAL_THREAD)
>
> // redundant but ok:
> (2) pidfd_send_signal(PIDFD_THREAD_GROUP, PIDFD_SIGNAL_THREAD_GROUP)
>
> // weird way to spell pidfd_send_signal(PIDFD_THREAD_GROUP, 0)
> (3) pidfd_send_signal(PIDFD_SELF,         PIDFD_SIGNAL_THREAD_GROUP)
>
> // weird way to spell pidfd_send_signal(PIDFD_SELF, 0)
> (4) pidfd_send_signal(PIDFD_THREAD_GROUP, PIDFD_SIGNAL_THREAD)
>
> I think all of this is ok but does anyone else have a strong opinion?

I think it's fine to allow all 4 and we should get this behaviour by
default (if we have no flags we use the f_flags as a hint, which will be
set correctly).

I think people might find contradictory ones, i.e. 3 and 4, strange, but it
makes sense for the flags to override the pidfd (as they would for a
non-sentinel pidfd) and it makes everything consistent vs. if you were not
using a sentinel value.

So yes I think that's fine.