[LTP] [PATCH v2] fzsync: revoke thread_B when parent hit accidental break

[LTP] [PATCH v2] fzsync: revoke thread_B when parent hit accidental break

[Li Wang]

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(-)

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

[LTP] [PATCH v2] fzsync: revoke thread_B when parent hit accidental break

[Cyril Hrubis]

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

[LTP] [PATCH v2] fzsync: revoke thread_B when parent hit accidental break

[Li Wang]

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.

-- 
Regards,
Li Wang

[LTP] [PATCH v2] fzsync: revoke thread_B when parent hit accidental break

[Richard Palethorpe]

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.

[LTP] [PATCH v2] fzsync: revoke thread_B when parent hit accidental break

[Cyril Hrubis]

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.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] Check recvmmsg exists before entering fuzzy loop

[Richard Palethorpe]

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.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
Reported-by: Li Wang <liwang@redhat.com>
---
 testcases/cve/cve-2016-7117.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/testcases/cve/cve-2016-7117.c b/testcases/cve/cve-2016-7117.c
index 6290af077..effab083d 100644
--- a/testcases/cve/cve-2016-7117.c
+++ b/testcases/cve/cve-2016-7117.c
@@ -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);
 }
 
-- 
2.19.1

[LTP] [PATCH] Check recvmmsg exists before entering fuzzy loop

[Li Wang]

Hi Richard,

Richard Palethorpe <rpalethorpe@suse.com> wrote:

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

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.

>
> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> Reported-by: Li Wang <liwang@redhat.com>
> ---
>  testcases/cve/cve-2016-7117.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/testcases/cve/cve-2016-7117.c b/testcases/cve/cve-2016-7117.c
> index 6290af077..effab083d 100644
> --- a/testcases/cve/cve-2016-7117.c
> +++ b/testcases/cve/cve-2016-7117.c
> @@ -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);

> +       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; \\
})

> +
>         tst_fzsync_pair_init(&fzsync_pair);
>  }

>
> --
> 2.19.1
>


--
Regards,
Li Wang

[LTP] [PATCH] Check recvmmsg exists before entering fuzzy loop

[Richard Palethorpe]

Hello,

Li Wang <liwang@redhat.com> writes:

> Hi Richard,
>
> Richard Palethorpe <rpalethorpe@suse.com> wrote:
>
>> 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.
>
> 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.
>
>>
>> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
>> Reported-by: Li Wang <liwang@redhat.com>
>> ---
>>  testcases/cve/cve-2016-7117.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/testcases/cve/cve-2016-7117.c b/testcases/cve/cve-2016-7117.c
>> index 6290af077..effab083d 100644
>> --- a/testcases/cve/cve-2016-7117.c
>> +++ b/testcases/cve/cve-2016-7117.c
>> @@ -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);

I'm not sure it is necessary, but it is probably safer, so I will change
it.

>
>> +       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__); \\
>         } \\

Ah I misread the if statement, thanks.

>         if (tst_ret == -1 && errno == ENOSYS) { \\
>                 tst_brk(TCONF, "syscall(%d) " #NR " not supported", NR); \\
>         } \\
>         tst_ret; \\
> })
>
>> +
>>         tst_fzsync_pair_init(&fzsync_pair);
>>  }
>
>>
>> --
>> 2.19.1
>>


--
Thank you,
Richard.

[LTP] [PATCH v2] Check recvmmsg exists before entering fuzzy loop

[Richard Palethorpe]

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.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
Reported-by: Li Wang <liwang@redhat.com>
---
 testcases/cve/cve-2016-7117.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/testcases/cve/cve-2016-7117.c b/testcases/cve/cve-2016-7117.c
index 6290af077..db62909b3 100644
--- a/testcases/cve/cve-2016-7117.c
+++ b/testcases/cve/cve-2016-7117.c
@@ -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);
 }
 
-- 
2.19.1

[LTP] [PATCH v2] Check recvmmsg exists before entering fuzzy loop

[Li Wang]

On Thu, Jan 10, 2019 at 6:00 PM Richard Palethorpe <rpalethorpe@suse.com> wrote:
>
> 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.
>
> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> Reported-by: Li Wang <liwang@redhat.com>
Reviewed-by: Li Wang <liwang@redhat.com>

-- 
Regards,
Li Wang

[LTP] [PATCH v2] Check recvmmsg exists before entering fuzzy loop

[Cyril Hrubis]

Hi!
Pushed, thanks.

-- 
Cyril Hrubis
chrubis@suse.cz