fzsync: tst_fzsync_pair_wait exit when parent hit accidental break

For system(rhel7.6, s390x) without __NR_recvmmsg supported, run cve-2016-7117 result in timeout and killed by LTP framework. The root reason is tst_syscall break with cleanup() function calling in this trace path:

  tst_syscall(__NR_recvmmsg, ...)
    tst_brk()
      cleanup()
        tst_fzsync_pair_cleanup()
          SAFE_PTHREAD_JOIN(pair->thread_b, NULL);

cve-2016-7117 hung at here to wait for thread_b send_and_close() finishing. But thread_b fall into infinite loop because of tst_fzsync_wait_b without an extra condition to exit. Eventually, test get timeout error like:

cve-2016-7117.c:145: CONF: syscall(-1) __NR_recvmmsg not supported
Test timeouted, sending SIGKILL!
tst_test.c:1125: INFO: If you are running on slow machine, try exporting LTP_TIMEOUT_MUL > 1 tst_test.c:1126: BROK: Test killed! (timeout?)

Signed-off-by Li Wang <[email protected]>

Cc Richard Palethorpe <[email protected]>

---
 include/tst_fuzzy_sync.h | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

include/tst_fuzzy_sync.h +6 -3

index de0402c9b..7e4d48f0a 100644
@@ -517,7 +517,8 @@ static void tst_fzsync_pair_update(struct tst_fzsync_pair *pair)
  * @return A non-zero value if the thread should continue otherwise the
  * calling thread should exit.
  */
-static inline void tst_fzsync_pair_wait(int *our_cntr,
+static inline void tst_fzsync_pair_wait(struct tst_fzsync_pair *pair,
+					int *our_cntr,
 					int *other_cntr,
 					int *spins)
 {

@@ -530,7 +531,8 @@ static inline void tst_fzsync_pair_wait(int *our_cntr,
 		 * then our counter may already have been set to zero.
 		 */
 		while (tst_atomic_load(our_cntr) > 0
-		       && tst_atomic_load(our_cntr) < INT_MAX) {
+		       && tst_atomic_load(our_cntr) < INT_MAX
+		       && !tst_atomic_load(&pair->exit)) {
 			if (spins)
 				(*spins)++;
 		}

@@ -540,14 +542,16 @@ static inline void tst_fzsync_pair_wait(int *our_cntr,
 		 * Once both counters have been set to zero the invariant
 		 * is restored and we can continue.
 		 */
-		while (tst_atomic_load(our_cntr) > 1)
+		while (tst_atomic_load(our_cntr) > 1
+			&& !tst_atomic_load(&pair->exit))
 			;
 	} else {

This is how it worked before, so it is fairly safe. However I don't like atomically checking for the exit value on every spin of the delay loop. Also because setting exit just causes it to drop through there is still the (theoretical) risk of it getting stuck on another operation before breaking out of thread B's main loop.

Also removing the exit variable makes formal verification a bit easier.

Another option might be to use pthread_kill with a realtime signal and a signal handler which immediately exits the current thread. I am not sure how much complexity that will introduce though?

Quoted 6 lines · Richard Palethorpe

This is how it worked before, so it is fairly safe. However I don't like atomically checking for the exit value on every spin of the delay loop. Also because setting exit just causes it to drop through there is still the (theoretical) risk of it getting stuck on another operation before breaking out of thread B's main loop.

Yes, and I noticed you removed the exit checking in last update, but I didn't realize that thread B will fall into infinite loop when parent is break abnormally.

Quoted 3 lines

Also removing the exit variable makes formal verification a bit easier.

Good point.

Quoted 1 line

Quoted 3 lines

Another option might be to use pthread_kill with a realtime signal and a signal handler which immediately exits the current thread. I am not sure how much complexity that will introduce though?

Well we can have a try, seems the only disadvantage of this method is thread_B sets signal handler at each loop start in tst_fzsync_run_b repeatedly.

Not sure if I understand correctly, what drafted in my mind is:

include/tst_fuzzy_sync.h +2 -0

index de0402c9b..6ef6bee01 100644
@@ -63,6 +63,8 @@
 #include <time.h>
 #include <math.h>
 #include <stdlib.h>
+#include <pthread.h>
+#include <errno.h>
 #include "tst_atomic.h"
 #include "tst_timer.h"
 #include "tst_safe_pthread.h"

Quoted 3 lines

Well we can have a try, seems the only disadvantage of this method is thread_B sets signal handler at each loop start in tst_fzsync_run_b repeatedly.

We could wrap thread B's main function 'run_b', which is passed to tst_fzsync_pair_reset, in another function which sets the singal handler at the start of the thread.

include/tst_fuzzy_sync.h struct tst_fzsync_pair { · line 160

Quoted 2 lines

include/tst_fuzzy_sync.h +0 -2

-       /** Internal; Used by tst_fzsync_pair_exit() and fzsync_pair_wait() */
-       int exit;

I was thinking of keeping the exit variable and using the kill signal as a backup. The reason being it should allow thread B to exit gracefully in most scenarious. In theory this should not matter because the test writer should not do any setup in thread B, but it might result in some wierd error/warning messages being printed for some tests.

Unfortunately pthread_join has no timeout and pthread_timedjoin_np is non-standard.

Another option might be to spin-wait for 'exit' to be incremented to 2 by thread B and send the signal after some arbitrarily large number of spins. What do you think?

Hi Richard,

Quoted 7 lines · Richard Palethorpe

Quoted 3 lines

Well we can have a try, seems the only disadvantage of this method is thread_B sets signal handler at each loop start in tst_fzsync_run_b repeatedly.

We could wrap thread B's main function 'run_b', which is passed to tst_fzsync_pair_reset, in another function which sets the singal handler at the start of the thread.

Good suggestion! This make sense to me.

Quoted 8 lines

include/tst_fuzzy_sync.h struct tst_fzsync_pair { · line 160

Quoted 2 lines

include/tst_fuzzy_sync.h +0 -2

-       /** Internal; Used by tst_fzsync_pair_exit() and fzsync_pair_wait() */
-       int exit;

I was thinking of keeping the exit variable and using the kill signal as a backup. The reason being it should allow thread B to exit gracefully in most scenarious. In theory this should not matter because the test writer should not do any setup in thread B, but it might result in some wierd error/warning messages being printed for some tests.

Yes, that's not a bad solution, but I was a little worried before is that would make things a bit mixed for the thread exiting. However, if we use pair->exit only for normal exiting and signal for unexpected abort, that's also accessible I guess.

Quoted 3 lines

Unfortunately pthread_join has no timeout and pthread_timedjoin_np is non-standard.

or maybe we could achieve a LTP private pthread_timedjoin_np version? I haven't look into more about that so have no idea for the detail/complexity.

Quoted 4 lines

Another option might be to spin-wait for 'exit' to be incremented to 2 by thread B and send the signal after some arbitrarily large number of spins. What do you think?

Hmm, what's the best value for arbitrarily large number? it seems hard to decide.

Comparing the above approaches, currently it's hard to say which one is better. If I have to make a choice, I'd like to try the first
method: pair->exit (for normal exiting) + signal(for unexpected abort).

For system(rhel7.6, s390x) without __NR_recvmmsg supported, run cve-2016-7117 result in timeout and killed by LTP framework. The root reason is tst_syscall break with cleanup() function calling in this trace path:

  tst_syscall(__NR_recvmmsg, ...)
    tst_brk()
      cleanup()
        tst_fzsync_pair_cleanup()
          SAFE_PTHREAD_JOIN(pair->thread_b, NULL);

cve-2016-7117 hung at here to wait for thread_b send_and_close() finishing. But thread_b fall into infinite loop because of tst_fzsync_wait_b without an extra condition to exit. Eventually, test get timeout error like:

cve-2016-7117.c:145: CONF: syscall(-1) __NR_recvmmsg not supported
Test timeouted, sending SIGKILL!
tst_test.c:1125: INFO: If you are running on slow machine, try exporting LTP_TIMEOUT_MUL > 1 tst_test.c:1126: BROK: Test killed! (timeout?)

Hi!

Quoted 5 lines

For system(rhel7.6, s390x) without __NR_recvmmsg supported, run cve-2016-7117 result in timeout and killed by LTP framework. The root reason is tst_syscall break with cleanup() function calling in this trace path:

Quoted 7 lines

  tst_syscall(__NR_recvmmsg, ...)
    tst_brk()
      cleanup()
        tst_fzsync_pair_cleanup()
          SAFE_PTHREAD_JOIN(pair->thread_b, NULL);

cve-2016-7117 hung at here to wait for thread_b send_and_close()

finishing.

Quoted 3 lines

But thread_b fall into infinite loop because of tst_fzsync_wait_b without an extra condition to exit. Eventually, test get timeout error like:

Quoted 3 lines

cve-2016-7117.c:145: CONF: syscall(-1) __NR_recvmmsg not supported Test timeouted, sending SIGKILL!
tst_test.c:1125: INFO: If you are running on slow machine, try

exporting LTP_TIMEOUT_MUL > 1

Quoted 1 line

tst_test.c:1126: BROK: Test killed! (timeout?)

Looks like the complete solution will be more complex, so what about we do a simple solution that would make it to the release?

We can change the cve-2016-7117 to check if __NR_recvmmsg() is supported in the test setup(), then we can avoid this problem to begin with.