[f2fs-dev] [PATCH] f2fs: fix to set inode->i_mode correctly for posix_acl_update_mode

[f2fs-dev] [PATCH] f2fs: fix to set inode->i_mode correctly for posix_acl_update_mode

[Weichao Guo]

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

Re: [f2fs-dev] [PATCH] f2fs: fix to set inode->i_mode correctly for posix_acl_update_mode

[Chao Yu]

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

Re: [f2fs-dev] [PATCH] f2fs: fix to set inode->i_mode correctly for posix_acl_update_mode

[Chao Yu]

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

Re: [f2fs-dev] [PATCH] f2fs: fix to set inode->i_mode correctly for posix_acl_update_mode

[Jaegeuk Kim]

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

Re: [f2fs-dev] [PATCH] f2fs: fix to set inode->i_mode correctly for posix_acl_update_mode

[Chao Yu]

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

Re: [f2fs-dev] [PATCH] f2fs: fix to set inode->i_mode correctly for posix_acl_update_mode

[Weichao Guo]

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

Re: [f2fs-dev] [PATCH] f2fs: fix to set inode->i_mode correctly for posix_acl_update_mode

[Chao Yu]

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

Re: [f2fs-dev] [PATCH] f2fs: fix to set inode->i_mode correctly for posix_acl_update_mode

[Weichao Guo]

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

Re: [f2fs-dev] [PATCH] f2fs: fix to set inode->i_mode correctly for posix_acl_update_mode

[Chao Yu]

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

Re: [f2fs-dev] [PATCH] f2fs: fix to set inode->i_mode correctly for posix_acl_update_mode

[Weichao Guo]

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

Re: [f2fs-dev] [PATCH] f2fs: fix to set inode->i_mode correctly for posix_acl_update_mode

[Weichao Guo]

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

Re: [f2fs-dev] [PATCH] f2fs: fix to set inode->i_mode correctly for posix_acl_update_mode

[Chao Yu]

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