[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] xen/x86: irq: Avoid a TOCTOU race in pirq_spin_lock_irq_desc()



Hi,

On 17/08/2020 13:46, Roger Pau Monné wrote:
On Fri, Aug 14, 2020 at 08:25:28PM +0100, Julien Grall wrote:
Hi Andrew,

Sorry for the late answer.

On 23/07/2020 14:59, Andrew Cooper wrote:
On 23/07/2020 14:22, Julien Grall wrote:
Hi Jan,

On 23/07/2020 12:23, Jan Beulich wrote:
On 22.07.2020 18:53, Julien Grall wrote:
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -1187,7 +1187,7 @@ struct irq_desc *pirq_spin_lock_irq_desc(
          for ( ; ; )
        {
-        int irq = pirq->arch.irq;
+        int irq = read_atomic(&pirq->arch.irq);

There we go - I'd be fine this way, but I'm pretty sure Andrew
would want this to be ACCESS_ONCE(). So I guess now is the time
to settle which one to prefer in new code (or which criteria
there are to prefer one over the other).

I would prefer if we have a single way to force the compiler to do a
single access (read/write).

Unlikely to happen, I'd expect.

But I would really like to get rid of (or at least rename)
read_atomic()/write_atomic() specifically because they've got nothing to
do with atomic_t's and the set of functionality who's namespace they share.

Would you be happy if I rename both to READ_ONCE() and WRITE_ONCE()? I would
also suggest to move them implementation in a new header asm/lib.h.

Maybe {READ/WRITE}_SINGLE (to note those should be implemented using a
single instruction)?

The asm volatile statement contains only one instruction, but this doesn't mean the helper will generate a single instruction.

You may have other instructions to get the registers ready for the access.


ACCESS_ONCE (which also has the _ONCE suffix) IIRC could be
implemented using several instructions, and hence doesn't seem right
that they all have the _ONCE suffix.

The goal here is the same, we want to access the variable *only* once.

May I ask why we would want to expose the difference to the user?

Cheers,

--
Julien Grall



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.