Re: [PATCH v4 02/10] evtchn: bind-interdomain doesn't need to hold both domains' event locks

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.01.2021 17:14, Julien Grall wrote:
> On 09/01/2021 15:41, Julien Grall wrote:
>> On 05/01/2021 13:09, Jan Beulich wrote:
>>> The local domain's lock is needed for the port allocation, but for the
>>> remote side the per-channel lock is sufficient. The per-channel locks
>>> then need to be acquired slightly earlier, though.
>>
>> I was expecting is little bit more information in the commit message 
>> because there are a few changes in behavior with this change:
>>
>>   1) AFAICT, evtchn_allocate_port() rely on rchn->state to be protected 
>> by the rd->event_lock. Now that you dropped the rd->event_lock, 
>> rchn->state may be accessed while it is updated in 
>> evtchn_bind_interdomain(). The port cannot go back to ECS_FREE here, but 
>> I think the access needs to be switched to {read, write}_atomic() or 
>> ACCESS_ONCE.
>>
>>    2) xsm_evtchn_interdomain() is now going to be called without the 
>> rd->event_lock. Can you confirm that the lock is not needed by XSM?
> 
> Actually, I think there is a bigger issue. evtchn_close() will check 
> chn1->state with just d1->event_lock held (IOW, there chn1->lock is not 
> held).
> 
> If the remote domain happen to close the unbound port at the same time 
> the local domain bound it, then you may end up in the following situation:
> 
> 
> evtchn_bind_interdomain()        | evtchn_close()
>                                   |
>                                   |  switch ( chn1->state )
>                                   |  case ECS_UNBOUND:
>                                   |      /* nothing to do */
>     double_evtchn_lock()          |
>     rchn->state = ECS_INTERDOMAIN |
>     double_evtchn_unlock()        |
>                                   |  evtchn_write_lock(chn1)
>                                   |  evtchn_free(d1, chn1)
>                                   |  evtchn_write_unlock(chn1)
> 
> When the local domain will try to close the port, it will hit the 
> BUG_ON(chn2->state != ECS_INTERDOMAIN) because the remote port were 
> already freed.

Hmm, yes, thanks for spotting (and sorry for taking a while to
reply).

> I think this can be solved by acquiring the event lock earlier on in 
> evtchn_close(). Although, this may become a can of worms as it would be 
> more complex to prevent lock inversion because chn1->lock and chn2->lock.

Indeed. I think I'll give up on trying to eliminate the double
per-domain event locking for the time being, and resubmit with
both patches dropped.

Jan