On Wed, Apr 28, 2021 at 11:12 PM Nicolas Iooss <nicolas.iooss@xxxxxxx> wrote:
> 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?
I hope it can :) I posted it as an RFC initially mainly to get some
high-level feedback whether I'm going in the right direction. But
based on your reply it seems it's already close enough to
acceptability that I could drop the RFC in the next revision. I also
didn't test it all that thoroughly, especially w.r.t. various
corner-cases...
>
> 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?
That's a good question. Since this will be the first function in
libselinux with some internal parallelism, I guess I just didn't want
to affect the status quo of existing code too much... Indeed the lock
would be taken also in the single-threaded implementation, but since
in selinux_restorecon() selinux_log() is called only in non-hot paths,
I didn't bother optimizing that.
Anyway, I agree that making selinux_log() synchronized globally may be
a good idea. The cost should be minimal and it would prevent
accidental race conditions. If some reasonable quorum of maintainers
agrees, I will make that change in v2.
> 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.
Hm, yes, I think I'll just add the cast to void there... Not a big fan
of the comma operator, but I couldn't resist the temptation to use it
here :)
> 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).
Ah, thanks for pointing that out. I'll address it in v2.
--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.
