Re: [RFC PATCH v1 1/7] fs: Add inode_get_ino() and implement get_ino() for NFS

[Anna Schumaker <anna.schumaker@oracle.com>]

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 <trondmy@kernel.org>
> Cc: Anna Schumaker <anna@kernel.org>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Jan Kara <jack@suse.cz>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> ---
> 
> 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"

>  {
>  #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.

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,
>  };
>  
>  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 *,