fzsync: revoke thread_B when parent hit accidental break
PATCH v2 1/1 fzsync: revoke thread_B 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?)
To solve this problem, we're trying to use pthread_kill with an realtime signal and a signal handler to revoke the thread_B at abonormal situation. Also, we wrap thread B's main function 'run_b' as function 'wrap_run_b' to sets the singal handler at the start of the thread.
--- include/tst_fuzzy_sync.h | 39 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-)
include/tst_fuzzy_sync.h +37 -2
@@ -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"
@@ -156,7 +158,7 @@ struct tst_fzsync_pair {int a_cntr;/** Internal; Atomic counter used by fzsync_pair_wait() */ int b_cntr; -/** Internal; Used by tst_fzsync_pair_exit() and fzsync_pair_wait() */ +/** Internal; Used by tst_fzsync_run_b() to exit normally */ int exit;/** * The maximum desired execution time as a proportion of the timeout
@@ -217,13 +219,30 @@ static void tst_fzsync_pair_init(struct tst_fzsync_pair *pair)*/ static void tst_fzsync_pair_cleanup (struct tst_fzsync_pair *pair) { +int kill_ret; +if (pair->thread_b) {tst_atomic_store (1 , &pair->exit); + + kill_ret =pthread_kill (pair->thread_b,0 ); +if (kill_ret ==0 ) +pthread_kill (pair->thread_b,SIGUSR1 ); +else if (kill_ret ==ESRCH ) +tst_res (TINFO ,"thread_b is not exist" ); +else if (kill_ret ==EINVAL ) +tst_res (TINFO ,"Invalid signal was specified" ); +SAFE_PTHREAD_JOIN (pair->thread_b,NULL ); pair->thread_b =0 ; } } +static void sighandler (int sig) +{ +if (sig ==SIGUSR1 ) +pthread_exit (NULL ); +} +/** * Zero some stat fields *
@@ -235,6 +254,22 @@ static void tst_init_stat(struct tst_fzsync_stat *s) s->avg_dev =0 ; } +/** +* Wrap run_b for tst_fzsync_pair_reset to set the singal handler +* at the start of the thread B. +*/ +static void *wrap_run_b (void * run_b) +{ +void *(*real_run_b)(void *) = run_b; + +if (real_run_b) { +SAFE_SIGNAL (SIGUSR1 , sighandler); + (*real_run_b)(NULL ); + } + +return NULL ; +} +/** * Reset or initialise fzsync. *
@@ -272,7 +307,7 @@ static void tst_fzsync_pair_reset(struct tst_fzsync_pair *pair, pair->b_cntr =0 ; pair->exit =0 ;if (run_b) -SAFE_PTHREAD_CREATE (&pair->thread_b,0 , run_b,0 ); +SAFE_PTHREAD_CREATE (&pair->thread_b,0 , wrap_run_b, run_b); pair->exec_time_start = (float )tst_timeout_remaining (); }
Signature · 1 line
2.14.5
PATCH v2 1/1 fzsync: revoke thread_B when parent hit accidental break
Hi!
Quoted 45 lines
Signed-off-by Li Wang <[email protected]>Cc Richard Palethorpe <[email protected]>--- include/tst_fuzzy_sync.h | 39 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-)include/tst_fuzzy_sync.h +14
-1@@ -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" @@ -156,7 +158,7 @@ struct tst_fzsync_pair {int a_cntr;/** Internal; Atomic counter used by fzsync_pair_wait() */ int b_cntr; -/** Internal; Used by tst_fzsync_pair_exit() and fzsync_pair_wait() */ +/** Internal; Used by tst_fzsync_run_b() to exit normally */ int exit;/** * The maximum desired execution time as a proportion of the timeout @@ -217,13 +219,30 @@ static void tst_fzsync_pair_init(struct tst_fzsync_pair *pair)*/ static void tst_fzsync_pair_cleanup (struct tst_fzsync_pair *pair) { +int kill_ret; +if (pair->thread_b) {tst_atomic_store (1 , &pair->exit); + + kill_ret =pthread_kill (pair->thread_b,0 ); +if (kill_ret ==0 ) +pthread_kill (pair->thread_b,SIGUSR1 ); +else if (kill_ret ==ESRCH ) +tst_res (TINFO ,"thread_b is not exist" ); +else if (kill_ret ==EINVAL ) +tst_res (TINFO ,"Invalid signal was specified" ); +
include/tst_fuzzy_sync.h static void tst_fzsync_pair_init(struct tst_fzsync_pair *pair) · line 244
Quoted 10 lines
include/tst_fuzzy_sync.h +5
-0SAFE_PTHREAD_JOIN (pair->thread_b,NULL ); pair->thread_b =0 ; } } +static void sighandler (int sig) +{ +if (sig ==SIGUSR1 ) +pthread_exit (NULL ); +}
As far as I can tell pthread_exit() is not async-signal-safe, so it shouldn't be called from signal context.
Quoted 12 lines
include/tst_fuzzy_sync.h +5
-0/** * Zero some stat fields * @@ -235,6 +254,22 @@ static void tst_init_stat(struct tst_fzsync_stat *s) s->avg_dev =0 ; } +/** +* Wrap run_b for tst_fzsync_pair_reset to set the singal handler +* at the start of the thread B. +*/ +static void *wrap_run_b (void * run_b)
include/tst_fuzzy_sync.h static void tst_init_stat(struct tst_fzsync_stat *s) · line 271
Quoted 10 lines
include/tst_fuzzy_sync.h +10
-0+{ +void *(*real_run_b)(void *) = run_b; + +if (real_run_b) { +SAFE_SIGNAL (SIGUSR1 , sighandler); + (*real_run_b)(NULL ); + } + +return NULL ; +}
As far as I can tell the call to signal() will set the signal handler for all threads in the process, so there is no point in wrapping the run function like that, we can as well set up the signal handler before we start the b thread.
Quoted 18 lines
include/tst_fuzzy_sync.h +1
-1/** * Reset or initialise fzsync. * @@ -272,7 +307,7 @@ static void tst_fzsync_pair_reset(struct tst_fzsync_pair *pair, pair->b_cntr =0 ; pair->exit =0 ;if (run_b) -SAFE_PTHREAD_CREATE (&pair->thread_b,0 , run_b,0 ); +SAFE_PTHREAD_CREATE (&pair->thread_b,0 , wrap_run_b, run_b); pair->exec_time_start = (float )tst_timeout_remaining (); }Signature · 5 lines
2.14.5 -- Mailing list info: https://lists.linux.it/listinfo/ltp
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH v2 1/1 fzsync: revoke thread_B when parent hit accidental break
Quoted 8 lines · Cyril Hrubis
include/tst_fuzzy_sync.h static void tst_fzsync_pair_init(struct tst_fzsync_pair *pair) · line 244
Quoted 5 lines
include/tst_fuzzy_sync.h +5
-0+static void sighandler (int sig) +{ +if (sig ==SIGUSR1 ) +pthread_exit (NULL ); +}As far as I can tell
pthread_exit()is not async-signal-safe, so it shouldn't be called from signal context.
Right! This is really a good reminder. Maybe we could use _exit() as a replacement?
Quoted 12 lines
Quoted 12 lines
include/tst_fuzzy_sync.h +12
-0+/** +* Wrap run_b for tst_fzsync_pair_reset to set the singal handler +* at the start of the thread B. +*/ +static void *wrap_run_b (void * run_b) +{ +void *(*real_run_b)(void *) = run_b; + +if (real_run_b) { +SAFE_SIGNAL (SIGUSR1 , sighandler); + (*real_run_b)(NULL ); + }
Quoted 8 lines
include/tst_fuzzy_sync.h static void tst_init_stat(struct tst_fzsync_stat *s) · line 271
Quoted 3 lines
include/tst_fuzzy_sync.h +3
-0+ +return NULL ; +}As far as I can tell the call to
signal()will set the signal handler for all threads in the process, so there is no point in wrapping the run function like that, we can as well set up the signal handler before we start the b thread.
To wrap 'run_b' and set up signal handler only for that one thread is to make things more precise, but as you pointed out it seems made an unnecessary wrapping. Currently I don't have a perfect idea for solving that and need thinking for a while. Anyway, if someone can come up with a better solution that'd be appreciated.
Signature · 2 lines
Regards, Li Wang
PATCH v2 1/1 fzsync: revoke thread_B when parent hit accidental break
Hello,
Quoted 35 lines · Li Wang
Quoted 8 lines · Cyril Hrubis
include/tst_fuzzy_sync.h static void tst_fzsync_pair_init(struct tst_fzsync_pair *pair) · line 244
Quoted 5 lines
include/tst_fuzzy_sync.h +5
-0+static void sighandler (int sig) +{ +if (sig ==SIGUSR1 ) +pthread_exit (NULL ); +}As far as I can tell
pthread_exit()is not async-signal-safe, so it shouldn't be called from signal context.Right! This is really a good reminder. Maybe we could use
_exit()as a replacement?Quoted 12 lines
Quoted 12 lines
include/tst_fuzzy_sync.h +12
-0+/** +* Wrap run_b for tst_fzsync_pair_reset to set the singal handler +* at the start of the thread B. +*/ +static void *wrap_run_b (void * run_b) +{ +void *(*real_run_b)(void *) = run_b; + +if (real_run_b) { +SAFE_SIGNAL (SIGUSR1 , sighandler); + (*real_run_b)(NULL ); + }Quoted 8 lines
include/tst_fuzzy_sync.h static void tst_init_stat(struct tst_fzsync_stat *s) · line 271
Quoted 3 lines
include/tst_fuzzy_sync.h +3
-0+ +return NULL ; +}As far as I can tell the call to
signal()will set the signal handler for all threads in the process, so there is no point in wrapping the run function like that, we can as well set up the signal handler before we start the b thread.
Quoted 5 lines
To wrap '
run_b' and set up signal handler only for that one thread is to make things more precise, but as you pointed out it seems made an unnecessary wrapping. Currently I don't have a perfect idea for solving that and need thinking for a while. Anyway, if someone can come up with a better solution that'd be appreciated.
Actually I think the correct way to kill a thread is with pthread_cancel.
AFAICT we could call:
pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS) pthread_setcancelstate(PTHREAD_CANCEL_ENABLE)
at the start of thread B, then call:
pthread_cancel(<thread_b_pid>)
in thread A if we can't rely on setting the exit flag to work
There are other ways to use pthread_cancel, but this looks like the simplest for our use case.
--
Thank you,
Richard.
PATCH v2 1/1 fzsync: revoke thread_B when parent hit accidental break
Hi!
Quoted 8 lines
Actually I think the correct way to kill a thread is with
pthread_cancel.AFAICT we could call:
pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS)pthread_setcancelstate(PTHREAD_CANCEL_ENABLE)
Quoted 8 lines
at the start of thread B, then call:
pthread_cancel(<thread_b_pid>)in thread A if we can't rely on setting the exit flag to work
There are other ways to use
pthread_cancel, but this looks like the simplest for our use case.
That may work, at least this avoids the need to have a signal handler in the first place.
But I would still like to push the simpler solution before the release, i.e. checking for recvmmsg support in the test setup.
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH 1/1 Check recvmmsg exists before entering fuzzy loop
Avoid thread B entering infinite loop if recvmmsg doesn't exist causing tst_brk to be called and thread A to make an ungraceful exit.
A more general fix can be added to tst_fuzzy_sync as well, but will take longer to develop.
--- testcases/cve/cve-2016-7117.c | 4 ++++ 1 file changed, 4 insertions(+)
testcases/cve/cve-2016-7117.c +4 -0
@@ -99,6 +99,10 @@ static void setup(void) { fzsync_pair.min_samples =10000 ; +tst_syscall (__NR_recvmmsg); +if (errno ==ENOSYS ) +tst_brk (TCONF ,"recvmmsg not supported" ); +tst_fzsync_pair_init (&fzsync_pair); }
Signature · 1 line
2.19.1
PATCH 1/1 Check recvmmsg exists before entering fuzzy loop
Hi Richard,
Quoted 5 lines · Richard Palethorpe
Avoid thread B entering infinite loop if recvmmsg doesn't exist causing
tst_brkto be called and thread A to make an ungraceful exit.A more general fix can be added to
tst_fuzzy_syncas well, but will take longer to develop.
Yes, now we just add a syscall checking in setup() as a workaround, for the fzsync library issue we could take more time to find a best solution after this new LTP releasing.
Quoted 12 lines
Signed-off-by Richard Palethorpe <[email protected]>Reported-by Li Wang <[email protected]>--- testcases/cve/cve-2016-7117.c | 4 ++++ 1 file changed, 4 insertions(+)testcases/cve/cve-2016-7117.c +0
-0
testcases/cve/cve-2016-7117.c static void setup(void) · line 102
Quoted 4 lines
testcases/cve/cve-2016-7117.c +1
-0@@ -99,6 +99,10 @@ static void setup(void) { fzsync_pair.min_samples =10000 ; +tst_syscall (__NR_recvmmsg);
maybe adding with useless parameters?
tst_syscall(__NR_recvmmsg, 0 ,0, 0, 0, 0);
testcases/cve/cve-2016-7117.c static void setup(void) · line 104
Quoted 2 lines
testcases/cve/cve-2016-7117.c +2
-0+if (errno ==ENOSYS ) +tst_brk (TCONF ,"recvmmsg not supported" );
Seems errno check is not necessary here, the macro tst_syscall() has already defined with tst_brk(TCONF,) calling when errno is ENOSYS.
FYI:
#define tst_syscall(NR, ...) ({ \\
int tst_ret; \\
if (NR == __LTP__NR_INVALID_SYSCALL) { \\
errno = ENOSYS; \\
tst_ret = -1; \\
} else { \\
tst_ret = syscall(NR, ##__VA_ARGS__); \\
} \\
if (tst_ret == -1 && errno == ENOSYS) { \\
tst_brk(TCONF, "syscall(%d) " #NR " not supported", NR); \\
} \\
tst_ret; \\
})Quoted 8 lines
testcases/cve/cve-2016-7117.c +1
-1+tst_fzsync_pair_init (&fzsync_pair); } --2.19.1
--
Regards,
Li Wang
PATCH 1/1 Check recvmmsg exists before entering fuzzy loop
Hello,
Quoted 29 lines · Li Wang
Hi Richard,
Quoted 5 lines · Richard Palethorpe
Avoid thread B entering infinite loop if recvmmsg doesn't exist causing
tst_brkto be called and thread A to make an ungraceful exit.A more general fix can be added to
tst_fuzzy_syncas well, but will take longer to develop.Yes, now we just add a syscall checking in
setup()as a workaround, for the fzsync library issue we could take more time to find a best solution after this new LTP releasing.Quoted 15 lines
Signed-off-by Richard Palethorpe <[email protected]>Reported-by Li Wang <[email protected]>--- testcases/cve/cve-2016-7117.c | 4 ++++ 1 file changed, 4 insertions(+)testcases/cve/cve-2016-7117.c +0
-0@@ -99,6 +99,10 @@ static void setup(void) { fzsync_pair.min_samples =10000 ;
Quoted 4 lines
testcases/cve/cve-2016-7117.c static void setup(void) · line 102
Quoted 1 line
testcases/cve/cve-2016-7117.c +1
-0+tst_syscall (__NR_recvmmsg);maybe adding with useless parameters?
tst_syscall(__NR_recvmmsg, 0 ,0, 0, 0, 0);
I'm not sure it is necessary, but it is probably safer, so I will change it.
Quoted 9 lines
testcases/cve/cve-2016-7117.c static void setup(void) · line 104
Quoted 3 lines
testcases/cve/cve-2016-7117.c +2
-0+if (errno ==ENOSYS ) +tst_brk (TCONF ,"recvmmsg not supported" );Seems errno check is not necessary here, the macro
tst_syscall()has already defined withtst_brk(TCONF,)calling when errno is ENOSYS.FYI:
Quoted 8 lines
#define tst_syscall(NR, ...) ({ \\
int tst_ret; \\
if (NR == __LTP__NR_INVALID_SYSCALL) { \\
errno = ENOSYS; \\
tst_ret = -1; \\
} else { \\
tst_ret = syscall(NR, ##__VA_ARGS__); \\
} \\Ah I misread the if statement, thanks.
Quoted 14 lines
if (tst_ret == -1 && errno == ENOSYS) { \\ tst_brk(TCONF, "syscall(%d) " #NR " not supported", NR); \\ } \\ tst_ret; \\ })Quoted 8 lines
testcases/cve/cve-2016-7117.c +1
-1+tst_fzsync_pair_init (&fzsync_pair); } --2.19.1
--
Thank you,
Richard.
PATCH v2 1/1 Check recvmmsg exists before entering fuzzy loop
In reply to Li WangAvoid thread B entering infinite loop if recvmmsg doesn't exist causing tst_brk to be called and thread A to make an ungraceful exit.
A more general fix can be added to tst_fuzzy_sync as well, but will take longer to develop.
--- testcases/cve/cve-2016-7117.c | 2 ++ 1 file changed, 2 insertions(+)
testcases/cve/cve-2016-7117.c +2 -0
@@ -99,6 +99,8 @@ static void setup(void) { fzsync_pair.min_samples =10000 ; +tst_syscall (__NR_recvmmsg,0 ,0 ,0 ,0 ,0 ); +tst_fzsync_pair_init (&fzsync_pair); }
Signature · 1 line
2.19.1
PATCH v2 1/1 Check recvmmsg exists before entering fuzzy loop
Quoted 9 lines · Richard Palethorpe
Avoid thread B entering infinite loop if recvmmsg doesn't exist causing
tst_brkto be called and thread A to make an ungraceful exit.A more general fix can be added to
tst_fuzzy_syncas well, but will take longer to develop.Signed-off-by Richard Palethorpe <[email protected]>Reported-by Li Wang <[email protected]>
Signature · 2 lines
Regards, Li Wang
PATCH v2 1/1 Check recvmmsg exists before entering fuzzy loop
In reply to Richard PalethorpeHi!
Pushed, thanks.
Signature · 2 lines
Cyril Hrubis [email protected]