[PATCH v2] um: time-travel: fix time corruption

[Johannes Berg]

On Thu, 2023-10-26 at 07:23 +0000, Vincent Whitchurch wrote:
> > @@ -839,9 +863,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_update_time_rel(TIMER_MULTIPLIER);
> >  		return time_travel_time / TIMER_MULTIPLIER;
> >  	}
> 
> The reason I hesitated with putting the whole of
> time_travel_update_time() under local_irq_save() in my attempt was
> because I didn't quite understand the reason for the !irqs_disabled()
> condition here and the comment just above it about recursion and things
> getting messed up.  If it's OK to disable interrupts as this patch does,
> is the !irqs_disabled() condition valid?

Hmm. I was going to say that's different, because it wants to only
prevent us from doing this while we're *already* in IRQ context, and the
bug you found is calling timer_read() not in IRQ context, but getting an
event queued by the signal.

But ... now that I think about it, I have a feeling that this was a
workaround for the exact same problem, and I just didn't understand it
at the time? I mean, recursing into our own processing is now impossible
here after this patch - either we're running normally, or the interrupt
cannot hit timer_read() in the middle, same as it cannot hit
time_travel_handle_real_alarm() in the middle now.

Removing that still seems to work with your test, but it's also not a
good test for this, since there are no devices etc. that could have
interrupts, not sure how to test it right now?

Maybe I'll add a comment there saying this might no longer be needed?

johannes