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

[Trond Myklebust <trondmy@hammerspace.com>]

On Wed, 2024-10-16 at 19:05 -0400, Paul Moore wrote:
> On Wed, Oct 16, 2024 at 10:23 AM Christian Brauner
> <brauner@kernel.org> wrote:
> > 
> > On Thu, Oct 10, 2024 at 05:26:41PM +0200, 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.
> > 
> > What are the other filesystems that are presumably affected by this
> > that
> > would need an inode accessor?
> 
> I don't want to speak for Mickaël, but my reading of the patchset was
> that he was suspecting that other filesystems had the same issue
> (privately maintained inode numbers) and was posting this as a RFC
> partly for clarity on this from the VFS developers such as yourself.
> 
> > If this is just about NFS then just add a helper function that
> > audit and
> > whatever can call if they need to know the real inode number
> > without
> > forcing a new get_inode() method onto struct inode_operations.
> 
> If this really is just limited to NFS, or perhaps NFS and a small
> number of filesystems, then a a helper function is a reasonable
> solution.  I think Mickaël was worried that private inode numbers
> would be more common, in which case a get_ino() method makes a bit
> more sense.
> 
> > And I don't buy that is suddenly rush hour for this.
> 
> I don't think Mickaël ever characterized this as a "rush hour" issue
> and I know I didn't.  It definitely caught us by surprise to learn
> that inode->i_no wasn't always maintained, and we want to find a
> solution, but I'm not hearing anyone screaming for a solution
> "yesterday".
> 
> > Seemingly no one noticed this in the past idk how many years.
> 
> Yet the issue has been noticed and we would like to find a solution,
> one that is acceptable both to the VFS and LSM folks.
> 
> Can we start with compiling a list of filesystems that maintain their
> inode numbers outside of inode->i_no?  NFS is obviously first on that
> list, are there others that the VFS devs can add?
> 

Pretty much any filesystem that uses 64-bit inode numbers has the same
problem: if the application calls stat(), 32-bit glibc will happily
convert that into a call to fstatat64() and then cry foul if the kernel
dares return an inode number that doesn't fit in 32 bits.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com