Messages in this thread | | | Subject | Re: [PATCH 03/11] VFS: Add security label support to *notify | From | Dave Quigley <> | Date | Thu, 28 Feb 2008 11:07:55 -0500 |
| |
On Thu, 2008-02-28 at 12:20 +1100, James Morris wrote: > On Wed, 27 Feb 2008, David P. Quigley wrote: > > > +int inode_setsecurity(struct inode *inode, struct iattr *attr) > > +{ > > + const char *suffix = security_maclabel_getname() > > + + XATTR_SECURITY_PREFIX_LEN; > > + int error; > > + > > + if (!attr->ia_valid & ATTR_SECURITY_LABEL) > > + return -EINVAL; > > Do you mean: > > if (!(attr->ia_valid & ATTR_SECURITY_LABEL)) > > ?
Yep that is what it should be.
> > > mode &= ~S_ISGID; > > inode->i_mode = mode; > > } > > +#ifdef CONFIG_SECURITY > > + if (ia_valid & ATTR_SECURITY_LABEL) > > + inode_setsecurity(inode, attr); > > +#endif > > You're not checking the return value of inode_setsecurity(). > > Why not just rely on inode_setsecurity() to perform the check, then you > can lose the #ifdefs in the core code & make it a noop for > !CONFIG_SECURITY.
I'm not clear as to what you are suggesting here. are you saying put the #ifdef CONFIG_SECURITY around inode_setsecurity and make the case where CONFIG_SECURITY isn't set an empty static inline function?
> > > > +#ifdef CONFIG_SECURITY > > + if (ia_valid & ATTR_SECURITY_LABEL) { > > + char *key = (char *)security_maclabel_getname(); > > + vfs_setxattr_locked(dentry, key, > > + attr->ia_label, attr->ia_label_len, 0); > > + /* Avoid calling inode_setsecurity() > > + * via inode_setattr() below > > + */ > > + attr->ia_valid &= ~ATTR_SECURITY_LABEL; > > + } > > +#endif > > + > > Similarly, make this a function which is compiled away for > !CONFIG_SECURITY.
Same as above.
> > > + if (!error) { > > +#ifdef CONFIG_SECURITY > > + fsnotify_change(dentry, ATTR_SECURITY_LABEL); > > +#endif > > fsnotify_xattr(dentry); > > Put the #ifdef inside fsnotify_change() and only process > ATTR_SECURITY_LABEL if CONFIG_SECURITY.
Will do.
> > > > +#ifdef CONFIG_SECURITY > > +#define ATTR_SECURITY_LABEL 65536 > > +#endif > > I don't think there's any harm in always defining this. > >
| |