Re: [PATCH] knfsd: fix the fallback implementation of the get_name export operation

[Chuck Lever <chuck.lever@oracle.com>]

On Fri, Dec 29, 2023 at 07:44:20PM +0200, Amir Goldstein wrote:
> On Fri, Dec 29, 2023 at 4:35 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> >
> > On Fri, Dec 29, 2023 at 07:46:54AM +0200, Amir Goldstein wrote:
> > > [CC: fsdevel, viro]
> >
> > Thanks for picking this up, Amir, and for copying viro/fsdevel. I
> > was planning to repost this next week when more folks are back, but
> > this works too.
> >
> > Trond, if you'd like, I can handle review changes if you don't have
> > time to follow up.
> >
> >
> > > On Thu, Dec 28, 2023 at 10:22 PM <trondmy@kernel.org> wrote:
> > > >
> > > > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > >
> > > > The fallback implementation for the get_name export operation uses
> > > > readdir() to try to match the inode number to a filename. That filename
> > > > is then used together with lookup_one() to produce a dentry.
> > > > A problem arises when we match the '.' or '..' entries, since that
> > > > causes lookup_one() to fail. This has sometimes been seen to occur for
> > > > filesystems that violate POSIX requirements around uniqueness of inode
> > > > numbers, something that is common for snapshot directories.
> > >
> > > Ouch. Nasty.
> > >
> > > Looks to me like the root cause is "filesystems that violate POSIX
> > > requirements around uniqueness of inode numbers".
> > > This violation can cause any of the parent's children to wrongly match
> > > get_name() not only '.' and '..' and fail the d_inode sanity check after
> > > lookup_one().
> > >
> > > I understand why this would be common with parent of snapshot dir,
> > > but the only fs that support snapshots that I know of (btrfs, bcachefs)
> > > do implement ->get_name(), so which filesystem did you encounter
> > > this behavior with? can it be fixed by implementing a snapshot
> > > aware ->get_name()?
> > >
> > > > This patch just ensures that we skip '.' and '..' rather than allowing a
> > > > match.
> > >
> > > I agree that skipping '.' and '..' makes sense, but...
> >
> > Does skipping '.' and '..' make sense for file systems that do
> 
> It makes sense because if the child's name in its parent would
> have been "." or ".." it would have been its own parent or its own
> grandparent (ELOOP situation).
> IOW, we can safely skip "." and "..", regardless of anything else.

This new comment:

+	/* Ignore the '.' and '..' entries */

then seems inadequate to explain why dot and dot-dot are now never
matched. Perhaps the function's documenting comment could expand on
this a little. I'll give it some thought.


> > indeed guarantee inode number uniqueness? Given your explanation
> > here, I'm wondering whether the generic get_name() function is the
> > right place to address the issue.

-- 
Chuck Lever