* Re: [PATCH] [RFC] xfs: initialise attr fork on inode create [not found] ` <20201203214426.GE3913616@dread.disaster.area> @ 2020-12-04 7:54 ` Christoph Hellwig 2020-12-07 17:22 ` Casey Schaufler 0 siblings, 1 reply; 6+ messages in thread From: Christoph Hellwig @ 2020-12-04 7:54 UTC (permalink / raw) To: Dave Chinner; +Cc: Christoph Hellwig, linux-xfs, linux-security-module 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [RFC] xfs: initialise attr fork on inode create 2020-12-04 7:54 ` [PATCH] [RFC] xfs: initialise attr fork on inode create Christoph Hellwig @ 2020-12-07 17:22 ` Casey Schaufler 2020-12-07 17:25 ` Christoph Hellwig 0 siblings, 1 reply; 6+ messages in thread From: Casey Schaufler @ 2020-12-07 17:22 UTC (permalink / raw) To: Christoph Hellwig, Dave Chinner Cc: linux-xfs, linux-security-module, 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() ? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [RFC] xfs: initialise attr fork on inode create 2020-12-07 17:22 ` Casey Schaufler @ 2020-12-07 17:25 ` Christoph Hellwig 2020-12-07 20:49 ` Dave Chinner 0 siblings, 1 reply; 6+ messages in thread From: Christoph Hellwig @ 2020-12-07 17:25 UTC (permalink / raw) To: Casey Schaufler Cc: Christoph Hellwig, Dave Chinner, linux-xfs, linux-security-module 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [RFC] xfs: initialise attr fork on inode create 2020-12-07 17:25 ` Christoph Hellwig @ 2020-12-07 20:49 ` Dave Chinner 0 siblings, 0 replies; 6+ messages in thread From: Dave Chinner @ 2020-12-07 20:49 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Casey Schaufler, linux-xfs, linux-security-module 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [RFC] xfs: initialise attr fork on inode create [not found] <20201202232724.1730114-1-david@fromorbit.com> [not found] ` <20201203084012.GA32480@infradead.org> @ 2020-12-07 17:31 ` Christoph Hellwig 2020-12-07 20:42 ` Dave Chinner 1 sibling, 1 reply; 6+ messages in thread From: Christoph Hellwig @ 2020-12-07 17:31 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-xfs, linux-security-module, Casey Schaufler 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [RFC] xfs: initialise attr fork on inode create 2020-12-07 17:31 ` Christoph Hellwig @ 2020-12-07 20:42 ` Dave Chinner 0 siblings, 0 replies; 6+ messages in thread From: Dave Chinner @ 2020-12-07 20:42 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-xfs, linux-security-module, Casey Schaufler 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-12-07 21:14 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20201202232724.1730114-1-david@fromorbit.com> [not found] ` <20201203084012.GA32480@infradead.org> [not found] ` <20201203214426.GE3913616@dread.disaster.area> 2020-12-04 7:54 ` [PATCH] [RFC] xfs: initialise attr fork on inode create Christoph Hellwig 2020-12-07 17:22 ` Casey Schaufler 2020-12-07 17:25 ` Christoph Hellwig 2020-12-07 20:49 ` Dave Chinner 2020-12-07 17:31 ` Christoph Hellwig 2020-12-07 20:42 ` Dave Chinner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).