[PATCH v2 0/2] fix the fallback implementation of get_name

[PATCH v2 0/2] fix the fallback implementation of get_name

[Chuck Lever]

I've set up a testing topic branch for exportfs in my kernel.org git
repo:

  https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git

But IIRC, Christian wants exportfs changes to go through the VFS
tree. Please correct me if I'm wrong.

Changes since v1:
- Fixes: was dropped from 1/2
- Added a patch to hoist is_dot_dotdot() into linux/fs.h

---

Chuck Lever (1):
      fs: Create a generic is_dot_dotdot() utility

Trond Myklebust (1):
      exportfs: fix the fallback implementation of the get_name export operation


 fs/crypto/fname.c    |  8 +-------
 fs/ecryptfs/crypto.c | 10 ----------
 fs/exportfs/expfs.c  |  2 +-
 fs/f2fs/f2fs.h       | 11 -----------
 include/linux/fs.h   |  9 +++++++++
 5 files changed, 11 insertions(+), 29 deletions(-)

--
Chuck Lever

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

[Chuck Lever]

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

[PATCH v2 2/2] fs: Create a generic is_dot_dotdot() utility

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

De-duplicate the same functionality in several places by hoisting
the is_dot_dotdot() function into linux/fs.h.

Suggested-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/crypto/fname.c    |    8 +-------
 fs/ecryptfs/crypto.c |   10 ----------
 fs/exportfs/expfs.c  |    4 +---
 fs/f2fs/f2fs.h       |   11 -----------
 include/linux/fs.h   |    9 +++++++++
 5 files changed, 11 insertions(+), 31 deletions(-)

diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
index 7b3fc189593a..0ad52fbe51c9 100644
--- a/fs/crypto/fname.c
+++ b/fs/crypto/fname.c
@@ -74,13 +74,7 @@ struct fscrypt_nokey_name {
 
 static inline bool fscrypt_is_dot_dotdot(const struct qstr *str)
 {
-	if (str->len == 1 && str->name[0] == '.')
-		return true;
-
-	if (str->len == 2 && str->name[0] == '.' && str->name[1] == '.')
-		return true;
-
-	return false;
+	return is_dot_dotdot(str->name, str->len);
 }
 
 /**
diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
index 03bd55069d86..2fe0f3af1a08 100644
--- a/fs/ecryptfs/crypto.c
+++ b/fs/ecryptfs/crypto.c
@@ -1949,16 +1949,6 @@ int ecryptfs_encrypt_and_encode_filename(
 	return rc;
 }
 
-static bool is_dot_dotdot(const char *name, size_t name_size)
-{
-	if (name_size == 1 && name[0] == '.')
-		return true;
-	else if (name_size == 2 && name[0] == '.' && name[1] == '.')
-		return true;
-
-	return false;
-}
-
 /**
  * ecryptfs_decode_and_decrypt_filename - converts the encoded cipher text name to decoded plaintext
  * @plaintext_name: The plaintext name
diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
index 84af58eaf2ca..07ea3d62b298 100644
--- a/fs/exportfs/expfs.c
+++ b/fs/exportfs/expfs.c
@@ -255,9 +255,7 @@ static bool filldir_one(struct dir_context *ctx, const char *name, int len,
 		container_of(ctx, struct getdents_callback, ctx);
 
 	buf->sequence++;
-	/* Ignore the '.' and '..' entries */
-	if ((len > 2 || name[0] != '.' || (len == 2 && name[1] != '.')) &&
-	    buf->ino == ino && len <= NAME_MAX) {
+	if (buf->ino == ino && len <= NAME_MAX && !is_dot_dotdot(name, len)) {
 		memcpy(buf->name, name, len);
 		buf->name[len] = '\0';
 		buf->found = 1;
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 9043cedfa12b..322a3b8a3533 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -3368,17 +3368,6 @@ static inline bool f2fs_cp_error(struct f2fs_sb_info *sbi)
 	return is_set_ckpt_flags(sbi, CP_ERROR_FLAG);
 }
 
-static inline bool is_dot_dotdot(const u8 *name, size_t len)
-{
-	if (len == 1 && name[0] == '.')
-		return true;
-
-	if (len == 2 && name[0] == '.' && name[1] == '.')
-		return true;
-
-	return false;
-}
-
 static inline void *f2fs_kmalloc(struct f2fs_sb_info *sbi,
 					size_t size, gfp_t flags)
 {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 98b7a7a8c42e..179eea797c22 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2846,6 +2846,15 @@ extern bool path_is_under(const struct path *, const struct path *);
 
 extern char *file_path(struct file *, char *, int);
 
+static inline bool is_dot_dotdot(const char *name, size_t len)
+{
+	if (len == 1 && name[0] == '.')
+		return true;
+	if (len == 2 && name[0] == '.' && name[1] == '.')
+		return true;
+	return false;
+}
+
 #include <linux/err.h>
 
 /* needed for stackable file system support */

Re: [PATCH v2 2/2] fs: Create a generic is_dot_dotdot() utility

[Jeff Layton]

On Wed, 2024-01-03 at 10:19 -0500, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> De-duplicate the same functionality in several places by hoisting
> the is_dot_dotdot() function into linux/fs.h.
> 
> Suggested-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/crypto/fname.c    |    8 +-------
>  fs/ecryptfs/crypto.c |   10 ----------
>  fs/exportfs/expfs.c  |    4 +---
>  fs/f2fs/f2fs.h       |   11 -----------
>  include/linux/fs.h   |    9 +++++++++
>  5 files changed, 11 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
> index 7b3fc189593a..0ad52fbe51c9 100644
> --- a/fs/crypto/fname.c
> +++ b/fs/crypto/fname.c
> @@ -74,13 +74,7 @@ struct fscrypt_nokey_name {
>  
>  static inline bool fscrypt_is_dot_dotdot(const struct qstr *str)
>  {
> -	if (str->len == 1 && str->name[0] == '.')
> -		return true;
> -
> -	if (str->len == 2 && str->name[0] == '.' && str->name[1] == '.')
> -		return true;
> -
> -	return false;
> +	return is_dot_dotdot(str->name, str->len);
>  }
>  
>  /**
> diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
> index 03bd55069d86..2fe0f3af1a08 100644
> --- a/fs/ecryptfs/crypto.c
> +++ b/fs/ecryptfs/crypto.c
> @@ -1949,16 +1949,6 @@ int ecryptfs_encrypt_and_encode_filename(
>  	return rc;
>  }
>  
> -static bool is_dot_dotdot(const char *name, size_t name_size)
> -{
> -	if (name_size == 1 && name[0] == '.')
> -		return true;
> -	else if (name_size == 2 && name[0] == '.' && name[1] == '.')
> -		return true;
> -
> -	return false;
> -}
> -
>  /**
>   * ecryptfs_decode_and_decrypt_filename - converts the encoded cipher text name to decoded plaintext
>   * @plaintext_name: The plaintext name
> diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
> index 84af58eaf2ca..07ea3d62b298 100644
> --- a/fs/exportfs/expfs.c
> +++ b/fs/exportfs/expfs.c
> @@ -255,9 +255,7 @@ static bool filldir_one(struct dir_context *ctx, const char *name, int len,
>  		container_of(ctx, struct getdents_callback, ctx);
>  
>  	buf->sequence++;
> -	/* Ignore the '.' and '..' entries */
> -	if ((len > 2 || name[0] != '.' || (len == 2 && name[1] != '.')) &&
> -	    buf->ino == ino && len <= NAME_MAX) {
> +	if (buf->ino == ino && len <= NAME_MAX && !is_dot_dotdot(name, len)) {
>  		memcpy(buf->name, name, len);
>  		buf->name[len] = '\0';
>  		buf->found = 1;
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 9043cedfa12b..322a3b8a3533 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -3368,17 +3368,6 @@ static inline bool f2fs_cp_error(struct f2fs_sb_info *sbi)
>  	return is_set_ckpt_flags(sbi, CP_ERROR_FLAG);
>  }
>  
> -static inline bool is_dot_dotdot(const u8 *name, size_t len)
> -{
> -	if (len == 1 && name[0] == '.')
> -		return true;
> -
> -	if (len == 2 && name[0] == '.' && name[1] == '.')
> -		return true;
> -
> -	return false;
> -}
> -
>  static inline void *f2fs_kmalloc(struct f2fs_sb_info *sbi,
>  					size_t size, gfp_t flags)
>  {
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 98b7a7a8c42e..179eea797c22 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2846,6 +2846,15 @@ extern bool path_is_under(const struct path *, const struct path *);
>  
>  extern char *file_path(struct file *, char *, int);
>  
> +static inline bool is_dot_dotdot(const char *name, size_t len)
> +{
> +	if (len == 1 && name[0] == '.')
> +		return true;
> +	if (len == 2 && name[0] == '.' && name[1] == '.')
> +		return true;
> +	return false;
> +}
> +
>  #include <linux/err.h>
>  
>  /* needed for stackable file system support */
> 
> 

Looks good to me. I took a quick look to see if there were other open-
coded versions, but I didn't see any.

Reviewed-by: Jeff Layton <jlayton@kernel.org>

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

[Amir Goldstein]

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.

Re: [PATCH v2 2/2] fs: Create a generic is_dot_dotdot() utility

[Amir Goldstein]

On Wed, Jan 3, 2024 at 9:08 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Wed, 2024-01-03 at 10:19 -0500, Chuck Lever wrote:
> > From: Chuck Lever <chuck.lever@oracle.com>
> >
> > De-duplicate the same functionality in several places by hoisting
> > the is_dot_dotdot() function into linux/fs.h.
> >
> > Suggested-by: Amir Goldstein <amir73il@gmail.com>
> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > ---
> >  fs/crypto/fname.c    |    8 +-------
> >  fs/ecryptfs/crypto.c |   10 ----------
> >  fs/exportfs/expfs.c  |    4 +---
> >  fs/f2fs/f2fs.h       |   11 -----------
> >  include/linux/fs.h   |    9 +++++++++
> >  5 files changed, 11 insertions(+), 31 deletions(-)
> >
> > diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
> > index 7b3fc189593a..0ad52fbe51c9 100644
> > --- a/fs/crypto/fname.c
> > +++ b/fs/crypto/fname.c
> > @@ -74,13 +74,7 @@ struct fscrypt_nokey_name {
> >
> >  static inline bool fscrypt_is_dot_dotdot(const struct qstr *str)
> >  {
> > -     if (str->len == 1 && str->name[0] == '.')
> > -             return true;
> > -
> > -     if (str->len == 2 && str->name[0] == '.' && str->name[1] == '.')
> > -             return true;
> > -
> > -     return false;
> > +     return is_dot_dotdot(str->name, str->len);
> >  }
> >
> >  /**
> > diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
> > index 03bd55069d86..2fe0f3af1a08 100644
> > --- a/fs/ecryptfs/crypto.c
> > +++ b/fs/ecryptfs/crypto.c
> > @@ -1949,16 +1949,6 @@ int ecryptfs_encrypt_and_encode_filename(
> >       return rc;
> >  }
> >
> > -static bool is_dot_dotdot(const char *name, size_t name_size)
> > -{
> > -     if (name_size == 1 && name[0] == '.')
> > -             return true;
> > -     else if (name_size == 2 && name[0] == '.' && name[1] == '.')
> > -             return true;
> > -
> > -     return false;
> > -}
> > -
> >  /**
> >   * ecryptfs_decode_and_decrypt_filename - converts the encoded cipher text name to decoded plaintext
> >   * @plaintext_name: The plaintext name
> > diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
> > index 84af58eaf2ca..07ea3d62b298 100644
> > --- a/fs/exportfs/expfs.c
> > +++ b/fs/exportfs/expfs.c
> > @@ -255,9 +255,7 @@ static bool filldir_one(struct dir_context *ctx, const char *name, int len,
> >               container_of(ctx, struct getdents_callback, ctx);
> >
> >       buf->sequence++;
> > -     /* Ignore the '.' and '..' entries */
> > -     if ((len > 2 || name[0] != '.' || (len == 2 && name[1] != '.')) &&
> > -         buf->ino == ino && len <= NAME_MAX) {
> > +     if (buf->ino == ino && len <= NAME_MAX && !is_dot_dotdot(name, len)) {
> >               memcpy(buf->name, name, len);
> >               buf->name[len] = '\0';
> >               buf->found = 1;
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index 9043cedfa12b..322a3b8a3533 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -3368,17 +3368,6 @@ static inline bool f2fs_cp_error(struct f2fs_sb_info *sbi)
> >       return is_set_ckpt_flags(sbi, CP_ERROR_FLAG);
> >  }
> >
> > -static inline bool is_dot_dotdot(const u8 *name, size_t len)
> > -{
> > -     if (len == 1 && name[0] == '.')
> > -             return true;
> > -
> > -     if (len == 2 && name[0] == '.' && name[1] == '.')
> > -             return true;
> > -
> > -     return false;
> > -}
> > -
> >  static inline void *f2fs_kmalloc(struct f2fs_sb_info *sbi,
> >                                       size_t size, gfp_t flags)
> >  {
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 98b7a7a8c42e..179eea797c22 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2846,6 +2846,15 @@ extern bool path_is_under(const struct path *, const struct path *);
> >
> >  extern char *file_path(struct file *, char *, int);
> >
> > +static inline bool is_dot_dotdot(const char *name, size_t len)
> > +{
> > +     if (len == 1 && name[0] == '.')
> > +             return true;
> > +     if (len == 2 && name[0] == '.' && name[1] == '.')
> > +             return true;
> > +     return false;
> > +}
> > +
> >  #include <linux/err.h>
> >
> >  /* needed for stackable file system support */
> >
> >
>
> Looks good to me. I took a quick look to see if there were other open-
> coded versions, but I didn't see any.
>

The outstanding open-coded version that wasn't deduped is in
lookup_one_common(), which is the version that Trond used and
mentioned in his patch.

It is also a slightly more "efficient" version, but I have no idea if
that really matters.

In any case, having lookup_one_common() and get_name() use
the same helper is clearly prefered, because the check in lookup_one()
is the declared reason for the get_name() patch.

Thanks,
Amir.

Re: [PATCH v2 2/2] fs: Create a generic is_dot_dotdot() utility

[Chuck Lever]

On Thu, Jan 04, 2024 at 09:46:07AM +0200, Amir Goldstein wrote:
> On Wed, Jan 3, 2024 at 9:08 PM Jeff Layton <jlayton@kernel.org> wrote:
> >
> > On Wed, 2024-01-03 at 10:19 -0500, Chuck Lever wrote:
> > > From: Chuck Lever <chuck.lever@oracle.com>
> > >
> > > De-duplicate the same functionality in several places by hoisting
> > > the is_dot_dotdot() function into linux/fs.h.
> > >
> > > Suggested-by: Amir Goldstein <amir73il@gmail.com>
> > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > > ---
> > >  fs/crypto/fname.c    |    8 +-------
> > >  fs/ecryptfs/crypto.c |   10 ----------
> > >  fs/exportfs/expfs.c  |    4 +---
> > >  fs/f2fs/f2fs.h       |   11 -----------
> > >  include/linux/fs.h   |    9 +++++++++
> > >  5 files changed, 11 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
> > > index 7b3fc189593a..0ad52fbe51c9 100644
> > > --- a/fs/crypto/fname.c
> > > +++ b/fs/crypto/fname.c
> > > @@ -74,13 +74,7 @@ struct fscrypt_nokey_name {
> > >
> > >  static inline bool fscrypt_is_dot_dotdot(const struct qstr *str)
> > >  {
> > > -     if (str->len == 1 && str->name[0] == '.')
> > > -             return true;
> > > -
> > > -     if (str->len == 2 && str->name[0] == '.' && str->name[1] == '.')
> > > -             return true;
> > > -
> > > -     return false;
> > > +     return is_dot_dotdot(str->name, str->len);
> > >  }
> > >
> > >  /**
> > > diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
> > > index 03bd55069d86..2fe0f3af1a08 100644
> > > --- a/fs/ecryptfs/crypto.c
> > > +++ b/fs/ecryptfs/crypto.c
> > > @@ -1949,16 +1949,6 @@ int ecryptfs_encrypt_and_encode_filename(
> > >       return rc;
> > >  }
> > >
> > > -static bool is_dot_dotdot(const char *name, size_t name_size)
> > > -{
> > > -     if (name_size == 1 && name[0] == '.')
> > > -             return true;
> > > -     else if (name_size == 2 && name[0] == '.' && name[1] == '.')
> > > -             return true;
> > > -
> > > -     return false;
> > > -}
> > > -
> > >  /**
> > >   * ecryptfs_decode_and_decrypt_filename - converts the encoded cipher text name to decoded plaintext
> > >   * @plaintext_name: The plaintext name
> > > diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
> > > index 84af58eaf2ca..07ea3d62b298 100644
> > > --- a/fs/exportfs/expfs.c
> > > +++ b/fs/exportfs/expfs.c
> > > @@ -255,9 +255,7 @@ static bool filldir_one(struct dir_context *ctx, const char *name, int len,
> > >               container_of(ctx, struct getdents_callback, ctx);
> > >
> > >       buf->sequence++;
> > > -     /* Ignore the '.' and '..' entries */
> > > -     if ((len > 2 || name[0] != '.' || (len == 2 && name[1] != '.')) &&
> > > -         buf->ino == ino && len <= NAME_MAX) {
> > > +     if (buf->ino == ino && len <= NAME_MAX && !is_dot_dotdot(name, len)) {
> > >               memcpy(buf->name, name, len);
> > >               buf->name[len] = '\0';
> > >               buf->found = 1;
> > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > index 9043cedfa12b..322a3b8a3533 100644
> > > --- a/fs/f2fs/f2fs.h
> > > +++ b/fs/f2fs/f2fs.h
> > > @@ -3368,17 +3368,6 @@ static inline bool f2fs_cp_error(struct f2fs_sb_info *sbi)
> > >       return is_set_ckpt_flags(sbi, CP_ERROR_FLAG);
> > >  }
> > >
> > > -static inline bool is_dot_dotdot(const u8 *name, size_t len)
> > > -{
> > > -     if (len == 1 && name[0] == '.')
> > > -             return true;
> > > -
> > > -     if (len == 2 && name[0] == '.' && name[1] == '.')
> > > -             return true;
> > > -
> > > -     return false;
> > > -}
> > > -
> > >  static inline void *f2fs_kmalloc(struct f2fs_sb_info *sbi,
> > >                                       size_t size, gfp_t flags)
> > >  {
> > > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > > index 98b7a7a8c42e..179eea797c22 100644
> > > --- a/include/linux/fs.h
> > > +++ b/include/linux/fs.h
> > > @@ -2846,6 +2846,15 @@ extern bool path_is_under(const struct path *, const struct path *);
> > >
> > >  extern char *file_path(struct file *, char *, int);
> > >
> > > +static inline bool is_dot_dotdot(const char *name, size_t len)
> > > +{
> > > +     if (len == 1 && name[0] == '.')
> > > +             return true;
> > > +     if (len == 2 && name[0] == '.' && name[1] == '.')
> > > +             return true;
> > > +     return false;
> > > +}
> > > +
> > >  #include <linux/err.h>
> > >
> > >  /* needed for stackable file system support */
> > >
> > >
> >
> > Looks good to me. I took a quick look to see if there were other open-
> > coded versions, but I didn't see any.
> >
> 
> The outstanding open-coded version that wasn't deduped is in
> lookup_one_common(), which is the version that Trond used and
> mentioned in his patch.
> 
> It is also a slightly more "efficient" version, but I have no idea if
> that really matters.

It probably matters in lookup_one_common(), but not in any of the
other callers.


> In any case, having lookup_one_common() and get_name() use
> the same helper is clearly prefered, because the check in lookup_one()
> is the declared reason for the get_name() patch.

I will send a v3.

-- 
Chuck Lever

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

[Chuck Lever]

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.


-- 
Chuck Lever

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

[Amir Goldstein]

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.