($INBOX_DIR/description missing)
 help / color / Atom feed
* [PATCH] ovl: fix error for ovl_fill_super()
@ 2021-03-01  6:19 Chengguang Xu
  2021-03-01  7:57 ` Amir Goldstein
  0 siblings, 1 reply; 3+ messages in thread
From: Chengguang Xu @ 2021-03-01  6:19 UTC (permalink / raw)
  To: miklos; +Cc: linux-unionfs, 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



^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] ovl: fix error for ovl_fill_super()
  2021-03-01  6:19 [PATCH] ovl: fix error for ovl_fill_super() Chengguang Xu
@ 2021-03-01  7:57 ` Amir Goldstein
  2021-03-24 16:12   ` Miklos Szeredi
  0 siblings, 1 reply; 3+ messages in thread
From: Amir Goldstein @ 2021-03-01  7:57 UTC (permalink / raw)
  To: Chengguang Xu; +Cc: Miklos Szeredi, overlayfs

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.

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] ovl: fix error for ovl_fill_super()
  2021-03-01  7:57 ` Amir Goldstein
@ 2021-03-24 16:12   ` Miklos Szeredi
  0 siblings, 0 replies; 3+ messages in thread
From: Miklos Szeredi @ 2021-03-24 16:12 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Chengguang Xu, overlayfs

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-01  6:19 [PATCH] ovl: fix error for ovl_fill_super() Chengguang Xu
2021-03-01  7:57 ` Amir Goldstein
2021-03-24 16:12   ` Miklos Szeredi

($INBOX_DIR/description missing)

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-unionfs/0 linux-unionfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-unionfs linux-unionfs/ https://lore.kernel.org/linux-unionfs \
		linux-unionfs@vger.kernel.org
	public-inbox-index linux-unionfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-unionfs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git