[PATCH] jffs2: reduce stack usage in jffs2_build_xattr_subsystem()

[PATCH] jffs2: reduce stack usage in jffs2_build_xattr_subsystem()

[Christian Marangi]

From: Fabian Frederick <fabf@skynet.be>

Use kcalloc() for allocation/flush of 128 pointers table to
reduce stack usage.

Function now returns -ENOMEM or 0 on success.

stackusage
Before:
./fs/jffs2/xattr.c:775  jffs2_build_xattr_subsystem     1208
dynamic,bounded

After:
./fs/jffs2/xattr.c:775  jffs2_build_xattr_subsystem     192
dynamic,bounded

Also update definition when CONFIG_JFFS2_FS_XATTR is not enabled

Tested with an MTD mount point and some user set/getfattr.

Many current target on OpenWRT also suffer from a compilation warning
(that become an error with CONFIG_WERROR) with the following output:

fs/jffs2/xattr.c: In function 'jffs2_build_xattr_subsystem':
fs/jffs2/xattr.c:887:1: error: the frame size of 1088 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
  887 | }
      | ^

Using dynamic allocation fix this compilation warning.

Fixes: c9f700f840bd ("[JFFS2][XATTR] using 'delete marker' for xdatum/xref deletion")
Reported-by: Tim Gardner <tim.gardner@canonical.com>
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Ron Economos <re@w6rz.net>
Reported-by: Nathan Chancellor <nathan@kernel.org>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Fabian Frederick <fabf@skynet.be>
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
Cc: stable@vger.kernel.org
---
 fs/jffs2/build.c |  5 ++++-
 fs/jffs2/xattr.c | 13 +++++++++----
 fs/jffs2/xattr.h |  4 ++--
 3 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/fs/jffs2/build.c b/fs/jffs2/build.c
index 837cd55fd4c5..6ae9d6fefb86 100644
--- a/fs/jffs2/build.c
+++ b/fs/jffs2/build.c
@@ -211,7 +211,10 @@ static int jffs2_build_filesystem(struct jffs2_sb_info *c)
 		ic->scan_dents = NULL;
 		cond_resched();
 	}
-	jffs2_build_xattr_subsystem(c);
+	ret = jffs2_build_xattr_subsystem(c);
+	if (ret)
+		goto exit;
+
 	c->flags &= ~JFFS2_SB_FLAG_BUILDING;
 
 	dbg_fsbuild("FS build complete\n");
diff --git a/fs/jffs2/xattr.c b/fs/jffs2/xattr.c
index aa4048a27f31..3b6bdc9a49e1 100644
--- a/fs/jffs2/xattr.c
+++ b/fs/jffs2/xattr.c
@@ -772,10 +772,10 @@ void jffs2_clear_xattr_subsystem(struct jffs2_sb_info *c)
 }
 
 #define XREF_TMPHASH_SIZE	(128)
-void jffs2_build_xattr_subsystem(struct jffs2_sb_info *c)
+int jffs2_build_xattr_subsystem(struct jffs2_sb_info *c)
 {
 	struct jffs2_xattr_ref *ref, *_ref;
-	struct jffs2_xattr_ref *xref_tmphash[XREF_TMPHASH_SIZE];
+	struct jffs2_xattr_ref **xref_tmphash;
 	struct jffs2_xattr_datum *xd, *_xd;
 	struct jffs2_inode_cache *ic;
 	struct jffs2_raw_node_ref *raw;
@@ -784,9 +784,12 @@ void jffs2_build_xattr_subsystem(struct jffs2_sb_info *c)
 
 	BUG_ON(!(c->flags & JFFS2_SB_FLAG_BUILDING));
 
+	xref_tmphash = kcalloc(XREF_TMPHASH_SIZE,
+			       sizeof(struct jffs2_xattr_ref *), GFP_KERNEL);
+	if (!xref_tmphash)
+		return -ENOMEM;
+
 	/* Phase.1 : Merge same xref */
-	for (i=0; i < XREF_TMPHASH_SIZE; i++)
-		xref_tmphash[i] = NULL;
 	for (ref=c->xref_temp; ref; ref=_ref) {
 		struct jffs2_xattr_ref *tmp;
 
@@ -884,6 +887,8 @@ void jffs2_build_xattr_subsystem(struct jffs2_sb_info *c)
 		     "%u of xref (%u dead, %u orphan) found.\n",
 		     xdatum_count, xdatum_unchecked_count, xdatum_orphan_count,
 		     xref_count, xref_dead_count, xref_orphan_count);
+	kfree(xref_tmphash);
+	return 0;
 }
 
 struct jffs2_xattr_datum *jffs2_setup_xattr_datum(struct jffs2_sb_info *c,
diff --git a/fs/jffs2/xattr.h b/fs/jffs2/xattr.h
index 720007b2fd65..1b5030a3349d 100644
--- a/fs/jffs2/xattr.h
+++ b/fs/jffs2/xattr.h
@@ -71,7 +71,7 @@ static inline int is_xattr_ref_dead(struct jffs2_xattr_ref *ref)
 #ifdef CONFIG_JFFS2_FS_XATTR
 
 extern void jffs2_init_xattr_subsystem(struct jffs2_sb_info *c);
-extern void jffs2_build_xattr_subsystem(struct jffs2_sb_info *c);
+extern int jffs2_build_xattr_subsystem(struct jffs2_sb_info *c);
 extern void jffs2_clear_xattr_subsystem(struct jffs2_sb_info *c);
 
 extern struct jffs2_xattr_datum *jffs2_setup_xattr_datum(struct jffs2_sb_info *c,
@@ -103,7 +103,7 @@ extern ssize_t jffs2_listxattr(struct dentry *, char *, size_t);
 #else
 
 #define jffs2_init_xattr_subsystem(c)
-#define jffs2_build_xattr_subsystem(c)
+#define jffs2_build_xattr_subsystem(c)		(0)
 #define jffs2_clear_xattr_subsystem(c)
 
 #define jffs2_xattr_do_crccheck_inode(c, ic)
-- 
2.39.2

Re: [PATCH] jffs2: reduce stack usage in jffs2_build_xattr_subsystem()

[Zhihao Cheng]

在 2023/5/6 12:56, Christian Marangi 写道:
> From: Fabian Frederick <fabf@skynet.be>
> 
> Use kcalloc() for allocation/flush of 128 pointers table to
> reduce stack usage.
> 
> Function now returns -ENOMEM or 0 on success.
> 
> stackusage
> Before:
> ./fs/jffs2/xattr.c:775  jffs2_build_xattr_subsystem     1208
> dynamic,bounded
> 
> After:
> ./fs/jffs2/xattr.c:775  jffs2_build_xattr_subsystem     192
> dynamic,bounded
> 
> Also update definition when CONFIG_JFFS2_FS_XATTR is not enabled
> 
> Tested with an MTD mount point and some user set/getfattr.
> 
> Many current target on OpenWRT also suffer from a compilation warning
> (that become an error with CONFIG_WERROR) with the following output:
> 
> fs/jffs2/xattr.c: In function 'jffs2_build_xattr_subsystem':
> fs/jffs2/xattr.c:887:1: error: the frame size of 1088 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
>    887 | }
>        | ^
> 
> Using dynamic allocation fix this compilation warning.
> 
> Fixes: c9f700f840bd ("[JFFS2][XATTR] using 'delete marker' for xdatum/xref deletion")
> Reported-by: Tim Gardner <tim.gardner@canonical.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Ron Economos <re@w6rz.net>
> Reported-by: Nathan Chancellor <nathan@kernel.org>
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Fabian Frederick <fabf@skynet.be>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> Cc: stable@vger.kernel.org
> ---
>   fs/jffs2/build.c |  5 ++++-
>   fs/jffs2/xattr.c | 13 +++++++++----
>   fs/jffs2/xattr.h |  4 ++--
>   3 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/jffs2/build.c b/fs/jffs2/build.c
> index 837cd55fd4c5..6ae9d6fefb86 100644
> --- a/fs/jffs2/build.c
> +++ b/fs/jffs2/build.c
> @@ -211,7 +211,10 @@ static int jffs2_build_filesystem(struct jffs2_sb_info *c)
>   		ic->scan_dents = NULL;
>   		cond_resched();
>   	}
> -	jffs2_build_xattr_subsystem(c);
> +	ret = jffs2_build_xattr_subsystem(c);
> +	if (ret)
> +		goto exit;
> +
>   	c->flags &= ~JFFS2_SB_FLAG_BUILDING;
>   
>   	dbg_fsbuild("FS build complete\n");
> diff --git a/fs/jffs2/xattr.c b/fs/jffs2/xattr.c
> index aa4048a27f31..3b6bdc9a49e1 100644
> --- a/fs/jffs2/xattr.c
> +++ b/fs/jffs2/xattr.c
> @@ -772,10 +772,10 @@ void jffs2_clear_xattr_subsystem(struct jffs2_sb_info *c)
>   }
>   
>   #define XREF_TMPHASH_SIZE	(128)
> -void jffs2_build_xattr_subsystem(struct jffs2_sb_info *c)
> +int jffs2_build_xattr_subsystem(struct jffs2_sb_info *c)
>   {
>   	struct jffs2_xattr_ref *ref, *_ref;
> -	struct jffs2_xattr_ref *xref_tmphash[XREF_TMPHASH_SIZE];
> +	struct jffs2_xattr_ref **xref_tmphash;
>   	struct jffs2_xattr_datum *xd, *_xd;
>   	struct jffs2_inode_cache *ic;
>   	struct jffs2_raw_node_ref *raw;
> @@ -784,9 +784,12 @@ void jffs2_build_xattr_subsystem(struct jffs2_sb_info *c)
>   
>   	BUG_ON(!(c->flags & JFFS2_SB_FLAG_BUILDING));
>   
> +	xref_tmphash = kcalloc(XREF_TMPHASH_SIZE,
> +			       sizeof(struct jffs2_xattr_ref *), GFP_KERNEL);
> +	if (!xref_tmphash)
> +		return -ENOMEM;
> +

I have made some fault injection tests, jffs2 works fine, this patch 
imports no memleak problems. It seems okay.

>   	/* Phase.1 : Merge same xref */
> -	for (i=0; i < XREF_TMPHASH_SIZE; i++)
> -		xref_tmphash[i] = NULL;
>   	for (ref=c->xref_temp; ref; ref=_ref) {
>   		struct jffs2_xattr_ref *tmp;
>   
> @@ -884,6 +887,8 @@ void jffs2_build_xattr_subsystem(struct jffs2_sb_info *c)
>   		     "%u of xref (%u dead, %u orphan) found.\n",
>   		     xdatum_count, xdatum_unchecked_count, xdatum_orphan_count,
>   		     xref_count, xref_dead_count, xref_orphan_count);
> +	kfree(xref_tmphash);
> +	return 0;
>   }
>   
>   struct jffs2_xattr_datum *jffs2_setup_xattr_datum(struct jffs2_sb_info *c,
> diff --git a/fs/jffs2/xattr.h b/fs/jffs2/xattr.h
> index 720007b2fd65..1b5030a3349d 100644
> --- a/fs/jffs2/xattr.h
> +++ b/fs/jffs2/xattr.h
> @@ -71,7 +71,7 @@ static inline int is_xattr_ref_dead(struct jffs2_xattr_ref *ref)
>   #ifdef CONFIG_JFFS2_FS_XATTR
>   
>   extern void jffs2_init_xattr_subsystem(struct jffs2_sb_info *c);
> -extern void jffs2_build_xattr_subsystem(struct jffs2_sb_info *c);
> +extern int jffs2_build_xattr_subsystem(struct jffs2_sb_info *c);
>   extern void jffs2_clear_xattr_subsystem(struct jffs2_sb_info *c);
>   
>   extern struct jffs2_xattr_datum *jffs2_setup_xattr_datum(struct jffs2_sb_info *c,
> @@ -103,7 +103,7 @@ extern ssize_t jffs2_listxattr(struct dentry *, char *, size_t);
>   #else
>   
>   #define jffs2_init_xattr_subsystem(c)
> -#define jffs2_build_xattr_subsystem(c)
> +#define jffs2_build_xattr_subsystem(c)		(0)
>   #define jffs2_clear_xattr_subsystem(c)
>   
>   #define jffs2_xattr_do_crccheck_inode(c, ic)
> 

Re: [PATCH] jffs2: reduce stack usage in jffs2_build_xattr_subsystem()

[Christian Brauner]

On Sat, 06 May 2023 06:56:12 +0200, Christian Marangi wrote:
> Use kcalloc() for allocation/flush of 128 pointers table to
> reduce stack usage.
> 
> Function now returns -ENOMEM or 0 on success.
> 
> stackusage
> Before:
> ./fs/jffs2/xattr.c:775  jffs2_build_xattr_subsystem     1208
> dynamic,bounded
> 
> [...]

I assume I was Cced to pick this up. I'm happy to do that. If this is
rather supposed to go through the jffs2 tree then please tell me so we
can drop it.

Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.misc

[1/1] jffs2: reduce stack usage in jffs2_build_xattr_subsystem()
      https://git.kernel.org/vfs/vfs/c/493e7cebb906