Re: [v9.1] sepgsql - userspace access vector cache

On 2011-07-14 21:46, Kohei KaiGai wrote:
> Sorry, the syscache part was mixed to contrib/sepgsql part
> in my previous post.
> Please see the attached revision.
>
> Although its functionality is enough simple (it just reduces
> number of system-call invocation), its performance
> improvement is obvious.
> So, I hope someone to volunteer to review these patches.
This is a review of the two userspace access vector cache patches for 
SE-PostgreSQL. Proofreading the source I've seen mostly cosmetic things. 
Though I have a few questions, I think the overal shape of the patch is 
good enough to mark it ready for comitter.

Remarks:

* The patches apply cleanly, compile cleanly on Fedora 15. It depends on 
libselinux-2.0.99, and that is checked on by the configure script: good.

* I run SELECT sepgsql_restorecon(NULL) and saw comparable numbers in 
speed gain and also backend process memory increase as indicated by 
KaiGai-san. The vmsize without the patch increases from running 
restorecon 3864kB, with the patch is increases 8160kB, a difference of 
~5MB. Where this change comes from is unclear to me. The avc_cache holds 
only 7 entries, and the avc memory context stats indicate it's only at 
8kB. Investigation into the SECLABELOID syscache revealed that even when 
that cache was set to a #buckets of 2, still after restorecon() the 
backend's vmsize increased about the ~5MB more.

* The size of SECLABELOID is currently set to 2048, which is equal to 
sizes of the pg_proc and pg_attribute caches and hence makes sense. The 
initial contents of pg_seclabel is 3346 entries. Just to get some idea 
what the cachesize means for restorecon performance I tested some 
different values (debugging was on). Until a size of 256, restorecon had 
comparable performance. Small drop from ~415 to ~425 from 128 to 64. 
Cache of 32 had ~435ms performance. Cache of 2 had 680ms. Without 
debugging symbols, I also got a slightly better performance on the 
restorecon call with a 1024 syscache size. This might be something to 
tweak in later versions, when there is more experience what this cache 
size means for performance on real databases.

* The cache is called userspace, presumably because it isn't cached in 
shared memory? If sepgsql is targeted at installations with many 
different kinds of clients (having different subject contexts), or 
access different objects, this is a good choice.

* Checked that the cache keeps working after errors, ok.

* uavc.c defines some static variables like lru_hint, threshold and 
unlabeled. It would be nice to have a small comment with each one that 
says what the variable represents (like is done for the avc_cache struct 
members right above it)

* I had to google SELINUX_AVD_FLAGS_PERMISSIVE to understand what was 
going on. Since the goal why it is recorded a domain is permissive, is 
to prevent flooding the log, maybe renaming avc_cache.permissive into 
avc_cache.log_violations_once would explain itself.

* selinux_status_open() is called and it's man page mentions 
selinux_status_close(). It currently unmaps memory and closes a file 
descriptor, which is done on process exit anyway. I don't have enough 
experience with these kinds of system calls to have a feeling whether it 
might change in the future (and in such cases I'd have added a call to 
selinux_status_close() on proc_exit, just to be on the safe side).

* sepgsel_avs_unlabeled(). I was confused a bit because objects can have 
a label 'unlabeled', and the comments use wording like 'when unlabeled'. 
Does this mean with no label, or with a label 'unlabeled'?

* sepgsql_avc_compute(): the large comment in the beginning is hard to 
follow since sentences seem to have been a bit mixed up.

* question: why is ucontext stored in avc_cache?

* there is a new guc parameter for the user: avc_threshold, which is 
also documented. My initial question after reading the description was: 
what are the 'items' and how can I see current usage / what are numbers 
to expect? Without that knowledge any educated change of that parameter 
would be impossible. Would it be possible to remove this guc?

* avc_init has magic numbers 384, 4096. Maybe these can be defines (with 
a small comment what it is?)

* Finally, IMO the call sites, e.g. check_relation_priviliges, have 
improved in readability with this patch.

-- 

Yeb Havinga
http://www.mgrid.net/
Mastering Medical Data