Re: [PATCH 2/2] pidfd: add pidfdfs

[Jiri Slaby <jirislaby@kernel.org>]

On 16. 05. 24, 7:28, Jiri Slaby wrote:
> On 15. 05. 24, 18:39, Christian Brauner wrote:
>> On Wed, May 15, 2024 at 01:10:49PM +0200, Jiri Slaby wrote:
>>> On 13. 02. 24, 17:45, Christian Brauner wrote:
>>>> This moves pidfds from the anonymous inode infrastructure to a tiny
>>>> pseudo filesystem. This has been on my todo for quite a while as it 
>>>> will
>>>> unblock further work that we weren't able to do simply because of the
>>>> very justified limitations of anonymous inodes. Moving pidfds to a tiny
>>>> pseudo filesystem allows:
>>>>
>>>> * statx() on pidfds becomes useful for the first time.
>>>> * pidfds can be compared simply via statx() and then comparing inode
>>>>     numbers.
>>>> * pidfds have unique inode numbers for the system lifetime.
>>>> * struct pid is now stashed in inode->i_private instead of
>>>>     file->private_data. This means it is now possible to introduce
>>>>     concepts that operate on a process once all file descriptors 
>>>> have been
>>>>     closed. A concrete example is kill-on-last-close.
>>>> * file->private_data is freed up for per-file options for pidfds.
>>>> * Each struct pid will refer to a different inode but the same struct
>>>>     pid will refer to the same inode if it's opened multiple times. In
>>>>     contrast to now where each struct pid refers to the same inode. 
>>>> Even
>>>>     if we were to move to anon_inode_create_getfile() which creates new
>>>>     inodes we'd still be associating the same struct pid with multiple
>>>>     different inodes.
>>>> * Pidfds now go through the regular dentry_open() path which means that
>>>>     all security hooks are called unblocking proper LSM management for
>>>>     pidfds. In addition fsnotify hooks are called and allow for 
>>>> listening
>>>>     to open events on pidfds.
>>>>
>>>> The tiny pseudo filesystem is not visible anywhere in userspace exactly
>>>> like e.g., pipefs and sockfs. There's no lookup, there's no complex
>>>> inode operations, nothing. Dentries and inodes are always deleted when
>>>> the last pidfd is closed.
>>>
>>> This breaks lsof and util-linux.
>>>
>>> Without the commit, lsof shows:
>>> systemd      ... 59 [pidfd:899]
>>>
>>>
>>> With the commit:
>>> systemd      ... 1187 pidfd
>>>
>>>
>>> And that user-visible change breaks a lot of stuff, incl. lsof tests.
>>>
>>> For util-linux, its test fail with:
>>>
>>>> [  125s] --- tests/expected/lsfd/column-name-pidfd    2024-05-06 
>>>> 07:20:54.655845940 +0000
>>>> [  125s] +++ tests/output/lsfd/column-name-pidfd    2024-05-15 
>>>> 01:04:15.406666666 +0000
>>>> [  125s] @@ -1,2 +1,2 @@
>>>> [  125s] -3 anon_inode:[pidfd] pid=1 comm= nspid=1
>>>> [  125s] +3 pidfd:[INODENUM] pidfd:[INODENUM]
>>>> [  125s]  pidfd:ASSOC,KNAME,NAME: 0
>>>> [  125s]          lsfd: NAME and KNAME column: [02] 
>>>> pidfd             ... FAILED (lsfd/column-name-pidfd)
>>>
>>> And:
>>>> [  125s] --- tests/expected/lsfd/column-type-pidfd    2024-05-06 
>>>> 07:20:54.655845940 +0000
>>>> [  125s] +++ tests/output/lsfd/column-type-pidfd    2024-05-15 
>>>> 01:04:15.573333333 +0000
>>>> [  125s] @@ -1,2 +1,2 @@
>>>> [  125s] -3 UNKN pidfd
>>>> [  125s] +3 REG REG
>>>> [  125s]  pidfd:ASSOC,STTYPE,TYPE: 0
>>>> [  125s]          lsfd: TYPE and STTYPE column: [02] 
>>>> pidfd            ... FAILED (lsfd/column-type-pidfd)
>>>
>>> Any ideas?
>>
>> util-linux upstream is already handling that correctly now but it 
>> seems that
>> lsof is not. To fix this in the kernel we'll need something like. If 
>> you could
>> test this it'd be great as I'm currently traveling:
>>
>> diff --git a/fs/pidfs.c b/fs/pidfs.c
>> index a63d5d24aa02..3da848a8a95e 100644
>> --- a/fs/pidfs.c
>> +++ b/fs/pidfs.c
>> @@ -201,10 +201,8 @@ static const struct super_operations pidfs_sops = {
>>
>>   static char *pidfs_dname(struct dentry *dentry, char *buffer, int 
>> buflen)
>>   {
>> -       struct inode *inode = d_inode(dentry);
>> -       struct pid *pid = inode->i_private;
>> -
>> -       return dynamic_dname(buffer, buflen, "pidfd:[%llu]", pid->ino);
>> +       /* Fake the old name as some userspace seems to rely on this. */
>> +       return dynamic_dname(buffer, buflen, "anon_inode:[pidfd]");
> 
> No, the lsof test runs "lsof -p $pid -a -d $fd -F pfn" and expects:
> "p${pid} f${fd} n[pidfd:$pid]"
> 
> But it gets now:
> p959 f3 nanon_inode
> 
> I.e. "anon_inode" instead of "n[pidfd:959]".
> 
> Did you intend to fix by the patch the lsfd's (util-linux) 
> column-name-pidfd test by this instead (the above)?

This is now discussed in https://github.com/lsof-org/lsof/issues/317 too.

So yeah,
# ll /proc/984/fd
total 0
lrwx------ 1 xslaby users 64 May 17 09:00 0 -> /dev/pts/1
lrwx------ 1 xslaby users 64 May 17 09:00 2 -> /dev/pts/1
lrwx------ 1 xslaby users 64 May 17 09:00 3 -> anon_inode:[pidfd]

looks good with the patch. But lsof checks if this IS_REG(). And if it 
is, pidfd handling is not done.

> thanks,
-- 
js
suse labs