[PATCH] xfs: initialize default acls for ->tmpfile()

[PATCH] xfs: initialize default acls for ->tmpfile()

[Brian Foster]

The current tmpfile handler does not initialize default ACLs. Doing so
within xfs_vn_tmpfile() makes it roughly equivalent to xfs_vn_mknod(),
which is already used as a common create handler.

xfs_vn_mknod() does not currently have a mechanism to determine whether
to link the file into the namespace. Therefore, further abstract
xfs_vn_mknod() into a new xfs_generic_create() handler with a tmpfile
parameter. This new handler passes a NULL xname to the create and calls
d_tmpfile() on the dentry when called via ->tmpfile().

Signed-off-by: Brian Foster <bfoster@redhat.com>
---

Hi all,

It appears that we want to initialize default ACLs for ->tmpfile() after
all. This patch reintroduces the refactoring to initialize security and
ACLs through the current xfs_vn_mknod(). This is based on top of the
previously posted series:

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

Brian

 fs/xfs/xfs_iops.c | 57 +++++++++++++++++++++++++++++--------------------------
 1 file changed, 30 insertions(+), 27 deletions(-)

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index f315a38..b430eb7 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -124,20 +124,20 @@ 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;
 	struct posix_acl *default_acl, *acl;
-	struct xfs_name	name;
+	struct xfs_name	name, *namep = NULL;
 	int		error;
 
 	/*
@@ -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);
+		namep = &name;
+	}
+
+	error = xfs_create(XFS_I(dir), namep, mode, rdev, &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,25 +1074,7 @@ xfs_vn_tmpfile(
 	struct dentry	*dentry,
 	umode_t		mode)
 {
-	int			error;
-	struct xfs_inode	*ip;
-	struct inode		*inode;
-
-	error = xfs_create(XFS_I(dir), NULL, mode, 0, &ip);
-	if (unlikely(error))
-		return -error;
-
-	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;
+	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

Re: [PATCH] xfs: initialize default acls for ->tmpfile()

[Christoph Hellwig]

On Thu, May 01, 2014 at 09:38:07AM -0400, Brian Foster wrote:
> The current tmpfile handler does not initialize default ACLs. Doing so
> within xfs_vn_tmpfile() makes it roughly equivalent to xfs_vn_mknod(),
> which is already used as a common create handler.
> 
> xfs_vn_mknod() does not currently have a mechanism to determine whether
> to link the file into the namespace. Therefore, further abstract
> xfs_vn_mknod() into a new xfs_generic_create() handler with a tmpfile
> parameter. This new handler passes a NULL xname to the create and calls
> d_tmpfile() on the dentry when called via ->tmpfile().
> 
> 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

Re: [PATCH] xfs: initialize default acls for ->tmpfile()

[Dave Chinner]

On Thu, May 01, 2014 at 09:38:07AM -0400, Brian Foster wrote:
> The current tmpfile handler does not initialize default ACLs. Doing so
> within xfs_vn_tmpfile() makes it roughly equivalent to xfs_vn_mknod(),
> which is already used as a common create handler.
> 
> xfs_vn_mknod() does not currently have a mechanism to determine whether
> to link the file into the namespace. Therefore, further abstract
> xfs_vn_mknod() into a new xfs_generic_create() handler with a tmpfile
> parameter. This new handler passes a NULL xname to the create and calls
> d_tmpfile() on the dentry when called via ->tmpfile().
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
> 
> Hi all,
> 
> It appears that we want to initialize default ACLs for ->tmpfile() after
> all. This patch reintroduces the refactoring to initialize security and
> ACLs through the current xfs_vn_mknod(). This is based on top of the
> previously posted series:
> 
> 	http://oss.sgi.com/archives/xfs/2014-04/msg00396.html
> 
> Brian
.....
> @@ -1053,25 +1074,7 @@ xfs_vn_tmpfile(
>  	struct dentry	*dentry,
>  	umode_t		mode)
>  {
> -	int			error;
> -	struct xfs_inode	*ip;
> -	struct inode		*inode;
> -
> -	error = xfs_create(XFS_I(dir), NULL, mode, 0, &ip);
> -	if (unlikely(error))
> -		return -error;
> -
> -	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;
> +	return xfs_generic_create(dir, dentry, mode, 0, true);
>  }

This hunk doesn't apply to a 3.15-rc2 kernel - it calls
xfs_create_tmpfile(). Just applying it as is after fixing the hunk
causes a crash in xfs-create(), because the code in the patch is
calling xfs-create ratehr than xfs_create_tmpfile().

Brian, can you regenerate this patch against a current linus tree
(3.15-rc4)?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH] xfs: initialize default acls for ->tmpfile()

[Brian Foster]

On Mon, May 05, 2014 at 08:24:03PM +1000, Dave Chinner wrote:
> On Thu, May 01, 2014 at 09:38:07AM -0400, Brian Foster wrote:
> > The current tmpfile handler does not initialize default ACLs. Doing so
> > within xfs_vn_tmpfile() makes it roughly equivalent to xfs_vn_mknod(),
> > which is already used as a common create handler.
> > 
> > xfs_vn_mknod() does not currently have a mechanism to determine whether
> > to link the file into the namespace. Therefore, further abstract
> > xfs_vn_mknod() into a new xfs_generic_create() handler with a tmpfile
> > parameter. This new handler passes a NULL xname to the create and calls
> > d_tmpfile() on the dentry when called via ->tmpfile().
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> > 
> > Hi all,
> > 
> > It appears that we want to initialize default ACLs for ->tmpfile() after
> > all. This patch reintroduces the refactoring to initialize security and
> > ACLs through the current xfs_vn_mknod(). This is based on top of the
> > previously posted series:
> > 
> > 	http://oss.sgi.com/archives/xfs/2014-04/msg00396.html
> > 
> > Brian
> .....
> > @@ -1053,25 +1074,7 @@ xfs_vn_tmpfile(
> >  	struct dentry	*dentry,
> >  	umode_t		mode)
> >  {
> > -	int			error;
> > -	struct xfs_inode	*ip;
> > -	struct inode		*inode;
> > -
> > -	error = xfs_create(XFS_I(dir), NULL, mode, 0, &ip);
> > -	if (unlikely(error))
> > -		return -error;
> > -
> > -	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;
> > +	return xfs_generic_create(dir, dentry, mode, 0, true);
> >  }
> 
> This hunk doesn't apply to a 3.15-rc2 kernel - it calls
> xfs_create_tmpfile(). Just applying it as is after fixing the hunk
> causes a crash in xfs-create(), because the code in the patch is
> calling xfs-create ratehr than xfs_create_tmpfile().
> 

Right, this is based on the previously posted series (link above), which
fixes up xfs_create() such that we can use it from xfs_generic_create()
via all associated codepaths. Sorry, I probably should have posted this
as a [5/4 ...] patch to make that more clear...

> Brian, can you regenerate this patch against a current linus tree
> (3.15-rc4)?
> 

It isn't clear to me what the expectation is with this series at this
point, beyond the agreement that we do want to initialize the acls. It
looks like the v1 patch that only initialized security is merged, so
we're Ok as far as that goes. v2 added the bits to initialize the acls
as well:

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

... and followed up with the xfs_create_tmpfile() removal. Is that
(minus the already merged selinux fix) what you're asking for here?

v3 dropped the default acl bits and thus the xfs_generic_create()
handler as well. The xfs_create_tmpfile() cleanup persisted, and now the
default acl bits are required, so it's added back in this 5/4.

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

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

Re: [PATCH] xfs: initialize default acls for ->tmpfile()

[Dave Chinner]

On Mon, May 05, 2014 at 09:17:46AM -0400, Brian Foster wrote:
> On Mon, May 05, 2014 at 08:24:03PM +1000, Dave Chinner wrote:
> > On Thu, May 01, 2014 at 09:38:07AM -0400, Brian Foster wrote:
> > > The current tmpfile handler does not initialize default ACLs. Doing so
> > > within xfs_vn_tmpfile() makes it roughly equivalent to xfs_vn_mknod(),
> > > which is already used as a common create handler.
> > > 
> > > xfs_vn_mknod() does not currently have a mechanism to determine whether
> > > to link the file into the namespace. Therefore, further abstract
> > > xfs_vn_mknod() into a new xfs_generic_create() handler with a tmpfile
> > > parameter. This new handler passes a NULL xname to the create and calls
> > > d_tmpfile() on the dentry when called via ->tmpfile().
> > > 
> > > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > > ---
> > > 
> > > Hi all,
> > > 
> > > It appears that we want to initialize default ACLs for ->tmpfile() after
> > > all. This patch reintroduces the refactoring to initialize security and
> > > ACLs through the current xfs_vn_mknod(). This is based on top of the
> > > previously posted series:
> > > 
> > > 	http://oss.sgi.com/archives/xfs/2014-04/msg00396.html
> > > 
> > > Brian
> > .....
> > > @@ -1053,25 +1074,7 @@ xfs_vn_tmpfile(
> > >  	struct dentry	*dentry,
> > >  	umode_t		mode)
> > >  {
> > > -	int			error;
> > > -	struct xfs_inode	*ip;
> > > -	struct inode		*inode;
> > > -
> > > -	error = xfs_create(XFS_I(dir), NULL, mode, 0, &ip);
> > > -	if (unlikely(error))
> > > -		return -error;
> > > -
> > > -	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;
> > > +	return xfs_generic_create(dir, dentry, mode, 0, true);
> > >  }
> > 
> > This hunk doesn't apply to a 3.15-rc2 kernel - it calls
> > xfs_create_tmpfile(). Just applying it as is after fixing the hunk
> > causes a crash in xfs-create(), because the code in the patch is
> > calling xfs-create ratehr than xfs_create_tmpfile().
> > 
> 
> Right, this is based on the previously posted series (link above), which
> fixes up xfs_create() such that we can use it from xfs_generic_create()
> via all associated codepaths. Sorry, I probably should have posted this
> as a [5/4 ...] patch to make that more clear...
> 
> > Brian, can you regenerate this patch against a current linus tree
> > (3.15-rc4)?
> > 
> 
> It isn't clear to me what the expectation is with this series at this
> point, beyond the agreement that we do want to initialize the acls. It
> looks like the v1 patch that only initialized security is merged, so
> we're Ok as far as that goes. v2 added the bits to initialize the acls
> as well:
> 
> http://oss.sgi.com/archives/xfs/2014-04/msg00182.html
> 
> ... and followed up with the xfs_create_tmpfile() removal. Is that
> (minus the already merged selinux fix) what you're asking for here?
> 
> v3 dropped the default acl bits and thus the xfs_generic_create()
> handler as well. The xfs_create_tmpfile() cleanup persisted, and now the
> default acl bits are required, so it's added back in this 5/4.

Basically, cleanups and factoring, etc, are beyond the scope of
adding the missing default acl initialisation to 3.15-rc4. i.e. the
cleanups and refactoring will go into 3.16, so will end up layered
on top of this bug fix....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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