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 <[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 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 <[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
