| Message ID | 20211014145319.798740-1-omosnace@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Parallel setfiles/restorecon | expand |
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 ================== 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 ================== Performance on 4 cores: real 0m27.323s user 0m20.824s sys 0m4.029s vs real 0m12.462s user 0m31.220s sys 0m4.316s
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...
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... -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
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.