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: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
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.


> Does the problem go away if the Fixes patch is reverted?
> I don't think so, I think you would just hit the d_inode sanity check
> after lookup_one() succeeds.
> Maybe I did not understand the problem then.
> 
> > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > ---
> >  fs/exportfs/expfs.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
> > index 3ae0154c5680..84af58eaf2ca 100644
> > --- a/fs/exportfs/expfs.c
> > +++ b/fs/exportfs/expfs.c
> > @@ -255,7 +255,9 @@ static bool filldir_one(struct dir_context *ctx, const char *name, int len,
> >                 container_of(ctx, struct getdents_callback, ctx);
> >
> >         buf->sequence++;
> > -       if (buf->ino == ino && len <= NAME_MAX) {
> > +       /* Ignore the '.' and '..' entries */
> > +       if ((len > 2 || name[0] != '.' || (len == 2 && name[1] != '.')) &&
> 
> I wish I did not have to review that this condition is correct.
> I wish there was a common helper is_dot_or_dotdot() that would be
> used here as !is_dot_dotdot(name, len).
> I found 3 copies of is_dot_dotdot().
> I didn't even try to find how many places have open coded this.


-- 
Chuck Lever