[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()
On 17/08/2020 18:33, Roger Pau Monné wrote:
On Mon, Aug 17, 2020 at 04:53:51PM +0100, Julien Grall wrote:
On 17/08/2020 16:03, Roger Pau Monné wrote:
On Mon, Aug 17, 2020 at 03:39:52PM +0100, Julien Grall wrote:
On 17/08/2020 15:01, Roger Pau Monné wrote:
On Mon, Aug 17, 2020 at 02:14:01PM +0100, Julien Grall wrote:
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.
Well, the access should be done using a single instruction, which is
what we care about when using this helpers.
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.
Right, but this is not guaranteed by the current implementation of
ACCESS_ONCE AFAICT, as the compiler *might* split the access into two
(or more) instructions, and hence won't be an atomic access anymore?
From my understanding, at least on GCC/Clang, ACCESS_ONCE() should be atomic
if you are using aligned address and the size smaller than a register size.
Yes, any sane compiler shouldn't split such access, but this is not
guaranteed by the current code in ACCESS_ONCE.
To be sure, your concern here is not about GCC/Clang but other compilers. Am
I correct?
Or about the existing ones switching behavior, which is again quite
unlikely I would like to assume.
The main goal of the macro is to mark place which require the variable
to be accessed once. So, in the unlikely event this may happen, it would
be easy to modify the implementation.
We already have a collection of compiler specific macros in compiler.h. So
how about we classify this macro as a compiler specific one? (See more
below).
May I ask why we would want to expose the difference to the user?
I'm not saying we should, but naming them using the _ONCE suffix seems
misleading IMO, as they have different guarantees than what
ACCESS_ONCE currently provides.
Lets leave aside how ACCESS_ONCE() is implemented for a moment.
If ACCESS_ONCE() doesn't guarantee atomicy, then it means you may read a mix
of the old and new value. This would most likely break quite a few of the
users because the result wouldn't be coherent.
Do you have place in mind where the non-atomicity would be useful?
Not that I'm aware, I think they could all be safely switched to use
the atomic variants
There is concern that read_atomic(), write_atomic() prevent the compiler to
do certain optimization. Andrew gave the example of:
ACCESS_ONCE(...) |= ...
I'm not sure how will that behave when used with a compile known
value that's smaller than the size of the destination. Could the
compiler optimize this as a partial read/write if only the lower byte
is modified for example?
Here what Andrew wrote in a previous answer:
"Which a sufficiently clever compiler could convert to a single `or
$val, ptr` instruction on x86, while read_atomic()/write_atomic() would
force it to be `mov ptr, %reg; or $val, %reg; mov %reg, ptr`."
On Arm, a RwM operation will still not be atomic as it would require 3
instructions.
In fact I wouldn't be surprised if users of ACCESS_ONCE break if the
access was split into multiple instructions.
My comment was to notice that just renaming the atomic read/write
helpers to use the _ONCE prefix is IMO weird as they offer different
properties than ACCESS_ONCE, and hence might confuse users.Just
looking at READ_ONCE users could assume all _ONCE helpers would
guarantee atomicity, which is not the case.
Our implementation of ACCESS_ONCE() is very similar to what Linux used to
have. There READ_ONCE()/WRITE_ONCE() are also using the same principles.
From my understanding, you can safely assume the access will be atomic if
the following conditions are met:
- The address is correctly size
- The size is smaller than the word machine size
I guess we could go that route, and properly document what each helper
is supposed to do, and that {READ/WRITE}_ONCE guarantee atomicity
while ACCESS_ONCE requires special condition for us to guarantee the
operation will be atomic.
I would agree this may not be correct on all the existing compilers. But
this macro could easily be re-implemented if we add support for a compiler
with different guarantee.
Therefore, I fail to see why we can't use the same guarantee in Xen.
I'm fine if what's expected from each helper is documented, it just
seems IMO more confusing that using more differentiated naming for the
helpers, because the fact that ACCESS_ONCE is atomic is a compiler
defined behavior, but not something that could be guaranteed from the
code itself.
I am happy to try to document the behavior of each helpers. Are you
happy if I attempt to do the renaming in a follow-up patch?
Cheers,
--
Julien Grall
|
