Re: [PATCH] [RFC] xfs: initialise attr fork on inode create

[Dave Chinner <david@fromorbit.com>]

On Thu, Dec 03, 2020 at 08:40:12AM +0000, Christoph Hellwig wrote:
> This looks pretty sensible, and pretty simple.  Why the RFC?
> 
> This looks good to me modulo a few tiny nitpicks below:
> 
> > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > index 1414ab79eacf..75b44b82ad1f 100644
> > --- a/fs/xfs/xfs_iops.c
> > +++ b/fs/xfs/xfs_iops.c
> > @@ -126,6 +126,7 @@ xfs_cleanup_inode(
> >  	xfs_remove(XFS_I(dir), &teardown, XFS_I(inode));
> >  }
> >  
> > +
> >  STATIC int
> >  xfs_generic_create(
> >  	struct inode	*dir,
> 
> Nit: this adds a spuurious empty line.

Fixed.

> > @@ -161,7 +162,14 @@ xfs_generic_create(
> >  		goto out_free_acl;
> >  
> >  	if (!tmpfile) {
> > -		error = xfs_create(XFS_I(dir), &name, mode, rdev, &ip);
> > +		bool need_xattr = false;
> > +
> > +		if ((IS_ENABLED(CONFIG_SECURITY) && dir->i_sb->s_security) ||
> > +		    default_acl || acl)
> > +			need_xattr = true;
> > +
> > +		error = xfs_create(XFS_I(dir), &name, mode, rdev,
> > +					need_xattr, &ip);
> 
> It might be wort to factor the condition into a little helper.  Also
> I think we also have security labels for O_TMPFILE inodes, so it might
> be worth plugging into that path as well.

Yeah, a helper is a good idea - I just wanted to get some feedback
first on whether it's a good idea to peek directly at
i_sb->s_security or whether there is some other way of knowing ahead
of time that a security xattr is going to be created. I couldn't
find one, but that doesn't mean such an interface doesn't exist in
all the twisty passages of the LSM layers...

You didn't shout and run screaming, so that's a positive :)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com