On Tue, May 12, 2009 at 08:18:34AM -0400, Stephen Smalley wrote: > On Tue, 2009-05-12 at 11:12 +1000, James Morris wrote: > > On Mon, 11 May 2009, Joel Becker wrote: > > > > > > e.g. SELinux will need to perform some checks on the operation, then > > > > calculate a new security context for the new file. > > > > > > Do I need to pass in preserve_security as well so SELinux knows > > > what the ownership check determined? > > > > Not for SELinux -- its security attributes are orthogonal to DAC, and it > > will perform its own checks on them. > > Is preserve_security supposed to also control the preservation of the > SELinux security attribute (security.selinux extended attribute)? I'd > expect that either we preserve all the security-relevant attributes or > none of them. And if that is the case, then SELinux has to know about > preserve_security in order to know what the security context of the new > inode will be. Thank you Stephen, you read my mind. In the ocfs2 case, we're expecting to just reflink the extended attribute structures verbatim in the preserve_security case. So we would be ignoring whatever was set on the new_dentry by security_inode_reflink(). This gets us the best CoW sharing of the xattr extents, but I want to make sure that's "safe" in the preserve_security case. > Also, if you are going to automatically degrade reflink(2) behavior > based on the owner_or_cap test, then you ought to allow the same to be > true if the security module vetoes the attempt to preserve attributes. > Either DAC or MAC logic may say that security attributes cannot be > preserved. Your current logic will only allow graceful degradation in > the DAC case, but the MAC case will remain a hard failure. I did not think of this, and its a very good point as well. I'm not sure how to have the return value of security_inode_reflink() distinguish between "disallow the reflink" and "disallow preserve_security". But since !preserve_security requires read access only, perhaps we move security_inode_reflink up higher and say: error = security_inode_reflink(old_dentry, dir); if (error) preserve_security = 0; Here security_inode_reflink() does not need new_dentry, because it isn't setting a security context. If it's ok with the reflink, we'll be copying the extended attribute. If it's not OK, it falls through to the inode_permission(inode, MAY_READ) check, which will check for plain old read access. What do we think? Joel -- "Under capitalism, man exploits man. Under Communism, it's just the opposite." - John Kenneth Galbraith Joel Becker Principal Software Developer Oracle E-mail: joel.becker@xxxxxxxxxx Phone: (650) 506-8127 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
