[RFC] xfs: fix up non-directory creation in SGID directories when umask S_IXGRP

[RFC] xfs: fix up non-directory creation in SGID directories when umask S_IXGRP

[Yang Xu]

Petr reported a problem that S_ISGID bit was not clean when testing ltp case
create09[1] by using umask(077).

It fails because xfs will call posix_acl_create before xfs_init_new_node calls
inode_init_owner, so S_IXGRP mode will be clear when enable CONFIG_XFS_POSIXACL
and doesn't set acl or default acl on dir, then inode_init_owner will not clear
S_ISGID bit.

The calltrace as below:

   will use  inode_init_owner(struct user_namespace *mnt_userns, structinode *inode)
[  296.760675]  xfs_init_new_inode+0x10e/0x6c0
[  296.760678]  xfs_create+0x401/0x610
   will use posix_acl_create(dir, &mode, &default_acl, &acl);
[  296.760681]  xfs_generic_create+0x123/0x2e0
[  296.760684]  ? _raw_spin_unlock+0x16/0x30
[  296.760687]  path_openat+0xfb8/0x1210
[  296.760689]  do_filp_open+0xb4/0x120
[  296.760691]  ? file_tty_write.isra.31+0x203/0x340
[  296.760697]  ? __check_object_size+0x150/0x170
[  296.760699]  do_sys_openat2+0x242/0x310
[  296.760702]  do_sys_open+0x4b/0x80
[  296.760704]  do_syscall_64+0x3a/0x80

Fix this is simple, we can call posix_acl_create after xfs_init_new_inode completed,
so inode_init_owner can clear S_ISGID bit correctly like ext4 or btrfs does.

But commit e6a688c33238 ("xfs: initialise attr fork on inode create") has created
attr fork in advance according to acl, so a better solution is that moving these
functions into xfs_init_new_inode.

[1]https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/creat/creat09.c
Reported-by: Petr Vorel <pvorel@suse.cz>
Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 fs/xfs/xfs_inode.c | 54 +++++++++++++++++++++++++++++++++++++--
 fs/xfs/xfs_iops.c  | 63 ++++------------------------------------------
 2 files changed, 57 insertions(+), 60 deletions(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 26227d26f274..8127b83b376c 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -4,6 +4,7 @@
  * All Rights Reserved.
  */
 #include <linux/iversion.h>
+#include <linux/posix_acl.h>
 
 #include "xfs.h"
 #include "xfs_fs.h"
@@ -36,6 +37,7 @@
 #include "xfs_reflink.h"
 #include "xfs_ag.h"
 #include "xfs_log_priv.h"
+#include "xfs_acl.h"
 
 struct kmem_cache *xfs_inode_cache;
 
@@ -781,6 +783,36 @@ xfs_inode_inherit_flags2(
 	}
 }
 
+/*
+ * Check to see if we are likely to need an extended attribute to be added to
+ * the inode we are about to allocate. This allows the attribute fork to be
+ * created during the inode allocation, reducing the number of transactions we
+ * need to do in this fast path.
+ *
+ * The security checks are optimistic, but not guaranteed. The two LSMs that
+ * require xattrs to be added here (selinux and smack) are also the only two
+ * LSMs that add a sb->s_security structure to the superblock. Hence if security
+ * is enabled and sb->s_security is set, we have a pretty good idea that we are
+ * going to be asked to add a security xattr immediately after allocating the
+ * xfs inode and instantiating the VFS inode.
+ */
+static inline bool
+xfs_create_need_xattr(
+	struct inode    *dir,
+	struct posix_acl *default_acl,
+	struct posix_acl *acl)
+{
+	if (acl)
+		return true;
+	if (default_acl)
+		return true;
+#if IS_ENABLED(CONFIG_SECURITY)
+	if (dir->i_sb->s_security)
+		return true;
+#endif
+	return false;
+}
+
 /*
  * Initialise a newly allocated inode and return the in-core inode to the
  * caller locked exclusively.
@@ -805,7 +837,7 @@ xfs_init_new_inode(
 	int			error;
 	struct timespec64	tv;
 	struct inode		*inode;
-
+	struct posix_acl 	*default_acl, *acl;
 	/*
 	 * Protect against obviously corrupt allocation btree records. Later
 	 * xfs_iget checks will catch re-allocation of other active in-memory
@@ -893,6 +925,9 @@ xfs_init_new_inode(
 		ASSERT(0);
 	}
 
+	error = posix_acl_create(dir, &inode->i_mode, &default_acl, &acl);
+	if (error)
+		return error;
 	/*
 	 * If we need to create attributes immediately after allocating the
 	 * inode, initialise an empty attribute fork right now. We use the
@@ -902,7 +937,9 @@ xfs_init_new_inode(
 	 * this saves us from needing to run a separate transaction to set the
 	 * fork offset in the immediate future.
 	 */
-	if (init_xattrs && xfs_has_attr(mp)) {
+	if (init_xattrs &&
+	    xfs_create_need_xattr(dir, default_acl, acl) &&
+	    xfs_has_attr(mp)) {
 		ip->i_forkoff = xfs_default_attroffset(ip) >> 3;
 		ip->i_afp = xfs_ifork_alloc(XFS_DINODE_FMT_EXTENTS, 0);
 	}
@@ -916,6 +953,19 @@ xfs_init_new_inode(
 	/* now that we have an i_mode we can setup the inode structure */
 	xfs_setup_inode(ip);
 
+#ifdef CONFIG_XFS_POSIX_ACL
+	if (default_acl) {
+		error = __xfs_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
+		if (error)
+			posix_acl_release(default_acl);
+	}
+	if (acl) {
+		error = __xfs_set_acl(inode, acl, ACL_TYPE_ACCESS);
+		if (error)
+			posix_acl_release(acl);
+	}
+#endif
+
 	*ipp = ip;
 	return 0;
 }
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index b34e8e4344a8..5f9fcb6e7cf8 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -127,37 +127,6 @@ xfs_cleanup_inode(
 	xfs_remove(XFS_I(dir), &teardown, XFS_I(inode));
 }
 
-/*
- * Check to see if we are likely to need an extended attribute to be added to
- * the inode we are about to allocate. This allows the attribute fork to be
- * created during the inode allocation, reducing the number of transactions we
- * need to do in this fast path.
- *
- * The security checks are optimistic, but not guaranteed. The two LSMs that
- * require xattrs to be added here (selinux and smack) are also the only two
- * LSMs that add a sb->s_security structure to the superblock. Hence if security
- * is enabled and sb->s_security is set, we have a pretty good idea that we are
- * going to be asked to add a security xattr immediately after allocating the
- * xfs inode and instantiating the VFS inode.
- */
-static inline bool
-xfs_create_need_xattr(
-	struct inode	*dir,
-	struct posix_acl *default_acl,
-	struct posix_acl *acl)
-{
-	if (acl)
-		return true;
-	if (default_acl)
-		return true;
-#if IS_ENABLED(CONFIG_SECURITY)
-	if (dir->i_sb->s_security)
-		return true;
-#endif
-	return false;
-}
-
-
 STATIC int
 xfs_generic_create(
 	struct user_namespace	*mnt_userns,
@@ -169,7 +138,6 @@ xfs_generic_create(
 {
 	struct inode	*inode;
 	struct xfs_inode *ip = NULL;
-	struct posix_acl *default_acl, *acl;
 	struct xfs_name	name;
 	int		error;
 
@@ -184,24 +152,19 @@ xfs_generic_create(
 		rdev = 0;
 	}
 
-	error = posix_acl_create(dir, &mode, &default_acl, &acl);
-	if (error)
-		return error;
-
 	/* Verify mode is valid also for tmpfile case */
 	error = xfs_dentry_mode_to_name(&name, dentry, mode);
 	if (unlikely(error))
-		goto out_free_acl;
+		return error;
 
 	if (!tmpfile) {
 		error = xfs_create(mnt_userns, XFS_I(dir), &name, mode, rdev,
-				xfs_create_need_xattr(dir, default_acl, acl),
-				&ip);
+				true, &ip);
 	} else {
 		error = xfs_create_tmpfile(mnt_userns, XFS_I(dir), mode, &ip);
 	}
 	if (unlikely(error))
-		goto out_free_acl;
+		return error;
 
 	inode = VFS_I(ip);
 
@@ -209,19 +172,6 @@ xfs_generic_create(
 	if (unlikely(error))
 		goto out_cleanup_inode;
 
-#ifdef CONFIG_XFS_POSIX_ACL
-	if (default_acl) {
-		error = __xfs_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
-		if (error)
-			goto out_cleanup_inode;
-	}
-	if (acl) {
-		error = __xfs_set_acl(inode, acl, ACL_TYPE_ACCESS);
-		if (error)
-			goto out_cleanup_inode;
-	}
-#endif
-
 	xfs_setup_iops(ip);
 
 	if (tmpfile) {
@@ -240,17 +190,14 @@ xfs_generic_create(
 
 	xfs_finish_inode_setup(ip);
 
- out_free_acl:
-	posix_acl_release(default_acl);
-	posix_acl_release(acl);
-	return error;
+	return 0;
 
  out_cleanup_inode:
 	xfs_finish_inode_setup(ip);
 	if (!tmpfile)
 		xfs_cleanup_inode(dir, inode, dentry);
 	xfs_irele(ip);
-	goto out_free_acl;
+	return error;
 }
 
 STATIC int
-- 
2.27.0

Re: [RFC] xfs: fix up non-directory creation in SGID directories when umask S_IXGRP

[xuyang2018.jy]

Since this patch miss posix_acl_release when succeed and break old logic
that only call from xfs_generic_create will call posix_acl_create.

I will resend a v1 patch. please ignore this patch

Best Regards
Yang Xu
> Petr reported a problem that S_ISGID bit was not clean when testing ltp case
> create09[1] by using umask(077).
> 
> It fails because xfs will call posix_acl_create before xfs_init_new_node calls
> inode_init_owner, so S_IXGRP mode will be clear when enable CONFIG_XFS_POSIXACL
> and doesn't set acl or default acl on dir, then inode_init_owner will not clear
> S_ISGID bit.
> 
> The calltrace as below:
> 
>     will use  inode_init_owner(struct user_namespace *mnt_userns, structinode *inode)
> [  296.760675]  xfs_init_new_inode+0x10e/0x6c0
> [  296.760678]  xfs_create+0x401/0x610
>     will use posix_acl_create(dir,&mode,&default_acl,&acl);
> [  296.760681]  xfs_generic_create+0x123/0x2e0
> [  296.760684]  ? _raw_spin_unlock+0x16/0x30
> [  296.760687]  path_openat+0xfb8/0x1210
> [  296.760689]  do_filp_open+0xb4/0x120
> [  296.760691]  ? file_tty_write.isra.31+0x203/0x340
> [  296.760697]  ? __check_object_size+0x150/0x170
> [  296.760699]  do_sys_openat2+0x242/0x310
> [  296.760702]  do_sys_open+0x4b/0x80
> [  296.760704]  do_syscall_64+0x3a/0x80
> 
> Fix this is simple, we can call posix_acl_create after xfs_init_new_inode completed,
> so inode_init_owner can clear S_ISGID bit correctly like ext4 or btrfs does.
> 
> But commit e6a688c33238 ("xfs: initialise attr fork on inode create") has created
> attr fork in advance according to acl, so a better solution is that moving these
> functions into xfs_init_new_inode.
> 
> [1]https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/creat/creat09.c
> Reported-by: Petr Vorel<pvorel@suse.cz>
> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
> ---
>   fs/xfs/xfs_inode.c | 54 +++++++++++++++++++++++++++++++++++++--
>   fs/xfs/xfs_iops.c  | 63 ++++------------------------------------------
>   2 files changed, 57 insertions(+), 60 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 26227d26f274..8127b83b376c 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -4,6 +4,7 @@
>    * All Rights Reserved.
>    */
>   #include<linux/iversion.h>
> +#include<linux/posix_acl.h>
> 
>   #include "xfs.h"
>   #include "xfs_fs.h"
> @@ -36,6 +37,7 @@
>   #include "xfs_reflink.h"
>   #include "xfs_ag.h"
>   #include "xfs_log_priv.h"
> +#include "xfs_acl.h"
> 
>   struct kmem_cache *xfs_inode_cache;
> 
> @@ -781,6 +783,36 @@ xfs_inode_inherit_flags2(
>   	}
>   }
> 
> +/*
> + * Check to see if we are likely to need an extended attribute to be added to
> + * the inode we are about to allocate. This allows the attribute fork to be
> + * created during the inode allocation, reducing the number of transactions we
> + * need to do in this fast path.
> + *
> + * The security checks are optimistic, but not guaranteed. The two LSMs that
> + * require xattrs to be added here (selinux and smack) are also the only two
> + * LSMs that add a sb->s_security structure to the superblock. Hence if security
> + * is enabled and sb->s_security is set, we have a pretty good idea that we are
> + * going to be asked to add a security xattr immediately after allocating the
> + * xfs inode and instantiating the VFS inode.
> + */
> +static inline bool
> +xfs_create_need_xattr(
> +	struct inode    *dir,
> +	struct posix_acl *default_acl,
> +	struct posix_acl *acl)
> +{
> +	if (acl)
> +		return true;
> +	if (default_acl)
> +		return true;
> +#if IS_ENABLED(CONFIG_SECURITY)
> +	if (dir->i_sb->s_security)
> +		return true;
> +#endif
> +	return false;
> +}
> +
>   /*
>    * Initialise a newly allocated inode and return the in-core inode to the
>    * caller locked exclusively.
> @@ -805,7 +837,7 @@ xfs_init_new_inode(
>   	int			error;
>   	struct timespec64	tv;
>   	struct inode		*inode;
> -
> +	struct posix_acl 	*default_acl, *acl;
>   	/*
>   	 * Protect against obviously corrupt allocation btree records. Later
>   	 * xfs_iget checks will catch re-allocation of other active in-memory
> @@ -893,6 +925,9 @@ xfs_init_new_inode(
>   		ASSERT(0);
>   	}
> 
> +	error = posix_acl_create(dir,&inode->i_mode,&default_acl,&acl);
> +	if (error)
> +		return error;
>   	/*
>   	 * If we need to create attributes immediately after allocating the
>   	 * inode, initialise an empty attribute fork right now. We use the
> @@ -902,7 +937,9 @@ xfs_init_new_inode(
>   	 * this saves us from needing to run a separate transaction to set the
>   	 * fork offset in the immediate future.
>   	 */
> -	if (init_xattrs&&  xfs_has_attr(mp)) {
> +	if (init_xattrs&&
> +	    xfs_create_need_xattr(dir, default_acl, acl)&&
> +	    xfs_has_attr(mp)) {
>   		ip->i_forkoff = xfs_default_attroffset(ip)>>  3;
>   		ip->i_afp = xfs_ifork_alloc(XFS_DINODE_FMT_EXTENTS, 0);
>   	}
> @@ -916,6 +953,19 @@ xfs_init_new_inode(
>   	/* now that we have an i_mode we can setup the inode structure */
>   	xfs_setup_inode(ip);
> 
> +#ifdef CONFIG_XFS_POSIX_ACL
> +	if (default_acl) {
> +		error = __xfs_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
> +		if (error)
> +			posix_acl_release(default_acl);
> +	}
> +	if (acl) {
> +		error = __xfs_set_acl(inode, acl, ACL_TYPE_ACCESS);
> +		if (error)
> +			posix_acl_release(acl);
> +	}
> +#endif
> +
>   	*ipp = ip;
>   	return 0;
>   }
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index b34e8e4344a8..5f9fcb6e7cf8 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -127,37 +127,6 @@ xfs_cleanup_inode(
>   	xfs_remove(XFS_I(dir),&teardown, XFS_I(inode));
>   }
> 
> -/*
> - * Check to see if we are likely to need an extended attribute to be added to
> - * the inode we are about to allocate. This allows the attribute fork to be
> - * created during the inode allocation, reducing the number of transactions we
> - * need to do in this fast path.
> - *
> - * The security checks are optimistic, but not guaranteed. The two LSMs that
> - * require xattrs to be added here (selinux and smack) are also the only two
> - * LSMs that add a sb->s_security structure to the superblock. Hence if security
> - * is enabled and sb->s_security is set, we have a pretty good idea that we are
> - * going to be asked to add a security xattr immediately after allocating the
> - * xfs inode and instantiating the VFS inode.
> - */
> -static inline bool
> -xfs_create_need_xattr(
> -	struct inode	*dir,
> -	struct posix_acl *default_acl,
> -	struct posix_acl *acl)
> -{
> -	if (acl)
> -		return true;
> -	if (default_acl)
> -		return true;
> -#if IS_ENABLED(CONFIG_SECURITY)
> -	if (dir->i_sb->s_security)
> -		return true;
> -#endif
> -	return false;
> -}
> -
> -
>   STATIC int
>   xfs_generic_create(
>   	struct user_namespace	*mnt_userns,
> @@ -169,7 +138,6 @@ xfs_generic_create(
>   {
>   	struct inode	*inode;
>   	struct xfs_inode *ip = NULL;
> -	struct posix_acl *default_acl, *acl;
>   	struct xfs_name	name;
>   	int		error;
> 
> @@ -184,24 +152,19 @@ xfs_generic_create(
>   		rdev = 0;
>   	}
> 
> -	error = posix_acl_create(dir,&mode,&default_acl,&acl);
> -	if (error)
> -		return error;
> -
>   	/* Verify mode is valid also for tmpfile case */
>   	error = xfs_dentry_mode_to_name(&name, dentry, mode);
>   	if (unlikely(error))
> -		goto out_free_acl;
> +		return error;
> 
>   	if (!tmpfile) {
>   		error = xfs_create(mnt_userns, XFS_I(dir),&name, mode, rdev,
> -				xfs_create_need_xattr(dir, default_acl, acl),
> -				&ip);
> +				true,&ip);
>   	} else {
>   		error = xfs_create_tmpfile(mnt_userns, XFS_I(dir), mode,&ip);
>   	}
>   	if (unlikely(error))
> -		goto out_free_acl;
> +		return error;
> 
>   	inode = VFS_I(ip);
> 
> @@ -209,19 +172,6 @@ xfs_generic_create(
>   	if (unlikely(error))
>   		goto out_cleanup_inode;
> 
> -#ifdef CONFIG_XFS_POSIX_ACL
> -	if (default_acl) {
> -		error = __xfs_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
> -		if (error)
> -			goto out_cleanup_inode;
> -	}
> -	if (acl) {
> -		error = __xfs_set_acl(inode, acl, ACL_TYPE_ACCESS);
> -		if (error)
> -			goto out_cleanup_inode;
> -	}
> -#endif
> -
>   	xfs_setup_iops(ip);
> 
>   	if (tmpfile) {
> @@ -240,17 +190,14 @@ xfs_generic_create(
> 
>   	xfs_finish_inode_setup(ip);
> 
> - out_free_acl:
> -	posix_acl_release(default_acl);
> -	posix_acl_release(acl);
> -	return error;
> +	return 0;
> 
>    out_cleanup_inode:
>   	xfs_finish_inode_setup(ip);
>   	if (!tmpfile)
>   		xfs_cleanup_inode(dir, inode, dentry);
>   	xfs_irele(ip);
> -	goto out_free_acl;
> +	return error;
>   }
> 
>   STATIC int