On Wed, Jul 3, 2024 at 4:48 PM Christian Brauner <brauner@xxxxxxxxxx> wrote: > > On Wed, Jul 03, 2024 at 02:35:49PM GMT, Amir Goldstein wrote: > > On Wed, Jul 3, 2024 at 7:48 AM Zhihao Cheng <chengzhihao1@xxxxxxxxxx> wrote: > > > > > > The max count of lowerdir is OVL_MAX_STACK[500], which is broken by > > > commit 37f32f526438("ovl: fix memory leak in ovl_parse_param()") for > > > parameter Opt_lowerdir. Since commit 819829f0319a("ovl: refactor layer > > > parsing helpers") and commit 24e16e385f22("ovl: add support for > > > appending lowerdirs one by one") added check ovl_mount_dir_check() in > > > function ovl_parse_param_lowerdir(), the 'ctx->nr' should be smaller > > > than OVL_MAX_STACK, after commit 37f32f526438("ovl: fix memory leak in > > > ovl_parse_param()") is applied, the 'ctx->nr' is updated before the > > > check ovl_mount_dir_check(), which leads the max count of lowerdir > > > to become 499 for parameter Opt_lowerdir. > > > Fix it by updating 'ctx->nr' after the check ovl_mount_dir_check(). > > > > > > Fixes: 37f32f526438 ("ovl: fix memory leak in ovl_parse_param()") > > > Signed-off-by: Zhihao Cheng <chengzhihao1@xxxxxxxxxx> > > > --- > > > fs/overlayfs/params.c | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c > > > index 4860fcc4611b..0d8c456aa8fa 100644 > > > --- a/fs/overlayfs/params.c > > > +++ b/fs/overlayfs/params.c > > > @@ -486,7 +486,6 @@ static int ovl_parse_param_lowerdir(const char *name, struct fs_context *fc) > > > iter = dup; > > > l = ctx->lower; > > > for (nr = 0; nr < nr_lower; nr++, l++) { > > > - ctx->nr++; > > > memset(l, 0, sizeof(*l)); > > > > > > err = ovl_mount_dir(iter, &l->path); > > > @@ -494,9 +493,12 @@ static int ovl_parse_param_lowerdir(const char *name, struct fs_context *fc) > > > goto out_put; > > > > > > err = ovl_mount_dir_check(fc, &l->path, Opt_lowerdir, iter, false); > > > - if (err) > > > + if (err) { > > > + path_put(&l->path); > > > goto out_put; > > > + } > > > > > > + ctx->nr++; > > > err = -ENOMEM; > > > l->name = kstrdup(iter, GFP_KERNEL_ACCOUNT); > > > if (!l->name) > > > -- > > > 2.39.2 > > > > > > > This fix looks correct, but it is not pretty IMO. > > The cleanup on error is much cleaner in ovl_parse_layer() -> ovl_add_layer() > > I wonder if we can reuse some of those helpers instead of the current code. > > > > Christian, what do you think? > > Yeah, sounds good. Something like the completely untested below. > Feel free to reuse in whatever form. This looks much nicer! I think you unintentionally dropped incrementing of ctx->nr_data for the notorious case of ::<lowerdatadir>. Zhihao, Please make sure to run the fstests overlay test for lowerdatadirs overlay/079 overlay/085 overlay/086 Thanks, Amir.