linux-kernel - Re: [PATCH v6 2/5] pidfd: add PIDFD_SELF_* sentinels to refer to own thread/process

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <b8f4664c-b8f0-46ca-b9a3-8d73e398b5ca@lucifer.local>
Date: Wed, 30 Oct 2024 16:37:37 +0000
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Christian Brauner <brauner@...nel.org>
Cc: Oleg Nesterov <oleg@...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, Oliver Sang <oliver.sang@...el.com>,
        John Hubbard <jhubbard@...dia.com>
Subject: Re: [PATCH v6 2/5] pidfd: add PIDFD_SELF_* sentinels to refer to own
 thread/process

On Mon, Oct 28, 2024 at 04:06:07PM +0000, Lorenzo Stoakes wrote:
> I guess I'll try to adapt that and respin a v7 when I get a chance.

Hm looking at this draft patch, it seems like a total rework of pidfd's
across the board right (now all pidfd's will need to be converted to
pid_fd)? Correct me if I'm wrong.

If only for the signal case, it seems like overkill to define a whole
pid_fd and to use this CLASS() wrapper just for this one instance.

If the intent is to convert _all_ pidfd's to use this type, it feels really
out of scope for this series and I think we'd probably instead want to go
off and do that as a separate series and put this on hold until that is
done.

If instead you mean that we ought to do something like this just for the
signal case, it feels like it'd be quite a bit of extra abstraction just
used in this one case but nowhere else, I think if you did an abstraction
like this it would _have_ to be across the board right?

I agree that the issue is with this one signal case that pins only the fd
(rather than this pid) where this 'pinning' doesn't _necessary_ mess around
with reference counts.

So we definitely must address this, but the issue you had with the first
approach was that I think (correct me if I'm wrong) I was passing a pointer
to a struct fd which is not permitted right?

Could we pass the struct fd by value to avoid this? I think we'd have to
unfortunately special-case this and probably duplicate some code which is a
pity as I liked the idea of abstracting everything to one place, but we can
obviously do that.

So I guess to TL;DR it, the options are:

1. Implement pid_fd everywhere, in which case I will leave off on
   this series and I guess, if I have time I could look at trying to
   implement that or perhaps you'd prefer to?

2. We are good for the sake of this series to special-case a pidfd_to_pid()
   implementation (used only by the pidfd_send_signal() syscall)

3. Something else, or I am misunderstanding your point :)

Let me know how you want me to proceed on this as we're at v6 already and I
want to be _really_ sure I'm doing what you want here.

Thanks!