Received-SPF: pass (google.com: domain of linux-nfs+bounces-840-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20231228201510.985235-1-trondmy@kernel.org> <CAOQ4uxiCf=FWtZWw2uLRmfPvgSxsnmqZC6A+FQgQs=MBQwA30w@mail.gmail.com>
 <ZY7ZC9q8dGtoC2U/@tissot.1015granger.net> <CAOQ4uxh1VDPVq7a82HECtKuVwhMRLGe3pvL6TY6Xoobp=vaTTw@mail.gmail.com>
 <ZY9WPKwO2M6FXKpT@tissot.1015granger.net> <a14bca2bb50eb0a305efc829262081b9b262d888.camel@hammerspace.com>
In-Reply-To: <a14bca2bb50eb0a305efc829262081b9b262d888.camel@hammerspace.com>
From: Amir Goldstein <amir73il@gmail.com>
Date: Sat, 30 Dec 2023 08:23:31 +0200
Message-ID: <CAOQ4uxgcCajCD_bNKSLJp2AG1Q=N0CW9P-h+JMiun48mY0ZyDQ@mail.gmail.com>
Subject: Re: [PATCH] knfsd: fix the fallback implementation of the get_name
 export operation
To: Trond Myklebust <trondmy@hammerspace.com>
Cc: "chuck.lever@oracle.com" <chuck.lever@oracle.com>, 
	"linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>, 
	"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>, 
	"viro@zeniv.linux.org.uk" <viro@zeniv.linux.org.uk>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sat, Dec 30, 2023 at 1:50=E2=80=AFAM Trond Myklebust <trondmy@hammerspac=
e.com> wrote:
>
> On Fri, 2023-12-29 at 18:29 -0500, Chuck Lever wrote:
> > On Fri, Dec 29, 2023 at 07:44:20PM +0200, Amir Goldstein wrote:
> > > 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> wro=
te:
> > > > > >
> > > > > > 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
> > >
> > > 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.
> >
> > This new comment:
> >
> > +     /* Ignore the '.' and '..' entries */
> >
> > then seems inadequate to explain why dot and dot-dot are now never
> > matched. Perhaps the function's documenting comment could expand on
> > this a little. I'll give it some thought.
>
> The point of this code is to attempt to create a valid path that
> connects the inode found by the filehandle to the export point. The
> readdir() must determine a valid name for a dentry that is a component
> of that path, which is why '.' and '..' can never be acceptable.
>
> This is why I think we should keep the 'Fixes:' line. The commit it
> points to explains quite concisely why this patch is needed.
>

By all means, mention this commit, just not with a fixed tag please.
IIUC, commit 21d8a15ac333 did not introduce a regression that this
patch fixes. Right?
So why insist on abusing Fixes: tag instead of a mention?

Thanks,
Amir.