Re: [RFC PATCH userspace 0/6] Parallel setfiles/restorecon

From: Nicolas Iooss <nicolas.iooss@m4x.org>
To: Ondrej Mosnacek <omosnace@redhat.com>
Cc: peter enderborg <peter.enderborg@sony.com>,
	SElinux list <selinux@vger.kernel.org>
Subject: Re: [RFC PATCH userspace 0/6] Parallel setfiles/restorecon
Date: Wed, 28 Apr 2021 23:11:28 +0200	[thread overview]
Message-ID: <CAJfZ7=mG+8RzeFD3ZOEuPFAhX+acc1YY3wOVi5k2tO9m6YgprA@mail.gmail.com> (raw)
In-Reply-To: <CAFqZXNvvDypzgzzbsBp9zQ1wOV=P9XmEPidmH_-8Zn2hGp-A5Q@mail.gmail.com>

On Wed, Mar 24, 2021 at 12:05 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> On Wed, Mar 24, 2021 at 10:59 AM peter enderborg
> <peter.enderborg@sony.com> 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