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

[Julien Grall <julien@xxxxxxx>]

On 23/06/2022 16:10, Rahul Singh wrote:
> 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 don't think it is Xen job to prevent a guest to close a static port. 
If the guest decide to do it, then it will just break itself and not Xen.
> 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.
To me, this reads as Linux should be modified in order to avoid closing 
static event channel. In fact...
> >   
> > [...]
> > > 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.
... this shows that we are papering over a bigger problem: Linux is not 
ready for static event channels.
> if (HYPERVISOR_event_channel_op(EVTCHNOP_close, &close) != 0)
>          BUG();
The BUG() in Linux is definitely not a reason to lie and claim the port 
was closed.
If you tell that to an OS, it may validly think that it know need to 
call bind interdomain in order to "re-open" the port. So your Linux will 
already need some information to know that the port is "static".
At which point, you can modify Linux to also prevent the port to be closed.

Cheers,

--
Julien Grall