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