linux-kernel - Re: [PATCH 4/4] pidfs: implement fh_to

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: <d71126d4-68e5-491a-be2d-3212636e7b60@e43.eu>
Date: Wed, 13 Nov 2024 14:48:43 +0100
From: Erin Shepherd <erin.shepherd@....eu>
To: Christian Brauner <brauner@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
 christian@...uner.io, paul@...l-moore.com, bluca@...ian.org
Subject: Re: [PATCH 4/4] pidfs: implement fh_to_dentry

On 13/11/2024 14:26, Christian Brauner wrote:

> On Wed, Nov 13, 2024 at 02:06:56PM +0100, Erin Shepherd wrote:
>> On 13/11/2024 13:09, Christian Brauner wrote:
>>
>>> Hm, a pidfd comes in two flavours:
>>>
>>> (1) thread-group leader pidfd: pidfd_open(<pid>, 0)
>>> (2) thread pidfd:              pidfd_open(<pid>, PIDFD_THREAD)
>>>
>>> In your current scheme fid->pid = pid_nr(pid) means that you always
>>> encode a pidfs file handle for a thread pidfd no matter if the provided
>>> pidfd was a thread-group leader pidfd or a thread pidfd. This is very
>>> likely wrong as it means users that use a thread-group pidfd get a
>>> thread-specific pid back.
>>>
>>> I think we need to encode (1) and (2) in the pidfs file handle so users
>>> always get back the correct type of pidfd.
>>>
>>> That very likely means name_to_handle_at() needs to encode this into the
>>> pidfs file handle.
>> I guess a question here is whether a pidfd handle encodes a handle to a pid
>> in a specific mode, or just to a pid in general? The thought had occurred
>> to me while I was working on this initially, but I felt like perhaps treating
>> it as a property of the file descriptor in general was better.
>>
>> Currently open_by_handle_at always returns a thread-group pidfd (since
>> PIDFD_THREAD) isn't set, regardless of what type of pidfd you passed to
>> name_to_handle_at. I had thought that PIDFD_THREAD/O_EXCL would have been
> I don't think you're returning a thread-groupd pidfd from
> open_by_handle_at() in your scheme. After all you're encoding the tid in
> pid_nr() so you'll always find the struct pid for the thread afaict. If
> I'm wrong could you please explain how you think this works? I might
> just be missing something obvious.

Moudlo namespaces, the pid in fid->pid is the same one passed to pidfd_open().
In the root namespace, you could replace name_to_handle_at(...) with
pidfd_open(fid->pid, 0) and get the same result (if both are successful, at least).

The resulting pidfd points to the same struct pid. The only thing that should differ
is whether PIDFD_THREAD is set in f->f_flags.

>> I feel like leaving it up to the caller of open_by_handle_at might be better
>> (because they are probably better informed about whether they want poll() to
>> inform them of thread or process exit) but I could lean either way.
> So in order to decode a pidfs file handle you want the caller to have to
> specify O_EXCL in the flags argument of open_by_handle_at()? Is that
> your idea?

If they want a PIDFD_THREAD pidfd, yes. I see it as similar to O_RDONLY, where its a
flag that applies to the file descriptor but not to the underlying file.

While ideally we'd implement it from an API completeness perspective, practically I'm
not sure how often the option would ever be used. While there are hundreds of reasons
why you might want to track the state of another process, I struggle to think of cases
where Process A needs to track Process B's threads besides a debugger (and a debugger
is probably better off using ptrace), and it can happily track its own threads by just
holding onto the pidfd.