[PATCH 1/3] ovl: print error message for invalid mount options

[PATCH 1/3] ovl: print error message for invalid mount options

[hujianyang]

Overlayfs should print an error message if an incorrect mount option
is caught like other filesystems.

After this patch, improper option input could be clearly known.

Reported-by: Fabian Sturm <fabian.sturm@aduu.de>
Signed-off-by: hujianyang <hujianyang@huawei.com>
---
 fs/overlayfs/super.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index b90952f..ab3c8cb 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -615,6 +615,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 			break;

 		default:
+			pr_err("overlayfs: unrecognized mount option \"%s\" or missing value\n", p);
 			return -EINVAL;
 		}
 	}
-- 
1.6.0.2

[PATCH 2/3] ovl: check lowerdir amount for non-upper mount

[hujianyang]

Recently multi-lower layer mount support allow upperdir and workdir
to be omitted, then cause overlayfs can be mount with only one
lowerdir directory. This action make no sense and have potential risk.

This patch check the total number of lower directories to prevent
mounting overlayfs with only one directory.

Also, an error message is added to indicate lower directories exceed
*OVL_MAX_STACK* limit.

Signed-off-by: hujianyang <hujianyang@huawei.com>
---
 fs/overlayfs/super.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index ab3c8cb..edbb3eb 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -870,8 +870,14 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)

 	err = -EINVAL;
 	stacklen = ovl_split_lowerdirs(lowertmp);
-	if (stacklen > OVL_MAX_STACK)
+	if (stacklen > OVL_MAX_STACK) {
+		pr_err("overlayfs: too many lower directries, limit is %d\n",
+		       OVL_MAX_STACK);
 		goto out_free_lowertmp;
+	} else if (!ufs->config.upperdir && stacklen == 1) {
+		pr_err("overlayfs: at least 2 lowerdir are needed while upperdir nonexistent\n");
+		goto out_free_lowertmp;
+	}

 	stack = kcalloc(stacklen, sizeof(struct path), GFP_KERNEL);
 	if (!stack)
-- 
1.6.0.2

[PATCH 3/3] ovl: upper fs should not be R/O

[hujianyang]

After importing multi-lower layer support, users could mount a r/o
partition as the left most lowerdir instead of using it as upperdir.
And a r/o upperdir may cause an error like

	overlayfs: failed to create directory ./workdir/work

during mount.

This patch check the *s_flags* of upper fs and return an error if
it is a r/o partition. The checking of *upper_mnt->mnt_sb->s_flags*
can be removed now.

This patch also remove

	/* FIXME: workdir is not needed for a R/O mount */
	
from ovl_fill_super() because:

1) for upper fs r/o case
Setting a r/o partition as upper is prevented, no need to care about
workdir in this case.

2) for "mount overlay -o ro" with a r/w upper fs case
Users could remount overlayfs to r/w in this case, so workdir should
not be omitted.

Signed-off-by: hujianyang <hujianyang@huawei.com>
---
 fs/overlayfs/super.c |   24 +++++++++++++++++++-----
 1 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index edbb3eb..0e7a477 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -529,8 +529,7 @@ static int ovl_remount(struct super_block *sb, int *flags, char *data)
 {
 	struct ovl_fs *ufs = sb->s_fs_info;

-	if (!(*flags & MS_RDONLY) &&
-	    (!ufs->upper_mnt || (ufs->upper_mnt->mnt_sb->s_flags & MS_RDONLY)))
+	if (!(*flags & MS_RDONLY) && (!ufs->upper_mnt))
 		return -EROFS;

 	return 0;
@@ -619,6 +618,15 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 			return -EINVAL;
 		}
 	}
+
+	/* Workdir is useless in non-upper mount */
+	if (!config->upperdir && config->workdir) {
+		pr_info("overlayfs: option \"workdir=%s\" is useless in a non-upper mount, ignore\n",
+			config->workdir);
+		kfree(config->workdir);
+		config->workdir = NULL;
+	}
+
 	return 0;
 }

@@ -838,7 +846,6 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)

 	sb->s_stack_depth = 0;
 	if (ufs->config.upperdir) {
-		/* FIXME: workdir is not needed for a R/O mount */
 		if (!ufs->config.workdir) {
 			pr_err("overlayfs: missing 'workdir'\n");
 			goto out_free_config;
@@ -848,6 +855,13 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 		if (err)
 			goto out_free_config;

+		/* Upper fs should not be r/o */
+		if (upperpath.mnt->mnt_sb->s_flags & MS_RDONLY) {
+			pr_err("overlayfs: upper fs is r/o, try multi-lower layers mount\n");
+			err = -EINVAL;
+			goto out_put_upperpath;
+		}
+
 		err = ovl_mount_dir(ufs->config.workdir, &workpath);
 		if (err)
 			goto out_put_upperpath;
@@ -939,8 +953,8 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 		ufs->numlower++;
 	}

-	/* If the upper fs is r/o or nonexistent, we mark overlayfs r/o too */
-	if (!ufs->upper_mnt || (ufs->upper_mnt->mnt_sb->s_flags & MS_RDONLY))
+	/* If the upper fs is nonexistent, we mark overlayfs r/o too */
+	if (!ufs->upper_mnt)
 		sb->s_flags |= MS_RDONLY;

 	sb->s_d_op = &ovl_dentry_operations;
-- 
1.6.0.2

Re: [PATCH 3/3] ovl: upper fs should not be R/O

[Seunghun Lee]

On January 15, 2015 2:20:57 PM GMT+09:00, hujianyang <hujianyang@huawei.com> wrote:
>After importing multi-lower layer support, users could mount a r/o
>partition as the left most lowerdir instead of using it as upperdir.
>And a r/o upperdir may cause an error like
>
>	overlayfs: failed to create directory ./workdir/work
>
>during mount.
>
>This patch check the *s_flags* of upper fs and return an error if
>it is a r/o partition. The checking of *upper_mnt->mnt_sb->s_flags*
>can be removed now.
>
>This patch also remove
>
>	/* FIXME: workdir is not needed for a R/O mount */
>	
>from ovl_fill_super() because:
>
>1) for upper fs r/o case
>Setting a r/o partition as upper is prevented, no need to care about
>workdir in this case.
>
>2) for "mount overlay -o ro" with a r/w upper fs case
>Users could remount overlayfs to r/w in this case, so workdir should
>not be omitted.
>
>Signed-off-by: hujianyang <hujianyang@huawei.com>
>---
> fs/overlayfs/super.c |   24 +++++++++++++++++++-----
> 1 files changed, 19 insertions(+), 5 deletions(-)
>
>diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
>index edbb3eb..0e7a477 100644
>--- a/fs/overlayfs/super.c
>+++ b/fs/overlayfs/super.c
>@@ -529,8 +529,7 @@ static int ovl_remount(struct super_block *sb, int
>*flags, char *data)
> {
> 	struct ovl_fs *ufs = sb->s_fs_info;
>
>-	if (!(*flags & MS_RDONLY) &&
>-	    (!ufs->upper_mnt || (ufs->upper_mnt->mnt_sb->s_flags &
>MS_RDONLY)))
>+	if (!(*flags & MS_RDONLY) && (!ufs->upper_mnt))
> 		return -EROFS;
>
> 	return 0;
>@@ -619,6 +618,15 @@ static int ovl_parse_opt(char *opt, struct
>ovl_config *config)
> 			return -EINVAL;
> 		}
> 	}
>+
>+	/* Workdir is useless in non-upper mount */
>+	if (!config->upperdir && config->workdir) {
>+		pr_info("overlayfs: option \"workdir=%s\" is useless in a non-upper
>mount, ignore\n",
>+			config->workdir);
>+		kfree(config->workdir);
>+		config->workdir = NULL;
>+	}
>+
> 	return 0;
> }
>
>@@ -838,7 +846,6 @@ static int ovl_fill_super(struct super_block *sb,
>void *data, int silent)
>
> 	sb->s_stack_depth = 0;
> 	if (ufs->config.upperdir) {
>-		/* FIXME: workdir is not needed for a R/O mount */
> 		if (!ufs->config.workdir) {
> 			pr_err("overlayfs: missing 'workdir'\n");
> 			goto out_free_config;
>@@ -848,6 +855,13 @@ static int ovl_fill_super(struct super_block *sb,
>void *data, int silent)
> 		if (err)
> 			goto out_free_config;
>
>+		/* Upper fs should not be r/o */
>+		if (upperpath.mnt->mnt_sb->s_flags & MS_RDONLY) {
>+			pr_err("overlayfs: upper fs is r/o, try multi-lower layers
>mount\n");
>+			err = -EINVAL;
>+			goto out_put_upperpath;
>+		}
>+
> 		err = ovl_mount_dir(ufs->config.workdir, &workpath);
> 		if (err)
> 			goto out_put_upperpath;
>@@ -939,8 +953,8 @@ static int ovl_fill_super(struct super_block *sb,
>void *data, int silent)
> 		ufs->numlower++;
> 	}
>
>-	/* If the upper fs is r/o or nonexistent, we mark overlayfs r/o too
>*/
>-	if (!ufs->upper_mnt || (ufs->upper_mnt->mnt_sb->s_flags & MS_RDONLY))
>+	/* If the upper fs is nonexistent, we mark overlayfs r/o too */
>+	if (!ufs->upper_mnt)
> 		sb->s_flags |= MS_RDONLY;
>
> 	sb->s_d_op = &ovl_dentry_operations;

It works fine to me.
And I think it is better than my implementation : )

Thanks.

Re: [PATCH 3/3] ovl: upper fs should not be R/O

[A. Wan]

Just one question.  If each r/o layer does not require a workdir, why
would a stack of r/o layers require one - and hence the requirement that
the top layer must be r/w?

Does it has to do with why workdir was introduced in the first place? 
Sorry but I couldn't find information about why workdir was introduced.  I
suppose it was to support some functions that older versions can't.

Alex

On Thu, January 15, 2015 12:20 am, Seunghun Lee wrote:
> On January 15, 2015 2:20:57 PM GMT+09:00, hujianyang
> <hujianyang@huawei.com> wrote:
>>After importing multi-lower layer support, users could mount a r/o
>>partition as the left most lowerdir instead of using it as upperdir.
>>And a r/o upperdir may cause an error like
>>
>>	overlayfs: failed to create directory ./workdir/work
>>
>>during mount.
>>
>>This patch check the *s_flags* of upper fs and return an error if
>>it is a r/o partition. The checking of *upper_mnt->mnt_sb->s_flags*
>>can be removed now.
>>
>>This patch also remove
>>
>>	/* FIXME: workdir is not needed for a R/O mount */
>>
>>from ovl_fill_super() because:
>>
>>1) for upper fs r/o case
>>Setting a r/o partition as upper is prevented, no need to care about
>>workdir in this case.
>>
>>2) for "mount overlay -o ro" with a r/w upper fs case
>>Users could remount overlayfs to r/w in this case, so workdir should
>>not be omitted.
>>
>>Signed-off-by: hujianyang <hujianyang@huawei.com>
>>---
>> fs/overlayfs/super.c |   24 +++++++++++++++++++-----
>> 1 files changed, 19 insertions(+), 5 deletions(-)
>>
>>diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
>>index edbb3eb..0e7a477 100644
>>--- a/fs/overlayfs/super.c
>>+++ b/fs/overlayfs/super.c
>>@@ -529,8 +529,7 @@ static int ovl_remount(struct super_block *sb, int
>>*flags, char *data)
>> {
>> 	struct ovl_fs *ufs = sb->s_fs_info;
>>
>>-	if (!(*flags & MS_RDONLY) &&
>>-	    (!ufs->upper_mnt || (ufs->upper_mnt->mnt_sb->s_flags &
>>MS_RDONLY)))
>>+	if (!(*flags & MS_RDONLY) && (!ufs->upper_mnt))
>> 		return -EROFS;
>>
>> 	return 0;
>>@@ -619,6 +618,15 @@ static int ovl_parse_opt(char *opt, struct
>>ovl_config *config)
>> 			return -EINVAL;
>> 		}
>> 	}
>>+
>>+	/* Workdir is useless in non-upper mount */
>>+	if (!config->upperdir && config->workdir) {
>>+		pr_info("overlayfs: option \"workdir=%s\" is useless in a non-upper
>>mount, ignore\n",
>>+			config->workdir);
>>+		kfree(config->workdir);
>>+		config->workdir = NULL;
>>+	}
>>+
>> 	return 0;
>> }
>>
>>@@ -838,7 +846,6 @@ static int ovl_fill_super(struct super_block *sb,
>>void *data, int silent)
>>
>> 	sb->s_stack_depth = 0;
>> 	if (ufs->config.upperdir) {
>>-		/* FIXME: workdir is not needed for a R/O mount */
>> 		if (!ufs->config.workdir) {
>> 			pr_err("overlayfs: missing 'workdir'\n");
>> 			goto out_free_config;
>>@@ -848,6 +855,13 @@ static int ovl_fill_super(struct super_block *sb,
>>void *data, int silent)
>> 		if (err)
>> 			goto out_free_config;
>>
>>+		/* Upper fs should not be r/o */
>>+		if (upperpath.mnt->mnt_sb->s_flags & MS_RDONLY) {
>>+			pr_err("overlayfs: upper fs is r/o, try multi-lower layers
>>mount\n");
>>+			err = -EINVAL;
>>+			goto out_put_upperpath;
>>+		}
>>+
>> 		err = ovl_mount_dir(ufs->config.workdir, &workpath);
>> 		if (err)
>> 			goto out_put_upperpath;
>>@@ -939,8 +953,8 @@ static int ovl_fill_super(struct super_block *sb,
>>void *data, int silent)
>> 		ufs->numlower++;
>> 	}
>>
>>-	/* If the upper fs is r/o or nonexistent, we mark overlayfs r/o too
>>*/
>>-	if (!ufs->upper_mnt || (ufs->upper_mnt->mnt_sb->s_flags & MS_RDONLY))
>>+	/* If the upper fs is nonexistent, we mark overlayfs r/o too */
>>+	if (!ufs->upper_mnt)
>> 		sb->s_flags |= MS_RDONLY;
>>
>> 	sb->s_d_op = &ovl_dentry_operations;
>
> It works fine to me.
> And I think it is better than my implementation : )
>
> Thanks.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-unionfs"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 3/3] ovl: upper fs should not be R/O

[hujianyang]

Hi Alex,

I think your question is about why workdir was introduced. I'd like
to share my thought with you. I should say Miklos is the best candidate
to explain it, because he is the author of this filesystem.

On 2015/1/16 1:09, A. Wan wrote:
> Just one question.  If each r/o layer does not require a workdir, why
> would a stack of r/o layers require one - and hence the requirement that
> the top layer must be r/w?
> 

As far as I know, "workdir" is not belong to any r/o or r/w layer in
overlayfs. Actually it belong to a overlayfs mount itself. "Workdir" is
not mandatory after recently multi-lower layer feature. It is used only
in r/w case, and only valuable in r/w case. My patch wants to make sure
the specified "workdir" in option line is significative.

If you just mount a stack of r/o layers, you don't need any "workdir",
but the mounted overlayfs partition will be marked as r/o. If you want a
r/w mount, you must specify a upperdir, the r/w layer on the top and
specify a workdir in the same mount with upperdir.

Overlayfs is a union filesystem. It gives ability to combine directories
in different mounts together. For r/w mount case, overlayfs allow users
change any file in its mount. But the implement of overlayfs not directly
write to each lower fs. All the write is reflected on the upper layer,
the top r/w layer.

See this link about unionfs:

http://lwn.net/Articles/324291/

"Workdir" works as the temp directory of a overlayfs mount. The file
changing is first done in it and then use rename() move to upper directory.
That's why "workdir" must in the same mount with upperdir.

For example,

if users delete a file which belong to a lower a/o layer in overlayfs
partition, the deletion is not perform on lower fs. Instead, a whiteout
file is created in "workdir" and then move to upperdir.

Other operations, like "copy_up", "rename" also use "workdir" as the
temporary directory. You can read this code in fs/overlayfs yourself.

I think "workdir" is used to keep the consistency of overlayfs and avoid
corrupt data damaging the filesystem. I'd like Miklos could explain more
for us.

> Does it has to do with why workdir was introduced in the first place?

It's not the business of my patch, "workdir" is not introduced by it.
But after rescanning the document in kernel, I think more explanation
about "workdir" is worth to be done.

> Sorry but I couldn't find information about why workdir was introduced.  I
> suppose it was to support some functions that older versions can't.
> 

No, "workdir" is introduced since overlayfs first merged into mainline
and is still needed in r/w mount. But I wonder if there are other
implements instead. Oh, Miklos may be unhappy with my unorthodox notion.

I'm sorry to say I'm not good at English expression, but I'm happy to
clarify any points that are still unclear.

Thanks,
Hu

> Alex
> 
> On Thu, January 15, 2015 12:20 am, Seunghun Lee wrote:
>> On January 15, 2015 2:20:57 PM GMT+09:00, hujianyang
>> <hujianyang@huawei.com> wrote:
>>> After importing multi-lower layer support, users could mount a r/o
>>> partition as the left most lowerdir instead of using it as upperdir.
>>> And a r/o upperdir may cause an error like
>>>
>>> 	overlayfs: failed to create directory ./workdir/work
>>>
>>> during mount.

Re: [PATCH 3/3] ovl: upper fs should not be R/O

[Miklos Szeredi]

The three patches are queued at

  git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git overlayfs-next

Thanks,
Miklos

Re: [PATCH 1/3] ovl: print error message for invalid mount options

[hujianyang]

On 2015/1/15 13:17, hujianyang wrote:
> Overlayfs should print an error message if an incorrect mount option
> is caught like other filesystems.
> 
> After this patch, improper option input could be clearly known.
> 
> Reported-by: Fabian Sturm <fabian.sturm@aduu.de>
> Signed-off-by: hujianyang <hujianyang@huawei.com>
> ---
>  fs/overlayfs/super.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index b90952f..ab3c8cb 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -615,6 +615,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>  			break;
> 
>  		default:
> +			pr_err("overlayfs: unrecognized mount option \"%s\" or missing value\n", p);

Hi Miklos,

I'm sorry. The parameter "p" should begin with a new line.

I'd like to resend a new patch after you review these patches.

Thanks,
Hu

>  			return -EINVAL;
>  		}
>  	}
>