All of lore.kernel.org
 help / color / mirror / Atom feed
From: Vincent Whitchurch <vincent.whitchurch@axis.com>
To: Richard Weinberger <richard@nod.at>,
	Anton Ivanov <anton.ivanov@cambridgegreys.com>,
	Johannes Berg <johannes@sipsolutions.net>
Cc: <linux-um@lists.infradead.org>, <linux-kernel@vger.kernel.org>,
	<kernel@axis.com>,
	Vincent Whitchurch <vincent.whitchurch@axis.com>
Subject: [PATCH] um: time-travel: fix time going backwards
Date: Fri, 20 Oct 2023 16:47:39 +0200	[thread overview]
Message-ID: <20231020-uml-time-backwards-v1-1-90b776fc6dfd@axis.com> (raw)

In basic time travel mode, I sometimes see "time goes backwards" panics
like the one below:

 Kernel panic: time-travel: time goes backwards 161689340000492 -> 161689339869814
 Call Trace:
  panic+0x1a1/0x3d7
  time_travel_update_time.cold+0xe9/0x133
  timer_read+0xc1/0x100
  ktime_get+0x10c/0x200
  copy_process+0x1899/0x2230
  kernel_clone+0x57/0x7a0
  kernel_thread+0x4a/0x50
  kthreadd+0x116/0x190

The problem is a race between time_travel_handle_real_alarm() and
timer_read().  time_travel_handle_real_alarm() changes the time after
time_read() reads the current time but before time_travel_update_time()
has had a chance to add the end event.

Fix this by doing the time read and event add atomically with respect to
time_travel_handle_real_alarm().

Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---
 arch/um/kernel/time.c | 48 ++++++++++++++++++++++++++++++++++--------------
 1 file changed, 34 insertions(+), 14 deletions(-)

diff --git a/arch/um/kernel/time.c b/arch/um/kernel/time.c
index fddd1dec27e6..38a94dd41b8f 100644
--- a/arch/um/kernel/time.c
+++ b/arch/um/kernel/time.c
@@ -392,17 +392,11 @@ bool time_travel_del_event(struct time_travel_event *e)
 	return true;
 }
 
-static void time_travel_update_time(unsigned long long next, bool idle)
+static void time_travel_update_to_event(struct time_travel_event *ne, bool idle)
 {
-	struct time_travel_event ne = {
-		.onstack = true,
-	};
 	struct time_travel_event *e;
 	bool finished = idle;
 
-	/* add it without a handler - we deal with that specifically below */
-	__time_travel_add_event(&ne, next);
-
 	do {
 		e = time_travel_first_event();
 
@@ -414,7 +408,7 @@ static void time_travel_update_time(unsigned long long next, bool idle)
 			BUG_ON(!time_travel_del_event(e));
 			BUG_ON(time_travel_time != e->time);
 
-			if (e == &ne) {
+			if (e == ne) {
 				finished = true;
 			} else {
 				if (e->onstack)
@@ -427,14 +421,38 @@ static void time_travel_update_time(unsigned long long next, bool idle)
 		e = time_travel_first_event();
 		if (e)
 			time_travel_ext_update_request(e->time);
-	} while (ne.pending && !finished);
+	} while (ne->pending && !finished);
+
+	time_travel_del_event(ne);
+}
 
-	time_travel_del_event(&ne);
+static void time_travel_update_time(unsigned long long next, bool idle)
+{
+	struct time_travel_event ne = {
+		.onstack = true,
+	};
+
+	__time_travel_add_event(&ne, next);
+	time_travel_update_to_event(&ne, idle);
+}
+
+static void time_travel_forward_time(unsigned long nsec, bool idle)
+{
+	struct time_travel_event ne = {
+		.onstack = true,
+	};
+	unsigned long flags;
+
+	local_irq_save(flags);
+	__time_travel_add_event(&ne, time_travel_time + nsec);
+	local_irq_restore(flags);
+
+	time_travel_update_to_event(&ne, idle);
 }
 
 void time_travel_ndelay(unsigned long nsec)
 {
-	time_travel_update_time(time_travel_time + nsec, false);
+	time_travel_forward_time(nsec, false);
 }
 EXPORT_SYMBOL(time_travel_ndelay);
 
@@ -572,6 +590,10 @@ static inline void time_travel_update_time(unsigned long long ns, bool retearly)
 {
 }
 
+static void time_travel_forward_time(unsigned long nsec, bool idle)
+{
+}
+
 static inline void time_travel_handle_real_alarm(void)
 {
 }
@@ -720,9 +742,7 @@ static u64 timer_read(struct clocksource *cs)
 		 */
 		if (!irqs_disabled() && !in_interrupt() && !in_softirq() &&
 		    !time_travel_ext_waiting)
-			time_travel_update_time(time_travel_time +
-						TIMER_MULTIPLIER,
-						false);
+			time_travel_forward_time(TIMER_MULTIPLIER, false);
 		return time_travel_time / TIMER_MULTIPLIER;
 	}
 

---
base-commit: 58720809f52779dc0f08e53e54b014209d13eebb
change-id: 20231020-uml-time-backwards-552c81aedd23

Best regards,
-- 
Vincent Whitchurch <vincent.whitchurch@axis.com>


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

WARNING: multiple messages have this Message-ID (diff)
From: Vincent Whitchurch <vincent.whitchurch@axis.com>
To: Richard Weinberger <richard@nod.at>,
	Anton Ivanov <anton.ivanov@cambridgegreys.com>,
	Johannes Berg <johannes@sipsolutions.net>
Cc: <linux-um@lists.infradead.org>, <linux-kernel@vger.kernel.org>,
	<kernel@axis.com>,
	Vincent Whitchurch <vincent.whitchurch@axis.com>
Subject: [PATCH] um: time-travel: fix time going backwards
Date: Fri, 20 Oct 2023 16:47:39 +0200	[thread overview]
Message-ID: <20231020-uml-time-backwards-v1-1-90b776fc6dfd@axis.com> (raw)

In basic time travel mode, I sometimes see "time goes backwards" panics
like the one below:

 Kernel panic: time-travel: time goes backwards 161689340000492 -> 161689339869814
 Call Trace:
  panic+0x1a1/0x3d7
  time_travel_update_time.cold+0xe9/0x133
  timer_read+0xc1/0x100
  ktime_get+0x10c/0x200
  copy_process+0x1899/0x2230
  kernel_clone+0x57/0x7a0
  kernel_thread+0x4a/0x50
  kthreadd+0x116/0x190

The problem is a race between time_travel_handle_real_alarm() and
timer_read().  time_travel_handle_real_alarm() changes the time after
time_read() reads the current time but before time_travel_update_time()
has had a chance to add the end event.

Fix this by doing the time read and event add atomically with respect to
time_travel_handle_real_alarm().

Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---
 arch/um/kernel/time.c | 48 ++++++++++++++++++++++++++++++++++--------------
 1 file changed, 34 insertions(+), 14 deletions(-)

diff --git a/arch/um/kernel/time.c b/arch/um/kernel/time.c
index fddd1dec27e6..38a94dd41b8f 100644
--- a/arch/um/kernel/time.c
+++ b/arch/um/kernel/time.c
@@ -392,17 +392,11 @@ bool time_travel_del_event(struct time_travel_event *e)
 	return true;
 }
 
-static void time_travel_update_time(unsigned long long next, bool idle)
+static void time_travel_update_to_event(struct time_travel_event *ne, bool idle)
 {
-	struct time_travel_event ne = {
-		.onstack = true,
-	};
 	struct time_travel_event *e;
 	bool finished = idle;
 
-	/* add it without a handler - we deal with that specifically below */
-	__time_travel_add_event(&ne, next);
-
 	do {
 		e = time_travel_first_event();
 
@@ -414,7 +408,7 @@ static void time_travel_update_time(unsigned long long next, bool idle)
 			BUG_ON(!time_travel_del_event(e));
 			BUG_ON(time_travel_time != e->time);
 
-			if (e == &ne) {
+			if (e == ne) {
 				finished = true;
 			} else {
 				if (e->onstack)
@@ -427,14 +421,38 @@ static void time_travel_update_time(unsigned long long next, bool idle)
 		e = time_travel_first_event();
 		if (e)
 			time_travel_ext_update_request(e->time);
-	} while (ne.pending && !finished);
+	} while (ne->pending && !finished);
+
+	time_travel_del_event(ne);
+}
 
-	time_travel_del_event(&ne);
+static void time_travel_update_time(unsigned long long next, bool idle)
+{
+	struct time_travel_event ne = {
+		.onstack = true,
+	};
+
+	__time_travel_add_event(&ne, next);
+	time_travel_update_to_event(&ne, idle);
+}
+
+static void time_travel_forward_time(unsigned long nsec, bool idle)
+{
+	struct time_travel_event ne = {
+		.onstack = true,
+	};
+	unsigned long flags;
+
+	local_irq_save(flags);
+	__time_travel_add_event(&ne, time_travel_time + nsec);
+	local_irq_restore(flags);
+
+	time_travel_update_to_event(&ne, idle);
 }
 
 void time_travel_ndelay(unsigned long nsec)
 {
-	time_travel_update_time(time_travel_time + nsec, false);
+	time_travel_forward_time(nsec, false);
 }
 EXPORT_SYMBOL(time_travel_ndelay);
 
@@ -572,6 +590,10 @@ static inline void time_travel_update_time(unsigned long long ns, bool retearly)
 {
 }
 
+static void time_travel_forward_time(unsigned long nsec, bool idle)
+{
+}
+
 static inline void time_travel_handle_real_alarm(void)
 {
 }
@@ -720,9 +742,7 @@ static u64 timer_read(struct clocksource *cs)
 		 */
 		if (!irqs_disabled() && !in_interrupt() && !in_softirq() &&
 		    !time_travel_ext_waiting)
-			time_travel_update_time(time_travel_time +
-						TIMER_MULTIPLIER,
-						false);
+			time_travel_forward_time(TIMER_MULTIPLIER, false);
 		return time_travel_time / TIMER_MULTIPLIER;
 	}
 

---
base-commit: 58720809f52779dc0f08e53e54b014209d13eebb
change-id: 20231020-uml-time-backwards-552c81aedd23

Best regards,
-- 
Vincent Whitchurch <vincent.whitchurch@axis.com>


             reply	other threads:[~2023-10-20 14:47 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-10-20 14:47 Vincent Whitchurch [this message]
2023-10-20 14:47 ` [PATCH] um: time-travel: fix time going backwards Vincent Whitchurch
2023-10-23  7:08 ` Vincent Whitchurch
2023-10-23  7:08   ` Vincent Whitchurch
2023-10-23  7:33   ` Johannes Berg
2023-10-23  7:33     ` Johannes Berg
2023-10-25 11:55     ` Vincent Whitchurch
2023-10-25 11:55       ` Vincent Whitchurch
2023-10-25 19:51       ` Johannes Berg
2023-10-25 19:51         ` Johannes Berg
2023-10-25 20:02         ` Johannes Berg
2023-10-25 20:02           ` Johannes Berg
2023-10-25 20:41           ` Johannes Berg
2023-10-25 20:41             ` Johannes Berg

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20231020-uml-time-backwards-v1-1-90b776fc6dfd@axis.com \
    --to=vincent.whitchurch@axis.com \
    --cc=anton.ivanov@cambridgegreys.com \
    --cc=johannes@sipsolutions.net \
    --cc=kernel@axis.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-um@lists.infradead.org \
    --cc=richard@nod.at \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.