On Thu, Oct 10, 2024 at 02:07:52PM -0400, Anna Schumaker wrote:
> Hi Mickaël,
>
> On 10/10/24 11:26 AM, Mickaël Salaün wrote:
> > When a filesystem manages its own inode numbers, like NFS's fileid shown
> > to user space with getattr(), other part of the kernel may still expose
> > the private inode->ino through kernel logs and audit.
> >
> > Another issue is on 32-bit architectures, on which ino_t is 32 bits,
> > whereas the user space's view of an inode number can still be 64 bits.
> >
> > Add a new inode_get_ino() helper calling the new struct
> > inode_operations' get_ino() when set, to get the user space's view of an
> > inode number. inode_get_ino() is called by generic_fillattr().
> >
> > Implement get_ino() for NFS.
> >
> > Cc: Trond Myklebust <[email protected]>
> > Cc: Anna Schumaker <[email protected]>
> > Cc: Alexander Viro <[email protected]>
> > Cc: Christian Brauner <[email protected]>
> > Cc: Jan Kara <[email protected]>
> > Signed-off-by: Mickaël Salaün <[email protected]>
> > ---
> >
> > I'm not sure about nfs_namespace_getattr(), please review carefully.
> >
> > I guess there are other filesystems exposing inode numbers different
> > than inode->i_ino, and they should be patched too.
> > ---
> > fs/nfs/inode.c | 6 ++++--
> > fs/nfs/internal.h | 1 +
> > fs/nfs/namespace.c | 2 ++
> > fs/stat.c | 2 +-
> > include/linux/fs.h | 9 +++++++++
> > 5 files changed, 17 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> > index 542c7d97b235..5dfc176b6d92 100644
> > --- a/fs/nfs/inode.c
> > +++ b/fs/nfs/inode.c
> > @@ -83,18 +83,19 @@ EXPORT_SYMBOL_GPL(nfs_wait_bit_killable);
> >
> > /**
> > * nfs_compat_user_ino64 - returns the user-visible inode number
> > - * @fileid: 64-bit fileid
> > + * @inode: inode pointer
> > *
> > * This function returns a 32-bit inode number if the boot parameter
> > * nfs.enable_ino64 is zero.
> > */
> > -u64 nfs_compat_user_ino64(u64 fileid)
> > +u64 nfs_compat_user_ino64(const struct *inode)
> ^^^^^^^^^^^^^^^^^^^
> This should be "const struct inode *inode"
Indeed...
>
> > {
> > #ifdef CONFIG_COMPAT
> > compat_ulong_t ino;
> > #else
> > unsigned long ino;
> > #endif
> > + u64 fileid = NFS_FILEID(inode);
> >
> > if (enable_ino64)
> > return fileid;
> > @@ -103,6 +104,7 @@ u64 nfs_compat_user_ino64(u64 fileid)
> > ino ^= fileid >> (sizeof(fileid)-sizeof(ino)) * 8;
> > return ino;
> > }
> > +EXPORT_SYMBOL_GPL(nfs_compat_user_ino64);
> >
> > int nfs_drop_inode(struct inode *inode)
> > {
> > diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> > index 430733e3eff2..f5555a71a733 100644
> > --- a/fs/nfs/internal.h
> > +++ b/fs/nfs/internal.h
> > @@ -451,6 +451,7 @@ extern void nfs_zap_acl_cache(struct inode *inode);
> > extern void nfs_set_cache_invalid(struct inode *inode, unsigned long
> > flags);
> > extern bool nfs_check_cache_invalid(struct inode *, unsigned long);
> > extern int nfs_wait_bit_killable(struct wait_bit_key *key, int mode);
> > +extern u64 nfs_compat_user_ino64(const struct *inode);
>
> Why add this here when it's already in include/linux/nfs_fs.h? Can you update
> that declaration instead?
>
> Also, there is a caller for nfs_compat_user_ino64() in fs/nfs/dir.c that
> needs to be updated. Can you double check that you have CONFIG_NFS_FS=m (or
> 'y') in your kernel .config? These are all issues my compiler caught when I
> applied your patch.
Oh, I enabled CONFIG_NFSD_V4 and CONFIG_NFS_COMMON but not
CONFIG_NFS_FS, that explains these uncaught errors... Thanks!
About the nfs_do_filldir(), the nfs_compat_user_ino64() call is indeed
not correct with this patch, but I'm wondering why not use ent->ino
instead?
>
> Thanks,
> Anna
>
> >
> > #if IS_ENABLED(CONFIG_NFS_LOCALIO)
> > /* localio.c */
> > diff --git a/fs/nfs/namespace.c b/fs/nfs/namespace.c
> > index e7494cdd957e..d9b1e0606833 100644
> > --- a/fs/nfs/namespace.c
> > +++ b/fs/nfs/namespace.c
> > @@ -232,11 +232,13 @@ nfs_namespace_setattr(struct mnt_idmap *idmap, struct
> > dentry *dentry,
> > const struct inode_operations nfs_mountpoint_inode_operations = {
> > .getattr = nfs_getattr,
> > .setattr = nfs_setattr,
> > + .get_ino = nfs_compat_user_ino64,
> > };
> >
> > const struct inode_operations nfs_referral_inode_operations = {
> > .getattr = nfs_namespace_getattr,
> > .setattr = nfs_namespace_setattr,
> > + .get_ino = nfs_compat_user_ino64,
Is it correct to use nfs_compat_user_ino64() for both of these inode
operation structs? With this patch, generic_fileattr() initialize the
stat struct with get_ino(), which might not what we want according to
these nfs_mountpoint and nfs_referral structs.
> > };
> >
> > static void nfs_expire_automounts(struct work_struct *work)
> > diff --git a/fs/stat.c b/fs/stat.c
> > index 41e598376d7e..05636919f94b 100644
> > --- a/fs/stat.c
> > +++ b/fs/stat.c
> > @@ -50,7 +50,7 @@ void generic_fillattr(struct mnt_idmap *idmap, u32
> > request_mask,
> > vfsgid_t vfsgid = i_gid_into_vfsgid(idmap, inode);
> >
> > stat->dev = inode->i_sb->s_dev;
> > - stat->ino = inode->i_ino;
> > + stat->ino = inode_get_ino(inode);
> > stat->mode = inode->i_mode;
> > stat->nlink = inode->i_nlink;
> > stat->uid = vfsuid_into_kuid(vfsuid);
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index e3c603d01337..0eba09a21cf7 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2165,6 +2165,7 @@ struct inode_operations {
> > struct dentry *dentry, struct fileattr *fa);
> > int (*fileattr_get)(struct dentry *dentry, struct fileattr *fa);
> > struct offset_ctx *(*get_offset_ctx)(struct inode *inode);
> > + u64 (*get_ino)(const struct inode *inode);
> > } ____cacheline_aligned;
> >
> > static inline int call_mmap(struct file *file, struct vm_area_struct *vma)
> > @@ -2172,6 +2173,14 @@ static inline int call_mmap(struct file *file,
> > struct vm_area_struct *vma)
> > return file->f_op->mmap(file, vma);
> > }
> >
> > +static inline u64 inode_get_ino(struct inode *inode)
> > +{
> > + if (unlikely(inode->i_op->get_ino))
> > + return inode->i_op->get_ino(inode);
> > +
> > + return inode->i_ino;
> > +}
> > +
> > extern ssize_t vfs_read(struct file *, char __user *, size_t, loff_t *);
> > extern ssize_t vfs_write(struct file *, const char __user *, size_t,
> > loff_t *);
> > extern ssize_t vfs_copy_file_range(struct file *, loff_t , struct file *,
>
>
