Re: [PATCH] xen/events: Fix race in set_evtchn_to_irq

[Heyne, Maximilian]

On Wed, 2021-08-11 at 11:05 -0400, Boris Ostrovsky wrote:
> On 8/11/21 10:08 AM, Maximilian Heyne wrote:
> > This patch sets evtchn_to_irq rows via a cmpxchg operation so that
> > they
> > will be set only once. Clearing the row was moved up before writing
> > the
> > row to evtchn_to_irq in order to not create a race once the row is
> > visible for other threads. Accesses to the rows are now guarded by
> > READ_ONCE and WRITE_ONCE just as for the columns in the data
> > structure.
> 
> Is this last part really needed? We needed to do that for array
> elements to avoid an interrupt handler from seeing a partially
> updated entry but I am not sure I see how this can happen to the row
> pointer. The only place where it might be important is when we update
> the pointer to the new page but you are using cmpxchg there already.

I think you are right. I will remove the changes related to the
READ_ONCE.

> 
> 
> > -             evtchn_to_irq[row] = (int
> > *)get_zeroed_page(GFP_KERNEL);
> > -             if (evtchn_to_irq[row] == NULL)
> > +             evtchn_row = (int *) get_zeroed_page(GFP_KERNEL);
> 
> Not directly related to this patch but I don't think we need to get a
> zeroed page --- we will initialize it to -1 immediately below.

That is correct. I will just fix this in the next version of the patch.

Max



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879