[PATCH v3] ovl: Check link ability between upperdir and workdir

[PATCH v3] ovl: Check link ability between upperdir and workdir

[Chengguang Xu]

Inspired by encountering unexpected write error when
upperdir and workdir having different project ids.
So if upper filesystem supports O_TMPFILE, try to check
link ability between upperdir and workdir and print a
warning message if check fails.
The failure of check does not directly lead to read-only
mounting because in some use cases may only write to
upperdir and do not modify anything on lowerdirs.

Signed-off-by: Chengguang Xu <cgxu519@icloud.com>
---

Changes since v2:
- Add comment about motivation.
- Modify warning message.

Changes since v1:
- Check link ablity between upperdir and workdir instead of checking
project ids of upperdir and workdir.
- Print a warning message instead of falling to readonly mode.

 fs/overlayfs/super.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 76440fe..7d6bd82 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -929,7 +929,7 @@ static int ovl_get_upper(struct ovl_fs *ofs, struct path *upperpath)
 
 static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath)
 {
-	struct dentry *temp;
+	struct dentry *uppertemp, *worktemp;
 	int err;
 
 	ofs->workdir = ovl_workdir_create(ofs, OVL_WORKDIR_NAME, false);
@@ -953,12 +953,28 @@ static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath)
 	if (!err)
 		pr_warn("overlayfs: upper fs needs to support d_type.\n");
 
-	/* Check if upper/work fs supports O_TMPFILE */
-	temp = ovl_do_tmpfile(ofs->workdir, S_IFREG | 0);
-	ofs->tmpfile = !IS_ERR(temp);
-	if (ofs->tmpfile)
-		dput(temp);
-	else
+	/*
+	 * Check if upper/work fs supports O_TMPFILE and if support, check
+	 * link ability between upperdir and workdir, this is inspired by
+	 * encountering unexpected write error when upperdir and workdir
+	 * having different project ids.
+	 */
+	uppertemp = ovl_do_tmpfile(ofs->upper_mnt->mnt_root, S_IFREG | 0);
+	ofs->tmpfile = !IS_ERR(uppertemp);
+	if (ofs->tmpfile) {
+		worktemp = ovl_lookup_temp(ofs->workdir);
+		if (!IS_ERR(worktemp)) {
+			inode_lock_nested(ofs->workdir->d_inode, I_MUTEX_PARENT);
+			err = ovl_do_link(uppertemp, ofs->workdir->d_inode, worktemp, true);
+			inode_unlock(ofs->workdir->d_inode);
+			if (err)
+				pr_warn("overlayfs: cannot link files between upperdir and workdir, it may cause write error.\n");
+			else
+				ovl_cleanup(ofs->workdir->d_inode, worktemp);
+			dput(worktemp);
+		}
+		dput(uppertemp);
+	} else
 		pr_warn("overlayfs: upper fs does not support tmpfile.\n");
 
 	/*
-- 
1.8.3.1

Re: [PATCH v3] ovl: Check link ability between upperdir and workdir

[Amir Goldstein]

On Mon, Dec 18, 2017 at 2:55 PM, Chengguang Xu <cgxu519@icloud.com> wrote:
> Inspired by encountering unexpected write error when
> upperdir and workdir having different project ids.
> So if upper filesystem supports O_TMPFILE, try to check
> link ability between upperdir and workdir and print a
> warning message if check fails.
> The failure of check does not directly lead to read-only
> mounting because in some use cases may only write to
> upperdir and do not modify anything on lowerdirs.
>
> Signed-off-by: Chengguang Xu <cgxu519@icloud.com>
> ---

Looks good.

Amir.

>
> Changes since v2:
> - Add comment about motivation.
> - Modify warning message.
>
> Changes since v1:
> - Check link ablity between upperdir and workdir instead of checking
> project ids of upperdir and workdir.
> - Print a warning message instead of falling to readonly mode.
>
>  fs/overlayfs/super.c | 30 +++++++++++++++++++++++-------
>  1 file changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 76440fe..7d6bd82 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -929,7 +929,7 @@ static int ovl_get_upper(struct ovl_fs *ofs, struct path *upperpath)
>
>  static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath)
>  {
> -       struct dentry *temp;
> +       struct dentry *uppertemp, *worktemp;
>         int err;
>
>         ofs->workdir = ovl_workdir_create(ofs, OVL_WORKDIR_NAME, false);
> @@ -953,12 +953,28 @@ static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath)
>         if (!err)
>                 pr_warn("overlayfs: upper fs needs to support d_type.\n");
>
> -       /* Check if upper/work fs supports O_TMPFILE */
> -       temp = ovl_do_tmpfile(ofs->workdir, S_IFREG | 0);
> -       ofs->tmpfile = !IS_ERR(temp);
> -       if (ofs->tmpfile)
> -               dput(temp);
> -       else
> +       /*
> +        * Check if upper/work fs supports O_TMPFILE and if support, check
> +        * link ability between upperdir and workdir, this is inspired by
> +        * encountering unexpected write error when upperdir and workdir
> +        * having different project ids.
> +        */
> +       uppertemp = ovl_do_tmpfile(ofs->upper_mnt->mnt_root, S_IFREG | 0);
> +       ofs->tmpfile = !IS_ERR(uppertemp);
> +       if (ofs->tmpfile) {
> +               worktemp = ovl_lookup_temp(ofs->workdir);
> +               if (!IS_ERR(worktemp)) {
> +                       inode_lock_nested(ofs->workdir->d_inode, I_MUTEX_PARENT);
> +                       err = ovl_do_link(uppertemp, ofs->workdir->d_inode, worktemp, true);
> +                       inode_unlock(ofs->workdir->d_inode);
> +                       if (err)
> +                               pr_warn("overlayfs: cannot link files between upperdir and workdir, it may cause write error.\n");
> +                       else
> +                               ovl_cleanup(ofs->workdir->d_inode, worktemp);
> +                       dput(worktemp);
> +               }
> +               dput(uppertemp);
> +       } else
>                 pr_warn("overlayfs: upper fs does not support tmpfile.\n");
>
>         /*
> --
> 1.8.3.1
>

Re: [PATCH v3] ovl: Check link ability between upperdir and workdir

[Vivek Goyal]

On Mon, Dec 18, 2017 at 08:55:48PM +0800, Chengguang Xu wrote:
> Inspired by encountering unexpected write error when
> upperdir and workdir having different project ids.

Can you please make this problem descrition little better. By
these two lines I really don't understand what's the problem
you are trying to solve. All I understood was that upperdir
and workdir had different project id. But not sure what
problem it led to and why.

In fact your first patch changelong was little better. It
atleast said that you encounter "-EXDEV". So is it rename
which fails. So rename is not allowed between two directories
having different project ids?

Vivek

> So if upper filesystem supports O_TMPFILE, try to check
> link ability between upperdir and workdir and print a
> warning message if check fails.
> The failure of check does not directly lead to read-only
> mounting because in some use cases may only write to
> upperdir and do not modify anything on lowerdirs.
> 
> Signed-off-by: Chengguang Xu <cgxu519@icloud.com>
> ---
> 
> Changes since v2:
> - Add comment about motivation.
> - Modify warning message.
> 
> Changes since v1:
> - Check link ablity between upperdir and workdir instead of checking
> project ids of upperdir and workdir.
> - Print a warning message instead of falling to readonly mode.
> 
>  fs/overlayfs/super.c | 30 +++++++++++++++++++++++-------
>  1 file changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 76440fe..7d6bd82 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -929,7 +929,7 @@ static int ovl_get_upper(struct ovl_fs *ofs, struct path *upperpath)
>  
>  static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath)
>  {
> -	struct dentry *temp;
> +	struct dentry *uppertemp, *worktemp;
>  	int err;
>  
>  	ofs->workdir = ovl_workdir_create(ofs, OVL_WORKDIR_NAME, false);
> @@ -953,12 +953,28 @@ static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath)
>  	if (!err)
>  		pr_warn("overlayfs: upper fs needs to support d_type.\n");
>  
> -	/* Check if upper/work fs supports O_TMPFILE */
> -	temp = ovl_do_tmpfile(ofs->workdir, S_IFREG | 0);
> -	ofs->tmpfile = !IS_ERR(temp);
> -	if (ofs->tmpfile)
> -		dput(temp);
> -	else
> +	/*
> +	 * Check if upper/work fs supports O_TMPFILE and if support, check
> +	 * link ability between upperdir and workdir, this is inspired by
> +	 * encountering unexpected write error when upperdir and workdir
> +	 * having different project ids.
> +	 */
> +	uppertemp = ovl_do_tmpfile(ofs->upper_mnt->mnt_root, S_IFREG | 0);
> +	ofs->tmpfile = !IS_ERR(uppertemp);
> +	if (ofs->tmpfile) {
> +		worktemp = ovl_lookup_temp(ofs->workdir);
> +		if (!IS_ERR(worktemp)) {
> +			inode_lock_nested(ofs->workdir->d_inode, I_MUTEX_PARENT);
> +			err = ovl_do_link(uppertemp, ofs->workdir->d_inode, worktemp, true);
> +			inode_unlock(ofs->workdir->d_inode);
> +			if (err)
> +				pr_warn("overlayfs: cannot link files between upperdir and workdir, it may cause write error.\n");
> +			else
> +				ovl_cleanup(ofs->workdir->d_inode, worktemp);
> +			dput(worktemp);
> +		}
> +		dput(uppertemp);
> +	} else
>  		pr_warn("overlayfs: upper fs does not support tmpfile.\n");
>  
>  	/*
> -- 
> 1.8.3.1
> 
> --
> 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 v3] ovl: Check link ability between upperdir and workdir

[Chengguang Xu]

> 
> 在 2017年12月19日，上午12:02，Vivek Goyal <vgoyal@redhat.com> 写道：
> 
> On Mon, Dec 18, 2017 at 08:55:48PM +0800, Chengguang Xu wrote:
>> Inspired by encountering unexpected write error when
>> upperdir and workdir having different project ids.
> 
> Can you please make this problem descrition little better. By
> these two lines I really don't understand what's the problem
> you are trying to solve. All I understood was that upperdir
> and workdir had different project id. But not sure what
> problem it led to and why.
> 
> In fact your first patch changelong was little better. It
> atleast said that you encounter "-EXDEV". So is it rename
> which fails. So rename is not allowed between two directories
> having different project ids?
> 

The background is
I encountered an unexpected write error with error code -EXDEV in
my environment which didn’t break any rules in overlayfs kernel
document.

So I did some investigations and found when upperdir and workdir
having different project quotas then rename/link operations between
those directories would be fail with -EXDEV because project quota
asks files in it strictly inherit project id with it’s own. This will
make write fail during copy-up process.

I wrote first patch to check this condition, but that check seems not
sufficient to detect write error, so after discussion with Amir,
we decided to check link ability between upperdir and workdir, it is
what copy-up actually doing when modifying files in lowerdirs, and also
I decided to only print a warning message instead of directly mounting
on read-only mode in case there is a special use case just read and do not 
modify anything in lowerdirs.



Thanks,
Chengguang.

Re: [PATCH v3] ovl: Check link ability between upperdir and workdir

[Vivek Goyal]

On Tue, Dec 19, 2017 at 09:49:28AM +0800, Chengguang Xu wrote:
> > 
> > 在 2017年12月19日，上午12:02，Vivek Goyal <vgoyal@redhat.com> 写道：
> > 
> > On Mon, Dec 18, 2017 at 08:55:48PM +0800, Chengguang Xu wrote:
> >> Inspired by encountering unexpected write error when
> >> upperdir and workdir having different project ids.
> > 
> > Can you please make this problem descrition little better. By
> > these two lines I really don't understand what's the problem
> > you are trying to solve. All I understood was that upperdir
> > and workdir had different project id. But not sure what
> > problem it led to and why.
> > 
> > In fact your first patch changelong was little better. It
> > atleast said that you encounter "-EXDEV". So is it rename
> > which fails. So rename is not allowed between two directories
> > having different project ids?
> > 
> 
> The background is
> I encountered an unexpected write error with error code -EXDEV in
> my environment which didn’t break any rules in overlayfs kernel
> document.
> 
> So I did some investigations and found when upperdir and workdir
> having different project quotas then rename/link operations between
> those directories would be fail with -EXDEV because project quota
> asks files in it strictly inherit project id with it’s own. This will
> make write fail during copy-up process.
> 
> I wrote first patch to check this condition, but that check seems not
> sufficient to detect write error, so after discussion with Amir,
> we decided to check link ability between upperdir and workdir, it is
> what copy-up actually doing when modifying files in lowerdirs, and also
> I decided to only print a warning message instead of directly mounting
> on read-only mode in case there is a special use case just read and do not 
> modify anything in lowerdirs.

Ok, thanks for the explanation. I really wish that some of it makes to
changelog so that somebody reading it later finds it much easier to
understand.

BTW, just curious, when upper supports O_TMPFILE, do we have to create
tmpfile in workdir/. Can we create it in upper/ and then link in
appropriate destination directory.

Amir, is there a fundamental requirement about why tmpfile creation has
to be in workdir.

Vivek

Re: [PATCH v3] ovl: Check link ability between upperdir and workdir

[Amir Goldstein]

On Tue, Dec 19, 2017 at 3:48 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Tue, Dec 19, 2017 at 09:49:28AM +0800, Chengguang Xu wrote:
>> >
>> > 在 2017年12月19日，上午12:02，Vivek Goyal <vgoyal@redhat.com> 写道：
>> >
>> > On Mon, Dec 18, 2017 at 08:55:48PM +0800, Chengguang Xu wrote:
>> >> Inspired by encountering unexpected write error when
>> >> upperdir and workdir having different project ids.
>> >
>> > Can you please make this problem descrition little better. By
>> > these two lines I really don't understand what's the problem
>> > you are trying to solve. All I understood was that upperdir
>> > and workdir had different project id. But not sure what
>> > problem it led to and why.
>> >
>> > In fact your first patch changelong was little better. It
>> > atleast said that you encounter "-EXDEV". So is it rename
>> > which fails. So rename is not allowed between two directories
>> > having different project ids?
>> >
>>
>> The background is
>> I encountered an unexpected write error with error code -EXDEV in
>> my environment which didn’t break any rules in overlayfs kernel
>> document.
>>
>> So I did some investigations and found when upperdir and workdir
>> having different project quotas then rename/link operations between
>> those directories would be fail with -EXDEV because project quota
>> asks files in it strictly inherit project id with it’s own. This will
>> make write fail during copy-up process.
>>
>> I wrote first patch to check this condition, but that check seems not
>> sufficient to detect write error, so after discussion with Amir,
>> we decided to check link ability between upperdir and workdir, it is
>> what copy-up actually doing when modifying files in lowerdirs, and also
>> I decided to only print a warning message instead of directly mounting
>> on read-only mode in case there is a special use case just read and do not
>> modify anything in lowerdirs.
>
> Ok, thanks for the explanation. I really wish that some of it makes to
> changelog so that somebody reading it later finds it much easier to
> understand.
>
> BTW, just curious, when upper supports O_TMPFILE, do we have to create
> tmpfile in workdir/. Can we create it in upper/ and then link in
> appropriate destination directory.
>

Do you mean like this:
d8514d8edb5b ovl: copy up regular file using O_TMPFILE

> Amir, is there a fundamental requirement about why tmpfile creation has
> to be in workdir.
>

It isn't. With indexed copy up, tmpfile is created in indexdir and linked to
index dir, then linked again to upper dir.
With non-indexed copy up (of regular file, when O_TMPFILE supported)
tmpfile is created in upperdir and linked to upper dir, which allows for
concurrent copy up of two files within different parent dirs.

Amir.

Re: [PATCH v3] ovl: Check link ability between upperdir and workdir

[Vivek Goyal]

On Tue, Dec 19, 2017 at 04:11:13PM +0200, Amir Goldstein wrote:
[..]
> With non-indexed copy up (of regular file, when O_TMPFILE supported)
> tmpfile is created in upperdir and linked to upper dir, which allows for
> concurrent copy up of two files within different parent dirs.

Is it correct. I am looking at the code and we always seem to get tmpfile
in work/work.

ovl_copy_up_locked()
  ovl_get_tmpfile()
      ovl_do_tmpfile(c->workdir, c->stat.mode)

I was hoping that for a regular file copy up, we could create tmpfile in
upper/ so that we don't have the requirement of allowing linking between
work/ and upper/ as long as upper supported tmpfile. But given that index 
will require linking with alias in upper, we can't get rid of this
requirement completely.

Vivek

Re: [PATCH v3] ovl: Check link ability between upperdir and workdir

[Amir Goldstein]

On Tue, Dec 19, 2017 at 5:42 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Tue, Dec 19, 2017 at 04:11:13PM +0200, Amir Goldstein wrote:
> [..]
>> With non-indexed copy up (of regular file, when O_TMPFILE supported)
>> tmpfile is created in upperdir and linked to upper dir, which allows for
>> concurrent copy up of two files within different parent dirs.
>
> Is it correct. I am looking at the code and we always seem to get tmpfile
> in work/work.
>
> ovl_copy_up_locked()
>   ovl_get_tmpfile()
>       ovl_do_tmpfile(c->workdir, c->stat.mode)
>
> I was hoping that for a regular file copy up, we could create tmpfile in
> upper/ so that we don't have the requirement of allowing linking between
> work/ and upper/ as long as upper supported tmpfile. But given that index
> will require linking with alias in upper, we can't get rid of this
> requirement completely.
>

Ah, I see. I guess there is just no strong reason to change that, because
still have the rename requirement for non-regular file.
The test I suggested to Chengguang is just a replacement to
rename test under the assumption that if rename would fail with -EXDEV,
link of tempfile will also fail with -EXDEV.

Amir.