Re: [PATCH] i386/xen: consistent locking around Xen singleshot timers

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 1916 bytes --]

On Fri, 2023-06-02 at 17:58 +0100, Peter Maydell wrote:
> On Mon, 22 May 2023 at 19:52, David Woodhouse <dwmw2@infradead.org> wrote:
> > 
> > From: David Woodhouse <dwmw@amazon.co.uk>
> > 
> > Coverity points out (CID 1507534) that we sometimes access
> > env->xen_singleshot_timer_ns under the protection of
> > env->xen_timers_lock (eg in xen_vcpu_singleshot_timer_event()) and
> > sometimes not (the specific case Coverity complains about is in
> > do_vcpu_soft_reset()).
> > 
> > This isn't strictly an issue. There are two modes for the timers; if
> > the kernel supports the EVTCHN_SEND capability then it handles all the
> > timer hypercalls and delivery internally, and all we need to do is an
> > ioctl to get/set the next timer as part of the vCPU state. If the
> > kernel doesn't have that support, then we do all the emulation within
> > qemu, and *those* are the code paths where we actually care about the
> > locking.
> > 
> > But it doesn't hurt to be a little bit more consistent and avoid having
> > to explain *why* it's OK.
> > 
> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> 
> Looking a bit more closely at the Coverity lists, there's also
> CID 1507968 which is for the access to env->xen_singleshot_timer_ns
> in kvm_get_xen_state(), which this patch doesn't touch, I think ?

That one is basically a false positive since it's for the case where
the kernel is handling the timers and all we do is an ioctl to read the
state. Which is *inherently* racy if any other vCPU can be running and
modifying them at the time. And doesn't have anything to race *with* if
not :)

On the other hand, the one in kvm_put_xen_state() which calls
do_set_singleshot_timer() is for the userspace case, and *that* one
probably wants locking. Again in practice it's probably never going to
have anything to race with, but that's a poor excuse. New patch
follows.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]