[fmac-discuss] [PATCH] Move create hooks into filesystem code, mediate existing file open

[Mark Shellenbaum]

Stephen Smalley wrote:
> On Thu, 2008-10-02 at 11:16 -0600, Mark Shellenbaum wrote:
>> Stephen Smalley wrote:
>>> On Thu, 2008-10-02 at 10:04 -0600, Mark Shellenbaum wrote:
>>>> Stephen Smalley wrote:
>>>>> Move the FMAC fmac_vnode_create() and post_create() hooks from
>>>>> fop_create() into zfs_create() so that they are only applied on new file
>>>>> creation not opening of an existing file, and call the
>>>>> fmac_vnode_access() hook from zfs_create() to mediate opening an
>>>>> existing file.  Similar hooking will be needed for other filesystem
>>>>> types like tmpfs as well.  
>>>>>
>>>>> Also move the corresponding hook calls from fop_mkdir() into zfs_mkdir()
>>>>> for consistency with create although we don't have the same problem
>>>>> there with distinguishing create from open.  Given the current need to
>>>>> place several of the hooks (create, unlink/rmdir, rename) in the
>>>>> filesystem code, I'm inclined to move the remaining FMAC vnode hooks in
>>>>> the fop layer into the filesystem code (like the existing DAC checking
>>>>> and secpolicy hooks) rather than having a mixture of some FMAC hooks in
>>>>> the fop_ layer and some in the filesystem code.
>>>>>
>>>> Even if fop_create() new that the file already existed or not doesn't 
>>>> really help.  It could have been removed/added by the time the actual 
>>>> file system xxx_create() function was called.
>>> Linux VFS performs the lookup first (open with create intent), and only
>>> invokes the create code path if the lookup fails.  I understand though
>>> that Solaris is different at present.
>>>
>> Does Linux then have the ability to stop deletion/additions to 
>> directories at their VFS layer?  Is their some coarse VFS level lock 
>> they apply?
> 
> They take a per-directory mutex prior to the lookup, and hold it until
> after the create.
> 
>>>> I don't suppose it would be possible at some point in the future to 
>>>> merge the secpolicy and fmac calls together?
>>> Possibly, but not trivially.  The secpolicy hooks are currently only
>>> invoked if the base DAC checks fail, so the callers would have to be
>>> changed to always invoke them (passing in the DAC decision as an
>>> argument), and then the secpolicy functions would have to be changed to
>>> first apply the FMAC logic, then only perform the existing secpolicy
>>> privilege tests if DAC failed.  Also, the secpolicy hook interface would
>>> have to be mappable to the FMAC checks; one secpolicy hook per vnode
>>> operation would be best for our purposes, whereas today there are
>>> multiple secpolicy hooks called at different points under different
>>> conditions on a given operation's code path.
>>>
>>>>> This patch is less invasive than the prior one, placing the FMAC hooks
>>>>> entirely within the zfs vnode ops rather than pushing
>>>>> fmac_vnode_post_create() down to zfs_perm_init().
>>>>>
>>>>> The wrapping of the vattr with an xvattr is pulled out of
>>>>> fmac_vnode_create() and handled by the callers (zfs_create, zfs_mkdir)
>>>>> now to make it clearer and less prone to bugs.  A xva_from_va() helper
>>>>> function is introduced for populating an xvattr from vattr.
>>>>>
>>>> I don't like converting every vattr_t that comes into 
>>>> zfs_create/zfs_mkdir into an xvattr.  That should only be done when 
>>>> necessary and is probably still best done in fmac_vnode_create().
>>> For FMAC, we need to assign a secctx to every file, so we do need this
>>> on every vattr that comes into zfs_create/mkdir (and ultimately the
>>> other file creation operations) if we are using xvattrs as the mechanism
>>> for conveying the secctx.  I can however move the logic back into
>>> fmac_vnode_create() if desired.  I moved it up into the caller so that
>>> it would be clear that the vap is being switched to refer to an xvattr
>>> declared/allocated in the caller vs. hiding that down inside of the fmac
>>> code.
>>>
>> I'm concerned about the cost of doing the memory copies when fmac isn't 
>> enabled.  Its going to slow down the create code path in ZFS.  Another 
>> alternative would be do do the xvattr allocation in fop_create(), but 
>> only when fmac is enabled.
> 
> For now I can take the memory copying back into fmac_vnode_create(),
> although I do need the xvattr structure to be declared/allocated in the
> caller (fop_create or zfs_create) in order to have the right lifecycle.
> But I'm a little worried that such a small fixed overhead is of concern
> to you, given that we'd like FMAC to ultimately be a default-enabled
> feature of OpenSolaris (just as SELinux is a default-enabled feature of
> Linux) and that the Solaris security team would actually prefer to not
> have to support a disabled mode at all.
> 

I hadn't realized that fmac would be enabled by default.

Since fmac will need xvattr_t in create, mkdir and other vops then maybe 
its time to consider changing the signature of those VOPS to pass an 
xvattr_t instead of a vattr_t. There is no point passing around a 
vattr_t, when we really need an xvattr_t.  You don't need to do this 
now, but this will need to be cleaned up at some point.

   -Mark