On Tue, Jun 20, 2023 at 3:51 AM Juraj Marcin <juraj@xxxxxxxxxxxxxxx> wrote: > > On 2023-06-19 17:53, Paul Moore wrote: > > On Sun, Jun 18, 2023 at 5:41 AM Juraj Marcin <juraj@xxxxxxxxxxxxxxx> wrote: > > > > > > On 2023-06-15 22:04, Paul Moore wrote: > > > > On Thu, Jun 1, 2023 at 1:03 PM Juraj Marcin <juraj@xxxxxxxxxxxxxxx> wrote: > > > > > On 2023-05-31 18:24, Paul Moore wrote: > > > > > > On Wed, May 31, 2023 at 7:32 AM Juraj Marcin <juraj@xxxxxxxxxxxxxxx> wrote: > > > > > > > > > > > > > > Currently, filename transitions are stored separately from other type > > > > > > > enforcement rules and only support exact name matching. However, in > > > > > > > practice, the names contain variable parts. This leads to many > > > > > > > duplicated rules in the policy that differ only in the part of the name, > > > > > > > or it is even impossible to cover all possible combinations. > > > > > > > > > > > > > > First, this series of patches moves the filename transitions to be part > > > > > > > of the avtab structures. This not only makes the implementation of > > > > > > > prefix/suffix matching and future enhancements easier, but also reduces > > > > > > > the technical debt regarding the filename transitions. Next, the last > > > > > > > patch implements the support for prefix/suffix name matching itself by > > > > > > > extending the structures added in previous patches in this series. > > > > > > > > > > > > > > Even though, moving everything to avtab increases the memory usage and > > > > > > > the size of the binary policy itself and thus the loading time, the > > > > > > > ability to match the prefix or suffix of the name will reduce the > > > > > > > overall number of rules in the policy which should mitigate this issue. > > > > > > > > > > > > > > This implementation has been successfully tested using the existing and > > > > > > > also new tests in the SELinux Testsuite. > > > > > > > > > > > > > > Juraj Marcin (5): > > > > > > > selinux: move transition to separate structure in avtab_datum > > > > > > > selinux: move filename transitions to avtab > > > > > > > selinux: implement new binary format for filename transitions in avtab > > > > > > > selinux: filename transitions move tests > > > > > > > selinux: add prefix/suffix matching support to filename type > > > > > > > transitions > > > > > > > > > > > > Just a quick comment as I haven't had a chance to properly review this > > > > > > series yet; you show some memory usage and performance measurements in > > > > > > some of the intermediate patches, that's good, but I don't see the > > > > > > same measurements taken when the full patchset is applied. Please > > > > > > provide the same memory usage and performance comparisons with the > > > > > > full patchset applied. > > > > > > > > > > Of course, here are the measurements with the whole patchset applied. > > > > > > > > > > I also included measurements with new policy (based on the Fedora > > > > > policy) that uses prefix filename transitions where possible. This new > > > > > policy has been generated by merging existing filename transitions into > > > > > prefix ones if it would reduce the number of transitions overall while > > > > > keeping the resulting type same. > > > > > > > > > > [1] Reference kernel (c52df19e3759), Fedora policy (format v33) > > > > > [2] This patchset, Fedora policy (format v33) > > > > > [3] This patchset, Fedora policy without prefix/suffix rules (format v35) > > > > > [4] This patchset, Fedora policy with prefix rules (format v35) > > > > > > > > > > > > > > > Test | Mem | Binary | Policy | Create tty | osbench > > > > > | Usage | policy | load | | create > > > > > | | size | time | (ms/file) | files > > > > > | (MiB) | (MiB) | (ms) | real | kernel | (us/file) > > > > > ------+-------+--------+--------+--------+--------+----------- > > > > > [1] | 157 | 3.4 | 78 | 1.1021 | 0.7586 | 7.8277 > > > > > [2] | 200 | 3.4 | 206 | 1.1193 | 0.7724 | 8.2711 > > > > > [3] | 169 | 5.8 | 106 | 1.1021 | 0.7724 | 8.0304 > > > > > [4] | 164 | 3.8 | 86 | 1.1029 | 0.7586 | 7.9609 > > > > > > > > Thanks for performing those measurements. > > > > > > > > I apologize that I haven't had an opportunity to review your patcheset > > > > in detail just yet (I've been struggling with some self-inflicted > > > > networking issues this week), but looking strictly at the numbers > > > > above it appears that by every metric in the table this patchset > > > > results in a policy that is larger (both on-disk and in-memory) as > > > > well as performance that is at best the same (although in most cases, > > > > it is worse). Are there any improvements expected beyond test > > > > configuration [4] (above)? > > > > > > The main goal of this patchset is to bring the possibility to use prefix > > > or suffix matching in filename transitions, as now it is not possible to > > > cover files that have fixed prefix and variable part after it. For > > > example devices in /dev (the policy now enumerates all possible number > > > suffixes) or files with random suffix/prefix (not possible at all). > > > > > > The next goal is to make future improvements easier, for example > > > supporting filename transitions in conditional policies or inherent > > > support for type attributes. > > > > > > As for performance, the goal is to implement previous goals while not > > > killing the performance by them. Christian suggested some possible > > > optimizations [1], but after trying them out [2] they either not provide > > > much measurable difference or the difference is small and the > > > implementation hacky. > > > > I understand performance improvements were not the main motivation > > behind this patchset, but I'm somewhat concerned that policy load time > > *almost* triples in the case of an unmodified policy with this patch > > applied. Since that will be most everyone as soon as this patch is > > applied, that regression does concern me ... I'm not sure just yet how > > much it concerns me, but it isn't trivial. > > I also understand your concern. That higher load time (and also memory > usage) is the cost of doing the conversion from the older binary policy > format in the kernel during load. > > However, to reduce both load time and memory usage to the values in the > third test, the only action needed is recompiling the policy with newer > checkpolicy/libsepol, patches to which I also proposed. I'm also struggling a bit with the justification here. If the runtime and memory usage is worse or no better in every dimension even with a policy rewritten to use the new prefix/suffix matching feature, it seems hard to justify the change. I'd be curious to see what results you would get if you simply added the new feature (prefix/suffix matching) without moving the name-based transitions into the avtab. Also wondering whether you considered the simpler approach of just augmenting the kernel to recognize and support use of wildcards at the beginning and/or end of the existing name field to signify a prefix or suffix match. That seems more amenable to extensions beyond just prefix or suffix match.
