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

The attached patch adds contrib/sepgsql a cache mechanism for access
control decision of SELinux. It shall reduce the total number of
system call invocations to improve the performance on its access
controls.

In the current implementation, the sepgsql always raises a query to
SELinux in-kernel. However, same answer shall be returned for some
pair of security labels and object class, unless the security policy
got reloaded.
It is a situation caching mechanism works well. Of course, we don't
assume the security policy is reloaded so frequently.

I tried to measure the performance to run sepgsql_restorecon(NULL)
that is used to assign initial labels of schemas, relations, columns
and procedures. It also invokes massive number of "relabelfrom" and
"relabelto" permission checks.

$ time -p psql -c 'SELECT sepgsql_restorecon(NULL);' postgres

without patch
real 2.73
real 2.70
real 2.72
real 2.67
real 2.68

with patch
real 0.67
real 0.61
real 0.63
real 0.63
real 0.63

The improvement is obvious.

From the viewpoint of implementation, this patch replaces
sepgsql_check_perms() by sepgsql_avc_check_perms(), from non-cache
interface to cached interface.
Every cached items are hashed using a pair of security labels and
object class, so, even if different objects have same security label,
system call invocation shall happen only once for an identical
combination.

The only modification by this patch to the core routine is a new
syscache for pg_seclabel system catalog. The SECLABELOID enables to
reference security label of the object using syscache interface.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Thu, Jun 9, 2011 at 3:59 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> The only modification by this patch to the core routine is a new
> syscache for pg_seclabel system catalog. The SECLABELOID enables to
> reference security label of the object using syscache interface.

I believe we decided against that previously on the grounds that we
don't want to add syscaches that might get really really big. In
particular, there could be a LOT of labelled large objects floating
around.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

* Kohei KaiGai (kaigai(at)kaigai(dot)gr(dot)jp) wrote:
> The only modification by this patch to the core routine is a new
> syscache for pg_seclabel system catalog. The SECLABELOID enables to
> reference security label of the object using syscache interface.

Perhaps I'm missing it, but.. why is this necessary to implement such a
cache? Also, I thought the SELinux userspace libraries provided a cache
solution? This issue is hardly unique to SELinux in PostgreSQL...

THanks,

Stephen

2011/6/9 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Thu, Jun 9, 2011 at 3:59 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> The only modification by this patch to the core routine is a new
>> syscache for pg_seclabel system catalog. The SECLABELOID enables to
>> reference security label of the object using syscache interface.
>
> I believe we decided against that previously on the grounds that we
> don't want to add syscaches that might get really really big.  In
> particular, there could be a LOT of labelled large objects floating
> around.
>
(Sorry, I missed to Cc: pgsql-hackers, so send again)

As long as we use syscache mechanism to hold security label of
relation or other cached objects, do you think it cause no troubles?

If so, it may be a good idea to distinct cases when we try to reference
the security label of blobs and others.
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

2011/6/9 Stephen Frost <sfrost(at)snowman(dot)net>:
> * Kohei KaiGai (kaigai(at)kaigai(dot)gr(dot)jp) wrote:
>> The only modification by this patch to the core routine is a new
>> syscache for pg_seclabel system catalog. The SECLABELOID enables to
>> reference security label of the object using syscache interface.
>
> Perhaps I'm missing it, but.. why is this necessary to implement such a
> cache?  Also, I thought the SELinux userspace libraries provided a cache
> solution?  This issue is hardly unique to SELinux in PostgreSQL...
>
I'm concerned about its interface, although it might be suitable for
X-Windows...

Its avc interface identifies security context using a pointer of
malloc()'ed cstring.
In our case, we need to look up this security context on the hash managed by
libselinux using the result of syscache lookup. It is quite nonsense.

In addition, avc of libselinux confirms whether the security policy is reloaded
for each avc lookup, unless we launch a system state monitoring thread.
But, it is not a suitable design to launch a worker thread for each
pgsql backend.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Thu, Jun 9, 2011 at 12:39 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> 2011/6/9 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Thu, Jun 9, 2011 at 3:59 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>> The only modification by this patch to the core routine is a new
>>> syscache for pg_seclabel system catalog. The SECLABELOID enables to
>>> reference security label of the object using syscache interface.
>>
>> I believe we decided against that previously on the grounds that we
>> don't want to add syscaches that might get really really big.  In
>> particular, there could be a LOT of labelled large objects floating
>> around.
>>
> (Sorry, I missed to Cc: pgsql-hackers, so send again)
>
> As long as we use syscache mechanism to hold security label of
> relation or other cached objects, do you think it cause no troubles?

Maybe, but why do we need it?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Jun 9, 2011 at 12:54 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> 2011/6/9 Stephen Frost <sfrost(at)snowman(dot)net>:
>> * Kohei KaiGai (kaigai(at)kaigai(dot)gr(dot)jp) wrote:
>>> The only modification by this patch to the core routine is a new
>>> syscache for pg_seclabel system catalog. The SECLABELOID enables to
>>> reference security label of the object using syscache interface.
>>
>> Perhaps I'm missing it, but.. why is this necessary to implement such a
>> cache?  Also, I thought the SELinux userspace libraries provided a cache
>> solution?  This issue is hardly unique to SELinux in PostgreSQL...
>>
> I'm concerned about its interface, although it might be suitable for
> X-Windows...
>
> Its avc interface identifies security context using a pointer of
> malloc()'ed cstring.
> In our case, we need to look up this security context on the hash managed by
> libselinux using the result of syscache lookup. It is quite nonsense.

So you're going to depend on the syscache not to move the pointers
around? Yikes.

> In addition, avc of libselinux confirms whether the security policy is reloaded
> for each avc lookup, unless we launch a system state monitoring thread.
> But, it is not a suitable design to launch a worker thread for each
> pgsql backend.

I thought there was something you could mmap() into each backend...?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

2011/6/9 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Thu, Jun 9, 2011 at 12:39 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> 2011/6/9 Robert Haas <robertmhaas(at)gmail(dot)com>:
>>> On Thu, Jun 9, 2011 at 3:59 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>>> The only modification by this patch to the core routine is a new
>>>> syscache for pg_seclabel system catalog. The SECLABELOID enables to
>>>> reference security label of the object using syscache interface.
>>>
>>> I believe we decided against that previously on the grounds that we
>>> don't want to add syscaches that might get really really big.  In
>>> particular, there could be a LOT of labelled large objects floating
>>> around.
>>>
>> (Sorry, I missed to Cc: pgsql-hackers, so send again)
>>
>> As long as we use syscache mechanism to hold security label of
>> relation or other cached objects, do you think it cause no troubles?
>
> Maybe, but why do we need it?
>
Of course, I'd like to look up security label of the referenced object with
smallest cost as possible as we can.

Here is two level lookups.
The first is from object identifiers to security label; it can be boosted
using syscache mechanism. The second is from security labels to
access control decision; it can be boosted using userspace avc.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

2011/6/9 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Thu, Jun 9, 2011 at 12:54 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> 2011/6/9 Stephen Frost <sfrost(at)snowman(dot)net>:
>>> * Kohei KaiGai (kaigai(at)kaigai(dot)gr(dot)jp) wrote:
>>>> The only modification by this patch to the core routine is a new
>>>> syscache for pg_seclabel system catalog. The SECLABELOID enables to
>>>> reference security label of the object using syscache interface.
>>>
>>> Perhaps I'm missing it, but.. why is this necessary to implement such a
>>> cache?  Also, I thought the SELinux userspace libraries provided a cache
>>> solution?  This issue is hardly unique to SELinux in PostgreSQL...
>>>
>> I'm concerned about its interface, although it might be suitable for
>> X-Windows...
>>
>> Its avc interface identifies security context using a pointer of
>> malloc()'ed cstring.
>> In our case, we need to look up this security context on the hash managed by
>> libselinux using the result of syscache lookup. It is quite nonsense.
>
> So you're going to depend on the syscache not to move the pointers
> around?  Yikes.
>
No. It is nonsense, because cached security label of table X and table Y
are allocated on different address, so it eventually invokes system calls
for each tables, even if these tables have identical security labels.

>> In addition, avc of libselinux confirms whether the security policy is reloaded
>> for each avc lookup, unless we launch a system state monitoring thread.
>> But, it is not a suitable design to launch a worker thread for each
>> pgsql backend.
>
> I thought there was something you could mmap() into each backend...?
>
The selinux_status_open() maps a status page of selinux that allows applications
to reference a flag to show whether the policy was reloaded, or not, without
system call invocations. This function is called when sepgsql
initializes itself,
then it shall be inherited to child processes.
(Please note that avc of libselinux does not use this feature yet...)

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Thu, Jun 9, 2011 at 3:09 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> Here is two level lookups.
> The first is from object identifiers to security label; it can be boosted
> using syscache mechanism. The second is from security labels to
> access control decision; it can be boosted using userspace avc.

OK. Let's have two separate patches, then.

Thinking a bit more about the issue of adding a syscache, I suspect
it's probably OK to use that mechanism rather than inventing something
more complicated that can kick out entries on an LRU basis. Even if
you accessed a few tens of thousands of entries, the cache shouldn't
grow to more than a few megabytes. And that's presumably going to be
rare, and could happen with other types of objects (such as tables)
using the existing system caches. So I guess it's probably premature
optimization to worry about the issue now.

I would, however, like to see some performance results indicating how
much the cache helps, and how much memory it eats up in the process.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

2011/6/12 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Thu, Jun 9, 2011 at 3:09 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> Here is two level lookups.
>> The first is from object identifiers to security label; it can be boosted
>> using syscache mechanism. The second is from security labels to
>> access control decision; it can be boosted using userspace avc.
>
> OK.  Let's have two separate patches, then.
>
> Thinking a bit more about the issue of adding a syscache, I suspect
> it's probably OK to use that mechanism rather than inventing something
> more complicated that can kick out entries on an LRU basis.  Even if
> you accessed a few tens of thousands of entries, the cache shouldn't
> grow to more than a few megabytes.  And that's presumably going to be
> rare, and could happen with other types of objects (such as tables)
> using the existing system caches.  So I guess it's probably premature
> optimization to worry about the issue now.
>
> I would, however, like to see some performance results indicating how
> much the cache helps, and how much memory it eats up in the process.
>
The attached patches are separated ones.

The smaller one adds a new SECLABELOID syscache, and modifies
GetSecurityLabel() that uses syscache interface when supplied
ObjectAddress does not point to LargeObjectRelationId.

The larger one is control/sepgsql part; that adds cache mechanism
of access control decision.

I tried to measure performance with/without these patches.
The avc improved the cost to make access control decision
in all cases. In addition, syscache feature also improved
performance when pg_seclabel is not heavily updated.

* Test 1. time to run SELECT sepgsql_restorecon(NULL)
selinux | SECLABELOID syscache
avc | without | with
---------+---------+----------
without | 2.63[s] | 2.81[s]
---------+---------+----------
with | 0.60[s] | 0.59[s]
---------+---------+----------

The selinux avc efficiently improved performance, however,
the effect of syscache is unclear because this workload also
invokes updates of system catalog, so it might be a bottle-neck
of the benchmark.

* Test 2. time to run 50,000 of SELECT from empty tables
selinux | SECLABELOID syscache
avc | without | with
---------+-----------------------
without | 185.59[s] | 178.38[s]
---------+-----------------------
with | 23.58[s] | 21.79[s]
---------+-----------------------

So, I also measured the performance of read-only queries.
The SECLABELOID syscache also improved performance
nearby 10%, although it contains whole of the query parsing,
optimizing and executing.

Regarding to memory consumption, we don't worry about
consumptions by uavc, because it caches an entry for a pair
of security labels and object class. A particular security label
tends to be shared by large number of objects.
For syscache, length of a typical security label in selinux is
less than 64 bytes. If we assume an entry consume 128bytes
including Oid pairs or pointers, its consumption is 128KBytes
per 1,000 of tables or others.
(Do we have a way to confirm syscache status?)

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Mon, Jun 13, 2011 at 7:51 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> For syscache, length of a typical security label in selinux is
> less than 64 bytes. If we assume an entry consume 128bytes
> including Oid pairs or pointers, its consumption is 128KBytes
> per 1,000 of tables or others.
> (Do we have a way to confirm syscache status?)

I was thinking you might start a new session, SELECT pg_backendd_pid()
to get the PID, use top/ps to get its memory usage; then do a bunch of
stuff and see how much it's grown. The difference between how much it
grows with and without the patch is the amount of additional memory
the patch consumes.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

2011/6/13 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Mon, Jun 13, 2011 at 7:51 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> For syscache, length of a typical security label in selinux is
>> less than 64 bytes. If we assume an entry consume 128bytes
>> including Oid pairs or pointers, its consumption is 128KBytes
>> per 1,000 of tables or others.
>> (Do we have a way to confirm syscache status?)
>
> I was thinking you might start a new session, SELECT pg_backendd_pid()
> to get the PID, use top/ps to get its memory usage; then do a bunch of
> stuff and see how much it's grown.  The difference between how much it
> grows with and without the patch is the amount of additional memory
> the patch consumes.
>
I checked memory consumption of the backend with / without
patches. Because sepgsql_restorecon() tries to reset security
label of all the schemas, relations, columns and procedures,
an execution of this function is suitable to emphasize differences
between two cases in maximum.

The results shows us about 3MB of additional consumption
in VmRSS, even if it caches all the security label of the objects
being created in the default (3331 entries).

* without patches before/after sepgsql_restorecon()

VmPeak: 150812 kB -> 170864 kB
VmSize: 150804 kB -> 154712 kB
VmLck: 0 kB -> 0kB
VmHWM: 3800 kB -> 22248 kB
VmRSS: 3800 kB -> 10620 kB
VmData: 1940 kB -> 5820 kB
VmStk: 196 kB -> 196 kB
VmExe: 5324 kB -> 5324 kB
VmLib: 2468 kB -> 2468 kB
VmPTE: 108 kB -> 120 kB
VmSwap: 0 kB -> 0kB

* with patches before/after sepgsql_restorecon()
VmPeak: 150816 kB -> 175092 kB
VmSize: 150808 kB -> 158804 kB
VmLck: 0 kB -> 0 kB
VmHWM: 3868 kB -> 25956 kB
VmRSS: 3868 kB -> 13736 kB
VmData: 1944 kB -> 9912 kB
VmStk: 192 kB -> 192 kB
VmExe: 5324 kB -> 5324 kB
VmLib: 2472 kB -> 2472 kB
VmPTE: 100 kB -> 124 kB
VmSwap: 0 kB -> 0 kB

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

The attached patch re-defines pg_seclabel.provider as NameData, instead of Text,
and revert changes of catcache.c about collations; to keep consistency with the
security label support on shared objects.
All the format changes are hidden by (Get|Set)SecurityLabel(), so no
need to change
on the patch to contrib/sepgsql.

Thanks,

2011/6/13 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> 2011/6/13 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Mon, Jun 13, 2011 at 7:51 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>> For syscache, length of a typical security label in selinux is
>>> less than 64 bytes. If we assume an entry consume 128bytes
>>> including Oid pairs or pointers, its consumption is 128KBytes
>>> per 1,000 of tables or others.
>>> (Do we have a way to confirm syscache status?)
>>
>> I was thinking you might start a new session, SELECT pg_backendd_pid()
>> to get the PID, use top/ps to get its memory usage; then do a bunch of
>> stuff and see how much it's grown.  The difference between how much it
>> grows with and without the patch is the amount of additional memory
>> the patch consumes.
>>
> I checked memory consumption of the backend with / without
> patches. Because sepgsql_restorecon() tries to reset security
> label of all the schemas, relations, columns and procedures,
> an execution of this function is suitable to emphasize differences
> between two cases in maximum.
>
> The results shows us about 3MB of additional consumption
> in VmRSS, even if it caches all the security label of the objects
> being created in the default (3331 entries).
>
> * without patches before/after sepgsql_restorecon()
>
> VmPeak:   150812 kB -> 170864 kB
> VmSize:   150804 kB -> 154712 kB
> VmLck:         0 kB -> 0kB
> VmHWM:      3800 kB -> 22248 kB
> VmRSS:      3800 kB -> 10620 kB
> VmData:     1940 kB ->  5820 kB
> VmStk:       196 kB -> 196 kB
> VmExe:      5324 kB -> 5324 kB
> VmLib:      2468 kB -> 2468 kB
> VmPTE:       108 kB -> 120 kB
> VmSwap:        0 kB -> 0kB
>
> * with patches before/after sepgsql_restorecon()
> VmPeak:   150816 kB -> 175092 kB
> VmSize:   150808 kB -> 158804 kB
> VmLck:         0 kB -> 0 kB
> VmHWM:      3868 kB -> 25956 kB
> VmRSS:      3868 kB -> 13736 kB
> VmData:     1944 kB -> 9912 kB
> VmStk:       192 kB -> 192 kB
> VmExe:      5324 kB -> 5324 kB
> VmLib:      2472 kB -> 2472 kB
> VmPTE:       100 kB -> 124 kB
> VmSwap:        0 kB -> 0 kB
>
> Thanks,
> --
> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>
>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

I rebased the userspace access vector cache patch to the latest tree.

I'll describe the background of this patch because this thread has not been
active more than a week.
The sepgsql asks in-kernel selinux when it needs to make its access control
decison, so it always causes system call invocations.
However, access control decision of selinux for a particular pair of security
label is consistent as long as its security policy is not reloaded.
Thus, it is a good idea to cache access control decisions recently used in
userspace.
In addition, current GetSecurityLabel() always open pg_seclabel catalog and
scan to fetch security label of database objects, although it is a situation we
can utilize syscache mechanism.

The "uavc-syscache" patch adds a new SECLABELOID syscache.
It also redefine pg_seclabel.provide as Name, instead of Text, according to
the suggestion from Tom.
(To avoid collation conscious datatype)

The "uavc-selinux-cache" patch adds cache mechanism of contrib/sepgsql.
Its internal api to communicate with selinux (sepgsql_check_perms) was
replaced by newer sepgsql_avc_check_perms that checks cached access
control decision at first, prior to system call invocations.

The result of performance improvement is obvious.

* Test 2. time to run 50,000 of SELECT from empty tables
selinux | SECLABELOID syscache
avc | without | with
---------+-----------------------
without | 185.59[s] | 178.38[s]
---------+-----------------------
with | 23.58[s] | 21.79[s]
---------+-----------------------

I strongly hope this patch (and security label support on shared objects) to
get unstreamed in this commit-fest, because it will perform as a basis of
other upcoming features.
Please volunteer the reviewing!

Thanks,

2011/7/2 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> The attached patch re-defines pg_seclabel.provider as NameData, instead of Text,
> and revert changes of catcache.c about collations; to keep consistency with the
> security label support on shared objects.
> All the format changes are hidden by (Get|Set)SecurityLabel(), so no
> need to change
> on the patch to contrib/sepgsql.
>
> Thanks,
>
> 2011/6/13 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
>> 2011/6/13 Robert Haas <robertmhaas(at)gmail(dot)com>:
>>> On Mon, Jun 13, 2011 at 7:51 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>>> For syscache, length of a typical security label in selinux is
>>>> less than 64 bytes. If we assume an entry consume 128bytes
>>>> including Oid pairs or pointers, its consumption is 128KBytes
>>>> per 1,000 of tables or others.
>>>> (Do we have a way to confirm syscache status?)
>>>
>>> I was thinking you might start a new session, SELECT pg_backendd_pid()
>>> to get the PID, use top/ps to get its memory usage; then do a bunch of
>>> stuff and see how much it's grown.  The difference between how much it
>>> grows with and without the patch is the amount of additional memory
>>> the patch consumes.
>>>
>> I checked memory consumption of the backend with / without
>> patches. Because sepgsql_restorecon() tries to reset security
>> label of all the schemas, relations, columns and procedures,
>> an execution of this function is suitable to emphasize differences
>> between two cases in maximum.
>>
>> The results shows us about 3MB of additional consumption
>> in VmRSS, even if it caches all the security label of the objects
>> being created in the default (3331 entries).
>>
>> * without patches before/after sepgsql_restorecon()
>>
>> VmPeak:   150812 kB -> 170864 kB
>> VmSize:   150804 kB -> 154712 kB
>> VmLck:         0 kB -> 0kB
>> VmHWM:      3800 kB -> 22248 kB
>> VmRSS:      3800 kB -> 10620 kB
>> VmData:     1940 kB ->  5820 kB
>> VmStk:       196 kB -> 196 kB
>> VmExe:      5324 kB -> 5324 kB
>> VmLib:      2468 kB -> 2468 kB
>> VmPTE:       108 kB -> 120 kB
>> VmSwap:        0 kB -> 0kB
>>
>> * with patches before/after sepgsql_restorecon()
>> VmPeak:   150816 kB -> 175092 kB
>> VmSize:   150808 kB -> 158804 kB
>> VmLck:         0 kB -> 0 kB
>> VmHWM:      3868 kB -> 25956 kB
>> VmRSS:      3868 kB -> 13736 kB
>> VmData:     1944 kB -> 9912 kB
>> VmStk:       192 kB -> 192 kB
>> VmExe:      5324 kB -> 5324 kB
>> VmLib:      2472 kB -> 2472 kB
>> VmPTE:       100 kB -> 124 kB
>> VmSwap:        0 kB -> 0 kB
>>
>> Thanks,
>> --
>> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>
>>
>
>
>
> --
> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>
>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

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.

Thanks,

2011/7/11 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> I rebased the userspace access vector cache patch to the latest tree.
>
> I'll describe the background of this patch because this thread has not been
> active more than a week.
> The sepgsql asks in-kernel selinux when it needs to make its access control
> decison, so it always causes system call invocations.
> However, access control decision of selinux for a particular pair of security
> label is consistent as long as its security policy is not reloaded.
> Thus, it is a good idea to cache access control decisions recently used in
> userspace.
> In addition, current GetSecurityLabel() always open pg_seclabel catalog and
> scan to fetch security label of database objects, although it is a situation we
> can utilize syscache mechanism.
>
> The "uavc-syscache" patch adds a new SECLABELOID syscache.
> It also redefine pg_seclabel.provide as Name, instead of Text, according to
> the suggestion from Tom.
> (To avoid collation conscious datatype)
>
> The "uavc-selinux-cache" patch adds cache mechanism of contrib/sepgsql.
> Its internal api to communicate with selinux (sepgsql_check_perms) was
> replaced by newer sepgsql_avc_check_perms that checks cached access
> control decision at first, prior to system call invocations.
>
> The result of performance improvement is obvious.
>
> * Test 2. time to run 50,000 of SELECT from empty tables
>  selinux | SECLABELOID syscache
>  avc   |   without |   with
> ---------+-----------------------
>  without | 185.59[s] | 178.38[s]
> ---------+-----------------------
>  with    |  23.58[s] |  21.79[s]
> ---------+-----------------------
>
> I strongly hope this patch (and security label support on shared objects) to
> get unstreamed  in this commit-fest, because it will perform as a basis of
> other upcoming features.
> Please volunteer the reviewing!
>
> Thanks,
>
> 2011/7/2 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
>> The attached patch re-defines pg_seclabel.provider as NameData, instead of Text,
>> and revert changes of catcache.c about collations; to keep consistency with the
>> security label support on shared objects.
>> All the format changes are hidden by (Get|Set)SecurityLabel(), so no
>> need to change
>> on the patch to contrib/sepgsql.
>>
>> Thanks,
>>
>> 2011/6/13 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
>>> 2011/6/13 Robert Haas <robertmhaas(at)gmail(dot)com>:
>>>> On Mon, Jun 13, 2011 at 7:51 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>>>> For syscache, length of a typical security label in selinux is
>>>>> less than 64 bytes. If we assume an entry consume 128bytes
>>>>> including Oid pairs or pointers, its consumption is 128KBytes
>>>>> per 1,000 of tables or others.
>>>>> (Do we have a way to confirm syscache status?)
>>>>
>>>> I was thinking you might start a new session, SELECT pg_backendd_pid()
>>>> to get the PID, use top/ps to get its memory usage; then do a bunch of
>>>> stuff and see how much it's grown.  The difference between how much it
>>>> grows with and without the patch is the amount of additional memory
>>>> the patch consumes.
>>>>
>>> I checked memory consumption of the backend with / without
>>> patches. Because sepgsql_restorecon() tries to reset security
>>> label of all the schemas, relations, columns and procedures,
>>> an execution of this function is suitable to emphasize differences
>>> between two cases in maximum.
>>>
>>> The results shows us about 3MB of additional consumption
>>> in VmRSS, even if it caches all the security label of the objects
>>> being created in the default (3331 entries).
>>>
>>> * without patches before/after sepgsql_restorecon()
>>>
>>> VmPeak:   150812 kB -> 170864 kB
>>> VmSize:   150804 kB -> 154712 kB
>>> VmLck:         0 kB -> 0kB
>>> VmHWM:      3800 kB -> 22248 kB
>>> VmRSS:      3800 kB -> 10620 kB
>>> VmData:     1940 kB ->  5820 kB
>>> VmStk:       196 kB -> 196 kB
>>> VmExe:      5324 kB -> 5324 kB
>>> VmLib:      2468 kB -> 2468 kB
>>> VmPTE:       108 kB -> 120 kB
>>> VmSwap:        0 kB -> 0kB
>>>
>>> * with patches before/after sepgsql_restorecon()
>>> VmPeak:   150816 kB -> 175092 kB
>>> VmSize:   150808 kB -> 158804 kB
>>> VmLck:         0 kB -> 0 kB
>>> VmHWM:      3868 kB -> 25956 kB
>>> VmRSS:      3868 kB -> 13736 kB
>>> VmData:     1944 kB -> 9912 kB
>>> VmStk:       192 kB -> 192 kB
>>> VmExe:      5324 kB -> 5324 kB
>>> VmLib:      2472 kB -> 2472 kB
>>> VmPTE:       100 kB -> 124 kB
>>> VmSwap:        0 kB -> 0 kB
>>>
>>> Thanks,
>>> --
>>> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>
>>>
>>
>>
>>
>> --
>> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>
>>
>
>
>
> --
> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>
>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

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.

I will be able to look at this patch next week on monday and tuesday,
without wanting to raise any expectations about that time being enough
for me to say anything useful. On a longer timescale, I believe that
sepgsql is one of the most important new features of PostgreSQL and that
I want to commit myself to spend more community work on this subject.

regards,
Yeb

--

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

Hello KaiGai-san,

I've been preparing to review this patch by reading both pgsql-hackers
history on sepgsql, and also the RHEL 6 guide on SELinux this weekend,
today I installed GIT HEAD with --with-selinux on Scientific Linux 6,
developer installation, so far almost everything looking good.

These things should probably be added to the 9.1beta3 documentation branch:

1) the line with for DBNAME in ... do postgres --single etc, lacks a -D
argument and hence gives the error:
postgres does not know where to find the server configuration file.

2) there is a dependency to objects outside of the postgresql
installation tree in /etc/selinux/targeted/contexts/sepgsql_contexts,
and that file has an error that is thrown when contrib/sepgsql is executed:
/etc/selinux/targeted/contexts/sepgsql_contexts: line 33 has invalid
object type db_blobs
(same for db_language)
I found your fix for the error on a forum on oss.tresys.com, but IMHO
either the contrib/sepgsql should mention that the dependency exists and
it might contain errors for (older) reference policies, or it should
include a bugfree reference policy for sepgsql to replace the system
installed refpolicy with (and mention that in the install documentation).

3) sepgsql is currently a bit hard to find in the documentation.
www.postgresql.org website search doesn't find sepgsql and selinux only
refers to an old PostgreSQL redhat bug in 2005 on bugzilla.redhat.com. I
had to manually remember it was a contrib module. Also sepgsql isn't
linked to at the SECURITY LABEL page. At the moment I'm unsure if I have
seen all sepgsql related sgml-based documentation.

After fixing the refpolicy I proceeded with the contrib/sepgsql manual,
with the goal to get something easy done, like create a top secret table
like 'thisyearsbonusses', and a single user 'boss' and configure sepgsql
in such a way that only the boss can access the top secret table. I've
read the the contrib documentation, browsed links on the bottom of the
page but until now I don't even have a clue how to proceed. Until I do
so, I don't feel it's appropriate for me to review the avc patch.

Would you be willing to help me getting a bit started? Specific
questions are:

1) The contrib doc under DML permissions talks about 'db_table:select'
etc? What are these things? They are not labels since I do not see them
listed in the output of 'select distinct label from pg_seclabel'.

2) The regression test label.sql introduces labels with types
sepgsql_trusted_proc_exec_t, sepgsql_ro_table_t. My question is: where
are these defined? What is their meaning? Can I define my own?

3) In the examples so far I've seen unconfined_u and system_u? Can I
define my own?

Thanks,
Yeb Havinga

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.
>
> Thanks,
>
> 2011/7/11 Kohei KaiGai<kaigai(at)kaigai(dot)gr(dot)jp>:
>> I rebased the userspace access vector cache patch to the latest tree.
>>
>> I'll describe the background of this patch because this thread has not been
>> active more than a week.
>> The sepgsql asks in-kernel selinux when it needs to make its access control
>> decison, so it always causes system call invocations.
>> However, access control decision of selinux for a particular pair of security
>> label is consistent as long as its security policy is not reloaded.
>> Thus, it is a good idea to cache access control decisions recently used in
>> userspace.
>> In addition, current GetSecurityLabel() always open pg_seclabel catalog and
>> scan to fetch security label of database objects, although it is a situation we
>> can utilize syscache mechanism.
>>
>> The "uavc-syscache" patch adds a new SECLABELOID syscache.
>> It also redefine pg_seclabel.provide as Name, instead of Text, according to
>> the suggestion from Tom.
>> (To avoid collation conscious datatype)
>>
>> The "uavc-selinux-cache" patch adds cache mechanism of contrib/sepgsql.
>> Its internal api to communicate with selinux (sepgsql_check_perms) was
>> replaced by newer sepgsql_avc_check_perms that checks cached access
>> control decision at first, prior to system call invocations.
>>
>> The result of performance improvement is obvious.
>>
>> * Test 2. time to run 50,000 of SELECT from empty tables
>> selinux | SECLABELOID syscache
>> avc | without | with
>> ---------+-----------------------
>> without | 185.59[s] | 178.38[s]
>> ---------+-----------------------
>> with | 23.58[s] | 21.79[s]
>> ---------+-----------------------
>>
>> I strongly hope this patch (and security label support on shared objects) to
>> get unstreamed in this commit-fest, because it will perform as a basis of
>> other upcoming features.
>> Please volunteer the reviewing!
>>
>> Thanks,
>>
>> 2011/7/2 Kohei KaiGai<kaigai(at)kaigai(dot)gr(dot)jp>:
>>> The attached patch re-defines pg_seclabel.provider as NameData, instead of Text,
>>> and revert changes of catcache.c about collations; to keep consistency with the
>>> security label support on shared objects.
>>> All the format changes are hidden by (Get|Set)SecurityLabel(), so no
>>> need to change
>>> on the patch to contrib/sepgsql.
>>>
>>> Thanks,
>>>
>>> 2011/6/13 Kohei KaiGai<kaigai(at)kaigai(dot)gr(dot)jp>:
>>>> 2011/6/13 Robert Haas<robertmhaas(at)gmail(dot)com>:
>>>>> On Mon, Jun 13, 2011 at 7:51 AM, Kohei KaiGai<kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>>>>> For syscache, length of a typical security label in selinux is
>>>>>> less than 64 bytes. If we assume an entry consume 128bytes
>>>>>> including Oid pairs or pointers, its consumption is 128KBytes
>>>>>> per 1,000 of tables or others.
>>>>>> (Do we have a way to confirm syscache status?)
>>>>> I was thinking you might start a new session, SELECT pg_backendd_pid()
>>>>> to get the PID, use top/ps to get its memory usage; then do a bunch of
>>>>> stuff and see how much it's grown. The difference between how much it
>>>>> grows with and without the patch is the amount of additional memory
>>>>> the patch consumes.
>>>>>
>>>> I checked memory consumption of the backend with / without
>>>> patches. Because sepgsql_restorecon() tries to reset security
>>>> label of all the schemas, relations, columns and procedures,
>>>> an execution of this function is suitable to emphasize differences
>>>> between two cases in maximum.
>>>>
>>>> The results shows us about 3MB of additional consumption
>>>> in VmRSS, even if it caches all the security label of the objects
>>>> being created in the default (3331 entries).
>>>>
>>>> * without patches before/after sepgsql_restorecon()
>>>>
>>>> VmPeak: 150812 kB -> 170864 kB
>>>> VmSize: 150804 kB -> 154712 kB
>>>> VmLck: 0 kB -> 0kB
>>>> VmHWM: 3800 kB -> 22248 kB
>>>> VmRSS: 3800 kB -> 10620 kB
>>>> VmData: 1940 kB -> 5820 kB
>>>> VmStk: 196 kB -> 196 kB
>>>> VmExe: 5324 kB -> 5324 kB
>>>> VmLib: 2468 kB -> 2468 kB
>>>> VmPTE: 108 kB -> 120 kB
>>>> VmSwap: 0 kB -> 0kB
>>>>
>>>> * with patches before/after sepgsql_restorecon()
>>>> VmPeak: 150816 kB -> 175092 kB
>>>> VmSize: 150808 kB -> 158804 kB
>>>> VmLck: 0 kB -> 0 kB
>>>> VmHWM: 3868 kB -> 25956 kB
>>>> VmRSS: 3868 kB -> 13736 kB
>>>> VmData: 1944 kB -> 9912 kB
>>>> VmStk: 192 kB -> 192 kB
>>>> VmExe: 5324 kB -> 5324 kB
>>>> VmLib: 2472 kB -> 2472 kB
>>>> VmPTE: 100 kB -> 124 kB
>>>> VmSwap: 0 kB -> 0 kB
>>>>
>>>> Thanks,
>>>> --
>>>> KaiGai Kohei<kaigai(at)kaigai(dot)gr(dot)jp>
>>>>
>>>
>>>
>>> --
>>> KaiGai Kohei<kaigai(at)kaigai(dot)gr(dot)jp>
>>>
>>
>>
>> --
>> KaiGai Kohei<kaigai(at)kaigai(dot)gr(dot)jp>
>>
>
>
>
>

Yeb, Thanks for your volunteering.

2011/7/18 Yeb Havinga <yebhavinga(at)gmail(dot)com>:
> Hello KaiGai-san,
>
> I've been preparing to review this patch by reading both pgsql-hackers
> history on sepgsql, and also the RHEL 6 guide on SELinux this weekend, today
> I installed GIT HEAD with --with-selinux on Scientific Linux 6, developer
> installation, so far almost everything looking good.
>
The Scientific Linux 6 is not suitable, because its libselinux version
is a bit older
than this patch expects (libselinux-2.0.99 or later).
My recommendation is Fedora 15, instead.

> 1) the line with for DBNAME in ... do postgres --single etc, lacks a -D
> argument and hence gives the error:
> postgres does not know where to find the server configuration file.
>
OK. I intended users to adjust their own paths (including -D option),
but an explicit "-D /path/to/database" seems to me more helpful as
an example.
I'll submit a patch in a separate thread.

> 2) there is a dependency to objects outside of the postgresql installation
> tree in /etc/selinux/targeted/contexts/sepgsql_contexts, and that file has
> an error that is thrown when contrib/sepgsql is executed:
> /etc/selinux/targeted/contexts/sepgsql_contexts:  line 33 has invalid object
> type db_blobs
> (same for db_language)
> I found your fix for the error on a forum on oss.tresys.com, but IMHO either
> the contrib/sepgsql should mention that the dependency exists and it might
> contain errors for (older) reference policies, or it should include a
> bugfree reference policy for sepgsql to replace the system installed
> refpolicy with (and mention that in the install documentation).
>
It is not an error, but just a notification to inform users that
sepgsql_contexts
file contains invalid lines. It is harmless, so we can ignore them.
I don't think sepgsql.sgml should mention about this noise, because it purely
come from the problem in libselinux and refpolicy; these are external packages
from viewpoint of PostgreSQL.

> 3) sepgsql is currently a bit hard to find in the documentation.
> www.postgresql.org website search doesn't find sepgsql and selinux only
> refers to an old PostgreSQL redhat bug in 2005 on bugzilla.redhat.com. I had
> to manually remember it was a contrib module. Also sepgsql isn't linked to
> at the SECURITY LABEL page. At the moment I'm unsure if I have seen all
> sepgsql related sgml-based documentation.
>
Improvement of documentation is an issue.
The wiki.postgresql.org should be an appropriate place, maybe.

The reason why SECURITY LABEL does not point to sepgsql.sgml is
that it is general purpose infrastructure for all the upcoming label based
security mechanism, not only sepgsql.
It was our consensus in v9.1 development.

> After fixing the refpolicy I proceeded with the contrib/sepgsql manual, with
> the goal to get something easy done, like create a top secret table like
> 'thisyearsbonusses', and a single user 'boss' and configure sepgsql in such
> a way that only the boss can access the top secret table. I've read the the
> contrib documentation, browsed links on the bottom of the page but until now
> I don't even have a clue how to proceed. Until I do so, I don't feel it's
> appropriate for me to review the avc patch.
>
At least, you don't need to fix the policy stuff anything.

The point of this patch is replacement of existing mechanism (that always
asks in-kernel selinux with system-call invocation) by a smart caching
mechanism (it requires minimum number of system-call invocation) without
any user-visible changes.
So, it is not necessary to define a new policy for testing.

> Would you be willing to help me getting a bit started? Specific questions
> are:
>
> 1) The contrib doc under DML permissions talks about 'db_table:select' etc?
> What are these things? They are not labels since I do not see them listed in
> the output of 'select distinct label from pg_seclabel'.
>
The security label is something like user-id or ownership/object-acl in the
default database access controls. It checks a relationship between user-id
and ownership/object-acl of the target object. If this relationship allowed
particular actions like 'select', 'update' or others, it shall be allowed when
user requires these actions.
In similar way, 'db_table:select' is a type of action; 'select' on table object,
not an identifier of user or objects.
SELinux defines a set of allowed actions (such as 'db_table:select') between
a particular pair of security labels (such as 'staff_t' and 'sepgsql_table_t').
The pg_seclabel holds only security label of object being referenced.
So, you should see /selinux/class/db_*/perms to see list of permissions
defined in the security policy (but limited number of them are in use, now).

> 2) The regression test label.sql introduces labels with types
> sepgsql_trusted_proc_exec_t, sepgsql_ro_table_t. My question is: where are
> these defined? What is their meaning? Can I define my own?
>
The system's default security policy (selinux-policy package) defines all the
necessary labeles, and access control rules between them.
So, we never need to modify security policy to run regression test.

The sepgsql_trusted_proc_exec_t means that functions labeled with this label
is a trusted procedure. It switches security label of the user during
execution of
this function. It is a similar mechanism like SetExec or security
definer function.

The sepgsql_ro_table_t means 'read-only' tables that disallow any
writer operations
except for administrative domains.

> 3) In the examples so far I've seen unconfined_u and system_u? Can I define
> my own?
>
You can define your own policy, however, I intend to run regression test
without any modification of the default security policy.

Thanks,

> Thanks,
> Yeb Havinga
>
>
> 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.
>
> Thanks,
>
> 2011/7/11 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
>
> I rebased the userspace access vector cache patch to the latest tree.
>
> I'll describe the background of this patch because this thread has not been
> active more than a week.
> The sepgsql asks in-kernel selinux when it needs to make its access control
> decison, so it always causes system call invocations.
> However, access control decision of selinux for a particular pair of
> security
> label is consistent as long as its security policy is not reloaded.
> Thus, it is a good idea to cache access control decisions recently used in
> userspace.
> In addition, current GetSecurityLabel() always open pg_seclabel catalog and
> scan to fetch security label of database objects, although it is a situation
> we
> can utilize syscache mechanism.
>
> The "uavc-syscache" patch adds a new SECLABELOID syscache.
> It also redefine pg_seclabel.provide as Name, instead of Text, according to
> the suggestion from Tom.
> (To avoid collation conscious datatype)
>
> The "uavc-selinux-cache" patch adds cache mechanism of contrib/sepgsql.
> Its internal api to communicate with selinux (sepgsql_check_perms) was
> replaced by newer sepgsql_avc_check_perms that checks cached access
> control decision at first, prior to system call invocations.
>
> The result of performance improvement is obvious.
>
> * Test 2. time to run 50,000 of SELECT from empty tables
>  selinux | SECLABELOID syscache
>  avc   |   without |   with
> ---------+-----------------------
>  without | 185.59[s] | 178.38[s]
> ---------+-----------------------
>  with    |  23.58[s] |  21.79[s]
> ---------+-----------------------
>
> I strongly hope this patch (and security label support on shared objects) to
> get unstreamed  in this commit-fest, because it will perform as a basis of
> other upcoming features.
> Please volunteer the reviewing!
>
> Thanks,
>
> 2011/7/2 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
>
> The attached patch re-defines pg_seclabel.provider as NameData, instead of
> Text,
> and revert changes of catcache.c about collations; to keep consistency with
> the
> security label support on shared objects.
> All the format changes are hidden by (Get|Set)SecurityLabel(), so no
> need to change
> on the patch to contrib/sepgsql.
>
> Thanks,
>
> 2011/6/13 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
>
> 2011/6/13 Robert Haas <robertmhaas(at)gmail(dot)com>:
>
> On Mon, Jun 13, 2011 at 7:51 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>
> For syscache, length of a typical security label in selinux is
> less than 64 bytes. If we assume an entry consume 128bytes
> including Oid pairs or pointers, its consumption is 128KBytes
> per 1,000 of tables or others.
> (Do we have a way to confirm syscache status?)
>
> I was thinking you might start a new session, SELECT pg_backendd_pid()
> to get the PID, use top/ps to get its memory usage; then do a bunch of
> stuff and see how much it's grown.  The difference between how much it
> grows with and without the patch is the amount of additional memory
> the patch consumes.
>
> I checked memory consumption of the backend with / without
> patches. Because sepgsql_restorecon() tries to reset security
> label of all the schemas, relations, columns and procedures,
> an execution of this function is suitable to emphasize differences
> between two cases in maximum.
>
> The results shows us about 3MB of additional consumption
> in VmRSS, even if it caches all the security label of the objects
> being created in the default (3331 entries).
>
> * without patches before/after sepgsql_restorecon()
>
> VmPeak:   150812 kB -> 170864 kB
> VmSize:   150804 kB -> 154712 kB
> VmLck:         0 kB -> 0kB
> VmHWM:      3800 kB -> 22248 kB
> VmRSS:      3800 kB -> 10620 kB
> VmData:     1940 kB ->  5820 kB
> VmStk:       196 kB -> 196 kB
> VmExe:      5324 kB -> 5324 kB
> VmLib:      2468 kB -> 2468 kB
> VmPTE:       108 kB -> 120 kB
> VmSwap:        0 kB -> 0kB
>
> * with patches before/after sepgsql_restorecon()
> VmPeak:   150816 kB -> 175092 kB
> VmSize:   150808 kB -> 158804 kB
> VmLck:         0 kB -> 0 kB
> VmHWM:      3868 kB -> 25956 kB
> VmRSS:      3868 kB -> 13736 kB
> VmData:     1944 kB -> 9912 kB
> VmStk:       192 kB -> 192 kB
> VmExe:      5324 kB -> 5324 kB
> VmLib:      2472 kB -> 2472 kB
> VmPTE:       100 kB -> 124 kB
> VmSwap:        0 kB -> 0 kB
>
> Thanks,
> --
> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>
>
>
>
> --
> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>
>
>
>
> --
> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>
>
>
>
>
>
>
>
>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Mon, Jul 18, 2011 at 4:21 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> 3) sepgsql is currently a bit hard to find in the documentation.
>> www.postgresql.org website search doesn't find sepgsql and selinux only
>> refers to an old PostgreSQL redhat bug in 2005 on bugzilla.redhat.com. I had
>> to manually remember it was a contrib module. Also sepgsql isn't linked to
>> at the SECURITY LABEL page. At the moment I'm unsure if I have seen all
>> sepgsql related sgml-based documentation.
>>
> Improvement of documentation is an issue.
> The wiki.postgresql.org should be an appropriate place, maybe.
>
> The reason why SECURITY LABEL does not point to sepgsql.sgml is
> that it is general purpose infrastructure for all the upcoming label based
> security mechanism, not only sepgsql.
> It was our consensus in v9.1 development.

Actually, I think it's that way mostly because we committed the
SECURITY LABEL stuff first. I'd be in favor of adding some kind of
cross-link.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

2011/7/18 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Mon, Jul 18, 2011 at 4:21 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>> 3) sepgsql is currently a bit hard to find in the documentation.
>>> www.postgresql.org website search doesn't find sepgsql and selinux only
>>> refers to an old PostgreSQL redhat bug in 2005 on bugzilla.redhat.com. I had
>>> to manually remember it was a contrib module. Also sepgsql isn't linked to
>>> at the SECURITY LABEL page. At the moment I'm unsure if I have seen all
>>> sepgsql related sgml-based documentation.
>>>
>> Improvement of documentation is an issue.
>> The wiki.postgresql.org should be an appropriate place, maybe.
>>
>> The reason why SECURITY LABEL does not point to sepgsql.sgml is
>> that it is general purpose infrastructure for all the upcoming label based
>> security mechanism, not only sepgsql.
>> It was our consensus in v9.1 development.
>
> Actually, I think it's that way mostly because we committed the
> SECURITY LABEL stuff first.  I'd be in favor of adding some kind of
> cross-link.
>
Hmm. OK, I'll submit a patch to update this documentation stuff tomorrow,
including an explicit -D option as Yeb mentioned.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On 2011-07-18 22:21, Kohei KaiGai wrote:
> The Scientific Linux 6 is not suitable, because its libselinux version
> is a bit older
> than this patch expects (libselinux-2.0.99 or later).
> My recommendation is Fedora 15, instead.
Installing right now, thanks for the heads up!

>> /etc/selinux/targeted/contexts/sepgsql_contexts: line 33 has invalid object
>> type db_blobs
> It is not an error, but just a notification to inform users that
> sepgsql_contexts
> file contains invalid lines. It is harmless, so we can ignore them.
> I don't think sepgsql.sgml should mention about this noise, because it purely
> come from the problem in libselinux and refpolicy; these are external packages
> from viewpoint of PostgreSQL.
This is in contradiction with the current phrase in the documentation
that's right after the sepgsql.sql loading: "If the installation process
completes without error, you can now start the server normally". IMHO if
there are warnings that can be ignored, it would limit confusion for
sepgsql users if the documentation would say it at this point, e.g. "If
the installation process completes without error, you can now start the
server normally. Warnings from errors in sepgsql_contexts, a file
external to PostgreSQL, are harmless and can be ignored."

> The point of this patch is replacement of existing mechanism<...>
> So, it is not necessary to define a new policy for testing.
Thanks for elaborating on this.
> The security label is something like user-id or ownership/object-acl in the
> default database access controls. It checks a relationship between user-id
> and ownership/object-acl of the target object. If this relationship allowed
> particular actions like 'select', 'update' or others, it shall be allowed when
> user requires these actions.
> In similar way, 'db_table:select' is a type of action; 'select' on table object,
> not an identifier of user or objects.
> SELinux defines a set of allowed actions (such as 'db_table:select') between
> a particular pair of security labels (such as 'staff_t' and 'sepgsql_table_t').
> The pg_seclabel holds only security label of object being referenced.
> So, you should see /selinux/class/db_*/perms to see list of permissions
> defined in the security policy (but limited number of them are in use, now).
> The system's default security policy (selinux-policy package) defines all the
> necessary labeles, and access control rules between them.
> So, we never need to modify security policy to run regression test.
>
> The sepgsql_trusted_proc_exec_t means that functions labeled with this label
> is a trusted procedure. It switches security label of the user during
> execution of
> this function. It is a similar mechanism like SetExec or security
> definer function.
>
> The sepgsql_ro_table_t means 'read-only' tables that disallow any
> writer operations
> except for administrative domains.
> You can define your own policy, however, I intend to run regression test
> without any modification of the default security policy.

Thank you for this clarification. I have some ideas of things that if
they were in the documentation they'd helped me. Instead of seeking
agreement on each item, I propose that I gather documentation additions
in a patch later after the review, and leave it up to you guys whether
to include them or not.

regards,
Yeb
--

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

> >> /etc/selinux/targeted/contexts/sepgsql_contexts: line 33 has invalid object
> >> type db_blobs
> > It is not an error, but just a notification to inform users that
> > sepgsql_contexts
> > file contains invalid lines. It is harmless, so we can ignore them.
> > I don't think sepgsql.sgml should mention about this noise, because it purely
> > come from the problem in libselinux and refpolicy; these are external packages
> > from viewpoint of PostgreSQL.
> This is in contradiction with the current phrase in the documentation
> that's right after the sepgsql.sql loading: "If the installation process
> completes without error, you can now start the server normally". IMHO if
> there are warnings that can be ignored, it would limit confusion for
> sepgsql users if the documentation would say it at this point, e.g. "If
> the installation process completes without error, you can now start the
> server normally. Warnings from errors in sepgsql_contexts, a file
> external to PostgreSQL, are harmless and can be ignored."
>
Indeed, it might be confusable to understand whether the installation got
completed correctly, or not.
So, I appended more descriptions about this messages, as follows:

+ <para>
+ Please note that you may see the following notifications depending on
+ the combination of a particular version of <productname>libselinux</>
+ and <productname>selinux-policy</>.
+<screen>
+/etc/selinux/targeted/contexts/sepgsql_contexts: line 33 has invalid object ty
+</screen>
+ It is harmless messages and already fixed. So, you can ignore these
+ messages or update related packages to the latest version.
+ </para>

See the attached patch, that contains other 3 documentation updates.

> Thank you for this clarification. I have some ideas of things that if
> they were in the documentation they'd helped me. Instead of seeking
> agreement on each item, I propose that I gather documentation additions
> in a patch later after the review, and leave it up to you guys whether
> to include them or not.
>
OK, I like to check them. In addition, I'll also revise the wikipage in
parallel to inform correctly.

Thanks,
--
NEC Europe Ltd, SAP Global Competence Center
KaiGai Kohei <kohei(dot)kaigai(at)emea(dot)nec(dot)com>

On 2011-07-19 12:10, Kohei Kaigai wrote:
>
> See the attached patch, that contains other 3 documentation updates.
I looked at the patch and the additions look good, though I didn't
actually apply it yet.

thanks
Yeb

On 19.07.2011 12:28, Yeb Havinga wrote:
> On 2011-07-18 22:21, Kohei KaiGai wrote:
>> The Scientific Linux 6 is not suitable, because its libselinux version
>> is a bit older
>> than this patch expects (libselinux-2.0.99 or later).
>> My recommendation is Fedora 15, instead.
> Installing right now, thanks for the heads up!

Would it be reasonable to #ifdefs the parts that require version 2.0.99?
That's very recent so might not be available on popular distributions
for some time, so it would be nice to not have a hard dependency on it.
You could have autoconf rules to check for the new functions, and only
use them if they are available.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On 2011-07-19 22:39, Heikki Linnakangas wrote:
> On 19.07.2011 12:28, Yeb Havinga wrote:
>> On 2011-07-18 22:21, Kohei KaiGai wrote:
>>> The Scientific Linux 6 is not suitable, because its libselinux version
>>> is a bit older
>>> than this patch expects (libselinux-2.0.99 or later).
>>> My recommendation is Fedora 15, instead.
>> Installing right now, thanks for the heads up!
>
> Would it be reasonable to #ifdefs the parts that require version
> 2.0.99? That's very recent so might not be available on popular
> distributions for some time, so it would be nice to not have a hard
> dependency on it. You could have autoconf rules to check for the new
> functions, and only use them if they are available.

In contrary to the subject I was under the impression the current patch
is for the 9.2 release since it is in a commitfest for the 9.2 release
cycle, which would make the libselinux-2.0.99 dependency less of a problem.

--

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

2011/7/19 Yeb Havinga <yebhavinga(at)gmail(dot)com>:
> On 2011-07-19 22:39, Heikki Linnakangas wrote:
>>
>> On 19.07.2011 12:28, Yeb Havinga wrote:
>>>
>>> On 2011-07-18 22:21, Kohei KaiGai wrote:
>>>>
>>>> The Scientific Linux 6 is not suitable, because its libselinux version
>>>> is a bit older
>>>> than this patch expects (libselinux-2.0.99 or later).
>>>> My recommendation is Fedora 15, instead.
>>>
>>> Installing right now, thanks for the heads up!
>>
>> Would it be reasonable to #ifdefs the parts that require version 2.0.99?
>> That's very recent so might not be available on popular distributions for
>> some time, so it would be nice to not have a hard dependency on it. You
>> could have autoconf rules to check for the new functions, and only use them
>> if they are available.
>
> In contrary to the subject I was under the impression the current patch is
> for the 9.2 release since it is in a commitfest for the 9.2 release cycle,
> which would make the libselinux-2.0.99 dependency less of a problem.
>
Sorry, the subject line was just my typo.

I'd like to agree with Yeb's opinion, because the reason why we determined
libselinux-2.0.93 as least requirement is the implementation in v9.1 used
a function that was supported in 2.0.93 due to omission of userspace avc
feature from the initial version to minimize patch size.
If we included userspace avc feature from the beginning, it would require
libselinux-2.0.99.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

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

On Wed, Jul 20, 2011 at 9:47 AM, Yeb Havinga <yebhavinga(at)gmail(dot)com> wrote:
> * 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.

Both of the above points make me real nervous, especially the second
one. We already know that the cost of initializing the system caches
is a significant chunk of backend startup overhead, and I believe that
sizeof(Dllist) = 16 on 64-bit machine, so that means that 2048-entry
catcache is going to require us to zero an additional 32kB of memory
on every backend startup for a feature that most people won't use.

http://archives.postgresql.org/pgsql-hackers/2010-11/msg01632.php
http://archives.postgresql.org/pgsql-hackers/2010-11/msg01640.php

As to the first point, the other big problem with adding a syscache
here is that it could get really big. I've worried about that issue
previously, and Yev's perplexity about where the memory is going is
not too reassuring.

I think there are two realistic ways forward here:

1. Bite the bullet and implement a system allowing catalog caches to
be expanded on the fly at runtime. This would be nice because, aside
from whatever value it may have for SE-PostgreSQL, it might allow us
to shrink some of the other caches down and thereby speed up backend
startup in general, with the confidence that if the session ends up
running for a while we can expand them as necessary. In the
particular case of SE-PostgreSQL, it might be nice to be able to set
the initial cache size to 0, and only pay the cost of setting it up if
we discover that the security label feature is in use; but maybe
that's overly complex. On the downside, this addresses only the
startup problem (zeroing previously unreferenced memory is fairly
expensive) and not the runtime problem (using too much memory is bad).
But perhaps there is some other solution to the second problem.

2. Implement a custom cache tailored to the needs of SE-PostgreSQL
within contrib/sepgsql. This is a bit tricky because you need some
mechanism for receiving invalidation events.
CacheRegisterSyscacheCallback() is the usual method, but that only
lets you notice catcache invalidations, and here the whole point would
be to avoid having a catcache in the first place. Possibly we could
add a hook to PrepareForTupleInvalidation(); assuming that the hook
engages only after the IsSystemRelation and IsToastRelation checks, it
shouldn't cost that much. Doing it this way would (1) allow the
sepgsql-specific cache to come into existence only when needed and (2)
allow the sepgsl-specific cache to apply sepgsql-specific policies for
controlling memory use. For example, it could cap the number of
entries in the cache and age out any that are too old; or it could
arrange to maintain a single copy of each label rather than a copy for
each object.

I think that the uavc cache stuff may still be something we can think
about committing for this 'fest (I haven't reviewed it yet), but IMHO
the syscache parts need some more thought.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2011-07-20 16:54, Robert Haas wrote:
>
> As to the first point, the other big problem with adding a syscache
> here is that it could get really big. I've worried about that issue
> previously, and Yev's perplexity about where the memory is going is
> not too reassuring.
Yeah I just realized that #buckets <> cache items stored, which makes
some of the comments I made a bit strange. If the syscaches store
everything that's looked up then the same 5MB memory usage under
changing #buckets makes perfect sense.

regards,
Yeb

Yeb, Thanks for your volunteering.

> 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 sepgsql_restorecon(NULL) assigns default security label on all the
database objects being controlled, thus, its workload caches security
label (including text data) of these objects.
So, ~5MB of difference is an upper limit of syscache usage because of
SECLABELOID.
The number of buckets is independent from the memory consumption.

> * 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.
>
Thanks for your research.
The reason why I set 2048 is just a copy from the catalog which may
hold biggest number of entries (pg_proc). It may be improved later.

> * 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.
>
SELinux's kernel implementation also has access vector cache:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=security/selinux/avc.c

The reason why I didn't choose shared memory is the length of security
label text is variable, so it is not feasible to acquire a fixed length
region on shared memory in startup time.

> * 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)
>
OK, I'll add comments here.

> * 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.
>
It is a bit concern for me, because the permissive domain means it shall
be performed like as system is configured as permissive mode.
The behavior of log-violation-at-once is a property of permissive mode.
So, how about an idea to add comments about permissive domain around
the code to modify cache->allowed?

> * 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).
>
I omitted it because OS will handle these things correctly on process
Exit time, however, it is good idea to move on 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'?
>
It means object has no label. I'll fix this comment.

> * sepgsql_avc_compute(): the large comment in the beginning is hard to
> follow since sentences seem to have been a bit mixed up.
>
OK, I'll simplify the comment. How about your feeling about?

/*
* Validation check of the supplied security context.
* Because it always invoke system-call, frequent check should be avoided.
* Unless security policy is reloaded, validation status shall be kept,
* so we also cache whether the supplied security context was valid, or not.
*/

> * question: why is ucontext stored in avc_cache?
>
The 'tcontext' has to be kept on avc_cache as-is, even if the security label
is not valid according to security_check_context_raw(), because it is used to
hash-key.
If your point is that validation status should be kept in bool, not char *,
it is fair enough for me.

> * 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?
>
Hmm, it might require users deep knowledge about inside of sepgsql,
although it is useful to few situations.
OK, I'll remove it in this version.

> * avc_init has magic numbers 384, 4096. Maybe these can be defines (with
> a small comment what it is?)
>
I'll add a comment around static variable of avc_threshold.

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

Regarding to the point you suggested, I'll revise my patch within a day.

Thanks,
--
NEC Europe Ltd, SAP Global Competence Center
KaiGai Kohei <kohei(dot)kaigai(at)emea(dot)nec(dot)com>

On Wed, Jul 20, 2011 at 12:04 PM, Kohei Kaigai
<Kohei(dot)Kaigai(at)emea(dot)nec(dot)com> wrote:
> The sepgsql_restorecon(NULL) assigns default security label on all the
> database objects being controlled, thus, its workload caches security
> label (including text data) of these objects.
> So, ~5MB of difference is an upper limit of syscache usage because of
> SECLABELOID.

No, it's not. It's just the upper limit of how large it can be on an
*empty* database. A real database could have hundreds of tables and
views and thousands of columns. To say nothing of large objects.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> On Wed, Jul 20, 2011 at 12:04 PM, Kohei Kaigai
> <Kohei(dot)Kaigai(at)emea(dot)nec(dot)com> wrote:
> > The sepgsql_restorecon(NULL) assigns default security label on all the
> > database objects being controlled, thus, its workload caches security
> > label (including text data) of these objects.
> > So, ~5MB of difference is an upper limit of syscache usage because of
> > SECLABELOID.
>
> No, it's not. It's just the upper limit of how large it can be on an
> *empty* database. A real database could have hundreds of tables and
> views and thousands of columns. To say nothing of large objects.
>
Ah, sorry, you are correct.

Regarding to large objects, GetSecurityLabel() is modified not to use
SECLABELOID to flood of the syscache.

Thanks,
--
NEC Europe Ltd, SAP Global Competence Center
KaiGai Kohei <kohei(dot)kaigai(at)emea(dot)nec(dot)com>

> On Wed, Jul 20, 2011 at 9:47 AM, Yeb Havinga <yebhavinga(at)gmail(dot)com> wrote:
> > * 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.
>
> Both of the above points make me real nervous, especially the second
> one. We already know that the cost of initializing the system caches
> is a significant chunk of backend startup overhead, and I believe that
> sizeof(Dllist) = 16 on 64-bit machine, so that means that 2048-entry
> catcache is going to require us to zero an additional 32kB of memory
> on every backend startup for a feature that most people won't use.
>
> http://archives.postgresql.org/pgsql-hackers/2010-11/msg01632.php
> http://archives.postgresql.org/pgsql-hackers/2010-11/msg01640.php
>
> As to the first point, the other big problem with adding a syscache
> here is that it could get really big. I've worried about that issue
> previously, and Yev's perplexity about where the memory is going is
> not too reassuring.
>
> I think there are two realistic ways forward here:
>
> 1. Bite the bullet and implement a system allowing catalog caches to
> be expanded on the fly at runtime. This would be nice because, aside
> from whatever value it may have for SE-PostgreSQL, it might allow us
> to shrink some of the other caches down and thereby speed up backend
> startup in general, with the confidence that if the session ends up
> running for a while we can expand them as necessary. In the
> particular case of SE-PostgreSQL, it might be nice to be able to set
> the initial cache size to 0, and only pay the cost of setting it up if
> we discover that the security label feature is in use; but maybe
> that's overly complex. On the downside, this addresses only the
> startup problem (zeroing previously unreferenced memory is fairly
> expensive) and not the runtime problem (using too much memory is bad).
> But perhaps there is some other solution to the second problem.
>
One question is why InitCatalogCache() should be invoked from InitPostgres()?
If we initialize syscache on starting up postmaster process, I think
all the syscache buckets will be ready on child process forks, and
unused syscache does not consume physical memory, even if security
label acquire 2048 of buckets.

> 2. Implement a custom cache tailored to the needs of SE-PostgreSQL
> within contrib/sepgsql. This is a bit tricky because you need some
> mechanism for receiving invalidation events.
> CacheRegisterSyscacheCallback() is the usual method, but that only
> lets you notice catcache invalidations, and here the whole point would
> be to avoid having a catcache in the first place. Possibly we could
> add a hook to PrepareForTupleInvalidation(); assuming that the hook
> engages only after the IsSystemRelation and IsToastRelation checks, it
> shouldn't cost that much. Doing it this way would (1) allow the
> sepgsql-specific cache to come into existence only when needed and (2)
> allow the sepgsl-specific cache to apply sepgsql-specific policies for
> controlling memory use. For example, it could cap the number of
> entries in the cache and age out any that are too old; or it could
> arrange to maintain a single copy of each label rather than a copy for
> each object.
>
I'm interested in this idea, however, not a work in this commit-fest.

So, I'll detach syscache part from the existing uavc patch (and shared
object's security label patch).

> I think that the uavc cache stuff may still be something we can think
> about committing for this 'fest (I haven't reviewed it yet), but IMHO
> the syscache parts need some more thought.
>
Thanks,
--
NEC Europe Ltd, SAP Global Competence Center
KaiGai Kohei <kohei(dot)kaigai(at)emea(dot)nec(dot)com>

On Wed, Jul 20, 2011 at 12:40 PM, Kohei Kaigai
<Kohei(dot)Kaigai(at)emea(dot)nec(dot)com> wrote:
> One question is why InitCatalogCache() should be invoked from InitPostgres()?
> If we initialize syscache on starting up postmaster process, I think
> all the syscache buckets will be ready on child process forks, and
> unused syscache does not consume physical memory, even if security
> label acquire 2048 of buckets.

Most of the overhead seems to be the cost of the page faults required
for the kernel to map the relevant pages into the process address
space. After a fork(), all those pages become copy-on-write, so if
the syscaches are actually used this doesn't save much of anything.
In the specific case of sepgsql it would help, though, because what
you're proposing is to add a syscache that will in most cases never be
accessed at all. We'd still have a problem in the EXEC_BACKEND (i.e.
Windows) case, however...

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> On Wed, Jul 20, 2011 at 12:40 PM, Kohei Kaigai
> <Kohei(dot)Kaigai(at)emea(dot)nec(dot)com> wrote:
> > One question is why InitCatalogCache() should be invoked from InitPostgres()?
> > If we initialize syscache on starting up postmaster process, I think
> > all the syscache buckets will be ready on child process forks, and
> > unused syscache does not consume physical memory, even if security
> > label acquire 2048 of buckets.
>
> Most of the overhead seems to be the cost of the page faults required
> for the kernel to map the relevant pages into the process address
> space. After a fork(), all those pages become copy-on-write, so if
> the syscaches are actually used this doesn't save much of anything.
> In the specific case of sepgsql it would help, though, because what
> you're proposing is to add a syscache that will in most cases never be
> accessed at all. We'd still have a problem in the EXEC_BACKEND (i.e.
> Windows) case, however...
>
Hmm. It might not work in windows case.

I'd like to have a discussion about syscache towards next commit-fest.
The issues may be:
- Initial bucket allocation on most cases never be referenced.
- Reclaim cache entries on growing up too large.

Thanks,
--
NEC Europe Ltd, SAP Global Competence Center
KaiGai Kohei <kohei(dot)kaigai(at)emea(dot)nec(dot)com>

Kaigai,

> I'd like to have a discussion about syscache towards next commit-fest.
> The issues may be:
> - Initial bucket allocation on most cases never be referenced.
> - Reclaim cache entries on growing up too large.

Should I move this patch to the next CF?

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Wed, Jul 20, 2011 at 4:06 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> Kaigai,
>
>> I'd like to have a discussion about syscache towards next commit-fest.
>> The issues may be:
>>  - Initial bucket allocation on most cases never be referenced.
>>  - Reclaim cache entries on growing up too large.
>
> Should I move this patch to the next CF?

Not yet. I'm still going to review the other half of this patch.

The discussion above should be undertaken on a new thread.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Jul 20, 2011 at 4:19 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Jul 20, 2011 at 4:06 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>> Kaigai,
>>
>>> I'd like to have a discussion about syscache towards next commit-fest.
>>> The issues may be:
>>>  - Initial bucket allocation on most cases never be referenced.
>>>  - Reclaim cache entries on growing up too large.
>>
>> Should I move this patch to the next CF?
>
> Not yet.  I'm still going to review the other half of this patch.

On second thought, never mind: the second half still needs another
update from KaiGai, and I think we shouldn't hold the CF open for
that. So I'll mark this Returned with Feedback.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Kohei Kaigai <Kohei(dot)Kaigai(at)EMEA(dot)NEC(dot)COM> writes:
> I'd like to have a discussion about syscache towards next commit-fest.
> The issues may be:
> - Initial bucket allocation on most cases never be referenced.
> - Reclaim cache entries on growing up too large.

There used to be support for limiting the number of entries in a
syscache. It got removed (cf commit
8b9bc234ad43dfa788bde40ebf12e94f16556b7f) because (1) it was remarkably
expensive to do it (extra list manipulations, etc), and (2) performance
tended to fall off a cliff as soon as you had a few more tables or
whatever than the caches would hold. I'm disinclined to reverse that
decision. It appears to me that the security label stuff needs a
different set of performance tradeoffs than the rest of the catalogs,
which means it probably ought to do its own caching, rather than trying
to talk us into pessimizing the other catalogs for seclabel's benefit.

regards, tom lane

On Wed, Jul 20, 2011 at 4:48 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Kohei Kaigai <Kohei(dot)Kaigai(at)EMEA(dot)NEC(dot)COM> writes:
>> I'd like to have a discussion about syscache towards next commit-fest.
>> The issues may be:
>>  - Initial bucket allocation on most cases never be referenced.
>>  - Reclaim cache entries on growing up too large.
>
> There used to be support for limiting the number of entries in a
> syscache.  It got removed (cf commit
> 8b9bc234ad43dfa788bde40ebf12e94f16556b7f) because (1) it was remarkably
> expensive to do it (extra list manipulations, etc), and (2) performance
> tended to fall off a cliff as soon as you had a few more tables or
> whatever than the caches would hold.  I'm disinclined to reverse that
> decision.  It appears to me that the security label stuff needs a
> different set of performance tradeoffs than the rest of the catalogs,
> which means it probably ought to do its own caching, rather than trying
> to talk us into pessimizing the other catalogs for seclabel's benefit.

I agree that we don't want to limit the size of the catcaches. We've
been careful to design them in such a way that they won't blow out
memory, and so far there's no evidence that they do. If it ain't
broke, don't fix it. Having catcaches that can grow in size as needed
sounds useful to me, though.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2011-07-21 00:08, Robert Haas wrote:
> On Wed, Jul 20, 2011 at 4:48 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Kohei Kaigai<Kohei(dot)Kaigai(at)EMEA(dot)NEC(dot)COM> writes:
>>> I'd like to have a discussion about syscache towards next commit-fest.
>>> The issues may be:
>>> - Initial bucket allocation on most cases never be referenced.
>>> - Reclaim cache entries on growing up too large.
>> There used to be support for limiting the number of entries in a
>> syscache. It got removed (cf commit
>> 8b9bc234ad43dfa788bde40ebf12e94f16556b7f) because (1) it was remarkably
>> expensive to do it (extra list manipulations, etc), and (2) performance
>> tended to fall off a cliff as soon as you had a few more tables or
>> whatever than the caches would hold. I'm disinclined to reverse that
>> decision. It appears to me that the security label stuff needs a
>> different set of performance tradeoffs than the rest of the catalogs,
>> which means it probably ought to do its own caching, rather than trying
>> to talk us into pessimizing the other catalogs for seclabel's benefit.
> I agree that we don't want to limit the size of the catcaches. We've
> been careful to design them in such a way that they won't blow out
> memory, and so far there's no evidence that they do. If it ain't
> broke, don't fix it. Having catcaches that can grow in size as needed
> sounds useful to me, though.
Is there a way to dump syscache statistics like there is for
MemoryContext..Stats (something - gdb helped me there)?

Besides that I have to admit having problems understanding why the 5MB
cache for pg_seclabel is a problem; it's memory consumption is lineair
only to the size of the underlying database. (in contrast with the
other cache storing access vectors which would have O(n*m) space
complexity if it wouldn't reclaim space). So it is proportional to the
number of objects in a database and in size it seems to be in the same
order as pg_proc, pg_class and pg_attribute.

regards,

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

The attached patch is revised userspace-avc patch.

List of updates:
- The GUC of sepgsql.avc_threshold was removed.
- "char *ucontext" of avc_cache was replaced by "bool tcontext_is_valid".
- Comments added onto static variables
- Comments of sepgsql_avc_unlabeled() was revised.
- Comments of sepgsql_avc_compute() was simplified.
- Comments of sepgsql_avc_check_perms_label() also mention about
permissive domain, that performs similar to system's permissive mode.
- selinux_status_close() become invoked on on_proc_exit() hook.

Thanks,
--
NEC Europe Ltd, SAP Global Competence Center
KaiGai Kohei <kohei(dot)kaigai(at)emea(dot)nec(dot)com>

> -----Original Message-----
> From: Kohei Kaigai
> Sent: 20. Juli 2011 17:04
> To: 'Yeb Havinga'; Kohei KaiGai
> Cc: Robert Haas; PgHacker
> Subject: RE: [HACKERS] [v9.1] sepgsql - userspace access vector cache
>
> Yeb, Thanks for your volunteering.
>
> > 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 sepgsql_restorecon(NULL) assigns default security label on all the
> database objects being controlled, thus, its workload caches security
> label (including text data) of these objects.
> So, ~5MB of difference is an upper limit of syscache usage because of
> SECLABELOID.
> The number of buckets is independent from the memory consumption.
>
> > * 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.
> >
> Thanks for your research.
> The reason why I set 2048 is just a copy from the catalog which may
> hold biggest number of entries (pg_proc). It may be improved later.
>
> > * 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.
> >
> SELinux's kernel implementation also has access vector cache:
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=security/selinux/avc.c
>
> The reason why I didn't choose shared memory is the length of security
> label text is variable, so it is not feasible to acquire a fixed length
> region on shared memory in startup time.
>
> > * 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)
> >
> OK, I'll add comments here.
>
> > * 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.
> >
> It is a bit concern for me, because the permissive domain means it shall
> be performed like as system is configured as permissive mode.
> The behavior of log-violation-at-once is a property of permissive mode.
> So, how about an idea to add comments about permissive domain around
> the code to modify cache->allowed?
>
> > * 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).
> >
> I omitted it because OS will handle these things correctly on process
> Exit time, however, it is good idea to move on 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'?
> >
> It means object has no label. I'll fix this comment.
>
> > * sepgsql_avc_compute(): the large comment in the beginning is hard to
> > follow since sentences seem to have been a bit mixed up.
> >
> OK, I'll simplify the comment. How about your feeling about?
>
> /*
> * Validation check of the supplied security context.
> * Because it always invoke system-call, frequent check should be avoided.
> * Unless security policy is reloaded, validation status shall be kept,
> * so we also cache whether the supplied security context was valid, or not.
> */
>
> > * question: why is ucontext stored in avc_cache?
> >
> The 'tcontext' has to be kept on avc_cache as-is, even if the security label
> is not valid according to security_check_context_raw(), because it is used to
> hash-key.
> If your point is that validation status should be kept in bool, not char *,
> it is fair enough for me.
>
> > * 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?
> >
> Hmm, it might require users deep knowledge about inside of sepgsql,
> although it is useful to few situations.
> OK, I'll remove it in this version.
>
> > * avc_init has magic numbers 384, 4096. Maybe these can be defines (with
> > a small comment what it is?)
> >
> I'll add a comment around static variable of avc_threshold.
>
> > * Finally, IMO the call sites, e.g. check_relation_priviliges, have
> > improved in readability with this patch.
> >
>
> Regarding to the point you suggested, I'll revise my patch within a day.
>
> Thanks,
> --
> NEC Europe Ltd, SAP Global Competence Center
> KaiGai Kohei <kohei(dot)kaigai(at)emea(dot)nec(dot)com>

On Thu, Jul 21, 2011 at 4:00 AM, Yeb Havinga <yebhavinga(at)gmail(dot)com> wrote:
> Is there a way to dump syscache statistics like there is for
> MemoryContext..Stats (something - gdb helped me there)?

I don't know of one.

> Besides that I have to admit having problems understanding why the 5MB cache
> for pg_seclabel is a problem; it's memory consumption is lineair only to the
> size of the underlying database.  (in contrast with the other cache storing
> access vectors which would have O(n*m) space complexity if it wouldn't
> reclaim space). So it is proportional to the number of objects in a database
> and in size it seems to be in the same order as pg_proc, pg_class and
> pg_attribute.

Fair enough. I'm not convinced that the sheer quantity of memory use
is a problem, although I would like to see a few more test results
before we decide that definitively. I *am* unwilling to pay the
startup overhead of initializing an extra 2048 syscache that only
sepgsql users will actually need.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2011-07-21 15:03, Robert Haas wrote:
> On Thu, Jul 21, 2011 at 4:00 AM, Yeb Havinga<yebhavinga(at)gmail(dot)com> wrote:
>> Besides that I have to admit having problems understanding why the 5MB cache
>> for pg_seclabel is a problem; it's memory consumption is lineair only to the
>> size of the underlying database. (in contrast with the other cache storing
>> access vectors which would have O(n*m) space complexity if it wouldn't
>> reclaim space). So it is proportional to the number of objects in a database
>> and in size it seems to be in the same order as pg_proc, pg_class and
>> pg_attribute.
> Fair enough. I'm not convinced that the sheer quantity of memory use
> is a problem, although I would like to see a few more test results
> before we decide that definitively. I *am* unwilling to pay the
> startup overhead of initializing an extra 2048 syscache that only
> sepgsql users will actually need.

Is it possible to only include the syscache on --enable-selinux
configurations? It would imply physical data incompatibility with
standard configurations, but that's also true for e.g. the block size.

Also, the tests I did with varying bucket sizes suggested that
decreasing the syscache to 256 didn't show a significant performance
decrease compared to the 2048 #buckets, for the restorecon test, which
hits over 3000 objects with security labels. My guess is that that is a
fair middle of the road database schema size. Are you unwilling to pay
the startup overhead for a extra 256 syscache?

--

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

On Thu, Jul 21, 2011 at 3:25 PM, Yeb Havinga <yebhavinga(at)gmail(dot)com> wrote:
> Is it possible to only include the syscache on --enable-selinux
> configurations? It would imply physical data incompatibility with standard
> configurations, but that's also true for e.g. the block size.

Not really. SECURITY LABEL is supposedly a generic facility that can
be used by a variety of providers, and the regression tests load a
dummy provider which works on any platform to test that it hasn't
gotten broken.

> Also, the tests I did with varying bucket sizes suggested that decreasing
> the syscache to 256 didn't show a significant performance decrease compared
> to the 2048 #buckets, for the restorecon test, which hits over 3000 objects
> with security labels. My guess is that that is a fair middle of the road
> database schema size. Are you unwilling to pay the startup overhead for a
> extra 256 syscache?

Not sure. I'd rather not, if it's easy to rejigger things so we don't
have to. I don't think this is necessarily a hard problem to solve -
it's just that no one has tried yet.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> Is it possible to only include the syscache on --enable-selinux
> configurations? It would imply physical data incompatibility with
> standard configurations, but that's also true for e.g. the block size.
>
> Also, the tests I did with varying bucket sizes suggested that
> decreasing the syscache to 256 didn't show a significant performance
> decrease compared to the 2048 #buckets, for the restorecon test, which
> hits over 3000 objects with security labels. My guess is that that is
> a fair middle of the road database schema size. Are you unwilling to
> pay the startup overhead for a extra 256 syscache?
>

Hello KaiGai-san,

off-list,

I was wondering why the catalog pg_seclabel exists at all. Why not store
the labels together with the objects (pg_class, pg_attribute etc) ? The
syscache wouldn't be needed in that case.

regards,
Yeb

2011/7/21 Yeb Havinga <yebhavinga(at)gmail(dot)com>:
>
>> Is it possible to only include the syscache on --enable-selinux
>> configurations? It would imply physical data incompatibility with standard
>> configurations, but that's also true for e.g. the block size.
>>
>> Also, the tests I did with varying bucket sizes suggested that decreasing
>> the syscache to 256 didn't show a significant performance decrease compared
>> to the 2048 #buckets, for the restorecon test, which hits over 3000 objects
>> with security labels. My guess is that that is a fair middle of the road
>> database schema size. Are you unwilling to pay the startup overhead for a
>> extra 256 syscache?
>>
>
> Hello KaiGai-san,
>
> off-list,
>
Unfortunatelly, not so...

> I was wondering why the catalog pg_seclabel exists at all. Why not store the
> labels together with the objects (pg_class, pg_attribute etc) ? The syscache
> wouldn't be needed in that case.
>
Although current sepgsql support to assign security label on limited number of
object classes (schema, relation, column and functions).
However, we have planed to control accesses on whole of objects managed by
PostgreSQL, not only these four.

If we needed to expand system catalog everytime when an object get newly
supported, the patch would be more invasive and make hard to upstream.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

2011/7/21 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Thu, Jul 21, 2011 at 3:25 PM, Yeb Havinga <yebhavinga(at)gmail(dot)com> wrote:
>> Is it possible to only include the syscache on --enable-selinux
>> configurations? It would imply physical data incompatibility with standard
>> configurations, but that's also true for e.g. the block size.
>
> Not really.  SECURITY LABEL is supposedly a generic facility that can
> be used by a variety of providers, and the regression tests load a
> dummy provider which works on any platform to test that it hasn't
> gotten broken.
>
>> Also, the tests I did with varying bucket sizes suggested that decreasing
>> the syscache to 256 didn't show a significant performance decrease compared
>> to the 2048 #buckets, for the restorecon test, which hits over 3000 objects
>> with security labels. My guess is that that is a fair middle of the road
>> database schema size. Are you unwilling to pay the startup overhead for a
>> extra 256 syscache?
>
> Not sure.  I'd rather not, if it's easy to rejigger things so we don't
> have to.  I don't think this is necessarily a hard problem to solve -
> it's just that no one has tried yet.
>
Now, I tend to implement a cache mechanism to translate ObjectAddress
to security label by sepgsql module itself, rather than generic syscache,
although it requires a new hook on PrepareForTupleInvalidation() as Robert
suggested in this thread.
Indeed, it seems to me worthwhile not to allocate memory being unused for
90% of users; from perspective of startup performance and resource consumption.

In addition, we may be potentially able to have a cache stuff well optimized
to the access control of SELinux; such as cache reclaim for recently unused
entries. So, I'd like to focus on the stuff in sepgsql/uavc.c right now.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On 2011-07-21 11:29, Kohei Kaigai wrote:
> The attached patch is revised userspace-avc patch.
>
> List of updates:
> - The GUC of sepgsql.avc_threshold was removed.
> - "char *ucontext" of avc_cache was replaced by "bool tcontext_is_valid".
> - Comments added onto static variables
> - Comments of sepgsql_avc_unlabeled() was revised.
> - Comments of sepgsql_avc_compute() was simplified.
> - Comments of sepgsql_avc_check_perms_label() also mention about
> permissive domain, that performs similar to system's permissive mode.
> - selinux_status_close() become invoked on on_proc_exit() hook.
Thank you for the update, I'm looking at it right now and with a new
look have some more questions. I took the liberty to supply a patch to
be applied after your v5 uavc patch.

1) At a few call sites of sepgsql_avc_lookup, a null tcontext is
detected, and then replaced by "unlabeled". I moved this to
sepgsql_avc_lookup itself.
2) Also I thought if it could work to not remember tcontext is valid,
but instead remember the consequence, which is that it is replaced by
"unlabeled". It makes the avc_cache struct shorter and the code somewhat
simpler.

regards,
--

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

> -----Original Message-----
> From: Yeb Havinga [mailto:yebhavinga(at)gmail(dot)com]
> Sent: 22. Juli 2011 10:23
> To: Kohei Kaigai
> Cc: Robert Haas; PgHacker; Kohei KaiGai
> Subject: Re: [HACKERS] [v9.1] sepgsql - userspace access vector cache
>
> On 2011-07-21 11:29, Kohei Kaigai wrote:
> > The attached patch is revised userspace-avc patch.
> >
> > List of updates:
> > - The GUC of sepgsql.avc_threshold was removed.
> > - "char *ucontext" of avc_cache was replaced by "bool tcontext_is_valid".
> > - Comments added onto static variables
> > - Comments of sepgsql_avc_unlabeled() was revised.
> > - Comments of sepgsql_avc_compute() was simplified.
> > - Comments of sepgsql_avc_check_perms_label() also mention about
> > permissive domain, that performs similar to system's permissive mode.
> > - selinux_status_close() become invoked on on_proc_exit() hook.
> Thank you for the update, I'm looking at it right now and with a new look have some more questions.
> I took the liberty to supply a patch to be applied after your v5 uavc patch.
>
> 1) At a few call sites of sepgsql_avc_lookup, a null tcontext is detected, and then replaced by
> "unlabeled". I moved this to sepgsql_avc_lookup itself.
>
Good improvement.

> 2) Also I thought if it could work to not remember tcontext is valid, but instead remember the consequence,
> which is that it is replaced by "unlabeled". It makes the avc_cache struct shorter and the code somewhat
> simpler.
>
Here is a reason why we hold tcontext, even if it is not valid.
The hash key of avc_cache is combination of scontext, tcontext and tclass. Thus, if we replaced an invalid
tcontext by unlabeled context, it would always make cache mishit and performance loss.

Thanks,
--
NEC Europe Ltd, SAP Global Competence Center
KaiGai Kohei <kohei(dot)kaigai(at)emea(dot)nec(dot)com>

On 2011-07-22 11:55, Kohei Kaigai wrote:
>
>> 2) Also I thought if it could work to not remember tcontext is valid, but instead remember the consequence,
>> which is that it is replaced by "unlabeled". It makes the avc_cache struct shorter and the code somewhat
>> simpler.
>>
> Here is a reason why we hold tcontext, even if it is not valid.
> The hash key of avc_cache is combination of scontext, tcontext and tclass. Thus, if we replaced an invalid
> tcontext by unlabeled context, it would always make cache mishit and performance loss.
I see that now, thanks.

I have no further comments, and I think that the patch in it's current
status is ready for committer.

regards,
--

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

2011/7/22 Yeb Havinga <yebhavinga(at)gmail(dot)com>:
> On 2011-07-22 11:55, Kohei Kaigai wrote:
>>
>>> 2) Also I thought if it could work to not remember tcontext is valid, but
>>> instead remember the consequence,
>>> which is that it is replaced by "unlabeled". It makes the avc_cache
>>> struct shorter and the code somewhat
>>> simpler.
>>>
>> Here is a reason why we hold tcontext, even if it is not valid.
>> The hash key of avc_cache is combination of scontext, tcontext and tclass.
>> Thus, if we replaced an invalid
>> tcontext by unlabeled context, it would always make cache mishit and
>> performance loss.
>
> I see that now, thanks.
>
> I have no further comments, and I think that the patch in it's current
> status is ready for committer.
>
Thanks for your reviewing.

The attached patch is a revised one according to your suggestion to
include fallback for 'unlabeled' label within sepgsql_avc_lookup().
And I found a noise in regression test results, so eliminated it from v5.
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

BTW, what is the current status of this patch?
The status of contrib/sepgsql part is unclear for me, although we agreed that
syscache is suitable mechanism for security labels.

Thanks,

2011/7/22 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> 2011/7/22 Yeb Havinga <yebhavinga(at)gmail(dot)com>:
>> On 2011-07-22 11:55, Kohei Kaigai wrote:
>>>
>>>> 2) Also I thought if it could work to not remember tcontext is valid, but
>>>> instead remember the consequence,
>>>> which is that it is replaced by "unlabeled". It makes the avc_cache
>>>> struct shorter and the code somewhat
>>>> simpler.
>>>>
>>> Here is a reason why we hold tcontext, even if it is not valid.
>>> The hash key of avc_cache is combination of scontext, tcontext and tclass.
>>> Thus, if we replaced an invalid
>>> tcontext by unlabeled context, it would always make cache mishit and
>>> performance loss.
>>
>> I see that now, thanks.
>>
>> I have no further comments, and I think that the patch in it's current
>> status is ready for committer.
>>
> Thanks for your reviewing.
>
> The attached patch is a revised one according to your suggestion to
> include fallback for 'unlabeled' label within sepgsql_avc_lookup().
> And I found a noise in regression test results, so eliminated it from v5.
> --
> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>
>
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Fri, Aug 5, 2011 at 2:36 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> BTW, what is the current status of this patch?

I think it's waiting for me to get around to reviewing it. Sorry,
been busy.... :-(

> The status of contrib/sepgsql part is unclear for me, although we agreed that
> syscache is suitable mechanism for security labels.

I thought we agreed the opposite - viz, you need a custom cache.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

2011/8/5 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Fri, Aug 5, 2011 at 2:36 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> BTW, what is the current status of this patch?
>
> I think it's waiting for me to get around to reviewing it.  Sorry,
> been busy.... :-(
>
Thanks for your efforts.

>> The status of contrib/sepgsql part is unclear for me, although we agreed that
>> syscache is suitable mechanism for security labels.
>
> I thought we agreed the opposite - viz, you need a custom cache.
>
Sorry, I missed to append "not" just after "syscache is ...".
Your explanation is right.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Thu, Jul 21, 2011 at 5:29 AM, Kohei Kaigai <Kohei(dot)Kaigai(at)emea(dot)nec(dot)com> wrote:
> The attached patch is revised userspace-avc patch.
>
> List of updates:
> - The GUC of sepgsql.avc_threshold was removed.
> - "char *ucontext" of avc_cache was replaced by "bool tcontext_is_valid".
> - Comments added onto static variables
> - Comments of sepgsql_avc_unlabeled() was revised.
> - Comments of sepgsql_avc_compute() was simplified.
> - Comments of sepgsql_avc_check_perms_label() also mention about
>  permissive domain, that performs similar to system's permissive mode.
> - selinux_status_close() become invoked on on_proc_exit() hook.

I tried to give this a test drive today but got stuck. I got sepgsql
compiled OK, but look what happens when I try to start the server:

[rhaas(at)f15selinux ~]$ postgres
FATAL: could not load library
"/home/rhaas/project/lib/postgresql/sepgsql.so":
/home/rhaas/project/lib/postgresql/sepgsql.so: undefined symbol:
getpeercon_raw

This is Fedora 15, with all available updates applied.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Aug 18, 2011 at 12:46 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Jul 21, 2011 at 5:29 AM, Kohei Kaigai <Kohei(dot)Kaigai(at)emea(dot)nec(dot)com> wrote:
>> The attached patch is revised userspace-avc patch.
>>
>> List of updates:
>> - The GUC of sepgsql.avc_threshold was removed.
>> - "char *ucontext" of avc_cache was replaced by "bool tcontext_is_valid".
>> - Comments added onto static variables
>> - Comments of sepgsql_avc_unlabeled() was revised.
>> - Comments of sepgsql_avc_compute() was simplified.
>> - Comments of sepgsql_avc_check_perms_label() also mention about
>>  permissive domain, that performs similar to system's permissive mode.
>> - selinux_status_close() become invoked on on_proc_exit() hook.
>
> I tried to give this a test drive today but got stuck.  I got sepgsql
> compiled OK, but look what happens when I try to start the server:
>
> [rhaas(at)f15selinux ~]$ postgres
> FATAL:  could not load library
> "/home/rhaas/project/lib/postgresql/sepgsql.so":
> /home/rhaas/project/lib/postgresql/sepgsql.so: undefined symbol:
> getpeercon_raw
>
> This is Fedora 15, with all available updates applied.

Oh. Apparently, this is what happens when you try to build sepgsql
without passing --with-selinux to configure.

That's lame. I think we need to patch contrib/sepgsql so that it
fails to build in that case, rather than building and then not
working.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Aug 18, 2011 at 12:52 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Aug 18, 2011 at 12:46 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Thu, Jul 21, 2011 at 5:29 AM, Kohei Kaigai <Kohei(dot)Kaigai(at)emea(dot)nec(dot)com> wrote:
>>> The attached patch is revised userspace-avc patch.
>>>
>>> List of updates:
>>> - The GUC of sepgsql.avc_threshold was removed.
>>> - "char *ucontext" of avc_cache was replaced by "bool tcontext_is_valid".
>>> - Comments added onto static variables
>>> - Comments of sepgsql_avc_unlabeled() was revised.
>>> - Comments of sepgsql_avc_compute() was simplified.
>>> - Comments of sepgsql_avc_check_perms_label() also mention about
>>>  permissive domain, that performs similar to system's permissive mode.
>>> - selinux_status_close() become invoked on on_proc_exit() hook.
>>
>> I tried to give this a test drive today but got stuck.  I got sepgsql
>> compiled OK, but look what happens when I try to start the server:
>>
>> [rhaas(at)f15selinux ~]$ postgres
>> FATAL:  could not load library
>> "/home/rhaas/project/lib/postgresql/sepgsql.so":
>> /home/rhaas/project/lib/postgresql/sepgsql.so: undefined symbol:
>> getpeercon_raw
>>
>> This is Fedora 15, with all available updates applied.
>
> Oh.  Apparently, this is what happens when you try to build sepgsql
> without passing --with-selinux to configure.
>
> That's lame.  I think we need to patch contrib/sepgsql so that it
> fails to build in that case, rather than building and then not
> working.

Also, I get these warnings:

/etc/selinux/targeted/contexts/sepgsql_contexts: line 33 has invalid
object type db_blobs
/etc/selinux/targeted/contexts/sepgsql_contexts: line 36 has invalid
object type db_language
/etc/selinux/targeted/contexts/sepgsql_contexts: line 37 has invalid
object type db_language
/etc/selinux/targeted/contexts/sepgsql_contexts: line 38 has invalid
object type db_language
/etc/selinux/targeted/contexts/sepgsql_contexts: line 39 has invalid
object type db_language
/etc/selinux/targeted/contexts/sepgsql_contexts: line 40 has invalid
object type db_language
1: sepgsql_restorecon = "t" (typeid = 16, len = 1, typmod = -1, byval = t)

The first is mentioned in the latest documentation, but the rest are not.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> That's lame. I think we need to patch contrib/sepgsql so that it
> fails to build in that case, rather than building and then not
> working.
>
It might be the following fix, but I have no idea to generate an error when $(with_selinux) != "yes" on makefile.

diff --git a/contrib/sepgsql/Makefile b/contrib/sepgsql/Makefile
index 7f997ee..fec4f1a 100644
--- a/contrib/sepgsql/Makefile
+++ b/contrib/sepgsql/Makefile
@@ -19,6 +19,12 @@ include $(top_builddir)/src/Makefile.global
include $(top_srcdir)/contrib/contrib-global.mk
endif

+ifneq ($(with_selinux),yes)
+##
+## Error generation
+##
+endif
+
SHLIB_LINK += $(filter -lselinux, $(LIBS))
REGRESS_OPTS += --launcher $(top_builddir)/contrib/sepgsql/launcher

--
NEC Europe Ltd, SAP Global Competence Center
KaiGai Kohei <kohei(dot)kaigai(at)emea(dot)nec(dot)com>

> -----Original Message-----
> From: Robert Haas [mailto:robertmhaas(at)gmail(dot)com]
> Sent: 18. August 2011 17:53
> To: Kohei Kaigai
> Cc: Yeb Havinga; PgHacker; Kohei KaiGai
> Subject: Re: [HACKERS] [v9.1] sepgsql - userspace access vector cache
>
> On Thu, Aug 18, 2011 at 12:46 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > On Thu, Jul 21, 2011 at 5:29 AM, Kohei Kaigai <Kohei(dot)Kaigai(at)emea(dot)nec(dot)com> wrote:
> >> The attached patch is revised userspace-avc patch.
> >>
> >> List of updates:
> >> - The GUC of sepgsql.avc_threshold was removed.
> >> - "char *ucontext" of avc_cache was replaced by "bool tcontext_is_valid".
> >> - Comments added onto static variables
> >> - Comments of sepgsql_avc_unlabeled() was revised.
> >> - Comments of sepgsql_avc_compute() was simplified.
> >> - Comments of sepgsql_avc_check_perms_label() also mention about
> >>  permissive domain, that performs similar to system's permissive mode.
> >> - selinux_status_close() become invoked on on_proc_exit() hook.
> >
> > I tried to give this a test drive today but got stuck.  I got sepgsql
> > compiled OK, but look what happens when I try to start the server:
> >
> > [rhaas(at)f15selinux ~]$ postgres
> > FATAL:  could not load library
> > "/home/rhaas/project/lib/postgresql/sepgsql.so":
> > /home/rhaas/project/lib/postgresql/sepgsql.so: undefined symbol:
> > getpeercon_raw
> >
> > This is Fedora 15, with all available updates applied.
>
> Oh. Apparently, this is what happens when you try to build sepgsql
> without passing --with-selinux to configure.
>
> That's lame. I think we need to patch contrib/sepgsql so that it
> fails to build in that case, rather than building and then not
> working.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
>
> Click
> https://www.mailcontrol.com/sr/i+p!jARD6rnTndxI!oX7Uu+NqWBeKfvsxHen8ElqAIKK2vDQ5PIqETvu3D1VdIOLM1
> BV3YJKcc+1yubdBaCdqw== to report this email as spam.

On Thu, Aug 18, 2011 at 1:00 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> [more problems]

OK, I'm giving up for now. I hit two more snags:

1. chkselinuxenv uses "which", and a Fedora 15 minimal install doesn't
include that. I fixed that by installing "which", but maybe we ought
to be looking for a way to eliminate that dependency, like testing for
the commands you need by running them with --help, or something like
that.

2. restorecon doesn't correctly set the permissions for me on
~/project/bin/psql. I get:

[rhaas(at)f15selinux sepgsql]$ ls -Z ~/project/bin/psql
-rwxr-xr-x. rhaas rhaas unconfined_u:object_r:user_home_t:s0
/home/rhaas/project/bin/psql

Now I can fix that by applying bin_t manually, as suggested in the
documentation. However, that just moves the failure to library load
time. regression.diffs has multiple copies of this error message:

/home/rhaas/project/bin/psql: error while loading shared libraries:
libpq.so.5: failed to map segment from shared object: Permission
denied

Help!

Thanks,

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> OK, I'm giving up for now. I hit two more snags:
>
> 1. chkselinuxenv uses "which", and a Fedora 15 minimal install doesn't
> include that. I fixed that by installing "which", but maybe we ought
> to be looking for a way to eliminate that dependency, like testing for
> the commands you need by running them with --help, or something like
> that.
>
Oops, I thought "which" is a part of coreutils.

I'll try to update chkselinuxenv to print a help message when necessary commands are not installed.

> 2. restorecon doesn't correctly set the permissions for me on
> ~/project/bin/psql. I get:
>
> [rhaas(at)f15selinux sepgsql]$ ls -Z ~/project/bin/psql
> -rwxr-xr-x. rhaas rhaas unconfined_u:object_r:user_home_t:s0
> /home/rhaas/project/bin/psql
>
> Now I can fix that by applying bin_t manually, as suggested in the
> documentation. However, that just moves the failure to library load
> time. regression.diffs has multiple copies of this error message:
>
> /home/rhaas/project/bin/psql: error while loading shared libraries:
> libpq.so.5: failed to map segment from shared object: Permission
> denied
>
I guess it tries to mmap(2) libpq.so.5 (labeled as user_home_t) with executable mode.
The regression test switches domain of psql command on its execution from "unconfined_t" to "sepgsql_regtest_user_t", however, I didn't allow this domain to mmap(2) files in user's home directory with executable mode.
It may need to revise the security policy of regression test to support installation onto home directory.

As a quick avoidance, how about --prefix=/usr/local/sepgsql instead?

Thanks,
--
NEC Europe Ltd, SAP Global Competence Center
KaiGai Kohei <kohei(dot)kaigai(at)emea(dot)nec(dot)com>

> -----Original Message-----
> From: Robert Haas [mailto:robertmhaas(at)gmail(dot)com]
> Sent: 18. August 2011 18:22
> To: Kohei Kaigai
> Cc: Yeb Havinga; PgHacker; Kohei KaiGai
> Subject: Re: [HACKERS] [v9.1] sepgsql - userspace access vector cache
>
> On Thu, Aug 18, 2011 at 1:00 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > [more problems]
>
> OK, I'm giving up for now. I hit two more snags:
>
> 1. chkselinuxenv uses "which", and a Fedora 15 minimal install doesn't
> include that. I fixed that by installing "which", but maybe we ought
> to be looking for a way to eliminate that dependency, like testing for
> the commands you need by running them with --help, or something like
> that.
>
> 2. restorecon doesn't correctly set the permissions for me on
> ~/project/bin/psql. I get:
>
> [rhaas(at)f15selinux sepgsql]$ ls -Z ~/project/bin/psql
> -rwxr-xr-x. rhaas rhaas unconfined_u:object_r:user_home_t:s0
> /home/rhaas/project/bin/psql
>
> Now I can fix that by applying bin_t manually, as suggested in the
> documentation. However, that just moves the failure to library load
> time. regression.diffs has multiple copies of this error message:
>
> /home/rhaas/project/bin/psql: error while loading shared libraries:
> libpq.so.5: failed to map segment from shared object: Permission
> denied
>
> Help!
>
> Thanks,
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
>
> Click
> https://www.mailcontrol.com/sr/g7UEZIfD10rTndxI!oX7Unz1!gA0DCbilsfI53CIRke!PbNpuk4RnjmGfZ8cEe1DM1
> BV3YJKcc9jEfBJ2k7YZA== to report this email as spam.

On Thu, Aug 18, 2011 at 1:17 PM, Kohei Kaigai <Kohei(dot)Kaigai(at)emea(dot)nec(dot)com> wrote:
>> That's lame.  I think we need to patch contrib/sepgsql so that it
>> fails to build in that case, rather than building and then not
>> working.
>>
> It might be the following fix, but I have no idea to generate an error when $(with_selinux) != "yes" on makefile.

Actually, as I look at this more, I think this build system is
completely mis-designed. Given that you want to build sepgsql,
selinux is not an optional feature. So the stuff in
contrib/sepgsql/Makefile that is intended to link against libselinux
only if --with-selinux was specified at configure time is nonsense.
We should just ALWAYS try to link against libselinux, and if it's not
there, then at least it'll fail right away at compile time instead of
appearing to compile OK but producing an so that then fails to load at
runtime.

The only actual legitimate purpose of --with-selinux is to allow
contrib/Makefile to decide whether, when someone tries to build "all
the contrib modules", we should try to build sepgsql too.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

2011/8/18 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Thu, Aug 18, 2011 at 1:17 PM, Kohei Kaigai <Kohei(dot)Kaigai(at)emea(dot)nec(dot)com> wrote:
>>> That's lame.  I think we need to patch contrib/sepgsql so that it
>>> fails to build in that case, rather than building and then not
>>> working.
>>>
>> It might be the following fix, but I have no idea to generate an error when $(with_selinux) != "yes" on makefile.
>
> Actually, as I look at this more, I think this build system is
> completely mis-designed.  Given that you want to build sepgsql,
> selinux is not an optional feature.  So the stuff in
> contrib/sepgsql/Makefile that is intended to link against libselinux
> only if --with-selinux was specified at configure time is nonsense.
> We should just ALWAYS try to link against libselinux, and if it's not
> there, then at least it'll fail right away at compile time instead of
> appearing to compile OK but producing an so that then fails to load at
> runtime.
>
> The only actual legitimate purpose of --with-selinux is to allow
> contrib/Makefile to decide whether, when someone tries to build "all
> the contrib modules", we should try to build sepgsql too.
>
I agree.

So, it seems to me we also need to revise configure script, not only
Makefile of sepgsql.

On configure script, we may need to check availability of libselinux
on the build system, independent from --with-selinux.
But it should not raise an error even if appropriate libselinux was not
available; except for the case when --with-selinux was explicitly given.
It just set flags of HAVE_SELINUX, instead.
I injected #error condition in sepgsql.h that shall be fired if user tries
to build contrib/sepgsql module without libselinux.

And, Makefile was revised to link libselinux always.

How about this design?

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

I fixed up the security policy for regression test, and chkselinuxenv script.

The revised security policy allows test domains to execute programs
being installed under home directories.
In addition, the revised chkselinuxenv newly checks necessary commands
to run this script itself, and changed the way to validate executability of
psql command. (The point of this test is whether the psql is executable
by sepgsql_regtest_user_t, or not. So, bin_t is not a criteria to fail the
script.)

Thanks,

2011/8/18 Kohei Kaigai <Kohei(dot)Kaigai(at)emea(dot)nec(dot)com>:
>> OK, I'm giving up for now.  I hit two more snags:
>>
>> 1. chkselinuxenv uses "which", and a Fedora 15 minimal install doesn't
>> include that.  I fixed that by installing "which", but maybe we ought
>> to be looking for a way to eliminate that dependency, like testing for
>> the commands you need by running them with --help, or something like
>> that.
>>
> Oops, I thought "which" is a part of coreutils.
>
> I'll try to update chkselinuxenv to print a help message when necessary commands are not installed.
>
>> 2. restorecon doesn't correctly set the permissions for me on
>> ~/project/bin/psql.  I get:
>>
>> [rhaas(at)f15selinux sepgsql]$ ls -Z ~/project/bin/psql
>> -rwxr-xr-x. rhaas rhaas unconfined_u:object_r:user_home_t:s0
>> /home/rhaas/project/bin/psql
>>
>> Now I can fix that by applying bin_t manually, as suggested in the
>> documentation.  However, that just moves the failure to library load
>> time.  regression.diffs has multiple copies of this error message:
>>
>> /home/rhaas/project/bin/psql: error while loading shared libraries:
>> libpq.so.5: failed to map segment from shared object: Permission
>> denied
>>
> I guess it tries to mmap(2) libpq.so.5 (labeled as user_home_t) with executable mode.
> The regression test switches domain of psql command on its execution from "unconfined_t" to "sepgsql_regtest_user_t", however, I didn't allow this domain to mmap(2) files in user's home directory with executable mode.
> It may need to revise the security policy of regression test to support installation onto home directory.
>
> As a quick avoidance, how about --prefix=/usr/local/sepgsql instead?
>
> Thanks,
> --
> NEC Europe Ltd, SAP Global Competence Center
> KaiGai Kohei <kohei(dot)kaigai(at)emea(dot)nec(dot)com>
>
>
>> -----Original Message-----
>> From: Robert Haas [mailto:robertmhaas(at)gmail(dot)com]
>> Sent: 18. August 2011 18:22
>> To: Kohei Kaigai
>> Cc: Yeb Havinga; PgHacker; Kohei KaiGai
>> Subject: Re: [HACKERS] [v9.1] sepgsql - userspace access vector cache
>>
>> On Thu, Aug 18, 2011 at 1:00 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> > [more problems]
>>
>> OK, I'm giving up for now.  I hit two more snags:
>>
>> 1. chkselinuxenv uses "which", and a Fedora 15 minimal install doesn't
>> include that.  I fixed that by installing "which", but maybe we ought
>> to be looking for a way to eliminate that dependency, like testing for
>> the commands you need by running them with --help, or something like
>> that.
>>
>> 2. restorecon doesn't correctly set the permissions for me on
>> ~/project/bin/psql.  I get:
>>
>> [rhaas(at)f15selinux sepgsql]$ ls -Z ~/project/bin/psql
>> -rwxr-xr-x. rhaas rhaas unconfined_u:object_r:user_home_t:s0
>> /home/rhaas/project/bin/psql
>>
>> Now I can fix that by applying bin_t manually, as suggested in the
>> documentation.  However, that just moves the failure to library load
>> time.  regression.diffs has multiple copies of this error message:
>>
>> /home/rhaas/project/bin/psql: error while loading shared libraries:
>> libpq.so.5: failed to map segment from shared object: Permission
>> denied
>>
>> Help!
>>
>> Thanks,
>>
>> --
>> Robert Haas
>> EnterpriseDB: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>>
>>  Click
>> https://www.mailcontrol.com/sr/g7UEZIfD10rTndxI!oX7Unz1!gA0DCbilsfI53CIRke!PbNpuk4RnjmGfZ8cEe1DM1
>> BV3YJKcc9jEfBJ2k7YZA==  to report this email as spam.
>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
> 2011/8/18 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> Actually, as I look at this more, I think this build system is
>> completely mis-designed. Given that you want to build sepgsql,
>> selinux is not an optional feature. So the stuff in
>> contrib/sepgsql/Makefile that is intended to link against libselinux
>> only if --with-selinux was specified at configure time is nonsense.

What stuff is that?

> So, it seems to me we also need to revise configure script, not only
> Makefile of sepgsql.

This patch seems unnecessary to me. The way it works now appears to be
quite parallel to the way that contrib/xml2 works, and has worked for
years. I don't think that sepgsql should behave differently from that.

regards, tom lane

On Fri, Aug 19, 2011 at 9:59 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
>> 2011/8/18 Robert Haas <robertmhaas(at)gmail(dot)com>:
>>> Actually, as I look at this more, I think this build system is
>>> completely mis-designed.  Given that you want to build sepgsql,
>>> selinux is not an optional feature.  So the stuff in
>>> contrib/sepgsql/Makefile that is intended to link against libselinux
>>> only if --with-selinux was specified at configure time is nonsense.
>
> What stuff is that?

SHLIB_LINK += $(filter -lselinux, $(LIBS))

> This patch seems unnecessary to me.  The way it works now appears to be
> quite parallel to the way that contrib/xml2 works, and has worked for
> years.  I don't think that sepgsql should behave differently from that.

Hmm. I see now that it's parallel, but I find it pretty confusing
that building sepgsql without specifying --with-selinux results in a
shared library that seems to compile OK but won't load. Why not
just:

SHLIB_LINK = -lselinux

Similarly, in the case of xml2 we have:

SHLIB_LINK += $(filter -lxslt, $(LIBS)) $(filter -lxml2, $(LIBS))

For xslt, it probably makes sense to filter it out if it wasn't found,
because the code has ifdefs for USE_XSLT that do something sensible if
the library is not there. But I fail to see what the point is of
filtering out xml2, because surely we're doomed if that's not there...
or am I confused?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Fri, Aug 19, 2011 at 9:59 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> This patch seems unnecessary to me.

> Hmm. I see now that it's parallel, but I find it pretty confusing
> that building sepgsql without specifying --with-selinux results in a
> shared library that seems to compile OK but won't load.

Well, that's a fair point, but the same happens in contrib/xml2 (if you
have a setup that doesn't need a special -I switch, or you provide that
some other way), and nobody has ever complained about it.

> Why not just:

> SHLIB_LINK = -lselinux

I wouldn't have any particular objection to that (although I think it's
supposed to be += here). I don't see that any of the other changes
Kaigai proposed are helpful, though.

> Similarly, in the case of xml2 we have:

> SHLIB_LINK += $(filter -lxslt, $(LIBS)) $(filter -lxml2, $(LIBS))

> For xslt, it probably makes sense to filter it out if it wasn't found,
> because the code has ifdefs for USE_XSLT that do something sensible if
> the library is not there. But I fail to see what the point is of
> filtering out xml2, because surely we're doomed if that's not there...
> or am I confused?

Hmm. I think it's just that way to make the code look parallel for both
libraries. But I can see potential value in making -lxml2 unconditional
--- as you say, that would result in a link failure instead of a
silently broken library.

regards, tom lane

On Fri, Aug 19, 2011 at 10:20 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Why not just:
>
>> SHLIB_LINK = -lselinux
>
> I wouldn't have any particular objection to that (although I think it's
> supposed to be += here).

Oh, right.

> I don't see that any of the other changes
> Kaigai proposed are helpful, though.

I was coming to the same conclusion. I sort of liked his idea of
sticking a conditional #error directive in the header files to make it
more clear why it was failing. But on closer examination there's
really no benefit: it gets lost in a sea of other failures, and if you
have to look through the failure messages anyway you may as well
notice that the #include of <selinux/selinux.h> failed as anything
else. So I think changing that line to link with libselinux
unconditionally is about as well as we can do.

>> Similarly, in the case of xml2 we have:
>
>> SHLIB_LINK += $(filter -lxslt, $(LIBS)) $(filter -lxml2, $(LIBS))
>
>> For xslt, it probably makes sense to filter it out if it wasn't found,
>> because the code has ifdefs for USE_XSLT that do something sensible if
>> the library is not there.  But I fail to see what the point is of
>> filtering out xml2, because surely we're doomed if that's not there...
>> or am I confused?
>
> Hmm.  I think it's just that way to make the code look parallel for both
> libraries.  But I can see potential value in making -lxml2 unconditional
> --- as you say, that would result in a link failure instead of a
> silently broken library.

Right.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Aug 19, 2011 at 10:31 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Aug 19, 2011 at 10:20 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Why not just:
>>
>>> SHLIB_LINK = -lselinux
>>
>> I wouldn't have any particular objection to that (although I think it's
>> supposed to be += here).
>
> Oh, right.
>
>> I don't see that any of the other changes
>> Kaigai proposed are helpful, though.
>
> I was coming to the same conclusion.  I sort of liked his idea of
> sticking a conditional #error directive in the header files to make it
> more clear why it was failing.  But on closer examination there's
> really no benefit: it gets lost in a sea of other failures, and if you
> have to look through the failure messages anyway you may as well
> notice that the #include of <selinux/selinux.h> failed as anything
> else.  So I think changing that line to link with libselinux
> unconditionally is about as well as we can do.
>
>>> Similarly, in the case of xml2 we have:
>>
>>> SHLIB_LINK += $(filter -lxslt, $(LIBS)) $(filter -lxml2, $(LIBS))
>>
>>> For xslt, it probably makes sense to filter it out if it wasn't found,
>>> because the code has ifdefs for USE_XSLT that do something sensible if
>>> the library is not there.  But I fail to see what the point is of
>>> filtering out xml2, because surely we're doomed if that's not there...
>>> or am I confused?
>>
>> Hmm.  I think it's just that way to make the code look parallel for both
>> libraries.  But I can see potential value in making -lxml2 unconditional
>> --- as you say, that would result in a link failure instead of a
>> silently broken library.
>
> Right.

On further review, if the initial configure was done without
--with-libxml, xml2 is doomed anyway. There's no value to changing
anything there one way or the other, because it relies not only on
symbols from the library, but also on symbols that are only defined
when PostgreSQL itself is configured with xml support. In other
words, there's no chance of undetected failure here.

On the other hand, we've worked pretty hard to keep sepgsql at arm's
length from the core server, so as it stands, it's quite easy to build
a busted sepgsql module.

This probably explains why no one's complained about this before, and
I think the appropriate fix is to change just sepgsql. I would like
to back-patch that (one-line) change into 9.1 as well, to eliminate
this as a foot-gun for anyone who may be packaging that module for the
first time.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> -----Original Message-----
> From: Robert Haas [mailto:robertmhaas(at)gmail(dot)com]
> Sent: 19. August 2011 15:55
> To: Tom Lane
> Cc: Kohei KaiGai; Kohei Kaigai; Yeb Havinga; PgHacker
> Subject: Re: [HACKERS] [v9.1] sepgsql - userspace access vector cache
>
> On Fri, Aug 19, 2011 at 10:31 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > On Fri, Aug 19, 2011 at 10:20 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >>> Why not just:
> >>
> >>> SHLIB_LINK = -lselinux
> >>
> >> I wouldn't have any particular objection to that (although I think it's
> >> supposed to be += here).
> >
> > Oh, right.
> >
> >> I don't see that any of the other changes
> >> Kaigai proposed are helpful, though.
> >
> > I was coming to the same conclusion.  I sort of liked his idea of
> > sticking a conditional #error directive in the header files to make it
> > more clear why it was failing.  But on closer examination there's
> > really no benefit: it gets lost in a sea of other failures, and if you
> > have to look through the failure messages anyway you may as well
> > notice that the #include of <selinux/selinux.h> failed as anything
> > else.  So I think changing that line to link with libselinux
> > unconditionally is about as well as we can do.
> >
> >>> Similarly, in the case of xml2 we have:
> >>
> >>> SHLIB_LINK += $(filter -lxslt, $(LIBS)) $(filter -lxml2, $(LIBS))
> >>
> >>> For xslt, it probably makes sense to filter it out if it wasn't found,
> >>> because the code has ifdefs for USE_XSLT that do something sensible if
> >>> the library is not there.  But I fail to see what the point is of
> >>> filtering out xml2, because surely we're doomed if that's not there...
> >>> or am I confused?
> >>
> >> Hmm.  I think it's just that way to make the code look parallel for both
> >> libraries.  But I can see potential value in making -lxml2 unconditional
> >> --- as you say, that would result in a link failure instead of a
> >> silently broken library.
> >
> > Right.
>
> On further review, if the initial configure was done without
> --with-libxml, xml2 is doomed anyway. There's no value to changing
> anything there one way or the other, because it relies not only on
> symbols from the library, but also on symbols that are only defined
> when PostgreSQL itself is configured with xml support. In other
> words, there's no chance of undetected failure here.
>
> On the other hand, we've worked pretty hard to keep sepgsql at arm's
> length from the core server, so as it stands, it's quite easy to build
> a busted sepgsql module.
>
> This probably explains why no one's complained about this before, and
> I think the appropriate fix is to change just sepgsql. I would like
> to back-patch that (one-line) change into 9.1 as well, to eliminate
> this as a foot-gun for anyone who may be packaging that module for the
> first time.
>
I almost agree with this change.

One point I'm worrying about is a case when contrib/sepgsql is compiled
with older libselinux than minimum requirement. In this case, we may not
notice the broken module unless user tries to load it actually.
Is there a good idea to ensure compile failure when we try to build sepgsql
module when libselinux-2.0.98 or older was installed?

Of course, using --with-selinux on ./configure time is the best way...

Thanks,
--
NEC Europe Ltd, SAP Global Competence Center
KaiGai Kohei <kohei(dot)kaigai(at)emea(dot)nec(dot)com>

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On further review, if the initial configure was done without
> --with-libxml, xml2 is doomed anyway.

True, but it's still possible to build a shlib that will then not work.
I just did, after manually supplying the right -I switch:

make PROFILE=-I/usr/include/libxml2

Now admittedly a clueless person would be unlikely to know to do that,
but if his libxml installation were arranged so that no special -I
switch was needed (unlike the Fedora packaging), it'd be more likely
that this could happen.

> This probably explains why no one's complained about this before, and
> I think the appropriate fix is to change just sepgsql. I would like
> to back-patch that (one-line) change into 9.1 as well, to eliminate
> this as a foot-gun for anyone who may be packaging that module for the
> first time.

No objection to fixing or backpatching this, but I'm not seeing the
argument for treating this module differently from contrib/xml2. If you
believe that someone will try to manually build in contrib/sepgsql after
having failed to configure correctly, why do you not believe that for
xml2?

regards, tom lane

On Fri, Aug 19, 2011 at 11:26 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On further review, if the initial configure was done without
>> --with-libxml, xml2 is doomed anyway.
>
> True, but it's still possible to build a shlib that will then not work.
> I just did, after manually supplying the right -I switch:
>
> make PROFILE=-I/usr/include/libxml2
>
> Now admittedly a clueless person would be unlikely to know to do that,
> but if his libxml installation were arranged so that no special -I
> switch was needed (unlike the Fedora packaging), it'd be more likely
> that this could happen.
>
>> This probably explains why no one's complained about this before, and
>> I think the appropriate fix is to change just sepgsql.  I would like
>> to back-patch that (one-line) change into 9.1 as well, to eliminate
>> this as a foot-gun for anyone who may be packaging that module for the
>> first time.
>
> No objection to fixing or backpatching this, but I'm not seeing the
> argument for treating this module differently from contrib/xml2.  If you
> believe that someone will try to manually build in contrib/sepgsql after
> having failed to configure correctly, why do you not believe that for
> xml2?

Because I screwed it up accidentally for sepgsql, and I can't screw it
up for xml2 on purpose even after working fairly hard. Even after
shoving in the necessary -I switch (through a slightly different
mechanism than the one you just proposed), it still won't link,
whether -lxml2 is on the command-line or not.

That having been said, I don't mind changing them both symmetrically;
I'm just convinced that there's any benefit. However, if you think
that way is more future-proof or that I might be overlooking some
scenario, fine! It won't hurt anything.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Kohei Kaigai <Kohei(dot)Kaigai(at)EMEA(dot)NEC(dot)COM> writes:
> One point I'm worrying about is a case when contrib/sepgsql is compiled
> with older libselinux than minimum requirement. In this case, we may not
> notice the broken module unless user tries to load it actually.
> Is there a good idea to ensure compile failure when we try to build sepgsql
> module when libselinux-2.0.98 or older was installed?

Well, they should get at least a warning from referencing undefined
functions, no?

There's a limit to how friendly we can be here, since Linux's shlib
stuff is designed to not require all symbols to be resolvable at shlib
construction time. This is one place where Darwin works better ...

regards, tom lane

> -----Original Message-----
> From: Tom Lane [mailto:tgl(at)sss(dot)pgh(dot)pa(dot)us]
> Sent: 19. August 2011 16:34
> To: Kohei Kaigai
> Cc: Robert Haas; Kohei KaiGai; Yeb Havinga; PgHacker
> Subject: Re: [HACKERS] [v9.1] sepgsql - userspace access vector cache
>
> Kohei Kaigai <Kohei(dot)Kaigai(at)EMEA(dot)NEC(dot)COM> writes:
> > One point I'm worrying about is a case when contrib/sepgsql is compiled
> > with older libselinux than minimum requirement. In this case, we may not
> > notice the broken module unless user tries to load it actually.
> > Is there a good idea to ensure compile failure when we try to build sepgsql
> > module when libselinux-2.0.98 or older was installed?
>
> Well, they should get at least a warning from referencing undefined
> functions, no?
>
Yes. User should notice warning messages due to undefined symbols.
I'm not certain whether it makes sense to add -Werror here, or not.

> There's a limit to how friendly we can be here, since Linux's shlib
> stuff is designed to not require all symbols to be resolvable at shlib
> construction time. This is one place where Darwin works better ...
>
Hmm...
--
NEC Europe Ltd, SAP Global Competence Center
KaiGai Kohei <kohei(dot)kaigai(at)emea(dot)nec(dot)com>

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Fri, Aug 19, 2011 at 11:26 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> No objection to fixing or backpatching this, but I'm not seeing the
>> argument for treating this module differently from contrib/xml2.

> Because I screwed it up accidentally for sepgsql, and I can't screw it
> up for xml2 on purpose even after working fairly hard. Even after
> shoving in the necessary -I switch (through a slightly different
> mechanism than the one you just proposed), it still won't link,
> whether -lxml2 is on the command-line or not.

Huh. Links for me on Fedora 14 ...

[tgl(at)rh3 ~]$ cd ~/pgsql/contrib/xml2
[tgl(at)rh3 xml2]$ make clean
rm -f pgxml.so libpgxml.a
rm -f xpath.o xslt_proc.o
rm -rf results/ regression.diffs regression.out tmp_check/ log/
[tgl(at)rh3 xml2]$ make PROFILE=-I/usr/include/libxml2
gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wformat-security -fno-strict-aliasing -fwrapv -g -I/usr/include/libxml2 -fpic -I. -I. -I../../src/include -D_GNU_SOURCE -c -o xpath.o xpath.c
gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wformat-security -fno-strict-aliasing -fwrapv -g -I/usr/include/libxml2 -fpic -I. -I. -I../../src/include -D_GNU_SOURCE -c -o xslt_proc.o xslt_proc.c
gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wformat-security -fno-strict-aliasing -fwrapv -g -I/usr/include/libxml2 -fpic -shared -o pgxml.so xpath.o xslt_proc.o -L../../src/port -Wl,--as-needed -Wl,-rpath,'/home/tgl/testversion/lib',--enable-new-dtags -I/usr/include/libxml2
[tgl(at)rh3 xml2]$

(and yes, this is in a build tree configured without --with-libxml).
As far as I can tell, this *must* work this way on Linux. Maybe you
were testing the xml2 case on OS X? That OS is pickier.

regards, tom lane

Kohei Kaigai <Kohei(dot)Kaigai(at)EMEA(dot)NEC(dot)COM> writes:
>> From: Tom Lane [mailto:tgl(at)sss(dot)pgh(dot)pa(dot)us]
>> Well, they should get at least a warning from referencing undefined
>> functions, no?

> Yes. User should notice warning messages due to undefined symbols.
> I'm not certain whether it makes sense to add -Werror here, or not.

Hmm. That would help catch the problem, but I'm a bit uncomfortable
with adding -Werror in relatively new code. On the other hand, it's
not like we expect sepgsql to work on a wide variety of systems, so
maybe it'd be OK.

On the whole I don't think it's worth messing with the cflags for this.

regards, tom lane

On Fri, Aug 19, 2011 at 11:46 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Kohei Kaigai <Kohei(dot)Kaigai(at)EMEA(dot)NEC(dot)COM> writes:
>>> From: Tom Lane [mailto:tgl(at)sss(dot)pgh(dot)pa(dot)us]
>>> Well, they should get at least a warning from referencing undefined
>>> functions, no?
>
>> Yes. User should notice warning messages due to undefined symbols.
>> I'm not certain whether it makes sense to add -Werror here, or not.
>
> Hmm.  That would help catch the problem, but I'm a bit uncomfortable
> with adding -Werror in relatively new code.  On the other hand, it's
> not like we expect sepgsql to work on a wide variety of systems, so
> maybe it'd be OK.
>
> On the whole I don't think it's worth messing with the cflags for this.

Yeah, I agree.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Aug 19, 2011 at 11:40 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Fri, Aug 19, 2011 at 11:26 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> No objection to fixing or backpatching this, but I'm not seeing the
>>> argument for treating this module differently from contrib/xml2.
>
>> Because I screwed it up accidentally for sepgsql, and I can't screw it
>> up for xml2 on purpose even after working fairly hard.  Even after
>> shoving in the necessary -I switch (through a slightly different
>> mechanism than the one you just proposed), it still won't link,
>> whether -lxml2 is on the command-line or not.
>
> Huh.  Links for me on Fedora 14 ...
>
> [tgl(at)rh3 ~]$ cd ~/pgsql/contrib/xml2
> [tgl(at)rh3 xml2]$ make clean
> rm -f pgxml.so   libpgxml.a
> rm -f xpath.o xslt_proc.o
> rm -rf results/ regression.diffs regression.out tmp_check/ log/
> [tgl(at)rh3 xml2]$ make PROFILE=-I/usr/include/libxml2
> gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wformat-security -fno-strict-aliasing -fwrapv -g -I/usr/include/libxml2 -fpic -I. -I. -I../../src/include -D_GNU_SOURCE   -c -o xpath.o xpath.c
> gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wformat-security -fno-strict-aliasing -fwrapv -g -I/usr/include/libxml2 -fpic -I. -I. -I../../src/include -D_GNU_SOURCE   -c -o xslt_proc.o xslt_proc.c
> gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wformat-security -fno-strict-aliasing -fwrapv -g -I/usr/include/libxml2 -fpic -shared -o pgxml.so xpath.o xslt_proc.o -L../../src/port -Wl,--as-needed -Wl,-rpath,'/home/tgl/testversion/lib',--enable-new-dtags -I/usr/include/libxml2
> [tgl(at)rh3 xml2]$
>
> (and yes, this is in a build tree configured without --with-libxml).
> As far as I can tell, this *must* work this way on Linux.  Maybe you
> were testing the xml2 case on OS X?  That OS is pickier.

Hrm, I *thought* I had tested on Linux, but maybe I was on OS X that
time around. Anyway, I can reproduce this now.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Aug 5, 2011 at 2:36 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> BTW, what is the current status of this patch?
> The status of contrib/sepgsql part is unclear for me, although we agreed that
> syscache is suitable mechanism for security labels.

Sorry it's taken me a while to get around to looking at this. Reviewing away...

For me, the line you removed from dml.out causes the regression tests to fail.

I don't understand what this is going for:

+ /*
+ * To boost up trusted procedure checks on db_procedure object
+ * class, we also confirm the decision when user calls a procedure
+ * labeled as 'tcontext'.
+ */

Can you explain?

sepgsql_avc_check_perms_label has a formatting error on the line that
says "result = false". It's not indented correctly.

Several functions do this: sepgsql_avc_check_valid(); do { ... } while
(!sepgsql_avc_check_valid); I don't understand why we need a loop
there.

The comment for sepgql_avc_check_perms_label uses the word "elsewhere"
when it really means "otherwise".

Changing the calling sequence of sepgsql_get_label() would perhaps be
better separated out into its own patch.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert, Thanks for your reviewing.

> For me, the line you removed from dml.out causes the regression tests to fail.
>
Fixed. Why did I removed this line??

> I don't understand what this is going for:
>
> +       /*
> +        * To boost up trusted procedure checks on db_procedure object
> +        * class, we also confirm the decision when user calls a procedure
> +        * labeled as 'tcontext'.
> +        */
>
> Can you explain?
>
Yes. It also caches an expected security label when a client being
labeled as "scontext" tries to execute a procedure being labeled as
"tcontext", to reduce number of system call invocations on fmgr_hook
and needs_fmgr_hook.
If the expected security label is not same with "scontext", it means
the procedure performs as a trusted procedure that switches security
label of the client during its execution; like a security invoker
function.
A pair of security labels are the only factor to determine whether the
procedure is a trusted-procedure, or not. Thus, it is suitable to
cache in userspace avc.

As an aside, the reason why we don't cache the default security label
being assigned on newly created named objects (such as tables, ...) is
that selinux allows to set up exceptional default security label on a
particular name, so it does not suitable for avc structure.
(I'm waiting for getting included this interface into libselinux.)

> sepgsql_avc_check_perms_label has a formatting error on the line that
> says "result = false".  It's not indented correctly.
>
OK, I fixed it.

> Several functions do this: sepgsql_avc_check_valid(); do { ... } while
> (!sepgsql_avc_check_valid);  I don't understand why we need a loop
> there.
>
It enables to prevent inconsistent access control decision from
concurrent security policy reloading.
I want the following steps being executed in atomic.
1) Lookup object class number in kernel-side
2) Lookup permission bits in kernel-side
3) Ask kernel-side its access control decision.

The selinux_status_update returns 1, if any status of selinux in
kernel side (that requires to flush userspace caches) had been changed
since the last invocation.
In this case, we retry whole of the process from the beginning to
ensure whole of access control decision being made by either old or
new policy.
Thus, I enclosed these blocks by do {...} while() loop.

> The comment for sepgql_avc_check_perms_label uses the word "elsewhere"
> when it really means "otherwise".
>
OK, I fixed it.

> Changing the calling sequence of sepgsql_get_label() would perhaps be
> better separated out into its own patch.
>
OK, I reverted it.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Fri, Aug 26, 2011 at 5:32 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> Yes. It also caches an expected security label when a client being
> labeled as "scontext" tries to execute a procedure being labeled as
> "tcontext", to reduce number of system call invocations on fmgr_hook
> and needs_fmgr_hook.
> If the expected security label is not same with "scontext", it means
> the procedure performs as a trusted procedure that switches security
> label of the client during its execution; like a security invoker
> function.
> A pair of security labels are the only factor to determine whether the
> procedure is a trusted-procedure, or not. Thus, it is suitable to
> cache in userspace avc.

I've committed this, but I still think it would be helpful to revise
that comment. The turn "boosted up" is not very precise or
informative. Could you submit a separate, comment-only patch to
improve this?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> On Fri, Aug 26, 2011 at 5:32 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> > Yes. It also caches an expected security label when a client being
> > labeled as "scontext" tries to execute a procedure being labeled as
> > "tcontext", to reduce number of system call invocations on fmgr_hook
> > and needs_fmgr_hook.
> > If the expected security label is not same with "scontext", it means
> > the procedure performs as a trusted procedure that switches security
> > label of the client during its execution; like a security invoker
> > function.
> > A pair of security labels are the only factor to determine whether the
> > procedure is a trusted-procedure, or not. Thus, it is suitable to
> > cache in userspace avc.
>
> I've committed this, but I still think it would be helpful to revise
> that comment. The turn "boosted up" is not very precise or
> informative. Could you submit a separate, comment-only patch to
> improve this?
>
OK, Please wait for a few days.

Thanks,
--
NEC Europe Ltd, SAP Global Competence Center
KaiGai Kohei <kohei(dot)kaigai(at)emea(dot)nec(dot)com>

> I've committed this, but I still think it would be helpful to revise
> that comment. The turn "boosted up" is not very precise or
> informative. Could you submit a separate, comment-only patch to
> improve this?
>
I tried to put more detailed explanation about the logic of do { ... } while
loop of sepgsql_avc_check_valid and the cache field of new security label to
be switched on execution of the procedure. Is it helpful?

Thanks,
--
NEC Europe Ltd, SAP Global Competence Center
KaiGai Kohei <kohei(dot)kaigai(at)emea(dot)nec(dot)com>

On 2011-09-01 14:40, Robert Haas wrote:
> userspace avc.
> I've committed this, but I still think it would be helpful to revise
> that comment. The turn "boosted up" is not very precise or
> informative. Could you submit a separate, comment-only patch to
> improve this?

I didn't see my name as one of the reviewers in the commit message. If
that is because the review was bad, I'd be interested to know what I can
improve for the next one.

regards,
Yeb Havinga

On Mon, Sep 5, 2011 at 9:14 AM, Yeb Havinga <yebhavinga(at)gmail(dot)com> wrote:
> On 2011-09-01 14:40, Robert Haas wrote:
>>
>>  userspace avc.
>> I've committed this, but I still think it would be helpful to revise
>> that comment.  The turn "boosted up" is not very precise or
>> informative.  Could you submit a separate, comment-only patch to
>> improve this?
>
> I didn't see my name as one of the reviewers in the commit message. If that
> is because the review was bad, I'd be interested to know what I can improve
> for the next one.

No, it's because I flaked. Sorry, Yeb.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Excerpts from Robert Haas's message of lun sep 05 23:27:16 -0300 2011:
> On Mon, Sep 5, 2011 at 9:14 AM, Yeb Havinga <yebhavinga(at)gmail(dot)com> wrote:
> > On 2011-09-01 14:40, Robert Haas wrote:
> >>
> >>  userspace avc.
> >> I've committed this, but I still think it would be helpful to revise
> >> that comment.  The turn "boosted up" is not very precise or
> >> informative.  Could you submit a separate, comment-only patch to
> >> improve this?
> >
> > I didn't see my name as one of the reviewers in the commit message. If that
> > is because the review was bad, I'd be interested to know what I can improve
> > for the next one.
>
> No, it's because I flaked. Sorry, Yeb.

Pity we can't use git notes.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Mon, Sep 5, 2011 at 10:52 PM, Alvaro Herrera
<alvherre(at)commandprompt(dot)com> wrote:
> Excerpts from Robert Haas's message of lun sep 05 23:27:16 -0300 2011:
>> On Mon, Sep 5, 2011 at 9:14 AM, Yeb Havinga <yebhavinga(at)gmail(dot)com> wrote:
>> > On 2011-09-01 14:40, Robert Haas wrote:
>> >>
>> >>  userspace avc.
>> >> I've committed this, but I still think it would be helpful to revise
>> >> that comment.  The turn "boosted up" is not very precise or
>> >> informative.  Could you submit a separate, comment-only patch to
>> >> improve this?
>> >
>> > I didn't see my name as one of the reviewers in the commit message. If that
>> > is because the review was bad, I'd be interested to know what I can improve
>> > for the next one.
>>
>> No, it's because I flaked.  Sorry, Yeb.
>
> Pity we can't use git notes.

Well, I guess there's no law that says we can't. Should I give it a try?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Excerpts from Robert Haas's message of lun sep 05 23:55:33 -0300 2011:
> On Mon, Sep 5, 2011 at 10:52 PM, Alvaro Herrera
> <alvherre(at)commandprompt(dot)com> wrote:
> > Excerpts from Robert Haas's message of lun sep 05 23:27:16 -0300 2011:
> >> On Mon, Sep 5, 2011 at 9:14 AM, Yeb Havinga <yebhavinga(at)gmail(dot)com> wrote:
> >> > On 2011-09-01 14:40, Robert Haas wrote:
> >> >>
> >> >>  userspace avc.
> >> >> I've committed this, but I still think it would be helpful to revise
> >> >> that comment.  The turn "boosted up" is not very precise or
> >> >> informative.  Could you submit a separate, comment-only patch to
> >> >> improve this?
> >> >
> >> > I didn't see my name as one of the reviewers in the commit message. If that
> >> > is because the review was bad, I'd be interested to know what I can improve
> >> > for the next one.
> >>
> >> No, it's because I flaked.  Sorry, Yeb.
> >
> > Pity we can't use git notes.
>
> Well, I guess there's no law that says we can't. Should I give it a try?

I don't see why not :-) (But my guess is that you're going to need to
publish some pull and push instructions, because I gather it's not trivial).

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On 2011-09-06 04:55, Robert Haas wrote:
> On Mon, Sep 5, 2011 at 10:52 PM, Alvaro Herrera
> <alvherre(at)commandprompt(dot)com> wrote:
>> Excerpts from Robert Haas's message of lun sep 05 23:27:16 -0300 2011:
>>> On Mon, Sep 5, 2011 at 9:14 AM, Yeb Havinga<yebhavinga(at)gmail(dot)com> wrote:
>>>> I didn't see my name as one of the reviewers in the commit message. If that
>>>> is because the review was bad, I'd be interested to know what I can improve
>>>> for the next one.
>>> No, it's because I flaked. Sorry, Yeb.
>> Pity we can't use git notes.
> Well, I guess there's no law that says we can't. Should I give it a try?
>

I don't mind keeping the current commit message, I was just trying to
read between the lines and that has been answered.

regards,
Yeb

On Tue, Sep 6, 2011 at 12:41 AM, Alvaro Herrera
<alvherre(at)commandprompt(dot)com> wrote:
>> > Pity we can't use git notes.
>>
>> Well, I guess there's no law that says we can't.  Should I give it a try?
>
> I don't see why not :-)  (But my guess is that you're going to need to
> publish some pull and push instructions, because I gather it's not trivial).

I spent some time looking at this this morning, and my reaction is....
yeeeeaggh!!

To make this work, everyone who commits to the repository and everyone
who pulls from the repository must update their .git/config file. And
if by some poor chance two people should happen to make changes to the
git notes on the same file, a horrible merge process ensues.

Here's a relevant blog entry: http://progit.org/2010/08/25/notes.html

I also discovered that when you first create a note in your local
repository, you get a new ref called notes/commits (branches and tags
are also refs). Once you have this, it is extremely nontrivial to
figure out how to get rid of it. Removing the note doesn't work,
because that's treated as a new commit on the ref, not a request to
undo the previous commit. Unlike regular branches, there don't seem
to be any tools for getting rid of commits (i.e. git reset --hard
origin/master) and unlike both branches and tags, there's no
particularly obvious way to delete the ref completely (git branch { -d
| -D }, git tag -d). I finally found a message on a mailing list with
the magic formula, which turns out to be "git update-ref -d
refs/notes/commits" (and not, as I had initially guessed, "git
update-ref -d notes/commits", which silently succeeds but fails to
accomplish anything).

As much as I'd like to have something like this, I'm inclined to think
that it will take a whole lot more time to get this facility working
than it's really worth. Unless someone has a strong opinion the other
way, I think we should just plan to revisit this in a year or two, or
whenever the git folks have gotten around to polishing this some more.
It feels like a potentially good feature that is still really
half-baked at this point.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 09/06/2011 09:35 AM, Robert Haas wrote:

[git notes is more than cumbersome ]

>
> As much as I'd like to have something like this, I'm inclined to think
> that it will take a whole lot more time to get this facility working
> than it's really worth. Unless someone has a strong opinion the other
> way, I think we should just plan to revisit this in a year or two, or
> whenever the git folks have gotten around to polishing this some more.
> It feels like a potentially good feature that is still really
> half-baked at this point.
>

Don't hold your breath waiting.

cheers

andrew

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Tue, Sep 6, 2011 at 12:41 AM, Alvaro Herrera
> <alvherre(at)commandprompt(dot)com> wrote:
>> Pity we can't use git notes.

> I spent some time looking at this this morning, and my reaction is....
> yeeeeaggh!!

Yeah, we had this same discussion six weeks ago.

http://archives.postgresql.org/pgsql-hackers/2011-07/msg01208.php

regards, tom lane

On Tue, Sep 6, 2011 at 4:40 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Tue, Sep 6, 2011 at 12:41 AM, Alvaro Herrera
>> <alvherre(at)commandprompt(dot)com> wrote:
>>> Pity we can't use git notes.
>
>> I spent some time looking at this this morning, and my reaction is....
>>  yeeeeaggh!!
>
> Yeah, we had this same discussion six weeks ago.
>
> http://archives.postgresql.org/pgsql-hackers/2011-07/msg01208.php

Oh, I forgot about that. You even found the same blog entry I did. Sigh...

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Sep 2, 2011 at 12:38 PM, Kohei Kaigai <Kohei(dot)Kaigai(at)emea(dot)nec(dot)com> wrote:
>> I've committed this, but I still think it would be helpful to revise
>> that comment.  The turn "boosted up" is not very precise or
>> informative.  Could you submit a separate, comment-only patch to
>> improve this?
>>
> I tried to put more detailed explanation about the logic of do { ... } while
> loop of sepgsql_avc_check_valid and the cache field of new security label to
> be switched on execution of the procedure. Is it helpful?

I edited this and committed it along with a bunch of further
wordsmithing on the comments in that file. Please let me know if you
see any isuses.

Thanks,

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company