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@xxxxxxxxxxxxx
