Re: Exporting a unique ino/dev pair to user space

[Mark Fasheh <mfasheh@suse.de>]

On Thu, Jun 07, 2018 at 10:38:51AM +0300, Amir Goldstein wrote:
> On Thu, Jun 7, 2018 at 12:38 AM, Mark Fasheh <mfasheh@suse.de> wrote:
> > Hi,
> >
> > We have an inconsistency in how the kernel is exporting inode number /
> > device pairs for user space. There's of course stat(2) and statx(2),
> > but aside from those we simply dump inode->i_ino and super->s_dev. In
> > some cases, the dumped values differ from what is returned via stat(2)
> > or statx(2). Some filesystems might even show duplicate (but
> > internally different!) pairs when the raw i_ino/s_dev is used.
> >
> > Some examples where we dump raw ino/dev:
> >
> > - /proc/<pid>/maps. I've written about how this confuses lsof(8):
> >   https://marc.info/?l=linux-btrfs&m=130074451403261&w=2
> >
> > - Unsurprisingly, many VFS tracepoints dump ino and/or dev. See
> >   trace/events/lock.h or trace/events/writeback.h for examples.
> >
> > - eventpoll also dumps the raw ino/dev pair via ep_show_fdinfo()
> >
> > - Audit records the raw ino/dev and passes them around. We do seem to
> >   have paths printed from audit as well, but if it's printed with the
> >   wrong ino/dev pair I believe my point still stands.
> >
> 
> knfsd also looks at i_ino. I converted one or two of these places
> to getattr callbacks recently, but there are still some more.
> 
> Anyway, w.r.t. Overlayfs, I believe Miklos' work to make file_inode()
> an overlay inode should resolve several of the issues listed above -
> probably not all, but didn't check every tracepoint..

Ok, thanks for letting me know about knfsd and the overlayfs work. I haven't
had a chance to check out the overlayfs work yet but I will shortly.


> > This breaks software which expects these pairs to be unique, and can
> > put the user in a situation where they might not be able to find an
> > inode referenced from the kernel. What's even worse - depending on how
> > ino is exported, they might even find the *wrong* inode.
> >
> > I also should point out that we're likely at this point because
> > stat(2) has been using an unsigned long for ino. On a 32 bit system,
> > it would have been impossible for the user to get the real inode
> > number in the first place. So there probably wasn't much we could do.
> >
> > These days though, we have statx(2) which will do the right thing on
> > all platforms so we no longer have that excuse. The user ought to be
> > able to take an ino/dev pair and ultimately find the actual file on
> > their system, partially with the help of statx(2).
> >
> >
> > Some examples of how ino is manipulated by filesystems:
> >
> > - On 64 bit i_ino and kstat->ino tend to be filled in correctly (from
> >   what I can tell). stat->dev is not always consistent with super->s_dev.
> >
> > - On 32 bits, many filesystems employ a transformation to squeeze a 64
> >   bit identifier into 32 bits. The exact methods are fs specific,
> >   what's important is that we're losing information, introducing the
> >   possibility of duplicate inode numbers.
> >
> > - On all platforms, Btrfs and Overlayfs can have duplicate inode
> >   numbers. Of course, device can be different across the fs as well
> >   with the kernel reporting s_dev and these filesystems returning
> >   a different device via stat() or statx().
> >
> >
> > Getting the inode number portion of this pair fixed would immediately
> > solve the situation for all filesystems except Btrfs and
> > Overlayfs - they report a different device from stat.
> >
> > Regarding the device portion of the pair, I'm honestly not sure
> > whether Overlayfs cares, and my attempts to fix the s_dev situation
> > for Btrfs have all come to the same dead ends that I've hit briefly
> > looking into this inode number issue - the problems are intrinsically
> > linked.
> >
> > So my questions are:
> >
> > 1) Do we care about this? On one hand, we've had this inconsistency
> >    for a long time, for various reasons. On the other hand, I can point
> >    to bugzilla's where these inconsistencies have become a problem.
> >
> >    In the case that we don't care, any fs-internal solutions are
> >    likely to be extremely disruptive to the end user.
> >
> >
> > 2) If we do care, how do we fix this?
> >
> >  2a) Do we use 64 bit i_ino on 32 bit systems? This has some obvious
> >      downsides from a memory usage standpoint. Also it doesn't fully
> >      address the issue - we still have a device field that Btrfs and
> >      Overlayfs override.
> >
> >      We could combine this with an intermediate structure between the
> >      inode and super block so s_dev could be abstracted out. See my
> >      fs_view patches for an example of how this could be done:
> >      https://www.spinics.net/lists/linux-fsdevel/msg125492.html
> >
> >  2b) Do we call ->getattr for this information? Plumbing a vfsmount to
> >      various regions of the kernel like audit, or some of the deeper
> >      tracepoints looks ugly and prone to life-timing issues (which
> >      vfsmount do we use?). On the upside, we could probably make it
> >      really light by only sending down the STATX_INO flag and letting
> >      the filesystem optimize accordingly.
> >
> >  2c) I don't think we can really use a dedicated callback without
> >      passing the vfsmount through since Overlayfs ->getattr might call
> >      the lower fs ->getattr. At that point we might as well use getattr.
> >
> 
> Didn't get the Overlayfs lower fs getattr argument.
> Overlayfs doesn't use the vfsmount passed into getattr
> and it could very well pass a dentry to lower fs getattr.

My main point in 2c) is that, from my understanding, Overlayfs may need to
call down to one of the filesystem ->getattr() calls. Since those take a
vfsmount we don't gain anything by having a unique callback from this - the
plumbing work would be the same.


> As a matter of fact, out of 35 getattr implementations in the kernel:
> (git grep "\s\.getattr\s" fs| awk '{print $4}'| sort -u|grep -v
> "nfs.*_proc_getattr"|wc -l)
> there is only one using the vfsmount - nfs_getattr() for MNT_NOATIME
> check and most of them only ever use d_inode(path->dentry).
> 
> This API seems quite odd.
> Maybe it should be fixed so more in kernel call sites could call getattr
> without a vfsmount.
> Not sure what would be the best way to handle nfs_getattr().

Yeah I saw that nfs_getattr() is the only user of the vfsmount. I totally
agree that this would be tons easier if we didn't have to pass the vfsmount
(and like you said, there's only the ONE user).

This is a bit hacky, but I wonder if we could blow the function signature
back out to a dentry + vfsmount and make the vfsmount optional when getattr
is called only for ino/dev. It's a bit ugly to have optional arguments like
that but nfs would work with just a line or two change and the other fs
would never even care.
	--Mark