| Message ID | 20231103-bb-timetravel-patches-v1-8-e2c68efcf664@uni-rostock.de |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Several Time Travel Mode Enhancements | expand |
On Fri, 2023-11-03 at 16:41 +0000, Benjamin Beichler wrote: > When the timetravel ext modus is used, accessing the timetravel event > list can be interrupted by a message on the timetravel socket in the > SIGIO signal handler. This interruption can potentially modify the event > list, leading to race conditions that cause deadlocks in the timetravel > protocol or disrupt the ordered nature of the list, triggering bugs. > > Previously, the normal irq-save function did not intentionally block the > timetravel handlers. However, the additional (un)block_signals_hard > functions do block them. Therefore, these functions have been added at > the appropriate places to address the issue. > > It's worth noting that although the functions are named as blocking, > they primarily defer the actual execution of the SIGIO handlers until > the unblock call. If no signal was issued, this mainly results in a > variable assignment and a memory barrier. Ahh ... _now_ I'm starting to understand what you meant earlier wrt. signals and blocking and it not doing anything ... > local_irq_save(flags); > + block_signals_hard(); > list_for_each_entry(tmp, &time_travel_events, list) { Yeah, indeed, signals are interrupts and we process them at a lower level, so here our abstraction of disabling interrupts is leaky because the signal can still happen to modify the list due to the time_travel_add_irq_event() ... Note I also have a race fix for signals_blocked_pending somewhere... Need to find time to flush out more of our patches. But yeah, I guess this makes sense. johannes
diff --git a/arch/um/kernel/time.c b/arch/um/kernel/time.c index e513256eadfe..f1b2ca45994d 100644 --- a/arch/um/kernel/time.c +++ b/arch/um/kernel/time.c @@ -288,6 +288,7 @@ static void __time_travel_add_event(struct time_travel_event *e, e->time = time; local_irq_save(flags); + block_signals_hard(); list_for_each_entry(tmp, &time_travel_events, list) { /* * Add the new entry before one with higher time, @@ -310,6 +311,7 @@ static void __time_travel_add_event(struct time_travel_event *e, tmp = time_travel_first_event(); time_travel_ext_update_request(tmp->time); time_travel_next_event = tmp->time; + unblock_signals_hard(); local_irq_restore(flags); } @@ -349,6 +351,7 @@ void deliver_time_travel_irqs(void) return; local_irq_save(flags); + block_signals_hard(); irq_enter(); while ((e = list_first_entry_or_null(&time_travel_irqs, struct time_travel_event, @@ -358,6 +361,7 @@ void deliver_time_travel_irqs(void) e->fn(e); } irq_exit(); + unblock_signals_hard(); local_irq_restore(flags); } @@ -370,7 +374,9 @@ static void time_travel_deliver_event(struct time_travel_event *e) */ e->fn(e); } else if (irqs_disabled()) { + block_signals_hard(); list_add_tail(&e->list, &time_travel_irqs); + unblock_signals_hard(); /* * set pending again, it was set to false when the * event was deleted from the original list, but @@ -395,8 +401,10 @@ bool time_travel_del_event(struct time_travel_event *e) if (!e->pending) return false; local_irq_save(flags); + block_signals_hard(); list_del(&e->list); e->pending = false; + unblock_signals_hard(); local_irq_restore(flags); return true; }
When the timetravel ext modus is used, accessing the timetravel event list can be interrupted by a message on the timetravel socket in the SIGIO signal handler. This interruption can potentially modify the event list, leading to race conditions that cause deadlocks in the timetravel protocol or disrupt the ordered nature of the list, triggering bugs. Previously, the normal irq-save function did not intentionally block the timetravel handlers. However, the additional (un)block_signals_hard functions do block them. Therefore, these functions have been added at the appropriate places to address the issue. It's worth noting that although the functions are named as blocking, they primarily defer the actual execution of the SIGIO handlers until the unblock call. If no signal was issued, this mainly results in a variable assignment and a memory barrier. Signed-off-by: Benjamin Beichler <benjamin.beichler@uni-rostock.de> --- arch/um/kernel/time.c | 8 ++++++++ 1 file changed, 8 insertions(+)