diff mbox series

[v2,1/2] exportfs: fix the fallback implementation of the get_name export operation

Message ID 170429517779.50646.9656897459585544068.stgit@bazille.1015granger.net (mailing list archive)
State New, archived
Headers show
Series fix the fallback implementation of get_name | expand

Commit Message

Chuck Lever Jan. 3, 2024, 3:19 p.m. UTC 
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.

This patch just ensures that we skip '.' and '..' rather than allowing 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-VBNXWeFKVwLW_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(-)

Comments

Amir Goldstein Jan. 4, 2024, 7:39 a.m. UTC | #1 
On Wed, Jan 3, 2024 at 5:19 PM Chuck Lever <cel@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.
>
> This patch just ensures that we skip '.' and '..' rather than allowing 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-VBNXWeFKVwLW_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 == ino && len <= NAME_MAX) {
> +       /* Ignore the '.' and '..' entries */
> +       if ((len > 2 || name[0] != '.' || (len == 2 && name[1] != '.')) &&
> +           buf->ino == ino && len <= NAME_MAX) {


Thank you for creating the helper, but if you already went to this trouble,
I think it is better to introduce is_dot_dotdot() as a local helper already
in this backportable patch, so that stable kernel code is same as upstream
code (good for future fixes) and then dedupe the local helper with the rest
of the local helpers in patch 2?

Thanks,
Amir.
Chuck Lever III Jan. 4, 2024, 2:27 p.m. UTC | #2 
On Thu, Jan 04, 2024 at 09:39:04AM +0200, Amir Goldstein wrote:
> On Wed, Jan 3, 2024 at 5:19 PM Chuck Lever <cel@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.
> >
> > This patch just ensures that we skip '.' and '..' rather than allowing 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-VBNXWeFKVwLW_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 == ino && len <= NAME_MAX) {
> > +       /* Ignore the '.' and '..' entries */
> > +       if ((len > 2 || name[0] != '.' || (len == 2 && name[1] != '.')) &&
> > +           buf->ino == ino && len <= NAME_MAX) {
> 
> 
> Thank you for creating the helper, but if you already went to this trouble,
> I think it is better to introduce is_dot_dotdot() as a local helper already
> in this backportable patch, so that stable kernel code is same as upstream
> 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.
Amir Goldstein Jan. 4, 2024, 5:43 p.m. UTC | #3 
On Thu, Jan 4, 2024 at 4:27 PM 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 PM Chuck Lever <cel@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.
> > >
> > > This patch just ensures that we skip '.' and '..' rather than allowing 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-VBNXWeFKVwLW_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 == ino && len <= NAME_MAX) {
> > > +       /* Ignore the '.' and '..' entries */
> > > +       if ((len > 2 || name[0] != '.' || (len == 2 && name[1] != '.')) &&
> > > +           buf->ino == ino && len <= NAME_MAX) {
> >
> >
> > Thank you for creating the helper, but if you already went to this trouble,
> > I think it is better to introduce is_dot_dotdot() as a local helper already
> > in this backportable patch, so that stable kernel code is same as upstream
> > 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 expfs.c

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

Thanks,
Amir.
diff mbox series

Patch

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] != '.')) &&
+	    buf->ino == ino && len <= NAME_MAX) {
 		memcpy(buf->name, name, len);
 		buf->name[len] = '\0';
 		buf->found = 1;