Quoting Stephen Smalley (sds@xxxxxxxxxxxxx):
> On Fri, 2007-10-26 at 10:02 -0500, Serge E. Hallyn wrote:
> > Quoting David P. Quigley (dpquigl@xxxxxxxxxxxxx):
> > > On Thu, 2007-10-25 at 19:02 -0500, Serge E. Hallyn wrote:
> > > > Quoting David P. Quigley (dpquigl@xxxxxxxxxxxxx):
> > > > > static int task_alloc_security(struct task_struct *task)
> > > > > @@ -2423,14 +2397,22 @@ static const char *selinux_inode_xattr_getsuffix(void)
> > > > > *
> > > > > * Permission check is handled by selinux_inode_getxattr hook.
> > > > > */
> > > > > -static int selinux_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
> > > > > +static int selinux_inode_getsecurity(const struct inode *inode,
> > > > > + const char *name,
> > > > > + void **buffer)
> > > > > {
> > > > > + u32 size;
> > > > > + int error;
> > > > > struct inode_security_struct *isec = inode->i_security;
> > > > >
> > > > > if (strcmp(name, XATTR_SELINUX_SUFFIX))
> > > > > return -EOPNOTSUPP;
> > > > >
> > > > > - return selinux_getsecurity(isec->sid, buffer, size);
> > > > > + error = security_sid_to_context(isec->sid, (char **)buffer, &size);
> > > >
> > > > The only other downside I see here is that when the user just passes in
> > > > NULL for a buffer, security_sid_to_context() will still
> > > > kmalloc the buffer only to have it immediately freed by
> > > > xattr_getsecurity() through release_secctx(). I trust that isn't seen
> > > > as any major performance impact?
> > >
> > > There is no way to avoid this in the SELinux case. SELinux doesn't store
> > > the sid to string mapping directly. Rather it takes the sid and then
> > > builds the string from fields in the related structure. So regardless
> > > this data is being allocated internally. The only issue I potentially
> > > see is that if someone passes in null expecting just to get the length
> > > we are actually returning a value. However we are changing the semantics
> > > of the function so the old semantics are no longer valid.
> >
> > Hmm? Which semantics are no longer valid?
> >
> > You're changing the semantincs of the in-kernel API, but userspace can
> > still send in NULL to query the length of the buffer needed. So if
> > userspace does two getxattrs, one to get the length, then another to get
> > the value, selinux will be kmallocing twice.
> >
> > For a file manager doing a listing on a huge directory and wanting to
> > list the selinux type, i could see that being a performance issue. Of
> > course they could get around that by sending in a 'reasonably large'
> > buffer for a first try.
>
> That's what current userland does. libselinux always tries with an
> initial buffer first (and usually succeeds), thereby avoiding the second
> call to getxattr in the common case.
Ok - I figured for doing thousands of these in one directory listing
that could waste quite a bit of memory, but since (as i check) selinux
has always done a kmalloc for every getsecurity call, I guess it's a
fair tradeoff
thanks,
-serge
-
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
