Mailing List Archive

[PATCH v2 1/8] evtchn: avoid race in get_xen_consumer()
There's no global lock around the updating of this global piece of data.
Make use of cmpxchgptr() to avoid two entities racing with their
updates.

While touching the functionality, mark xen_consumers[] read-mostly (or
else the if() condition could use the result of cmpxchgptr(), writing to
the slot unconditionally).

The use of cmpxchgptr() here points out (by way of clang warning about
it) that its original use of const was slightly wrong. Adjust the
placement, or else undefined behavior of const qualifying a function
type will result.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Use (and hence generalize) cmpxchgptr(). Add comment. Expand /
adjust description.

--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -57,7 +57,8 @@
* with a pointer, we stash them dynamically in a small lookup array which
* can be indexed by a small integer.
*/
-static xen_event_channel_notification_t xen_consumers[NR_XEN_CONSUMERS];
+static xen_event_channel_notification_t __read_mostly
+ xen_consumers[NR_XEN_CONSUMERS];

/* Default notification action: wake up from wait_on_xen_event_channel(). */
static void default_xen_notification_fn(struct vcpu *v, unsigned int port)
@@ -80,8 +81,9 @@ static uint8_t get_xen_consumer(xen_even

for ( i = 0; i < ARRAY_SIZE(xen_consumers); i++ )
{
+ /* Use cmpxchgptr() in lieu of a global lock. */
if ( xen_consumers[i] == NULL )
- xen_consumers[i] = fn;
+ cmpxchgptr(&xen_consumers[i], NULL, fn);
if ( xen_consumers[i] == fn )
break;
}
--- a/xen/include/asm-x86/system.h
+++ b/xen/include/asm-x86/system.h
@@ -148,13 +148,6 @@ static always_inline unsigned long cmpxc
return prev;
}

-#define cmpxchgptr(ptr,o,n) ({ \
- const __typeof__(**(ptr)) *__o = (o); \
- __typeof__(**(ptr)) *__n = (n); \
- ((__typeof__(*(ptr)))__cmpxchg((ptr),(unsigned long)__o, \
- (unsigned long)__n,sizeof(*(ptr)))); \
-})
-
/*
* Undefined symbol to cause link failure if a wrong size is used with
* arch_fetch_and_add().
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -178,6 +178,17 @@ unsigned long long parse_size_and_unit(c

uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c);

+/*
+ * A slightly more typesafe variant of cmpxchg() when the entities dealt with
+ * are pointers.
+ */
+#define cmpxchgptr(ptr, o, n) ({ \
+ __typeof__(**(ptr)) *const o_ = (o); \
+ __typeof__(**(ptr)) *n_ = (n); \
+ ((__typeof__(*(ptr)))__cmpxchg(ptr, (unsigned long)o_, \
+ (unsigned long)n_, sizeof(*(ptr)))); \
+})
+
#define TAINT_SYNC_CONSOLE (1u << 0)
#define TAINT_MACHINE_CHECK (1u << 1)
#define TAINT_ERROR_INJECT (1u << 2)
Re: [PATCH v2 1/8] evtchn: avoid race in get_xen_consumer() [ In reply to ]
On Tue, Oct 20, 2020 at 04:08:13PM +0200, Jan Beulich wrote:
> There's no global lock around the updating of this global piece of data.
> Make use of cmpxchgptr() to avoid two entities racing with their
> updates.
>
> While touching the functionality, mark xen_consumers[] read-mostly (or
> else the if() condition could use the result of cmpxchgptr(), writing to
> the slot unconditionally).

I'm not sure I get this, likely related to the comment I have below.

> The use of cmpxchgptr() here points out (by way of clang warning about
> it) that its original use of const was slightly wrong. Adjust the
> placement, or else undefined behavior of const qualifying a function
> type will result.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> ---
> v2: Use (and hence generalize) cmpxchgptr(). Add comment. Expand /
> adjust description.
>
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -57,7 +57,8 @@
> * with a pointer, we stash them dynamically in a small lookup array which
> * can be indexed by a small integer.
> */
> -static xen_event_channel_notification_t xen_consumers[NR_XEN_CONSUMERS];
> +static xen_event_channel_notification_t __read_mostly
> + xen_consumers[NR_XEN_CONSUMERS];
>
> /* Default notification action: wake up from wait_on_xen_event_channel(). */
> static void default_xen_notification_fn(struct vcpu *v, unsigned int port)
> @@ -80,8 +81,9 @@ static uint8_t get_xen_consumer(xen_even
>
> for ( i = 0; i < ARRAY_SIZE(xen_consumers); i++ )
> {
> + /* Use cmpxchgptr() in lieu of a global lock. */
> if ( xen_consumers[i] == NULL )
> - xen_consumers[i] = fn;
> + cmpxchgptr(&xen_consumers[i], NULL, fn);
> if ( xen_consumers[i] == fn )
> break;

I think you could join it as:

if ( !xen_consumers[i] &&
!cmpxchgptr(&xen_consumers[i], NULL, fn) )
break;

As cmpxchgptr will return the previous value of &xen_consumers[i]?

Thanks, Roger.
Re: [PATCH v2 1/8] evtchn: avoid race in get_xen_consumer() [ In reply to ]
On 21.10.2020 17:46, Roger Pau Monné wrote:
> On Tue, Oct 20, 2020 at 04:08:13PM +0200, Jan Beulich wrote:
>> There's no global lock around the updating of this global piece of data.
>> Make use of cmpxchgptr() to avoid two entities racing with their
>> updates.
>>
>> While touching the functionality, mark xen_consumers[] read-mostly (or
>> else the if() condition could use the result of cmpxchgptr(), writing to
>> the slot unconditionally).
>
> I'm not sure I get this, likely related to the comment I have below.

This is about the alternative case of invoking cmpxchgptr()
without the if() around it. On x86 this would mean always
writing the field, even if the designated value is already in
place.

>> --- a/xen/common/event_channel.c
>> +++ b/xen/common/event_channel.c
>> @@ -57,7 +57,8 @@
>> * with a pointer, we stash them dynamically in a small lookup array which
>> * can be indexed by a small integer.
>> */
>> -static xen_event_channel_notification_t xen_consumers[NR_XEN_CONSUMERS];
>> +static xen_event_channel_notification_t __read_mostly
>> + xen_consumers[NR_XEN_CONSUMERS];
>>
>> /* Default notification action: wake up from wait_on_xen_event_channel(). */
>> static void default_xen_notification_fn(struct vcpu *v, unsigned int port)
>> @@ -80,8 +81,9 @@ static uint8_t get_xen_consumer(xen_even
>>
>> for ( i = 0; i < ARRAY_SIZE(xen_consumers); i++ )
>> {
>> + /* Use cmpxchgptr() in lieu of a global lock. */
>> if ( xen_consumers[i] == NULL )
>> - xen_consumers[i] = fn;
>> + cmpxchgptr(&xen_consumers[i], NULL, fn);
>> if ( xen_consumers[i] == fn )
>> break;
>
> I think you could join it as:
>
> if ( !xen_consumers[i] &&
> !cmpxchgptr(&xen_consumers[i], NULL, fn) )
> break;
>
> As cmpxchgptr will return the previous value of &xen_consumers[i]?

But then you also have to check whether the returned value is
fn (or retain the 2nd if()).

Jan
Re: [PATCH v2 1/8] evtchn: avoid race in get_xen_consumer() [ In reply to ]
On Thu, Oct 22, 2020 at 09:33:27AM +0200, Jan Beulich wrote:
> On 21.10.2020 17:46, Roger Pau Monné wrote:
> > On Tue, Oct 20, 2020 at 04:08:13PM +0200, Jan Beulich wrote:
> >> There's no global lock around the updating of this global piece of data.
> >> Make use of cmpxchgptr() to avoid two entities racing with their
> >> updates.
> >>
> >> While touching the functionality, mark xen_consumers[] read-mostly (or
> >> else the if() condition could use the result of cmpxchgptr(), writing to
> >> the slot unconditionally).
> >
> > I'm not sure I get this, likely related to the comment I have below.
>
> This is about the alternative case of invoking cmpxchgptr()
> without the if() around it. On x86 this would mean always
> writing the field, even if the designated value is already in
> place.
>
> >> --- a/xen/common/event_channel.c
> >> +++ b/xen/common/event_channel.c
> >> @@ -57,7 +57,8 @@
> >> * with a pointer, we stash them dynamically in a small lookup array which
> >> * can be indexed by a small integer.
> >> */
> >> -static xen_event_channel_notification_t xen_consumers[NR_XEN_CONSUMERS];
> >> +static xen_event_channel_notification_t __read_mostly
> >> + xen_consumers[NR_XEN_CONSUMERS];
> >>
> >> /* Default notification action: wake up from wait_on_xen_event_channel(). */
> >> static void default_xen_notification_fn(struct vcpu *v, unsigned int port)
> >> @@ -80,8 +81,9 @@ static uint8_t get_xen_consumer(xen_even
> >>
> >> for ( i = 0; i < ARRAY_SIZE(xen_consumers); i++ )
> >> {
> >> + /* Use cmpxchgptr() in lieu of a global lock. */
> >> if ( xen_consumers[i] == NULL )
> >> - xen_consumers[i] = fn;
> >> + cmpxchgptr(&xen_consumers[i], NULL, fn);
> >> if ( xen_consumers[i] == fn )
> >> break;
> >
> > I think you could join it as:
> >
> > if ( !xen_consumers[i] &&
> > !cmpxchgptr(&xen_consumers[i], NULL, fn) )
> > break;
> >
> > As cmpxchgptr will return the previous value of &xen_consumers[i]?
>
> But then you also have to check whether the returned value is
> fn (or retain the 2nd if()).

__cmpxchg comment says that success of the operation is indicated when
the returned value equals the old value, so it's my understanding that
cmpxchgptr returning NULL would mean the exchange has succeed and that
xen_consumers[i] == fn?

Roger.
Re: [PATCH v2 1/8] evtchn: avoid race in get_xen_consumer() [ In reply to ]
On 22.10.2020 10:11, Roger Pau Monné wrote:
> On Thu, Oct 22, 2020 at 09:33:27AM +0200, Jan Beulich wrote:
>> On 21.10.2020 17:46, Roger Pau Monné wrote:
>>> On Tue, Oct 20, 2020 at 04:08:13PM +0200, Jan Beulich wrote:
>>>> @@ -80,8 +81,9 @@ static uint8_t get_xen_consumer(xen_even
>>>>
>>>> for ( i = 0; i < ARRAY_SIZE(xen_consumers); i++ )
>>>> {
>>>> + /* Use cmpxchgptr() in lieu of a global lock. */
>>>> if ( xen_consumers[i] == NULL )
>>>> - xen_consumers[i] = fn;
>>>> + cmpxchgptr(&xen_consumers[i], NULL, fn);
>>>> if ( xen_consumers[i] == fn )
>>>> break;
>>>
>>> I think you could join it as:
>>>
>>> if ( !xen_consumers[i] &&
>>> !cmpxchgptr(&xen_consumers[i], NULL, fn) )
>>> break;
>>>
>>> As cmpxchgptr will return the previous value of &xen_consumers[i]?
>>
>> But then you also have to check whether the returned value is
>> fn (or retain the 2nd if()).
>
> __cmpxchg comment says that success of the operation is indicated when
> the returned value equals the old value, so it's my understanding that
> cmpxchgptr returning NULL would mean the exchange has succeed and that
> xen_consumers[i] == fn?

Correct. But if xen_consumers[i] == fn before the call, the return
value will be fn. The cmpxchg() wasn't "successful" in this case
(it didn't update anything), but the state of the array slot is what
we want.

Jan
Re: [PATCH v2 1/8] evtchn: avoid race in get_xen_consumer() [ In reply to ]
On Thu, Oct 22, 2020 at 10:15:45AM +0200, Jan Beulich wrote:
> On 22.10.2020 10:11, Roger Pau Monné wrote:
> > On Thu, Oct 22, 2020 at 09:33:27AM +0200, Jan Beulich wrote:
> >> On 21.10.2020 17:46, Roger Pau Monné wrote:
> >>> On Tue, Oct 20, 2020 at 04:08:13PM +0200, Jan Beulich wrote:
> >>>> @@ -80,8 +81,9 @@ static uint8_t get_xen_consumer(xen_even
> >>>>
> >>>> for ( i = 0; i < ARRAY_SIZE(xen_consumers); i++ )
> >>>> {
> >>>> + /* Use cmpxchgptr() in lieu of a global lock. */
> >>>> if ( xen_consumers[i] == NULL )
> >>>> - xen_consumers[i] = fn;
> >>>> + cmpxchgptr(&xen_consumers[i], NULL, fn);
> >>>> if ( xen_consumers[i] == fn )
> >>>> break;
> >>>
> >>> I think you could join it as:
> >>>
> >>> if ( !xen_consumers[i] &&
> >>> !cmpxchgptr(&xen_consumers[i], NULL, fn) )
> >>> break;
> >>>
> >>> As cmpxchgptr will return the previous value of &xen_consumers[i]?
> >>
> >> But then you also have to check whether the returned value is
> >> fn (or retain the 2nd if()).
> >
> > __cmpxchg comment says that success of the operation is indicated when
> > the returned value equals the old value, so it's my understanding that
> > cmpxchgptr returning NULL would mean the exchange has succeed and that
> > xen_consumers[i] == fn?
>
> Correct. But if xen_consumers[i] == fn before the call, the return
> value will be fn. The cmpxchg() wasn't "successful" in this case
> (it didn't update anything), but the state of the array slot is what
> we want.

Oh, I get it now. You don't want the same fn populating more than one
slot.

I assume the reads of xen_consumers are not using ACCESS_ONCE or
read_atomic because we rely on the compiler performing such reads as
single instructions?

Roger.
Re: [PATCH v2 1/8] evtchn: avoid race in get_xen_consumer() [ In reply to ]
On 22.10.2020 10:29, Roger Pau Monné wrote:
> On Thu, Oct 22, 2020 at 10:15:45AM +0200, Jan Beulich wrote:
>> On 22.10.2020 10:11, Roger Pau Monné wrote:
>>> On Thu, Oct 22, 2020 at 09:33:27AM +0200, Jan Beulich wrote:
>>>> On 21.10.2020 17:46, Roger Pau Monné wrote:
>>>>> On Tue, Oct 20, 2020 at 04:08:13PM +0200, Jan Beulich wrote:
>>>>>> @@ -80,8 +81,9 @@ static uint8_t get_xen_consumer(xen_even
>>>>>>
>>>>>> for ( i = 0; i < ARRAY_SIZE(xen_consumers); i++ )
>>>>>> {
>>>>>> + /* Use cmpxchgptr() in lieu of a global lock. */
>>>>>> if ( xen_consumers[i] == NULL )
>>>>>> - xen_consumers[i] = fn;
>>>>>> + cmpxchgptr(&xen_consumers[i], NULL, fn);
>>>>>> if ( xen_consumers[i] == fn )
>>>>>> break;
>>>>>
>>>>> I think you could join it as:
>>>>>
>>>>> if ( !xen_consumers[i] &&
>>>>> !cmpxchgptr(&xen_consumers[i], NULL, fn) )
>>>>> break;
>>>>>
>>>>> As cmpxchgptr will return the previous value of &xen_consumers[i]?
>>>>
>>>> But then you also have to check whether the returned value is
>>>> fn (or retain the 2nd if()).
>>>
>>> __cmpxchg comment says that success of the operation is indicated when
>>> the returned value equals the old value, so it's my understanding that
>>> cmpxchgptr returning NULL would mean the exchange has succeed and that
>>> xen_consumers[i] == fn?
>>
>> Correct. But if xen_consumers[i] == fn before the call, the return
>> value will be fn. The cmpxchg() wasn't "successful" in this case
>> (it didn't update anything), but the state of the array slot is what
>> we want.
>
> Oh, I get it now. You don't want the same fn populating more than one
> slot.

FAOD it's not just "want", it's a strict requirement.

> I assume the reads of xen_consumers are not using ACCESS_ONCE or
> read_atomic because we rely on the compiler performing such reads as
> single instructions?

Yes, as in so many other places in the code base.

Jan
Re: [PATCH v2 1/8] evtchn: avoid race in get_xen_consumer() [ In reply to ]
On Tue, Oct 20, 2020 at 04:08:13PM +0200, Jan Beulich wrote:
> There's no global lock around the updating of this global piece of data.
> Make use of cmpxchgptr() to avoid two entities racing with their
> updates.
>
> While touching the functionality, mark xen_consumers[] read-mostly (or
> else the if() condition could use the result of cmpxchgptr(), writing to
> the slot unconditionally).
>
> The use of cmpxchgptr() here points out (by way of clang warning about
> it) that its original use of const was slightly wrong. Adjust the
> placement, or else undefined behavior of const qualifying a function
> type will result.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.
Re: [PATCH v2 1/8] evtchn: avoid race in get_xen_consumer() [ In reply to ]
On Thu, Oct 22, 2020 at 10:56:15AM +0200, Jan Beulich wrote:
> On 22.10.2020 10:29, Roger Pau Monné wrote:
> > On Thu, Oct 22, 2020 at 10:15:45AM +0200, Jan Beulich wrote:
> >> On 22.10.2020 10:11, Roger Pau Monné wrote:
> >>> On Thu, Oct 22, 2020 at 09:33:27AM +0200, Jan Beulich wrote:
> >>>> On 21.10.2020 17:46, Roger Pau Monné wrote:
> >>>>> On Tue, Oct 20, 2020 at 04:08:13PM +0200, Jan Beulich wrote:
> >>>>>> @@ -80,8 +81,9 @@ static uint8_t get_xen_consumer(xen_even
> >>>>>>
> >>>>>> for ( i = 0; i < ARRAY_SIZE(xen_consumers); i++ )
> >>>>>> {
> >>>>>> + /* Use cmpxchgptr() in lieu of a global lock. */
> >>>>>> if ( xen_consumers[i] == NULL )
> >>>>>> - xen_consumers[i] = fn;
> >>>>>> + cmpxchgptr(&xen_consumers[i], NULL, fn);
> >>>>>> if ( xen_consumers[i] == fn )
> >>>>>> break;
> >>>>>
> >>>>> I think you could join it as:
> >>>>>
> >>>>> if ( !xen_consumers[i] &&
> >>>>> !cmpxchgptr(&xen_consumers[i], NULL, fn) )
> >>>>> break;
> >>>>>
> >>>>> As cmpxchgptr will return the previous value of &xen_consumers[i]?
> >>>>
> >>>> But then you also have to check whether the returned value is
> >>>> fn (or retain the 2nd if()).
> >>>
> >>> __cmpxchg comment says that success of the operation is indicated when
> >>> the returned value equals the old value, so it's my understanding that
> >>> cmpxchgptr returning NULL would mean the exchange has succeed and that
> >>> xen_consumers[i] == fn?
> >>
> >> Correct. But if xen_consumers[i] == fn before the call, the return
> >> value will be fn. The cmpxchg() wasn't "successful" in this case
> >> (it didn't update anything), but the state of the array slot is what
> >> we want.
> >
> > Oh, I get it now. You don't want the same fn populating more than one
> > slot.
>
> FAOD it's not just "want", it's a strict requirement.

I wouldn't mind having a comment to that effect in the function, but I
won't insist.

Thanks, Roger.
Re: [PATCH v2 1/8] evtchn: avoid race in get_xen_consumer() [ In reply to ]
Hi Jan,

On 20/10/2020 15:08, Jan Beulich wrote:
> There's no global lock around the updating of this global piece of data.
> Make use of cmpxchgptr() to avoid two entities racing with their
> updates.
>
> While touching the functionality, mark xen_consumers[] read-mostly (or
> else the if() condition could use the result of cmpxchgptr(), writing to
> the slot unconditionally).
>
> The use of cmpxchgptr() here points out (by way of clang warning about
> it) that its original use of const was slightly wrong. Adjust the
> placement, or else undefined behavior of const qualifying a function
> type will result.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

--
Julien Grall