[PATCH v3 0/4] xfs: tmpfile fixes

[PATCH v3 0/4] xfs: tmpfile fixes

[Brian Foster]

Hi all,

Here's a v3 series for the patches previously posted here:

http://oss.sgi.com/archives/xfs/2014-04/msg00181.html

Patches 1 and 2 are just a split-up of the v1 patch:

http://oss.sgi.com/archives/xfs/2014-04/msg00149.html

Note the v1 patch has a reviewed-by, so feel free to drop 1 and 2 here
in favor of that version. Patches 3 and 4 are a couple cleanups in the
xfs_create() path.

Setting the default ACL is dropped until it is determined how that
should be handled for tmpfile(). This means the xfs_iops.c refactor bits
have been dropped as well.

Brian

v3:
- Split up the deadlock fix and inode security initialization into
  separate patches. [hch]
- Dropped the default ACL and associated refactor bits for now. [hch]
- Split the xfs_create() tres and xfs_create_tmpfile() cleanups into
  separate patches. [hch]
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 (4):
  xfs: fix tmpfile/selinux ilock deadlock
  xfs: initialize inode security on tmpfile creation
  xfs: replace on-stack xfs_trans_res with pointer in xfs_create()
  xfs: fold xfs_create_tmpfile() into xfs_create()

 fs/xfs/xfs_inode.c | 193 +++++++++++++----------------------------------------
 fs/xfs/xfs_inode.h |   2 -
 fs/xfs/xfs_iops.c  |  20 +++++-
 fs/xfs/xfs_trace.h |   7 +-
 4 files changed, 69 insertions(+), 153 deletions(-)

-- 
1.8.3.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH v3 1/4] xfs: fix tmpfile/selinux ilock deadlock

[Brian Foster]

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

Pull the d_tmpfile() call up into xfs_vn_tmpfile() after the transaction
has been committed and the inode unlocked. This pattern is consistent
with other dcache callers (e.g., d_instantiate()) in xfs_iops.c.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_inode.c |  6 +++---
 fs/xfs/xfs_inode.h |  4 ++--
 fs/xfs/xfs_iops.c  | 14 +++++++++++---
 3 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 5e7a38f..0e63c7d 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1333,8 +1333,8 @@ xfs_create(
 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 +1402,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 +1414,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..4a612fd 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -333,8 +333,8 @@ int		xfs_lookup(struct xfs_inode *dp, struct xfs_name *name,
 			   struct xfs_inode **ipp, struct xfs_name *ci_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);
+int		xfs_create_tmpfile(struct xfs_inode *dp, 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..8fdbc38 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -1053,11 +1053,19 @@ xfs_vn_tmpfile(
 	struct dentry	*dentry,
 	umode_t		mode)
 {
-	int		error;
+	int			error;
+	struct xfs_inode	*ip;
+	struct inode		*inode;
 
-	error = xfs_create_tmpfile(XFS_I(dir), dentry, mode);
+	error = xfs_create_tmpfile(XFS_I(dir), mode, &ip);
+	if (unlikely(error))
+		return -error;
 
-	return -error;
+	inode = VFS_I(ip);
+
+	d_tmpfile(dentry, inode);
+
+	return 0;
 }
 
 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

Re: [PATCH v3 1/4] xfs: fix tmpfile/selinux ilock deadlock

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH v3 2/4] xfs: initialize inode security on tmpfile creation

[Brian Foster]

Initialize security for inodes allocated via the tmpfile interface. This
ensures security is initialized if the inode is subsequently linked back
into the namespace.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_iops.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 8fdbc38..2b1d1bd 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -1063,6 +1063,12 @@ xfs_vn_tmpfile(
 
 	inode = VFS_I(ip);
 
+	error = xfs_init_security(inode, dir, &dentry->d_name);
+	if (unlikely(error)) {
+		iput(inode);
+		return -error;
+	}
+
 	d_tmpfile(dentry, inode);
 
 	return 0;
-- 
1.8.3.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v3 2/4] xfs: initialize inode security on tmpfile creation

[Christoph Hellwig]

On Tue, Apr 15, 2014 at 12:18:24PM -0400, Brian Foster wrote:
> +	error = xfs_init_security(inode, dir, &dentry->d_name);
> +	if (unlikely(error)) {
> +		iput(inode);
> +		return -error;
> +	}
> +
>  	d_tmpfile(dentry, inode);
>  

I'd really love to hear from the LSM people who they plan to deal with
O_TMPFILE inodes.    But given that this seems to fix a real life bug
let's go with it for now.

Re: [PATCH v3 2/4] xfs: initialize inode security on tmpfile creation

[Christoph Hellwig]

On Tue, Apr 15, 2014 at 12:18:24PM -0400, Brian Foster wrote:
> +	error = xfs_init_security(inode, dir, &dentry->d_name);
> +	if (unlikely(error)) {
> +		iput(inode);
> +		return -error;
> +	}
> +
>  	d_tmpfile(dentry, inode);
>  

I'd really love to hear from the LSM people who they plan to deal with
O_TMPFILE inodes.    But given that this seems to fix a real life bug
let's go with it for now.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v3 2/4] xfs: initialize inode security on tmpfile creation

[Stephen Smalley]

On 04/15/2014 01:50 PM, Christoph Hellwig wrote:
> On Tue, Apr 15, 2014 at 12:18:24PM -0400, Brian Foster wrote:
>> +	error = xfs_init_security(inode, dir, &dentry->d_name);
>> +	if (unlikely(error)) {
>> +		iput(inode);
>> +		return -error;
>> +	}
>> +
>>  	d_tmpfile(dentry, inode);
>>  
> 
> I'd really love to hear from the LSM people who they plan to deal with
> O_TMPFILE inodes.    But given that this seems to fix a real life bug
> let's go with it for now.

Is there a reason that xfs_init_security() isn't called from the inode
allocation function (e.g. xfs_ialloc), as in ext4 (__ext4_new_inode
calls ext4_init_security and also calls ext4_init_acl)?  That would have
ensured that tmpfile inodes would have been labeled without requiring a
separate change and more generally ensures complete coverage for all inodes.

For SELinux, we need the tmpfile inodes to be labeled at creation time,
not just if linked into the namespace, since they may be shared via
local socket IPC or inherited across a label-changing exec and since we
revalidate access on transfer or use.

Labeling based on the provided directory could be a bit random, although
it will work out with current policy if the provided directory
corresponds to existing tmpfile locations (e.g. /tmp, /var/tmp) and
therefore already has a label associated with temporary files.
Otherwise we might want some indication that it is a tmpfile passed into
security_inode_init_security() so that we can always select a stable
label irrespective of the directory.




_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v3 2/4] xfs: initialize inode security on tmpfile creation

[Stephen Smalley]

On 04/15/2014 04:04 PM, Stephen Smalley wrote:
> On 04/15/2014 01:50 PM, Christoph Hellwig wrote:
>> On Tue, Apr 15, 2014 at 12:18:24PM -0400, Brian Foster wrote:
>>> +	error = xfs_init_security(inode, dir, &dentry->d_name);
>>> +	if (unlikely(error)) {
>>> +		iput(inode);
>>> +		return -error;
>>> +	}
>>> +
>>>  	d_tmpfile(dentry, inode);
>>>  
>>
>> I'd really love to hear from the LSM people who they plan to deal with
>> O_TMPFILE inodes.    But given that this seems to fix a real life bug
>> let's go with it for now.
> 
> Is there a reason that xfs_init_security() isn't called from the inode
> allocation function (e.g. xfs_ialloc), as in ext4 (__ext4_new_inode
> calls ext4_init_security and also calls ext4_init_acl)?  That would have
> ensured that tmpfile inodes would have been labeled without requiring a
> separate change and more generally ensures complete coverage for all inodes.
> 
> For SELinux, we need the tmpfile inodes to be labeled at creation time,
> not just if linked into the namespace, since they may be shared via
> local socket IPC or inherited across a label-changing exec and since we
> revalidate access on transfer or use.
> 
> Labeling based on the provided directory could be a bit random, although
> it will work out with current policy if the provided directory
> corresponds to existing tmpfile locations (e.g. /tmp, /var/tmp) and
> therefore already has a label associated with temporary files.
> Otherwise we might want some indication that it is a tmpfile passed into
> security_inode_init_security() so that we can always select a stable
> label irrespective of the directory.

Hmm...wondering if we can use the qstr as a distinguisher; pass NULL for
tmpfile and not for others as in ext4?

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v3 2/4] xfs: initialize inode security on tmpfile creation

[Christoph Hellwig]

On Tue, Apr 15, 2014 at 04:04:32PM -0400, Stephen Smalley wrote:
> Is there a reason that xfs_init_security() isn't called from the inode
> allocation function (e.g. xfs_ialloc), as in ext4 (__ext4_new_inode
> calls ext4_init_security and also calls ext4_init_acl)?  That would have
> ensured that tmpfile inodes would have been labeled without requiring a
> separate change and more generally ensures complete coverage for all inodes.

Really just code structuring - we don't like callouts to high level VFS
functions from deep down in the guts of the filesystem.

> For SELinux, we need the tmpfile inodes to be labeled at creation time,
> not just if linked into the namespace, since they may be shared via
> local socket IPC or inherited across a label-changing exec and since we
> revalidate access on transfer or use.
> 
> Labeling based on the provided directory could be a bit random, although
> it will work out with current policy if the provided directory
> corresponds to existing tmpfile locations (e.g. /tmp, /var/tmp) and
> therefore already has a label associated with temporary files.
> Otherwise we might want some indication that it is a tmpfile passed into
> security_inode_init_security() so that we can always select a stable
> label irrespective of the directory.

Just check for I_LINKABLE in i_flags.

Re: [PATCH v3 2/4] xfs: initialize inode security on tmpfile creation

[Christoph Hellwig]

On Tue, Apr 15, 2014 at 04:04:32PM -0400, Stephen Smalley wrote:
> Is there a reason that xfs_init_security() isn't called from the inode
> allocation function (e.g. xfs_ialloc), as in ext4 (__ext4_new_inode
> calls ext4_init_security and also calls ext4_init_acl)?  That would have
> ensured that tmpfile inodes would have been labeled without requiring a
> separate change and more generally ensures complete coverage for all inodes.

Really just code structuring - we don't like callouts to high level VFS
functions from deep down in the guts of the filesystem.

> For SELinux, we need the tmpfile inodes to be labeled at creation time,
> not just if linked into the namespace, since they may be shared via
> local socket IPC or inherited across a label-changing exec and since we
> revalidate access on transfer or use.
> 
> Labeling based on the provided directory could be a bit random, although
> it will work out with current policy if the provided directory
> corresponds to existing tmpfile locations (e.g. /tmp, /var/tmp) and
> therefore already has a label associated with temporary files.
> Otherwise we might want some indication that it is a tmpfile passed into
> security_inode_init_security() so that we can always select a stable
> label irrespective of the directory.

Just check for I_LINKABLE in i_flags.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v3 2/4] xfs: initialize inode security on tmpfile creation

[Stephen Smalley]

On 04/15/2014 04:22 PM, Christoph Hellwig wrote:
> On Tue, Apr 15, 2014 at 04:04:32PM -0400, Stephen Smalley wrote:
>> Is there a reason that xfs_init_security() isn't called from the inode
>> allocation function (e.g. xfs_ialloc), as in ext4 (__ext4_new_inode
>> calls ext4_init_security and also calls ext4_init_acl)?  That would have
>> ensured that tmpfile inodes would have been labeled without requiring a
>> separate change and more generally ensures complete coverage for all inodes.
> 
> Really just code structuring - we don't like callouts to high level VFS
> functions from deep down in the guts of the filesystem.
> 
>> For SELinux, we need the tmpfile inodes to be labeled at creation time,
>> not just if linked into the namespace, since they may be shared via
>> local socket IPC or inherited across a label-changing exec and since we
>> revalidate access on transfer or use.
>>
>> Labeling based on the provided directory could be a bit random, although
>> it will work out with current policy if the provided directory
>> corresponds to existing tmpfile locations (e.g. /tmp, /var/tmp) and
>> therefore already has a label associated with temporary files.
>> Otherwise we might want some indication that it is a tmpfile passed into
>> security_inode_init_security() so that we can always select a stable
>> label irrespective of the directory.
> 
> Just check for I_LINKABLE in i_flags.

Thanks, that should allow us to handle it cleanly in the security modules!


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v3 2/4] xfs: initialize inode security on tmpfile creation

[Stephen Smalley]

On 04/15/2014 04:21 PM, Stephen Smalley wrote:
> On 04/15/2014 04:22 PM, Christoph Hellwig wrote:
>> On Tue, Apr 15, 2014 at 04:04:32PM -0400, Stephen Smalley wrote:
>>> Is there a reason that xfs_init_security() isn't called from the inode
>>> allocation function (e.g. xfs_ialloc), as in ext4 (__ext4_new_inode
>>> calls ext4_init_security and also calls ext4_init_acl)?  That would have
>>> ensured that tmpfile inodes would have been labeled without requiring a
>>> separate change and more generally ensures complete coverage for all inodes.
>>
>> Really just code structuring - we don't like callouts to high level VFS
>> functions from deep down in the guts of the filesystem.
>>
>>> For SELinux, we need the tmpfile inodes to be labeled at creation time,
>>> not just if linked into the namespace, since they may be shared via
>>> local socket IPC or inherited across a label-changing exec and since we
>>> revalidate access on transfer or use.
>>>
>>> Labeling based on the provided directory could be a bit random, although
>>> it will work out with current policy if the provided directory
>>> corresponds to existing tmpfile locations (e.g. /tmp, /var/tmp) and
>>> therefore already has a label associated with temporary files.
>>> Otherwise we might want some indication that it is a tmpfile passed into
>>> security_inode_init_security() so that we can always select a stable
>>> label irrespective of the directory.
>>
>> Just check for I_LINKABLE in i_flags.
> 
> Thanks, that should allow us to handle it cleanly in the security modules!

Maybe I spoke too soon.  IIUC, I_LINKABLE doesn't necessarily
distinguish tmpfiles from other files, as some tmpfiles may be linkable
and others not.  But what we want is a way to identify all tmpfiles when
security_inode_init_security() is called if we are going to label them
independently of the provided dir.

Also, in that situation, we would need to likewise distinguish them
during the create-time checking, i.e. when security_inode_create() is
called (from may_o_create), as we have to determine the label that will
be applied at that point for permission checking.  And there we do not
have the inode yet so we do not even have I_LINKABLE as a distinguisher.

So I think we need __O_TMPFILE or similar flag passed down to
may_o_create() -> security_inode_create() and to
security_inode_init_security() if we are going to label these files
independently of the provided directory.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v3 2/4] xfs: initialize inode security on tmpfile creation

[Christoph Hellwig]

On Wed, Apr 16, 2014 at 08:51:38AM -0400, Stephen Smalley wrote:
> Maybe I spoke too soon.  IIUC, I_LINKABLE doesn't necessarily
> distinguish tmpfiles from other files, as some tmpfiles may be linkable
> and others not.  But what we want is a way to identify all tmpfiles when
> security_inode_init_security() is called if we are going to label them
> independently of the provided dir.

Oh, right.  If O_EXCL is specified (another annoying overload of the
flag..) the tmpfile can't ever be linked back into the filesystem
and thus doesn't have I_LINKABLE set.

I guess the best way to fix this is using the magic qstr you suggested
before.  That means security_inode_init_security would need to be
called after d_tmpfile, which most filesystems don't do right now.

Re: [PATCH v3 2/4] xfs: initialize inode security on tmpfile creation

[Christoph Hellwig]

On Wed, Apr 16, 2014 at 08:51:38AM -0400, Stephen Smalley wrote:
> Maybe I spoke too soon.  IIUC, I_LINKABLE doesn't necessarily
> distinguish tmpfiles from other files, as some tmpfiles may be linkable
> and others not.  But what we want is a way to identify all tmpfiles when
> security_inode_init_security() is called if we are going to label them
> independently of the provided dir.

Oh, right.  If O_EXCL is specified (another annoying overload of the
flag..) the tmpfile can't ever be linked back into the filesystem
and thus doesn't have I_LINKABLE set.

I guess the best way to fix this is using the magic qstr you suggested
before.  That means security_inode_init_security would need to be
called after d_tmpfile, which most filesystems don't do right now.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v3 2/4] xfs: initialize inode security on tmpfile creation

[Stephen Smalley]

On 04/16/2014 10:14 AM, Christoph Hellwig wrote:
> On Wed, Apr 16, 2014 at 08:51:38AM -0400, Stephen Smalley wrote:
>> Maybe I spoke too soon.  IIUC, I_LINKABLE doesn't necessarily
>> distinguish tmpfiles from other files, as some tmpfiles may be linkable
>> and others not.  But what we want is a way to identify all tmpfiles when
>> security_inode_init_security() is called if we are going to label them
>> independently of the provided dir.
> 
> Oh, right.  If O_EXCL is specified (another annoying overload of the
> flag..) the tmpfile can't ever be linked back into the filesystem
> and thus doesn't have I_LINKABLE set.
> 
> I guess the best way to fix this is using the magic qstr you suggested
> before.  That means security_inode_init_security would need to be
> called after d_tmpfile, which most filesystems don't do right now.

I think one could just pass NULL for the qstr as an indicator, which
ext4 already does, so it doesn't require moving after d_tmpfile) IIUC.
However, that doesn't solve the problem for security_inode_create(),
which also needs to know it is dealing with a tmpfile.  So we might want
to just pass an explicit flag to both.

Re: [PATCH v3 2/4] xfs: initialize inode security on tmpfile creation

[Stephen Smalley]

On 04/16/2014 10:14 AM, Christoph Hellwig wrote:
> On Wed, Apr 16, 2014 at 08:51:38AM -0400, Stephen Smalley wrote:
>> Maybe I spoke too soon.  IIUC, I_LINKABLE doesn't necessarily
>> distinguish tmpfiles from other files, as some tmpfiles may be linkable
>> and others not.  But what we want is a way to identify all tmpfiles when
>> security_inode_init_security() is called if we are going to label them
>> independently of the provided dir.
> 
> Oh, right.  If O_EXCL is specified (another annoying overload of the
> flag..) the tmpfile can't ever be linked back into the filesystem
> and thus doesn't have I_LINKABLE set.
> 
> I guess the best way to fix this is using the magic qstr you suggested
> before.  That means security_inode_init_security would need to be
> called after d_tmpfile, which most filesystems don't do right now.

I think one could just pass NULL for the qstr as an indicator, which
ext4 already does, so it doesn't require moving after d_tmpfile) IIUC.
However, that doesn't solve the problem for security_inode_create(),
which also needs to know it is dealing with a tmpfile.  So we might want
to just pass an explicit flag to both.


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH v3 3/4] xfs: replace on-stack xfs_trans_res with pointer in xfs_create()

[Brian Foster]

There's no need to store a full struct xfs_trans_res on the stack in
xfs_create() and copy the fields. Use a pointer to the appropriate
structures embedded in the xfs_mount.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_inode.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 0e63c7d..f8a232a 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,13 +1181,11 @@ 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 {
 		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);
 	}
 
@@ -1199,17 +1197,16 @@ 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;
-- 
1.8.3.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v3 3/4] xfs: replace on-stack xfs_trans_res with pointer in xfs_create()

[Christoph Hellwig]

On Tue, Apr 15, 2014 at 12:18:25PM -0400, Brian Foster wrote:
> There's no need to store a full struct xfs_trans_res on the stack in
> xfs_create() and copy the fields. Use a pointer to the appropriate
> structures embedded in the xfs_mount.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH v3 4/4] xfs: fold xfs_create_tmpfile() into xfs_create()

[Brian Foster]

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 | 178 +++++++++++++----------------------------------------
 fs/xfs/xfs_inode.h |   2 -
 fs/xfs/xfs_iops.c  |   2 +-
 fs/xfs/xfs_trace.h |   7 ++-
 4 files changed, 47 insertions(+), 142 deletions(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index f8a232a..d82bddf 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1183,10 +1183,14 @@ xfs_create(
 		resblks = XFS_MKDIR_SPACE_RES(mp, name->len);
 		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 = &M_RES(mp)->tr_create;
 		tp = xfs_trans_alloc(mp, XFS_TRANS_CREATE);
+	} else {
+		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;
@@ -1213,9 +1217,6 @@ xfs_create(
 		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);
 
 	/*
@@ -1226,9 +1227,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
@@ -1243,34 +1249,41 @@ xfs_create(
 		goto out_trans_abort;
 	}
 
-	/*
-	 * Now we join the directory inode to the transaction.  We do not do it
-	 * earlier because xfs_dir_ialloc might commit the previous transaction
-	 * (and release all the locks).  An error from here on will result in
-	 * 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) {
+		/*
+		 * Now we join the directory inode to the transaction. We do not
+		 * do it earlier because xfs_dir_ialloc might commit the
+		 * previous transaction (and release all the locks). An error
+		 * from here on will result in 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;
 
-	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;
 	}
 
 	/*
@@ -1328,113 +1341,6 @@ xfs_create(
 }
 
 int
-xfs_create_tmpfile(
-	struct xfs_inode	*dp,
-	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_inode.h b/fs/xfs/xfs_inode.h
index 4a612fd..5a7f81a 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -333,8 +333,6 @@ int		xfs_lookup(struct xfs_inode *dp, struct xfs_name *name,
 			   struct xfs_inode **ipp, struct xfs_name *ci_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, 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 2b1d1bd..f315a38 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -1057,7 +1057,7 @@ xfs_vn_tmpfile(
 	struct xfs_inode	*ip;
 	struct inode		*inode;
 
-	error = xfs_create_tmpfile(XFS_I(dir), mode, &ip);
+	error = xfs_create(XFS_I(dir), NULL, mode, 0, &ip);
 	if (unlikely(error))
 		return -error;
 
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

Re: [PATCH v3 4/4] xfs: fold xfs_create_tmpfile() into xfs_create()

[Christoph Hellwig]

On Tue, Apr 15, 2014 at 12:18:26PM -0400, Brian Foster wrote:
> 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>

Looks correct although I still can't get excited about it..

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v3 0/4] xfs: tmpfile fixes

[Dave Chinner]

On Tue, Apr 15, 2014 at 12:18:22PM -0400, Brian Foster wrote:
> Hi all,
> 
> Here's a v3 series for the patches previously posted here:
> 
> http://oss.sgi.com/archives/xfs/2014-04/msg00181.html
> 
> Patches 1 and 2 are just a split-up of the v1 patch:
> 
> http://oss.sgi.com/archives/xfs/2014-04/msg00149.html
> 
> Note the v1 patch has a reviewed-by, so feel free to drop 1 and 2 here
> in favor of that version. Patches 3 and 4 are a couple cleanups in the
> xfs_create() path.

I've already got the orginal patch lined up for commit and have been
testing it for a couple of days, so I'll stick with the original.

The other two patches I'll add to the 3.16 queue as they aren't
really bug fixes....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs