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

[Christian Brauner <brauner@kernel.org>]

On Tue, May 21, 2024 at 08:13:08AM +0200, Jiri Slaby wrote:
> On 21. 05. 24, 8:07, Jiri Slaby wrote:
> > On 20. 05. 24, 21:15, Linus Torvalds wrote:
> > > On Mon, 20 May 2024 at 12:01, Linus Torvalds
> > > <torvalds@linux-foundation.org> wrote:
> > > > 
> > > > So how about just a patch like this?  It doesn't do anything
> > > > *internally* to the inodes, but it fixes up what we expose to user
> > > > level to make it look like lsof expects.
> > > 
> > > Note that the historical dname for those pidfs files was
> > > "anon_inode:[pidfd]", and that patch still kept the inode number in
> > > there, so now it's "anon_inode:[pidfd-XYZ]", but I think lsof is still
> > > happy with that.
> > 
> > Now the last column of lsof still differs from 6.8:
> > -[pidfd:1234]
> > +[pidfd-4321]
> > 
> > And lsof tests still fail, as "lsof -F pfn" is checked against:
> >      if ! fgrep -q "p${pid} f${fd} n[pidfd:$pid]" <<<"$line"; then
> > 
> > Where $line is:
> > p1015 f3 n[pidfd-1315]
> > 
> > Wait, even if I change that minus to a colon, the inner pid (1315)
> > differs from the outer (1015), but it should not (according to the
> > test).
> 
> This fixes the test (meaning literally "it shuts up the test", but I have no
> idea if it is correct thing to do at all):
> -       return dynamic_dname(buffer, buflen, "anon_inode:[pidfd-%llu]",
> pid->ino);
> +       return dynamic_dname(buffer, buflen, "anon_inode:[pidfd:%d]",
> pid_nr(pid));
> 
> Maybe pid_vnr() would be more appropriate, I have no idea either.

So as pointed out the legacy format for pidfds is:

lrwx------ 1 root root  64 21. Mai 14:24 39 -> 'anon_inode:[pidfd]'

So it's neither showing inode number nor pid.

The problem with showing the pid unconditionally like that in
dynamic_dname() is that it's wrong in various circumstances. For
example, when the pidfd is sent into a pid namespace outside the it's
pid namespace hierarchy (e.g., into a sibling pid namespace or when the
task has already been reaped.

Imho, showing the pid is more work than it's worth especially because we
expose that info in fdinfo/<nr> anyway. So let's just do the simple thing.