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

[Johannes Berg]

On Fri, 2023-10-27 at 16:05 +0200, Benjamin Beichler wrote:
> 
> As I said, I'm also preparing some patches, therefore my few bits about 
> those problems:
> 
> - we noticed, that those backward running time is mostly created by the 
> "unprotected" access to the variables of the time travel event list. 
>  From the stack trace you see the problem even do not appear directly at 
> timer_read but later. I think mostly this means the event list head was 
> moved and then the interrupt happens and the pointer (or stored time) 
> become outdated and the function (here the alarm_handler) continues with 
> them.

That's not entirely implausible.

> - besides this, when you look into local_irq_save for um, this does not 
> really deactivate the interrupts, but delay the processing and it adds a 
> memory barrier. I think that could be one of the important consequences 
> as changes to the event list are forced to be populated. But this is 
> only a guess :-D

No, it *does* disable interrupts from Linux's POV. The memory barrier is
just there to make _that_ actually visible "immediately".

Yes it doesn't actually disable the *signal* underneath, but that
doesn't really mean anything? Once you get the signal, nothing happens
if it's disabled.

In UML, the kernel binary is kind of both hypervisor and guest kernel,
so you can think of the low-level code here as being part of the
hypervisor, so that blocking the signal from actually calling out into
the guest is like blocking the PIC from sending to the CPU. Obviously
the device(s) still send(s) the interrupt(s), but they don't go into the
"guest".

> - I'm also not really convinced, that all accesses to the current time 
> should be delayed. I made a patch with a heuristic, that only delays the 
> time read, if it is read from userspace multiple times in a row. 

How would you even detect that though?

And on the other hand - why not? There's always a cost to things in a
real system, it's not free to access the current time? Maybe it's faster
in a real system with VDSO though.

> Since 
> there are no "busy" loops in the kernel itself and only in a few (bad 
> behaving) userspace programs, I think that helps to reduce the 
> inaccuracy in simulation from well behaving userspace programs only 
> reading the time once.

So I'm not sure I'd call it an inaccuracy? It's ... perfectly accurate
in the model that we're implementing? Maybe you don't like the model ;-)

> In this case, irqs_disabled more or less only tend to indicate, that the 
> event list could be manipulated, and therefore the update_time call is a 
> bad idea.

Which is kind of what I was thinking about earlier, but Vincent says
it's not _just_ for that.

We probably should just add a spinlock for this though - right now none
of this is good for eventual SMP support, and it's much easier to reason
about when we have a lock?

johannes