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

[Oleg Nesterov <oleg@redhat.com>]

On 02/18, Christian Brauner wrote:
>
> On Sat, Feb 17, 2024 at 09:30:19AM -0800, Linus Torvalds wrote:
> > On Sat, 17 Feb 2024 at 06:00, Oleg Nesterov <oleg@redhat.com> wrote:
> > >
> > > But I have a really stupid (I know nothing about vfs) question, why do we
> > > need pidfdfs_ino and pid->ino ? Can you explain why pidfdfs_alloc_file()
> > > can't simply use, say, iget_locked(pidfdfs_sb, (unsigned long)pid) ?
> > >
> > > IIUC, if this pid is freed and then another "struct pid" has the same address
> > > we can rely on __wait_on_freeing_inode() ?
> >
> > Heh. Maybe it would work, but we really don't want to expose core
> > kernel pointers to user space as the inode number.

We could use ptr_to_hashval(pid).

> And then also the property that the inode number is unique for the
> system lifetime is extremely useful for userspace and I would like to
> retain that property.

OK.

and perhaps task->thread_pid->ino can also be used as task_monotonic_id(task)
if we move the pid->ino initialization into init_task_pid(PIDTYPE_PID), this
allows to implement for_each_process_thread_break/continue... Nevermind, please
forget.

> > > +	if (inode->i_state & I_NEW) {
> > > +		inode->i_ino = pid->ino;
> >
> > I guess this is unnecessary, iget_locked() should initialize i_ino if I_NEW ?
>
> Yes, it does. I just like to be explicit in such cases.

Well. Of course I won't insist, this is minor. But to me this adds the
unnecessary confusion, as if we need to override ->ino for some reason.

Oleg.