Received-SPF: pass (google.com: domain of linux-nfs+bounces-943-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: <170429478711.50646.12675561629884992953.stgit@bazille.1015granger.net>
 <170429517779.50646.9656897459585544068.stgit@bazille.1015granger.net>
 <CAOQ4uxgMLWGqqoSNvSgB=Qfmw6Brk2eO6yB7FZqX6p-DcTiUtw@mail.gmail.com> <ZZbAQEgqbV72RJn8@tissot.1015granger.net>
In-Reply-To: <ZZbAQEgqbV72RJn8@tissot.1015granger.net>
From: Amir Goldstein <amir73il@gmail.com>
Date: Thu, 4 Jan 2024 19:43:10 +0200
Message-ID: <CAOQ4uxhB506ZunNzmnyk=FKCRHEPOupV34vcfjVMJ9o9SDVJow@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] exportfs: fix the fallback implementation of the
 get_name export operation
To: Chuck Lever <chuck.lever@oracle.com>
Cc: Chuck Lever <cel@kernel.org>, jlayton@redhat.com, 
	Trond Myklebust <trond.myklebust@hammerspace.com>, Jeff Layton <jlayton@kernel.org>, 
	linux-fsdevel@vger.kernel.org, linux-nfs@vger.kernel.org, 
	trondmy@hammerspace.com, viro@zeniv.linux.org.uk, brauner@kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Jan 4, 2024 at 4:27=E2=80=AFPM Chuck Lever <chuck.lever@oracle.com>=
 wrote:
>
> On Thu, Jan 04, 2024 at 09:39:04AM +0200, Amir Goldstein wrote:
> > On Wed, Jan 3, 2024 at 5:19=E2=80=AFPM Chuck Lever <cel@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 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.
> > >
> > > This patch just ensures that we skip '.' and '..' rather than allowin=
g a
> > > match.
> > >
> > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > Reviewed-by: Jeff Layton <jlayton@kernel.org>
> > > Acked-by: Amir Goldstein <amir73il@gmail.com>
> > > Link: https://lore.kernel.org/linux-nfs/CAOQ4uxiOZobN76OKB-VBNXWeFKVw=
LW_eK5QtthGyYzWU9mjb7Q@mail.gmail.com/
> > > Signed-off-by: Chuck Lever <chuck.lever@oracle.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 =3D=3D ino && len <=3D NAME_MAX) {
> > > +       /* Ignore the '.' and '..' entries */
> > > +       if ((len > 2 || name[0] !=3D '.' || (len =3D=3D 2 && name[1] =
!=3D '.')) &&
> > > +           buf->ino =3D=3D ino && len <=3D NAME_MAX) {
> >
> >
> > Thank you for creating the helper, but if you already went to this trou=
ble,
> > I think it is better to introduce is_dot_dotdot() as a local helper alr=
eady
> > in this backportable patch, so that stable kernel code is same as upstr=
eam
> > code (good for future fixes) and then dedupe the local helper with the =
rest
> > of the local helpers in patch 2?
>
> There's now no Fixes: nor a Cc: stable on 1/2. You convinced me that
> 1/2 will not result in any external behavior change.
>
> The upshot is I do not expect 1/2 will be backported, unless I have
> grossly misread your emails.
>

It's not what I meant, but I don't want to bother you about this.

I meant patch 1 is backportable:
- adds static is_dot_dotdot() in expfs.c and uses it
- patch 2 the same as you posted, but also removes is_dot_dotdot() from exp=
fs.c

No big deal.
Patch 1, as far as I am concerned, patch 1 can stay as it is

Thanks,
Amir.