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

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

[Christoph Hellwig]

On Fri, Dec 04, 2020 at 08:44:26AM +1100, Dave Chinner wrote:
> > > +		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...

I've added the relevant list, maybe someone there has an opinion.

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

[Casey Schaufler]

On 12/3/2020 11:54 PM, Christoph Hellwig wrote:
> On Fri, Dec 04, 2020 at 08:44:26AM +1100, Dave Chinner wrote:
>>>> +		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

Only security modules should ever look at what's in the security blob.
In fact, you can't assume that the presence of a security blob
(i.e. ...->s_security != NULL) implies "need_xattr", or any other
state for the superblock.

>>  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...
> I've added the relevant list, maybe someone there has an opinion.

How is what you're looking for different from security_ismaclabel() ?

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

[Christoph Hellwig]

On Mon, Dec 07, 2020 at 09:22:13AM -0800, Casey Schaufler wrote:
> Only security modules should ever look at what's in the security blob.
> In fact, you can't assume that the presence of a security blob
> (i.e. ...->s_security != NULL) implies "need_xattr", or any other
> state for the superblock.

Maybe "strongly suggests that an xattr will be added" is the better
wording.

> 
> >>  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...
> > I've added the relevant list, maybe someone there has an opinion.
> 
> How is what you're looking for different from security_ismaclabel() ?

Not at all.  What this needs is a guestimate (which doesn't have
to be 100% reliable) that a new inode created by ->create, ->mknod,
or ->mkdir will have an xattr set on it during the creation syscall.

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

[Dave Chinner]

On Mon, Dec 07, 2020 at 05:25:45PM +0000, Christoph Hellwig wrote:
> On Mon, Dec 07, 2020 at 09:22:13AM -0800, Casey Schaufler wrote:
> > Only security modules should ever look at what's in the security blob.
> > In fact, you can't assume that the presence of a security blob
> > (i.e. ...->s_security != NULL) implies "need_xattr", or any other
> > state for the superblock.
> 
> Maybe "strongly suggests that an xattr will be added" is the better
> wording.

Right, I did this knowing that only selinux and smack actually use
sb->s_security so it's not 100% reliable. However, these are also
the only two security modules that hook inode_init_security and
create xattrs.

So it seems like peeking at ->s_security here gives us a fairly
reliable indicator that we're going to have to create xattrs on this
new inode before we complete the create process...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Christoph Hellwig]

Btw, while looking at the code before replying to Casey I noticed
something else in this area of code which we should probably fix
if we touch all this.  We are really supposed to create the ACLs
and security labels atomically with the actual inode creation.  And
I think we have all the infrastructure to do this without too much
pain now for ACLs.  Security labels with the weird
security_inode_init_security interface might be a little harder but
not impossible.

And I suspect security_inode_init_security might be right thing
to reuse for the helper to figure out what attrs would be set.  If
security_inode_init_security with an idempotent callback is
idempotent itself we might be able to use it directly, but all the
weird hooking makes it rather hard to read.

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

[Dave Chinner]

On Mon, Dec 07, 2020 at 05:31:11PM +0000, Christoph Hellwig wrote:
> Btw, while looking at the code before replying to Casey I noticed
> something else in this area of code which we should probably fix
> if we touch all this.  We are really supposed to create the ACLs
> and security labels atomically with the actual inode creation.  And
> I think we have all the infrastructure to do this without too much
> pain now for ACLs.  Security labels with the weird
> security_inode_init_security interface might be a little harder but
> not impossible.

Yes, that's long been a known problem - it was a problem way back in
the day for DMF and the DMAPI attributes that needed to be added at
create time. That's where XFS_TRANS_RESERVE originally came from, so
that ENOSPC didn't prevent the dmapi xattr from being added to a
newly created inode.

This atomicity problem is one of the things that Allison's attribute
defer-op rework is intended to address.  i.e. being able to
atomically create xattrs at inode create time and have them fully
recoverable by intent replay if the initial inode allocation and
directory linkage transaction succeeds.

Essential the transaction context would start in
xfs_generic_create(), not xfs_create(), and roll through to the
xattr creations for ACLs and security contexts...

> And I suspect security_inode_init_security might be right thing
> to reuse for the helper to figure out what attrs would be set.

Problem is that it appears to need an inode to already be allocated
and instantiated to work....

> If
> security_inode_init_security with an idempotent callback is
> idempotent itself we might be able to use it directly, but all the
> weird hooking makes it rather hard to read.

Yeah, it's like a layer of inscrutible obscurity has been added to a
simple ops table... :/

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com