Message ID | 20170509203003.11986-1-fabf@skynet.be |
---|---|
State | Not Applicable |
Delegated to: | David Woodhouse |
Headers | show |
On Tue, May 09, 2017 at 10:30:03PM +0200, Fabian Frederick 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 > > 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. > > Signed-off-by: Fabian Frederick <fabf@skynet.be> Hi David, Any chance this patch can get picked up? It LGTM, and I see multiple reports of this issue on lore: https://lore.kernel.org/lkml/?q=jffs2_build_xattr_subsystem 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> > --- > fs/jffs2/build.c | 5 ++++- > fs/jffs2/xattr.c | 14 ++++++++++---- > fs/jffs2/xattr.h | 4 ++-- > 3 files changed, 16 insertions(+), 7 deletions(-) > > diff --git a/fs/jffs2/build.c b/fs/jffs2/build.c > index b288c8a..f88e0bf 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 da3e185..95c0496 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,13 @@ 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 +888,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 720007b..1b5030a 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.9.3 >
diff --git a/fs/jffs2/build.c b/fs/jffs2/build.c index b288c8a..f88e0bf 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 da3e185..95c0496 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,13 @@ 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 +888,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 720007b..1b5030a 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)
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. Signed-off-by: Fabian Frederick <fabf@skynet.be> --- fs/jffs2/build.c | 5 ++++- fs/jffs2/xattr.c | 14 ++++++++++---- fs/jffs2/xattr.h | 4 ++-- 3 files changed, 16 insertions(+), 7 deletions(-)