Re: [PATCH 5/8] xen/evtchn: don't close the static event channel.

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Julien,


> On 22 Jun 2022, at 4:05 pm, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi,
> 
> On 22/06/2022 15:38, Rahul Singh wrote:
>> Guest can request the Xen to close the event channels. Ignore the
>> request from guest to close the static channels as static event channels
>> should not be closed.
> 
> Why do you want to prevent the guest to close static ports? The problem I can 
> see is...

As a static event channel should be available during the lifetime of the guest 
we want to prevent
the guest to close the static ports. 

I tested this series to send/receive event notification from the Linux 
user-space application via "/dev/xen/evtchn” interface and
ioctl ( IOCTL_EVTCHN_*) calls. When we close the "/dev/xen/evtchn” interface 
Linux event channel
driver will try to close the static event channel also, that why we need this 
patch to avoid guests to close 
the event channel as we don’t want to close the static event channel.

>  
> [...]
> 
>> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
>> index 84f0055a5a..cedc98ccaf 100644
>> --- a/xen/common/event_channel.c
>> +++ b/xen/common/event_channel.c
>> @@ -294,7 +294,8 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
>>   * If port is zero get the next free port and allocate. If port is non-zero
>>   * allocate the specified port.
>>   */
>> -int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, evtchn_port_t port)
>> +int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, evtchn_port_t port,
>> +                         bool is_static)
>>  {
>>      struct evtchn *chn;
>>      struct domain *d;
>> @@ -330,6 +331,7 @@ int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, 
>> evtchn_port_t port)
>>      evtchn_write_lock(chn);
>>        chn->state = ECS_UNBOUND;
>> +    chn->is_static = is_static;
>>      if ( (chn->u.unbound.remote_domid = alloc->remote_dom) == DOMID_SELF )
>>          chn->u.unbound.remote_domid = current->domain->domain_id;
>>      evtchn_port_init(d, chn);
>> @@ -368,7 +370,7 @@ static void double_evtchn_unlock(struct evtchn *lchn, 
>> struct evtchn *rchn)
>>   * allocate the specified lport.
>>   */
>>  int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind, struct domain 
>> *ld,
>> -                            evtchn_port_t lport)
>> +                            evtchn_port_t lport, bool is_static)
>>  {
>>      struct evtchn *lchn, *rchn;
>>      struct domain *rd;
>> @@ -423,6 +425,7 @@ int evtchn_bind_interdomain(evtchn_bind_interdomain_t 
>> *bind, struct domain *ld,
>>      lchn->u.interdomain.remote_dom  = rd;
>>      lchn->u.interdomain.remote_port = rport;
>>      lchn->state                     = ECS_INTERDOMAIN;
>> +    lchn->is_static                 = is_static;
>>      evtchn_port_init(ld, lchn);
>>            rchn->u.interdomain.remote_dom  = ld;
>> @@ -659,6 +662,9 @@ int evtchn_close(struct domain *d1, int port1, bool 
>> guest)
>>          rc = -EINVAL;
>>          goto out;
>>      }
>> +    /* Guest cannot close a static event channel. */
>> +    if ( chn1->is_static && guest )
>> +        goto out;
> 
> ... at least the interdomain structure store pointer to the domain. I am a 
> bit concerned that we would end up to leave dangling pointers (such as 
> chn->u.interdomain.remote_domain) as evtchn_close() is also used while 
> destroying the domain.

Let me have a look again if we have to do the cleanup when we destroy the guest 
and close the static event channel.
> 
> Also, AFAICT Xen will return 0 (i.e. success) to the caller. I think this is 
> a mistake because we didn't close the port as requested.

If we return non-zero to guest (in particular if linux guest), Linux will 
report the BUG(). Therefore I decided to return 0. 

if (HYPERVISOR_event_channel_op(EVTCHNOP_close, &close) != 0)               
        BUG();

Regards,
Rahul