On Thu, 2009-02-26 at 10:49 -0500, Stephen Smalley wrote:
> On Wed, 2009-02-25 at 16:34 -0500, Eric Paris wrote:
> > On Wed, 2009-02-25 at 16:14 -0500, Stephen Smalley wrote:
> > > On Wed, 2009-02-25 at 15:50 -0500, Eric Paris wrote:
> > > > So the X people here at Red Hat complained the other day that they have
> > > > to do an open, write, read, close very very often on /selinux/create.
> > > > They'd like to cut the number of syscalls down. Even if the open and
> > > > close are fast, they are still syscalls that still take time and still
> > > > provide maximum limits on the operations per second they can do. (I
> > > > think ajax said he was estimating it at 10000/sec, but I don't remember
> > > > the math or even if it was reasonable)
> > > >
> > > > We've got 2 choices. We could reduce the number of syscalls to 3 by
> > > > adding another operation, maybe an ioctl, that would reset the
> > > > transaction. X could keep the file open indeffinitely and instead use
> > > > an open, write, read, ioctl, write read, ioctl, etc callpath.
> > > >
> > > > We could also reduce the number of syscalls needed to 2. We could just
> > > > say that after a full read we reset the transaction. So the process
> > > > would look like open, write, read, write, read, write, read, etc....
> > > >
> > > > Does anyone see a problem with going to the 2 syscall kernel interface?
> > > > Userspace which uses the library shouldn't even notice since the library
> > > > (I believe) takes care of the open, write, read, close chain and would
> > > > continue to work properly.
> > > >
> > > > If the X people can really show that this call chain makes a big
> > > > performance difference I'm sure we could come up with other ways to
> > > > speed things up (crazy things that sds would hate like exposing internal
> > > > sids to userspace and passing those back and forth across a new boundary
> > > > for fast in kernel operations. But for now, just stopping opening and
> > > > closing a file 1000s of times a second seems like an easy, obvious, and
> > > > reasonable win.
> > >
> > > Wouldn't it be simpler and more efficient to just start caching the
> > > results of security_compute_create in the AVC so that
> > > avc_compute_create() will get most answers from the cache, just like
> > > avc_has_perm()?
> >
> > I like how you respond to my "future work" part first :) I'll take a
> > look at caching these results but before I put much into what I can't
> > clearly say would be a win, I'm going to have to get some read perf data
> > showing where in selinux code we are spending time....
>
> I think you'll get the biggest win from having the userspace AVC cache
> the compute_create results. Shouldn't be hard.
>
> > > BTW, as I recall, the transaction ops don't actually require a separate
> > > write() + read() pair; they can be combined into a single call that does
> > > both request + response transfer.
> >
> > What syscall would we use to send the info pack and forth? don't say
> > ioctl, don't say ioctl, don't say ioctl......
>
> No, just read(). Something along these lines would allow a single
> read() call to both submit the request and collect the response, as long
> as the buffer is large enough for both.
>
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 01ec6d2..9a5d2a8 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -544,9 +544,20 @@ static ssize_t selinux_transaction_write(struct file *file, const char __user *b
> return rv;
> }
>
> +static ssize_t selinux_transaction_read(struct file *file, char __user *buf, size_t size, loff_t *pos)
> +{
> + if (!file->private_data) {
> + ssize_t rv = selinux_transaction_write(file, buf, size, pos);
> + if (rv < 0)
> + return rv;
> + }
> + return simple_transaction_read(file, buf, size, pos);
> +
> +}
> +
> static const struct file_operations transaction_ops = {
> .write = selinux_transaction_write,
> - .read = simple_transaction_read,
> + .read = selinux_transaction_read,
> .release = simple_transaction_release,
> };
>
> Some might object to this however on the grounds that read(2) isn't
> supposed to do anything with the initial contents of buf.
>
> nfsctl does something similar to allow a read() without a prior write()
> to get status information but it only does a 0-byte write so it isn't
> actually using the buffer contents.
>
> I'm not sure though that we'd gain much in performance from this.
> Eliminating the need for open/close on each transaction is likely to be
> more significant as you say. I'd start with the userspace AVC though
> because that offers the best situation - no need for any syscalls when
> the result has previously been computed.
So last night I actually coded up a single call version with ioctl (I
thought read() had buf declared const, but I guess not), that didn't
require close/open but now that I understand what you meant, I agree,
the fix is in userspace. I coded that up last night, and would even
test it if I could find a machine with F11 that I could destroy.
Someday if there is really a reason to believe the kernel should be
updated maybe I'll pull that patch back out.
Thanks.
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@xxxxxxxxxxxxx with
the words "unsubscribe selinux" without quotes as the message.
