[PATCH] ovl: Check project quota ids during ovl_fill_super()

[PATCH] ovl: Check project quota ids during ovl_fill_super()

[Chengguang Xu]

When upperdir has project quota and has different project id with workdir,
write may fail with error message "Invalid cross-device link" although mounted on r/w mode.
This patch checks project quota information of upperdir/workdir during ovl_fill_super(),
and if different mount on r/o mode. It doesn’t check detail inherit flag because
the implementations are different in specific filesystems.

Signed-off-by: Chengguang Xu <cgxu@mykernel.net>
Signed-off-by: Suyue <suyue@meili-inc.com>
---
 fs/overlayfs/super.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index f5738e9..7b64d5c 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -276,10 +276,34 @@ static int ovl_statfs(struct dentry *dentry, struct kstatfs *buf)
 	return err;
 }
 
+static bool ovl_check_project_quota_ok(struct ovl_fs *ufs)
+{
+	struct super_block *sb;
+	kprojid_t upperdir_projid = KPROJIDT_INIT(-1);
+	kprojid_t workdir_projid = KPROJIDT_INIT(-1);
+
+	if (!ufs->upper_mnt || !ufs->workdir)
+		return true;
+
+	sb = ufs->upper_mnt->mnt_sb;
+	if (!(sb->s_quota_types & QTYPE_MASK_PRJ))
+		return true;
+
+	if (sb->dq_op && sb->dq_op->get_projid) {
+		sb->dq_op->get_projid(ufs->upper_mnt->mnt_root->d_inode, &upperdir_projid);
+		sb->dq_op->get_projid(ufs->workdir->d_inode, &workdir_projid);
+		if (projid_valid(upperdir_projid) && projid_valid(workdir_projid))
+			if (!projid_eq(upperdir_projid, workdir_projid))
+				return false;
+	}
+
+	return true;
+}
+
 /* Will this overlay be forced to mount/remount ro? */
 static bool ovl_force_readonly(struct ovl_fs *ufs)
 {
-	return (!ufs->upper_mnt || !ufs->workdir);
+	return (!ufs->upper_mnt || !ufs->workdir || !ovl_check_project_quota_ok(ufs));
 }
 
 /**
@@ -1060,6 +1084,12 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	else if (ufs->upper_mnt->mnt_sb != ufs->same_sb)
 		ufs->same_sb = NULL;
 
+	/* if upperdir and workdir have different project quota ids, we mark overlayfs r/o too */
+	if (!ovl_check_project_quota_ok(ufs)) {
+		pr_warn("overlayfs: mounting read-only because upperdir and workdir have different project quota ids.\n");
+		sb->s_flags |= MS_RDONLY;
+	}
+
 	if (!(ovl_force_readonly(ufs)) && ufs->config.index) {
 		/* Verify lower root is upper root origin */
 		err = ovl_verify_origin(upperpath.dentry, ufs->lower_mnt[0],
-- 
1.8.3.1

Re: [PATCH] ovl: Check project quota ids during ovl_fill_super()

[cgxu]

Hi Miklos

Any suggestion for the patch?


Best Regards,
Chengguang



> 在 2017年11月13日，下午11:59，Chengguang Xu <cgxu@mykernel.net> 写道：
> 
> When upperdir has project quota and has different project id with workdir,
> write may fail with error message "Invalid cross-device link" although mounted on r/w mode.
> This patch checks project quota information of upperdir/workdir during ovl_fill_super(),
> and if different mount on r/o mode. It doesn’t check detail inherit flag because
> the implementations are different in specific filesystems.
> 
> Signed-off-by: Chengguang Xu <cgxu@mykernel.net>
> Signed-off-by: Suyue <suyue@meili-inc.com>
> ---
> fs/overlayfs/super.c | 32 +++++++++++++++++++++++++++++++-
> 1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index f5738e9..7b64d5c 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -276,10 +276,34 @@ static int ovl_statfs(struct dentry *dentry, struct kstatfs *buf)
> 	return err;
> }
> 
> +static bool ovl_check_project_quota_ok(struct ovl_fs *ufs)
> +{
> +	struct super_block *sb;
> +	kprojid_t upperdir_projid = KPROJIDT_INIT(-1);
> +	kprojid_t workdir_projid = KPROJIDT_INIT(-1);
> +
> +	if (!ufs->upper_mnt || !ufs->workdir)
> +		return true;
> +
> +	sb = ufs->upper_mnt->mnt_sb;
> +	if (!(sb->s_quota_types & QTYPE_MASK_PRJ))
> +		return true;
> +
> +	if (sb->dq_op && sb->dq_op->get_projid) {
> +		sb->dq_op->get_projid(ufs->upper_mnt->mnt_root->d_inode, &upperdir_projid);
> +		sb->dq_op->get_projid(ufs->workdir->d_inode, &workdir_projid);
> +		if (projid_valid(upperdir_projid) && projid_valid(workdir_projid))
> +			if (!projid_eq(upperdir_projid, workdir_projid))
> +				return false;
> +	}
> +
> +	return true;
> +}
> +
> /* Will this overlay be forced to mount/remount ro? */
> static bool ovl_force_readonly(struct ovl_fs *ufs)
> {
> -	return (!ufs->upper_mnt || !ufs->workdir);
> +	return (!ufs->upper_mnt || !ufs->workdir || !ovl_check_project_quota_ok(ufs));
> }
> 
> /**
> @@ -1060,6 +1084,12 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
> 	else if (ufs->upper_mnt->mnt_sb != ufs->same_sb)
> 		ufs->same_sb = NULL;
> 
> +	/* if upperdir and workdir have different project quota ids, we mark overlayfs r/o too */
> +	if (!ovl_check_project_quota_ok(ufs)) {
> +		pr_warn("overlayfs: mounting read-only because upperdir and workdir have different project quota ids.\n");
> +		sb->s_flags |= MS_RDONLY;
> +	}
> +
> 	if (!(ovl_force_readonly(ufs)) && ufs->config.index) {
> 		/* Verify lower root is upper root origin */
> 		err = ovl_verify_origin(upperpath.dentry, ufs->lower_mnt[0],
> -- 
> 1.8.3.1
> 
> 

Re: [PATCH] ovl: Check project quota ids during ovl_fill_super()

[Amir Goldstein]

On Mon, Nov 27, 2017 at 5:36 PM, cgxu <cgxu@mykernel.net> wrote:
> Hi Miklos
>
> Any suggestion for the patch?
>
>
> Best Regards,
> Chengguang
>
>
>
>> 在 2017年11月13日，下午11:59，Chengguang Xu <cgxu@mykernel.net> 写道：
>>
>> When upperdir has project quota and has different project id with workdir,
>> write may fail with error message "Invalid cross-device link" although mounted on r/w mode.
>> This patch checks project quota information of upperdir/workdir during ovl_fill_super(),
>> and if different mount on r/o mode. It doesn’t check detail inherit flag because
>> the implementations are different in specific filesystems.
>>

Since overlayfs removes and re-creates 'work' directory inside workdir,
why not set projid of 'work' directory according to upper projid instead of
failing the mount?
If projid cannot be set, 'work' dir creation will fail and overlayfs will
fall back to ro mount anyway.

Amir.

Re: [PATCH] ovl: Check project quota ids during ovl_fill_super()

[Amir Goldstein]

On Mon, Nov 27, 2017 at 7:58 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Mon, Nov 27, 2017 at 5:36 PM, cgxu <cgxu@mykernel.net> wrote:
>> Hi Miklos
>>
>> Any suggestion for the patch?
>>
>>
>> Best Regards,
>> Chengguang
>>
>>
>>
>>> 在 2017年11月13日，下午11:59，Chengguang Xu <cgxu@mykernel.net> 写道：
>>>
>>> When upperdir has project quota and has different project id with workdir,
>>> write may fail with error message "Invalid cross-device link" although mounted on r/w mode.
>>> This patch checks project quota information of upperdir/workdir during ovl_fill_super(),
>>> and if different mount on r/o mode. It doesn’t check detail inherit flag because
>>> the implementations are different in specific filesystems.
>>>
>
> Since overlayfs removes and re-creates 'work' directory inside workdir,
> why not set projid of 'work' directory according to upper projid instead of
> failing the mount?
> If projid cannot be set, 'work' dir creation will fail and overlayfs will
> fall back to ro mount anyway.
>

How about, instead of comparing project id, check that an O_TMPFILE
created in upperdir can be moved to workdir?
This tests exactly what overlayfs needs and not properties that lead to
what overlayfs needs.
Same as the check for setxattr and ovl_check_d_type_supported()
in ovl_make_workdir().

The only culprit is that this test won't work with fs that doesn't support
O_TMPFILE, but I don't think there are fs that support projid and not
O_TMPFILE.

Amir.

Re: [PATCH] ovl: Check project quota ids during ovl_fill_super()

[Chengguang Xu]

> 在 2017年12月12日，下午7:12，Amir Goldstein <amir73il@gmail.com> 写道：
> 
> On Mon, Nov 27, 2017 at 7:58 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Mon, Nov 27, 2017 at 5:36 PM, cgxu <cgxu@mykernel.net> wrote:
>>> Hi Miklos
>>> 
>>> Any suggestion for the patch?
>>> 
>>> 
>>> Best Regards,
>>> Chengguang
>>> 
>>> 
>>> 
>>>> 在 2017年11月13日，下午11:59，Chengguang Xu <cgxu@mykernel.net> 写道：
>>>> 
>>>> When upperdir has project quota and has different project id with workdir,
>>>> write may fail with error message "Invalid cross-device link" although mounted on r/w mode.
>>>> This patch checks project quota information of upperdir/workdir during ovl_fill_super(),
>>>> and if different mount on r/o mode. It doesn’t check detail inherit flag because
>>>> the implementations are different in specific filesystems.
>>>> 
>> 
>> Since overlayfs removes and re-creates 'work' directory inside workdir,
>> why not set projid of 'work' directory according to upper projid instead of
>> failing the mount?
>> If projid cannot be set, 'work' dir creation will fail and overlayfs will
>> fall back to ro mount anyway.
>> 
> 
> How about, instead of comparing project id, check that an O_TMPFILE
> created in upperdir can be moved to workdir?
> This tests exactly what overlayfs needs and not properties that lead to
> what overlayfs needs.
> Same as the check for setxattr and ovl_check_d_type_supported()
> in ovl_make_workdir().

Your suggestion is good as a method of warning. 
But what overlays does is moving files from workdir to upperdir rather than 
doing the opposite. Additionally printing out the error message of moving files 
from upperdir to workdir would probably get users confused.


Thanks,
Chengguang.

Re: [PATCH] ovl: Check project quota ids during ovl_fill_super()

[Amir Goldstein]

On Wed, Dec 13, 2017 at 1:22 PM, Chengguang Xu <cgxu519@icloud.com> wrote:
>
>> 在 2017年12月12日，下午7:12，Amir Goldstein <amir73il@gmail.com> 写道：
>>
>> On Mon, Nov 27, 2017 at 7:58 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>> On Mon, Nov 27, 2017 at 5:36 PM, cgxu <cgxu@mykernel.net> wrote:
>>>> Hi Miklos
>>>>
>>>> Any suggestion for the patch?
>>>>
>>>>
>>>> Best Regards,
>>>> Chengguang
>>>>
>>>>
>>>>
>>>>> 在 2017年11月13日，下午11:59，Chengguang Xu <cgxu@mykernel.net> 写道：
>>>>>
>>>>> When upperdir has project quota and has different project id with workdir,
>>>>> write may fail with error message "Invalid cross-device link" although mounted on r/w mode.
>>>>> This patch checks project quota information of upperdir/workdir during ovl_fill_super(),
>>>>> and if different mount on r/o mode. It doesn’t check detail inherit flag because
>>>>> the implementations are different in specific filesystems.
>>>>>
>>>
>>> Since overlayfs removes and re-creates 'work' directory inside workdir,
>>> why not set projid of 'work' directory according to upper projid instead of
>>> failing the mount?
>>> If projid cannot be set, 'work' dir creation will fail and overlayfs will
>>> fall back to ro mount anyway.
>>>
>>
>> How about, instead of comparing project id, check that an O_TMPFILE
>> created in upperdir can be moved to workdir?
>> This tests exactly what overlayfs needs and not properties that lead to
>> what overlayfs needs.
>> Same as the check for setxattr and ovl_check_d_type_supported()
>> in ovl_make_workdir().
>
> Your suggestion is good as a method of warning.
> But what overlays does is moving files from workdir to upperdir rather than
> doing the opposite. Additionally printing out the error message of moving files
> from upperdir to workdir would probably get users confused.
>
>

I am assuming the ability to rename from upper to work is symetric
and tightly coupled with the ability to link a file between upper and work,
as is the case with upper/work having different projid.

The error to user should be something along the lines of "cannot move
files between workdir and upperdir".
I miswrote in my previous email about moving a tempfile.
What I meant to write was, in ovl_make_workdir():

// open an O_TMPFILE wirh upper root dir projid:
uppertemp = ovl_do_tmpfile(ofs->upper_mnt, S_IFREG | 0);
// try to link tmpfile to workdir (so if leftovers are not cleaned
they are not in upperdir):
worktemp = ovl_lookup_temp(ofs->workdir);
err = ovl_do_link(uppertemp, ofs->workdir, worktemp, true);
ovl_cleanup(ofs->workdir, worktemp);

Amir.