On Fri, Oct 15, 2021 at 2:37 PM Christian Göttsche <cgzones@xxxxxxxxxxxxxx> wrote: > On Thu, 14 Oct 2021 at 16:53, Ondrej Mosnacek <omosnace@xxxxxxxxxx> 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... -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
