* [PATCH v2 0/2] xfs: tmpfile fixes for inode security/acl @ 2014-04-09 19:21 Brian Foster 2014-04-09 19:21 ` [PATCH v2 1/2] xfs: fix tmpfile/selinux deadlock and initialize security/acl Brian Foster 2014-04-09 19:21 ` [PATCH v2 2/2] xfs: fold xfs_create_tmpfile() into xfs_create() Brian Foster 0 siblings, 2 replies; 14+ messages in thread From: Brian Foster @ 2014-04-09 19:21 UTC (permalink / raw) To: xfs Hi all, Here's a v2 of the tmpfile and selinux deadlock patch. Since we also probably want to create the default acl, I created a generic helper out of xfs_vn_mknod(). Once I got that far, I noticed xfs_create_tmpfile() looked quite similar to xfs_create(). Patch 2 folds the former into the latter and eliminates the duplicate code. Patch 2 is optional with regard to addressing the selinux issue. Lightly tested... thoughts, reviews, flames appreciated. Brian v2: - Added a generic create helper to inherit the security/acl init. code in the tmpfile path. - Added patch 2 to fold xfs_create_tmpfile() into xfs_create(). Brian Foster (2): xfs: fix tmpfile/selinux deadlock and initialize security/acl xfs: fold xfs_create_tmpfile() into xfs_create() fs/xfs/xfs_inode.c | 184 ++++++++++++++--------------------------------------- fs/xfs/xfs_inode.h | 2 +- fs/xfs/xfs_iops.c | 43 +++++++++---- fs/xfs/xfs_trace.h | 7 +- 4 files changed, 81 insertions(+), 155 deletions(-) -- 1.8.3.1 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 1/2] xfs: fix tmpfile/selinux deadlock and initialize security/acl 2014-04-09 19:21 [PATCH v2 0/2] xfs: tmpfile fixes for inode security/acl Brian Foster @ 2014-04-09 19:21 ` Brian Foster 2014-04-10 10:24 ` Christoph Hellwig 2014-04-30 12:02 ` Christoph Hellwig 2014-04-09 19:21 ` [PATCH v2 2/2] xfs: fold xfs_create_tmpfile() into xfs_create() Brian Foster 1 sibling, 2 replies; 14+ messages in thread From: Brian Foster @ 2014-04-09 19:21 UTC (permalink / raw) To: xfs xfstests generic/004 reproduces an ilock deadlock using the tmpfile interface when selinux is enabled. This occurs because xfs_create_tmpfile() takes the ilock and then calls d_tmpfile(). The latter eventually calls into xfs_xattr_get() which attempts to get the lock again. E.g.: xfs_io D ffffffff81c134c0 4096 3561 3560 0x00000080 ffff8801176a1a68 0000000000000046 ffff8800b401b540 ffff8801176a1fd8 00000000001d5800 00000000001d5800 ffff8800b401b540 ffff8800b401b540 ffff8800b73a6bd0 fffffffeffffffff ffff8800b73a6bd8 ffff8800b5ddb480 Call Trace: [<ffffffff8177f969>] schedule+0x29/0x70 [<ffffffff81783a65>] rwsem_down_read_failed+0xc5/0x120 [<ffffffffa05aa97f>] ? xfs_ilock_attr_map_shared+0x1f/0x50 [xfs] [<ffffffff813b3434>] call_rwsem_down_read_failed+0x14/0x30 [<ffffffff810ed179>] ? down_read_nested+0x89/0xa0 [<ffffffffa05aa7f2>] ? xfs_ilock+0x122/0x250 [xfs] [<ffffffffa05aa7f2>] xfs_ilock+0x122/0x250 [xfs] [<ffffffffa05aa97f>] xfs_ilock_attr_map_shared+0x1f/0x50 [xfs] [<ffffffffa05701d0>] xfs_attr_get+0x90/0xe0 [xfs] [<ffffffffa0565e07>] xfs_xattr_get+0x37/0x50 [xfs] [<ffffffff8124842f>] generic_getxattr+0x4f/0x70 [<ffffffff8133fd9e>] inode_doinit_with_dentry+0x1ae/0x650 [<ffffffff81340e0c>] selinux_d_instantiate+0x1c/0x20 [<ffffffff813351bb>] security_d_instantiate+0x1b/0x30 [<ffffffff81237db0>] d_instantiate+0x50/0x70 [<ffffffff81237e85>] d_tmpfile+0xb5/0xc0 [<ffffffffa05add02>] xfs_create_tmpfile+0x362/0x410 [xfs] [<ffffffffa0559ac8>] xfs_vn_tmpfile+0x18/0x20 [xfs] [<ffffffff81230388>] path_openat+0x228/0x6a0 [<ffffffff810230f9>] ? sched_clock+0x9/0x10 [<ffffffff8105a427>] ? kvm_clock_read+0x27/0x40 [<ffffffff8124054f>] ? __alloc_fd+0xaf/0x1f0 [<ffffffff8123101a>] do_filp_open+0x3a/0x90 [<ffffffff817845e7>] ? _raw_spin_unlock+0x27/0x40 [<ffffffff8124054f>] ? __alloc_fd+0xaf/0x1f0 [<ffffffff8121e3ce>] do_sys_open+0x12e/0x210 [<ffffffff8121e4ce>] SyS_open+0x1e/0x20 [<ffffffff8178eda9>] system_call_fastpath+0x16/0x1b xfs_vn_tmpfile() also fails to initialize security or default acls on the newly created inode. The functionality missing from the tmpfile() handler is mostly covered by xfs_vn_mknod() but it currently has no means to determine whether a file is unnamed. Therefore, convert xfs_vn_mknod() to xfs_generic_create() and add a parameter to trigger the tmpfile-specific file creation and dentry mapping calls. The d_tmpfile() call is removed from xfs_create_tmpfile() and pulled up into the new handler to address the deadlock. E.g., xfs_create_tmpfile() has committed the create transaction and unlocked the inode prior to mapping the inode to the dentry. Signed-off-by: Brian Foster <bfoster@redhat.com> --- fs/xfs/xfs_inode.c | 5 +++-- fs/xfs/xfs_inode.h | 2 +- fs/xfs/xfs_iops.c | 41 +++++++++++++++++++++++++++++------------ 3 files changed, 33 insertions(+), 15 deletions(-) diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index 5e7a38f..768087b 100644 --- a/fs/xfs/xfs_inode.c +++ b/fs/xfs/xfs_inode.c @@ -1334,7 +1334,8 @@ int xfs_create_tmpfile( struct xfs_inode *dp, struct dentry *dentry, - umode_t mode) + umode_t mode, + struct xfs_inode **ipp) { struct xfs_mount *mp = dp->i_mount; struct xfs_inode *ip = NULL; @@ -1402,7 +1403,6 @@ xfs_create_tmpfile( xfs_qm_vop_create_dqattach(tp, ip, udqp, gdqp, pdqp); ip->i_d.di_nlink--; - d_tmpfile(dentry, VFS_I(ip)); error = xfs_iunlink(tp, ip); if (error) goto out_trans_abort; @@ -1415,6 +1415,7 @@ xfs_create_tmpfile( xfs_qm_dqrele(gdqp); xfs_qm_dqrele(pdqp); + *ipp = ip; return 0; out_trans_abort: diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h index 396cc1f..f2fcde5 100644 --- a/fs/xfs/xfs_inode.h +++ b/fs/xfs/xfs_inode.h @@ -334,7 +334,7 @@ int xfs_lookup(struct xfs_inode *dp, struct xfs_name *name, int xfs_create(struct xfs_inode *dp, struct xfs_name *name, umode_t mode, xfs_dev_t rdev, struct xfs_inode **ipp); int xfs_create_tmpfile(struct xfs_inode *dp, struct dentry *dentry, - umode_t mode); + umode_t mode, struct xfs_inode **ipp); int xfs_remove(struct xfs_inode *dp, struct xfs_name *name, struct xfs_inode *ip); int xfs_link(struct xfs_inode *tdp, struct xfs_inode *sip, diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c index 89b07e4..301ecbf 100644 --- a/fs/xfs/xfs_iops.c +++ b/fs/xfs/xfs_iops.c @@ -124,15 +124,15 @@ xfs_cleanup_inode( xfs_dentry_to_name(&teardown, dentry, 0); xfs_remove(XFS_I(dir), &teardown, XFS_I(inode)); - iput(inode); } STATIC int -xfs_vn_mknod( +xfs_generic_create( struct inode *dir, struct dentry *dentry, umode_t mode, - dev_t rdev) + dev_t rdev, + bool tmpfile) /* unnamed file */ { struct inode *inode; struct xfs_inode *ip = NULL; @@ -156,8 +156,12 @@ xfs_vn_mknod( if (error) return error; - xfs_dentry_to_name(&name, dentry, mode); - error = xfs_create(XFS_I(dir), &name, mode, rdev, &ip); + if (!tmpfile) { + xfs_dentry_to_name(&name, dentry, mode); + error = xfs_create(XFS_I(dir), &name, mode, rdev, &ip); + } else { + error = xfs_create_tmpfile(XFS_I(dir), dentry, mode, &ip); + } if (unlikely(error)) goto out_free_acl; @@ -180,7 +184,11 @@ xfs_vn_mknod( } #endif - d_instantiate(dentry, inode); + if (tmpfile) + d_tmpfile(dentry, inode); + else + d_instantiate(dentry, inode); + out_free_acl: if (default_acl) posix_acl_release(default_acl); @@ -189,11 +197,23 @@ xfs_vn_mknod( return -error; out_cleanup_inode: - xfs_cleanup_inode(dir, inode, dentry); + if (!tmpfile) + xfs_cleanup_inode(dir, inode, dentry); + iput(inode); goto out_free_acl; } STATIC int +xfs_vn_mknod( + struct inode *dir, + struct dentry *dentry, + umode_t mode, + dev_t rdev) +{ + return xfs_generic_create(dir, dentry, mode, rdev, false); +} + +STATIC int xfs_vn_create( struct inode *dir, struct dentry *dentry, @@ -353,6 +373,7 @@ xfs_vn_symlink( out_cleanup_inode: xfs_cleanup_inode(dir, inode, dentry); + iput(inode); out: return -error; } @@ -1053,11 +1074,7 @@ xfs_vn_tmpfile( struct dentry *dentry, umode_t mode) { - int error; - - error = xfs_create_tmpfile(XFS_I(dir), dentry, mode); - - return -error; + return xfs_generic_create(dir, dentry, mode, 0, true); } static const struct inode_operations xfs_inode_operations = { -- 1.8.3.1 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] xfs: fix tmpfile/selinux deadlock and initialize security/acl 2014-04-09 19:21 ` [PATCH v2 1/2] xfs: fix tmpfile/selinux deadlock and initialize security/acl Brian Foster @ 2014-04-10 10:24 ` Christoph Hellwig 2014-04-10 12:19 ` Brian Foster 2014-04-30 12:02 ` Christoph Hellwig 1 sibling, 1 reply; 14+ messages in thread From: Christoph Hellwig @ 2014-04-10 10:24 UTC (permalink / raw) To: Brian Foster; +Cc: xfs On Wed, Apr 09, 2014 at 03:21:50PM -0400, Brian Foster wrote: > xfs_vn_tmpfile() also fails to initialize security or default acls on > the newly created inode. Which it doesn't have to, as it is never available in the filesystem namespace. > The d_tmpfile() call is removed from xfs_create_tmpfile() and pulled up > into the new handler to address the deadlock. E.g., xfs_create_tmpfile() > has committed the create transaction and unlocked the inode prior to > mapping the inode to the dentry. This part of the patch looks sane, although the window where the XFS inode and VFS inode i_nlink are out of sync worries me a little. I don't think the other refactoring belongs into the same patch. If we decide that we want it please avoid the useless ACL inheritance for tmpfiles. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] xfs: fix tmpfile/selinux deadlock and initialize security/acl 2014-04-10 10:24 ` Christoph Hellwig @ 2014-04-10 12:19 ` Brian Foster 2014-04-10 12:29 ` Christoph Hellwig 0 siblings, 1 reply; 14+ messages in thread From: Brian Foster @ 2014-04-10 12:19 UTC (permalink / raw) To: Christoph Hellwig; +Cc: xfs On Thu, Apr 10, 2014 at 03:24:21AM -0700, Christoph Hellwig wrote: > On Wed, Apr 09, 2014 at 03:21:50PM -0400, Brian Foster wrote: > > xfs_vn_tmpfile() also fails to initialize security or default acls on > > the newly created inode. > > Which it doesn't have to, as it is never available in the filesystem > namespace. > Are you saying it doesn't have to initialize security or the default acl, or both? The intent here was to have the case covered where the inode happens to be linked back into the namespace since we don't do this work in the link path. > > The d_tmpfile() call is removed from xfs_create_tmpfile() and pulled up > > into the new handler to address the deadlock. E.g., xfs_create_tmpfile() > > has committed the create transaction and unlocked the inode prior to > > mapping the inode to the dentry. > > This part of the patch looks sane, although the window where the XFS > inode and VFS inode i_nlink are out of sync worries me a little. > > I don't think the other refactoring belongs into the same patch. > > If we decide that we want it please avoid the useless ACL inheritance > for tmpfiles. The bulk of the refactoring was with the idea that the inode setup for the tmpfile case is generally equivalent for the traditional create case. The original version was posted here: http://oss.sgi.com/archives/xfs/2014-04/msg00149.html ... and it just fixes the deadlock and adds the security initialization. I suppose I could still break that out into multiple patches, but that aside, is that behavior preferred? Brian _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] xfs: fix tmpfile/selinux deadlock and initialize security/acl 2014-04-10 12:19 ` Brian Foster @ 2014-04-10 12:29 ` Christoph Hellwig 2014-04-15 17:52 ` Christoph Hellwig 0 siblings, 1 reply; 14+ messages in thread From: Christoph Hellwig @ 2014-04-10 12:29 UTC (permalink / raw) To: Brian Foster Cc: linux-man, Andreas Gruenbacher, xfs, linux-security-module, Al Viro, linux-fsdevel, linux-ext4 On Thu, Apr 10, 2014 at 08:19:48AM -0400, Brian Foster wrote: > Are you saying it doesn't have to initialize security or the default > acl, or both? The ACLs for sure. LSM do run-time access decisions, so they will probably rely on the security data being initialized. Given that O_TMPFILE files aren't publicly available I'm not sure there's a point in them doing that, though. LSMs are also affected by the lack of a proper parent I'll discuss for ACLs below. > The intent here was to have the case covered where the inode happens to > be linked back into the namespace since we don't do this work in the > link path. That's an interesting one. O_TMFILE files don't have a real parent to inherit ACLs from, the pathname passed in just needs to point to a directory to find the filesystem to create the tmpfile in. On the other hand it seem like the extN implementations do inherity the ACL in this case. The link into the namespace is irrelavant here as ACL inheritance only happens on initial create, not at link time. I also think we'll absolutely need a test case for ACLs+tmpfile to make sure all filesystems handle it the same way. > The bulk of the refactoring was with the idea that the inode setup for > the tmpfile case is generally equivalent for the traditional create > case. The original version was posted here: > > http://oss.sgi.com/archives/xfs/2014-04/msg00149.html > > ... and it just fixes the deadlock and adds the security initialization. > I suppose I could still break that out into multiple patches, but that > aside, is that behavior preferred? I think just fixing the deadlock and initializing the security is enough for the first pass. If you want to do the refactoring on top send it as a second series on top of the actual fixes. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] xfs: fix tmpfile/selinux deadlock and initialize security/acl 2014-04-10 12:29 ` Christoph Hellwig @ 2014-04-15 17:52 ` Christoph Hellwig 2014-04-15 19:31 ` Andreas Gruenbacher 0 siblings, 1 reply; 14+ messages in thread From: Christoph Hellwig @ 2014-04-15 17:52 UTC (permalink / raw) To: Brian Foster Cc: linux-man, Andreas Gruenbacher, xfs, linux-security-module, Al Viro, linux-fsdevel, linux-ext4 So any opinions from other fs / security people on how O_TMPFILE files should behave for ACL inheritance / labeling? On Thu, Apr 10, 2014 at 05:29:44AM -0700, Christoph Hellwig wrote: > On Thu, Apr 10, 2014 at 08:19:48AM -0400, Brian Foster wrote: > > Are you saying it doesn't have to initialize security or the default > > acl, or both? > > The ACLs for sure. LSM do run-time access decisions, so they will > probably rely on the security data being initialized. Given that > O_TMPFILE files aren't publicly available I'm not sure there's a point > in them doing that, though. > > LSMs are also affected by the lack of a proper parent I'll discuss for > ACLs below. > > > The intent here was to have the case covered where the inode happens to > > be linked back into the namespace since we don't do this work in the > > link path. > > That's an interesting one. O_TMFILE files don't have a real parent > to inherit ACLs from, the pathname passed in just needs to point to > a directory to find the filesystem to create the tmpfile in. On > the other hand it seem like the extN implementations do inherity the > ACL in this case. > > The link into the namespace is irrelavant here as ACL inheritance only > happens on initial create, not at link time. > > I also think we'll absolutely need a test case for ACLs+tmpfile to > make sure all filesystems handle it the same way. > > > The bulk of the refactoring was with the idea that the inode setup for > > the tmpfile case is generally equivalent for the traditional create > > case. The original version was posted here: > > > > http://oss.sgi.com/archives/xfs/2014-04/msg00149.html > > > > ... and it just fixes the deadlock and adds the security initialization. > > I suppose I could still break that out into multiple patches, but that > > aside, is that behavior preferred? > > I think just fixing the deadlock and initializing the security is enough > for the first pass. If you want to do the refactoring on top send it as > a second series on top of the actual fixes. > > _______________________________________________ > xfs mailing list > xfs@oss.sgi.com > http://oss.sgi.com/mailman/listinfo/xfs ---end quoted text--- _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] xfs: fix tmpfile/selinux deadlock and initialize security/acl 2014-04-15 17:52 ` Christoph Hellwig @ 2014-04-15 19:31 ` Andreas Gruenbacher 2014-04-16 11:14 ` Christoph Hellwig 0 siblings, 1 reply; 14+ messages in thread From: Andreas Gruenbacher @ 2014-04-15 19:31 UTC (permalink / raw) To: Christoph Hellwig Cc: linux-man, Brian Foster, xfs, linux-security-module, Al Viro, linux-fsdevel, linux-ext4 Christoph, On Tue, Apr 15, 2014 at 10:52:28AM -0700, Christoph Hellwig wrote: > So any opinions from other fs / security people on how O_TMPFILE files > should behave for ACL inheritance / labeling? from how O_TMPFILE is documented right now [*], creating such a file and then linking it into the namespace is one of the obvious use cases. The intent seems to be to make it seem like the file was created and populated atomically, possibly with inherited permissions and all. I think this behavior require that the O_TMPFILE file inherits from the directory it was "created" in. Adding code to achieve the effect of create-time inheritance at link time, only for O_TMPFILE files or files without any links, doesn't seem reasonable to me: it would duplicate create code in the link code path, and it would make it harder to override inherited permissions or labels. (Trying to fake inheritance by reimplementing it in user space seems like a much worse idea still.) [*] http://man7.org/linux/man-pages/man2/open.2.html Thanks, Andreas _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] xfs: fix tmpfile/selinux deadlock and initialize security/acl 2014-04-15 19:31 ` Andreas Gruenbacher @ 2014-04-16 11:14 ` Christoph Hellwig 2014-04-16 17:29 ` Andreas Gruenbacher 0 siblings, 1 reply; 14+ messages in thread From: Christoph Hellwig @ 2014-04-16 11:14 UTC (permalink / raw) To: Andreas Gruenbacher Cc: linux-man, Brian Foster, xfs, linux-security-module, Al Viro, linux-fsdevel, linux-ext4 On Tue, Apr 15, 2014 at 09:31:02PM +0200, Andreas Gruenbacher wrote: > from how O_TMPFILE is documented right now [*], creating such a file and > then linking it into the namespace is one of the obvious use cases. Btw, I think the man page is wrong - given that the tmpfile is not visible in the namespace it is obviously not created in the directory. The directory passed in is just a handle for the filesystem it should be created in. Michael, should I send you an update for this, or do you want to do it yourself as you can probably come up with better language anyway? > The > intent seems to be to make it seem like the file was created and populated > atomically, possibly with inherited permissions and all. > I think this > behavior require that the O_TMPFILE file inherits from the directory it > was "created" in. > > Adding code to achieve the effect of create-time inheritance at link > time, only for O_TMPFILE files or files without any links, doesn't seem > reasonable to me: it would duplicate create code in the link code path, > and it would make it harder to override inherited permissions or labels. Inheriting any ACL on creating an anonymous file seems utterly wrong. Inheriting on link seems somewhat more sensible and not too bad in terms of code, but very confusing in terms of semantics. I think the best method is to make sure it simply does not inherit any ACL and document that clearly. > (Trying to fake inheritance by reimplementing it in user space seems like > a much worse idea still.) We don't fake inheritance when linking any other file either. And creating a file in a /tmp without any ACL and then linking it into the filesystem already is very common today. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] xfs: fix tmpfile/selinux deadlock and initialize security/acl 2014-04-16 11:14 ` Christoph Hellwig @ 2014-04-16 17:29 ` Andreas Gruenbacher 2014-04-18 16:39 ` Christoph Hellwig 0 siblings, 1 reply; 14+ messages in thread From: Andreas Gruenbacher @ 2014-04-16 17:29 UTC (permalink / raw) To: Christoph Hellwig Cc: linux-man, Brian Foster, xfs, linux-security-module, Al Viro, linux-fsdevel, linux-ext4 On Wed, Apr 16, 2014 at 04:14:02AM -0700, Christoph Hellwig wrote: > On Tue, Apr 15, 2014 at 09:31:02PM +0200, Andreas Gruenbacher wrote: > > from how O_TMPFILE is documented right now [*], creating such a file and > > then linking it into the namespace is one of the obvious use cases. > > Btw, I think the man page is wrong - given that the tmpfile is not > visible in the namespace it is obviously not created in the directory. > The directory passed in is just a handle for the filesystem it should be > created in. I don't agree. If the file is created with O_TMPFILE | O_EXCL, it is clear that the file will never be linked into the namespace. Even then, there are operations which are affected by the inode permissions and label of the anonymous file, and those should still behave reasonably. In this context, I would expect them to behave as if the file was actually created in the specified directory, not in the file system root or "nowhere" with no clearly defined permissions and security label. If the file is created without O_EXCL, then it is clear that the file can later become part of the namespace. In that case, I would also expect the file to be initialized in the context of the specified directory. The file can then be filled with data, permissions and other attributes can be changed if desired, and then the anonymous file can be linked into that directory. > Michael, should I send you an update for this, or do you want to do it > yourself as you can probably come up with better language anyway? > > > The > > intent seems to be to make it seem like the file was created and populated > > atomically, possibly with inherited permissions and all. > > I think this > > behavior require that the O_TMPFILE file inherits from the directory it > > was "created" in. > > > > Adding code to achieve the effect of create-time inheritance at link > > time, only for O_TMPFILE files or files without any links, doesn't seem > > reasonable to me: it would duplicate create code in the link code path, > > and it would make it harder to override inherited permissions or labels. > > Inheriting any ACL on creating an anonymous file seems utterly wrong. Why? > Inheriting on link seems somewhat more sensible and not too bad in terms > of code, but very confusing in terms of semantics. I think the best > method is to make sure it simply does not inherit any ACL and document > that clearly. Again, this approach would make it almost impossible to use O_TMPFILE as a way to atomically create and initialize a file with permission and security label inheritance. This would severely limit the usefulness of O_TMPFILE. > > (Trying to fake inheritance by reimplementing it in user space seems like > > a much worse idea still.) > > We don't fake inheritance when linking any other file either. And > creating a file in a /tmp without any ACL and then linking it into the > filesystem already is very common today. Well, creating a file in /tmp and moving it somewhere else often doesn't even work because of file system boundaries, and sometimes it simply is the wrong thing to do: When the intent is to "create a new file in a directory", such as when saving to a new file in an editor, the result should be as if the file was actually created in that directory. Creating the file in /tmp and then moving it to where it should end up is simply wrong. When the intent is to "move a file from here to there", the file should keep all its attributes, including permissions and security label. It should make no difference in result whether the file could actually be moved or if it had to be copied across file system boundaries. Thanks, Andreas _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] xfs: fix tmpfile/selinux deadlock and initialize security/acl 2014-04-16 17:29 ` Andreas Gruenbacher @ 2014-04-18 16:39 ` Christoph Hellwig 0 siblings, 0 replies; 14+ messages in thread From: Christoph Hellwig @ 2014-04-18 16:39 UTC (permalink / raw) To: Andreas Gruenbacher Cc: linux-man, Brian Foster, xfs, Christoph Hellwig, linux-security-module, Al Viro, linux-fsdevel, linux-ext4 On Wed, Apr 16, 2014 at 07:29:29PM +0200, Andreas Gruenbacher wrote: > > Btw, I think the man page is wrong - given that the tmpfile is not > > visible in the namespace it is obviously not created in the directory. > > The directory passed in is just a handle for the filesystem it should be > > created in. > > I don't agree. If the file is created with O_TMPFILE | O_EXCL, it is clear > that the file will never be linked into the namespace. Even then, there are > operations which are affected by the inode permissions and label of the > anonymous file, and those should still behave reasonably. In this context, > I would expect them to behave as if the file was actually created in the > specified directory, not in the file system root or "nowhere" with no clearly > defined permissions and security label. So you want to define the files as being in a directory, but not actually visible? That's defintively a new and strange state to be in. > > Inheriting any ACL on creating an anonymous file seems utterly wrong. > > Why? Because it has no parent to inherit it from. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] xfs: fix tmpfile/selinux deadlock and initialize security/acl 2014-04-09 19:21 ` [PATCH v2 1/2] xfs: fix tmpfile/selinux deadlock and initialize security/acl Brian Foster 2014-04-10 10:24 ` Christoph Hellwig @ 2014-04-30 12:02 ` Christoph Hellwig 1 sibling, 0 replies; 14+ messages in thread From: Christoph Hellwig @ 2014-04-30 12:02 UTC (permalink / raw) To: Brian Foster; +Cc: xfs Looks like I have lost the the argument on the ACLs. Do you want to resend a version of this rebased to the current tree, or should I do it? We probably should get this into 3.15 so that our tmpfile doesn't behave different from everyone else in the release where we introduce it. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 2/2] xfs: fold xfs_create_tmpfile() into xfs_create() 2014-04-09 19:21 [PATCH v2 0/2] xfs: tmpfile fixes for inode security/acl Brian Foster 2014-04-09 19:21 ` [PATCH v2 1/2] xfs: fix tmpfile/selinux deadlock and initialize security/acl Brian Foster @ 2014-04-09 19:21 ` Brian Foster 2014-04-10 10:29 ` Christoph Hellwig 1 sibling, 1 reply; 14+ messages in thread From: Brian Foster @ 2014-04-09 19:21 UTC (permalink / raw) To: xfs xfs_create_tmpfile() duplicates most of xfs_create() with the exception of a couple minor differences. We use a unique transaction reservation and place the allocated inode on the unlinked list rather than create a directory entry for it. Fold xfs_create_tmpfile() into xfs_create() to reduce some of this duplication. The name parameter that represents the name of the directory entry to create is now conditional and its existence dictates whether a directory entry is created for the inode or not. Signed-off-by: Brian Foster <bfoster@redhat.com> --- fs/xfs/xfs_inode.c | 185 +++++++++++++---------------------------------------- fs/xfs/xfs_iops.c | 8 +-- fs/xfs/xfs_trace.h | 7 +- 3 files changed, 54 insertions(+), 146 deletions(-) diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index 768087b..b895526 100644 --- a/fs/xfs/xfs_inode.c +++ b/fs/xfs/xfs_inode.c @@ -1158,7 +1158,7 @@ xfs_create( struct xfs_dquot *udqp = NULL; struct xfs_dquot *gdqp = NULL; struct xfs_dquot *pdqp = NULL; - struct xfs_trans_res tres; + struct xfs_trans_res *tres; uint resblks; trace_xfs_create(dp, name); @@ -1181,14 +1181,21 @@ xfs_create( if (is_dir) { rdev = 0; resblks = XFS_MKDIR_SPACE_RES(mp, name->len); - tres.tr_logres = M_RES(mp)->tr_mkdir.tr_logres; - tres.tr_logcount = XFS_MKDIR_LOG_COUNT; + tres = &M_RES(mp)->tr_mkdir; tp = xfs_trans_alloc(mp, XFS_TRANS_MKDIR); - } else { + } else if (name) { resblks = XFS_CREATE_SPACE_RES(mp, name->len); - tres.tr_logres = M_RES(mp)->tr_create.tr_logres; - tres.tr_logcount = XFS_CREATE_LOG_COUNT; + tres = &M_RES(mp)->tr_create; tp = xfs_trans_alloc(mp, XFS_TRANS_CREATE); + } else { + /* + * If we don't have a name, we're in the ->tmpfile() path. We + * have a unique transaction here since we modify the unlinked + * list rather than create a directory entry. + */ + resblks = XFS_IALLOC_SPACE_RES(mp); + tres = &M_RES(mp)->tr_create_tmpfile; + tp = xfs_trans_alloc(mp, XFS_TRANS_CREATE_TMPFILE); } cancel_flags = XFS_TRANS_RELEASE_LOG_RES; @@ -1199,26 +1206,22 @@ xfs_create( * the case we'll drop the one we have and get a more * appropriate transaction later. */ - tres.tr_logflags = XFS_TRANS_PERM_LOG_RES; - error = xfs_trans_reserve(tp, &tres, resblks, 0); + error = xfs_trans_reserve(tp, tres, resblks, 0); if (error == ENOSPC) { /* flush outstanding delalloc blocks and retry */ xfs_flush_inodes(mp); - error = xfs_trans_reserve(tp, &tres, resblks, 0); + error = xfs_trans_reserve(tp, tres, resblks, 0); } if (error == ENOSPC) { /* No space at all so try a "no-allocation" reservation */ resblks = 0; - error = xfs_trans_reserve(tp, &tres, 0, 0); + error = xfs_trans_reserve(tp, tres, 0, 0); } if (error) { cancel_flags = 0; goto out_trans_cancel; } - xfs_ilock(dp, XFS_ILOCK_EXCL | XFS_ILOCK_PARENT); - unlock_dp_on_error = true; - xfs_bmap_init(&free_list, &first_block); /* @@ -1229,9 +1232,14 @@ xfs_create( if (error) goto out_trans_cancel; - error = xfs_dir_canenter(tp, dp, name, resblks); - if (error) - goto out_trans_cancel; + if (name) { + xfs_ilock(dp, XFS_ILOCK_EXCL | XFS_ILOCK_PARENT); + unlock_dp_on_error = true; + + error = xfs_dir_canenter(tp, dp, name, resblks); + if (error) + goto out_trans_cancel; + } /* * A newly created regular or special file just has one directory @@ -1253,27 +1261,34 @@ xfs_create( * the transaction cancel unlocking dp so don't do it explicitly in the * error path. */ - xfs_trans_ijoin(tp, dp, XFS_ILOCK_EXCL); - unlock_dp_on_error = false; + if (name) { + xfs_trans_ijoin(tp, dp, XFS_ILOCK_EXCL); + unlock_dp_on_error = false; - error = xfs_dir_createname(tp, dp, name, ip->i_ino, + error = xfs_dir_createname(tp, dp, name, ip->i_ino, &first_block, &free_list, resblks ? resblks - XFS_IALLOC_SPACE_RES(mp) : 0); - if (error) { - ASSERT(error != ENOSPC); - goto out_trans_abort; - } - xfs_trans_ichgtime(tp, dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG); - xfs_trans_log_inode(tp, dp, XFS_ILOG_CORE); + if (error) { + ASSERT(error != ENOSPC); + goto out_trans_abort; + } + xfs_trans_ichgtime(tp, dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG); + xfs_trans_log_inode(tp, dp, XFS_ILOG_CORE); - if (is_dir) { - error = xfs_dir_init(tp, ip, dp); - if (error) - goto out_bmap_cancel; + if (is_dir) { + error = xfs_dir_init(tp, ip, dp); + if (error) + goto out_bmap_cancel; - error = xfs_bumplink(tp, dp); + error = xfs_bumplink(tp, dp); + if (error) + goto out_bmap_cancel; + } + } else { + ip->i_d.di_nlink--; + error = xfs_iunlink(tp, ip); if (error) - goto out_bmap_cancel; + goto out_trans_abort; } /* @@ -1331,114 +1346,6 @@ xfs_create( } int -xfs_create_tmpfile( - struct xfs_inode *dp, - struct dentry *dentry, - umode_t mode, - struct xfs_inode **ipp) -{ - struct xfs_mount *mp = dp->i_mount; - struct xfs_inode *ip = NULL; - struct xfs_trans *tp = NULL; - int error; - uint cancel_flags = XFS_TRANS_RELEASE_LOG_RES; - prid_t prid; - struct xfs_dquot *udqp = NULL; - struct xfs_dquot *gdqp = NULL; - struct xfs_dquot *pdqp = NULL; - struct xfs_trans_res *tres; - uint resblks; - - if (XFS_FORCED_SHUTDOWN(mp)) - return XFS_ERROR(EIO); - - prid = xfs_get_initial_prid(dp); - - /* - * Make sure that we have allocated dquot(s) on disk. - */ - error = xfs_qm_vop_dqalloc(dp, xfs_kuid_to_uid(current_fsuid()), - xfs_kgid_to_gid(current_fsgid()), prid, - XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT, - &udqp, &gdqp, &pdqp); - if (error) - return error; - - resblks = XFS_IALLOC_SPACE_RES(mp); - tp = xfs_trans_alloc(mp, XFS_TRANS_CREATE_TMPFILE); - - tres = &M_RES(mp)->tr_create_tmpfile; - error = xfs_trans_reserve(tp, tres, resblks, 0); - if (error == ENOSPC) { - /* No space at all so try a "no-allocation" reservation */ - resblks = 0; - error = xfs_trans_reserve(tp, tres, 0, 0); - } - if (error) { - cancel_flags = 0; - goto out_trans_cancel; - } - - error = xfs_trans_reserve_quota(tp, mp, udqp, gdqp, - pdqp, resblks, 1, 0); - if (error) - goto out_trans_cancel; - - error = xfs_dir_ialloc(&tp, dp, mode, 1, 0, - prid, resblks > 0, &ip, NULL); - if (error) { - if (error == ENOSPC) - goto out_trans_cancel; - goto out_trans_abort; - } - - if (mp->m_flags & XFS_MOUNT_WSYNC) - xfs_trans_set_sync(tp); - - /* - * Attach the dquot(s) to the inodes and modify them incore. - * These ids of the inode couldn't have changed since the new - * inode has been locked ever since it was created. - */ - xfs_qm_vop_create_dqattach(tp, ip, udqp, gdqp, pdqp); - - ip->i_d.di_nlink--; - error = xfs_iunlink(tp, ip); - if (error) - goto out_trans_abort; - - error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES); - if (error) - goto out_release_inode; - - xfs_qm_dqrele(udqp); - xfs_qm_dqrele(gdqp); - xfs_qm_dqrele(pdqp); - - *ipp = ip; - return 0; - - out_trans_abort: - cancel_flags |= XFS_TRANS_ABORT; - out_trans_cancel: - xfs_trans_cancel(tp, cancel_flags); - out_release_inode: - /* - * Wait until after the current transaction is aborted to - * release the inode. This prevents recursive transactions - * and deadlocks from xfs_inactive. - */ - if (ip) - IRELE(ip); - - xfs_qm_dqrele(udqp); - xfs_qm_dqrele(gdqp); - xfs_qm_dqrele(pdqp); - - return error; -} - -int xfs_link( xfs_inode_t *tdp, xfs_inode_t *sip, diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c index 301ecbf..b430eb7 100644 --- a/fs/xfs/xfs_iops.c +++ b/fs/xfs/xfs_iops.c @@ -137,7 +137,7 @@ xfs_generic_create( struct inode *inode; struct xfs_inode *ip = NULL; struct posix_acl *default_acl, *acl; - struct xfs_name name; + struct xfs_name name, *namep = NULL; int error; /* @@ -158,10 +158,10 @@ xfs_generic_create( if (!tmpfile) { xfs_dentry_to_name(&name, dentry, mode); - error = xfs_create(XFS_I(dir), &name, mode, rdev, &ip); - } else { - error = xfs_create_tmpfile(XFS_I(dir), dentry, mode, &ip); + namep = &name; } + + error = xfs_create(XFS_I(dir), namep, mode, rdev, &ip); if (unlikely(error)) goto out_free_acl; diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h index a4ae41c..da46c94 100644 --- a/fs/xfs/xfs_trace.h +++ b/fs/xfs/xfs_trace.h @@ -691,13 +691,14 @@ DECLARE_EVENT_CLASS(xfs_namespace_class, __field(dev_t, dev) __field(xfs_ino_t, dp_ino) __field(int, namelen) - __dynamic_array(char, name, name->len) + __dynamic_array(char, name, name ? name->len : 0) ), TP_fast_assign( __entry->dev = VFS_I(dp)->i_sb->s_dev; __entry->dp_ino = dp->i_ino; - __entry->namelen = name->len; - memcpy(__get_str(name), name->name, name->len); + __entry->namelen = name ? name->len : 0; + if (name) + memcpy(__get_str(name), name->name, name->len); ), TP_printk("dev %d:%d dp ino 0x%llx name %.*s", MAJOR(__entry->dev), MINOR(__entry->dev), -- 1.8.3.1 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] xfs: fold xfs_create_tmpfile() into xfs_create() 2014-04-09 19:21 ` [PATCH v2 2/2] xfs: fold xfs_create_tmpfile() into xfs_create() Brian Foster @ 2014-04-10 10:29 ` Christoph Hellwig 2014-04-10 12:19 ` Brian Foster 0 siblings, 1 reply; 14+ messages in thread From: Christoph Hellwig @ 2014-04-10 10:29 UTC (permalink / raw) To: Brian Foster; +Cc: xfs > - struct xfs_trans_res tres; > + struct xfs_trans_res *tres; > uint resblks; > > trace_xfs_create(dp, name); > @@ -1181,14 +1181,21 @@ xfs_create( > if (is_dir) { > rdev = 0; > resblks = XFS_MKDIR_SPACE_RES(mp, name->len); > - tres.tr_logres = M_RES(mp)->tr_mkdir.tr_logres; > - tres.tr_logcount = XFS_MKDIR_LOG_COUNT; > + tres = &M_RES(mp)->tr_mkdir; The (nice) reservation cleanup should be a patch of it's own. > + } else { > + /* > + * If we don't have a name, we're in the ->tmpfile() path. We > + * have a unique transaction here since we modify the unlinked > + * list rather than create a directory entry. > + */ How is that transaction more "uniqueue" than the others? Seems like this comment generally doesn't add a whole lot of value. > + if (name) { > + xfs_ilock(dp, XFS_ILOCK_EXCL | XFS_ILOCK_PARENT); > + unlock_dp_on_error = true; > + > + error = xfs_dir_canenter(tp, dp, name, resblks); > + if (error) > + goto out_trans_cancel; > + } So we get another special case in this function. Can't say I like that too much, on the other hand I don't really like the duplicate code either. So I'm not excited about this, but also not strongly against it. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] xfs: fold xfs_create_tmpfile() into xfs_create() 2014-04-10 10:29 ` Christoph Hellwig @ 2014-04-10 12:19 ` Brian Foster 0 siblings, 0 replies; 14+ messages in thread From: Brian Foster @ 2014-04-10 12:19 UTC (permalink / raw) To: Christoph Hellwig; +Cc: xfs On Thu, Apr 10, 2014 at 03:29:01AM -0700, Christoph Hellwig wrote: > > - struct xfs_trans_res tres; > > + struct xfs_trans_res *tres; > > uint resblks; > > > > trace_xfs_create(dp, name); > > @@ -1181,14 +1181,21 @@ xfs_create( > > if (is_dir) { > > rdev = 0; > > resblks = XFS_MKDIR_SPACE_RES(mp, name->len); > > - tres.tr_logres = M_RES(mp)->tr_mkdir.tr_logres; > > - tres.tr_logcount = XFS_MKDIR_LOG_COUNT; > > + tres = &M_RES(mp)->tr_mkdir; > > The (nice) reservation cleanup should be a patch of it's own. > Ok. > > + } else { > > + /* > > + * If we don't have a name, we're in the ->tmpfile() path. We > > + * have a unique transaction here since we modify the unlinked > > + * list rather than create a directory entry. > > + */ > > How is that transaction more "uniqueue" than the others? Seems like > this comment generally doesn't add a whole lot of value. > It's just as unique as the others. ;) I wasn't intending to call out this transaction as special in any way. Rather, I was just trying to document why there is a separate transaction depending on the existence of the name. I can drop the comment. > > + if (name) { > > + xfs_ilock(dp, XFS_ILOCK_EXCL | XFS_ILOCK_PARENT); > > + unlock_dp_on_error = true; > > + > > + error = xfs_dir_canenter(tp, dp, name, resblks); > > + if (error) > > + goto out_trans_cancel; > > + } > > So we get another special case in this function. Can't say I like that > too much, on the other hand I don't really like the duplicate code > either. So I'm not excited about this, but also not strongly against it. > Indeed. I debated whether it would be reasonable to make this function slightly longer and more complex on its own. When I realized xfs_create_tmpfile() was 90% duplicate, it seemed worth the tradeoff for a 100+ line function. Brian _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2014-04-30 12:02 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-04-09 19:21 [PATCH v2 0/2] xfs: tmpfile fixes for inode security/acl Brian Foster 2014-04-09 19:21 ` [PATCH v2 1/2] xfs: fix tmpfile/selinux deadlock and initialize security/acl Brian Foster 2014-04-10 10:24 ` Christoph Hellwig 2014-04-10 12:19 ` Brian Foster 2014-04-10 12:29 ` Christoph Hellwig 2014-04-15 17:52 ` Christoph Hellwig 2014-04-15 19:31 ` Andreas Gruenbacher 2014-04-16 11:14 ` Christoph Hellwig 2014-04-16 17:29 ` Andreas Gruenbacher 2014-04-18 16:39 ` Christoph Hellwig 2014-04-30 12:02 ` Christoph Hellwig 2014-04-09 19:21 ` [PATCH v2 2/2] xfs: fold xfs_create_tmpfile() into xfs_create() Brian Foster 2014-04-10 10:29 ` Christoph Hellwig 2014-04-10 12:19 ` Brian Foster
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).