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@xxxxxxx>
> Signed-off-by: Yang Xu<xuyang2018.jy@xxxxxxxxxxx>
> ---
> 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
