[v2] 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.

Signed-off-by: Li Wang <liwang@redhat.com>
Cc: Richard Palethorpe <rpalethorpe@suse.com>
---
 include/tst_fuzzy_sync.h | 39 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

Hi!
> Signed-off-by: Li Wang <liwang@redhat.com>
> Cc: Richard Palethorpe <rpalethorpe@suse.com>
> ---
>  include/tst_fuzzy_sync.h | 39 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/include/tst_fuzzy_sync.h b/include/tst_fuzzy_sync.h
> index de0402c9b..6814d8d34 100644
> --- a/include/tst_fuzzy_sync.h
> +++ b/include/tst_fuzzy_sync.h
> @@ -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);
> +}

As far as I can tell pthread_exit() is not async-signal-safe, so it
shouldn't be called from signal context.

>  /**
>   * 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;
> +}

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.

>  /**
>   * 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();
>  }
> -- 
> 2.14.5
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

Cyril Hrubis <chrubis@suse.cz> wrote:

> > +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?

> > +/**
> > + * 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;
> > +}
>
> 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.

Hello,

Li Wang <liwang@redhat.com> writes:

> Cyril Hrubis <chrubis@suse.cz> wrote:
>
>> > +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?
>
>> > +/**
>> > + * 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;
>> > +}
>>
>> 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.

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.

Hi!
> 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.

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.