On 2023-06-02 15:13, Christian Göttsche wrote:
> On Wed, 31 May 2023 at 13:32, Juraj Marcin <juraj@xxxxxxxxxxxxxxx> wrote:
> >
> > Currently, filename transitions are stored separately from other type
> > enforcement rules. This leads to possibly sub-optimal performance and
> > makes further improvements cumbersome.
> >
> > This patch adds a symbol table of filename transitions to the transition
> > structure added to avtab in the previous patch. It also implements
> > functions required for reading and writing filename transitions in
> > kernel policy format and updates computation of new type to use filename
> > transitions embedded in avtab. Last but not least, it updates the
> > conflict check in the conditional avtab to account for empty transitions
> > in the non-conditional avtab.
> >
> > These changes are expected to cause higher memory usage, as now there
> > needs to be a filename transition structure for every stype. This patch
> > effectively undoes most of the commit c3a276111ea2 ("selinux: optimize
> > storage of filename transitions"), but this will be mitigated by
> > providing support for matching prefix/suffix of the filename for
> > filename transitions in future patches which will reduce to need to have
> > so many of them.
> >
> > On the other hand, the changes do not significantly slow down the
> > creation of new files.
> >
> > Kernel | Mem | Create test_tty | Create test_tty | osbench [1]
> > | usage | (real time) | (kernel time) | create_files
> > -----------+-------+-----------------+-----------------+--------------
> > reference | 155MB | 1.3440 ms/file | 1.0071 ms/file | 10.6507 us/file
> > this patch | 198MB | 1.3912 ms/file | 1.0172 ms/file | 10.5567 us/file
> >
> > Create test_tty benchmark:
> >
> > mknod /dev/test_tty c 4 1
> > time for i in `seq 1 10000`; do
> > mknod /dev/test_tty$i c 4 1
> > done
> >
> > This benchmark should simulate the worst case scenario as many filename
> > transitions affect files created in the /dev directory.
> >
> > [1] https://github.com/mbitsnbites/osbench
> >
> > Reviewed-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
> > Signed-off-by: Juraj Marcin <juraj@xxxxxxxxxxxxxxx>
> > ---
<snip>
> > diff --git a/security/selinux/ss/avtab.h b/security/selinux/ss/avtab.h
> > index 6c8eb7c379cf..162ef1be85e7 100644
> > --- a/security/selinux/ss/avtab.h
> > +++ b/security/selinux/ss/avtab.h
> > @@ -22,6 +22,7 @@
> > #define _SS_AVTAB_H_
> >
> > #include "security.h"
> > +#include "symtab.h"
> >
> > struct avtab_key {
> > u16 source_type; /* source type */
> > @@ -49,6 +50,7 @@ struct avtab_key {
> >
> > struct avtab_trans {
> > u32 otype; /* default resulting type of the new object */
> > + struct symtab name_trans; /* filename transitions */
>
> What about using a bare hashtab to save the 4 bytes for the unused
> nprim member (+padding)?
>
> Also what about instead of storing an extra allocated u32 in the
> tables use the pointer itself as value?
Hi
I have implemented both optimizations to see the gain in performance and
memory usage, here are the results:
[1] Fedora policy (format v33)
[2] Fedora policy without prefix/suffix rules (format v35)
[3] Fedora policy with prefix rules (format v35)
before - before optimizations
opt1 - replace symtab with hashtab
opt2 - storing u32 value in the hashtab directly instead of a pointer
Test || Mem usage || Policy load time
|| (MiB) || (ms)
|| before | opt1 | opt2 || before | opt1 | opt2
------++--------+------+------++----------------------
[1] || 202 | 198 | 199 || 228 | 227 | 208
[2] || 170 | 171 | 167 || 120 | 114 | 107
[3] || 164 | 161 | 161 || 91 | 94 | 92
(Average values from 5 runs, measured 15 seconds after boot.)
As can be seen from the table, replacing the symtab with bare hashtab
does not seem to provide real world difference, but could be done easily
by adding helper functions to hashtab.c.
With Fedora policy, there were around 16.7k (combinations of src, tgt,
class) symtab tables, so replacing them with hashtab saves around 65 KiB
(+padding for each) in total.
However, even though storing the u32 value directly shows some small
difference in the loading time, I am not sure if it is worth it
considering the hacky nature of the optimization.
This optimization should in theory save around 900 KiB without prefix
rules [1][2] or 350 KiB with prefix rules [3] with Fedora policy.
>
> > };
> >
> > /*
<snip>
--
Juraj Marcin
