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@xxxxxxxxxx> wrote:
> >
> > From: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > ---
> > 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
