[PATCH v4 1/8] fs: move sgid strip operation from inode_init_owner into inode_sgid_strip

[PATCH v4 1/8] fs: move sgid strip operation from inode_init_owner into inode_sgid_strip

[Yang Xu]

This has no functional change. Just create and export inode_sgid_strip api for
the subsequent patch. This function is used to strip S_ISGID mode when init
a new inode.

Acked-by: Christian Brauner (Microsoft) <brauner@kernel.org>
Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 fs/inode.c         | 22 ++++++++++++++++++----
 include/linux/fs.h |  3 ++-
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index 9d9b422504d1..3215e61a0021 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2246,10 +2246,8 @@ void inode_init_owner(struct user_namespace *mnt_userns, struct inode *inode,
 		/* Directories are special, and always inherit S_ISGID */
 		if (S_ISDIR(mode))
 			mode |= S_ISGID;
-		else if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP) &&
-			 !in_group_p(i_gid_into_mnt(mnt_userns, dir)) &&
-			 !capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
-			mode &= ~S_ISGID;
+		else
+			inode_sgid_strip(mnt_userns, dir, &mode);
 	} else
 		inode_fsgid_set(inode, mnt_userns);
 	inode->i_mode = mode;
@@ -2405,3 +2403,19 @@ struct timespec64 current_time(struct inode *inode)
 	return timestamp_truncate(now, inode);
 }
 EXPORT_SYMBOL(current_time);
+
+void inode_sgid_strip(struct user_namespace *mnt_userns,
+		      const struct inode *dir, umode_t *mode)
+{
+	if (S_ISDIR(*mode) || !dir || !(dir->i_mode & S_ISGID))
+		return;
+	if ((*mode & (S_ISGID | S_IXGRP)) != (S_ISGID | S_IXGRP))
+		return;
+	if (in_group_p(i_gid_into_mnt(mnt_userns, dir)))
+		return;
+	if (capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
+		return;
+
+	*mode &= ~S_ISGID;
+}
+EXPORT_SYMBOL(inode_sgid_strip);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index bbde95387a23..4a617aaab6f6 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1897,7 +1897,8 @@ extern long compat_ptr_ioctl(struct file *file, unsigned int cmd,
 void inode_init_owner(struct user_namespace *mnt_userns, struct inode *inode,
 		      const struct inode *dir, umode_t mode);
 extern bool may_open_dev(const struct path *path);
-
+void inode_sgid_strip(struct user_namespace *mnt_userns,
+		      const struct inode *dir, umode_t *mode);
 /*
  * This is the "filldir" function type, used by readdir() to let
  * the kernel specify what kind of dirent layout it wants to have.
-- 
2.27.0

[f2fs-dev] [PATCH v4 1/8] fs: move sgid strip operation from inode_init_owner into inode_sgid_strip

[Yang Xu]

This has no functional change. Just create and export inode_sgid_strip api for
the subsequent patch. This function is used to strip S_ISGID mode when init
a new inode.

Acked-by: Christian Brauner (Microsoft) <brauner@kernel.org>
Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 fs/inode.c         | 22 ++++++++++++++++++----
 include/linux/fs.h |  3 ++-
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index 9d9b422504d1..3215e61a0021 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2246,10 +2246,8 @@ void inode_init_owner(struct user_namespace *mnt_userns, struct inode *inode,
 		/* Directories are special, and always inherit S_ISGID */
 		if (S_ISDIR(mode))
 			mode |= S_ISGID;
-		else if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP) &&
-			 !in_group_p(i_gid_into_mnt(mnt_userns, dir)) &&
-			 !capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
-			mode &= ~S_ISGID;
+		else
+			inode_sgid_strip(mnt_userns, dir, &mode);
 	} else
 		inode_fsgid_set(inode, mnt_userns);
 	inode->i_mode = mode;
@@ -2405,3 +2403,19 @@ struct timespec64 current_time(struct inode *inode)
 	return timestamp_truncate(now, inode);
 }
 EXPORT_SYMBOL(current_time);
+
+void inode_sgid_strip(struct user_namespace *mnt_userns,
+		      const struct inode *dir, umode_t *mode)
+{
+	if (S_ISDIR(*mode) || !dir || !(dir->i_mode & S_ISGID))
+		return;
+	if ((*mode & (S_ISGID | S_IXGRP)) != (S_ISGID | S_IXGRP))
+		return;
+	if (in_group_p(i_gid_into_mnt(mnt_userns, dir)))
+		return;
+	if (capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
+		return;
+
+	*mode &= ~S_ISGID;
+}
+EXPORT_SYMBOL(inode_sgid_strip);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index bbde95387a23..4a617aaab6f6 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1897,7 +1897,8 @@ extern long compat_ptr_ioctl(struct file *file, unsigned int cmd,
 void inode_init_owner(struct user_namespace *mnt_userns, struct inode *inode,
 		      const struct inode *dir, umode_t mode);
 extern bool may_open_dev(const struct path *path);
-
+void inode_sgid_strip(struct user_namespace *mnt_userns,
+		      const struct inode *dir, umode_t *mode);
 /*
  * This is the "filldir" function type, used by readdir() to let
  * the kernel specify what kind of dirent layout it wants to have.
-- 
2.27.0



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[PATCH v4 2/8] fs: Add missing umask strip in vfs_tmpfile

[Yang Xu]

All creation paths except for O_TMPFILE handle umask in the vfs directly
if the filesystem doesn't support or enable POSIX ACLs. If the filesystem
does then umask handling is deferred until posix_acl_create().
Because, O_TMPFILE misses umask handling in the vfs it will not honor
umask settings. Fix this by adding the missing umask handling.

Reported-by: Christian Brauner (Microsoft) <brauner@kernel.org>
Acked-by: Christian Brauner (Microsoft) <brauner@kernel.org>
Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 fs/namei.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/namei.c b/fs/namei.c
index 509657fdf4f5..73646e28fae0 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3521,6 +3521,8 @@ struct dentry *vfs_tmpfile(struct user_namespace *mnt_userns,
 	child = d_alloc(dentry, &slash_name);
 	if (unlikely(!child))
 		goto out_err;
+	if (!IS_POSIXACL(dir))
+		mode &= ~current_umask();
 	error = dir->i_op->tmpfile(mnt_userns, dir, child, mode);
 	if (error)
 		goto out_err;
-- 
2.27.0

[f2fs-dev] [PATCH v4 2/8] fs: Add missing umask strip in vfs_tmpfile

[Yang Xu]

All creation paths except for O_TMPFILE handle umask in the vfs directly
if the filesystem doesn't support or enable POSIX ACLs. If the filesystem
does then umask handling is deferred until posix_acl_create().
Because, O_TMPFILE misses umask handling in the vfs it will not honor
umask settings. Fix this by adding the missing umask handling.

Reported-by: Christian Brauner (Microsoft) <brauner@kernel.org>
Acked-by: Christian Brauner (Microsoft) <brauner@kernel.org>
Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 fs/namei.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/namei.c b/fs/namei.c
index 509657fdf4f5..73646e28fae0 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3521,6 +3521,8 @@ struct dentry *vfs_tmpfile(struct user_namespace *mnt_userns,
 	child = d_alloc(dentry, &slash_name);
 	if (unlikely(!child))
 		goto out_err;
+	if (!IS_POSIXACL(dir))
+		mode &= ~current_umask();
 	error = dir->i_op->tmpfile(mnt_userns, dir, child, mode);
 	if (error)
 		goto out_err;
-- 
2.27.0



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[PATCH v4 3/8] xfs: only call posix_acl_create under CONFIG_XFS_POSIX_ACL

[Yang Xu]

Since xfs_generic_create only calls xfs_set_acl when enable this kconfig, we
don't need to call posix_acl_create for the !CONFIG_XFS_POSIX_ACL case.

The previous patch has added missing umask strip for tmpfile, so all creation
paths handle umask in the vfs directly if the filesystem doesn't support or
enable POSIX ACLs.

So just put this function under CONFIG_XFS_POSIX_ACL and umask strip still works
well.

Also use unified rule for CONFIG_XFS_POSIX_ACL in this file, so use IS_ENABLED in
xfs_generic_create.

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 fs/xfs/xfs_iops.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index b34e8e4344a8..6b8df9ab215a 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -150,6 +150,7 @@ xfs_create_need_xattr(
 		return true;
 	if (default_acl)
 		return true;
+
 #if IS_ENABLED(CONFIG_SECURITY)
 	if (dir->i_sb->s_security)
 		return true;
@@ -169,7 +170,7 @@ xfs_generic_create(
 {
 	struct inode	*inode;
 	struct xfs_inode *ip = NULL;
-	struct posix_acl *default_acl, *acl;
+	struct posix_acl *default_acl = NULL, *acl = NULL;
 	struct xfs_name	name;
 	int		error;
 
@@ -184,9 +185,11 @@ xfs_generic_create(
 		rdev = 0;
 	}
 
+#if IS_ENABLED(CONFIG_XFS_POSIX_ACL)
 	error = posix_acl_create(dir, &mode, &default_acl, &acl);
 	if (error)
 		return error;
+#endif
 
 	/* Verify mode is valid also for tmpfile case */
 	error = xfs_dentry_mode_to_name(&name, dentry, mode);
@@ -209,7 +212,7 @@ xfs_generic_create(
 	if (unlikely(error))
 		goto out_cleanup_inode;
 
-#ifdef CONFIG_XFS_POSIX_ACL
+#if IS_ENABLED(CONFIG_XFS_POSIX_ACL)
 	if (default_acl) {
 		error = __xfs_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
 		if (error)
-- 
2.27.0

[f2fs-dev] [PATCH v4 3/8] xfs: only call posix_acl_create under CONFIG_XFS_POSIX_ACL

[Yang Xu]

Since xfs_generic_create only calls xfs_set_acl when enable this kconfig, we
don't need to call posix_acl_create for the !CONFIG_XFS_POSIX_ACL case.

The previous patch has added missing umask strip for tmpfile, so all creation
paths handle umask in the vfs directly if the filesystem doesn't support or
enable POSIX ACLs.

So just put this function under CONFIG_XFS_POSIX_ACL and umask strip still works
well.

Also use unified rule for CONFIG_XFS_POSIX_ACL in this file, so use IS_ENABLED in
xfs_generic_create.

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 fs/xfs/xfs_iops.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index b34e8e4344a8..6b8df9ab215a 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -150,6 +150,7 @@ xfs_create_need_xattr(
 		return true;
 	if (default_acl)
 		return true;
+
 #if IS_ENABLED(CONFIG_SECURITY)
 	if (dir->i_sb->s_security)
 		return true;
@@ -169,7 +170,7 @@ xfs_generic_create(
 {
 	struct inode	*inode;
 	struct xfs_inode *ip = NULL;
-	struct posix_acl *default_acl, *acl;
+	struct posix_acl *default_acl = NULL, *acl = NULL;
 	struct xfs_name	name;
 	int		error;
 
@@ -184,9 +185,11 @@ xfs_generic_create(
 		rdev = 0;
 	}
 
+#if IS_ENABLED(CONFIG_XFS_POSIX_ACL)
 	error = posix_acl_create(dir, &mode, &default_acl, &acl);
 	if (error)
 		return error;
+#endif
 
 	/* Verify mode is valid also for tmpfile case */
 	error = xfs_dentry_mode_to_name(&name, dentry, mode);
@@ -209,7 +212,7 @@ xfs_generic_create(
 	if (unlikely(error))
 		goto out_cleanup_inode;
 
-#ifdef CONFIG_XFS_POSIX_ACL
+#if IS_ENABLED(CONFIG_XFS_POSIX_ACL)
 	if (default_acl) {
 		error = __xfs_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
 		if (error)
-- 
2.27.0



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH v4 3/8] xfs: only call posix_acl_create under CONFIG_XFS_POSIX_ACL

[Christian Brauner]

On Tue, Apr 19, 2022 at 07:47:09PM +0800, Yang Xu wrote:
> Since xfs_generic_create only calls xfs_set_acl when enable this kconfig, we
> don't need to call posix_acl_create for the !CONFIG_XFS_POSIX_ACL case.
> 
> The previous patch has added missing umask strip for tmpfile, so all creation
> paths handle umask in the vfs directly if the filesystem doesn't support or
> enable POSIX ACLs.
> 
> So just put this function under CONFIG_XFS_POSIX_ACL and umask strip still works
> well.
> 
> Also use unified rule for CONFIG_XFS_POSIX_ACL in this file, so use IS_ENABLED in
> xfs_generic_create.
> 
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
>  fs/xfs/xfs_iops.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index b34e8e4344a8..6b8df9ab215a 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -150,6 +150,7 @@ xfs_create_need_xattr(
>  		return true;
>  	if (default_acl)
>  		return true;
> +
>  #if IS_ENABLED(CONFIG_SECURITY)
>  	if (dir->i_sb->s_security)
>  		return true;
> @@ -169,7 +170,7 @@ xfs_generic_create(
>  {
>  	struct inode	*inode;
>  	struct xfs_inode *ip = NULL;
> -	struct posix_acl *default_acl, *acl;
> +	struct posix_acl *default_acl = NULL, *acl = NULL;
>  	struct xfs_name	name;
>  	int		error;
>  
> @@ -184,9 +185,11 @@ xfs_generic_create(
>  		rdev = 0;
>  	}
>  
> +#if IS_ENABLED(CONFIG_XFS_POSIX_ACL)
>  	error = posix_acl_create(dir, &mode, &default_acl, &acl);
>  	if (error)
>  		return error;
> +#endif

Does this actually fix or improve anything?
If CONFIG_XFS_POSIX_ACL isn't selected then SB_POSIXACL won't be set in
inode->i_sb->s_flags and consequently posix_acl_create() is a nop. So
ifdefing this doesn't really do anything so I'd argue to not bother with
this change.

>  	/* Verify mode is valid also for tmpfile case */
>  	error = xfs_dentry_mode_to_name(&name, dentry, mode);
> @@ -209,7 +212,7 @@ xfs_generic_create(
>  	if (unlikely(error))
>  		goto out_cleanup_inode;
>  
> -#ifdef CONFIG_XFS_POSIX_ACL
> +#if IS_ENABLED(CONFIG_XFS_POSIX_ACL)
>  	if (default_acl) {
>  		error = __xfs_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
>  		if (error)

Side-note, I think

	#ifdef CONFIG_XFS_POSIX_ACL
	extern struct posix_acl *xfs_get_acl(struct inode *inode, int type, bool rcu);
	extern int xfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
			       struct posix_acl *acl, int type);
	extern int __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type);
	#else
	extern int xfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
			       struct posix_acl *acl, int type)
	{
		return 0;
	}
	
	extern int __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
	{
		return 0;
	}
	#endif

and then removing the inline-ifdef might be an improvement.

Re: [f2fs-dev] [PATCH v4 3/8] xfs: only call posix_acl_create under CONFIG_XFS_POSIX_ACL

[Christian Brauner]

On Tue, Apr 19, 2022 at 07:47:09PM +0800, Yang Xu wrote:
> Since xfs_generic_create only calls xfs_set_acl when enable this kconfig, we
> don't need to call posix_acl_create for the !CONFIG_XFS_POSIX_ACL case.
> 
> The previous patch has added missing umask strip for tmpfile, so all creation
> paths handle umask in the vfs directly if the filesystem doesn't support or
> enable POSIX ACLs.
> 
> So just put this function under CONFIG_XFS_POSIX_ACL and umask strip still works
> well.
> 
> Also use unified rule for CONFIG_XFS_POSIX_ACL in this file, so use IS_ENABLED in
> xfs_generic_create.
> 
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
>  fs/xfs/xfs_iops.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index b34e8e4344a8..6b8df9ab215a 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -150,6 +150,7 @@ xfs_create_need_xattr(
>  		return true;
>  	if (default_acl)
>  		return true;
> +
>  #if IS_ENABLED(CONFIG_SECURITY)
>  	if (dir->i_sb->s_security)
>  		return true;
> @@ -169,7 +170,7 @@ xfs_generic_create(
>  {
>  	struct inode	*inode;
>  	struct xfs_inode *ip = NULL;
> -	struct posix_acl *default_acl, *acl;
> +	struct posix_acl *default_acl = NULL, *acl = NULL;
>  	struct xfs_name	name;
>  	int		error;
>  
> @@ -184,9 +185,11 @@ xfs_generic_create(
>  		rdev = 0;
>  	}
>  
> +#if IS_ENABLED(CONFIG_XFS_POSIX_ACL)
>  	error = posix_acl_create(dir, &mode, &default_acl, &acl);
>  	if (error)
>  		return error;
> +#endif

Does this actually fix or improve anything?
If CONFIG_XFS_POSIX_ACL isn't selected then SB_POSIXACL won't be set in
inode->i_sb->s_flags and consequently posix_acl_create() is a nop. So
ifdefing this doesn't really do anything so I'd argue to not bother with
this change.

>  	/* Verify mode is valid also for tmpfile case */
>  	error = xfs_dentry_mode_to_name(&name, dentry, mode);
> @@ -209,7 +212,7 @@ xfs_generic_create(
>  	if (unlikely(error))
>  		goto out_cleanup_inode;
>  
> -#ifdef CONFIG_XFS_POSIX_ACL
> +#if IS_ENABLED(CONFIG_XFS_POSIX_ACL)
>  	if (default_acl) {
>  		error = __xfs_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
>  		if (error)

Side-note, I think

	#ifdef CONFIG_XFS_POSIX_ACL
	extern struct posix_acl *xfs_get_acl(struct inode *inode, int type, bool rcu);
	extern int xfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
			       struct posix_acl *acl, int type);
	extern int __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type);
	#else
	extern int xfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
			       struct posix_acl *acl, int type)
	{
		return 0;
	}
	
	extern int __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
	{
		return 0;
	}
	#endif

and then removing the inline-ifdef might be an improvement.


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH v4 3/8] xfs: only call posix_acl_create under CONFIG_XFS_POSIX_ACL

[xuyang2018.jy]

on 2022/4/19 21:53, Christian Brauner wrote:
> On Tue, Apr 19, 2022 at 07:47:09PM +0800, Yang Xu wrote:
>> Since xfs_generic_create only calls xfs_set_acl when enable this kconfig, we
>> don't need to call posix_acl_create for the !CONFIG_XFS_POSIX_ACL case.
>>
>> The previous patch has added missing umask strip for tmpfile, so all creation
>> paths handle umask in the vfs directly if the filesystem doesn't support or
>> enable POSIX ACLs.
>>
>> So just put this function under CONFIG_XFS_POSIX_ACL and umask strip still works
>> well.
>>
>> Also use unified rule for CONFIG_XFS_POSIX_ACL in this file, so use IS_ENABLED in
>> xfs_generic_create.
>>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>>   fs/xfs/xfs_iops.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
>> index b34e8e4344a8..6b8df9ab215a 100644
>> --- a/fs/xfs/xfs_iops.c
>> +++ b/fs/xfs/xfs_iops.c
>> @@ -150,6 +150,7 @@ xfs_create_need_xattr(
>>   		return true;
>>   	if (default_acl)
>>   		return true;
>> +
>>   #if IS_ENABLED(CONFIG_SECURITY)
>>   	if (dir->i_sb->s_security)
>>   		return true;
>> @@ -169,7 +170,7 @@ xfs_generic_create(
>>   {
>>   	struct inode	*inode;
>>   	struct xfs_inode *ip = NULL;
>> -	struct posix_acl *default_acl, *acl;
>> +	struct posix_acl *default_acl = NULL, *acl = NULL;
>>   	struct xfs_name	name;
>>   	int		error;
>>
>> @@ -184,9 +185,11 @@ xfs_generic_create(
>>   		rdev = 0;
>>   	}
>>
>> +#if IS_ENABLED(CONFIG_XFS_POSIX_ACL)
>>   	error = posix_acl_create(dir,&mode,&default_acl,&acl);
>>   	if (error)
>>   		return error;
>> +#endif
>
> Does this actually fix or improve anything?
> If CONFIG_XFS_POSIX_ACL isn't selected then SB_POSIXACL won't be set in
> inode->i_sb->s_flags and consequently posix_acl_create() is a nop. So
> ifdefing this doesn't really do anything so I'd argue to not bother with
> this change.
It only avoid useless mode &= ~current_mask here.
>
>>   	/* Verify mode is valid also for tmpfile case */
>>   	error = xfs_dentry_mode_to_name(&name, dentry, mode);
>> @@ -209,7 +212,7 @@ xfs_generic_create(
>>   	if (unlikely(error))
>>   		goto out_cleanup_inode;
>>
>> -#ifdef CONFIG_XFS_POSIX_ACL
>> +#if IS_ENABLED(CONFIG_XFS_POSIX_ACL)
>>   	if (default_acl) {
>>   		error = __xfs_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
>>   		if (error)
>
> Side-note, I think
>
> 	#ifdef CONFIG_XFS_POSIX_ACL
> 	extern struct posix_acl *xfs_get_acl(struct inode *inode, int type, bool rcu);
> 	extern int xfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
> 			       struct posix_acl *acl, int type);
> 	extern int __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type);
> 	#else
> 	extern int xfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
> 			       struct posix_acl *acl, int type)
> 	{
> 		return 0;
> 	}
> 	
> 	extern int __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> 	{
> 		return 0;
> 	}
> 	#endif
>
> and then removing the inline-ifdef might be an improvement.
Maybe, but it should not be in this patchset as you said.

Best Regards
Yang Xu

Re: [f2fs-dev] [PATCH v4 3/8] xfs: only call posix_acl_create under CONFIG_XFS_POSIX_ACL

[xuyang2018.jy]

on 2022/4/19 21:53, Christian Brauner wrote:
> On Tue, Apr 19, 2022 at 07:47:09PM +0800, Yang Xu wrote:
>> Since xfs_generic_create only calls xfs_set_acl when enable this kconfig, we
>> don't need to call posix_acl_create for the !CONFIG_XFS_POSIX_ACL case.
>>
>> The previous patch has added missing umask strip for tmpfile, so all creation
>> paths handle umask in the vfs directly if the filesystem doesn't support or
>> enable POSIX ACLs.
>>
>> So just put this function under CONFIG_XFS_POSIX_ACL and umask strip still works
>> well.
>>
>> Also use unified rule for CONFIG_XFS_POSIX_ACL in this file, so use IS_ENABLED in
>> xfs_generic_create.
>>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>>   fs/xfs/xfs_iops.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
>> index b34e8e4344a8..6b8df9ab215a 100644
>> --- a/fs/xfs/xfs_iops.c
>> +++ b/fs/xfs/xfs_iops.c
>> @@ -150,6 +150,7 @@ xfs_create_need_xattr(
>>   		return true;
>>   	if (default_acl)
>>   		return true;
>> +
>>   #if IS_ENABLED(CONFIG_SECURITY)
>>   	if (dir->i_sb->s_security)
>>   		return true;
>> @@ -169,7 +170,7 @@ xfs_generic_create(
>>   {
>>   	struct inode	*inode;
>>   	struct xfs_inode *ip = NULL;
>> -	struct posix_acl *default_acl, *acl;
>> +	struct posix_acl *default_acl = NULL, *acl = NULL;
>>   	struct xfs_name	name;
>>   	int		error;
>>
>> @@ -184,9 +185,11 @@ xfs_generic_create(
>>   		rdev = 0;
>>   	}
>>
>> +#if IS_ENABLED(CONFIG_XFS_POSIX_ACL)
>>   	error = posix_acl_create(dir,&mode,&default_acl,&acl);
>>   	if (error)
>>   		return error;
>> +#endif
>
> Does this actually fix or improve anything?
> If CONFIG_XFS_POSIX_ACL isn't selected then SB_POSIXACL won't be set in
> inode->i_sb->s_flags and consequently posix_acl_create() is a nop. So
> ifdefing this doesn't really do anything so I'd argue to not bother with
> this change.
It only avoid useless mode &= ~current_mask here.
>
>>   	/* Verify mode is valid also for tmpfile case */
>>   	error = xfs_dentry_mode_to_name(&name, dentry, mode);
>> @@ -209,7 +212,7 @@ xfs_generic_create(
>>   	if (unlikely(error))
>>   		goto out_cleanup_inode;
>>
>> -#ifdef CONFIG_XFS_POSIX_ACL
>> +#if IS_ENABLED(CONFIG_XFS_POSIX_ACL)
>>   	if (default_acl) {
>>   		error = __xfs_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
>>   		if (error)
>
> Side-note, I think
>
> 	#ifdef CONFIG_XFS_POSIX_ACL
> 	extern struct posix_acl *xfs_get_acl(struct inode *inode, int type, bool rcu);
> 	extern int xfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
> 			       struct posix_acl *acl, int type);
> 	extern int __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type);
> 	#else
> 	extern int xfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
> 			       struct posix_acl *acl, int type)
> 	{
> 		return 0;
> 	}
> 	
> 	extern int __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> 	{
> 		return 0;
> 	}
> 	#endif
>
> and then removing the inline-ifdef might be an improvement.
Maybe, but it should not be in this patchset as you said.

Best Regards
Yang Xu

_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH v4 3/8] xfs: only call posix_acl_create under CONFIG_XFS_POSIX_ACL

[Christoph Hellwig]

A nitpick in addition to the comments from Christian:  Please wrap
your commit messages after 73 characters.  The very long lines make them
rather unreadable.

Re: [f2fs-dev] [PATCH v4 3/8] xfs: only call posix_acl_create under CONFIG_XFS_POSIX_ACL

[Christoph Hellwig]

A nitpick in addition to the comments from Christian:  Please wrap
your commit messages after 73 characters.  The very long lines make them
rather unreadable.


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[PATCH v4 4/8] NFSv3: only do posix_acl_create under CONFIG_NFS_V3_ACL

[Yang Xu]

Since nfs3_proc_create/nfs3_proc_mkdir/nfs3_proc_mknod these rpc ops are called
by nfs_create/nfs_mkdir/nfs_mkdir these inode ops, so they are all in control of
vfs.

nfs3_proc_setacls does nothing in the !CONFIG_NFS_V3_ACL case, so we put
posix_acl_create under CONFIG_NFS_V3_ACL and it also doesn't affect
sattr->ia_mode value because vfs has did umask strip.

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 fs/nfs/nfs3proc.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
index 1597eef40d54..9ab93427db30 100644
--- a/fs/nfs/nfs3proc.c
+++ b/fs/nfs/nfs3proc.c
@@ -337,7 +337,7 @@ static int
 nfs3_proc_create(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
 		 int flags)
 {
-	struct posix_acl *default_acl, *acl;
+	struct posix_acl *default_acl = NULL, *acl = NULL;
 	struct nfs3_createdata *data;
 	struct dentry *d_alias;
 	int status = -ENOMEM;
@@ -361,9 +361,11 @@ nfs3_proc_create(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
 		data->arg.create.verifier[1] = cpu_to_be32(current->pid);
 	}
 
+#if IS_ENABLED(CONFIG_NFS_V3_ACL)
 	status = posix_acl_create(dir, &sattr->ia_mode, &default_acl, &acl);
 	if (status)
 		goto out;
+#endif
 
 	for (;;) {
 		d_alias = nfs3_do_create(dir, dentry, data);
@@ -580,7 +582,7 @@ nfs3_proc_symlink(struct inode *dir, struct dentry *dentry, struct page *page,
 static int
 nfs3_proc_mkdir(struct inode *dir, struct dentry *dentry, struct iattr *sattr)
 {
-	struct posix_acl *default_acl, *acl;
+	struct posix_acl *default_acl = NULL, *acl = NULL;
 	struct nfs3_createdata *data;
 	struct dentry *d_alias;
 	int status = -ENOMEM;
@@ -591,9 +593,11 @@ nfs3_proc_mkdir(struct inode *dir, struct dentry *dentry, struct iattr *sattr)
 	if (data == NULL)
 		goto out;
 
+#if IS_ENABLED(CONFIG_NFS_V3_ACL)
 	status = posix_acl_create(dir, &sattr->ia_mode, &default_acl, &acl);
 	if (status)
 		goto out;
+#endif
 
 	data->msg.rpc_proc = &nfs3_procedures[NFS3PROC_MKDIR];
 	data->arg.mkdir.fh = NFS_FH(dir);
@@ -711,7 +715,7 @@ static int
 nfs3_proc_mknod(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
 		dev_t rdev)
 {
-	struct posix_acl *default_acl, *acl;
+	struct posix_acl *default_acl = NULL, *acl = NULL;
 	struct nfs3_createdata *data;
 	struct dentry *d_alias;
 	int status = -ENOMEM;
@@ -723,9 +727,11 @@ nfs3_proc_mknod(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
 	if (data == NULL)
 		goto out;
 
+#if IS_ENABLED(CONFIG_NFS_V3_ACL)
 	status = posix_acl_create(dir, &sattr->ia_mode, &default_acl, &acl);
 	if (status)
 		goto out;
+#endif
 
 	data->msg.rpc_proc = &nfs3_procedures[NFS3PROC_MKNOD];
 	data->arg.mknod.fh = NFS_FH(dir);
-- 
2.27.0

[f2fs-dev] [PATCH v4 4/8] NFSv3: only do posix_acl_create under CONFIG_NFS_V3_ACL

[Yang Xu]

Since nfs3_proc_create/nfs3_proc_mkdir/nfs3_proc_mknod these rpc ops are called
by nfs_create/nfs_mkdir/nfs_mkdir these inode ops, so they are all in control of
vfs.

nfs3_proc_setacls does nothing in the !CONFIG_NFS_V3_ACL case, so we put
posix_acl_create under CONFIG_NFS_V3_ACL and it also doesn't affect
sattr->ia_mode value because vfs has did umask strip.

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 fs/nfs/nfs3proc.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
index 1597eef40d54..9ab93427db30 100644
--- a/fs/nfs/nfs3proc.c
+++ b/fs/nfs/nfs3proc.c
@@ -337,7 +337,7 @@ static int
 nfs3_proc_create(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
 		 int flags)
 {
-	struct posix_acl *default_acl, *acl;
+	struct posix_acl *default_acl = NULL, *acl = NULL;
 	struct nfs3_createdata *data;
 	struct dentry *d_alias;
 	int status = -ENOMEM;
@@ -361,9 +361,11 @@ nfs3_proc_create(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
 		data->arg.create.verifier[1] = cpu_to_be32(current->pid);
 	}
 
+#if IS_ENABLED(CONFIG_NFS_V3_ACL)
 	status = posix_acl_create(dir, &sattr->ia_mode, &default_acl, &acl);
 	if (status)
 		goto out;
+#endif
 
 	for (;;) {
 		d_alias = nfs3_do_create(dir, dentry, data);
@@ -580,7 +582,7 @@ nfs3_proc_symlink(struct inode *dir, struct dentry *dentry, struct page *page,
 static int
 nfs3_proc_mkdir(struct inode *dir, struct dentry *dentry, struct iattr *sattr)
 {
-	struct posix_acl *default_acl, *acl;
+	struct posix_acl *default_acl = NULL, *acl = NULL;
 	struct nfs3_createdata *data;
 	struct dentry *d_alias;
 	int status = -ENOMEM;
@@ -591,9 +593,11 @@ nfs3_proc_mkdir(struct inode *dir, struct dentry *dentry, struct iattr *sattr)
 	if (data == NULL)
 		goto out;
 
+#if IS_ENABLED(CONFIG_NFS_V3_ACL)
 	status = posix_acl_create(dir, &sattr->ia_mode, &default_acl, &acl);
 	if (status)
 		goto out;
+#endif
 
 	data->msg.rpc_proc = &nfs3_procedures[NFS3PROC_MKDIR];
 	data->arg.mkdir.fh = NFS_FH(dir);
@@ -711,7 +715,7 @@ static int
 nfs3_proc_mknod(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
 		dev_t rdev)
 {
-	struct posix_acl *default_acl, *acl;
+	struct posix_acl *default_acl = NULL, *acl = NULL;
 	struct nfs3_createdata *data;
 	struct dentry *d_alias;
 	int status = -ENOMEM;
@@ -723,9 +727,11 @@ nfs3_proc_mknod(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
 	if (data == NULL)
 		goto out;
 
+#if IS_ENABLED(CONFIG_NFS_V3_ACL)
 	status = posix_acl_create(dir, &sattr->ia_mode, &default_acl, &acl);
 	if (status)
 		goto out;
+#endif
 
 	data->msg.rpc_proc = &nfs3_procedures[NFS3PROC_MKNOD];
 	data->arg.mknod.fh = NFS_FH(dir);
-- 
2.27.0



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH v4 4/8] NFSv3: only do posix_acl_create under CONFIG_NFS_V3_ACL

[Christian Brauner]

On Tue, Apr 19, 2022 at 07:47:10PM +0800, Yang Xu wrote:
> Since nfs3_proc_create/nfs3_proc_mkdir/nfs3_proc_mknod these rpc ops are called
> by nfs_create/nfs_mkdir/nfs_mkdir these inode ops, so they are all in control of
> vfs.
> 
> nfs3_proc_setacls does nothing in the !CONFIG_NFS_V3_ACL case, so we put
> posix_acl_create under CONFIG_NFS_V3_ACL and it also doesn't affect
> sattr->ia_mode value because vfs has did umask strip.
> 
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---

I have the same comment as on the xfs patch. If the filesystem has opted
out of acls and SB_POSIXACL isn't set in sb->s_flags then
posix_acl_create() is a nop. Why bother placing it under an ifdef?

It adds visual noise and it implies that posix_acl_create() actually
does something even if the filesystem doesn't support posix acls.

Unless this actually fixes something I'd drop this patch too.

Re: [f2fs-dev] [PATCH v4 4/8] NFSv3: only do posix_acl_create under CONFIG_NFS_V3_ACL

[Christian Brauner]

On Tue, Apr 19, 2022 at 07:47:10PM +0800, Yang Xu wrote:
> Since nfs3_proc_create/nfs3_proc_mkdir/nfs3_proc_mknod these rpc ops are called
> by nfs_create/nfs_mkdir/nfs_mkdir these inode ops, so they are all in control of
> vfs.
> 
> nfs3_proc_setacls does nothing in the !CONFIG_NFS_V3_ACL case, so we put
> posix_acl_create under CONFIG_NFS_V3_ACL and it also doesn't affect
> sattr->ia_mode value because vfs has did umask strip.
> 
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---

I have the same comment as on the xfs patch. If the filesystem has opted
out of acls and SB_POSIXACL isn't set in sb->s_flags then
posix_acl_create() is a nop. Why bother placing it under an ifdef?

It adds visual noise and it implies that posix_acl_create() actually
does something even if the filesystem doesn't support posix acls.

Unless this actually fixes something I'd drop this patch too.


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH v4 4/8] NFSv3: only do posix_acl_create under CONFIG_NFS_V3_ACL

[Trond Myklebust]

On Tue, 2022-04-19 at 15:59 +0200, Christian Brauner wrote:
> On Tue, Apr 19, 2022 at 07:47:10PM +0800, Yang Xu wrote:
> > Since nfs3_proc_create/nfs3_proc_mkdir/nfs3_proc_mknod these rpc
> > ops are called
> > by nfs_create/nfs_mkdir/nfs_mkdir these inode ops, so they are all
> > in control of
> > vfs.
> > 
> > nfs3_proc_setacls does nothing in the !CONFIG_NFS_V3_ACL case, so
> > we put
> > posix_acl_create under CONFIG_NFS_V3_ACL and it also doesn't affect
> > sattr->ia_mode value because vfs has did umask strip.
> > 
> > Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> > ---
> 
> I have the same comment as on the xfs patch. If the filesystem has
> opted
> out of acls and SB_POSIXACL isn't set in sb->s_flags then
> posix_acl_create() is a nop. Why bother placing it under an ifdef?
> 
> It adds visual noise and it implies that posix_acl_create() actually
> does something even if the filesystem doesn't support posix acls.
> 

Agreed and NACKed...

Any patch that gratuitously adds #ifdefs in situations where cleaner
alternatives exist is not going going to be applied by the NFS
maintainers.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [f2fs-dev] [PATCH v4 4/8] NFSv3: only do posix_acl_create under CONFIG_NFS_V3_ACL

[Trond Myklebust]

On Tue, 2022-04-19 at 15:59 +0200, Christian Brauner wrote:
> On Tue, Apr 19, 2022 at 07:47:10PM +0800, Yang Xu wrote:
> > Since nfs3_proc_create/nfs3_proc_mkdir/nfs3_proc_mknod these rpc
> > ops are called
> > by nfs_create/nfs_mkdir/nfs_mkdir these inode ops, so they are all
> > in control of
> > vfs.
> > 
> > nfs3_proc_setacls does nothing in the !CONFIG_NFS_V3_ACL case, so
> > we put
> > posix_acl_create under CONFIG_NFS_V3_ACL and it also doesn't affect
> > sattr->ia_mode value because vfs has did umask strip.
> > 
> > Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> > ---
> 
> I have the same comment as on the xfs patch. If the filesystem has
> opted
> out of acls and SB_POSIXACL isn't set in sb->s_flags then
> posix_acl_create() is a nop. Why bother placing it under an ifdef?
> 
> It adds visual noise and it implies that posix_acl_create() actually
> does something even if the filesystem doesn't support posix acls.
> 

Agreed and NACKed...

Any patch that gratuitously adds #ifdefs in situations where cleaner
alternatives exist is not going going to be applied by the NFS
maintainers.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH v4 4/8] NFSv3: only do posix_acl_create under CONFIG_NFS_V3_ACL

[xuyang2018.jy]

on 2022/4/19 22:11, Trond Myklebust wrote:
> On Tue, 2022-04-19 at 15:59 +0200, Christian Brauner wrote:
>> On Tue, Apr 19, 2022 at 07:47:10PM +0800, Yang Xu wrote:
>>> Since nfs3_proc_create/nfs3_proc_mkdir/nfs3_proc_mknod these rpc
>>> ops are called
>>> by nfs_create/nfs_mkdir/nfs_mkdir these inode ops, so they are all
>>> in control of
>>> vfs.
>>>
>>> nfs3_proc_setacls does nothing in the !CONFIG_NFS_V3_ACL case, so
>>> we put
>>> posix_acl_create under CONFIG_NFS_V3_ACL and it also doesn't affect
>>> sattr->ia_mode value because vfs has did umask strip.
>>>
>>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>>> ---
>>
>> I have the same comment as on the xfs patch. If the filesystem has
>> opted
>> out of acls and SB_POSIXACL isn't set in sb->s_flags then
>> posix_acl_create() is a nop. Why bother placing it under an ifdef?
>>
>> It adds visual noise and it implies that posix_acl_create() actually
>> does something even if the filesystem doesn't support posix acls.
>>
>
> Agreed and NACKed...
>
> Any patch that gratuitously adds #ifdefs in situations where cleaner
> alternatives exist is not going going to be applied by the NFS
> maintainers.
Ok, will drop this patch.
>

Re: [f2fs-dev] [PATCH v4 4/8] NFSv3: only do posix_acl_create under CONFIG_NFS_V3_ACL

[xuyang2018.jy]

on 2022/4/19 22:11, Trond Myklebust wrote:
> On Tue, 2022-04-19 at 15:59 +0200, Christian Brauner wrote:
>> On Tue, Apr 19, 2022 at 07:47:10PM +0800, Yang Xu wrote:
>>> Since nfs3_proc_create/nfs3_proc_mkdir/nfs3_proc_mknod these rpc
>>> ops are called
>>> by nfs_create/nfs_mkdir/nfs_mkdir these inode ops, so they are all
>>> in control of
>>> vfs.
>>>
>>> nfs3_proc_setacls does nothing in the !CONFIG_NFS_V3_ACL case, so
>>> we put
>>> posix_acl_create under CONFIG_NFS_V3_ACL and it also doesn't affect
>>> sattr->ia_mode value because vfs has did umask strip.
>>>
>>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>>> ---
>>
>> I have the same comment as on the xfs patch. If the filesystem has
>> opted
>> out of acls and SB_POSIXACL isn't set in sb->s_flags then
>> posix_acl_create() is a nop. Why bother placing it under an ifdef?
>>
>> It adds visual noise and it implies that posix_acl_create() actually
>> does something even if the filesystem doesn't support posix acls.
>>
>
> Agreed and NACKed...
>
> Any patch that gratuitously adds #ifdefs in situations where cleaner
> alternatives exist is not going going to be applied by the NFS
> maintainers.
Ok, will drop this patch.
>

_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[PATCH v4 5/8] f2fs: Remove useless NULL assign value for acl and default_acl

[Yang Xu]

Like other use ${fs}_init_acl and posix_acl_create filesystem, we don't
need to assign NULL for acl and default_acl pointer because f2fs_acl_create
will do this job. So remove it.

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 fs/f2fs/acl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
index eaa240b21f07..9ae2d2fec58b 100644
--- a/fs/f2fs/acl.c
+++ b/fs/f2fs/acl.c
@@ -412,7 +412,7 @@ static int f2fs_acl_create(struct inode *dir, umode_t *mode,
 int f2fs_init_acl(struct inode *inode, struct inode *dir, struct page *ipage,
 							struct page *dpage)
 {
-	struct posix_acl *default_acl = NULL, *acl = NULL;
+	struct posix_acl *default_acl, *acl;
 	int error;
 
 	error = f2fs_acl_create(dir, &inode->i_mode, &default_acl, &acl, dpage);
-- 
2.27.0

[f2fs-dev] [PATCH v4 5/8] f2fs: Remove useless NULL assign value for acl and default_acl

[Yang Xu]

Like other use ${fs}_init_acl and posix_acl_create filesystem, we don't
need to assign NULL for acl and default_acl pointer because f2fs_acl_create
will do this job. So remove it.

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 fs/f2fs/acl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
index eaa240b21f07..9ae2d2fec58b 100644
--- a/fs/f2fs/acl.c
+++ b/fs/f2fs/acl.c
@@ -412,7 +412,7 @@ static int f2fs_acl_create(struct inode *dir, umode_t *mode,
 int f2fs_init_acl(struct inode *inode, struct inode *dir, struct page *ipage,
 							struct page *dpage)
 {
-	struct posix_acl *default_acl = NULL, *acl = NULL;
+	struct posix_acl *default_acl, *acl;
 	int error;
 
 	error = f2fs_acl_create(dir, &inode->i_mode, &default_acl, &acl, dpage);
-- 
2.27.0



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH v4 5/8] f2fs: Remove useless NULL assign value for acl and default_acl

[Christian Brauner]

On Tue, Apr 19, 2022 at 07:47:11PM +0800, Yang Xu wrote:
> Like other use ${fs}_init_acl and posix_acl_create filesystem, we don't
> need to assign NULL for acl and default_acl pointer because f2fs_acl_create
> will do this job. So remove it.
> 
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
>  fs/f2fs/acl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
> index eaa240b21f07..9ae2d2fec58b 100644
> --- a/fs/f2fs/acl.c
> +++ b/fs/f2fs/acl.c
> @@ -412,7 +412,7 @@ static int f2fs_acl_create(struct inode *dir, umode_t *mode,
>  int f2fs_init_acl(struct inode *inode, struct inode *dir, struct page *ipage,
>  							struct page *dpage)
>  {
> -	struct posix_acl *default_acl = NULL, *acl = NULL;
> +	struct posix_acl *default_acl, *acl;

Hm, patches like this have nothing to do with the theme of this patch
series. They can go as completely independent patches to the relevant
fses. Imho, they don't belong with this series at all.

Re: [f2fs-dev] [PATCH v4 5/8] f2fs: Remove useless NULL assign value for acl and default_acl

[Christian Brauner]

On Tue, Apr 19, 2022 at 07:47:11PM +0800, Yang Xu wrote:
> Like other use ${fs}_init_acl and posix_acl_create filesystem, we don't
> need to assign NULL for acl and default_acl pointer because f2fs_acl_create
> will do this job. So remove it.
> 
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
>  fs/f2fs/acl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
> index eaa240b21f07..9ae2d2fec58b 100644
> --- a/fs/f2fs/acl.c
> +++ b/fs/f2fs/acl.c
> @@ -412,7 +412,7 @@ static int f2fs_acl_create(struct inode *dir, umode_t *mode,
>  int f2fs_init_acl(struct inode *inode, struct inode *dir, struct page *ipage,
>  							struct page *dpage)
>  {
> -	struct posix_acl *default_acl = NULL, *acl = NULL;
> +	struct posix_acl *default_acl, *acl;

Hm, patches like this have nothing to do with the theme of this patch
series. They can go as completely independent patches to the relevant
fses. Imho, they don't belong with this series at all.


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH v4 5/8] f2fs: Remove useless NULL assign value for acl and default_acl

[xuyang2018.jy]

on 2022/4/19 22:02, Christian Brauner wrote:
> On Tue, Apr 19, 2022 at 07:47:11PM +0800, Yang Xu wrote:
>> Like other use ${fs}_init_acl and posix_acl_create filesystem, we don't
>> need to assign NULL for acl and default_acl pointer because f2fs_acl_create
>> will do this job. So remove it.
>>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>>   fs/f2fs/acl.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
>> index eaa240b21f07..9ae2d2fec58b 100644
>> --- a/fs/f2fs/acl.c
>> +++ b/fs/f2fs/acl.c
>> @@ -412,7 +412,7 @@ static int f2fs_acl_create(struct inode *dir, umode_t *mode,
>>   int f2fs_init_acl(struct inode *inode, struct inode *dir, struct page *ipage,
>>   							struct page *dpage)
>>   {
>> -	struct posix_acl *default_acl = NULL, *acl = NULL;
>> +	struct posix_acl *default_acl, *acl;
>
> Hm, patches like this have nothing to do with the theme of this patch
> series. They can go as completely independent patches to the relevant
> fses. Imho, they don't belong with this series at all.
Ok.

Re: [f2fs-dev] [PATCH v4 5/8] f2fs: Remove useless NULL assign value for acl and default_acl

[xuyang2018.jy]

on 2022/4/19 22:02, Christian Brauner wrote:
> On Tue, Apr 19, 2022 at 07:47:11PM +0800, Yang Xu wrote:
>> Like other use ${fs}_init_acl and posix_acl_create filesystem, we don't
>> need to assign NULL for acl and default_acl pointer because f2fs_acl_create
>> will do this job. So remove it.
>>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>>   fs/f2fs/acl.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
>> index eaa240b21f07..9ae2d2fec58b 100644
>> --- a/fs/f2fs/acl.c
>> +++ b/fs/f2fs/acl.c
>> @@ -412,7 +412,7 @@ static int f2fs_acl_create(struct inode *dir, umode_t *mode,
>>   int f2fs_init_acl(struct inode *inode, struct inode *dir, struct page *ipage,
>>   							struct page *dpage)
>>   {
>> -	struct posix_acl *default_acl = NULL, *acl = NULL;
>> +	struct posix_acl *default_acl, *acl;
>
> Hm, patches like this have nothing to do with the theme of this patch
> series. They can go as completely independent patches to the relevant
> fses. Imho, they don't belong with this series at all.
Ok.

_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[PATCH v4 6/8] ntfs3: Use the same order for acl pointer check in ntfs_init_acl

[Yang Xu]

Like ext4 and other use ${fs}_init_acl filesystem, they all used the following
style

       error = posix_acl_create(dir, &inode->i_mode, &default_acl, &acl);
       if (error)
                return error;

        if (default_acl) {
                error = __ext4_set_acl(handle, inode, ACL_TYPE_DEFAULT,
                                       default_acl, XATTR_CREATE);
                posix_acl_release(default_acl);
        } else {
                inode->i_default_acl = NULL;
        }
        if (acl) {
                if (!error)
                        error = __ext4_set_acl(handle, inode, ACL_TYPE_ACCESS,
                                               acl, XATTR_CREATE);
                posix_acl_release(acl);
        } else {
                inode->i_acl = NULL;
        }
	...

So for the readability and unity of the code, adjust this order.

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 fs/ntfs3/xattr.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
index afd0ddad826f..64cefa869a61 100644
--- a/fs/ntfs3/xattr.c
+++ b/fs/ntfs3/xattr.c
@@ -642,13 +642,13 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode,
 		inode->i_default_acl = NULL;
 	}
 
-	if (!acl)
-		inode->i_acl = NULL;
-	else {
+	if (acl) {
 		if (!err)
 			err = ntfs_set_acl_ex(mnt_userns, inode, acl,
 					      ACL_TYPE_ACCESS);
 		posix_acl_release(acl);
+	} else {
+		inode->i_acl = NULL;
 	}
 
 	return err;
-- 
2.27.0

[f2fs-dev] [PATCH v4 6/8] ntfs3: Use the same order for acl pointer check in ntfs_init_acl

[Yang Xu]

Like ext4 and other use ${fs}_init_acl filesystem, they all used the following
style

       error = posix_acl_create(dir, &inode->i_mode, &default_acl, &acl);
       if (error)
                return error;

        if (default_acl) {
                error = __ext4_set_acl(handle, inode, ACL_TYPE_DEFAULT,
                                       default_acl, XATTR_CREATE);
                posix_acl_release(default_acl);
        } else {
                inode->i_default_acl = NULL;
        }
        if (acl) {
                if (!error)
                        error = __ext4_set_acl(handle, inode, ACL_TYPE_ACCESS,
                                               acl, XATTR_CREATE);
                posix_acl_release(acl);
        } else {
                inode->i_acl = NULL;
        }
	...

So for the readability and unity of the code, adjust this order.

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 fs/ntfs3/xattr.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
index afd0ddad826f..64cefa869a61 100644
--- a/fs/ntfs3/xattr.c
+++ b/fs/ntfs3/xattr.c
@@ -642,13 +642,13 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode,
 		inode->i_default_acl = NULL;
 	}
 
-	if (!acl)
-		inode->i_acl = NULL;
-	else {
+	if (acl) {
 		if (!err)
 			err = ntfs_set_acl_ex(mnt_userns, inode, acl,
 					      ACL_TYPE_ACCESS);
 		posix_acl_release(acl);
+	} else {
+		inode->i_acl = NULL;
 	}
 
 	return err;
-- 
2.27.0



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH v4 6/8] ntfs3: Use the same order for acl pointer check in ntfs_init_acl

[Christian Brauner]

On Tue, Apr 19, 2022 at 07:47:12PM +0800, Yang Xu wrote:
> Like ext4 and other use ${fs}_init_acl filesystem, they all used the following
> style
> 
>        error = posix_acl_create(dir, &inode->i_mode, &default_acl, &acl);
>        if (error)
>                 return error;
> 
>         if (default_acl) {
>                 error = __ext4_set_acl(handle, inode, ACL_TYPE_DEFAULT,
>                                        default_acl, XATTR_CREATE);
>                 posix_acl_release(default_acl);
>         } else {
>                 inode->i_default_acl = NULL;
>         }
>         if (acl) {
>                 if (!error)
>                         error = __ext4_set_acl(handle, inode, ACL_TYPE_ACCESS,
>                                                acl, XATTR_CREATE);
>                 posix_acl_release(acl);
>         } else {
>                 inode->i_acl = NULL;
>         }
> 	...
> 
> So for the readability and unity of the code, adjust this order.
> 
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---

Again, this patch is irrelevant to the main drive of this patch series
and it's sensitive enough as it is. Just drop it from this series and
upstream it separately to the relevant filesystem imho.

Re: [f2fs-dev] [PATCH v4 6/8] ntfs3: Use the same order for acl pointer check in ntfs_init_acl

[Christian Brauner]

On Tue, Apr 19, 2022 at 07:47:12PM +0800, Yang Xu wrote:
> Like ext4 and other use ${fs}_init_acl filesystem, they all used the following
> style
> 
>        error = posix_acl_create(dir, &inode->i_mode, &default_acl, &acl);
>        if (error)
>                 return error;
> 
>         if (default_acl) {
>                 error = __ext4_set_acl(handle, inode, ACL_TYPE_DEFAULT,
>                                        default_acl, XATTR_CREATE);
>                 posix_acl_release(default_acl);
>         } else {
>                 inode->i_default_acl = NULL;
>         }
>         if (acl) {
>                 if (!error)
>                         error = __ext4_set_acl(handle, inode, ACL_TYPE_ACCESS,
>                                                acl, XATTR_CREATE);
>                 posix_acl_release(acl);
>         } else {
>                 inode->i_acl = NULL;
>         }
> 	...
> 
> So for the readability and unity of the code, adjust this order.
> 
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---

Again, this patch is irrelevant to the main drive of this patch series
and it's sensitive enough as it is. Just drop it from this series and
upstream it separately to the relevant filesystem imho.


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH v4 6/8] ntfs3: Use the same order for acl pointer check in ntfs_init_acl

[xuyang2018.jy]

on 2022/4/19 22:03, Christian Brauner wrote:
> On Tue, Apr 19, 2022 at 07:47:12PM +0800, Yang Xu wrote:
>> Like ext4 and other use ${fs}_init_acl filesystem, they all used the following
>> style
>>
>>         error = posix_acl_create(dir,&inode->i_mode,&default_acl,&acl);
>>         if (error)
>>                  return error;
>>
>>          if (default_acl) {
>>                  error = __ext4_set_acl(handle, inode, ACL_TYPE_DEFAULT,
>>                                         default_acl, XATTR_CREATE);
>>                  posix_acl_release(default_acl);
>>          } else {
>>                  inode->i_default_acl = NULL;
>>          }
>>          if (acl) {
>>                  if (!error)
>>                          error = __ext4_set_acl(handle, inode, ACL_TYPE_ACCESS,
>>                                                 acl, XATTR_CREATE);
>>                  posix_acl_release(acl);
>>          } else {
>>                  inode->i_acl = NULL;
>>          }
>> 	...
>>
>> So for the readability and unity of the code, adjust this order.
>>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>
> Again, this patch is irrelevant to the main drive of this patch series
> and it's sensitive enough as it is. Just drop it from this series and
> upstream it separately to the relevant filesystem imho.
OK, will do.

Re: [f2fs-dev] [PATCH v4 6/8] ntfs3: Use the same order for acl pointer check in ntfs_init_acl

[xuyang2018.jy]

on 2022/4/19 22:03, Christian Brauner wrote:
> On Tue, Apr 19, 2022 at 07:47:12PM +0800, Yang Xu wrote:
>> Like ext4 and other use ${fs}_init_acl filesystem, they all used the following
>> style
>>
>>         error = posix_acl_create(dir,&inode->i_mode,&default_acl,&acl);
>>         if (error)
>>                  return error;
>>
>>          if (default_acl) {
>>                  error = __ext4_set_acl(handle, inode, ACL_TYPE_DEFAULT,
>>                                         default_acl, XATTR_CREATE);
>>                  posix_acl_release(default_acl);
>>          } else {
>>                  inode->i_default_acl = NULL;
>>          }
>>          if (acl) {
>>                  if (!error)
>>                          error = __ext4_set_acl(handle, inode, ACL_TYPE_ACCESS,
>>                                                 acl, XATTR_CREATE);
>>                  posix_acl_release(acl);
>>          } else {
>>                  inode->i_acl = NULL;
>>          }
>> 	...
>>
>> So for the readability and unity of the code, adjust this order.
>>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>
> Again, this patch is irrelevant to the main drive of this patch series
> and it's sensitive enough as it is. Just drop it from this series and
> upstream it separately to the relevant filesystem imho.
OK, will do.
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[PATCH v4 7/8] fs: strip file's S_ISGID mode on vfs instead of on underlying filesystem

[Yang Xu]

Currently, vfs only passes mode argument to filesystem, then use inode_init_owner()
to strip S_ISGID. Some filesystem(ie ext4/btrfs) will call inode_init_owner
firstly, then posxi acl setup, but xfs uses the contrary order. It will affect
S_ISGID clear especially we filter S_IXGRP by umask or acl.

Regardless of which filesystem is in use, failure to strip the SGID correctly is
considered a security failure that needs to be fixed. The current VFS infrastructure
requires the filesystem to do everything right and not step on any landmines to
strip the SGID bit, when in fact it can easily be done at the VFS and the filesystems
then don't even need to be aware that the SGID needs to be (or has been stripped) by
the operation the user asked to be done.

Vfs has all the info it needs - it doesn't need the filesystems to do everything
correctly with the mode and ensuring that they order things like posix acl setup
functions correctly with inode_init_owner() to strip the SGID bit.

Just strip the SGID bit at the VFS, and then the filesystems can't get it wrong.

Also, the inode_sgid_strip() api should be used before IS_POSIXACL() because
this api may change mode.

Only the following places use inode_init_owner
"
arch/powerpc/platforms/cell/spufs/inode.c:      inode_init_owner(&init_user_ns, inode, dir, mode | S_IFDIR);
arch/powerpc/platforms/cell/spufs/inode.c:      inode_init_owner(&init_user_ns, inode, dir, mode | S_IFDIR);
fs/9p/vfs_inode.c:      inode_init_owner(&init_user_ns, inode, NULL, mode);
fs/bfs/dir.c:   inode_init_owner(&init_user_ns, inode, dir, mode);
fs/btrfs/inode.c:       inode_init_owner(mnt_userns, inode, dir, mode);
fs/btrfs/tests/btrfs-tests.c:   inode_init_owner(&init_user_ns, inode, NULL, S_IFREG);
fs/ext2/ialloc.c:               inode_init_owner(&init_user_ns, inode, dir, mode);
fs/ext4/ialloc.c:               inode_init_owner(mnt_userns, inode, dir, mode);
fs/f2fs/namei.c:        inode_init_owner(mnt_userns, inode, dir, mode);
fs/hfsplus/inode.c:     inode_init_owner(&init_user_ns, inode, dir, mode);
fs/hugetlbfs/inode.c:           inode_init_owner(&init_user_ns, inode, dir, mode);
fs/jfs/jfs_inode.c:     inode_init_owner(&init_user_ns, inode, parent, mode);
fs/minix/bitmap.c:      inode_init_owner(&init_user_ns, inode, dir, mode);
fs/nilfs2/inode.c:      inode_init_owner(&init_user_ns, inode, dir, mode);
fs/ntfs3/inode.c:       inode_init_owner(mnt_userns, inode, dir, mode);
fs/ocfs2/dlmfs/dlmfs.c:         inode_init_owner(&init_user_ns, inode, NULL, mode);
fs/ocfs2/dlmfs/dlmfs.c: inode_init_owner(&init_user_ns, inode, parent, mode);
fs/ocfs2/namei.c:       inode_init_owner(&init_user_ns, inode, dir, mode);
fs/omfs/inode.c:        inode_init_owner(&init_user_ns, inode, NULL, mode);
fs/overlayfs/dir.c:     inode_init_owner(&init_user_ns, inode, dentry->d_parent->d_inode, mode);
fs/ramfs/inode.c:               inode_init_owner(&init_user_ns, inode, dir, mode);
fs/reiserfs/namei.c:    inode_init_owner(&init_user_ns, inode, dir, mode);
fs/sysv/ialloc.c:       inode_init_owner(&init_user_ns, inode, dir, mode);
fs/ubifs/dir.c: inode_init_owner(&init_user_ns, inode, dir, mode);
fs/udf/ialloc.c:        inode_init_owner(&init_user_ns, inode, dir, mode);
fs/ufs/ialloc.c:        inode_init_owner(&init_user_ns, inode, dir, mode);
fs/xfs/xfs_inode.c:             inode_init_owner(mnt_userns, inode, dir, mode);
fs/zonefs/super.c:      inode_init_owner(&init_user_ns, inode, parent, S_IFDIR | 0555);
kernel/bpf/inode.c:     inode_init_owner(&init_user_ns, inode, dir, mode);
mm/shmem.c:             inode_init_owner(&init_user_ns, inode, dir, mode);
"

They are used in filesystem init new inode function and these init inode functions are used
by following operations:
mkdir
symlink
mknod
create
tmpfile
rename

We don't care about mkdir because we don't strip SGID bit for directory except fs.xfs.irix_sgid_inherit.
But we even call it in do_mkdirat() since inode_sgid_strip() will skip directories anyway. This will
enforce the same ordering for all relevant operations and it will make the code more uniform and
easier to understand by using new helper prepare_mode().

symlink and rename only use valid mode that doesn't have SGID bit.

We have added inode_sgid_strip api for the remaining operations.

In addition to the above six operations, four filesystems has a little difference
1) btrfs has btrfs_create_subvol_root to create new inode but used non SGID bit mode and can ignore
2) ocfs2 reflink function should add inode_sgid_strip api manually because we don't add it in vfs
3) spufs which doesn't really go hrough the regular VFS callpath because it has separate system call
spu_create, but it t only allows the creation of directories and only allows bits in 0777 and can ignore
4)bpf use vfs_mkobj in bpf_obj_do_pin with "S_IFREG | ((S_IRUSR | S_IWUSR) & ~current_umask()) mode and
use bpf_mkobj_ops in bpf_iter_link_pin_kernel with S_IFREG | S_IRUSR; , so bpf is also not affected

This patch also changed grpid behaviour for ext4/xfs because the mode passed to them may been
changed by inode_sgid_strip.

Also as Christian Brauner said"
The patch itself is useful as it would move a security sensitive operation that is currently burried in
individual filesystems into the vfs layer. But it has a decent regression  potential since it might strip
filesystems that have so far relied on getting the S_ISGID bit with a mode argument. So this needs a lot
of testing and long exposure in -next for at least one full kernel cycle."

Suggested-by: Dave Chinner <david@fromorbit.com>
Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 fs/inode.c         |  2 --
 fs/namei.c         | 22 +++++++++-------------
 fs/ocfs2/namei.c   |  1 +
 include/linux/fs.h |  9 +++++++++
 4 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index 3215e61a0021..0eb1dab99893 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2246,8 +2246,6 @@ void inode_init_owner(struct user_namespace *mnt_userns, struct inode *inode,
 		/* Directories are special, and always inherit S_ISGID */
 		if (S_ISDIR(mode))
 			mode |= S_ISGID;
-		else
-			inode_sgid_strip(mnt_userns, dir, &mode);
 	} else
 		inode_fsgid_set(inode, mnt_userns);
 	inode->i_mode = mode;
diff --git a/fs/namei.c b/fs/namei.c
index 73646e28fae0..f86614ab841f 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3287,8 +3287,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
 	if (open_flag & O_CREAT) {
 		if (open_flag & O_EXCL)
 			open_flag &= ~O_TRUNC;
-		if (!IS_POSIXACL(dir->d_inode))
-			mode &= ~current_umask();
+		prepare_mode(mnt_userns, dir->d_inode, &mode);
 		if (likely(got_write))
 			create_error = may_o_create(mnt_userns, &nd->path,
 						    dentry, mode);
@@ -3521,8 +3520,7 @@ struct dentry *vfs_tmpfile(struct user_namespace *mnt_userns,
 	child = d_alloc(dentry, &slash_name);
 	if (unlikely(!child))
 		goto out_err;
-	if (!IS_POSIXACL(dir))
-		mode &= ~current_umask();
+	prepare_mode(mnt_userns, dir, &mode);
 	error = dir->i_op->tmpfile(mnt_userns, dir, child, mode);
 	if (error)
 		goto out_err;
@@ -3850,13 +3848,12 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
 	if (IS_ERR(dentry))
 		goto out1;
 
-	if (!IS_POSIXACL(path.dentry->d_inode))
-		mode &= ~current_umask();
+	mnt_userns = mnt_user_ns(path.mnt);
+	prepare_mode(mnt_userns, path.dentry->d_inode, &mode);
 	error = security_path_mknod(&path, dentry, mode, dev);
 	if (error)
 		goto out2;
 
-	mnt_userns = mnt_user_ns(path.mnt);
 	switch (mode & S_IFMT) {
 		case 0: case S_IFREG:
 			error = vfs_create(mnt_userns, path.dentry->d_inode,
@@ -3943,6 +3940,7 @@ int do_mkdirat(int dfd, struct filename *name, umode_t mode)
 	struct path path;
 	int error;
 	unsigned int lookup_flags = LOOKUP_DIRECTORY;
+	struct user_namespace *mnt_userns;
 
 retry:
 	dentry = filename_create(dfd, name, &path, lookup_flags);
@@ -3950,15 +3948,13 @@ int do_mkdirat(int dfd, struct filename *name, umode_t mode)
 	if (IS_ERR(dentry))
 		goto out_putname;
 
-	if (!IS_POSIXACL(path.dentry->d_inode))
-		mode &= ~current_umask();
+	mnt_userns = mnt_user_ns(path.mnt);
+	prepare_mode(mnt_userns, path.dentry->d_inode, &mode);
 	error = security_path_mkdir(&path, dentry, mode);
-	if (!error) {
-		struct user_namespace *mnt_userns;
-		mnt_userns = mnt_user_ns(path.mnt);
+	if (!error)
 		error = vfs_mkdir(mnt_userns, path.dentry->d_inode, dentry,
 				  mode);
-	}
+
 	done_path_create(&path, dentry);
 	if (retry_estale(error, lookup_flags)) {
 		lookup_flags |= LOOKUP_REVAL;
diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
index c75fd54b9185..c81b8e0847aa 100644
--- a/fs/ocfs2/namei.c
+++ b/fs/ocfs2/namei.c
@@ -198,6 +198,7 @@ static struct inode *ocfs2_get_init_inode(struct inode *dir, umode_t mode)
 	if (S_ISDIR(mode))
 		set_nlink(inode, 2);
 	inode_init_owner(&init_user_ns, inode, dir, mode);
+	inode_sgid_strip(&init_user_ns, dir, &mode);
 	status = dquot_initialize(inode);
 	if (status)
 		return ERR_PTR(status);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 4a617aaab6f6..8c2f4cde974b 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3458,6 +3458,15 @@ static inline bool dir_relax_shared(struct inode *inode)
 	return !IS_DEADDIR(inode);
 }
 
+static inline void prepare_mode(struct user_namespace *mnt_userns,
+				const struct inode *dir, umode_t *mode)
+{
+	inode_sgid_strip(mnt_userns, dir, mode);
+
+	if (!IS_POSIXACL(dir))
+		*mode &= ~current_umask();
+}
+
 extern bool path_noexec(const struct path *path);
 extern void inode_nohighmem(struct inode *inode);
 
-- 
2.27.0

[f2fs-dev] [PATCH v4 7/8] fs: strip file's S_ISGID mode on vfs instead of on underlying filesystem

[Yang Xu]

Currently, vfs only passes mode argument to filesystem, then use inode_init_owner()
to strip S_ISGID. Some filesystem(ie ext4/btrfs) will call inode_init_owner
firstly, then posxi acl setup, but xfs uses the contrary order. It will affect
S_ISGID clear especially we filter S_IXGRP by umask or acl.

Regardless of which filesystem is in use, failure to strip the SGID correctly is
considered a security failure that needs to be fixed. The current VFS infrastructure
requires the filesystem to do everything right and not step on any landmines to
strip the SGID bit, when in fact it can easily be done at the VFS and the filesystems
then don't even need to be aware that the SGID needs to be (or has been stripped) by
the operation the user asked to be done.

Vfs has all the info it needs - it doesn't need the filesystems to do everything
correctly with the mode and ensuring that they order things like posix acl setup
functions correctly with inode_init_owner() to strip the SGID bit.

Just strip the SGID bit at the VFS, and then the filesystems can't get it wrong.

Also, the inode_sgid_strip() api should be used before IS_POSIXACL() because
this api may change mode.

Only the following places use inode_init_owner
"
arch/powerpc/platforms/cell/spufs/inode.c:      inode_init_owner(&init_user_ns, inode, dir, mode | S_IFDIR);
arch/powerpc/platforms/cell/spufs/inode.c:      inode_init_owner(&init_user_ns, inode, dir, mode | S_IFDIR);
fs/9p/vfs_inode.c:      inode_init_owner(&init_user_ns, inode, NULL, mode);
fs/bfs/dir.c:   inode_init_owner(&init_user_ns, inode, dir, mode);
fs/btrfs/inode.c:       inode_init_owner(mnt_userns, inode, dir, mode);
fs/btrfs/tests/btrfs-tests.c:   inode_init_owner(&init_user_ns, inode, NULL, S_IFREG);
fs/ext2/ialloc.c:               inode_init_owner(&init_user_ns, inode, dir, mode);
fs/ext4/ialloc.c:               inode_init_owner(mnt_userns, inode, dir, mode);
fs/f2fs/namei.c:        inode_init_owner(mnt_userns, inode, dir, mode);
fs/hfsplus/inode.c:     inode_init_owner(&init_user_ns, inode, dir, mode);
fs/hugetlbfs/inode.c:           inode_init_owner(&init_user_ns, inode, dir, mode);
fs/jfs/jfs_inode.c:     inode_init_owner(&init_user_ns, inode, parent, mode);
fs/minix/bitmap.c:      inode_init_owner(&init_user_ns, inode, dir, mode);
fs/nilfs2/inode.c:      inode_init_owner(&init_user_ns, inode, dir, mode);
fs/ntfs3/inode.c:       inode_init_owner(mnt_userns, inode, dir, mode);
fs/ocfs2/dlmfs/dlmfs.c:         inode_init_owner(&init_user_ns, inode, NULL, mode);
fs/ocfs2/dlmfs/dlmfs.c: inode_init_owner(&init_user_ns, inode, parent, mode);
fs/ocfs2/namei.c:       inode_init_owner(&init_user_ns, inode, dir, mode);
fs/omfs/inode.c:        inode_init_owner(&init_user_ns, inode, NULL, mode);
fs/overlayfs/dir.c:     inode_init_owner(&init_user_ns, inode, dentry->d_parent->d_inode, mode);
fs/ramfs/inode.c:               inode_init_owner(&init_user_ns, inode, dir, mode);
fs/reiserfs/namei.c:    inode_init_owner(&init_user_ns, inode, dir, mode);
fs/sysv/ialloc.c:       inode_init_owner(&init_user_ns, inode, dir, mode);
fs/ubifs/dir.c: inode_init_owner(&init_user_ns, inode, dir, mode);
fs/udf/ialloc.c:        inode_init_owner(&init_user_ns, inode, dir, mode);
fs/ufs/ialloc.c:        inode_init_owner(&init_user_ns, inode, dir, mode);
fs/xfs/xfs_inode.c:             inode_init_owner(mnt_userns, inode, dir, mode);
fs/zonefs/super.c:      inode_init_owner(&init_user_ns, inode, parent, S_IFDIR | 0555);
kernel/bpf/inode.c:     inode_init_owner(&init_user_ns, inode, dir, mode);
mm/shmem.c:             inode_init_owner(&init_user_ns, inode, dir, mode);
"

They are used in filesystem init new inode function and these init inode functions are used
by following operations:
mkdir
symlink
mknod
create
tmpfile
rename

We don't care about mkdir because we don't strip SGID bit for directory except fs.xfs.irix_sgid_inherit.
But we even call it in do_mkdirat() since inode_sgid_strip() will skip directories anyway. This will
enforce the same ordering for all relevant operations and it will make the code more uniform and
easier to understand by using new helper prepare_mode().

symlink and rename only use valid mode that doesn't have SGID bit.

We have added inode_sgid_strip api for the remaining operations.

In addition to the above six operations, four filesystems has a little difference
1) btrfs has btrfs_create_subvol_root to create new inode but used non SGID bit mode and can ignore
2) ocfs2 reflink function should add inode_sgid_strip api manually because we don't add it in vfs
3) spufs which doesn't really go hrough the regular VFS callpath because it has separate system call
spu_create, but it t only allows the creation of directories and only allows bits in 0777 and can ignore
4)bpf use vfs_mkobj in bpf_obj_do_pin with "S_IFREG | ((S_IRUSR | S_IWUSR) & ~current_umask()) mode and
use bpf_mkobj_ops in bpf_iter_link_pin_kernel with S_IFREG | S_IRUSR; , so bpf is also not affected

This patch also changed grpid behaviour for ext4/xfs because the mode passed to them may been
changed by inode_sgid_strip.

Also as Christian Brauner said"
The patch itself is useful as it would move a security sensitive operation that is currently burried in
individual filesystems into the vfs layer. But it has a decent regression  potential since it might strip
filesystems that have so far relied on getting the S_ISGID bit with a mode argument. So this needs a lot
of testing and long exposure in -next for at least one full kernel cycle."

Suggested-by: Dave Chinner <david@fromorbit.com>
Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 fs/inode.c         |  2 --
 fs/namei.c         | 22 +++++++++-------------
 fs/ocfs2/namei.c   |  1 +
 include/linux/fs.h |  9 +++++++++
 4 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index 3215e61a0021..0eb1dab99893 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2246,8 +2246,6 @@ void inode_init_owner(struct user_namespace *mnt_userns, struct inode *inode,
 		/* Directories are special, and always inherit S_ISGID */
 		if (S_ISDIR(mode))
 			mode |= S_ISGID;
-		else
-			inode_sgid_strip(mnt_userns, dir, &mode);
 	} else
 		inode_fsgid_set(inode, mnt_userns);
 	inode->i_mode = mode;
diff --git a/fs/namei.c b/fs/namei.c
index 73646e28fae0..f86614ab841f 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3287,8 +3287,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
 	if (open_flag & O_CREAT) {
 		if (open_flag & O_EXCL)
 			open_flag &= ~O_TRUNC;
-		if (!IS_POSIXACL(dir->d_inode))
-			mode &= ~current_umask();
+		prepare_mode(mnt_userns, dir->d_inode, &mode);
 		if (likely(got_write))
 			create_error = may_o_create(mnt_userns, &nd->path,
 						    dentry, mode);
@@ -3521,8 +3520,7 @@ struct dentry *vfs_tmpfile(struct user_namespace *mnt_userns,
 	child = d_alloc(dentry, &slash_name);
 	if (unlikely(!child))
 		goto out_err;
-	if (!IS_POSIXACL(dir))
-		mode &= ~current_umask();
+	prepare_mode(mnt_userns, dir, &mode);
 	error = dir->i_op->tmpfile(mnt_userns, dir, child, mode);
 	if (error)
 		goto out_err;
@@ -3850,13 +3848,12 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode,
 	if (IS_ERR(dentry))
 		goto out1;
 
-	if (!IS_POSIXACL(path.dentry->d_inode))
-		mode &= ~current_umask();
+	mnt_userns = mnt_user_ns(path.mnt);
+	prepare_mode(mnt_userns, path.dentry->d_inode, &mode);
 	error = security_path_mknod(&path, dentry, mode, dev);
 	if (error)
 		goto out2;
 
-	mnt_userns = mnt_user_ns(path.mnt);
 	switch (mode & S_IFMT) {
 		case 0: case S_IFREG:
 			error = vfs_create(mnt_userns, path.dentry->d_inode,
@@ -3943,6 +3940,7 @@ int do_mkdirat(int dfd, struct filename *name, umode_t mode)
 	struct path path;
 	int error;
 	unsigned int lookup_flags = LOOKUP_DIRECTORY;
+	struct user_namespace *mnt_userns;
 
 retry:
 	dentry = filename_create(dfd, name, &path, lookup_flags);
@@ -3950,15 +3948,13 @@ int do_mkdirat(int dfd, struct filename *name, umode_t mode)
 	if (IS_ERR(dentry))
 		goto out_putname;
 
-	if (!IS_POSIXACL(path.dentry->d_inode))
-		mode &= ~current_umask();
+	mnt_userns = mnt_user_ns(path.mnt);
+	prepare_mode(mnt_userns, path.dentry->d_inode, &mode);
 	error = security_path_mkdir(&path, dentry, mode);
-	if (!error) {
-		struct user_namespace *mnt_userns;
-		mnt_userns = mnt_user_ns(path.mnt);
+	if (!error)
 		error = vfs_mkdir(mnt_userns, path.dentry->d_inode, dentry,
 				  mode);
-	}
+
 	done_path_create(&path, dentry);
 	if (retry_estale(error, lookup_flags)) {
 		lookup_flags |= LOOKUP_REVAL;
diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
index c75fd54b9185..c81b8e0847aa 100644
--- a/fs/ocfs2/namei.c
+++ b/fs/ocfs2/namei.c
@@ -198,6 +198,7 @@ static struct inode *ocfs2_get_init_inode(struct inode *dir, umode_t mode)
 	if (S_ISDIR(mode))
 		set_nlink(inode, 2);
 	inode_init_owner(&init_user_ns, inode, dir, mode);
+	inode_sgid_strip(&init_user_ns, dir, &mode);
 	status = dquot_initialize(inode);
 	if (status)
 		return ERR_PTR(status);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 4a617aaab6f6..8c2f4cde974b 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3458,6 +3458,15 @@ static inline bool dir_relax_shared(struct inode *inode)
 	return !IS_DEADDIR(inode);
 }
 
+static inline void prepare_mode(struct user_namespace *mnt_userns,
+				const struct inode *dir, umode_t *mode)
+{
+	inode_sgid_strip(mnt_userns, dir, mode);
+
+	if (!IS_POSIXACL(dir))
+		*mode &= ~current_umask();
+}
+
 extern bool path_noexec(const struct path *path);
 extern void inode_nohighmem(struct inode *inode);
 
-- 
2.27.0



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH v4 7/8] fs: strip file's S_ISGID mode on vfs instead of on underlying filesystem

[Christian Brauner]

On Tue, Apr 19, 2022 at 07:47:13PM +0800, Yang Xu wrote:
> Currently, vfs only passes mode argument to filesystem, then use inode_init_owner()
> to strip S_ISGID. Some filesystem(ie ext4/btrfs) will call inode_init_owner
> firstly, then posxi acl setup, but xfs uses the contrary order. It will affect
> S_ISGID clear especially we filter S_IXGRP by umask or acl.
> 
> Regardless of which filesystem is in use, failure to strip the SGID correctly is
> considered a security failure that needs to be fixed. The current VFS infrastructure
> requires the filesystem to do everything right and not step on any landmines to
> strip the SGID bit, when in fact it can easily be done at the VFS and the filesystems
> then don't even need to be aware that the SGID needs to be (or has been stripped) by
> the operation the user asked to be done.
> 
> Vfs has all the info it needs - it doesn't need the filesystems to do everything
> correctly with the mode and ensuring that they order things like posix acl setup
> functions correctly with inode_init_owner() to strip the SGID bit.
> 
> Just strip the SGID bit at the VFS, and then the filesystems can't get it wrong.
> 
> Also, the inode_sgid_strip() api should be used before IS_POSIXACL() because
> this api may change mode.
> 
> Only the following places use inode_init_owner
> "
> arch/powerpc/platforms/cell/spufs/inode.c:      inode_init_owner(&init_user_ns, inode, dir, mode | S_IFDIR);
> arch/powerpc/platforms/cell/spufs/inode.c:      inode_init_owner(&init_user_ns, inode, dir, mode | S_IFDIR);
> fs/9p/vfs_inode.c:      inode_init_owner(&init_user_ns, inode, NULL, mode);
> fs/bfs/dir.c:   inode_init_owner(&init_user_ns, inode, dir, mode);
> fs/btrfs/inode.c:       inode_init_owner(mnt_userns, inode, dir, mode);
> fs/btrfs/tests/btrfs-tests.c:   inode_init_owner(&init_user_ns, inode, NULL, S_IFREG);
> fs/ext2/ialloc.c:               inode_init_owner(&init_user_ns, inode, dir, mode);
> fs/ext4/ialloc.c:               inode_init_owner(mnt_userns, inode, dir, mode);
> fs/f2fs/namei.c:        inode_init_owner(mnt_userns, inode, dir, mode);
> fs/hfsplus/inode.c:     inode_init_owner(&init_user_ns, inode, dir, mode);
> fs/hugetlbfs/inode.c:           inode_init_owner(&init_user_ns, inode, dir, mode);
> fs/jfs/jfs_inode.c:     inode_init_owner(&init_user_ns, inode, parent, mode);
> fs/minix/bitmap.c:      inode_init_owner(&init_user_ns, inode, dir, mode);
> fs/nilfs2/inode.c:      inode_init_owner(&init_user_ns, inode, dir, mode);
> fs/ntfs3/inode.c:       inode_init_owner(mnt_userns, inode, dir, mode);
> fs/ocfs2/dlmfs/dlmfs.c:         inode_init_owner(&init_user_ns, inode, NULL, mode);
> fs/ocfs2/dlmfs/dlmfs.c: inode_init_owner(&init_user_ns, inode, parent, mode);
> fs/ocfs2/namei.c:       inode_init_owner(&init_user_ns, inode, dir, mode);
> fs/omfs/inode.c:        inode_init_owner(&init_user_ns, inode, NULL, mode);
> fs/overlayfs/dir.c:     inode_init_owner(&init_user_ns, inode, dentry->d_parent->d_inode, mode);
> fs/ramfs/inode.c:               inode_init_owner(&init_user_ns, inode, dir, mode);
> fs/reiserfs/namei.c:    inode_init_owner(&init_user_ns, inode, dir, mode);
> fs/sysv/ialloc.c:       inode_init_owner(&init_user_ns, inode, dir, mode);
> fs/ubifs/dir.c: inode_init_owner(&init_user_ns, inode, dir, mode);
> fs/udf/ialloc.c:        inode_init_owner(&init_user_ns, inode, dir, mode);
> fs/ufs/ialloc.c:        inode_init_owner(&init_user_ns, inode, dir, mode);
> fs/xfs/xfs_inode.c:             inode_init_owner(mnt_userns, inode, dir, mode);
> fs/zonefs/super.c:      inode_init_owner(&init_user_ns, inode, parent, S_IFDIR | 0555);
> kernel/bpf/inode.c:     inode_init_owner(&init_user_ns, inode, dir, mode);
> mm/shmem.c:             inode_init_owner(&init_user_ns, inode, dir, mode);
> "
> 
> They are used in filesystem init new inode function and these init inode functions are used
> by following operations:
> mkdir
> symlink
> mknod
> create
> tmpfile
> rename
> 
> We don't care about mkdir because we don't strip SGID bit for directory except fs.xfs.irix_sgid_inherit.
> But we even call it in do_mkdirat() since inode_sgid_strip() will skip directories anyway. This will
> enforce the same ordering for all relevant operations and it will make the code more uniform and
> easier to understand by using new helper prepare_mode().
> 
> symlink and rename only use valid mode that doesn't have SGID bit.
> 
> We have added inode_sgid_strip api for the remaining operations.
> 
> In addition to the above six operations, four filesystems has a little difference
> 1) btrfs has btrfs_create_subvol_root to create new inode but used non SGID bit mode and can ignore
> 2) ocfs2 reflink function should add inode_sgid_strip api manually because we don't add it in vfs
> 3) spufs which doesn't really go hrough the regular VFS callpath because it has separate system call
> spu_create, but it t only allows the creation of directories and only allows bits in 0777 and can ignore
> 4)bpf use vfs_mkobj in bpf_obj_do_pin with "S_IFREG | ((S_IRUSR | S_IWUSR) & ~current_umask()) mode and
> use bpf_mkobj_ops in bpf_iter_link_pin_kernel with S_IFREG | S_IRUSR; , so bpf is also not affected
> 
> This patch also changed grpid behaviour for ext4/xfs because the mode passed to them may been
> changed by inode_sgid_strip.
> 
> Also as Christian Brauner said"
> The patch itself is useful as it would move a security sensitive operation that is currently burried in
> individual filesystems into the vfs layer. But it has a decent regression  potential since it might strip
> filesystems that have so far relied on getting the S_ISGID bit with a mode argument. So this needs a lot
> of testing and long exposure in -next for at least one full kernel cycle."
> 
> Suggested-by: Dave Chinner <david@fromorbit.com>
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---

I think we're getting closer but please focus the patch series. This has
morphed into an 8 patch series where 4 or 5 of these patches are fixes
that a) I'm not sure are worth it or fix anything b) they are filesystem
specific and can be independently upstreamed and c) have nothing to do
with the core of this patch series.

So I'd suggest you'd just make this about sgid stripping and then this
doesn't have to be more than 3 maybe 4 patches, imho.

Re: [f2fs-dev] [PATCH v4 7/8] fs: strip file's S_ISGID mode on vfs instead of on underlying filesystem

[Christian Brauner]

On Tue, Apr 19, 2022 at 07:47:13PM +0800, Yang Xu wrote:
> Currently, vfs only passes mode argument to filesystem, then use inode_init_owner()
> to strip S_ISGID. Some filesystem(ie ext4/btrfs) will call inode_init_owner
> firstly, then posxi acl setup, but xfs uses the contrary order. It will affect
> S_ISGID clear especially we filter S_IXGRP by umask or acl.
> 
> Regardless of which filesystem is in use, failure to strip the SGID correctly is
> considered a security failure that needs to be fixed. The current VFS infrastructure
> requires the filesystem to do everything right and not step on any landmines to
> strip the SGID bit, when in fact it can easily be done at the VFS and the filesystems
> then don't even need to be aware that the SGID needs to be (or has been stripped) by
> the operation the user asked to be done.
> 
> Vfs has all the info it needs - it doesn't need the filesystems to do everything
> correctly with the mode and ensuring that they order things like posix acl setup
> functions correctly with inode_init_owner() to strip the SGID bit.
> 
> Just strip the SGID bit at the VFS, and then the filesystems can't get it wrong.
> 
> Also, the inode_sgid_strip() api should be used before IS_POSIXACL() because
> this api may change mode.
> 
> Only the following places use inode_init_owner
> "
> arch/powerpc/platforms/cell/spufs/inode.c:      inode_init_owner(&init_user_ns, inode, dir, mode | S_IFDIR);
> arch/powerpc/platforms/cell/spufs/inode.c:      inode_init_owner(&init_user_ns, inode, dir, mode | S_IFDIR);
> fs/9p/vfs_inode.c:      inode_init_owner(&init_user_ns, inode, NULL, mode);
> fs/bfs/dir.c:   inode_init_owner(&init_user_ns, inode, dir, mode);
> fs/btrfs/inode.c:       inode_init_owner(mnt_userns, inode, dir, mode);
> fs/btrfs/tests/btrfs-tests.c:   inode_init_owner(&init_user_ns, inode, NULL, S_IFREG);
> fs/ext2/ialloc.c:               inode_init_owner(&init_user_ns, inode, dir, mode);
> fs/ext4/ialloc.c:               inode_init_owner(mnt_userns, inode, dir, mode);
> fs/f2fs/namei.c:        inode_init_owner(mnt_userns, inode, dir, mode);
> fs/hfsplus/inode.c:     inode_init_owner(&init_user_ns, inode, dir, mode);
> fs/hugetlbfs/inode.c:           inode_init_owner(&init_user_ns, inode, dir, mode);
> fs/jfs/jfs_inode.c:     inode_init_owner(&init_user_ns, inode, parent, mode);
> fs/minix/bitmap.c:      inode_init_owner(&init_user_ns, inode, dir, mode);
> fs/nilfs2/inode.c:      inode_init_owner(&init_user_ns, inode, dir, mode);
> fs/ntfs3/inode.c:       inode_init_owner(mnt_userns, inode, dir, mode);
> fs/ocfs2/dlmfs/dlmfs.c:         inode_init_owner(&init_user_ns, inode, NULL, mode);
> fs/ocfs2/dlmfs/dlmfs.c: inode_init_owner(&init_user_ns, inode, parent, mode);
> fs/ocfs2/namei.c:       inode_init_owner(&init_user_ns, inode, dir, mode);
> fs/omfs/inode.c:        inode_init_owner(&init_user_ns, inode, NULL, mode);
> fs/overlayfs/dir.c:     inode_init_owner(&init_user_ns, inode, dentry->d_parent->d_inode, mode);
> fs/ramfs/inode.c:               inode_init_owner(&init_user_ns, inode, dir, mode);
> fs/reiserfs/namei.c:    inode_init_owner(&init_user_ns, inode, dir, mode);
> fs/sysv/ialloc.c:       inode_init_owner(&init_user_ns, inode, dir, mode);
> fs/ubifs/dir.c: inode_init_owner(&init_user_ns, inode, dir, mode);
> fs/udf/ialloc.c:        inode_init_owner(&init_user_ns, inode, dir, mode);
> fs/ufs/ialloc.c:        inode_init_owner(&init_user_ns, inode, dir, mode);
> fs/xfs/xfs_inode.c:             inode_init_owner(mnt_userns, inode, dir, mode);
> fs/zonefs/super.c:      inode_init_owner(&init_user_ns, inode, parent, S_IFDIR | 0555);
> kernel/bpf/inode.c:     inode_init_owner(&init_user_ns, inode, dir, mode);
> mm/shmem.c:             inode_init_owner(&init_user_ns, inode, dir, mode);
> "
> 
> They are used in filesystem init new inode function and these init inode functions are used
> by following operations:
> mkdir
> symlink
> mknod
> create
> tmpfile
> rename
> 
> We don't care about mkdir because we don't strip SGID bit for directory except fs.xfs.irix_sgid_inherit.
> But we even call it in do_mkdirat() since inode_sgid_strip() will skip directories anyway. This will
> enforce the same ordering for all relevant operations and it will make the code more uniform and
> easier to understand by using new helper prepare_mode().
> 
> symlink and rename only use valid mode that doesn't have SGID bit.
> 
> We have added inode_sgid_strip api for the remaining operations.
> 
> In addition to the above six operations, four filesystems has a little difference
> 1) btrfs has btrfs_create_subvol_root to create new inode but used non SGID bit mode and can ignore
> 2) ocfs2 reflink function should add inode_sgid_strip api manually because we don't add it in vfs
> 3) spufs which doesn't really go hrough the regular VFS callpath because it has separate system call
> spu_create, but it t only allows the creation of directories and only allows bits in 0777 and can ignore
> 4)bpf use vfs_mkobj in bpf_obj_do_pin with "S_IFREG | ((S_IRUSR | S_IWUSR) & ~current_umask()) mode and
> use bpf_mkobj_ops in bpf_iter_link_pin_kernel with S_IFREG | S_IRUSR; , so bpf is also not affected
> 
> This patch also changed grpid behaviour for ext4/xfs because the mode passed to them may been
> changed by inode_sgid_strip.
> 
> Also as Christian Brauner said"
> The patch itself is useful as it would move a security sensitive operation that is currently burried in
> individual filesystems into the vfs layer. But it has a decent regression  potential since it might strip
> filesystems that have so far relied on getting the S_ISGID bit with a mode argument. So this needs a lot
> of testing and long exposure in -next for at least one full kernel cycle."
> 
> Suggested-by: Dave Chinner <david@fromorbit.com>
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---

I think we're getting closer but please focus the patch series. This has
morphed into an 8 patch series where 4 or 5 of these patches are fixes
that a) I'm not sure are worth it or fix anything b) they are filesystem
specific and can be independently upstreamed and c) have nothing to do
with the core of this patch series.

So I'd suggest you'd just make this about sgid stripping and then this
doesn't have to be more than 3 maybe 4 patches, imho.


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH v4 7/8] fs: strip file's S_ISGID mode on vfs instead of on underlying filesystem

[xuyang2018.jy]

on 2022/4/19 22:09, Christian Brauner wrote:
> On Tue, Apr 19, 2022 at 07:47:13PM +0800, Yang Xu wrote:
>> Currently, vfs only passes mode argument to filesystem, then use inode_init_owner()
>> to strip S_ISGID. Some filesystem(ie ext4/btrfs) will call inode_init_owner
>> firstly, then posxi acl setup, but xfs uses the contrary order. It will affect
>> S_ISGID clear especially we filter S_IXGRP by umask or acl.
>>
>> Regardless of which filesystem is in use, failure to strip the SGID correctly is
>> considered a security failure that needs to be fixed. The current VFS infrastructure
>> requires the filesystem to do everything right and not step on any landmines to
>> strip the SGID bit, when in fact it can easily be done at the VFS and the filesystems
>> then don't even need to be aware that the SGID needs to be (or has been stripped) by
>> the operation the user asked to be done.
>>
>> Vfs has all the info it needs - it doesn't need the filesystems to do everything
>> correctly with the mode and ensuring that they order things like posix acl setup
>> functions correctly with inode_init_owner() to strip the SGID bit.
>>
>> Just strip the SGID bit at the VFS, and then the filesystems can't get it wrong.
>>
>> Also, the inode_sgid_strip() api should be used before IS_POSIXACL() because
>> this api may change mode.
>>
>> Only the following places use inode_init_owner
>> "
>> arch/powerpc/platforms/cell/spufs/inode.c:      inode_init_owner(&init_user_ns, inode, dir, mode | S_IFDIR);
>> arch/powerpc/platforms/cell/spufs/inode.c:      inode_init_owner(&init_user_ns, inode, dir, mode | S_IFDIR);
>> fs/9p/vfs_inode.c:      inode_init_owner(&init_user_ns, inode, NULL, mode);
>> fs/bfs/dir.c:   inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/btrfs/inode.c:       inode_init_owner(mnt_userns, inode, dir, mode);
>> fs/btrfs/tests/btrfs-tests.c:   inode_init_owner(&init_user_ns, inode, NULL, S_IFREG);
>> fs/ext2/ialloc.c:               inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/ext4/ialloc.c:               inode_init_owner(mnt_userns, inode, dir, mode);
>> fs/f2fs/namei.c:        inode_init_owner(mnt_userns, inode, dir, mode);
>> fs/hfsplus/inode.c:     inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/hugetlbfs/inode.c:           inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/jfs/jfs_inode.c:     inode_init_owner(&init_user_ns, inode, parent, mode);
>> fs/minix/bitmap.c:      inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/nilfs2/inode.c:      inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/ntfs3/inode.c:       inode_init_owner(mnt_userns, inode, dir, mode);
>> fs/ocfs2/dlmfs/dlmfs.c:         inode_init_owner(&init_user_ns, inode, NULL, mode);
>> fs/ocfs2/dlmfs/dlmfs.c: inode_init_owner(&init_user_ns, inode, parent, mode);
>> fs/ocfs2/namei.c:       inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/omfs/inode.c:        inode_init_owner(&init_user_ns, inode, NULL, mode);
>> fs/overlayfs/dir.c:     inode_init_owner(&init_user_ns, inode, dentry->d_parent->d_inode, mode);
>> fs/ramfs/inode.c:               inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/reiserfs/namei.c:    inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/sysv/ialloc.c:       inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/ubifs/dir.c: inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/udf/ialloc.c:        inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/ufs/ialloc.c:        inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/xfs/xfs_inode.c:             inode_init_owner(mnt_userns, inode, dir, mode);
>> fs/zonefs/super.c:      inode_init_owner(&init_user_ns, inode, parent, S_IFDIR | 0555);
>> kernel/bpf/inode.c:     inode_init_owner(&init_user_ns, inode, dir, mode);
>> mm/shmem.c:             inode_init_owner(&init_user_ns, inode, dir, mode);
>> "
>>
>> They are used in filesystem init new inode function and these init inode functions are used
>> by following operations:
>> mkdir
>> symlink
>> mknod
>> create
>> tmpfile
>> rename
>>
>> We don't care about mkdir because we don't strip SGID bit for directory except fs.xfs.irix_sgid_inherit.
>> But we even call it in do_mkdirat() since inode_sgid_strip() will skip directories anyway. This will
>> enforce the same ordering for all relevant operations and it will make the code more uniform and
>> easier to understand by using new helper prepare_mode().
>>
>> symlink and rename only use valid mode that doesn't have SGID bit.
>>
>> We have added inode_sgid_strip api for the remaining operations.
>>
>> In addition to the above six operations, four filesystems has a little difference
>> 1) btrfs has btrfs_create_subvol_root to create new inode but used non SGID bit mode and can ignore
>> 2) ocfs2 reflink function should add inode_sgid_strip api manually because we don't add it in vfs
>> 3) spufs which doesn't really go hrough the regular VFS callpath because it has separate system call
>> spu_create, but it t only allows the creation of directories and only allows bits in 0777 and can ignore
>> 4)bpf use vfs_mkobj in bpf_obj_do_pin with "S_IFREG | ((S_IRUSR | S_IWUSR)&  ~current_umask()) mode and
>> use bpf_mkobj_ops in bpf_iter_link_pin_kernel with S_IFREG | S_IRUSR; , so bpf is also not affected
>>
>> This patch also changed grpid behaviour for ext4/xfs because the mode passed to them may been
>> changed by inode_sgid_strip.
>>
>> Also as Christian Brauner said"
>> The patch itself is useful as it would move a security sensitive operation that is currently burried in
>> individual filesystems into the vfs layer. But it has a decent regression  potential since it might strip
>> filesystems that have so far relied on getting the S_ISGID bit with a mode argument. So this needs a lot
>> of testing and long exposure in -next for at least one full kernel cycle."
>>
>> Suggested-by: Dave Chinner<david@fromorbit.com>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>
> I think we're getting closer but please focus the patch series. This has
> morphed into an 8 patch series where 4 or 5 of these patches are fixes
> that a) I'm not sure are worth it or fix anything b) they are filesystem
> specific and can be independently upstreamed and c) have nothing to do
> with the core of this patch series.
>
> So I'd suggest you'd just make this about sgid stripping and then this
> doesn't have to be more than 3 maybe 4 patches, imho.
Ok, will focus on this sgid stripping.

Re: [f2fs-dev] [PATCH v4 7/8] fs: strip file's S_ISGID mode on vfs instead of on underlying filesystem

[xuyang2018.jy]

on 2022/4/19 22:09, Christian Brauner wrote:
> On Tue, Apr 19, 2022 at 07:47:13PM +0800, Yang Xu wrote:
>> Currently, vfs only passes mode argument to filesystem, then use inode_init_owner()
>> to strip S_ISGID. Some filesystem(ie ext4/btrfs) will call inode_init_owner
>> firstly, then posxi acl setup, but xfs uses the contrary order. It will affect
>> S_ISGID clear especially we filter S_IXGRP by umask or acl.
>>
>> Regardless of which filesystem is in use, failure to strip the SGID correctly is
>> considered a security failure that needs to be fixed. The current VFS infrastructure
>> requires the filesystem to do everything right and not step on any landmines to
>> strip the SGID bit, when in fact it can easily be done at the VFS and the filesystems
>> then don't even need to be aware that the SGID needs to be (or has been stripped) by
>> the operation the user asked to be done.
>>
>> Vfs has all the info it needs - it doesn't need the filesystems to do everything
>> correctly with the mode and ensuring that they order things like posix acl setup
>> functions correctly with inode_init_owner() to strip the SGID bit.
>>
>> Just strip the SGID bit at the VFS, and then the filesystems can't get it wrong.
>>
>> Also, the inode_sgid_strip() api should be used before IS_POSIXACL() because
>> this api may change mode.
>>
>> Only the following places use inode_init_owner
>> "
>> arch/powerpc/platforms/cell/spufs/inode.c:      inode_init_owner(&init_user_ns, inode, dir, mode | S_IFDIR);
>> arch/powerpc/platforms/cell/spufs/inode.c:      inode_init_owner(&init_user_ns, inode, dir, mode | S_IFDIR);
>> fs/9p/vfs_inode.c:      inode_init_owner(&init_user_ns, inode, NULL, mode);
>> fs/bfs/dir.c:   inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/btrfs/inode.c:       inode_init_owner(mnt_userns, inode, dir, mode);
>> fs/btrfs/tests/btrfs-tests.c:   inode_init_owner(&init_user_ns, inode, NULL, S_IFREG);
>> fs/ext2/ialloc.c:               inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/ext4/ialloc.c:               inode_init_owner(mnt_userns, inode, dir, mode);
>> fs/f2fs/namei.c:        inode_init_owner(mnt_userns, inode, dir, mode);
>> fs/hfsplus/inode.c:     inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/hugetlbfs/inode.c:           inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/jfs/jfs_inode.c:     inode_init_owner(&init_user_ns, inode, parent, mode);
>> fs/minix/bitmap.c:      inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/nilfs2/inode.c:      inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/ntfs3/inode.c:       inode_init_owner(mnt_userns, inode, dir, mode);
>> fs/ocfs2/dlmfs/dlmfs.c:         inode_init_owner(&init_user_ns, inode, NULL, mode);
>> fs/ocfs2/dlmfs/dlmfs.c: inode_init_owner(&init_user_ns, inode, parent, mode);
>> fs/ocfs2/namei.c:       inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/omfs/inode.c:        inode_init_owner(&init_user_ns, inode, NULL, mode);
>> fs/overlayfs/dir.c:     inode_init_owner(&init_user_ns, inode, dentry->d_parent->d_inode, mode);
>> fs/ramfs/inode.c:               inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/reiserfs/namei.c:    inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/sysv/ialloc.c:       inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/ubifs/dir.c: inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/udf/ialloc.c:        inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/ufs/ialloc.c:        inode_init_owner(&init_user_ns, inode, dir, mode);
>> fs/xfs/xfs_inode.c:             inode_init_owner(mnt_userns, inode, dir, mode);
>> fs/zonefs/super.c:      inode_init_owner(&init_user_ns, inode, parent, S_IFDIR | 0555);
>> kernel/bpf/inode.c:     inode_init_owner(&init_user_ns, inode, dir, mode);
>> mm/shmem.c:             inode_init_owner(&init_user_ns, inode, dir, mode);
>> "
>>
>> They are used in filesystem init new inode function and these init inode functions are used
>> by following operations:
>> mkdir
>> symlink
>> mknod
>> create
>> tmpfile
>> rename
>>
>> We don't care about mkdir because we don't strip SGID bit for directory except fs.xfs.irix_sgid_inherit.
>> But we even call it in do_mkdirat() since inode_sgid_strip() will skip directories anyway. This will
>> enforce the same ordering for all relevant operations and it will make the code more uniform and
>> easier to understand by using new helper prepare_mode().
>>
>> symlink and rename only use valid mode that doesn't have SGID bit.
>>
>> We have added inode_sgid_strip api for the remaining operations.
>>
>> In addition to the above six operations, four filesystems has a little difference
>> 1) btrfs has btrfs_create_subvol_root to create new inode but used non SGID bit mode and can ignore
>> 2) ocfs2 reflink function should add inode_sgid_strip api manually because we don't add it in vfs
>> 3) spufs which doesn't really go hrough the regular VFS callpath because it has separate system call
>> spu_create, but it t only allows the creation of directories and only allows bits in 0777 and can ignore
>> 4)bpf use vfs_mkobj in bpf_obj_do_pin with "S_IFREG | ((S_IRUSR | S_IWUSR)&  ~current_umask()) mode and
>> use bpf_mkobj_ops in bpf_iter_link_pin_kernel with S_IFREG | S_IRUSR; , so bpf is also not affected
>>
>> This patch also changed grpid behaviour for ext4/xfs because the mode passed to them may been
>> changed by inode_sgid_strip.
>>
>> Also as Christian Brauner said"
>> The patch itself is useful as it would move a security sensitive operation that is currently burried in
>> individual filesystems into the vfs layer. But it has a decent regression  potential since it might strip
>> filesystems that have so far relied on getting the S_ISGID bit with a mode argument. So this needs a lot
>> of testing and long exposure in -next for at least one full kernel cycle."
>>
>> Suggested-by: Dave Chinner<david@fromorbit.com>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>
> I think we're getting closer but please focus the patch series. This has
> morphed into an 8 patch series where 4 or 5 of these patches are fixes
> that a) I'm not sure are worth it or fix anything b) they are filesystem
> specific and can be independently upstreamed and c) have nothing to do
> with the core of this patch series.
>
> So I'd suggest you'd just make this about sgid stripping and then this
> doesn't have to be more than 3 maybe 4 patches, imho.
Ok, will focus on this sgid stripping.

_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[PATCH v4 8/8] ceph: Remove S_ISGID clear code in ceph_finish_async_create

[Yang Xu]

Since vfs has stripped S_ISGID in the previous patch, the calltrace
as below:

vfs:	lookup_open
	...
	  if (open_flag & O_CREAT) {
                if (open_flag & O_EXCL)
                        open_flag &= ~O_TRUNC;
                prepare_mode(mnt_userns, dir->d_inode, &mode);
	...
	   dir_inode->i_op->atomic_open

ceph:	ceph_atomic_open
	...
	      if (flags & O_CREAT)
            		ceph_finish_async_create

We have stripped sgid and umask in prepare mode, so remove this useless
code here.

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 fs/ceph/file.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index 6c9e837aa1d3..8e3b99853333 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -651,10 +651,6 @@ static int ceph_finish_async_create(struct inode *dir, struct dentry *dentry,
 		/* Directories always inherit the setgid bit. */
 		if (S_ISDIR(mode))
 			mode |= S_ISGID;
-		else if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP) &&
-			 !in_group_p(dir->i_gid) &&
-			 !capable_wrt_inode_uidgid(&init_user_ns, dir, CAP_FSETID))
-			mode &= ~S_ISGID;
 	} else {
 		in.gid = cpu_to_le32(from_kgid(&init_user_ns, current_fsgid()));
 	}
-- 
2.27.0

[f2fs-dev] [PATCH v4 8/8] ceph: Remove S_ISGID clear code in ceph_finish_async_create

[Yang Xu]

Since vfs has stripped S_ISGID in the previous patch, the calltrace
as below:

vfs:	lookup_open
	...
	  if (open_flag & O_CREAT) {
                if (open_flag & O_EXCL)
                        open_flag &= ~O_TRUNC;
                prepare_mode(mnt_userns, dir->d_inode, &mode);
	...
	   dir_inode->i_op->atomic_open

ceph:	ceph_atomic_open
	...
	      if (flags & O_CREAT)
            		ceph_finish_async_create

We have stripped sgid and umask in prepare mode, so remove this useless
code here.

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 fs/ceph/file.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index 6c9e837aa1d3..8e3b99853333 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -651,10 +651,6 @@ static int ceph_finish_async_create(struct inode *dir, struct dentry *dentry,
 		/* Directories always inherit the setgid bit. */
 		if (S_ISDIR(mode))
 			mode |= S_ISGID;
-		else if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP) &&
-			 !in_group_p(dir->i_gid) &&
-			 !capable_wrt_inode_uidgid(&init_user_ns, dir, CAP_FSETID))
-			mode &= ~S_ISGID;
 	} else {
 		in.gid = cpu_to_le32(from_kgid(&init_user_ns, current_fsgid()));
 	}
-- 
2.27.0



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH v4 1/8] fs: move sgid strip operation from inode_init_owner into inode_sgid_strip

[Christian Brauner]

On Tue, Apr 19, 2022 at 07:47:07PM +0800, Yang Xu wrote:
> This has no functional change. Just create and export inode_sgid_strip api for
> the subsequent patch. This function is used to strip S_ISGID mode when init
> a new inode.
> 
> Acked-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
>  fs/inode.c         | 22 ++++++++++++++++++----
>  include/linux/fs.h |  3 ++-
>  2 files changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 9d9b422504d1..3215e61a0021 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -2246,10 +2246,8 @@ void inode_init_owner(struct user_namespace *mnt_userns, struct inode *inode,
>  		/* Directories are special, and always inherit S_ISGID */
>  		if (S_ISDIR(mode))
>  			mode |= S_ISGID;
> -		else if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP) &&
> -			 !in_group_p(i_gid_into_mnt(mnt_userns, dir)) &&
> -			 !capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
> -			mode &= ~S_ISGID;
> +		else
> +			inode_sgid_strip(mnt_userns, dir, &mode);
>  	} else
>  		inode_fsgid_set(inode, mnt_userns);
>  	inode->i_mode = mode;
> @@ -2405,3 +2403,19 @@ struct timespec64 current_time(struct inode *inode)
>  	return timestamp_truncate(now, inode);
>  }
>  EXPORT_SYMBOL(current_time);
> +
> +void inode_sgid_strip(struct user_namespace *mnt_userns,
> +		      const struct inode *dir, umode_t *mode)
> +{

I think with Willy agreeing in an earlier version with me and you
needing to resend anyway I'd say have this return umode_t instead of
passing a pointer.

Re: [f2fs-dev] [PATCH v4 1/8] fs: move sgid strip operation from inode_init_owner into inode_sgid_strip

[Christian Brauner]

On Tue, Apr 19, 2022 at 07:47:07PM +0800, Yang Xu wrote:
> This has no functional change. Just create and export inode_sgid_strip api for
> the subsequent patch. This function is used to strip S_ISGID mode when init
> a new inode.
> 
> Acked-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
>  fs/inode.c         | 22 ++++++++++++++++++----
>  include/linux/fs.h |  3 ++-
>  2 files changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 9d9b422504d1..3215e61a0021 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -2246,10 +2246,8 @@ void inode_init_owner(struct user_namespace *mnt_userns, struct inode *inode,
>  		/* Directories are special, and always inherit S_ISGID */
>  		if (S_ISDIR(mode))
>  			mode |= S_ISGID;
> -		else if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP) &&
> -			 !in_group_p(i_gid_into_mnt(mnt_userns, dir)) &&
> -			 !capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
> -			mode &= ~S_ISGID;
> +		else
> +			inode_sgid_strip(mnt_userns, dir, &mode);
>  	} else
>  		inode_fsgid_set(inode, mnt_userns);
>  	inode->i_mode = mode;
> @@ -2405,3 +2403,19 @@ struct timespec64 current_time(struct inode *inode)
>  	return timestamp_truncate(now, inode);
>  }
>  EXPORT_SYMBOL(current_time);
> +
> +void inode_sgid_strip(struct user_namespace *mnt_userns,
> +		      const struct inode *dir, umode_t *mode)
> +{

I think with Willy agreeing in an earlier version with me and you
needing to resend anyway I'd say have this return umode_t instead of
passing a pointer.


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH v4 1/8] fs: move sgid strip operation from inode_init_owner into inode_sgid_strip

[xuyang2018.jy]

on 2022/4/19 22:05, Christian Brauner wrote:
> On Tue, Apr 19, 2022 at 07:47:07PM +0800, Yang Xu wrote:
>> This has no functional change. Just create and export inode_sgid_strip api for
>> the subsequent patch. This function is used to strip S_ISGID mode when init
>> a new inode.
>>
>> Acked-by: Christian Brauner (Microsoft)<brauner@kernel.org>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>>   fs/inode.c         | 22 ++++++++++++++++++----
>>   include/linux/fs.h |  3 ++-
>>   2 files changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/inode.c b/fs/inode.c
>> index 9d9b422504d1..3215e61a0021 100644
>> --- a/fs/inode.c
>> +++ b/fs/inode.c
>> @@ -2246,10 +2246,8 @@ void inode_init_owner(struct user_namespace *mnt_userns, struct inode *inode,
>>   		/* Directories are special, and always inherit S_ISGID */
>>   		if (S_ISDIR(mode))
>>   			mode |= S_ISGID;
>> -		else if ((mode&  (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)&&
>> -			 !in_group_p(i_gid_into_mnt(mnt_userns, dir))&&
>> -			 !capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
>> -			mode&= ~S_ISGID;
>> +		else
>> +			inode_sgid_strip(mnt_userns, dir,&mode);
>>   	} else
>>   		inode_fsgid_set(inode, mnt_userns);
>>   	inode->i_mode = mode;
>> @@ -2405,3 +2403,19 @@ struct timespec64 current_time(struct inode *inode)
>>   	return timestamp_truncate(now, inode);
>>   }
>>   EXPORT_SYMBOL(current_time);
>> +
>> +void inode_sgid_strip(struct user_namespace *mnt_userns,
>> +		      const struct inode *dir, umode_t *mode)
>> +{
>
> I think with Willy agreeing in an earlier version with me and you
> needing to resend anyway I'd say have this return umode_t instead of
> passing a pointer.

IMO, I am fine with your and Willy way. But I need a reason otherwise
I can't convince myself why not use mode pointer directly.

I have asked you and Willy before why return umode_t value is better, 
why not modify mode pointer directly? Since we have use mode as 
argument, why not modify mode pointer directly in function?

Also the function name(inode_sgid_strip and prepare_mode) has  expressed 
their function clearly.

Re: [f2fs-dev] [PATCH v4 1/8] fs: move sgid strip operation from inode_init_owner into inode_sgid_strip

[xuyang2018.jy]

on 2022/4/19 22:05, Christian Brauner wrote:
> On Tue, Apr 19, 2022 at 07:47:07PM +0800, Yang Xu wrote:
>> This has no functional change. Just create and export inode_sgid_strip api for
>> the subsequent patch. This function is used to strip S_ISGID mode when init
>> a new inode.
>>
>> Acked-by: Christian Brauner (Microsoft)<brauner@kernel.org>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>>   fs/inode.c         | 22 ++++++++++++++++++----
>>   include/linux/fs.h |  3 ++-
>>   2 files changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/inode.c b/fs/inode.c
>> index 9d9b422504d1..3215e61a0021 100644
>> --- a/fs/inode.c
>> +++ b/fs/inode.c
>> @@ -2246,10 +2246,8 @@ void inode_init_owner(struct user_namespace *mnt_userns, struct inode *inode,
>>   		/* Directories are special, and always inherit S_ISGID */
>>   		if (S_ISDIR(mode))
>>   			mode |= S_ISGID;
>> -		else if ((mode&  (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)&&
>> -			 !in_group_p(i_gid_into_mnt(mnt_userns, dir))&&
>> -			 !capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
>> -			mode&= ~S_ISGID;
>> +		else
>> +			inode_sgid_strip(mnt_userns, dir,&mode);
>>   	} else
>>   		inode_fsgid_set(inode, mnt_userns);
>>   	inode->i_mode = mode;
>> @@ -2405,3 +2403,19 @@ struct timespec64 current_time(struct inode *inode)
>>   	return timestamp_truncate(now, inode);
>>   }
>>   EXPORT_SYMBOL(current_time);
>> +
>> +void inode_sgid_strip(struct user_namespace *mnt_userns,
>> +		      const struct inode *dir, umode_t *mode)
>> +{
>
> I think with Willy agreeing in an earlier version with me and you
> needing to resend anyway I'd say have this return umode_t instead of
> passing a pointer.

IMO, I am fine with your and Willy way. But I need a reason otherwise
I can't convince myself why not use mode pointer directly.

I have asked you and Willy before why return umode_t value is better, 
why not modify mode pointer directly? Since we have use mode as 
argument, why not modify mode pointer directly in function?

Also the function name(inode_sgid_strip and prepare_mode) has  expressed 
their function clearly.



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH v4 1/8] fs: move sgid strip operation from inode_init_owner into inode_sgid_strip

[Dave Chinner]

On Wed, Apr 20, 2022 at 01:27:39AM +0000, xuyang2018.jy@fujitsu.com wrote:
> on 2022/4/19 22:05, Christian Brauner wrote:
> > On Tue, Apr 19, 2022 at 07:47:07PM +0800, Yang Xu wrote:
> >> This has no functional change. Just create and export inode_sgid_strip api for
> >> the subsequent patch. This function is used to strip S_ISGID mode when init
> >> a new inode.
> >>
> >> Acked-by: Christian Brauner (Microsoft)<brauner@kernel.org>
> >> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
> >> ---
> >>   fs/inode.c         | 22 ++++++++++++++++++----
> >>   include/linux/fs.h |  3 ++-
> >>   2 files changed, 20 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/fs/inode.c b/fs/inode.c
> >> index 9d9b422504d1..3215e61a0021 100644
> >> --- a/fs/inode.c
> >> +++ b/fs/inode.c
> >> @@ -2246,10 +2246,8 @@ void inode_init_owner(struct user_namespace *mnt_userns, struct inode *inode,
> >>   		/* Directories are special, and always inherit S_ISGID */
> >>   		if (S_ISDIR(mode))
> >>   			mode |= S_ISGID;
> >> -		else if ((mode&  (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)&&
> >> -			 !in_group_p(i_gid_into_mnt(mnt_userns, dir))&&
> >> -			 !capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
> >> -			mode&= ~S_ISGID;
> >> +		else
> >> +			inode_sgid_strip(mnt_userns, dir,&mode);
> >>   	} else
> >>   		inode_fsgid_set(inode, mnt_userns);
> >>   	inode->i_mode = mode;
> >> @@ -2405,3 +2403,19 @@ struct timespec64 current_time(struct inode *inode)
> >>   	return timestamp_truncate(now, inode);
> >>   }
> >>   EXPORT_SYMBOL(current_time);
> >> +
> >> +void inode_sgid_strip(struct user_namespace *mnt_userns,
> >> +		      const struct inode *dir, umode_t *mode)
> >> +{
> >
> > I think with Willy agreeing in an earlier version with me and you
> > needing to resend anyway I'd say have this return umode_t instead of
> > passing a pointer.
> 
> IMO, I am fine with your and Willy way. But I need a reason otherwise
> I can't convince myself why not use mode pointer directly.

You should listen to experienced developers like Willy and Christian
when they say "follow existing coding conventions".  Indeed, Darrick
has also mentioned he'd prefer it to return the new mode, and I'd
also prefer that it returns the new mode.

> I have asked you and Willy before why return umode_t value is better, 
> why not modify mode pointer directly? Since we have use mode as 
> argument, why not modify mode pointer directly in function?

If the function had mulitple return status (e.g. an error or a mode)
the convention is to pass the mode output variable by reference and
return the error status. But there is only one return value from
this function - the mode - and hence it should be returned in the
return value, not passed by reference.

Passing by reference unnecessarily makes the code more complex and
less mainatainable.  Code that returns a single value is easy to
understand, is more flexible in the way callers can use it and it's
simpler to maintain.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [f2fs-dev] [PATCH v4 1/8] fs: move sgid strip operation from inode_init_owner into inode_sgid_strip

[Dave Chinner]

On Wed, Apr 20, 2022 at 01:27:39AM +0000, xuyang2018.jy@fujitsu.com wrote:
> on 2022/4/19 22:05, Christian Brauner wrote:
> > On Tue, Apr 19, 2022 at 07:47:07PM +0800, Yang Xu wrote:
> >> This has no functional change. Just create and export inode_sgid_strip api for
> >> the subsequent patch. This function is used to strip S_ISGID mode when init
> >> a new inode.
> >>
> >> Acked-by: Christian Brauner (Microsoft)<brauner@kernel.org>
> >> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
> >> ---
> >>   fs/inode.c         | 22 ++++++++++++++++++----
> >>   include/linux/fs.h |  3 ++-
> >>   2 files changed, 20 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/fs/inode.c b/fs/inode.c
> >> index 9d9b422504d1..3215e61a0021 100644
> >> --- a/fs/inode.c
> >> +++ b/fs/inode.c
> >> @@ -2246,10 +2246,8 @@ void inode_init_owner(struct user_namespace *mnt_userns, struct inode *inode,
> >>   		/* Directories are special, and always inherit S_ISGID */
> >>   		if (S_ISDIR(mode))
> >>   			mode |= S_ISGID;
> >> -		else if ((mode&  (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)&&
> >> -			 !in_group_p(i_gid_into_mnt(mnt_userns, dir))&&
> >> -			 !capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
> >> -			mode&= ~S_ISGID;
> >> +		else
> >> +			inode_sgid_strip(mnt_userns, dir,&mode);
> >>   	} else
> >>   		inode_fsgid_set(inode, mnt_userns);
> >>   	inode->i_mode = mode;
> >> @@ -2405,3 +2403,19 @@ struct timespec64 current_time(struct inode *inode)
> >>   	return timestamp_truncate(now, inode);
> >>   }
> >>   EXPORT_SYMBOL(current_time);
> >> +
> >> +void inode_sgid_strip(struct user_namespace *mnt_userns,
> >> +		      const struct inode *dir, umode_t *mode)
> >> +{
> >
> > I think with Willy agreeing in an earlier version with me and you
> > needing to resend anyway I'd say have this return umode_t instead of
> > passing a pointer.
> 
> IMO, I am fine with your and Willy way. But I need a reason otherwise
> I can't convince myself why not use mode pointer directly.

You should listen to experienced developers like Willy and Christian
when they say "follow existing coding conventions".  Indeed, Darrick
has also mentioned he'd prefer it to return the new mode, and I'd
also prefer that it returns the new mode.

> I have asked you and Willy before why return umode_t value is better, 
> why not modify mode pointer directly? Since we have use mode as 
> argument, why not modify mode pointer directly in function?

If the function had mulitple return status (e.g. an error or a mode)
the convention is to pass the mode output variable by reference and
return the error status. But there is only one return value from
this function - the mode - and hence it should be returned in the
return value, not passed by reference.

Passing by reference unnecessarily makes the code more complex and
less mainatainable.  Code that returns a single value is easy to
understand, is more flexible in the way callers can use it and it's
simpler to maintain.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH v4 1/8] fs: move sgid strip operation from inode_init_owner into inode_sgid_strip

[xuyang2018.jy]

on 2022/4/21 5:52, Dave Chinner wrote:
> On Wed, Apr 20, 2022 at 01:27:39AM +0000, xuyang2018.jy@fujitsu.com wrote:
>> on 2022/4/19 22:05, Christian Brauner wrote:
>>> On Tue, Apr 19, 2022 at 07:47:07PM +0800, Yang Xu wrote:
>>>> This has no functional change. Just create and export inode_sgid_strip api for
>>>> the subsequent patch. This function is used to strip S_ISGID mode when init
>>>> a new inode.
>>>>
>>>> Acked-by: Christian Brauner (Microsoft)<brauner@kernel.org>
>>>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>>>> ---
>>>>    fs/inode.c         | 22 ++++++++++++++++++----
>>>>    include/linux/fs.h |  3 ++-
>>>>    2 files changed, 20 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/fs/inode.c b/fs/inode.c
>>>> index 9d9b422504d1..3215e61a0021 100644
>>>> --- a/fs/inode.c
>>>> +++ b/fs/inode.c
>>>> @@ -2246,10 +2246,8 @@ void inode_init_owner(struct user_namespace *mnt_userns, struct inode *inode,
>>>>    		/* Directories are special, and always inherit S_ISGID */
>>>>    		if (S_ISDIR(mode))
>>>>    			mode |= S_ISGID;
>>>> -		else if ((mode&   (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)&&
>>>> -			 !in_group_p(i_gid_into_mnt(mnt_userns, dir))&&
>>>> -			 !capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
>>>> -			mode&= ~S_ISGID;
>>>> +		else
>>>> +			inode_sgid_strip(mnt_userns, dir,&mode);
>>>>    	} else
>>>>    		inode_fsgid_set(inode, mnt_userns);
>>>>    	inode->i_mode = mode;
>>>> @@ -2405,3 +2403,19 @@ struct timespec64 current_time(struct inode *inode)
>>>>    	return timestamp_truncate(now, inode);
>>>>    }
>>>>    EXPORT_SYMBOL(current_time);
>>>> +
>>>> +void inode_sgid_strip(struct user_namespace *mnt_userns,
>>>> +		      const struct inode *dir, umode_t *mode)
>>>> +{
>>>
>>> I think with Willy agreeing in an earlier version with me and you
>>> needing to resend anyway I'd say have this return umode_t instead of
>>> passing a pointer.
>>
>> IMO, I am fine with your and Willy way. But I need a reason otherwise
>> I can't convince myself why not use mode pointer directly.
>
> You should listen to experienced developers like Willy and Christian
> when they say "follow existing coding conventions".  Indeed, Darrick
> has also mentioned he'd prefer it to return the new mode, and I'd
> also prefer that it returns the new mode.
OK. I just don't know  the "follow existing coding conventions" reason. 
Now, I know and understand.
>
>> I have asked you and Willy before why return umode_t value is better,
>> why not modify mode pointer directly? Since we have use mode as
>> argument, why not modify mode pointer directly in function?
>
> If the function had mulitple return status (e.g. an error or a mode)
> the convention is to pass the mode output variable by reference and
> return the error status. But there is only one return value from
> this function - the mode - and hence it should be returned in the
> return value, not passed by reference.
>
> Passing by reference unnecessarily makes the code more complex and
> less mainatainable.  Code that returns a single value is easy to
> understand, is more flexible in the way callers can use it and it's
> simpler to maintain.
OK,  it sounds reasonable and will use return value.

ps: Of course, I will remember this in my mind. Thanks for your replay.

Best Regards
Yang Xu

>
> Cheers,
>
> Dave.

Re: [f2fs-dev] [PATCH v4 1/8] fs: move sgid strip operation from inode_init_owner into inode_sgid_strip

[xuyang2018.jy]

on 2022/4/21 5:52, Dave Chinner wrote:
> On Wed, Apr 20, 2022 at 01:27:39AM +0000, xuyang2018.jy@fujitsu.com wrote:
>> on 2022/4/19 22:05, Christian Brauner wrote:
>>> On Tue, Apr 19, 2022 at 07:47:07PM +0800, Yang Xu wrote:
>>>> This has no functional change. Just create and export inode_sgid_strip api for
>>>> the subsequent patch. This function is used to strip S_ISGID mode when init
>>>> a new inode.
>>>>
>>>> Acked-by: Christian Brauner (Microsoft)<brauner@kernel.org>
>>>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>>>> ---
>>>>    fs/inode.c         | 22 ++++++++++++++++++----
>>>>    include/linux/fs.h |  3 ++-
>>>>    2 files changed, 20 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/fs/inode.c b/fs/inode.c
>>>> index 9d9b422504d1..3215e61a0021 100644
>>>> --- a/fs/inode.c
>>>> +++ b/fs/inode.c
>>>> @@ -2246,10 +2246,8 @@ void inode_init_owner(struct user_namespace *mnt_userns, struct inode *inode,
>>>>    		/* Directories are special, and always inherit S_ISGID */
>>>>    		if (S_ISDIR(mode))
>>>>    			mode |= S_ISGID;
>>>> -		else if ((mode&   (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)&&
>>>> -			 !in_group_p(i_gid_into_mnt(mnt_userns, dir))&&
>>>> -			 !capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
>>>> -			mode&= ~S_ISGID;
>>>> +		else
>>>> +			inode_sgid_strip(mnt_userns, dir,&mode);
>>>>    	} else
>>>>    		inode_fsgid_set(inode, mnt_userns);
>>>>    	inode->i_mode = mode;
>>>> @@ -2405,3 +2403,19 @@ struct timespec64 current_time(struct inode *inode)
>>>>    	return timestamp_truncate(now, inode);
>>>>    }
>>>>    EXPORT_SYMBOL(current_time);
>>>> +
>>>> +void inode_sgid_strip(struct user_namespace *mnt_userns,
>>>> +		      const struct inode *dir, umode_t *mode)
>>>> +{
>>>
>>> I think with Willy agreeing in an earlier version with me and you
>>> needing to resend anyway I'd say have this return umode_t instead of
>>> passing a pointer.
>>
>> IMO, I am fine with your and Willy way. But I need a reason otherwise
>> I can't convince myself why not use mode pointer directly.
>
> You should listen to experienced developers like Willy and Christian
> when they say "follow existing coding conventions".  Indeed, Darrick
> has also mentioned he'd prefer it to return the new mode, and I'd
> also prefer that it returns the new mode.
OK. I just don't know  the "follow existing coding conventions" reason. 
Now, I know and understand.
>
>> I have asked you and Willy before why return umode_t value is better,
>> why not modify mode pointer directly? Since we have use mode as
>> argument, why not modify mode pointer directly in function?
>
> If the function had mulitple return status (e.g. an error or a mode)
> the convention is to pass the mode output variable by reference and
> return the error status. But there is only one return value from
> this function - the mode - and hence it should be returned in the
> return value, not passed by reference.
>
> Passing by reference unnecessarily makes the code more complex and
> less mainatainable.  Code that returns a single value is easy to
> understand, is more flexible in the way callers can use it and it's
> simpler to maintain.
OK,  it sounds reasonable and will use return value.

ps: Of course, I will remember this in my mind. Thanks for your replay.

Best Regards
Yang Xu

>
> Cheers,
>
> Dave.

_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel