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

[Amir Goldstein <amir73il@gmail.com>]

[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()?

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