* [f2fs-dev] [PATCH] f2fs: fix to set inode->i_mode correctly for posix_acl_update_mode
@ 2020-12-14 3:54 Weichao Guo
2020-12-16 9:16 ` Chao Yu
2020-12-22 8:24 ` Chao Yu
0 siblings, 2 replies; 12+ messages in thread
From: Weichao Guo @ 2020-12-14 3:54 UTC (permalink / raw)
To: jaegeuk, chao; +Cc: Bin Shu, fh, linux-f2fs-devel
We should update the ~S_IRWXUGO part of inode->i_mode in __setattr_copy,
because posix_acl_update_mode updates mode based on inode->i_mode,
which finally overwrites the ~S_IRWXUGO part of i_acl_mode with old i_mode.
Testcase to reproduce this bug:
0. adduser abc
1. mkfs.f2fs /dev/sdd
2. mount -t f2fs /dev/sdd /mnt/f2fs
3. mkdir /mnt/f2fs/test
4. setfacl -m u:abc:r /mnt/f2fs/test
5. chmod +s /mnt/f2fs/test
Signed-off-by: Weichao Guo <guoweichao@oppo.com>
Signed-off-by: Bin Shu <shubin@oppo.com>
---
fs/f2fs/file.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 16ea10f..4d355f9 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -850,6 +850,7 @@ static void __setattr_copy(struct inode *inode, const struct iattr *attr)
if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
mode &= ~S_ISGID;
+ inode->i_mode = (inode->i_mode & S_IRWXUGO) | (mode & ~S_IRWXUGO);
set_acl_inode(inode, mode);
}
}
--
2.7.4
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to set inode->i_mode correctly for posix_acl_update_mode
2020-12-14 3:54 [f2fs-dev] [PATCH] f2fs: fix to set inode->i_mode correctly for posix_acl_update_mode Weichao Guo
@ 2020-12-16 9:16 ` Chao Yu
2020-12-16 9:21 ` Chao Yu
2020-12-22 8:24 ` Chao Yu
1 sibling, 1 reply; 12+ messages in thread
From: Chao Yu @ 2020-12-16 9:16 UTC (permalink / raw)
To: Weichao Guo, jaegeuk, chao; +Cc: fh, Bin Shu, linux-f2fs-devel
On 2020/12/14 11:54, Weichao Guo wrote:
> We should update the ~S_IRWXUGO part of inode->i_mode in __setattr_copy,
> because posix_acl_update_mode updates mode based on inode->i_mode,
> which finally overwrites the ~S_IRWXUGO part of i_acl_mode with old i_mode.
>
> Testcase to reproduce this bug:
> 0. adduser abc
> 1. mkfs.f2fs /dev/sdd
> 2. mount -t f2fs /dev/sdd /mnt/f2fs
> 3. mkdir /mnt/f2fs/test
> 4. setfacl -m u:abc:r /mnt/f2fs/test
> 5. chmod +s /mnt/f2fs/test
Good catch!
>
> Signed-off-by: Weichao Guo <guoweichao@oppo.com>
> Signed-off-by: Bin Shu <shubin@oppo.com>
Reviewed-by: Chao Yu <yuchao0@huawei.com>
Thanks,
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to set inode->i_mode correctly for posix_acl_update_mode
2020-12-16 9:16 ` Chao Yu
@ 2020-12-16 9:21 ` Chao Yu
2020-12-17 6:24 ` Jaegeuk Kim
0 siblings, 1 reply; 12+ messages in thread
From: Chao Yu @ 2020-12-16 9:21 UTC (permalink / raw)
To: jaegeuk; +Cc: fh, linux-f2fs-devel, Bin Shu
Jaegeuk,
Do you remember why we use i_acl_mode to store acl related mode?
Can we get rid of it?
Thanks,
On 2020/12/16 17:16, Chao Yu wrote:
> On 2020/12/14 11:54, Weichao Guo wrote:
>> We should update the ~S_IRWXUGO part of inode->i_mode in __setattr_copy,
>> because posix_acl_update_mode updates mode based on inode->i_mode,
>> which finally overwrites the ~S_IRWXUGO part of i_acl_mode with old i_mode.
>>
>> Testcase to reproduce this bug:
>> 0. adduser abc
>> 1. mkfs.f2fs /dev/sdd
>> 2. mount -t f2fs /dev/sdd /mnt/f2fs
>> 3. mkdir /mnt/f2fs/test
>> 4. setfacl -m u:abc:r /mnt/f2fs/test
>> 5. chmod +s /mnt/f2fs/test
>
> Good catch!
>
>>
>> Signed-off-by: Weichao Guo <guoweichao@oppo.com>
>> Signed-off-by: Bin Shu <shubin@oppo.com>
>
> Reviewed-by: Chao Yu <yuchao0@huawei.com>
>
> Thanks,
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> .
>
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to set inode->i_mode correctly for posix_acl_update_mode
2020-12-16 9:21 ` Chao Yu
@ 2020-12-17 6:24 ` Jaegeuk Kim
0 siblings, 0 replies; 12+ messages in thread
From: Jaegeuk Kim @ 2020-12-17 6:24 UTC (permalink / raw)
To: Chao Yu; +Cc: fh, linux-f2fs-devel, Bin Shu
On 12/16, Chao Yu wrote:
> Jaegeuk,
>
> Do you remember why we use i_acl_mode to store acl related mode?
> Can we get rid of it?
IIRC, it is used for error handling, so it seems we can't remove it.
>
> Thanks,
>
> On 2020/12/16 17:16, Chao Yu wrote:
> > On 2020/12/14 11:54, Weichao Guo wrote:
> > > We should update the ~S_IRWXUGO part of inode->i_mode in __setattr_copy,
> > > because posix_acl_update_mode updates mode based on inode->i_mode,
> > > which finally overwrites the ~S_IRWXUGO part of i_acl_mode with old i_mode.
> > >
> > > Testcase to reproduce this bug:
> > > 0. adduser abc
> > > 1. mkfs.f2fs /dev/sdd
> > > 2. mount -t f2fs /dev/sdd /mnt/f2fs
> > > 3. mkdir /mnt/f2fs/test
> > > 4. setfacl -m u:abc:r /mnt/f2fs/test
> > > 5. chmod +s /mnt/f2fs/test
> >
> > Good catch!
> >
> > >
> > > Signed-off-by: Weichao Guo <guoweichao@oppo.com>
> > > Signed-off-by: Bin Shu <shubin@oppo.com>
> >
> > Reviewed-by: Chao Yu <yuchao0@huawei.com>
> >
> > Thanks,
> >
> >
> > _______________________________________________
> > Linux-f2fs-devel mailing list
> > Linux-f2fs-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> > .
> >
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to set inode->i_mode correctly for posix_acl_update_mode
2020-12-14 3:54 [f2fs-dev] [PATCH] f2fs: fix to set inode->i_mode correctly for posix_acl_update_mode Weichao Guo
2020-12-16 9:16 ` Chao Yu
@ 2020-12-22 8:24 ` Chao Yu
2020-12-22 9:32 ` Weichao Guo
1 sibling, 1 reply; 12+ messages in thread
From: Chao Yu @ 2020-12-22 8:24 UTC (permalink / raw)
To: Weichao Guo, jaegeuk, chao; +Cc: fh, Bin Shu, linux-f2fs-devel
On 2020/12/14 11:54, Weichao Guo wrote:
> We should update the ~S_IRWXUGO part of inode->i_mode in __setattr_copy,
> because posix_acl_update_mode updates mode based on inode->i_mode,
> which finally overwrites the ~S_IRWXUGO part of i_acl_mode with old i_mode.
>
> Testcase to reproduce this bug:
> 0. adduser abc
> 1. mkfs.f2fs /dev/sdd
> 2. mount -t f2fs /dev/sdd /mnt/f2fs
> 3. mkdir /mnt/f2fs/test
> 4. setfacl -m u:abc:r /mnt/f2fs/test
> 5. chmod +s /mnt/f2fs/test
>
> Signed-off-by: Weichao Guo <guoweichao@oppo.com>
> Signed-off-by: Bin Shu <shubin@oppo.com>
> ---
> fs/f2fs/file.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 16ea10f..4d355f9 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -850,6 +850,7 @@ static void __setattr_copy(struct inode *inode, const struct iattr *attr)
>
> if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
> mode &= ~S_ISGID;
> + inode->i_mode = (inode->i_mode & S_IRWXUGO) | (mode & ~S_IRWXUGO);
Sorry, I still have problem with this patch.
I think this equals to inode->i_mode = mode;
Because in chmod_common(), @mode was assigned as:
newattrs.ia_mode = (mode & S_IALLUGO) | (inode->i_mode & ~S_IALLUGO);
and only S_ISUID and S_ISGID bits of newattrs.ia_mode can be changed during chmod()
That's why setattr_copy() in fs/attr.c just uses "inode->i_mode = mode;"
> set_acl_inode(inode, mode);
Another problem is if i_acl_mode is used for error path handling, here i_acl_mode
and i_mode have the same value, that's not correct?
Jaegeuk,
IIUC, i_acl_mode was introduced for i_mode recovery once acl progress fails?
Thanks,
> }
> }
>
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to set inode->i_mode correctly for posix_acl_update_mode
2020-12-22 8:24 ` Chao Yu
@ 2020-12-22 9:32 ` Weichao Guo
2020-12-22 10:49 ` Chao Yu
0 siblings, 1 reply; 12+ messages in thread
From: Weichao Guo @ 2020-12-22 9:32 UTC (permalink / raw)
To: Chao Yu, jaegeuk, chao; +Cc: fh, Bin Shu, linux-f2fs-devel
On 2020/12/22 16:24, Chao Yu wrote:
> On 2020/12/14 11:54, Weichao Guo wrote:
>> We should update the ~S_IRWXUGO part of inode->i_mode in __setattr_copy,
>> because posix_acl_update_mode updates mode based on inode->i_mode,
>> which finally overwrites the ~S_IRWXUGO part of i_acl_mode with old
>> i_mode.
>>
>> Testcase to reproduce this bug:
>> 0. adduser abc
>> 1. mkfs.f2fs /dev/sdd
>> 2. mount -t f2fs /dev/sdd /mnt/f2fs
>> 3. mkdir /mnt/f2fs/test
>> 4. setfacl -m u:abc:r /mnt/f2fs/test
>> 5. chmod +s /mnt/f2fs/test
>>
>> Signed-off-by: Weichao Guo <guoweichao@oppo.com>
>> Signed-off-by: Bin Shu <shubin@oppo.com>
>> ---
>> fs/f2fs/file.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>> index 16ea10f..4d355f9 100644
>> --- a/fs/f2fs/file.c
>> +++ b/fs/f2fs/file.c
>> @@ -850,6 +850,7 @@ static void __setattr_copy(struct inode *inode,
>> const struct iattr *attr)
>> if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
>> mode &= ~S_ISGID;
>> + inode->i_mode = (inode->i_mode & S_IRWXUGO) | (mode &
>> ~S_IRWXUGO);
>
> Sorry, I still have problem with this patch.
>
> I think this equals to inode->i_mode = mode;
>
> Because in chmod_common(), @mode was assigned as:
>
> newattrs.ia_mode = (mode & S_IALLUGO) | (inode->i_mode & ~S_IALLUGO);
Hi Chao,
I think this means all bits of S_IALLUGO can be changed during chmod(),
and i_acl_mode has
new S_IRWXUGO bits , i_mode has old S_IRWXUGO bits.
BR,
Weichao
>
> and only S_ISUID and S_ISGID bits of newattrs.ia_mode can be changed
> during chmod()
>
> That's why setattr_copy() in fs/attr.c just uses "inode->i_mode = mode;"
>
>> set_acl_inode(inode, mode);
>
> Another problem is if i_acl_mode is used for error path handling, here
> i_acl_mode
> and i_mode have the same value, that's not correct?
>
> Jaegeuk,
>
> IIUC, i_acl_mode was introduced for i_mode recovery once acl progress
> fails?
>
> Thanks,
>
>> }
>> }
>>
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to set inode->i_mode correctly for posix_acl_update_mode
2020-12-22 9:32 ` Weichao Guo
@ 2020-12-22 10:49 ` Chao Yu
2020-12-22 12:14 ` Weichao Guo
0 siblings, 1 reply; 12+ messages in thread
From: Chao Yu @ 2020-12-22 10:49 UTC (permalink / raw)
To: Weichao Guo, jaegeuk, chao; +Cc: fh, Bin Shu, linux-f2fs-devel
On 2020/12/22 17:32, Weichao Guo wrote:
>
> On 2020/12/22 16:24, Chao Yu wrote:
>> On 2020/12/14 11:54, Weichao Guo wrote:
>>> We should update the ~S_IRWXUGO part of inode->i_mode in __setattr_copy,
>>> because posix_acl_update_mode updates mode based on inode->i_mode,
>>> which finally overwrites the ~S_IRWXUGO part of i_acl_mode with old
>>> i_mode.
>>>
>>> Testcase to reproduce this bug:
>>> 0. adduser abc
>>> 1. mkfs.f2fs /dev/sdd
>>> 2. mount -t f2fs /dev/sdd /mnt/f2fs
>>> 3. mkdir /mnt/f2fs/test
>>> 4. setfacl -m u:abc:r /mnt/f2fs/test
>>> 5. chmod +s /mnt/f2fs/test
>>>
>>> Signed-off-by: Weichao Guo <guoweichao@oppo.com>
>>> Signed-off-by: Bin Shu <shubin@oppo.com>
>>> ---
>>> fs/f2fs/file.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>> index 16ea10f..4d355f9 100644
>>> --- a/fs/f2fs/file.c
>>> +++ b/fs/f2fs/file.c
>>> @@ -850,6 +850,7 @@ static void __setattr_copy(struct inode *inode,
>>> const struct iattr *attr)
>>> if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
>>> mode &= ~S_ISGID;
>>> + inode->i_mode = (inode->i_mode & S_IRWXUGO) | (mode &
>>> ~S_IRWXUGO);
>>
>> Sorry, I still have problem with this patch.
>>
>> I think this equals to inode->i_mode = mode;
>>
>> Because in chmod_common(), @mode was assigned as:
>>
>> newattrs.ia_mode = (mode & S_IALLUGO) | (inode->i_mode & ~S_IALLUGO);
>
> Hi Chao,
>
> I think this means all bits of S_IALLUGO can be changed during chmod(),
> and i_acl_mode has
Hi Weichao,
Correct,
>
> new S_IRWXUGO bits , i_mode has old S_IRWXUGO bits.
Look into acl related code again, I found what f2fs now do is trying to
update i_mode and acl xattr entry atomically, so in advance updated mode
will be record to i_acl_mode, and finally, it will update i_mode w/ i_acl_mode
while acl entry update in path of f2fs_set_acl() - f2fs_setxattr().
In order to keep this rule, I think we need to change as below, let me know
if I missed something.
From: Weichao Guo <guoweichao@oppo.com>
Subject: [PATCH] f2fs: fix to set inode->i_mode correctly for
posix_acl_update_mode
---
fs/f2fs/acl.c | 23 ++++++++++++++++++++++-
fs/f2fs/xattr.c | 15 +++++++++------
2 files changed, 31 insertions(+), 7 deletions(-)
diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
index 1e5e9b1136ee..732ec10e7890 100644
--- a/fs/f2fs/acl.c
+++ b/fs/f2fs/acl.c
@@ -200,6 +200,27 @@ struct posix_acl *f2fs_get_acl(struct inode *inode, int type)
return __f2fs_get_acl(inode, type, NULL);
}
+static int f2fs_acl_update_mode(struct inode *inode, umode_t *mode_p,
+ struct posix_acl **acl)
+{
+ umode_t mode = inode->i_mode;
+ int error;
+
+ if (is_inode_flag_set(inode, FI_ACL_MODE))
+ mode = F2FS_I(inode)->i_acl_mode;
+
+ error = posix_acl_equiv_mode(*acl, &mode);
+ if (error < 0)
+ return error;
+ if (error == 0)
+ *acl = NULL;
+ if (!in_group_p(inode->i_gid) &&
+ !capable_wrt_inode_uidgid(inode, CAP_FSETID))
+ mode &= ~S_ISGID;
+ *mode_p = mode;
+ return 0;
+}
+
static int __f2fs_set_acl(struct inode *inode, int type,
struct posix_acl *acl, struct page *ipage)
{
@@ -213,7 +234,7 @@ static int __f2fs_set_acl(struct inode *inode, int type,
case ACL_TYPE_ACCESS:
name_index = F2FS_XATTR_INDEX_POSIX_ACL_ACCESS;
if (acl && !ipage) {
- error = posix_acl_update_mode(inode, &mode, &acl);
+ error = f2fs_acl_update_mode(inode, &mode, &acl);
if (error)
return error;
set_acl_inode(inode, mode);
diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
index 65afcc3cc68a..2086bef6c154 100644
--- a/fs/f2fs/xattr.c
+++ b/fs/f2fs/xattr.c
@@ -673,7 +673,7 @@ static int __f2fs_setxattr(struct inode *inode, int index,
}
if (value && f2fs_xattr_value_same(here, value, size))
- goto exit;
+ goto same;
} else if ((flags & XATTR_REPLACE)) {
error = -ENODATA;
goto exit;
@@ -738,17 +738,20 @@ static int __f2fs_setxattr(struct inode *inode, int index,
if (error)
goto exit;
- if (is_inode_flag_set(inode, FI_ACL_MODE)) {
- inode->i_mode = F2FS_I(inode)->i_acl_mode;
- inode->i_ctime = current_time(inode);
- clear_inode_flag(inode, FI_ACL_MODE);
- }
if (index == F2FS_XATTR_INDEX_ENCRYPTION &&
!strcmp(name, F2FS_XATTR_NAME_ENCRYPTION_CONTEXT))
f2fs_set_encrypted_inode(inode);
f2fs_mark_inode_dirty_sync(inode, true);
if (!error && S_ISDIR(inode->i_mode))
set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_CP);
+
+same:
+ if (is_inode_flag_set(inode, FI_ACL_MODE)) {
+ inode->i_mode = F2FS_I(inode)->i_acl_mode;
+ inode->i_ctime = current_time(inode);
+ clear_inode_flag(inode, FI_ACL_MODE);
+ }
+
exit:
kfree(base_addr);
return error;
--
2.29.2
>
> BR,
>
> Weichao
>
>>
>> and only S_ISUID and S_ISGID bits of newattrs.ia_mode can be changed
>> during chmod()
>>
>> That's why setattr_copy() in fs/attr.c just uses "inode->i_mode = mode;"
>>
>>> set_acl_inode(inode, mode);
>>
>> Another problem is if i_acl_mode is used for error path handling, here
>> i_acl_mode
>> and i_mode have the same value, that's not correct?
>>
>> Jaegeuk,
>>
>> IIUC, i_acl_mode was introduced for i_mode recovery once acl progress
>> fails?
>>
>> Thanks,
>>
>>> }
>>> }
>>>
> .
>
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to set inode->i_mode correctly for posix_acl_update_mode
2020-12-22 10:49 ` Chao Yu
@ 2020-12-22 12:14 ` Weichao Guo
2020-12-23 1:14 ` Chao Yu
0 siblings, 1 reply; 12+ messages in thread
From: Weichao Guo @ 2020-12-22 12:14 UTC (permalink / raw)
To: Chao Yu, jaegeuk, chao; +Cc: fh, Bin Shu, linux-f2fs-devel
On 2020/12/22 18:49, Chao Yu wrote:
> On 2020/12/22 17:32, Weichao Guo wrote:
>>
>> On 2020/12/22 16:24, Chao Yu wrote:
>>> On 2020/12/14 11:54, Weichao Guo wrote:
>>>> We should update the ~S_IRWXUGO part of inode->i_mode in
>>>> __setattr_copy,
>>>> because posix_acl_update_mode updates mode based on inode->i_mode,
>>>> which finally overwrites the ~S_IRWXUGO part of i_acl_mode with old
>>>> i_mode.
>>>>
>>>> Testcase to reproduce this bug:
>>>> 0. adduser abc
>>>> 1. mkfs.f2fs /dev/sdd
>>>> 2. mount -t f2fs /dev/sdd /mnt/f2fs
>>>> 3. mkdir /mnt/f2fs/test
>>>> 4. setfacl -m u:abc:r /mnt/f2fs/test
>>>> 5. chmod +s /mnt/f2fs/test
>>>>
>>>> Signed-off-by: Weichao Guo <guoweichao@oppo.com>
>>>> Signed-off-by: Bin Shu <shubin@oppo.com>
>>>> ---
>>>> fs/f2fs/file.c | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>>> index 16ea10f..4d355f9 100644
>>>> --- a/fs/f2fs/file.c
>>>> +++ b/fs/f2fs/file.c
>>>> @@ -850,6 +850,7 @@ static void __setattr_copy(struct inode *inode,
>>>> const struct iattr *attr)
>>>> if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
>>>> mode &= ~S_ISGID;
>>>> + inode->i_mode = (inode->i_mode & S_IRWXUGO) | (mode &
>>>> ~S_IRWXUGO);
>>>
>>> Sorry, I still have problem with this patch.
>>>
>>> I think this equals to inode->i_mode = mode;
>>>
>>> Because in chmod_common(), @mode was assigned as:
>>>
>>> newattrs.ia_mode = (mode & S_IALLUGO) | (inode->i_mode & ~S_IALLUGO);
>>
>> Hi Chao,
>>
>> I think this means all bits of S_IALLUGO can be changed during chmod(),
>> and i_acl_mode has
>
> Hi Weichao,
>
> Correct,
>
>>
>> new S_IRWXUGO bits , i_mode has old S_IRWXUGO bits.
>
> Look into acl related code again, I found what f2fs now do is trying to
> update i_mode and acl xattr entry atomically, so in advance updated mode
> will be record to i_acl_mode, and finally, it will update i_mode w/
> i_acl_mode
> while acl entry update in path of f2fs_set_acl() - f2fs_setxattr().
Hi Chao,
IMO, only the S_IRWXUGO of part of i_mode needs update with acl xattr
entry atomically.
>
> In order to keep this rule, I think we need to change as below, let me
> know
> if I missed something.
>
If we change as below, "chmod +s dir" may be failed if ACL related code
occurs some error. However,
this command should be successful , which is irrelevant with ACL.
BR,
Weichao
> From: Weichao Guo <guoweichao@oppo.com>
> Subject: [PATCH] f2fs: fix to set inode->i_mode correctly for
> posix_acl_update_mode
>
> ---
> fs/f2fs/acl.c | 23 ++++++++++++++++++++++-
> fs/f2fs/xattr.c | 15 +++++++++------
> 2 files changed, 31 insertions(+), 7 deletions(-)
>
> diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
> index 1e5e9b1136ee..732ec10e7890 100644
> --- a/fs/f2fs/acl.c
> +++ b/fs/f2fs/acl.c
> @@ -200,6 +200,27 @@ struct posix_acl *f2fs_get_acl(struct inode
> *inode, int type)
> return __f2fs_get_acl(inode, type, NULL);
> }
>
> +static int f2fs_acl_update_mode(struct inode *inode, umode_t *mode_p,
> + struct posix_acl **acl)
> +{
> + umode_t mode = inode->i_mode;
> + int error;
> +
> + if (is_inode_flag_set(inode, FI_ACL_MODE))
> + mode = F2FS_I(inode)->i_acl_mode;
> +
> + error = posix_acl_equiv_mode(*acl, &mode);
> + if (error < 0)
> + return error;
> + if (error == 0)
> + *acl = NULL;
> + if (!in_group_p(inode->i_gid) &&
> + !capable_wrt_inode_uidgid(inode, CAP_FSETID))
> + mode &= ~S_ISGID;
> + *mode_p = mode;
> + return 0;
> +}
> +
> static int __f2fs_set_acl(struct inode *inode, int type,
> struct posix_acl *acl, struct page *ipage)
> {
> @@ -213,7 +234,7 @@ static int __f2fs_set_acl(struct inode *inode, int
> type,
> case ACL_TYPE_ACCESS:
> name_index = F2FS_XATTR_INDEX_POSIX_ACL_ACCESS;
> if (acl && !ipage) {
> - error = posix_acl_update_mode(inode, &mode, &acl);
> + error = f2fs_acl_update_mode(inode, &mode, &acl);
> if (error)
> return error;
> set_acl_inode(inode, mode);
> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
> index 65afcc3cc68a..2086bef6c154 100644
> --- a/fs/f2fs/xattr.c
> +++ b/fs/f2fs/xattr.c
> @@ -673,7 +673,7 @@ static int __f2fs_setxattr(struct inode *inode,
> int index,
> }
>
> if (value && f2fs_xattr_value_same(here, value, size))
> - goto exit;
> + goto same;
> } else if ((flags & XATTR_REPLACE)) {
> error = -ENODATA;
> goto exit;
> @@ -738,17 +738,20 @@ static int __f2fs_setxattr(struct inode *inode,
> int index,
> if (error)
> goto exit;
>
> - if (is_inode_flag_set(inode, FI_ACL_MODE)) {
> - inode->i_mode = F2FS_I(inode)->i_acl_mode;
> - inode->i_ctime = current_time(inode);
> - clear_inode_flag(inode, FI_ACL_MODE);
> - }
> if (index == F2FS_XATTR_INDEX_ENCRYPTION &&
> !strcmp(name, F2FS_XATTR_NAME_ENCRYPTION_CONTEXT))
> f2fs_set_encrypted_inode(inode);
> f2fs_mark_inode_dirty_sync(inode, true);
> if (!error && S_ISDIR(inode->i_mode))
> set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_CP);
> +
> +same:
> + if (is_inode_flag_set(inode, FI_ACL_MODE)) {
> + inode->i_mode = F2FS_I(inode)->i_acl_mode;
> + inode->i_ctime = current_time(inode);
> + clear_inode_flag(inode, FI_ACL_MODE);
> + }
> +
> exit:
> kfree(base_addr);
> return error;
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to set inode->i_mode correctly for posix_acl_update_mode
2020-12-22 12:14 ` Weichao Guo
@ 2020-12-23 1:14 ` Chao Yu
2020-12-23 4:38 ` Weichao Guo
0 siblings, 1 reply; 12+ messages in thread
From: Chao Yu @ 2020-12-23 1:14 UTC (permalink / raw)
To: Weichao Guo, jaegeuk, chao; +Cc: fh, Bin Shu, linux-f2fs-devel
On 2020/12/22 20:14, Weichao Guo wrote:
>
> On 2020/12/22 18:49, Chao Yu wrote:
>> On 2020/12/22 17:32, Weichao Guo wrote:
>>>
>>> On 2020/12/22 16:24, Chao Yu wrote:
>>>> On 2020/12/14 11:54, Weichao Guo wrote:
>>>>> We should update the ~S_IRWXUGO part of inode->i_mode in
>>>>> __setattr_copy,
>>>>> because posix_acl_update_mode updates mode based on inode->i_mode,
>>>>> which finally overwrites the ~S_IRWXUGO part of i_acl_mode with old
>>>>> i_mode.
>>>>>
>>>>> Testcase to reproduce this bug:
>>>>> 0. adduser abc
>>>>> 1. mkfs.f2fs /dev/sdd
>>>>> 2. mount -t f2fs /dev/sdd /mnt/f2fs
>>>>> 3. mkdir /mnt/f2fs/test
>>>>> 4. setfacl -m u:abc:r /mnt/f2fs/test
>>>>> 5. chmod +s /mnt/f2fs/test
>>>>>
>>>>> Signed-off-by: Weichao Guo <guoweichao@oppo.com>
>>>>> Signed-off-by: Bin Shu <shubin@oppo.com>
>>>>> ---
>>>>> fs/f2fs/file.c | 1 +
>>>>> 1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>>>> index 16ea10f..4d355f9 100644
>>>>> --- a/fs/f2fs/file.c
>>>>> +++ b/fs/f2fs/file.c
>>>>> @@ -850,6 +850,7 @@ static void __setattr_copy(struct inode *inode,
>>>>> const struct iattr *attr)
>>>>> if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
>>>>> mode &= ~S_ISGID;
>>>>> + inode->i_mode = (inode->i_mode & S_IRWXUGO) | (mode &
>>>>> ~S_IRWXUGO);
>>>>
>>>> Sorry, I still have problem with this patch.
>>>>
>>>> I think this equals to inode->i_mode = mode;
>>>>
>>>> Because in chmod_common(), @mode was assigned as:
>>>>
>>>> newattrs.ia_mode = (mode & S_IALLUGO) | (inode->i_mode & ~S_IALLUGO);
>>>
>>> Hi Chao,
>>>
>>> I think this means all bits of S_IALLUGO can be changed during chmod(),
>>> and i_acl_mode has
>>
>> Hi Weichao,
>>
>> Correct,
>>
>>>
>>> new S_IRWXUGO bits , i_mode has old S_IRWXUGO bits.
>>
>> Look into acl related code again, I found what f2fs now do is trying to
>> update i_mode and acl xattr entry atomically, so in advance updated mode
>> will be record to i_acl_mode, and finally, it will update i_mode w/
>> i_acl_mode
>> while acl entry update in path of f2fs_set_acl() - f2fs_setxattr().
>
> Hi Chao,
>
> IMO, only the S_IRWXUGO of part of i_mode needs update with acl xattr
> entry atomically.
I don't get the point, IMO, all S_IALLUGO bits of i_mode and acl entries
should be updated atomically.
>
>>
>> In order to keep this rule, I think we need to change as below, let me
>> know
>> if I missed something.
>>
> If we change as below, "chmod +s dir" may be failed if ACL related code
> occurs some error. However,
>
> this command should be successful , which is irrelevant with ACL.
Will below appended change make sense to you? If posix_acl_chmod() failed,
just bail out w/o updating i_mode.
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 5bcaa68f74ad..8998fddde3e4 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -950,10 +950,9 @@ int f2fs_setattr(struct dentry *dentry, struct iattr *attr)
if (attr->ia_valid & ATTR_MODE) {
err = posix_acl_chmod(inode, f2fs_get_inode_mode(inode));
- if (err || is_inode_flag_set(inode, FI_ACL_MODE)) {
- inode->i_mode = F2FS_I(inode)->i_acl_mode;
+
+ if (is_inode_flag_set(inode, FI_ACL_MODE))
clear_inode_flag(inode, FI_ACL_MODE);
- }
}
>
> BR,
>
> Weichao
>
>> From: Weichao Guo <guoweichao@oppo.com>
>> Subject: [PATCH] f2fs: fix to set inode->i_mode correctly for
>> posix_acl_update_mode
>>
>> ---
>> fs/f2fs/acl.c | 23 ++++++++++++++++++++++-
>> fs/f2fs/xattr.c | 15 +++++++++------
>> 2 files changed, 31 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
>> index 1e5e9b1136ee..732ec10e7890 100644
>> --- a/fs/f2fs/acl.c
>> +++ b/fs/f2fs/acl.c
>> @@ -200,6 +200,27 @@ struct posix_acl *f2fs_get_acl(struct inode
>> *inode, int type)
>> return __f2fs_get_acl(inode, type, NULL);
>> }
>>
>> +static int f2fs_acl_update_mode(struct inode *inode, umode_t *mode_p,
>> + struct posix_acl **acl)
>> +{
>> + umode_t mode = inode->i_mode;
>> + int error;
>> +
>> + if (is_inode_flag_set(inode, FI_ACL_MODE))
>> + mode = F2FS_I(inode)->i_acl_mode;
>> +
>> + error = posix_acl_equiv_mode(*acl, &mode);
>> + if (error < 0)
>> + return error;
>> + if (error == 0)
>> + *acl = NULL;
>> + if (!in_group_p(inode->i_gid) &&
>> + !capable_wrt_inode_uidgid(inode, CAP_FSETID))
>> + mode &= ~S_ISGID;
>> + *mode_p = mode;
>> + return 0;
>> +}
>> +
>> static int __f2fs_set_acl(struct inode *inode, int type,
>> struct posix_acl *acl, struct page *ipage)
>> {
>> @@ -213,7 +234,7 @@ static int __f2fs_set_acl(struct inode *inode, int
>> type,
>> case ACL_TYPE_ACCESS:
>> name_index = F2FS_XATTR_INDEX_POSIX_ACL_ACCESS;
>> if (acl && !ipage) {
>> - error = posix_acl_update_mode(inode, &mode, &acl);
>> + error = f2fs_acl_update_mode(inode, &mode, &acl);
>> if (error)
>> return error;
>> set_acl_inode(inode, mode);
>> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
>> index 65afcc3cc68a..2086bef6c154 100644
>> --- a/fs/f2fs/xattr.c
>> +++ b/fs/f2fs/xattr.c
>> @@ -673,7 +673,7 @@ static int __f2fs_setxattr(struct inode *inode,
>> int index,
>> }
>>
>> if (value && f2fs_xattr_value_same(here, value, size))
>> - goto exit;
>> + goto same;
>> } else if ((flags & XATTR_REPLACE)) {
>> error = -ENODATA;
>> goto exit;
>> @@ -738,17 +738,20 @@ static int __f2fs_setxattr(struct inode *inode,
>> int index,
>> if (error)
>> goto exit;
>>
>> - if (is_inode_flag_set(inode, FI_ACL_MODE)) {
>> - inode->i_mode = F2FS_I(inode)->i_acl_mode;
>> - inode->i_ctime = current_time(inode);
>> - clear_inode_flag(inode, FI_ACL_MODE);
>> - }
>> if (index == F2FS_XATTR_INDEX_ENCRYPTION &&
>> !strcmp(name, F2FS_XATTR_NAME_ENCRYPTION_CONTEXT))
>> f2fs_set_encrypted_inode(inode);
>> f2fs_mark_inode_dirty_sync(inode, true);
>> if (!error && S_ISDIR(inode->i_mode))
>> set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_CP);
>> +
>> +same:
>> + if (is_inode_flag_set(inode, FI_ACL_MODE)) {
>> + inode->i_mode = F2FS_I(inode)->i_acl_mode;
>> + inode->i_ctime = current_time(inode);
>> + clear_inode_flag(inode, FI_ACL_MODE);
>> + }
>> +
>> exit:
>> kfree(base_addr);
>> return error;
> .
>
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to set inode->i_mode correctly for posix_acl_update_mode
2020-12-23 1:14 ` Chao Yu
@ 2020-12-23 4:38 ` Weichao Guo
2020-12-23 4:42 ` Weichao Guo
2020-12-23 8:48 ` Chao Yu
0 siblings, 2 replies; 12+ messages in thread
From: Weichao Guo @ 2020-12-23 4:38 UTC (permalink / raw)
To: Chao Yu, jaegeuk, chao; +Cc: fh, Bin Shu, linux-f2fs-devel
On 2020/12/23 9:14, Chao Yu wrote:
> On 2020/12/22 20:14, Weichao Guo wrote:
>>
>> On 2020/12/22 18:49, Chao Yu wrote:
>>> On 2020/12/22 17:32, Weichao Guo wrote:
>>>>
>>>> On 2020/12/22 16:24, Chao Yu wrote:
>>>>> On 2020/12/14 11:54, Weichao Guo wrote:
>>>>>> We should update the ~S_IRWXUGO part of inode->i_mode in
>>>>>> __setattr_copy,
>>>>>> because posix_acl_update_mode updates mode based on inode->i_mode,
>>>>>> which finally overwrites the ~S_IRWXUGO part of i_acl_mode with old
>>>>>> i_mode.
>>>>>>
>>>>>> Testcase to reproduce this bug:
>>>>>> 0. adduser abc
>>>>>> 1. mkfs.f2fs /dev/sdd
>>>>>> 2. mount -t f2fs /dev/sdd /mnt/f2fs
>>>>>> 3. mkdir /mnt/f2fs/test
>>>>>> 4. setfacl -m u:abc:r /mnt/f2fs/test
>>>>>> 5. chmod +s /mnt/f2fs/test
>>>>>>
>>>>>> Signed-off-by: Weichao Guo <guoweichao@oppo.com>
>>>>>> Signed-off-by: Bin Shu <shubin@oppo.com>
>>>>>> ---
>>>>>> fs/f2fs/file.c | 1 +
>>>>>> 1 file changed, 1 insertion(+)
>>>>>>
>>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>>>>> index 16ea10f..4d355f9 100644
>>>>>> --- a/fs/f2fs/file.c
>>>>>> +++ b/fs/f2fs/file.c
>>>>>> @@ -850,6 +850,7 @@ static void __setattr_copy(struct inode *inode,
>>>>>> const struct iattr *attr)
>>>>>> if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
>>>>>> mode &= ~S_ISGID;
>>>>>> + inode->i_mode = (inode->i_mode & S_IRWXUGO) | (mode &
>>>>>> ~S_IRWXUGO);
>>>>>
>>>>> Sorry, I still have problem with this patch.
>>>>>
>>>>> I think this equals to inode->i_mode = mode;
>>>>>
>>>>> Because in chmod_common(), @mode was assigned as:
>>>>>
>>>>> newattrs.ia_mode = (mode & S_IALLUGO) | (inode->i_mode & ~S_IALLUGO);
>>>>
>>>> Hi Chao,
>>>>
>>>> I think this means all bits of S_IALLUGO can be changed during
>>>> chmod(),
>>>> and i_acl_mode has
>>>
>>> Hi Weichao,
>>>
>>> Correct,
>>>
>>>>
>>>> new S_IRWXUGO bits , i_mode has old S_IRWXUGO bits.
>>>
>>> Look into acl related code again, I found what f2fs now do is trying to
>>> update i_mode and acl xattr entry atomically, so in advance updated
>>> mode
>>> will be record to i_acl_mode, and finally, it will update i_mode w/
>>> i_acl_mode
>>> while acl entry update in path of f2fs_set_acl() - f2fs_setxattr().
>>
>> Hi Chao,
>>
>> IMO, only the S_IRWXUGO of part of i_mode needs update with acl xattr
>> entry atomically.
>
> I don't get the point, IMO, all S_IALLUGO bits of i_mode and acl entries
> should be updated atomically.
Hi Chao,
I mean ACL is only related with user/group permissions (IIUC), it makes
sense to
keep the atomicity of ACL & S_IRWXUGO bits. For S_ISGID bit, why we
should keep
this atomicity? It seems that even the atomicity of ACL & S_IRWXUGO bits
is not
considerd in EXT4.
>
>>
>>>
>>> In order to keep this rule, I think we need to change as below, let me
>>> know
>>> if I missed something.
>>>
>> If we change as below, "chmod +s dir" may be failed if ACL related code
>> occurs some error. However,
>>
>> this command should be successful , which is irrelevant with ACL.
>
> Will below appended change make sense to you? If posix_acl_chmod()
> failed,
> just bail out w/o updating i_mode.
Forget my example about "chmod +s dir", it will be executed correctly if
ACL errors
occur. Below change is not needed & will cause the problem in my example
if included.
Overall, keeping the atomicity of S_IALLUGO bits w/ ACL is enough to me.
Keeping the
atomicity of S_IALLUGO bits w/ ACL is also OK to me.
BR,
Weichao
>
>
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 5bcaa68f74ad..8998fddde3e4 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -950,10 +950,9 @@ int f2fs_setattr(struct dentry *dentry, struct
> iattr *attr)
>
> if (attr->ia_valid & ATTR_MODE) {
> err = posix_acl_chmod(inode, f2fs_get_inode_mode(inode));
> - if (err || is_inode_flag_set(inode, FI_ACL_MODE)) {
> - inode->i_mode = F2FS_I(inode)->i_acl_mode;
> +
> + if (is_inode_flag_set(inode, FI_ACL_MODE))
> clear_inode_flag(inode, FI_ACL_MODE);
> - }
> }
>
>>
>> BR,
>>
>> Weichao
>>
>>> From: Weichao Guo <guoweichao@oppo.com>
>>> Subject: [PATCH] f2fs: fix to set inode->i_mode correctly for
>>> posix_acl_update_mode
>>>
>>> ---
>>> fs/f2fs/acl.c | 23 ++++++++++++++++++++++-
>>> fs/f2fs/xattr.c | 15 +++++++++------
>>> 2 files changed, 31 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
>>> index 1e5e9b1136ee..732ec10e7890 100644
>>> --- a/fs/f2fs/acl.c
>>> +++ b/fs/f2fs/acl.c
>>> @@ -200,6 +200,27 @@ struct posix_acl *f2fs_get_acl(struct inode
>>> *inode, int type)
>>> return __f2fs_get_acl(inode, type, NULL);
>>> }
>>>
>>> +static int f2fs_acl_update_mode(struct inode *inode, umode_t *mode_p,
>>> + struct posix_acl **acl)
>>> +{
>>> + umode_t mode = inode->i_mode;
>>> + int error;
>>> +
>>> + if (is_inode_flag_set(inode, FI_ACL_MODE))
>>> + mode = F2FS_I(inode)->i_acl_mode;
>>> +
>>> + error = posix_acl_equiv_mode(*acl, &mode);
>>> + if (error < 0)
>>> + return error;
>>> + if (error == 0)
>>> + *acl = NULL;
>>> + if (!in_group_p(inode->i_gid) &&
>>> + !capable_wrt_inode_uidgid(inode, CAP_FSETID))
>>> + mode &= ~S_ISGID;
>>> + *mode_p = mode;
>>> + return 0;
>>> +}
>>> +
>>> static int __f2fs_set_acl(struct inode *inode, int type,
>>> struct posix_acl *acl, struct page *ipage)
>>> {
>>> @@ -213,7 +234,7 @@ static int __f2fs_set_acl(struct inode *inode, int
>>> type,
>>> case ACL_TYPE_ACCESS:
>>> name_index = F2FS_XATTR_INDEX_POSIX_ACL_ACCESS;
>>> if (acl && !ipage) {
>>> - error = posix_acl_update_mode(inode, &mode, &acl);
>>> + error = f2fs_acl_update_mode(inode, &mode, &acl);
>>> if (error)
>>> return error;
>>> set_acl_inode(inode, mode);
>>> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
>>> index 65afcc3cc68a..2086bef6c154 100644
>>> --- a/fs/f2fs/xattr.c
>>> +++ b/fs/f2fs/xattr.c
>>> @@ -673,7 +673,7 @@ static int __f2fs_setxattr(struct inode *inode,
>>> int index,
>>> }
>>>
>>> if (value && f2fs_xattr_value_same(here, value, size))
>>> - goto exit;
>>> + goto same;
>>> } else if ((flags & XATTR_REPLACE)) {
>>> error = -ENODATA;
>>> goto exit;
>>> @@ -738,17 +738,20 @@ static int __f2fs_setxattr(struct inode *inode,
>>> int index,
>>> if (error)
>>> goto exit;
>>>
>>> - if (is_inode_flag_set(inode, FI_ACL_MODE)) {
>>> - inode->i_mode = F2FS_I(inode)->i_acl_mode;
>>> - inode->i_ctime = current_time(inode);
>>> - clear_inode_flag(inode, FI_ACL_MODE);
>>> - }
>>> if (index == F2FS_XATTR_INDEX_ENCRYPTION &&
>>> !strcmp(name, F2FS_XATTR_NAME_ENCRYPTION_CONTEXT))
>>> f2fs_set_encrypted_inode(inode);
>>> f2fs_mark_inode_dirty_sync(inode, true);
>>> if (!error && S_ISDIR(inode->i_mode))
>>> set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_CP);
>>> +
>>> +same:
>>> + if (is_inode_flag_set(inode, FI_ACL_MODE)) {
>>> + inode->i_mode = F2FS_I(inode)->i_acl_mode;
>>> + inode->i_ctime = current_time(inode);
>>> + clear_inode_flag(inode, FI_ACL_MODE);
>>> + }
>>> +
>>> exit:
>>> kfree(base_addr);
>>> return error;
>> .
>>
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to set inode->i_mode correctly for posix_acl_update_mode
2020-12-23 4:38 ` Weichao Guo
@ 2020-12-23 4:42 ` Weichao Guo
2020-12-23 8:48 ` Chao Yu
1 sibling, 0 replies; 12+ messages in thread
From: Weichao Guo @ 2020-12-23 4:42 UTC (permalink / raw)
To: Chao Yu, jaegeuk, chao; +Cc: fh, Bin Shu, linux-f2fs-devel
On 2020/12/23 12:38, Weichao Guo wrote:
>
> On 2020/12/23 9:14, Chao Yu wrote:
>> On 2020/12/22 20:14, Weichao Guo wrote:
>>>
>>> On 2020/12/22 18:49, Chao Yu wrote:
>>>> On 2020/12/22 17:32, Weichao Guo wrote:
>>>>>
>>>>> On 2020/12/22 16:24, Chao Yu wrote:
>>>>>> On 2020/12/14 11:54, Weichao Guo wrote:
>>>>>>> We should update the ~S_IRWXUGO part of inode->i_mode in
>>>>>>> __setattr_copy,
>>>>>>> because posix_acl_update_mode updates mode based on inode->i_mode,
>>>>>>> which finally overwrites the ~S_IRWXUGO part of i_acl_mode with old
>>>>>>> i_mode.
>>>>>>>
>>>>>>> Testcase to reproduce this bug:
>>>>>>> 0. adduser abc
>>>>>>> 1. mkfs.f2fs /dev/sdd
>>>>>>> 2. mount -t f2fs /dev/sdd /mnt/f2fs
>>>>>>> 3. mkdir /mnt/f2fs/test
>>>>>>> 4. setfacl -m u:abc:r /mnt/f2fs/test
>>>>>>> 5. chmod +s /mnt/f2fs/test
>>>>>>>
>>>>>>> Signed-off-by: Weichao Guo <guoweichao@oppo.com>
>>>>>>> Signed-off-by: Bin Shu <shubin@oppo.com>
>>>>>>> ---
>>>>>>> fs/f2fs/file.c | 1 +
>>>>>>> 1 file changed, 1 insertion(+)
>>>>>>>
>>>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>>>>>> index 16ea10f..4d355f9 100644
>>>>>>> --- a/fs/f2fs/file.c
>>>>>>> +++ b/fs/f2fs/file.c
>>>>>>> @@ -850,6 +850,7 @@ static void __setattr_copy(struct inode *inode,
>>>>>>> const struct iattr *attr)
>>>>>>> if (!in_group_p(inode->i_gid) &&
>>>>>>> !capable(CAP_FSETID))
>>>>>>> mode &= ~S_ISGID;
>>>>>>> + inode->i_mode = (inode->i_mode & S_IRWXUGO) | (mode &
>>>>>>> ~S_IRWXUGO);
>>>>>>
>>>>>> Sorry, I still have problem with this patch.
>>>>>>
>>>>>> I think this equals to inode->i_mode = mode;
>>>>>>
>>>>>> Because in chmod_common(), @mode was assigned as:
>>>>>>
>>>>>> newattrs.ia_mode = (mode & S_IALLUGO) | (inode->i_mode &
>>>>>> ~S_IALLUGO);
>>>>>
>>>>> Hi Chao,
>>>>>
>>>>> I think this means all bits of S_IALLUGO can be changed during
>>>>> chmod(),
>>>>> and i_acl_mode has
>>>>
>>>> Hi Weichao,
>>>>
>>>> Correct,
>>>>
>>>>>
>>>>> new S_IRWXUGO bits , i_mode has old S_IRWXUGO bits.
>>>>
>>>> Look into acl related code again, I found what f2fs now do is
>>>> trying to
>>>> update i_mode and acl xattr entry atomically, so in advance updated
>>>> mode
>>>> will be record to i_acl_mode, and finally, it will update i_mode w/
>>>> i_acl_mode
>>>> while acl entry update in path of f2fs_set_acl() - f2fs_setxattr().
>>>
>>> Hi Chao,
>>>
>>> IMO, only the S_IRWXUGO of part of i_mode needs update with acl xattr
>>> entry atomically.
>>
>> I don't get the point, IMO, all S_IALLUGO bits of i_mode and acl entries
>> should be updated atomically.
>
> Hi Chao,
>
> I mean ACL is only related with user/group permissions (IIUC), it
> makes sense to
>
> keep the atomicity of ACL & S_IRWXUGO bits. For S_ISGID bit, why we
> should keep
>
> this atomicity? It seems that even the atomicity of ACL & S_IRWXUGO
> bits is not
>
> considerd in EXT4.
>
>>
>>>
>>>>
>>>> In order to keep this rule, I think we need to change as below, let me
>>>> know
>>>> if I missed something.
>>>>
>>> If we change as below, "chmod +s dir" may be failed if ACL related code
>>> occurs some error. However,
>>>
>>> this command should be successful , which is irrelevant with ACL.
>>
>> Will below appended change make sense to you? If posix_acl_chmod()
>> failed,
>> just bail out w/o updating i_mode.
>
> Forget my example about "chmod +s dir", it will be executed correctly
> if ACL errors
>
> occur. Below change is not needed & will cause the problem in my
> example if included.
>
> Overall, keeping the atomicity of S_IALLUGO bits w/ ACL is enough to
> me. Keeping the
Sorry, typo here: keeping the atomicity of S_IRWXUGO bits w/ ACL is
enough to me
>
> atomicity of S_IALLUGO bits w/ ACL is also OK to me.
>
> BR,
>
> Weichao
>
>>
>>
>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>> index 5bcaa68f74ad..8998fddde3e4 100644
>> --- a/fs/f2fs/file.c
>> +++ b/fs/f2fs/file.c
>> @@ -950,10 +950,9 @@ int f2fs_setattr(struct dentry *dentry, struct
>> iattr *attr)
>>
>> if (attr->ia_valid & ATTR_MODE) {
>> err = posix_acl_chmod(inode, f2fs_get_inode_mode(inode));
>> - if (err || is_inode_flag_set(inode, FI_ACL_MODE)) {
>> - inode->i_mode = F2FS_I(inode)->i_acl_mode;
>> +
>> + if (is_inode_flag_set(inode, FI_ACL_MODE))
>> clear_inode_flag(inode, FI_ACL_MODE);
>> - }
>> }
>>
>>>
>>> BR,
>>>
>>> Weichao
>>>
>>>> From: Weichao Guo <guoweichao@oppo.com>
>>>> Subject: [PATCH] f2fs: fix to set inode->i_mode correctly for
>>>> posix_acl_update_mode
>>>>
>>>> ---
>>>> fs/f2fs/acl.c | 23 ++++++++++++++++++++++-
>>>> fs/f2fs/xattr.c | 15 +++++++++------
>>>> 2 files changed, 31 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
>>>> index 1e5e9b1136ee..732ec10e7890 100644
>>>> --- a/fs/f2fs/acl.c
>>>> +++ b/fs/f2fs/acl.c
>>>> @@ -200,6 +200,27 @@ struct posix_acl *f2fs_get_acl(struct inode
>>>> *inode, int type)
>>>> return __f2fs_get_acl(inode, type, NULL);
>>>> }
>>>>
>>>> +static int f2fs_acl_update_mode(struct inode *inode, umode_t *mode_p,
>>>> + struct posix_acl **acl)
>>>> +{
>>>> + umode_t mode = inode->i_mode;
>>>> + int error;
>>>> +
>>>> + if (is_inode_flag_set(inode, FI_ACL_MODE))
>>>> + mode = F2FS_I(inode)->i_acl_mode;
>>>> +
>>>> + error = posix_acl_equiv_mode(*acl, &mode);
>>>> + if (error < 0)
>>>> + return error;
>>>> + if (error == 0)
>>>> + *acl = NULL;
>>>> + if (!in_group_p(inode->i_gid) &&
>>>> + !capable_wrt_inode_uidgid(inode, CAP_FSETID))
>>>> + mode &= ~S_ISGID;
>>>> + *mode_p = mode;
>>>> + return 0;
>>>> +}
>>>> +
>>>> static int __f2fs_set_acl(struct inode *inode, int type,
>>>> struct posix_acl *acl, struct page *ipage)
>>>> {
>>>> @@ -213,7 +234,7 @@ static int __f2fs_set_acl(struct inode *inode, int
>>>> type,
>>>> case ACL_TYPE_ACCESS:
>>>> name_index = F2FS_XATTR_INDEX_POSIX_ACL_ACCESS;
>>>> if (acl && !ipage) {
>>>> - error = posix_acl_update_mode(inode, &mode, &acl);
>>>> + error = f2fs_acl_update_mode(inode, &mode, &acl);
>>>> if (error)
>>>> return error;
>>>> set_acl_inode(inode, mode);
>>>> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
>>>> index 65afcc3cc68a..2086bef6c154 100644
>>>> --- a/fs/f2fs/xattr.c
>>>> +++ b/fs/f2fs/xattr.c
>>>> @@ -673,7 +673,7 @@ static int __f2fs_setxattr(struct inode *inode,
>>>> int index,
>>>> }
>>>>
>>>> if (value && f2fs_xattr_value_same(here, value, size))
>>>> - goto exit;
>>>> + goto same;
>>>> } else if ((flags & XATTR_REPLACE)) {
>>>> error = -ENODATA;
>>>> goto exit;
>>>> @@ -738,17 +738,20 @@ static int __f2fs_setxattr(struct inode *inode,
>>>> int index,
>>>> if (error)
>>>> goto exit;
>>>>
>>>> - if (is_inode_flag_set(inode, FI_ACL_MODE)) {
>>>> - inode->i_mode = F2FS_I(inode)->i_acl_mode;
>>>> - inode->i_ctime = current_time(inode);
>>>> - clear_inode_flag(inode, FI_ACL_MODE);
>>>> - }
>>>> if (index == F2FS_XATTR_INDEX_ENCRYPTION &&
>>>> !strcmp(name, F2FS_XATTR_NAME_ENCRYPTION_CONTEXT))
>>>> f2fs_set_encrypted_inode(inode);
>>>> f2fs_mark_inode_dirty_sync(inode, true);
>>>> if (!error && S_ISDIR(inode->i_mode))
>>>> set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_CP);
>>>> +
>>>> +same:
>>>> + if (is_inode_flag_set(inode, FI_ACL_MODE)) {
>>>> + inode->i_mode = F2FS_I(inode)->i_acl_mode;
>>>> + inode->i_ctime = current_time(inode);
>>>> + clear_inode_flag(inode, FI_ACL_MODE);
>>>> + }
>>>> +
>>>> exit:
>>>> kfree(base_addr);
>>>> return error;
>>> .
>>>
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to set inode->i_mode correctly for posix_acl_update_mode
2020-12-23 4:38 ` Weichao Guo
2020-12-23 4:42 ` Weichao Guo
@ 2020-12-23 8:48 ` Chao Yu
1 sibling, 0 replies; 12+ messages in thread
From: Chao Yu @ 2020-12-23 8:48 UTC (permalink / raw)
To: Weichao Guo, jaegeuk, chao; +Cc: fh, Bin Shu, linux-f2fs-devel
On 2020/12/23 12:38, Weichao Guo wrote:
>
> On 2020/12/23 9:14, Chao Yu wrote:
>> On 2020/12/22 20:14, Weichao Guo wrote:
>>>
>>> On 2020/12/22 18:49, Chao Yu wrote:
>>>> On 2020/12/22 17:32, Weichao Guo wrote:
>>>>>
>>>>> On 2020/12/22 16:24, Chao Yu wrote:
>>>>>> On 2020/12/14 11:54, Weichao Guo wrote:
>>>>>>> We should update the ~S_IRWXUGO part of inode->i_mode in
>>>>>>> __setattr_copy,
>>>>>>> because posix_acl_update_mode updates mode based on inode->i_mode,
>>>>>>> which finally overwrites the ~S_IRWXUGO part of i_acl_mode with old
>>>>>>> i_mode.
>>>>>>>
>>>>>>> Testcase to reproduce this bug:
>>>>>>> 0. adduser abc
>>>>>>> 1. mkfs.f2fs /dev/sdd
>>>>>>> 2. mount -t f2fs /dev/sdd /mnt/f2fs
>>>>>>> 3. mkdir /mnt/f2fs/test
>>>>>>> 4. setfacl -m u:abc:r /mnt/f2fs/test
>>>>>>> 5. chmod +s /mnt/f2fs/test
>>>>>>>
>>>>>>> Signed-off-by: Weichao Guo <guoweichao@oppo.com>
>>>>>>> Signed-off-by: Bin Shu <shubin@oppo.com>
>>>>>>> ---
>>>>>>> fs/f2fs/file.c | 1 +
>>>>>>> 1 file changed, 1 insertion(+)
>>>>>>>
>>>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>>>>>> index 16ea10f..4d355f9 100644
>>>>>>> --- a/fs/f2fs/file.c
>>>>>>> +++ b/fs/f2fs/file.c
>>>>>>> @@ -850,6 +850,7 @@ static void __setattr_copy(struct inode *inode,
>>>>>>> const struct iattr *attr)
>>>>>>> if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
>>>>>>> mode &= ~S_ISGID;
>>>>>>> + inode->i_mode = (inode->i_mode & S_IRWXUGO) | (mode &
>>>>>>> ~S_IRWXUGO);
>>>>>>
>>>>>> Sorry, I still have problem with this patch.
>>>>>>
>>>>>> I think this equals to inode->i_mode = mode;
>>>>>>
>>>>>> Because in chmod_common(), @mode was assigned as:
>>>>>>
>>>>>> newattrs.ia_mode = (mode & S_IALLUGO) | (inode->i_mode & ~S_IALLUGO);
>>>>>
>>>>> Hi Chao,
>>>>>
>>>>> I think this means all bits of S_IALLUGO can be changed during
>>>>> chmod(),
>>>>> and i_acl_mode has
>>>>
>>>> Hi Weichao,
>>>>
>>>> Correct,
>>>>
>>>>>
>>>>> new S_IRWXUGO bits , i_mode has old S_IRWXUGO bits.
>>>>
>>>> Look into acl related code again, I found what f2fs now do is trying to
>>>> update i_mode and acl xattr entry atomically, so in advance updated
>>>> mode
>>>> will be record to i_acl_mode, and finally, it will update i_mode w/
>>>> i_acl_mode
>>>> while acl entry update in path of f2fs_set_acl() - f2fs_setxattr().
>>>
>>> Hi Chao,
>>>
>>> IMO, only the S_IRWXUGO of part of i_mode needs update with acl xattr
>>> entry atomically.
>>
>> I don't get the point, IMO, all S_IALLUGO bits of i_mode and acl entries
>> should be updated atomically.
>
> Hi Chao,
>
> I mean ACL is only related with user/group permissions (IIUC), it makes
> sense to
>
> keep the atomicity of ACL & S_IRWXUGO bits. For S_ISGID bit, why we
> should keep
Hi Weichao,
Yeah, there is no such restriction on what order we update i_mode and acl, my
concern is to keep line with original implementation strategy of f2fs acl part.
>
> this atomicity? It seems that even the atomicity of ACL & S_IRWXUGO bits
> is not
>
> considerd in EXT4.
Maybe we can do better than ext4. :)
Anyway your patch looks good to me, maybe I can send a separated patch to
cleanup flow of acl part.
Thanks,
>
>>
>>>
>>>>
>>>> In order to keep this rule, I think we need to change as below, let me
>>>> know
>>>> if I missed something.
>>>>
>>> If we change as below, "chmod +s dir" may be failed if ACL related code
>>> occurs some error. However,
>>>
>>> this command should be successful , which is irrelevant with ACL.
>>
>> Will below appended change make sense to you? If posix_acl_chmod()
>> failed,
>> just bail out w/o updating i_mode.
>
> Forget my example about "chmod +s dir", it will be executed correctly if
> ACL errors
>
> occur. Below change is not needed & will cause the problem in my example
> if included.
>
> Overall, keeping the atomicity of S_IALLUGO bits w/ ACL is enough to me.
> Keeping the
>
> atomicity of S_IALLUGO bits w/ ACL is also OK to me.
>
> BR,
>
> Weichao
>
>>
>>
>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>> index 5bcaa68f74ad..8998fddde3e4 100644
>> --- a/fs/f2fs/file.c
>> +++ b/fs/f2fs/file.c
>> @@ -950,10 +950,9 @@ int f2fs_setattr(struct dentry *dentry, struct
>> iattr *attr)
>>
>> if (attr->ia_valid & ATTR_MODE) {
>> err = posix_acl_chmod(inode, f2fs_get_inode_mode(inode));
>> - if (err || is_inode_flag_set(inode, FI_ACL_MODE)) {
>> - inode->i_mode = F2FS_I(inode)->i_acl_mode;
>> +
>> + if (is_inode_flag_set(inode, FI_ACL_MODE))
>> clear_inode_flag(inode, FI_ACL_MODE);
>> - }
>> }
>>
>>>
>>> BR,
>>>
>>> Weichao
>>>
>>>> From: Weichao Guo <guoweichao@oppo.com>
>>>> Subject: [PATCH] f2fs: fix to set inode->i_mode correctly for
>>>> posix_acl_update_mode
>>>>
>>>> ---
>>>> fs/f2fs/acl.c | 23 ++++++++++++++++++++++-
>>>> fs/f2fs/xattr.c | 15 +++++++++------
>>>> 2 files changed, 31 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
>>>> index 1e5e9b1136ee..732ec10e7890 100644
>>>> --- a/fs/f2fs/acl.c
>>>> +++ b/fs/f2fs/acl.c
>>>> @@ -200,6 +200,27 @@ struct posix_acl *f2fs_get_acl(struct inode
>>>> *inode, int type)
>>>> return __f2fs_get_acl(inode, type, NULL);
>>>> }
>>>>
>>>> +static int f2fs_acl_update_mode(struct inode *inode, umode_t *mode_p,
>>>> + struct posix_acl **acl)
>>>> +{
>>>> + umode_t mode = inode->i_mode;
>>>> + int error;
>>>> +
>>>> + if (is_inode_flag_set(inode, FI_ACL_MODE))
>>>> + mode = F2FS_I(inode)->i_acl_mode;
>>>> +
>>>> + error = posix_acl_equiv_mode(*acl, &mode);
>>>> + if (error < 0)
>>>> + return error;
>>>> + if (error == 0)
>>>> + *acl = NULL;
>>>> + if (!in_group_p(inode->i_gid) &&
>>>> + !capable_wrt_inode_uidgid(inode, CAP_FSETID))
>>>> + mode &= ~S_ISGID;
>>>> + *mode_p = mode;
>>>> + return 0;
>>>> +}
>>>> +
>>>> static int __f2fs_set_acl(struct inode *inode, int type,
>>>> struct posix_acl *acl, struct page *ipage)
>>>> {
>>>> @@ -213,7 +234,7 @@ static int __f2fs_set_acl(struct inode *inode, int
>>>> type,
>>>> case ACL_TYPE_ACCESS:
>>>> name_index = F2FS_XATTR_INDEX_POSIX_ACL_ACCESS;
>>>> if (acl && !ipage) {
>>>> - error = posix_acl_update_mode(inode, &mode, &acl);
>>>> + error = f2fs_acl_update_mode(inode, &mode, &acl);
>>>> if (error)
>>>> return error;
>>>> set_acl_inode(inode, mode);
>>>> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
>>>> index 65afcc3cc68a..2086bef6c154 100644
>>>> --- a/fs/f2fs/xattr.c
>>>> +++ b/fs/f2fs/xattr.c
>>>> @@ -673,7 +673,7 @@ static int __f2fs_setxattr(struct inode *inode,
>>>> int index,
>>>> }
>>>>
>>>> if (value && f2fs_xattr_value_same(here, value, size))
>>>> - goto exit;
>>>> + goto same;
>>>> } else if ((flags & XATTR_REPLACE)) {
>>>> error = -ENODATA;
>>>> goto exit;
>>>> @@ -738,17 +738,20 @@ static int __f2fs_setxattr(struct inode *inode,
>>>> int index,
>>>> if (error)
>>>> goto exit;
>>>>
>>>> - if (is_inode_flag_set(inode, FI_ACL_MODE)) {
>>>> - inode->i_mode = F2FS_I(inode)->i_acl_mode;
>>>> - inode->i_ctime = current_time(inode);
>>>> - clear_inode_flag(inode, FI_ACL_MODE);
>>>> - }
>>>> if (index == F2FS_XATTR_INDEX_ENCRYPTION &&
>>>> !strcmp(name, F2FS_XATTR_NAME_ENCRYPTION_CONTEXT))
>>>> f2fs_set_encrypted_inode(inode);
>>>> f2fs_mark_inode_dirty_sync(inode, true);
>>>> if (!error && S_ISDIR(inode->i_mode))
>>>> set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_CP);
>>>> +
>>>> +same:
>>>> + if (is_inode_flag_set(inode, FI_ACL_MODE)) {
>>>> + inode->i_mode = F2FS_I(inode)->i_acl_mode;
>>>> + inode->i_ctime = current_time(inode);
>>>> + clear_inode_flag(inode, FI_ACL_MODE);
>>>> + }
>>>> +
>>>> exit:
>>>> kfree(base_addr);
>>>> return error;
>>> .
>>>
> .
>
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-12-23 8:48 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-14 3:54 [f2fs-dev] [PATCH] f2fs: fix to set inode->i_mode correctly for posix_acl_update_mode Weichao Guo
2020-12-16 9:16 ` Chao Yu
2020-12-16 9:21 ` Chao Yu
2020-12-17 6:24 ` Jaegeuk Kim
2020-12-22 8:24 ` Chao Yu
2020-12-22 9:32 ` Weichao Guo
2020-12-22 10:49 ` Chao Yu
2020-12-22 12:14 ` Weichao Guo
2020-12-23 1:14 ` Chao Yu
2020-12-23 4:38 ` Weichao Guo
2020-12-23 4:42 ` Weichao Guo
2020-12-23 8:48 ` Chao Yu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.