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

[Jiri Slaby <jirislaby@kernel.org>]

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)?

thanks,
-- 
js
suse labs