Received-SPF: pass (google.com: domain of linux-nfs+bounces-836-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20231228201510.985235-1-trondmy@kernel.org> <CAOQ4uxiCf=FWtZWw2uLRmfPvgSxsnmqZC6A+FQgQs=MBQwA30w@mail.gmail.com>
 <ZY7ZC9q8dGtoC2U/@tissot.1015granger.net>
In-Reply-To: <ZY7ZC9q8dGtoC2U/@tissot.1015granger.net>
From: Amir Goldstein <amir73il@gmail.com>
Date: Fri, 29 Dec 2023 19:44:20 +0200
Message-ID: <CAOQ4uxh1VDPVq7a82HECtKuVwhMRLGe3pvL6TY6Xoobp=vaTTw@mail.gmail.com>
Subject: Re: [PATCH] knfsd: fix the fallback implementation of the get_name
 export operation
To: Chuck Lever <chuck.lever@oracle.com>
Cc: trondmy@kernel.org, Linux NFS Mailing List <linux-nfs@vger.kernel.org>, 
	linux-fsdevel <linux-fsdevel@vger.kernel.org>, Al Viro <viro@zeniv.linux.org.uk>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Dec 29, 2023 at 4:35=E2=80=AFPM 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=E2=80=AFPM <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 filena=
me
> > > 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 fo=
r
> > > filesystems that violate POSIX requirements around uniqueness of inod=
e
> > > 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 afte=
r
> > 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 allowin=
g 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.