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

[Jiri Slaby <jirislaby@kernel.org>]

On 17. 05. 24, 9:09, Jiri Slaby wrote:
> 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.

So this fixes it, of course:
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -2026,7 +2026,6 @@ static struct dentry *prepare_anon_dentry(struct 
dentry **stashed,
         }

         /* Notice when this is changed. */
-       WARN_ON_ONCE(!S_ISREG(inode->i_mode));
         WARN_ON_ONCE(!IS_IMMUTABLE(inode));

         dentry = d_alloc_anon(sb);
diff --git a/fs/pidfs.c b/fs/pidfs.c
index a63d5d24..f51a794f 100644
--- a/fs/pidfs.c
+++ b/fs/pidfs.c
@@ -201,10 +201,7 @@ 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);
+       return dynamic_dname(buffer, buflen, "anon_inode:[pidfd]");
  }

  static const struct dentry_operations pidfs_dentry_operations = {
@@ -217,6 +214,7 @@ static int pidfs_init_inode(struct inode *inode, 
void *data)
  {
         inode->i_private = data;
         inode->i_flags |= S_PRIVATE;
+       inode->i_mode &= ~S_IFREG;
         inode->i_mode |= S_IRWXU;
         inode->i_op = &pidfs_inode_operations;
         inode->i_fop = &pidfs_file_operations;


> 
>> thanks,

-- 
js
suse labs