[PATCH v6 0/3] evtchn: (not so) recent XSAs follow-on

[PATCH v6 0/3] evtchn: (not so) recent XSAs follow-on

[Jan Beulich]

These are grouped into a series largely because of their origin,
not so much because there are (heavy) dependencies among them.
The main change from v6 is the dropping of three more patches,
and re-basing.

1: slightly defer lock acquire where possible
2: add helper for port_is_valid() + evtchn_from_port()
3: type adjustments

Jan

[PATCH v6 1/3] evtchn: slightly defer lock acquire where possible

[Jan Beulich]

port_is_valid() and evtchn_from_port() are fine to use without holding
any locks. Accordingly acquire the per-domain lock slightly later in
evtchn_close() and evtchn_bind_vcpu().

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v6: Re-base for re-ordering / shrinking of series.
v4: New.

--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -606,17 +606,14 @@ int evtchn_close(struct domain *d1, int
     int            port2;
     long           rc = 0;
 
- again:
-    spin_lock(&d1->event_lock);
-
     if ( !port_is_valid(d1, port1) )
-    {
-        rc = -EINVAL;
-        goto out;
-    }
+        return -EINVAL;
 
     chn1 = evtchn_from_port(d1, port1);
 
+ again:
+    spin_lock(&d1->event_lock);
+
     /* Guest cannot close a Xen-attached event channel. */
     if ( unlikely(consumer_is_xen(chn1)) && guest )
     {
@@ -1041,16 +1038,13 @@ long evtchn_bind_vcpu(unsigned int port,
     if ( (v = domain_vcpu(d, vcpu_id)) == NULL )
         return -ENOENT;
 
-    spin_lock(&d->event_lock);
-
     if ( !port_is_valid(d, port) )
-    {
-        rc = -EINVAL;
-        goto out;
-    }
+        return -EINVAL;
 
     chn = evtchn_from_port(d, port);
 
+    spin_lock(&d->event_lock);
+
     /* Guest cannot re-bind a Xen-attached event channel. */
     if ( unlikely(consumer_is_xen(chn)) )
     {

[PATCH v6 2/3] evtchn: add helper for port_is_valid() + evtchn_from_port()

[Jan Beulich]

The combination is pretty common, so adding a simple local helper seems
worthwhile. Make it const- and type-correct, in turn requiring the
two called function to also be const-correct (and at this occasion also
make them type-correct).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---
v6: Re-base, also for re-ordering / shrinking of series.
v4: New.

--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -147,6 +147,12 @@ static bool virq_is_global(unsigned int
     return true;
 }
 
+static struct evtchn *_evtchn_from_port(const struct domain *d,
+                                        evtchn_port_t port)
+{
+    return port_is_valid(d, port) ? evtchn_from_port(d, port) : NULL;
+}
+
 static void free_evtchn_bucket(struct domain *d, struct evtchn *bucket)
 {
     if ( !bucket )
@@ -319,7 +325,6 @@ static long evtchn_alloc_unbound(evtchn_
     return rc;
 }
 
-
 static void double_evtchn_lock(struct evtchn *lchn, struct evtchn *rchn)
 {
     ASSERT(lchn != rchn);
@@ -365,9 +370,9 @@ static long evtchn_bind_interdomain(evtc
         ERROR_EXIT(lport);
     lchn = evtchn_from_port(ld, lport);
 
-    if ( !port_is_valid(rd, rport) )
+    rchn = _evtchn_from_port(rd, rport);
+    if ( !rchn )
         ERROR_EXIT_DOM(-EINVAL, rd);
-    rchn = evtchn_from_port(rd, rport);
     if ( (rchn->state != ECS_UNBOUND) ||
          (rchn->u.unbound.remote_domid != ld->domain_id) )
         ERROR_EXIT_DOM(-EINVAL, rd);
@@ -602,15 +607,12 @@ static long evtchn_bind_pirq(evtchn_bind
 int evtchn_close(struct domain *d1, int port1, bool guest)
 {
     struct domain *d2 = NULL;
-    struct evtchn *chn1, *chn2;
-    int            port2;
+    struct evtchn *chn1 = _evtchn_from_port(d1, port1), *chn2;
     long           rc = 0;
 
-    if ( !port_is_valid(d1, port1) )
+    if ( !chn1 )
         return -EINVAL;
 
-    chn1 = evtchn_from_port(d1, port1);
-
  again:
     spin_lock(&d1->event_lock);
 
@@ -698,10 +700,8 @@ int evtchn_close(struct domain *d1, int
             goto out;
         }
 
-        port2 = chn1->u.interdomain.remote_port;
-        BUG_ON(!port_is_valid(d2, port2));
-
-        chn2 = evtchn_from_port(d2, port2);
+        chn2 = _evtchn_from_port(d2, chn1->u.interdomain.remote_port);
+        BUG_ON(!chn2);
         BUG_ON(chn2->state != ECS_INTERDOMAIN);
         BUG_ON(chn2->u.interdomain.remote_dom != d1);
 
@@ -739,15 +739,13 @@ int evtchn_close(struct domain *d1, int
 
 int evtchn_send(struct domain *ld, unsigned int lport)
 {
-    struct evtchn *lchn, *rchn;
+    struct evtchn *lchn = _evtchn_from_port(ld, lport), *rchn;
     struct domain *rd;
     int            rport, ret = 0;
 
-    if ( !port_is_valid(ld, lport) )
+    if ( !lchn )
         return -EINVAL;
 
-    lchn = evtchn_from_port(ld, lport);
-
     evtchn_read_lock(lchn);
 
     /* Guest cannot send via a Xen-attached event channel. */
@@ -967,15 +965,15 @@ int evtchn_status(evtchn_status_t *statu
     if ( d == NULL )
         return -ESRCH;
 
-    spin_lock(&d->event_lock);
-
-    if ( !port_is_valid(d, port) )
+    chn = _evtchn_from_port(d, port);
+    if ( !chn )
     {
-        rc = -EINVAL;
-        goto out;
+        rcu_unlock_domain(d);
+        return -EINVAL;
     }
 
-    chn = evtchn_from_port(d, port);
+    spin_lock(&d->event_lock);
+
     if ( consumer_is_xen(chn) )
     {
         rc = -EACCES;
@@ -1038,11 +1036,10 @@ long evtchn_bind_vcpu(unsigned int port,
     if ( (v = domain_vcpu(d, vcpu_id)) == NULL )
         return -ENOENT;
 
-    if ( !port_is_valid(d, port) )
+    chn = _evtchn_from_port(d, port);
+    if ( !chn )
         return -EINVAL;
 
-    chn = evtchn_from_port(d, port);
-
     spin_lock(&d->event_lock);
 
     /* Guest cannot re-bind a Xen-attached event channel. */
@@ -1088,13 +1085,11 @@ long evtchn_bind_vcpu(unsigned int port,
 int evtchn_unmask(unsigned int port)
 {
     struct domain *d = current->domain;
-    struct evtchn *evtchn;
+    struct evtchn *evtchn = _evtchn_from_port(d, port);
 
-    if ( unlikely(!port_is_valid(d, port)) )
+    if ( unlikely(!evtchn) )
         return -EINVAL;
 
-    evtchn = evtchn_from_port(d, port);
-
     evtchn_read_lock(evtchn);
 
     evtchn_port_unmask(d, evtchn);
@@ -1177,14 +1172,12 @@ static long evtchn_set_priority(const st
 {
     struct domain *d = current->domain;
     unsigned int port = set_priority->port;
-    struct evtchn *chn;
+    struct evtchn *chn = _evtchn_from_port(d, port);
     long ret;
 
-    if ( !port_is_valid(d, port) )
+    if ( !chn )
         return -EINVAL;
 
-    chn = evtchn_from_port(d, port);
-
     evtchn_read_lock(chn);
 
     ret = evtchn_port_set_priority(d, chn, set_priority->priority);
@@ -1410,10 +1403,10 @@ void free_xen_event_channel(struct domai
 
 void notify_via_xen_event_channel(struct domain *ld, int lport)
 {
-    struct evtchn *lchn, *rchn;
+    struct evtchn *lchn = _evtchn_from_port(ld, lport), *rchn;
     struct domain *rd;
 
-    if ( !port_is_valid(ld, lport) )
+    if ( !lchn )
     {
         /*
          * Make sure ->is_dying is read /after/ ->valid_evtchns, pairing
@@ -1424,8 +1417,6 @@ void notify_via_xen_event_channel(struct
         return;
     }
 
-    lchn = evtchn_from_port(ld, lport);
-
     if ( !evtchn_read_trylock(lchn) )
         return;
 
@@ -1580,12 +1571,14 @@ static void domain_dump_evtchn_info(stru
 
     spin_lock(&d->event_lock);
 
-    for ( port = 1; port_is_valid(d, port); ++port )
+    for ( port = 1; ; ++port )
     {
-        const struct evtchn *chn;
+        const struct evtchn *chn = _evtchn_from_port(d, port);
         char *ssid;
 
-        chn = evtchn_from_port(d, port);
+        if ( !chn )
+            break;
+
         if ( chn->state == ECS_FREE )
             continue;
 
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -120,7 +120,7 @@ static inline void evtchn_read_unlock(st
     read_unlock(&evtchn->lock);
 }
 
-static inline bool_t port_is_valid(struct domain *d, unsigned int p)
+static inline bool port_is_valid(const struct domain *d, evtchn_port_t p)
 {
     if ( p >= read_atomic(&d->valid_evtchns) )
         return false;
@@ -135,7 +135,8 @@ static inline bool_t port_is_valid(struc
     return true;
 }
 
-static inline struct evtchn *evtchn_from_port(struct domain *d, unsigned int p)
+static inline struct evtchn *evtchn_from_port(const struct domain *d,
+                                              evtchn_port_t p)
 {
     if ( p < EVTCHNS_PER_BUCKET )
         return &d->evtchn[array_index_nospec(p, EVTCHNS_PER_BUCKET)];

[PATCH v6 3/3] evtchn: type adjustments

[Jan Beulich]

First of all avoid "long" when "int" suffices, i.e. in particular when
merely conveying error codes. 32-bit values are slightly cheaper to
deal with on x86, and their processing is at least no more expensive on
Arm. Where possible use evtchn_port_t for port numbers and unsigned int
for other unsigned quantities in adjacent code. In evtchn_set_priority()
eliminate a local variable altogether instead of changing its type.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v6: Re-base for re-ordering / shrinking of series.
v4: New.

--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -284,13 +284,12 @@ void evtchn_free(struct domain *d, struc
     xsm_evtchn_close_post(chn);
 }
 
-static long evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
+static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
 {
     struct evtchn *chn;
     struct domain *d;
-    int            port;
+    int            port, rc;
     domid_t        dom = alloc->dom;
-    long           rc;
 
     d = rcu_lock_domain_by_any_id(dom);
     if ( d == NULL )
@@ -342,13 +341,13 @@ static void double_evtchn_unlock(struct
     evtchn_write_unlock(rchn);
 }
 
-static long evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind)
+static int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind)
 {
     struct evtchn *lchn, *rchn;
     struct domain *ld = current->domain, *rd;
-    int            lport, rport = bind->remote_port;
+    int            lport, rc;
+    evtchn_port_t  rport = bind->remote_port;
     domid_t        rdom = bind->remote_dom;
-    long           rc;
 
     if ( (rd = rcu_lock_domain_by_any_id(rdom)) == NULL )
         return -ESRCH;
@@ -484,12 +483,12 @@ int evtchn_bind_virq(evtchn_bind_virq_t
 }
 
 
-static long evtchn_bind_ipi(evtchn_bind_ipi_t *bind)
+static int evtchn_bind_ipi(evtchn_bind_ipi_t *bind)
 {
     struct evtchn *chn;
     struct domain *d = current->domain;
-    int            port, vcpu = bind->vcpu;
-    long           rc = 0;
+    int            port, rc = 0;
+    unsigned int   vcpu = bind->vcpu;
 
     if ( domain_vcpu(d, vcpu) == NULL )
         return -ENOENT;
@@ -543,16 +542,16 @@ static void unlink_pirq_port(struct evtc
 }
 
 
-static long evtchn_bind_pirq(evtchn_bind_pirq_t *bind)
+static int evtchn_bind_pirq(evtchn_bind_pirq_t *bind)
 {
     struct evtchn *chn;
     struct domain *d = current->domain;
     struct vcpu   *v = d->vcpu[0];
     struct pirq   *info;
-    int            port = 0, pirq = bind->pirq;
-    long           rc;
+    int            port = 0, rc;
+    unsigned int   pirq = bind->pirq;
 
-    if ( (pirq < 0) || (pirq >= d->nr_pirqs) )
+    if ( pirq >= d->nr_pirqs )
         return -EINVAL;
 
     if ( !is_hvm_domain(d) && !pirq_access_permitted(d, pirq) )
@@ -608,7 +607,7 @@ int evtchn_close(struct domain *d1, int
 {
     struct domain *d2 = NULL;
     struct evtchn *chn1 = _evtchn_from_port(d1, port1), *chn2;
-    long           rc = 0;
+    int            rc = 0;
 
     if ( !chn1 )
         return -EINVAL;
@@ -959,7 +958,7 @@ int evtchn_status(evtchn_status_t *statu
     domid_t          dom = status->dom;
     int              port = status->port;
     struct evtchn   *chn;
-    long             rc = 0;
+    int              rc = 0;
 
     d = rcu_lock_domain_by_any_id(dom);
     if ( d == NULL )
@@ -1025,11 +1024,11 @@ int evtchn_status(evtchn_status_t *statu
 }
 
 
-long evtchn_bind_vcpu(unsigned int port, unsigned int vcpu_id)
+int evtchn_bind_vcpu(evtchn_port_t port, unsigned int vcpu_id)
 {
     struct domain *d = current->domain;
     struct evtchn *chn;
-    long           rc = 0;
+    int           rc = 0;
     struct vcpu   *v;
 
     /* Use the vcpu info to prevent speculative out-of-bound accesses */
@@ -1168,12 +1167,11 @@ int evtchn_reset(struct domain *d, bool
     return rc;
 }
 
-static long evtchn_set_priority(const struct evtchn_set_priority *set_priority)
+static int evtchn_set_priority(const struct evtchn_set_priority *set_priority)
 {
     struct domain *d = current->domain;
-    unsigned int port = set_priority->port;
-    struct evtchn *chn = _evtchn_from_port(d, port);
-    long ret;
+    struct evtchn *chn = _evtchn_from_port(d, set_priority->port);
+    int ret;
 
     if ( !chn )
         return -EINVAL;
@@ -1189,7 +1187,7 @@ static long evtchn_set_priority(const st
 
 long do_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 {
-    long rc;
+    int rc;
 
     switch ( cmd )
     {
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -54,7 +54,7 @@ void send_guest_pirq(struct domain *, co
 int evtchn_send(struct domain *d, unsigned int lport);
 
 /* Bind a local event-channel port to the specified VCPU. */
-long evtchn_bind_vcpu(unsigned int port, unsigned int vcpu_id);
+int evtchn_bind_vcpu(evtchn_port_t port, unsigned int vcpu_id);
 
 /* Bind a VIRQ. */
 int evtchn_bind_virq(evtchn_bind_virq_t *bind, evtchn_port_t port);

Re: [PATCH v6 1/3] evtchn: slightly defer lock acquire where possible

[Roger Pau Monné]

On Thu, May 27, 2021 at 01:28:07PM +0200, Jan Beulich wrote:
> port_is_valid() and evtchn_from_port() are fine to use without holding
> any locks. Accordingly acquire the per-domain lock slightly later in
> evtchn_close() and evtchn_bind_vcpu().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

Thanks, Roger.

Re: [PATCH v6 3/3] evtchn: type adjustments

[Roger Pau Monné]

On Thu, May 27, 2021 at 01:28:59PM +0200, Jan Beulich wrote:
> First of all avoid "long" when "int" suffices, i.e. in particular when
> merely conveying error codes. 32-bit values are slightly cheaper to
> deal with on x86, and their processing is at least no more expensive on
> Arm. Where possible use evtchn_port_t for port numbers and unsigned int
> for other unsigned quantities in adjacent code. In evtchn_set_priority()
> eliminate a local variable altogether instead of changing its type.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

Just one style nit below.

> -long evtchn_bind_vcpu(unsigned int port, unsigned int vcpu_id)
> +int evtchn_bind_vcpu(evtchn_port_t port, unsigned int vcpu_id)
>  {
>      struct domain *d = current->domain;
>      struct evtchn *chn;
> -    long           rc = 0;
> +    int           rc = 0;
                     ^ I think you are missing one space here, other
                       functions in the same file don't align at the *.

Thanks, Roger.

Re: [PATCH v6 1/3] evtchn: slightly defer lock acquire where possible

[Julien Grall]

Hi Jan,

On 27/05/2021 12:28, Jan Beulich wrote:
> port_is_valid() and evtchn_from_port() are fine to use without holding
> any locks. Accordingly acquire the per-domain lock slightly later in
> evtchn_close() and evtchn_bind_vcpu().

So I agree that port_is_valid() and evtchn_from_port() are fine to use 
without holding any locks in evtchn_bind_vcpu(). However, this is 
misleading to say there is no problem with evtchn_close().

evtchn_close() can be called with current != d and therefore, there is a 
risk that port_is_valid() may be valid and then invalid because 
d->valid_evtchns is decremented in evtchn_destroy().

Thankfully the memory is still there. So the current code is okayish and 
I could reluctantly accept this behavior to be spread. However, I don't 
think this should be left uncommented in both the code (maybe on top of 
port_is_valid()?) and the commit message.

> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v6: Re-base for re-ordering / shrinking of series.
> v4: New.
> 
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -606,17 +606,14 @@ int evtchn_close(struct domain *d1, int
>       int            port2;
>       long           rc = 0;
>   
> - again:
> -    spin_lock(&d1->event_lock);
> -
>       if ( !port_is_valid(d1, port1) )
> -    {
> -        rc = -EINVAL;
> -        goto out;
> -    }
> +        return -EINVAL;
>   
>       chn1 = evtchn_from_port(d1, port1);
>   
> + again:
> +    spin_lock(&d1->event_lock);
> +
>       /* Guest cannot close a Xen-attached event channel. */
>       if ( unlikely(consumer_is_xen(chn1)) && guest )
>       {
> @@ -1041,16 +1038,13 @@ long evtchn_bind_vcpu(unsigned int port,
>       if ( (v = domain_vcpu(d, vcpu_id)) == NULL )
>           return -ENOENT;
>   
> -    spin_lock(&d->event_lock);
> -
>       if ( !port_is_valid(d, port) )
> -    {
> -        rc = -EINVAL;
> -        goto out;
> -    }
> +        return -EINVAL;
>   
>       chn = evtchn_from_port(d, port);
>   
> +    spin_lock(&d->event_lock);
> +
>       /* Guest cannot re-bind a Xen-attached event channel. */
>       if ( unlikely(consumer_is_xen(chn)) )
>       {
> 

Cheers,

-- 
Julien Grall

Re: [PATCH v6 1/3] evtchn: slightly defer lock acquire where possible

[Roger Pau Monné]

On Thu, May 27, 2021 at 07:48:41PM +0100, Julien Grall wrote:
> Hi Jan,
> 
> On 27/05/2021 12:28, Jan Beulich wrote:
> > port_is_valid() and evtchn_from_port() are fine to use without holding
> > any locks. Accordingly acquire the per-domain lock slightly later in
> > evtchn_close() and evtchn_bind_vcpu().
> 
> So I agree that port_is_valid() and evtchn_from_port() are fine to use
> without holding any locks in evtchn_bind_vcpu(). However, this is misleading
> to say there is no problem with evtchn_close().
> 
> evtchn_close() can be called with current != d and therefore, there is a

The only instances evtchn_close is called with current != d and the
domain could be unpaused is in free_xen_event_channel AFAICT.

> risk that port_is_valid() may be valid and then invalid because
> d->valid_evtchns is decremented in evtchn_destroy().

Hm, I guess you could indeed have parallel calls to
free_xen_event_channel and evtchn_destroy in a way that
free_xen_event_channel could race with valid_evtchns getting
decreased?

All callers of free_xen_event_channel are internal to the hypervisor,
so maybe with proper ordering of the operations this could be avoided,
albeit it's not easy to assert.

> Thankfully the memory is still there. So the current code is okayish and I
> could reluctantly accept this behavior to be spread. However, I don't think
> this should be left uncommented in both the code (maybe on top of
> port_is_valid()?) and the commit message.

Indeed, I think we need some expansion of the comment in port_is_valid
to clarify all this. I'm not sure I understand it properly myself when
it's fine to use port_is_valid without holding the per domain event
lock.

evtchn_close shouldn't be a very common operation anyway, so it
holding the per domain lock a bit longer doesn't seem that critical to
me anyway.

Thanks, Roger.

Re: [PATCH v6 1/3] evtchn: slightly defer lock acquire where possible

[Jan Beulich]

On 28.05.2021 10:30, Roger Pau Monné wrote:
> On Thu, May 27, 2021 at 07:48:41PM +0100, Julien Grall wrote:
>> On 27/05/2021 12:28, Jan Beulich wrote:
>>> port_is_valid() and evtchn_from_port() are fine to use without holding
>>> any locks. Accordingly acquire the per-domain lock slightly later in
>>> evtchn_close() and evtchn_bind_vcpu().
>>
>> So I agree that port_is_valid() and evtchn_from_port() are fine to use
>> without holding any locks in evtchn_bind_vcpu(). However, this is misleading
>> to say there is no problem with evtchn_close().
>>
>> evtchn_close() can be called with current != d and therefore, there is a
> 
> The only instances evtchn_close is called with current != d and the
> domain could be unpaused is in free_xen_event_channel AFAICT.

As long as the domain is not paused, ->valid_evtchns can't ever
decrease: The only point where this gets done is in evtchn_destroy().
Hence ...

>> risk that port_is_valid() may be valid and then invalid because
>> d->valid_evtchns is decremented in evtchn_destroy().
> 
> Hm, I guess you could indeed have parallel calls to
> free_xen_event_channel and evtchn_destroy in a way that
> free_xen_event_channel could race with valid_evtchns getting
> decreased?

... I don't see this as relevant.

>> Thankfully the memory is still there. So the current code is okayish and I
>> could reluctantly accept this behavior to be spread. However, I don't think
>> this should be left uncommented in both the code (maybe on top of
>> port_is_valid()?) and the commit message.
> 
> Indeed, I think we need some expansion of the comment in port_is_valid
> to clarify all this. I'm not sure I understand it properly myself when
> it's fine to use port_is_valid without holding the per domain event
> lock.

Because of the above property plus the fact that even if
->valid_evtchns decreases, the underlying struct evtchn instance
will remain valid (i.e. won't get de-allocated, which happens only
in evtchn_destroy_final()), it is always fine to use it without
lock. With this I'm having trouble seeing what would need adding
to port_is_valid()'s commentary.

The only thing which shouldn't happen anywhere is following a
port_is_valid() check which has returned false by code assuming
the port is going to remain invalid. But I think that's obvious
when you don't hold any suitable lock.

I do intend to follow Julien's request to explain things more for
evtchn_close().

Jan

Re: [PATCH v6 1/3] evtchn: slightly defer lock acquire where possible

[Julien Grall]

Hi Jan,

On 28/05/2021 11:23, Jan Beulich wrote:
> On 28.05.2021 10:30, Roger Pau Monné wrote:
>> On Thu, May 27, 2021 at 07:48:41PM +0100, Julien Grall wrote:
>>> On 27/05/2021 12:28, Jan Beulich wrote:
>>>> port_is_valid() and evtchn_from_port() are fine to use without holding
>>>> any locks. Accordingly acquire the per-domain lock slightly later in
>>>> evtchn_close() and evtchn_bind_vcpu().
>>>
>>> So I agree that port_is_valid() and evtchn_from_port() are fine to use
>>> without holding any locks in evtchn_bind_vcpu(). However, this is misleading
>>> to say there is no problem with evtchn_close().
>>>
>>> evtchn_close() can be called with current != d and therefore, there is a
>>
>> The only instances evtchn_close is called with current != d and the
>> domain could be unpaused is in free_xen_event_channel AFAICT.
> 
> As long as the domain is not paused, ->valid_evtchns can't ever
> decrease: The only point where this gets done is in evtchn_destroy().
> Hence ...
> 
>>> risk that port_is_valid() may be valid and then invalid because
>>> d->valid_evtchns is decremented in evtchn_destroy().
>>
>> Hm, I guess you could indeed have parallel calls to
>> free_xen_event_channel and evtchn_destroy in a way that
>> free_xen_event_channel could race with valid_evtchns getting
>> decreased?
> 
> ... I don't see this as relevant.
> 
>>> Thankfully the memory is still there. So the current code is okayish and I
>>> could reluctantly accept this behavior to be spread. However, I don't think
>>> this should be left uncommented in both the code (maybe on top of
>>> port_is_valid()?) and the commit message.
>>
>> Indeed, I think we need some expansion of the comment in port_is_valid
>> to clarify all this. I'm not sure I understand it properly myself when
>> it's fine to use port_is_valid without holding the per domain event
>> lock.
> 
> Because of the above property plus the fact that even if
> ->valid_evtchns decreases, the underlying struct evtchn instance
> will remain valid (i.e. won't get de-allocated, which happens only
> in evtchn_destroy_final()), it is always fine to use it without
> lock. With this I'm having trouble seeing what would need adding
> to port_is_valid()'s commentary.

Lets take the example of free_xen_event_channel(). The function is 
checking if the port is valid. If it is, then evtchn_close() will be called.

At this point, it would be fair for a developper to assume that 
port_is_valid() will also return true in event_close().

To push to the extreme, if free_xen_event_channel() was the only caller 
of evtchn_close(), one could argue that the check in evtchn_close() 
could be a BUG_ON().

However, this can't be because this would sooner or later turn to an XSA.

Effectively, it means that is_port_valid() *cannot* be used in an 
ASSERT()/BUG_ON() and every caller should check the return even if the 
port was previously validated.

So I think a comment on top of is_port_valid() would be really useful 
before someone rediscover it the hard way.

Cheers,

-- 
Julien Grall

Re: [PATCH v6 1/3] evtchn: slightly defer lock acquire where possible

[Roger Pau Monné]

On Fri, May 28, 2021 at 11:48:51AM +0100, Julien Grall wrote:
> Hi Jan,
> 
> On 28/05/2021 11:23, Jan Beulich wrote:
> > On 28.05.2021 10:30, Roger Pau Monné wrote:
> > > On Thu, May 27, 2021 at 07:48:41PM +0100, Julien Grall wrote:
> > > > On 27/05/2021 12:28, Jan Beulich wrote:
> > > > > port_is_valid() and evtchn_from_port() are fine to use without holding
> > > > > any locks. Accordingly acquire the per-domain lock slightly later in
> > > > > evtchn_close() and evtchn_bind_vcpu().
> > > > 
> > > > So I agree that port_is_valid() and evtchn_from_port() are fine to use
> > > > without holding any locks in evtchn_bind_vcpu(). However, this is misleading
> > > > to say there is no problem with evtchn_close().
> > > > 
> > > > evtchn_close() can be called with current != d and therefore, there is a
> > > 
> > > The only instances evtchn_close is called with current != d and the
> > > domain could be unpaused is in free_xen_event_channel AFAICT.
> > 
> > As long as the domain is not paused, ->valid_evtchns can't ever
> > decrease: The only point where this gets done is in evtchn_destroy().
> > Hence ...
> > 
> > > > risk that port_is_valid() may be valid and then invalid because
> > > > d->valid_evtchns is decremented in evtchn_destroy().
> > > 
> > > Hm, I guess you could indeed have parallel calls to
> > > free_xen_event_channel and evtchn_destroy in a way that
> > > free_xen_event_channel could race with valid_evtchns getting
> > > decreased?
> > 
> > ... I don't see this as relevant.
> > 
> > > > Thankfully the memory is still there. So the current code is okayish and I
> > > > could reluctantly accept this behavior to be spread. However, I don't think
> > > > this should be left uncommented in both the code (maybe on top of
> > > > port_is_valid()?) and the commit message.
> > > 
> > > Indeed, I think we need some expansion of the comment in port_is_valid
> > > to clarify all this. I'm not sure I understand it properly myself when
> > > it's fine to use port_is_valid without holding the per domain event
> > > lock.
> > 
> > Because of the above property plus the fact that even if
> > ->valid_evtchns decreases, the underlying struct evtchn instance
> > will remain valid (i.e. won't get de-allocated, which happens only
> > in evtchn_destroy_final()), it is always fine to use it without
> > lock. With this I'm having trouble seeing what would need adding
> > to port_is_valid()'s commentary.
> 
> Lets take the example of free_xen_event_channel(). The function is checking
> if the port is valid. If it is, then evtchn_close() will be called.
> 
> At this point, it would be fair for a developper to assume that
> port_is_valid() will also return true in event_close().
> 
> To push to the extreme, if free_xen_event_channel() was the only caller of
> evtchn_close(), one could argue that the check in evtchn_close() could be a
> BUG_ON().
> 
> However, this can't be because this would sooner or later turn to an XSA.
> 
> Effectively, it means that is_port_valid() *cannot* be used in an
> ASSERT()/BUG_ON() and every caller should check the return even if the port
> was previously validated.

We already have cases of is_port_valid being used in ASSERTs (in the
shim) and a BUG_ON (with the domain event lock held in evtchn_close).

> So I think a comment on top of is_port_valid() would be really useful before
> someone rediscover it the hard way.

I think I'm being extremely dull here, sorry. From your text I
understand that the value returned by is_port_valid could be stale by
the time the user reads it?

I think there's some condition that makes this value stale, and it's
not the common case?

Thanks, Roger.

Re: [PATCH v6 1/3] evtchn: slightly defer lock acquire where possible

[Jan Beulich]

On 28.05.2021 15:31, Roger Pau Monné wrote:
> I think I'm being extremely dull here, sorry. From your text I
> understand that the value returned by is_port_valid could be stale by
> the time the user reads it?
> 
> I think there's some condition that makes this value stale, and it's
> not the common case?

It's evtchn_destroy() running in parallel which can have this effect.

Jan

Re: [PATCH v6 1/3] evtchn: slightly defer lock acquire where possible

[Julien Grall]

Hi Roger,

On 28/05/2021 14:31, Roger Pau Monné wrote:
> On Fri, May 28, 2021 at 11:48:51AM +0100, Julien Grall wrote:
>> Hi Jan,
>>
>> On 28/05/2021 11:23, Jan Beulich wrote:
>>> On 28.05.2021 10:30, Roger Pau Monné wrote:
>>>> On Thu, May 27, 2021 at 07:48:41PM +0100, Julien Grall wrote:
>>>>> On 27/05/2021 12:28, Jan Beulich wrote:
>>>>>> port_is_valid() and evtchn_from_port() are fine to use without holding
>>>>>> any locks. Accordingly acquire the per-domain lock slightly later in
>>>>>> evtchn_close() and evtchn_bind_vcpu().
>>>>>
>>>>> So I agree that port_is_valid() and evtchn_from_port() are fine to use
>>>>> without holding any locks in evtchn_bind_vcpu(). However, this is misleading
>>>>> to say there is no problem with evtchn_close().
>>>>>
>>>>> evtchn_close() can be called with current != d and therefore, there is a
>>>>
>>>> The only instances evtchn_close is called with current != d and the
>>>> domain could be unpaused is in free_xen_event_channel AFAICT.
>>>
>>> As long as the domain is not paused, ->valid_evtchns can't ever
>>> decrease: The only point where this gets done is in evtchn_destroy().
>>> Hence ...
>>>
>>>>> risk that port_is_valid() may be valid and then invalid because
>>>>> d->valid_evtchns is decremented in evtchn_destroy().
>>>>
>>>> Hm, I guess you could indeed have parallel calls to
>>>> free_xen_event_channel and evtchn_destroy in a way that
>>>> free_xen_event_channel could race with valid_evtchns getting
>>>> decreased?
>>>
>>> ... I don't see this as relevant.
>>>
>>>>> Thankfully the memory is still there. So the current code is okayish and I
>>>>> could reluctantly accept this behavior to be spread. However, I don't think
>>>>> this should be left uncommented in both the code (maybe on top of
>>>>> port_is_valid()?) and the commit message.
>>>>
>>>> Indeed, I think we need some expansion of the comment in port_is_valid
>>>> to clarify all this. I'm not sure I understand it properly myself when
>>>> it's fine to use port_is_valid without holding the per domain event
>>>> lock.
>>>
>>> Because of the above property plus the fact that even if
>>> ->valid_evtchns decreases, the underlying struct evtchn instance
>>> will remain valid (i.e. won't get de-allocated, which happens only
>>> in evtchn_destroy_final()), it is always fine to use it without
>>> lock. With this I'm having trouble seeing what would need adding
>>> to port_is_valid()'s commentary.
>>
>> Lets take the example of free_xen_event_channel(). The function is checking
>> if the port is valid. If it is, then evtchn_close() will be called.
>>
>> At this point, it would be fair for a developper to assume that
>> port_is_valid() will also return true in event_close().
>>
>> To push to the extreme, if free_xen_event_channel() was the only caller of
>> evtchn_close(), one could argue that the check in evtchn_close() could be a
>> BUG_ON().
>>
>> However, this can't be because this would sooner or later turn to an XSA.
>>
>> Effectively, it means that is_port_valid() *cannot* be used in an
>> ASSERT()/BUG_ON() and every caller should check the return even if the port
>> was previously validated.
> 
> We already have cases of is_port_valid being used in ASSERTs (in the
> shim) and a BUG_ON (with the domain event lock held in evtchn_close).

I was likely a bit too restrictive in my remark. The BUG_ON() in 
evtchn_close() is fine because this is used on a (in theory) an open 
port with the both domains event lock held.

This would be more a problem if we have either:

if ( !port_is_valid(d1, port1) )
   return -EINVAL;

/* .... */

BUG_ON(port_is_valid(d1, port1));

> 
>> So I think a comment on top of is_port_valid() would be really useful before
>> someone rediscover it the hard way.
> 
> I think I'm being extremely dull here, sorry. From your text I
> understand that the value returned by is_port_valid could be stale by
> the time the user reads it?
> 
> I think there's some condition that makes this value stale, and it's
> not the common case?

It is any code that race with evtchn_destroy().

Cheers,

-- 
Julien Grall

Re: [PATCH v6 1/3] evtchn: slightly defer lock acquire where possible

[Jan Beulich]

On 27.05.2021 20:48, Julien Grall wrote:
> On 27/05/2021 12:28, Jan Beulich wrote:
>> port_is_valid() and evtchn_from_port() are fine to use without holding
>> any locks. Accordingly acquire the per-domain lock slightly later in
>> evtchn_close() and evtchn_bind_vcpu().
> 
> So I agree that port_is_valid() and evtchn_from_port() are fine to use 
> without holding any locks in evtchn_bind_vcpu(). However, this is 
> misleading to say there is no problem with evtchn_close().
> 
> evtchn_close() can be called with current != d and therefore, there is a 
> risk that port_is_valid() may be valid and then invalid because 
> d->valid_evtchns is decremented in evtchn_destroy().

While this is the case for other functions as well (and hence a
comment along the lines of what you ask for below should have
been in place already), I've added

/*
 * While calling the function is okay without holding a suitable lock yet
 * (see the comment ahead of struct evtchn_port_ops for which ones those
 * are), for a dying domain it may start returning false at any point - see
 * evtchn_destroy(). This is not a fundamental problem though, as the
 * struct evtchn instance won't disappear (and will continue to hold valid
 * data) until final cleanup of the domain, at which point the domain itself
 * cannot be looked up anymore and hence calls here can't occur anymore in
 * the first place.
 */

...

> Thankfully the memory is still there. So the current code is okayish and 
> I could reluctantly accept this behavior to be spread. However, I don't 
> think this should be left uncommented in both the code (maybe on top of 
> port_is_valid()?) and the commit message.

... ahead of port_is_valid() (and not, as I did intend originally,
in evtchn_close()). As far as the commit message goes, I'll have it
refer to the comment only.

I hope this satisfies the requests of both of you. I'll take the
liberty and retain your ack, Roger.

Jan

Re: [PATCH v6 1/3] evtchn: slightly defer lock acquire where possible

[Julien Grall]

Hi Jan,

On 01/06/2021 12:54, Jan Beulich wrote:
> On 27.05.2021 20:48, Julien Grall wrote:
>> On 27/05/2021 12:28, Jan Beulich wrote:
>>> port_is_valid() and evtchn_from_port() are fine to use without holding
>>> any locks. Accordingly acquire the per-domain lock slightly later in
>>> evtchn_close() and evtchn_bind_vcpu().
>>
>> So I agree that port_is_valid() and evtchn_from_port() are fine to use
>> without holding any locks in evtchn_bind_vcpu(). However, this is
>> misleading to say there is no problem with evtchn_close().
>>
>> evtchn_close() can be called with current != d and therefore, there is a
>> risk that port_is_valid() may be valid and then invalid because
>> d->valid_evtchns is decremented in evtchn_destroy().
> 
> While this is the case for other functions as well (and hence a
> comment along the lines of what you ask for below should have
> been in place already), I've added
> 
> /*
>   * While calling the function is okay without holding a suitable lock yet
>   * (see the comment ahead of struct evtchn_port_ops for which ones those
>   * are), for a dying domain it may start returning false at any point - see
>   * evtchn_destroy(). This is not a fundamental problem though, as the
>   * struct evtchn instance won't disappear (and will continue to hold valid
>   * data) until final cleanup of the domain, at which point the domain itself
>   * cannot be looked up anymore and hence calls here can't occur anymore in
>   * the first place.
>   */
> 
> ...
> 
>> Thankfully the memory is still there. So the current code is okayish and
>> I could reluctantly accept this behavior to be spread. However, I don't
>> think this should be left uncommented in both the code (maybe on top of
>> port_is_valid()?) and the commit message.
> 
> ... ahead of port_is_valid() (and not, as I did intend originally,
> in evtchn_close()). As far as the commit message goes, I'll have it
> refer to the comment only.
> 
> I hope this satisfies the requests of both of you. I'll take the
> liberty and retain your ack, Roger.

Yes, this satistfies my requests. Feel free to add my reviewed-by on the 
patch.

Cheers,

-- 
Julien Grall