On 2020/12/23 12:38, Weichao Guo wrote:
Sorry, typo here: keeping the atomicity of S_IRWXUGO bits w/ ACL is enough to meOn 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 <[email protected]> Signed-off-by: Bin Shu <[email protected]> --- 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 hasHi 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 modewill 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 tokeep the atomicity of ACL & S_IRWXUGO bits. For S_ISGID bit, why we should keepthis atomicity? It seems that even the atomicity of ACL & S_IRWXUGO bits is notconsiderd 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 errorsoccur. 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, Weichaodiff --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, WeichaoFrom: Weichao Guo <[email protected]> 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 [email protected] https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
_______________________________________________ Linux-f2fs-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
