On Thu, 2010-09-02 at 17:16 +0900, KaiGai Kohei wrote: > (2010/08/28 12:24), KaiGai Kohei wrote: > > (2010/08/28 1:19), Eric Paris wrote: > >> On Fri, Aug 27, 2010 at 11:48 AM, Eric Paris<eparis@xxxxxxxxxxxxxx> wrote: > >>> 2010/8/27 KaiGai Kohei<kaigai@xxxxxxxxxxxxx>: > >>>> I revised the /selinux/status implementation. > >>>> > >>>> * It becomes to report 'deny_unknown'. Userspace object manager > >>>> also reference this flag to decide its behavior when the loaded > >>>> policy does not support expected object classes. > >>>> * It provided PAGE_READONLY to remap_pfn_range() as page protection > >>>> flag independent from argument of mmap(2), but it was uncommon. > >>>> I fixed to pass vma->vm_page_prot instead of the hardwired flag > >>>> according to any other implementation style. > >>>> Now it returns an error, if user tries to map /selinux/status as > >>>> writable pages. > >>> > >>> I really hate blowing 4k of memory on every system to show 40 bytes of > >>> data on just a few systems. Is there any change we could allocate the > >>> page the first time it is needed rather that at boot? I know compared > >>> to the size of policy and other memory usage in SELinux it's odd for > >>> me to complain, but I've decided to get on a reduction if possible > >>> kick. > >>> > >>> Only other comment is that __initcall() is deprecated and we are > >>> supposed to use device_initcall() now. > >>> > >>> If you plan to use it, I'll ack if you change both of those things.... > >> > >> actually if you move to dynamic allocation of the status page and use > >> static DEFINE_SPINLOCK instead of static spinlock_t you can get rid of > >> the __init() code altogether.... > >> > > > > I revised the patch. > > It was changed the selinux_kernel_page being allocated at the first time > > when application tries to reference the /selinux/status. > > At the same time, it declares selinux_status_lock using DEFINE_MUTEX(), > > so whole of the __init section has gone. > > > > In addition, I changed first member of the selinux_kernel_status from > > 'length' to 'version', because sizeof(struct ...) is aligned to 64bit > > boundary (24bytes) on x86_64 system, although it is actually 20bytes. > > If we want to add a 32bit member in the future, 'length' may not inform > > applications enough. > > > How about getting the feature? > Although I've not found out this idea for a long time, it is quite helpful > feature to implement SE-PostgreSQL (and other upcoming userspace object > managers) in less invasive way. > > I fixed up two minor points in the patch, as follows: > * The 4K of status page becomes allocated at the file_operations::open() > method, because it seems to me a bit unnatural that either read() or > mmap() fails due to memory allocation error. > * I forgot to eliminate an unnecessary declaration of extern variable. > > Signed-off-by: KaiGai Kohei <kaigai@xxxxxxxxxxxxx> Sorry I was on vacation for the last 2 weeks. I'm happy with it so: Acked-by: Eric Paris <eparis@xxxxxxxxxx> As to one comment in the code: + * In addition, application should also checks the sequence number at + * tail of the access control routine. If it is changed from the value + * on the head, it means kernel status was changed under processing the + * routine. In this case, application should repeat to run the routine + * from head, but we expect it is much rare case. Is this just to eliminate the race where: userspace checks seqno kernel loads new policy userspace avc responds to request but the response 'should' have been different thanks to the policy load that the userspace object manager didn't hear? I claim there is no race here, since he request had to be made before the policy load, even if the userspace AVC didn't respond until after the load. I just don't see where closing that 'race' actually improves security. The application is going to actually do the privileged operation some time after the access check. So what we have today seems just as good when we consider the set of operations userspace checks seqno userspace avc respond to request userspace checks seqno kernel loads new policy userspace actually performs priv operation. Doesn't change the patch at all, I just think it makes your test case looks a lot worse than it needs to. I wonder what your results are when you use the separate thread method which doesn't have anything expect 1,000,000 calls to avc_has_perm(). Doesn't matter just me wondering out loud and wondering if there is any reason to use the separate thread implementation if this is just about as fast. -Eric -- 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.
