From: Wang Shilong <[email protected]>
Currently, project quota could be changed by fssetxattr
ioctl, and existed permission check inode_owner_or_capable()
is obviously not enough, just think that common users could
change project id of file, that could make users to
break project quota easily.
This patch try to follow same regular of xfs project
quota:
"Project Quota ID state is only allowed to change from
within the init namespace. Enforce that restriction only
if we are trying to change the quota ID state.
Everything else is allowed in user namespaces."
Besides that, check and set project id'state should
be an atomic operation, protect whole operation with
inode lock.
Signed-off-by: Wang Shilong <[email protected]>
Reviewed-by: Andreas Dilger <[email protected]>
---
v2->v3: change function declaration to one line.
v1->v2: rename ext4_ioctl_setattr_check_projid()
to ext4_ioctl_check_project()
---
fs/ext4/ioctl.c | 60 ++++++++++++++++++++++++++++++-------------------
1 file changed, 37 insertions(+), 23 deletions(-)
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index a7074115d6f6..f81102bd3203 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -339,19 +339,14 @@ static int ext4_ioctl_setproject(struct file *filp, __u32 projid)
if (projid_eq(kprojid, EXT4_I(inode)->i_projid))
return 0;
- err = mnt_want_write_file(filp);
- if (err)
- return err;
-
err = -EPERM;
- inode_lock(inode);
/* Is it quota file? Do not allow user to mess with it */
if (ext4_is_quota_file(inode))
- goto out_unlock;
+ return err;
err = ext4_get_inode_loc(inode, &iloc);
if (err)
- goto out_unlock;
+ return err;
raw_inode = ext4_raw_inode(&iloc);
if (!EXT4_FITS_IN_INODE(raw_inode, ei, i_projid)) {
@@ -359,7 +354,7 @@ static int ext4_ioctl_setproject(struct file *filp, __u32 projid)
EXT4_SB(sb)->s_want_extra_isize,
&iloc);
if (err)
- goto out_unlock;
+ return err;
} else {
brelse(iloc.bh);
}
@@ -369,10 +364,8 @@ static int ext4_ioctl_setproject(struct file *filp, __u32 projid)
handle = ext4_journal_start(inode, EXT4_HT_QUOTA,
EXT4_QUOTA_INIT_BLOCKS(sb) +
EXT4_QUOTA_DEL_BLOCKS(sb) + 3);
- if (IS_ERR(handle)) {
- err = PTR_ERR(handle);
- goto out_unlock;
- }
+ if (IS_ERR(handle))
+ return PTR_ERR(handle);
err = ext4_reserve_inode_write(handle, inode, &iloc);
if (err)
@@ -400,9 +393,6 @@ static int ext4_ioctl_setproject(struct file *filp, __u32 projid)
err = rc;
out_stop:
ext4_journal_stop(handle);
-out_unlock:
- inode_unlock(inode);
- mnt_drop_write_file(filp);
return err;
}
#else
@@ -626,6 +616,30 @@ static long ext4_ioctl_group_add(struct file *file,
return err;
}
+static int ext4_ioctl_check_project(struct inode *inode, struct fsxattr *fa)
+{
+ /*
+ * Project Quota ID state is only allowed to change from within the init
+ * namespace. Enforce that restriction only if we are trying to change
+ * the quota ID state. Everything else is allowed in user namespaces.
+ */
+ if (current_user_ns() == &init_user_ns)
+ return 0;
+
+ if (__kprojid_val(EXT4_I(inode)->i_projid) != fa->fsx_projid)
+ return -EINVAL;
+
+ if (ext4_test_inode_flag(inode, EXT4_INODE_PROJINHERIT)) {
+ if (!(fa->fsx_xflags & FS_XFLAG_PROJINHERIT))
+ return -EINVAL;
+ } else {
+ if (fa->fsx_xflags & FS_XFLAG_PROJINHERIT)
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
{
struct inode *inode = file_inode(filp);
@@ -1025,19 +1039,19 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
return err;
inode_lock(inode);
+ err = ext4_ioctl_check_project(inode, &fa);
+ if (err)
+ goto out;
flags = (ei->i_flags & ~EXT4_FL_XFLAG_VISIBLE) |
(flags & EXT4_FL_XFLAG_VISIBLE);
err = ext4_ioctl_setflags(inode, flags);
- inode_unlock(inode);
- mnt_drop_write_file(filp);
if (err)
- return err;
-
+ goto out;
err = ext4_ioctl_setproject(filp, fa.fsx_projid);
- if (err)
- return err;
-
- return 0;
+out:
+ inode_unlock(inode);
+ mnt_drop_write_file(filp);
+ return err;
}
case EXT4_IOC_SHUTDOWN:
return ext4_shutdown(sb, arg);
--
2.17.1
From: Wang Shilong <[email protected]>
We return most failure of dquota_initialize() except
inode evict, this could make a bit sense, for example
we allow file removal even quota files are broken?
But it dosen't make sense to allow setting project
if quota files etc are broken.
Signed-off-by: Wang Shilong <[email protected]>
---
v1->v2:
based it on one patchset, to make reviewers happy.
---
fs/ext4/ioctl.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index f81102bd3203..781dd699bd7b 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -359,7 +359,9 @@ static int ext4_ioctl_setproject(struct file *filp, __u32 projid)
brelse(iloc.bh);
}
- dquot_initialize(inode);
+ err = dquot_initialize(inode);
+ if (err)
+ return err;
handle = ext4_journal_start(inode, EXT4_HT_QUOTA,
EXT4_QUOTA_INIT_BLOCKS(sb) +
--
2.17.1
From: Wang Shilong <[email protected]>
Currently, project quota could be changed by fssetxattr
ioctl, and existed permission check inode_owner_or_capable()
is obviously not enough, just think that common users could
change project id of file, that could make users to
break project quota easily.
This patch try to follow same regular of xfs project
quota:
"Project Quota ID state is only allowed to change from
within the init namespace. Enforce that restriction only
if we are trying to change the quota ID state.
Everything else is allowed in user namespaces."
Besides that, check and set project id'state should
be an atomic operation, protect whole operation with
inode lock.
Signed-off-by: Wang Shilong <[email protected]>
Reviewed-by: Chao Yu <[email protected]>
---
v3->v4: change function declaration to one line.
v2->v3: remove inode_lock() and mnt_want_write_file()
inside f2fs_ioc_setproject()
v1->v2: rename f2fs_ioctl_setattr_check_projid()
---
fs/f2fs/file.c | 60 +++++++++++++++++++++++++++++++-------------------
1 file changed, 37 insertions(+), 23 deletions(-)
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 5474aaa274b9..e7896c5da0c2 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2617,34 +2617,26 @@ static int f2fs_ioc_setproject(struct file *filp, __u32 projid)
if (projid_eq(kprojid, F2FS_I(inode)->i_projid))
return 0;
- err = mnt_want_write_file(filp);
- if (err)
- return err;
-
err = -EPERM;
- inode_lock(inode);
-
/* Is it quota file? Do not allow user to mess with it */
if (IS_NOQUOTA(inode))
- goto out_unlock;
+ return err;
ipage = f2fs_get_node_page(sbi, inode->i_ino);
- if (IS_ERR(ipage)) {
- err = PTR_ERR(ipage);
- goto out_unlock;
- }
+ if (IS_ERR(ipage))
+ return PTR_ERR(ipage);
if (!F2FS_FITS_IN_INODE(F2FS_INODE(ipage), fi->i_extra_isize,
i_projid)) {
err = -EOVERFLOW;
f2fs_put_page(ipage, 1);
- goto out_unlock;
+ return err;
}
f2fs_put_page(ipage, 1);
err = dquot_initialize(inode);
if (err)
- goto out_unlock;
+ return err;
transfer_to[PRJQUOTA] = dqget(sb, make_kqid_projid(kprojid));
if (!IS_ERR(transfer_to[PRJQUOTA])) {
@@ -2658,9 +2650,6 @@ static int f2fs_ioc_setproject(struct file *filp, __u32 projid)
inode->i_ctime = current_time(inode);
out_dirty:
f2fs_mark_inode_dirty_sync(inode, true);
-out_unlock:
- inode_unlock(inode);
- mnt_drop_write_file(filp);
return err;
}
#else
@@ -2736,6 +2725,30 @@ static int f2fs_ioc_fsgetxattr(struct file *filp, unsigned long arg)
return 0;
}
+static int f2fs_ioctl_check_project(struct inode *inode, struct fsxattr *fa)
+{
+ /*
+ * Project Quota ID state is only allowed to change from within the init
+ * namespace. Enforce that restriction only if we are trying to change
+ * the quota ID state. Everything else is allowed in user namespaces.
+ */
+ if (current_user_ns() == &init_user_ns)
+ return 0;
+
+ if (__kprojid_val(F2FS_I(inode)->i_projid) != fa->fsx_projid)
+ return -EINVAL;
+
+ if (F2FS_I(inode)->i_flags & F2FS_PROJINHERIT_FL) {
+ if (!(fa->fsx_xflags & FS_XFLAG_PROJINHERIT))
+ return -EINVAL;
+ } else {
+ if (fa->fsx_xflags & FS_XFLAG_PROJINHERIT)
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
static int f2fs_ioc_fssetxattr(struct file *filp, unsigned long arg)
{
struct inode *inode = file_inode(filp);
@@ -2763,19 +2776,20 @@ static int f2fs_ioc_fssetxattr(struct file *filp, unsigned long arg)
return err;
inode_lock(inode);
+ err = f2fs_ioctl_check_project(inode, &fa);
+ if (err)
+ goto out;
flags = (fi->i_flags & ~F2FS_FL_XFLAG_VISIBLE) |
(flags & F2FS_FL_XFLAG_VISIBLE);
err = __f2fs_ioc_setflags(inode, flags);
- inode_unlock(inode);
- mnt_drop_write_file(filp);
if (err)
- return err;
+ goto out;
err = f2fs_ioc_setproject(filp, fa.fsx_projid);
- if (err)
- return err;
-
- return 0;
+out:
+ inode_unlock(inode);
+ mnt_drop_write_file(filp);
+ return err;
}
int f2fs_pin_file_control(struct inode *inode, bool inc)
--
2.17.1
On Wed, Sep 12, 2018 at 08:57:16AM +0900, Wang Shilong wrote:
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index a7074115d6f6..f81102bd3203 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -339,19 +339,14 @@ static int ext4_ioctl_setproject(struct file *filp, __u32 projid)
> if (projid_eq(kprojid, EXT4_I(inode)->i_projid))
> return 0;
>
> - err = mnt_want_write_file(filp);
> - if (err)
> - return err;
> -
A huge part of this patch is dropping the calls to
mnt_want_write_file() and mnt_drop_write_file(). What's the
justification for doing this? The use of mnt_want_write_file() looks
necessary to me...
- Ted
> ?? 2018??9??16?գ?????11:55??Theodore Y. Ts'o <[email protected]> д????
>
> On Wed, Sep 12, 2018 at 08:57:16AM +0900, Wang Shilong wrote:
>> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
>> index a7074115d6f6..f81102bd3203 100644
>> --- a/fs/ext4/ioctl.c
>> +++ b/fs/ext4/ioctl.c
>> @@ -339,19 +339,14 @@ static int ext4_ioctl_setproject(struct file *filp, __u32 projid)
>> if (projid_eq(kprojid, EXT4_I(inode)->i_projid))
>> return 0;
>>
>> - err = mnt_want_write_file(filp);
>> - if (err)
>> - return err;
>> -
>
> A huge part of this patch is dropping the calls to
> mnt_want_write_file() and mnt_drop_write_file(). What's the
> justification for doing this? The use of mnt_want_write_file() looks
> necessary to me??
Hi Ted,
mnt_want_write_file() and inode_lock is held before this function called now.
Since both ioctl_set_flags and ext4_set_project() need call them.
>
> - Ted
_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
On Sun, Sep 16, 2018 at 04:02:52AM +0000, Wang Shilong wrote:
>
>
> > 在 2018年9月16日，上午11:55，Theodore Y. Ts'o <[email protected]> 写道：
> >
> > On Wed, Sep 12, 2018 at 08:57:16AM +0900, Wang Shilong wrote:
> >> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> >> index a7074115d6f6..f81102bd3203 100644
> >> --- a/fs/ext4/ioctl.c
> >> +++ b/fs/ext4/ioctl.c
> >> @@ -339,19 +339,14 @@ static int ext4_ioctl_setproject(struct file *filp, __u32 projid)
> >> if (projid_eq(kprojid, EXT4_I(inode)->i_projid))
> >> return 0;
> >>
> >> - err = mnt_want_write_file(filp);
> >> - if (err)
> >> - return err;
> >> -
> >
> > A huge part of this patch is dropping the calls to
> > mnt_want_write_file() and mnt_drop_write_file(). What's the
> > justification for doing this? The use of mnt_want_write_file() looks
> > necessary to me…
>
> Hi Ted,
>
> mnt_want_write_file() and inode_lock is held before this function called now.
> Since both ioctl_set_flags and ext4_set_project() need call them.
I don't see any place in this patch where mnt_want_write_file() is
being called. What am I missing? And if there is a dependent patch
to this first patch in the patch series, please include it in this
patch series.... or least point it out after the --- list.
Thanks,
- Ted
_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> 在 2018年9月16日，下午8:20，Theodore Y. Ts'o <[email protected]> 写道：
>
> On Sun, Sep 16, 2018 at 04:02:52AM +0000, Wang Shilong wrote:
>>
>>
>>> 在 2018年9月16日，上午11:55，Theodore Y. Ts'o <[email protected]> 写道：
>>>
>>> On Wed, Sep 12, 2018 at 08:57:16AM +0900, Wang Shilong wrote:
>>>> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
>>>> index a7074115d6f6..f81102bd3203 100644
>>>> --- a/fs/ext4/ioctl.c
>>>> +++ b/fs/ext4/ioctl.c
>>>> @@ -339,19 +339,14 @@ static int ext4_ioctl_setproject(struct file *filp, __u32 projid)
>>>> if (projid_eq(kprojid, EXT4_I(inode)->i_projid))
>>>> return 0;
>>>>
>>>> - err = mnt_want_write_file(filp);
>>>> - if (err)
>>>> - return err;
>>>> -
>>>
>>> A huge part of this patch is dropping the calls to
>>> mnt_want_write_file() and mnt_drop_write_file(). What's the
>>> justification for doing this? The use of mnt_want_write_file() looks
>>> necessary to me…
>>
>> Hi Ted,
>>
>> mnt_want_write_file() and inode_lock is held before this function called now.
>> Since both ioctl_set_flags and ext4_set_project() need call them.
>
> I don't see any place in this patch where mnt_want_write_file() is
> being called. What am I missing? And if there is a dependent patch
> to this first patch in the patch series, please include it in this
> patch series.... or least point it out after the --- list.
Sorry, I might not explain it clearly, here is current codes after applying patch:
039 err = mnt_want_write_file(filp); ———————>called here, this is originally there for ext4_ioctl_setflags()
1040 if (err)
1041 return err;
1042
1043 inode_lock(inode);
1044 err = ext4_ioctl_setattr_check_projid(inode, &fa);
1045 if (err)
1046 goto out;
1047 flags = (ei->i_flags & ~EXT4_FL_XFLAG_VISIBLE) |
1048 (flags & EXT4_FL_XFLAG_VISIBLE);
1049 err = ext4_ioctl_setflags(inode, flags);
1050 if (err)
1051 goto out;
1052 err = ext4_ioctl_setproject(filp, fa.fsx_projid);
1053 out:
1054 inode_unlock(inode);
1055 mnt_drop_write_file(filp); ————————>dropped here.
1056 return err;
1057 }
Thanks,
Shilong
>
> Thanks,
>
> - Ted
_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> 在 2018年9月16日，下午8:25，Wang Shilong <[email protected]> 写道：
>
>
>
>> 在 2018年9月16日，下午8:20，Theodore Y. Ts'o <[email protected]> 写道：
>>
>> On Sun, Sep 16, 2018 at 04:02:52AM +0000, Wang Shilong wrote:
>>>
>>>
>>>> 在 2018年9月16日，上午11:55，Theodore Y. Ts'o <[email protected]> 写道：
>>>>
>>>> On Wed, Sep 12, 2018 at 08:57:16AM +0900, Wang Shilong wrote:
>>>>> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
>>>>> index a7074115d6f6..f81102bd3203 100644
>>>>> --- a/fs/ext4/ioctl.c
>>>>> +++ b/fs/ext4/ioctl.c
>>>>> @@ -339,19 +339,14 @@ static int ext4_ioctl_setproject(struct file *filp, __u32 projid)
>>>>> if (projid_eq(kprojid, EXT4_I(inode)->i_projid))
>>>>> return 0;
>>>>>
>>>>> - err = mnt_want_write_file(filp);
>>>>> - if (err)
>>>>> - return err;
>>>>> -
>>>>
>>>> A huge part of this patch is dropping the calls to
>>>> mnt_want_write_file() and mnt_drop_write_file(). What's the
>>>> justification for doing this? The use of mnt_want_write_file() looks
>>>> necessary to me…
>>>
>>> Hi Ted,
>>>
>>> mnt_want_write_file() and inode_lock is held before this function called now.
>>> Since both ioctl_set_flags and ext4_set_project() need call them.
>>
>> I don't see any place in this patch where mnt_want_write_file() is
>> being called. What am I missing? And if there is a dependent patch
>> to this first patch in the patch series, please include it in this
>> patch series.... or least point it out after the --- list.
>
> Sorry, I might not explain it clearly, here is current codes after applying patch:
>
> 039 err = mnt_want_write_file(filp); ———————>called here, this is originally there for ext4_ioctl_setflags()
> 1040 if (err)
> 1041 return err;
> 1042
> 1043 inode_lock(inode);
> 1044 err = ext4_ioctl_setattr_check_projid(inode, &fa);
> 1045 if (err)
> 1046 goto out;
> 1047 flags = (ei->i_flags & ~EXT4_FL_XFLAG_VISIBLE) |
> 1048 (flags & EXT4_FL_XFLAG_VISIBLE);
> 1049 err = ext4_ioctl_setflags(inode, flags);
> 1050 if (err)
> 1051 goto out;
> 1052 err = ext4_ioctl_setproject(filp, fa.fsx_projid);
> 1053 out:
> 1054 inode_unlock(inode);
> 1055 mnt_drop_write_file(filp); ————————>dropped here.
> 1056 return err;
> 1057 }
>
> Thanks,
> Shilong
Just a general ping for this, make sure you did not miss reply.
Since I saw you sent new pull request.
Is there anything more I could do?
>
>>
>> Thanks,
>>
>> - Ted
>
_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel