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

[Ondrej Mosnacek <omosnace@redhat.com>]

On Mon, Oct 18, 2021 at 10:56 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Fri, Oct 15, 2021 at 3:25 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > On Fri, Oct 15, 2021 at 2:37 PM Christian Göttsche
> > <cgzones@googlemail.com> wrote:
> > > On Thu, 14 Oct 2021 at 16:53, Ondrej Mosnacek <omosnace@redhat.com> 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.
> > > >
> > > > 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.
> > > >
> > > > Changes v1->v2:
> > > > - make selinux_log() synchronized instead of introducing selinux_log_sync()
> > > > - fix -Wcomma warning
> > > > - update the swig files as well
> > > > - bump new symbol version to LIBSELINUX_3.3 (this may need further update
> > > >   depending on when this gets merged)
> > > >
> > > > Ondrej Mosnacek (6):
> > > >   selinux_restorecon: simplify fl_head allocation by using calloc()
> > > >   selinux_restorecon: protect file_spec list with a mutex
> > > >   libselinux: make selinux_log() thread-safe
> > > >   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/callbacks.c                    |   8 +-
> > > >  libselinux/src/callbacks.h                    |  13 +-
> > > >  libselinux/src/libselinux.map                 |   5 +
> > > >  libselinux/src/selinux_internal.h             |  14 +
> > > >  libselinux/src/selinux_restorecon.c           | 466 ++++++++++++------
> > > >  libselinux/src/selinuxswig_python.i           |   6 +-
> > > >  libselinux/src/selinuxswig_python_exception.i |   8 +
> > > >  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 +-
> > > >  16 files changed, 444 insertions(+), 177 deletions(-)
> > > >  create mode 100644 libselinux/man/man3/selinux_restorecon_parallel.3
> > > >
> > > > --
> > > > 2.31.1
> > > >
> > >
> > >
> > > Running under ThreadSanitizer shows multiple instances of the following issue:
> > >
> > > ==================
> > > WARNING: ThreadSanitizer: data race (pid=16933)
> > >   Read of size 4 at 0x7f8790d636f4 by thread T2:
> > >     #0 lookup_all ./libselinux/src/label_file.c:954 (libselinux.so.1+0x14d1b)
> > >     #1 lookup_common ./libselinux/src/label_file.c:997 (libselinux.so.1+0x14f62)
> > >     #2 lookup ./libselinux/src/label_file.c:1095 (libselinux.so.1+0x14fff)
> > >     #3 selabel_lookup_common ./libselinux/src/label.c:167
> > > (libselinux.so.1+0x12291)
> > >     #4 selabel_lookup_raw ./libselinux/src/label.c:256 (libselinux.so.1+0x126ca)
> > >     #5 restorecon_sb ./libselinux/src/selinux_restorecon.c:638
> > > (libselinux.so.1+0x20c76)
> > >     #6 selinux_restorecon_thread
> > > ./libselinux/src/selinux_restorecon.c:947 (libselinux.so.1+0x21e99)
> > >
> > >   Previous write of size 4 at 0x7f8790d636f4 by thread T1:
> > >     #0 lookup_all ./libselinux/src/label_file.c:954 (libselinux.so.1+0x14d29)
> > >     #1 lookup_common ./libselinux/src/label_file.c:997 (libselinux.so.1+0x14f62)
> > >     #2 lookup ./libselinux/src/label_file.c:1095 (libselinux.so.1+0x14fff)
> > >     #3 selabel_lookup_common ./libselinux/src/label.c:167
> > > (libselinux.so.1+0x12291)
> > >     #4 selabel_lookup_raw ./libselinux/src/label.c:256 (libselinux.so.1+0x126ca)
> > >     #5 restorecon_sb ./libselinux/src/selinux_restorecon.c:638
> > > (libselinux.so.1+0x20c76)
> > >     #6 selinux_restorecon_thread
> > > ./libselinux/src/selinux_restorecon.c:947 (libselinux.so.1+0x21e99)
> > >
> > >   Location is heap block of size 267120 at 0x7f8790d45000 allocated by
> > > main thread:
> > >     #0 malloc ../../../../src/libsanitizer/tsan/tsan_interceptors_posix.cpp:655
> > > (libtsan.so.0+0x31799)
> > >     #1 sort_specs ./libselinux/src/label_file.h:207 (libselinux.so.1+0x17bd9)
> > >     #2 init ./libselinux/src/label_file.c:795 (libselinux.so.1+0x17bd9)
> > >     #3 selabel_file_init ./libselinux/src/label_file.c:1293
> > > (libselinux.so.1+0x1835b)
> > >     #4 selabel_open ./libselinux/src/label.c:228 (libselinux.so.1+0x125d8)
> > >     #5 restore_init ./policycoreutils/setfiles/restore.c:30 (setfiles+0x3886)
> > >     #6 main ./policycoreutils/setfiles/setfiles.c:434 (setfiles+0x344e)
> > >
> > >   Thread T2 (tid=16940, running) created by main thread at:
> > >     #0 pthread_create
> > > ../../../../src/libsanitizer/tsan/tsan_interceptors_posix.cpp:969
> > > (libtsan.so.0+0x5ad75)
> > >     #1 selinux_restorecon_common
> > > ./libselinux/src/selinux_restorecon.c:1197 (libselinux.so.1+0x2341c)
> > >     #2 selinux_restorecon_parallel
> > > ./libselinux/src/selinux_restorecon.c:1306 (libselinux.so.1+0x23985)
> > >     #3 process_glob ./policycoreutils/setfiles/restore.c:94 (setfiles+0x3ba4)
> > >     #4 main ./policycoreutils/setfiles/setfiles.c:463 (setfiles+0x362a)
> > >
> > >   Thread T1 (tid=16939, running) created by main thread at:
> > >     #0 pthread_create
> > > ../../../../src/libsanitizer/tsan/tsan_interceptors_posix.cpp:969
> > > (libtsan.so.0+0x5ad75)
> > >     #1 selinux_restorecon_common
> > > ./libselinux/src/selinux_restorecon.c:1197 (libselinux.so.1+0x2341c)
> > >     #2 selinux_restorecon_parallel
> > > ./libselinux/src/selinux_restorecon.c:1306 (libselinux.so.1+0x23985)
> > >     #3 process_glob ./policycoreutils/setfiles/restore.c:94 (setfiles+0x3ba4)
> > >     #4 main ./policycoreutils/setfiles/setfiles.c:463 (setfiles+0x362a)
> > >
> > > SUMMARY: ThreadSanitizer: data race ./libselinux/src/label_file.c:954
> > > in lookup_all
> > > ==================
> >
> > Good catch, although this one seems to be pre-existing w.r.t. thread
> > safety of selinux_restorecon(3) (which seems to be implied by the
> > existence of struct spec::regex_lock). I spotted some existing usage
> > of GCC atomic builtins, so I'll try to fix it using those.
> >
> > > Also some thread joins seems to be missing:
> > >
> > > ==================
> > > WARNING: ThreadSanitizer: thread leak (pid=16933)
> > >   Thread T1 (tid=16939, finished) created by main thread at:
> > >     #0 pthread_create
> > > ../../../../src/libsanitizer/tsan/tsan_interceptors_posix.cpp:969
> > > (libtsan.so.0+0x5ad75)
> > >     #1 selinux_restorecon_common
> > > ./libselinux/src/selinux_restorecon.c:1197 (libselinux.so.1+0x2341c)
> > >     #2 selinux_restorecon_parallel
> > > ./libselinux/src/selinux_restorecon.c:1306 (libselinux.so.1+0x23985)
> > >     #3 process_glob ./policycoreutils/setfiles/restore.c:94 (setfiles+0x3ba4)
> > >     #4 main ./policycoreutils/setfiles/setfiles.c:463 (setfiles+0x362a)
> > >
> > >   And 2 more similar thread leaks.
> > >
> > > SUMMARY: ThreadSanitizer: thread leak
> > > ./libselinux/src/selinux_restorecon.c:1197 in
> > > selinux_restorecon_common
> > > ==================
> >
> > Hm... My intention was to avoid the need for joins by synchronizing
> > via other means and just letting the threads exit on their own, but I
> > guess that doesn't fly with the sanitizer. Guess I'll have to allocate
> > an array for the handles and join the threads at the end after all...
>
> So I was able to get ThreadSanitizer to detect the race (by using only
> two threads and a very small number of files), but then the program
> just runs forever and I never get to see the thread leaks... Any hints
> on how you ran/compiled it to see the thread leaks? Or how long you
> had to wait for the program to finish? BTW, I only managed to get
> thread sanitizer to work with GCC. With CLang I always got some linker
> error...

Seems it was getting stuck because of the lack of joins. Once I
rewrote selinux_restorecon_common() to wait for the threads via
pthread_join(), setfiles built with -fsanitize=thread exited quickly
and without warnings.

-- 
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.