lib: Allow user to easily get LTP_TIMEOUT_MUL value

PATCH New Richard Palethorpe 4 participants 12 messages Aug 16, 2018 – Aug 17, 2018 Library code

lore

Share & tools

lore

Richard Palethorpe

author

2018-08-16 12:00 UTC #0 lore raw

PATCH v3 1/4 lib: Allow user to easily get LTP_TIMEOUT_MUL value

Signed-off-by Richard Palethorpe <[email protected]>

---
 include/tst_test.h |  1 +
 lib/tst_test.c     | 23 ++++++++++++++---------
 2 files changed, 15 insertions(+), 9 deletions(-)

include/tst_test.h +1 -0

index 98dacf387..0ad9025f2 100644
@@ -217,6 +217,7 @@ const char *tst_strsig(int sig);
  */
 const char *tst_strstatus(int status);
 
+float tst_timeout_mul(void);
 void tst_set_timeout(int timeout);
 
 #ifndef TST_NO_DEFAULT_MAIN

lib/tst_test.c +14 -9

index 2f3d357d2..f760e05b3 100644
@@ -992,26 +992,31 @@ static void sigint_handler(int sig LTP_ATTRIBUTE_UNUSED)
 	}
 }
 
-void tst_set_timeout(int timeout)
+float tst_timeout_mul(void)
 {
 	char *mul = getenv("LTP_TIMEOUT_MUL");
 
-	if (timeout == -1) {
-		tst_res(TINFO, "Timeout per run is disabled");
-		return;
-	}
-
-	results->timeout = timeout;
-
 	if (mul) {
 		float m = atof(mul);
 
 		if (m < 1)
 			tst_brk(TBROK, "Invalid timeout multiplier '%s'", mul);
 
-		results->timeout = results->timeout * m + 0.5;
+		return m;
+	}
+
+	return 1;
+}
+
+void tst_set_timeout(int timeout)
+{
+	if (timeout == -1) {
+		tst_res(TINFO, "Timeout per run is disabled");
+		return;
 	}
 
+	results->timeout = timeout * tst_timeout_mul() + 0.5;
+
 	tst_res(TINFO, "Timeout per run is %uh %02um %02us",
 		results->timeout/3600, (results->timeout%3600)/60,
 		results->timeout % 60);

Signature · 1 line

2.18.0

Richard Palethorpe

author

2018-08-16 12:00 UTC #1 lore raw

PATCH v3 2/4 tst_timer: Convert to new library

So we can use functions from new lib.

Signed-off-by Richard Palethorpe <[email protected]>

---
 lib/tst_timer.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

lib/tst_timer.c +8 -6

index 53ff36777..d5e4c49d2 100644
@@ -23,7 +23,9 @@
 
 #include <errno.h>
 
-#include "test.h"
+#define TST_NO_DEFAULT_MAIN
+
+#include "tst_test.h"
 #include "tst_timer.h"
 #include "tst_clocks.h"
 #include "lapi/posix_clocks.h"

@@ -59,13 +61,13 @@ void tst_timer_check(clockid_t clk_id)
 {
 	if (tst_clock_gettime(clk_id, &start_time)) {
 		if (errno == EINVAL) {
-			tst_brkm(TCONF, NULL,
+			tst_brk(TCONF,
 			         "Clock id %s(%u) not supported by kernel",
 				 clock_name(clk_id), clk_id);
 			return;
 		}
 
-		tst_brkm(TBROK | TERRNO, NULL, "tst_clock_gettime() failed");
+		tst_brk(TBROK | TERRNO, "tst_clock_gettime() failed");
 	}
 }

@@ -74,7 +76,7 @@ void tst_timer_start(clockid_t clk_id)
 	clock_id = clk_id;
 
 	if (tst_clock_gettime(clock_id, &start_time))
-		tst_resm(TWARN | TERRNO, "tst_clock_gettime() failed");
+		tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
 }
 
 int tst_timer_expired_ms(long long ms)

@@ -82,7 +84,7 @@ int tst_timer_expired_ms(long long ms)
 	struct timespec cur_time;
 
 	if (tst_clock_gettime(clock_id, &cur_time))
-		tst_resm(TWARN | TERRNO, "tst_clock_gettime() failed");
+		tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
 
 	return tst_timespec_diff_ms(cur_time, start_time) >= ms;
 }

@@ -90,7 +92,7 @@ int tst_timer_expired_ms(long long ms)
 void tst_timer_stop(void)
 {
 	if (tst_clock_gettime(clock_id, &stop_time))
-		tst_resm(TWARN | TERRNO, "tst_clock_gettime() failed");
+		tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
 }
 
 struct timespec tst_timer_elapsed(void)

Signature · 1 line

2.18.0

Cyril Hrubis

2018-08-16 12:08 UTC #2 lore raw

PATCH v3 2/4 tst_timer: Convert to new library

Hi!

Quoted 1 line

So we can use functions from new lib.

We still do have oldlib tests that link against this code.

Just grep for tst_timer_start, there are at least:

testcases/kernel/syscalls/nanosleep/nanosleep02.c
testcases/kernel/syscalls/fcntl/fcntl33.c

That has to be converted before we can apply this patch.

Signature · 2 lines

Cyril Hrubis
[email protected]

Richard Palethorpe

author

2018-08-16 12:00 UTC #3 lore raw

PATCH v3 3/4 tst_timer: Create interface for using multiple timers

In reply to Richard Palethorpe

Encapsulate information about a stopwatch timer in a struct and provide methods for operating on that timer. This allows test authors to create multiple timers in parallel.

Signed-off-by Richard Palethorpe <[email protected]>

---
 include/tst_timer.h | 30 ++++++++++++++++++++++++++++++
 lib/tst_timer.c     | 17 +++++++++++++++++
 2 files changed, 47 insertions(+)

include/tst_timer.h +30 -0

index 0fd7ed6cf..06deae88a 100644
@@ -34,6 +34,20 @@
 #include <sys/time.h>
 #include <time.h>
 
+/**
+ * Encapsulates a stopwatch timer.
+ *
+ * Useful when you need to use multiple timers in a single test.
+ */
+struct tst_timer {
+	/** The POSIX clock type  */
+	clockid_t clock_id;
+	/** How much time can pass before tst_timer_expired_st() returns true */
+	struct timespec limit;
+	/** Where to start measuring time from */
+	struct timespec start_time;
+};
+
 /*
  * Converts timespec to microseconds.
  */

@@ -251,6 +265,14 @@ void tst_timer_check(clockid_t clk_id);
  */
 void tst_timer_start(clockid_t clk_id);
 
+/**
+ * Sets the start time for timer tim.
+ *
+ * @relates tst_timer
+ * @param tim The timer to start
+ */
+void tst_timer_start_st(struct tst_timer *tim);
+
 /*
  * Returns true if timer started by tst_timer_start() has been running for
  * longer than ms seconds.

@@ -259,6 +281,14 @@ void tst_timer_start(clockid_t clk_id);
  */
 int tst_timer_expired_ms(long long ms);
 
+/**
+ * Returns true if timer tim has been running for longer than tst_timer::limit
+ *
+ * @relates tst_timer
+ * @param tim The timer to check
+ */
+int tst_timer_expired_st(struct tst_timer *tim);
+
 /*
  * Marks timer end time.
  */

lib/tst_timer.c +17 -0

index d5e4c49d2..baec18280 100644
@@ -79,6 +79,12 @@ void tst_timer_start(clockid_t clk_id)
 		tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
 }
 
+void tst_timer_start_st(struct tst_timer *tim)
+{
+	if (tst_clock_gettime(tim->clock_id, &tim->start_time))
+		tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
+}
+
 int tst_timer_expired_ms(long long ms)
 {
 	struct timespec cur_time;

@@ -89,6 +95,17 @@ int tst_timer_expired_ms(long long ms)
 	return tst_timespec_diff_ms(cur_time, start_time) >= ms;
 }
 
+int tst_timer_expired_st(struct tst_timer *tim)
+{
+	struct timespec cur_time;
+
+	if (tst_clock_gettime(tim->clock_id, &cur_time))
+		tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
+
+	return tst_timespec_lt(tim->limit,
+			       tst_timespec_diff(cur_time, tim->start_time));
+}
+
 void tst_timer_stop(void)
 {
 	if (tst_clock_gettime(clock_id, &stop_time))

Signature · 1 line

2.18.0

Cyril Hrubis

2018-08-16 12:22 UTC #4 lore raw

PATCH v3 3/4 tst_timer: Create interface for using multiple timers

Hi!

Quoted 18 lines

include/tst_timer.h +14 -0

+/**
+ * Encapsulates a stopwatch timer.
+ *
+ * Useful when you need to use multiple timers in a single test.
+ */
+struct tst_timer {
+	/** The POSIX clock type  */
+	clockid_t clock_id;
+	/** How much time can pass before tst_timer_expired_st() returns true */
+	struct timespec limit;
+	/** Where to start measuring time from */
+	struct timespec start_time;
+};
+
 /*
  * Converts timespec to microseconds.
  */

include/tst_timer.h void tst_timer_check(clockid_t clk_id); · line 274

Quoted 10 lines

include/tst_timer.h +7 -0

@@ -251,6 +265,14 @@ void tst_timer_check(clockid_t clk_id);
  */
 void tst_timer_start(clockid_t clk_id);
 
+/**
+ * Sets the start time for timer tim.
+ *
+ * @relates tst_timer
+ * @param tim The timer to start
+ */
+void tst_timer_start_st(struct tst_timer *tim);

So user is expected to fill in the tst_timer structure before this call is called, right?

Maybe it would be easier to pass the timeout and CLOCK_ID to this function and keep the initialization in the test library, that way we can also check if the timer is supported first before we return from the start function.

And lastly but not least, we were also discussion special timer id, something as TST_TIMER_DEFAULT that would select appropriate timer, althoug I guess that CLOCK_MONOTONIC is supporoted on anything that we care for these days.

Quoted 59 lines

include/tst_timer.h +8 -0

 /*
  * Returns true if timer started by tst_timer_start() has been running for
  * longer than ms seconds.

@@ -259,6 +281,14 @@ void tst_timer_start(clockid_t clk_id);
  */
 int tst_timer_expired_ms(long long ms);
 
+/**
+ * Returns true if timer tim has been running for longer than tst_timer::limit
+ *
+ * @relates tst_timer
+ * @param tim The timer to check
+ */
+int tst_timer_expired_st(struct tst_timer *tim);
+
 /*
  * Marks timer end time.
  */

lib/tst_timer.c +17 -0

index d5e4c49d2..baec18280 100644
@@ -79,6 +79,12 @@ void tst_timer_start(clockid_t clk_id)
 		tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
 }
 
+void tst_timer_start_st(struct tst_timer *tim)
+{
+	if (tst_clock_gettime(tim->clock_id, &tim->start_time))
+		tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
+}
+
 int tst_timer_expired_ms(long long ms)
 {
 	struct timespec cur_time;

@@ -89,6 +95,17 @@ int tst_timer_expired_ms(long long ms)
 	return tst_timespec_diff_ms(cur_time, start_time) >= ms;
 }
 
+int tst_timer_expired_st(struct tst_timer *tim)
+{
+	struct timespec cur_time;
+
+	if (tst_clock_gettime(tim->clock_id, &cur_time))
+		tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
+
+	return tst_timespec_lt(tim->limit,
+			       tst_timespec_diff(cur_time, tim->start_time));
+}
+
 void tst_timer_stop(void)
 {
 	if (tst_clock_gettime(clock_id, &stop_time))

Signature · 5 lines

2.18.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Signature · 2 lines

Cyril Hrubis
[email protected]

Cyril Hrubis

2018-08-16 12:27 UTC #5 lore raw

PATCH v3 3/4 tst_timer: Create interface for using multiple timers

Hi!

Quoted 5 lines

Quoted 5 lines
+/**
+ * Encapsulates a stopwatch timer.
+ *
+ * Useful when you need to use multiple timers in a single test.
+ */

Quoted 8 lines

include/tst_timer.h line 49

Quoted 8 lines

include/tst_timer.h +8 -0

+struct tst_timer {
+	/** The POSIX clock type  */
+	clockid_t clock_id;
+	/** How much time can pass before tst_timer_expired_st() returns true */
+	struct timespec limit;
+	/** Where to start measuring time from */
+	struct timespec start_time;
+};

On a related note, do you really think that we will need nsec resoltion for the timeout timers? I would say that keeping the limit as unsigned integer holding number of seconds would be more than enough.

Signature · 2 lines

Cyril Hrubis
[email protected]

Richard Palethorpe

2018-08-16 13:06 UTC #6 lore raw

PATCH v3 3/4 tst_timer: Create interface for using multiple timers

Hello,

Quoted 10 lines · Cyril Hrubis

Hi!

Quoted 9 lines

include/tst_timer.h +9 -0

+/**
+ * Encapsulates a stopwatch timer.
+ *
+ * Useful when you need to use multiple timers in a single test.
+ */
+struct tst_timer {
+	/** The POSIX clock type  */
+	clockid_t clock_id;
+	/** How much time can pass before tst_timer_expired_st() returns true */

Quoted 8 lines

Quoted 4 lines
include/tst_timer.h line 49
Quoted 4 lines
include/tst_timer.h +4 -0
+	struct timespec limit;
+	/** Where to start measuring time from */
+	struct timespec start_time;
+};
On a related note, do you really think that we will need nsec resoltion for the timeout timers? I would say that keeping the limit as unsigned integer holding number of seconds would be more than enough.

I agree that seconds are appropriate for test timeouts, but it is practically 'free' to use the struct here because of the existing API to compare timespec structs. Also I might use this interface to time syscalls and maybe remove some code duplication.

--
Thank you,
Richard.

Richard Palethorpe

2018-08-16 12:58 UTC #7 lore raw

PATCH v3 3/4 tst_timer: Create interface for using multiple timers

In reply to Cyril Hrubis

Hello,

Quoted 33 lines · Cyril Hrubis

Hi!

Quoted 18 lines

include/tst_timer.h +14 -0

+/**
+ * Encapsulates a stopwatch timer.
+ *
+ * Useful when you need to use multiple timers in a single test.
+ */
+struct tst_timer {
+	/** The POSIX clock type  */
+	clockid_t clock_id;
+	/** How much time can pass before tst_timer_expired_st() returns true */
+	struct timespec limit;
+	/** Where to start measuring time from */
+	struct timespec start_time;
+};
+
 /*
  * Converts timespec to microseconds.
  */

include/tst_timer.h void tst_timer_check(clockid_t clk_id); · line 274

Quoted 10 lines

include/tst_timer.h +7 -0

@@ -251,6 +265,14 @@ void tst_timer_check(clockid_t clk_id);
  */
 void tst_timer_start(clockid_t clk_id);
 
+/**
+ * Sets the start time for timer tim.
+ *
+ * @relates tst_timer
+ * @param tim The timer to start
+ */
+void tst_timer_start_st(struct tst_timer *tim);

So user is expected to fill in the tst_timer structure before this call is called, right?

Quoted 4 lines

Maybe it would be easier to pass the timeout and CLOCK_ID to this function and keep the initialization in the test library, that way we can also check if the timer is supported first before we return from the start function.

Well if the clock is not supported and the call fails, then the user will have to deal with that at the call site of tst_timer_start which is not very convenient for tst_fuzzy_sync. I would rather have a seperate initialisation function for the tst_timer struct which can be called in the setup function. e.g. tst_timer_init[_st](clock_id, timeout, ...).

Quoted 5 lines

And lastly but not least, we were also discussion special timer id, something as TST_TIMER_DEFAULT that would select appropriate timer, althoug I guess that CLOCK_MONOTONIC is supporoted on anything that we care for these days.

Yeah, fuzzy_sync just defines CLOCK_MONOTIC to CLOCK_MONOTONIC_RAW if possible and uses that. Not had any reports of CLOCK_MONOTONIC being unavailabe.

Quoted 58 lines

include/tst_timer.h +8 -0

 /*
  * Returns true if timer started by tst_timer_start() has been running for
  * longer than ms seconds.

@@ -259,6 +281,14 @@ void tst_timer_start(clockid_t clk_id);
  */
 int tst_timer_expired_ms(long long ms);
 
+/**
+ * Returns true if timer tim has been running for longer than tst_timer::limit
+ *
+ * @relates tst_timer
+ * @param tim The timer to check
+ */
+int tst_timer_expired_st(struct tst_timer *tim);
+
 /*
  * Marks timer end time.
  */

lib/tst_timer.c +17 -1

index d5e4c49d2..baec18280 100644
@@ -79,6 +79,12 @@ void tst_timer_start(clockid_t clk_id)
 		tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
 }
 
+void tst_timer_start_st(struct tst_timer *tim)
+{
+	if (tst_clock_gettime(tim->clock_id, &tim->start_time))
+		tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
+}
+
 int tst_timer_expired_ms(long long ms)
 {
 	struct timespec cur_time;

@@ -89,6 +95,17 @@ int tst_timer_expired_ms(long long ms)
 	return tst_timespec_diff_ms(cur_time, start_time) >= ms;
 }
 
+int tst_timer_expired_st(struct tst_timer *tim)
+{
+	struct timespec cur_time;
+
+	if (tst_clock_gettime(tim->clock_id, &cur_time))
+		tst_res(TWARN | TERRNO, "tst_clock_gettime() failed");
+
+	return tst_timespec_lt(tim->limit,
+			       tst_timespec_diff(cur_time, tim->start_time));
+}
+
 void tst_timer_stop(void)
 {
 	if (tst_clock_gettime(clock_id, &stop_time))
--

2.18.0

Quoted 2 lines

Quoted 2 lines
--
Mailing list info: https://lists.linux.it/listinfo/ltp

--
Thank you,
Richard.

Cyril Hrubis

2018-08-16 12:59 UTC #8 lore raw

PATCH v3 3/4 tst_timer: Create interface for using multiple timers

Hi!

Quoted 7 lines

Well if the clock is not supported and the call fails, then the user will have to deal with that at the call site of tst_timer_start which is not very convenient for tst_fuzzy_sync. I would rather have a seperate initialisation function for the tst_timer struct which can be called in the setup function. e.g. tst_timer_init[_st](clock_id, timeout, ...).
Quoted 1 line

Quoted 8 lines

Quoted 4 lines
And lastly but not least, we were also discussion special timer id, something as TST_TIMER_DEFAULT that would select appropriate timer, althoug I guess that CLOCK_MONOTONIC is supporoted on anything that we care for these days.
Yeah, fuzzy_sync just defines CLOCK_MONOTIC to CLOCK_MONOTONIC_RAW if possible and uses that. Not had any reports of CLOCK_MONOTONIC being unavailabe.

Maybe we can just do:

#ifdef CLOCK_MONOTONIC_RAW

# define TST_CLOCK_DEFAULT CLOCK_MONOTONIC_RAW
#else
# define TST_CLOCK_DEFAULT CLOCK_MONOTONIC
#endif

And be done with it.

Signature · 2 lines

Cyril Hrubis
[email protected]

Richard Palethorpe

author

2018-08-16 12:00 UTC #9 lore raw

PATCH v3 4/4 fzsync: Limit execution time to prevent test timeouts

In reply to Richard Palethorpe

Use the tst_timer library to check how long the main test loop has been running for and break the loop if it exceeds (60 * LTP_TIMEOUT_MUL) seconds. This prevents an overall test timeout which is reported as a failure.

Signed-off-by Richard Palethorpe <[email protected]>

---
 include/tst_fuzzy_sync.h                      | 50 +++++++++++++------
 lib/newlib_tests/test16.c                     |  4 +-
 testcases/cve/cve-2014-0196.c                 |  4 +-
 testcases/cve/cve-2016-7117.c                 |  6 ++-
 testcases/cve/cve-2017-2671.c                 |  4 +-
 testcases/kernel/syscalls/inotify/inotify09.c |  5 +-
 6 files changed, 51 insertions(+), 22 deletions(-)

include/tst_fuzzy_sync.h +34 ~~-16~~

index bcc625e24..5e0ff3602 100644
@@ -42,6 +42,7 @@
 #include <time.h>
 #include <math.h>
 #include "tst_atomic.h"
+#include "tst_timer.h"
 
 #ifndef CLOCK_MONOTONIC_RAW
 # define CLOCK_MONOTONIC_RAW CLOCK_MONOTONIC

@@ -66,6 +67,7 @@
  * @a_cntr: Internal; Atomic counter used by fzsync_pair_wait()
  * @b_cntr: Internal; Atomic counter used by fzsync_pair_wait()
  * @exit: Internal; Used by tst_fzsync_pair_exit() and fzsync_pair_wait()
+ * @timer: Internal; Used to limit the execution time.
  *
  * This contains all the necessary state for synchronising two points A and
  * B. Where A is the time of an event in one process and B is the time of an

@@ -87,6 +89,7 @@ struct tst_fzsync_pair {
 	int a_cntr;
 	int b_cntr;
 	int exit;
+	struct tst_timer timer;
 };
 
 /**

@@ -99,6 +102,15 @@ struct tst_fzsync_pair {
 	.info_gap = 0x7FFFF     \
 }
 
+static void tst_fzsync_pair_reset(struct tst_fzsync_pair *pair)
+{
+	pair->timer.clock_id = CLOCK_MONOTONIC_RAW;
+	pair->timer.limit.tv_sec = 60 * tst_timeout_mul();
+	pair->timer.limit.tv_nsec = 0;
+
+	tst_timer_start_st(&pair->timer);
+}
+
 /**
  * tst_fzsync_pair_info - Print some synchronisation statistics
  */

@@ -265,9 +277,7 @@ static inline int tst_fzsync_pair_wait(struct tst_fzsync_pair *pair,
 			;
 	}
 
-	/* Only exit if we have done the same amount of work as the other thread */
-	return !tst_atomic_load(&pair->exit) ||
-	  tst_atomic_load(other_cntr) <= tst_atomic_load(our_cntr);
+	return !tst_atomic_load(&pair->exit);
 }
 
 static inline int tst_fzsync_wait_a(struct tst_fzsync_pair *pair)

@@ -280,6 +290,17 @@ static inline int tst_fzsync_wait_b(struct tst_fzsync_pair *pair)
 	return tst_fzsync_pair_wait(pair, &pair->b_cntr, &pair->a_cntr);
 }
 
+/**
+ * tst_fzsync_pair_exit - Signal that the other thread should exit
+ *
+ * Causes tst_fzsync_pair_wait() and tst_fzsync_pair_wait_update() to return
+ * 0.
+ */
+static inline void tst_fzsync_pair_exit(struct tst_fzsync_pair *pair)
+{
+	tst_atomic_store(1, &pair->exit);
+}
+
 /**
  * tst_fzsync_pair_wait_update_{a,b} - Wait and then recalculate
  *

@@ -303,8 +324,16 @@ static inline int tst_fzsync_wait_update_a(struct tst_fzsync_pair *pair)
 	static int loop_index;
 
 	tst_fzsync_pair_wait(pair, &pair->a_cntr, &pair->b_cntr);
-	loop_index++;
-	tst_fzsync_pair_update(loop_index, pair);
+
+	if (tst_timer_expired_st(&pair->timer)) {
+		tst_res(TINFO,
+			"Exceeded fuzzy sync time limit, requesting exit");
+		tst_fzsync_pair_exit(pair);
+	} else {
+		loop_index++;
+		tst_fzsync_pair_update(loop_index, pair);
+	}
+
 	return tst_fzsync_pair_wait(pair, &pair->a_cntr, &pair->b_cntr);
 }

@@ -313,14 +342,3 @@ static inline int tst_fzsync_wait_update_b(struct tst_fzsync_pair *pair)
 	tst_fzsync_pair_wait(pair, &pair->b_cntr, &pair->a_cntr);
 	return tst_fzsync_pair_wait(pair, &pair->b_cntr, &pair->a_cntr);
 }
-
-/**
- * tst_fzsync_pair_exit - Signal that the other thread should exit
- *
- * Causes tst_fzsync_pair_wait() and tst_fzsync_pair_wait_update() to return
- * 0.
- */
-static inline void tst_fzsync_pair_exit(struct tst_fzsync_pair *pair)
-{
-	tst_atomic_store(1, &pair->exit);
-}

lib/newlib_tests/test16.c +3 -1

index d80bd5369..3447dbc5a 100644
@@ -64,8 +64,10 @@ static void run(void)
 {
 	unsigned int i, j, fail = 0;
 
+	tst_fzsync_pair_reset(&pair);
 	for (i = 0; i < LOOPS; i++) {
-		tst_fzsync_wait_update_a(&pair);
+		if (!tst_fzsync_wait_update_a(&pair))
+			break;
 		tst_fzsync_delay_a(&pair);
 		seq[seq_n] = 'A';
 		seq_n = i * 2 + 1;

testcases/cve/cve-2014-0196.c +3 -1

index d18108897..4a3f24026 100644
@@ -102,6 +102,7 @@ static void run(void)
 
 	tst_res(TINFO, "Attempting to overflow into a tty_struct...");
 
+	tst_fzsync_pair_reset(&fzsync_pair);
 	for (i = 0; i < ATTEMPTS; i++) {
 		create_pty((int *)&master_fd, (int *)&slave_fd);

@@ -118,7 +119,8 @@ static void run(void)
 		t.c_lflag |= ECHO;
 		tcsetattr(master_fd, TCSANOW, &t);
 
-		tst_fzsync_wait_update_a(&fzsync_pair);
+		if (!tst_fzsync_wait_update_a(&fzsync_pair))
+			break;
 
 		tst_fzsync_delay_a(&fzsync_pair);
 		tst_fzsync_time_a(&fzsync_pair);

testcases/cve/cve-2016-7117.c +4 -2

index 3cb7efcdf..fecc5889f 100644
@@ -136,11 +136,13 @@ static void run(void)
 
 	msghdrs[0].msg_hdr.msg_iov->iov_base = (void *)&rbuf;
 
+	tst_fzsync_pair_reset(&fzsync_pair);
 	for (i = 1; i < ATTEMPTS; i++) {
 		if (socketpair(AF_LOCAL, SOCK_DGRAM, 0, (int *)socket_fds))
 			tst_brk(TBROK | TERRNO, "Socket creation failed");
 
-		tst_fzsync_wait_update_a(&fzsync_pair);
+		if (!tst_fzsync_wait_update_a(&fzsync_pair))
+			break;
 		tst_fzsync_delay_a(&fzsync_pair);
 
 		stat = tst_syscall(__NR_recvmmsg,

@@ -156,7 +158,7 @@ static void run(void)
 		tst_fzsync_wait_a(&fzsync_pair);
 	}
 
-	tst_res(TPASS, "Nothing happened after %d attempts", ATTEMPTS);
+	tst_res(TPASS, "Nothing happened after %d attempts", i);
 }
 
 static struct tst_test test = {

testcases/cve/cve-2017-2671.c +3 -1

index b0471bfff..ce3022ce2 100644
@@ -111,11 +111,13 @@ static void run(void)
 {
 	int i;
 
+	tst_fzsync_pair_reset(&fzsync_pair);
 	for (i = 0; i < ATTEMPTS; i++) {
 		SAFE_CONNECT(sockfd,
 			     (struct sockaddr *)&iaddr, sizeof(iaddr));
 
-		tst_fzsync_wait_update_a(&fzsync_pair);
+		if (!tst_fzsync_wait_update_a(&fzsync_pair))
+			break;
 		tst_fzsync_delay_a(&fzsync_pair);
 		connect(sockfd, (struct sockaddr *)&uaddr, sizeof(uaddr));
 		tst_fzsync_time_a(&fzsync_pair);

testcases/kernel/syscalls/inotify/inotify09.c +4 -1

index 475411311..701ab1487 100644
@@ -97,12 +97,15 @@ static void verify_inotify(void)
 	inotify_fd = myinotify_init1(0);
 	if (inotify_fd < 0)
 		tst_brk(TBROK | TERRNO, "inotify_init failed");
+
+	tst_fzsync_pair_reset(&fzsync_pair);
 	for (tests = 0; tests < TEARDOWNS; tests++) {
 		wd = myinotify_add_watch(inotify_fd, FNAME, IN_MODIFY);
 		if (wd < 0)
 			tst_brk(TBROK | TERRNO, "inotify_add_watch() failed.");
 
-		tst_fzsync_wait_update_a(&fzsync_pair);
+		if (!tst_fzsync_wait_update_a(&fzsync_pair))
+			break;
 		tst_fzsync_delay_a(&fzsync_pair);
 
 		wd = myinotify_rm_watch(inotify_fd, wd);

Signature · 1 line

2.18.0

Li Wang

2018-08-17 07:43 UTC #10 lore raw

PATCH v3 4/4 fzsync: Limit execution time to prevent test timeouts

Quoted 8 lines · Richard Palethorpe

...

include/tst_fuzzy_sync.h +1 -0

@@ -99,6 +102,15 @@ struct tst_fzsync_pair {
        .info_gap = 0x7FFFF     \
 }
 
+

include/tst_fuzzy_sync.h struct tst_fzsync_pair { · line 115

Quoted 10 lines

include/tst_fuzzy_sync.h +8 -0

 static void tst_fzsync_pair_reset(struct tst_fzsync_pair *pair)
+{
+       pair->timer.clock_id = CLOCK_MONOTONIC_RAW;
+       pair->timer.limit.tv_sec = 60 * tst_timeout_mul();
+       pair->timer.limit.tv_nsec = 0;
+
+       tst_timer_start_st(&pair->timer);
+}
+

There is a loop defect in this method as I commented in patch V2.

If we don't reset the pair->exit to 0 after one loop, it will be never run into the second
fzsync function because the pair->exit has been set to 1 at the first expired time.

something result like:
----------------------------

# ./cve-2016-7117 -i 3
tst_test.c:1022: INFO: Timeout per run is 0h 05m 00s
../../include/tst_fuzzy_sync.h:121: INFO: avg_diff = -216ns, avg_dev =
565ns, delay = 02474 loops
../../include/tst_fuzzy_sync.h:121: INFO: avg_diff = 12ns, avg_dev = 430ns,
delay = 02604 loops
../../include/tst_fuzzy_sync.h:330: INFO: Exceeded fuzzy sync time limit,
requesting exit
cve-2016-7117.c:161: PASS: Nothing happened after 1564741 attempts
cve-2016-7117.c:161: PASS: Nothing happened after 1 attempts
cve-2016-7117.c:161: PASS: Nothing happened after 1 attempts

Summary:

passed   3
failed   0
skipped  0

warnings 0

But, if we just reset the pair->exit to 0 in the new function tst_fzsync_pair_reset(),
there still NOT fix the problem totally, because in the last test expired time, all threads
created by setup() function have exited, and here we'll only loop in tst_fzsync_wait_a()
and wait there forever. :(

Signature · 5 lines

Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20180817/df3e421d/attachment.html>

Li Wang

2018-08-17 09:04 UTC #11 lore raw

PATCH v3 4/4 fzsync: Limit execution time to prevent test timeouts

Quoted 48 lines · Li Wang

Quoted 8 lines · Richard Palethorpe
...
(quoted diff) +1 -0
@@ -99,6 +102,15 @@ struct tst_fzsync_pair {
        .info_gap = 0x7FFFF     \
 }
 
+
Review struct tst_fzsync_pair { · line 115
Quoted 10 lines
(quoted diff) +8 -0
 static void tst_fzsync_pair_reset(struct tst_fzsync_pair *pair)
+{
+       pair->timer.clock_id = CLOCK_MONOTONIC_RAW;
+       pair->timer.limit.tv_sec = 60 * tst_timeout_mul();
+       pair->timer.limit.tv_nsec = 0;
+
+       tst_timer_start_st(&pair->timer);
+}
+
 
There is a loop defect in this method as I commented in patch V2.
If we don't reset the pair->exit to 0 after one loop, it will be never run into the second
fzsync function because the pair->exit has been set to 1 at the first expired time.
something result like:
----------------------------
# ./cve-2016-7117 -i 3
tst_test.c:1022: INFO: Timeout per run is 0h 05m 00s
../../include/tst_fuzzy_sync.h:121: INFO: avg_diff = -216ns, avg_dev =
565ns, delay = 02474 loops
../../include/tst_fuzzy_sync.h:121: INFO: avg_diff = 12ns, avg_dev =
430ns, delay = 02604 loops
../../include/tst_fuzzy_sync.h:330: INFO: Exceeded fuzzy sync time limit,
requesting exit
cve-2016-7117.c:161: PASS: Nothing happened after 1564741 attempts
cve-2016-7117.c:161: PASS: Nothing happened after 1 attempts
cve-2016-7117.c:161: PASS: Nothing happened after 1 attempts
Summary:
passed   3
failed   0
skipped  0
warnings 0

Quoted 8 lines

But, if we just reset the pair->exit to 0 in the new function tst_fzsync_pair_reset(),
there still NOT fix the problem totally, because in the last test expired time, all threads
created by setup() function have exited, and here we'll only loop in tst_fzsync_wait_a()
and wait there forever. :(

I just come up with a stupid patch to fix that, but personally I insist believe
that maybe we should not leave this kind of works to LTP user, we'd better encapsulate that all in fuzzy_sync library.

Just FYI:

include/tst_fuzzy_sync.h +7 -1

index 5e0ff36..862ab7e 100644
@@ -102,8 +102,14 @@ struct tst_fzsync_pair {
        .info_gap = 0x7FFFF     \
 }
 
-static void tst_fzsync_pair_reset(struct tst_fzsync_pair *pair)
+static void tst_fzsync_pair_init(struct tst_fzsync_pair *pair)
 {
+       pair->exit = 0;
+       pair->delay = 0;
+       pair->a_cntr = pair->b_cntr = 0;
+       pair->avg_dev = pair->avg_diff = 0;
+       pair->a.tv_sec = pair->a.tv_nsec = 0;
+       pair->b.tv_sec = pair->b.tv_nsec = 0;
        pair->timer.clock_id = CLOCK_MONOTONIC_RAW;
        pair->timer.limit.tv_sec = 60 * tst_timeout_mul();
        pair->timer.limit.tv_nsec = 0;

testcases/cve/cve-2016-7117.c +4 -1

index fecc588..f8993c7 100644
@@ -136,7 +136,10 @@ static void run(void)
 
        msghdrs[0].msg_hdr.msg_iov->iov_base = (void *)&rbuf;
 
-       tst_fzsync_pair_reset(&fzsync_pair);
+       if (fzsync_pair.exit == 1)
+               setup();
+
+       tst_fzsync_pair_init(&fzsync_pair);
        for (i = 1; i < ATTEMPTS; i++) {
                if (socketpair(AF_LOCAL, SOCK_DGRAM, 0, (int *)socket_fds))
                        tst_brk(TBROK | TERRNO, "Socket creation failed");

Signature · 5 lines

Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20180817/fb6ddbf3/attachment.html>