Re: [PATCH] ovl: fix wrong lowerdir number check for parameter Opt_lowerdir

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.





[Index of Archives]     [Linux Filesystems Devel]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux