[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()


  • To: Julien Grall <julien@xxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Thu, 23 Jul 2020 14:59:57 +0100
  • Authentication-results: esa3.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none
  • Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx, Julien Grall <jgrall@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Delivery-date: Thu, 23 Jul 2020 14:00:04 +0000
  • Ironport-sdr: +QVIcrGkzDbY1sC1JXGCJhALYgzzGqzN1o4PQCMi4Sf16hIF/SFhWylvazzkbu5Yt0pGABQiua acRDUA/6J2viG8z1rTsmk4aQ0I4EWAYja/7p9s8UKTMg4/OSZW32lg688RDD/3MCJw2iQJpnbJ D6+wnRqNU20jCo5Le9/piLYvZ7A+D7JIkouwmCalDGqwf7BUthM4i2DXAlDgWGBeiDs/5DMP4P rnw2OGcJwGV0uG5/zqnAdN8AB1pbcRRkdc1DL8SZdQVg5RT1gT+V+hCVWGcEX3cYCU3ofJP605 Sc8=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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.

>
> The existing implementation of ACCESS_ONCE() can only work on scalar
> type. The implementation is based on a Linux, although we have an
> extra check. Looking through the Linux history, it looks like it is
> not possible to make ACCESS_ONCE() work with non-scalar types:
>
>     ACCESS_ONCE does not work reliably on non-scalar types. For
>     example gcc 4.6 and 4.7 might remove the volatile tag for such
>     accesses during the SRA (scalar replacement of aggregates) step
>     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58145)
>
> I understand that our implementation of read_atomic(), write_atomic()
> would lead to less optimized code.

There are cases where read_atomic()/write_atomic() prevent optimisations
which ACCESS_ONCE() would allow, but it is only for code of the form:

ACCESS_ONCE(ptr) |= val;

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`.

That said - your note about GCC treating the pointed-to object as
volatile probably means it won't make the above optimisation, even
though it would be appropriate to do so.

> So maybe we want to import READ_ONCE() and WRITE_ONCE() from Linux?

There is no point.  Linux has taken a massive detour through wildly
different READ/WRITE_ONCE() functions (to fix the above GCC bugs), and
are now back to something very similar to ACCESS_ONCE().

~Andrew



 


Rackspace

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