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.
We are changing the semantics of the LSM hook. From a memory allocation
stand point the only difference is who allocates the buffer. Before this
patch we would allocate one outside of the LSM hook and then fill it
(even if null were passed). Since the LSM hook is in a better position
to accurately create this buffer we pushed the allocation to it.
> 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.
I'd agree but these allocations have always been happening and no one
has seen a problem with it. There should be the same number of
allocations as before the question is where are they done.
>
> -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
