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

[Trond Myklebust <trondmy@hammerspace.com>]

On Fri, 2023-12-29 at 07:46 +0200, Amir Goldstein wrote:
> [CC: fsdevel, viro]
> 
> 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()?

NFS (i.e. re-exporting NFS).

Why do you not want a fix in the generic code?

> 
> > 
> > This patch just ensures that we skip '.' and '..' rather than
> > allowing a
> > match.
> 
> I agree that skipping '.' and '..' makes sense, but...
> 
> > 
> > Fixes: 21d8a15ac333 ("lookup_one_len: don't accept . and ..")
> 
> ...This Fixes is a bit odd to me.
> 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.
> 
> Thanks,
> Amir.
> 

-- 
Trond Myklebust 
CTO, Hammerspace Inc 
1900 S Norfolk St, Suite 350 - #45 
San Mateo, CA 94403 
​
www.hammerspace.com