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