[PATCH] ovl: fix error for ovl_fill_super()

[PATCH] ovl: fix error for ovl_fill_super()

[Chengguang Xu]

There are some places should return -EINVAL instead of
-ENOMEM in ovl_fill_super(), so just fix it.

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 fs/overlayfs/super.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index fdd72f1a9c5e..3dda1d530a43 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1984,6 +1984,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	if (numlower > OVL_MAX_STACK) {
 		pr_err("too many lower directories, limit is %d\n",
 		       OVL_MAX_STACK);
+		err = -EINVAL;
 		goto out_err;
 	}
 
@@ -2010,6 +2011,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	/* alloc/destroy_inode needed for setting up traps in inode cache */
 	sb->s_op = &ovl_super_operations;
 
+	err = -EINVAL;
 	if (ofs->config.upperdir) {
 		struct super_block *upper_sb;
 
-- 
2.27.0

Re: [PATCH] ovl: fix error for ovl_fill_super()

[Amir Goldstein]

On Mon, Mar 1, 2021 at 8:28 AM Chengguang Xu <cgxu519@mykernel.net> wrote:
>
> There are some places should return -EINVAL instead of
> -ENOMEM in ovl_fill_super(), so just fix it.
>
> Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
> ---
>  fs/overlayfs/super.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index fdd72f1a9c5e..3dda1d530a43 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -1984,6 +1984,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>         if (numlower > OVL_MAX_STACK) {
>                 pr_err("too many lower directories, limit is %d\n",
>                        OVL_MAX_STACK);
> +               err = -EINVAL;
>                 goto out_err;
>         }
>
> @@ -2010,6 +2011,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>         /* alloc/destroy_inode needed for setting up traps in inode cache */
>         sb->s_op = &ovl_super_operations;
>
> +       err = -EINVAL;
>         if (ofs->config.upperdir) {
>                 struct super_block *upper_sb;
>

OK. But we are not really being consistent in the ways we set err in this
function, which means we will have more of these bugs in the future
(and we have had them in the past as well...)

So either set err = -EINVAL after every pr_err() in this function:
- "missing lowerdir"
- "too many lower dirs"
- "missing workdir"

Or always set err before the tests including err = -ENOMEM
before allocating layers.

Mixing seems worse than either choice IMO.

Maybe Miklos has a better idea for tyding this up?

Thanks,
Amir.

Re: [PATCH] ovl: fix error for ovl_fill_super()

[Miklos Szeredi]

On Mon, Mar 1, 2021 at 8:57 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Mon, Mar 1, 2021 at 8:28 AM Chengguang Xu <cgxu519@mykernel.net> wrote:
> >
> > There are some places should return -EINVAL instead of
> > -ENOMEM in ovl_fill_super(), so just fix it.
> >
> > Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
> > ---
> >  fs/overlayfs/super.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> > index fdd72f1a9c5e..3dda1d530a43 100644
> > --- a/fs/overlayfs/super.c
> > +++ b/fs/overlayfs/super.c
> > @@ -1984,6 +1984,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
> >         if (numlower > OVL_MAX_STACK) {
> >                 pr_err("too many lower directories, limit is %d\n",
> >                        OVL_MAX_STACK);
> > +               err = -EINVAL;
> >                 goto out_err;
> >         }
> >
> > @@ -2010,6 +2011,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
> >         /* alloc/destroy_inode needed for setting up traps in inode cache */
> >         sb->s_op = &ovl_super_operations;
> >
> > +       err = -EINVAL;
> >         if (ofs->config.upperdir) {
> >                 struct super_block *upper_sb;
> >
>
> OK. But we are not really being consistent in the ways we set err in this
> function, which means we will have more of these bugs in the future
> (and we have had them in the past as well...)
>
> So either set err = -EINVAL after every pr_err() in this function:
> - "missing lowerdir"
> - "too many lower dirs"
> - "missing workdir"
>
> Or always set err before the tests including err = -ENOMEM
> before allocating layers.
>
> Mixing seems worse than either choice IMO.
>
> Maybe Miklos has a better idea for tyding this up?

Thanks, applied with consistency fixup.

Miklos