Re: [PATCH v4] proc: report open files as size in stat() for /proc/pid/fd

[Christian Brauner]

On Fri, Nov 18, 2022 at 02:33:27PM -0500, Brian Foster wrote:
> On Fri, Nov 18, 2022 at 11:18:36AM -0800, Ivan Babrou wrote:
> > On Fri, Nov 18, 2022 at 11:10 AM Brian Foster <bfoster@xxxxxxxxxx> wrote:
> > > > +static int proc_fd_getattr(struct user_namespace *mnt_userns,
> > > > +                     const struct path *path, struct kstat *stat,
> > > > +                     u32 request_mask, unsigned int query_flags)
> > > > +{
> > > > +     struct inode *inode = d_inode(path->dentry);
> > > > +     int rv = 0;
> > > > +
> > > > +     generic_fillattr(&init_user_ns, inode, stat);
> > > > +
> > >
> > > Sorry I missed this on v3, but shouldn't this pass through the
> > > mnt_userns parameter?
> > 
> > The mnt_userns parameter was added in 549c729 (fs: make helpers idmap
> > mount aware), and it's not passed anywhere in fs/proc.
> > 
> > Looking at other uses of generic_fillattr, all of them use "init_user_ns":
> > 
> 
> Interesting. It looks like this would have used mnt_userns from
> vfs_getattr_nosec() before proc_fd_getattr() is wired up, right? I'm not
> familiar enough with that change to say whether /proc should use one
> value or the other, or perhaps it just doesn't matter.?
> 
> Christian?

Hey Brian,

This should pass init_user_ns. So it is correct the way it is done now.
The init_user_ns is used to indicate that no idmappings are used and
since procfs doesn't support the creation of idmapped mounts and doesn't
need to, passing it here makes the most sense. Technically passing down
mnt_userns would work too but that would make it look like procfs could
support idmapped mounts which isn't the case and so we don't do it this
way.

Starting soon this will be a lot clearer too since we're about to
introduce struct mnt_idmap and replace passing around userns here.
That'll make things also safer as the helpers that currently could be
passed a mnt_userns - which could be any userns - will now only be able
to take mnt_idmap which is a different type.

Long story short, the way your patch does it is correct.

Thanks!
Christian