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

[Amir Goldstein <amir73il@gmail.com>]

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.

> 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.
>
>
> > > Fixes: 21d8a15ac333 ("lookup_one_len: don't accept . and ..")
> >
> > ...This Fixes is a bit odd to me.
>
> Me too, but I didn't see a more obvious choice. Maybe drop the
> specific Fixes: tag in favor of just Cc: stable.
>

Yeh, that'd be fine.

Thanks,
Amir.