On Wed, Mar 24, 2021 at 12:05 PM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
>
> On Wed, Mar 24, 2021 at 10:59 AM peter enderborg
> <peter.enderborg@xxxxxxxx> wrote:
> > On 3/23/21 6:08 PM, Ondrej Mosnacek wrote:
> > > This series adds basic support for parallel relabeling to the libselinux
> > > API and the setfiles/restorecon CLI tools. It turns out that doing the
> > > relabeling in parallel can significantly reduce the time even with a
> > > relatively simple approach.
> > Nice! Have you any figures? Is it valid for both solid state and mechanical storage?
>
> They are in the last patch :) The VM setup I measured that on probably
> had the storage backed up by an SSD (or something with similar
> characteristics). I haven't tried it on an HDD yet.
>
> > > The first patch is a small cleanup that was found along the way and can
> > > be applied independently. Patches 2-4 are small incremental changes that
> > > make the internal selinux_restorecon functions more thread-safe (I kept
> > > them separate for ease of of review, but maybe they should be rather
> > > folded into the netx patch...). Patch 5 then completes the parallel
> > > relabeling implementation at libselinux level and adds a new function
> > > to the API that allows to make use of it. Finally, patch 6 adds parallel
> > > relabeling support to he setfiles/restorecon tools.
> > >
> > > The relevant man pages are also updated to reflect the new
> > > functionality.
> > >
> > > The patch descriptions contain more details, namely the last patch has
> > > also some benchmark numbers.
> > >
> > > Please test and review. I'm still not fully confident I got everything
> > > right (esp. regarding error handling), but I wanted to put this forward
> > > as an RFC to get some early feedback.
> > >
> > > Ondrej Mosnacek (6):
> > > selinux_restorecon: simplify fl_head allocation by using calloc()
> > > selinux_restorecon: protect file_spec list with a mutex
> > > selinux_restorecon: introduce selinux_log_sync()
> > > selinux_restorecon: add a global mutex to synchronize progress output
> > > selinux_restorecon: introduce selinux_restorecon_parallel(3)
> > > setfiles/restorecon: support parallel relabeling
> > >
> > > libselinux/include/selinux/restorecon.h | 14 +
> > > libselinux/man/man3/selinux_restorecon.3 | 29 +
> > > .../man/man3/selinux_restorecon_parallel.3 | 1 +
> > > libselinux/src/libselinux.map | 5 +
> > > libselinux/src/selinux_internal.h | 14 +
> > > libselinux/src/selinux_restorecon.c | 498 ++++++++++++------
> > > policycoreutils/setfiles/Makefile | 2 +-
> > > policycoreutils/setfiles/restore.c | 7 +-
> > > policycoreutils/setfiles/restore.h | 2 +-
> > > policycoreutils/setfiles/restorecon.8 | 9 +
> > > policycoreutils/setfiles/setfiles.8 | 9 +
> > > policycoreutils/setfiles/setfiles.c | 28 +-
> > > 12 files changed, 436 insertions(+), 182 deletions(-)
> > > create mode 100644 libselinux/man/man3/selinux_restorecon_parallel.3
> > >
> >
Hello,
I haven't seen any review of this RFC, so I decided to take a look.
The result looks quite good :) What is the current status of this
series, and can it become a "to-be-merged" patch series?
Anyway, here are some comments.
First, I was a little puzzled by the introduction of
selinux_log_sync() and the fact that it is used by
selinux_restorecon_common(), which means that callers of
selinux_restorecon() will also take the mutex log_mutex. This
surprised me because the description of one commit was very clear
about not depending very hard on libpthread... but in fact your code
is right there. I have just re-discovered the pthread helpers in
libselinux/src/selinux_internal.h :)
Nevertheless, now that I saw the pthread helpers, which enable using
libselinux without linking with libpthread, I am wondering: why is
introducing selinux_log_sync() like you do (and keeping calls to
selinux_log_sync() and selinux_log() in the code and praying that all
invocations from parallel code are converted to selinux_log_sync() )
better than introducing the mutex directly "in selinux_log()"? I
understand this is not so easy, because selinux_log is in fact a
function pointer... what I have in my mind consists in renaming the
pointer and in renaming a selinux_log_sync() to selinux_log(). This
would make the code less error-prone, regarding the issue of ensuring
to never call selinux_log callback in two parallel threads. What do
you think?
Then, when I compiled your patches with clang and some warning flags,
I got this warning:
selinux_restorecon.c:867:19: error: possible misuse of comma operator
here [-Werror,-Wcomma]
while ((errno = 0, ftsent = fts_read(fts)) != NULL) {
^
selinux_restorecon.c:867:10: note: cast expression to void to silence warning
while ((errno = 0, ftsent = fts_read(fts)) != NULL) {
^~~~~~~~~
(void)( )
/usr/include/errno.h:38:16: note: expanded from macro 'errno'
# define errno (*__errno_location ())
^
1 error generated.
Using a comma operator seems to be right, here, and using the
suggested workaround to silence the compiler warning seems to be fine.
Finally, the generated file
libselinux/src/selinuxswig_python_exception.i needs to be upgraded,
because the new function selinux_restorecon_parallel is being added to
it. It would be nice to have a patch which updates this file, or to
have this update in patch "selinux_restorecon: introduce
selinux_restorecon_parallel(3)" (your choice).
Thanks!
Nicolas
