[Xen-devel] [PATCH v3 2/2] argo: correctly report pending message length

[Xen-devel] [PATCH v3 2/2] argo: correctly report pending message length

[Nicholas Tsirakis]

When a message is requeue'd in Xen's internal queue, the queue
entry contains the length of the message so that Xen knows to
send a VIRQ to the respective domain when enough space frees up
in the ring. Due to a small bug, however, Xen doesn't populate
the length of the msg if a given write fails, so this length is
always reported as zero. This causes Xen to spurriously wake up
a domain even when the ring doesn't have enough space.

This patch makes sure that the msg len is properly reported by
populating it in the event of a write failure.

Signed-off-by: Nicholas Tsirakis <tsirakisn@ainfosec.com>
---
 xen/common/argo.c | 39 +++++++++++++++++++--------------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/xen/common/argo.c b/xen/common/argo.c
index 2f874a570d..c8f4302963 100644
--- a/xen/common/argo.c
+++ b/xen/common/argo.c
@@ -765,27 +765,20 @@ iov_count(const xen_argo_iov_t *piov, unsigned int niov,
 static int
 ringbuf_insert(const struct domain *d, struct argo_ring_info *ring_info,
                const struct argo_ring_id *src_id, xen_argo_iov_t *iovs,
-               unsigned int niov, uint32_t message_type,
-               unsigned long *out_len)
+               unsigned int niov, uint32_t message_type, unsigned int len)
 {
     xen_argo_ring_t ring;
     struct xen_argo_ring_message_header mh = { };
     int sp, ret;
-    unsigned int len = 0;
     xen_argo_iov_t *piov;
     XEN_GUEST_HANDLE(uint8) NULL_hnd = { };
 
     ASSERT(LOCKING_L3(d, ring_info));
 
     /*
-     * Obtain the total size of data to transmit -- sets the 'len' variable
-     * -- and sanity check that the iovs conform to size and number limits.
      * Enforced below: no more than 'len' bytes of guest data
      * (plus the message header) will be sent in this operation.
      */
-    ret = iov_count(iovs, niov, &len);
-    if ( ret )
-        return ret;
 
     /*
      * Upper bound check the message len against the ring size.
@@ -983,8 +976,6 @@ ringbuf_insert(const struct domain *d, struct argo_ring_info *ring_info,
      * versus performance cost could be added to decide that here.
      */
 
-    *out_len = len;
-
     return ret;
 }
 
@@ -1976,7 +1967,7 @@ sendv(struct domain *src_d, xen_argo_addr_t *src_addr,
     struct argo_ring_id src_id;
     struct argo_ring_info *ring_info;
     int ret = 0;
-    unsigned long len = 0;
+    unsigned int len = 0;
 
     argo_dprintk("sendv: (%u:%x)->(%u:%x) niov:%u type:%x\n",
                  src_addr->domain_id, src_addr->aport, dst_addr->domain_id,
@@ -2044,17 +2035,25 @@ sendv(struct domain *src_d, xen_argo_addr_t *src_addr,
     {
         spin_lock(&ring_info->L3_lock);
 
-        ret = ringbuf_insert(dst_d, ring_info, &src_id, iovs, niov,
-                             message_type, &len);
-        if ( ret == -EAGAIN )
+        /*
+         * Obtain the total size of data to transmit -- sets the 'len' variable
+         * -- and sanity check that the iovs conform to size and number limits.
+         */
+        ret = iov_count(iovs, niov, &len);
+        if ( !ret )
         {
-            int rc;
+            ret = ringbuf_insert(dst_d, ring_info, &src_id, iovs, niov,
+                                 message_type, len);
+            if ( ret == -EAGAIN )
+            {
+                int rc;
 
-            argo_dprintk("argo_ringbuf_sendv failed, EAGAIN\n");
-            /* requeue to issue a notification when space is there */
-            rc = pending_requeue(dst_d, ring_info, src_id.domain_id, len);
-            if ( rc )
-                ret = rc;
+                argo_dprintk("argo_ringbuf_sendv failed, EAGAIN\n");
+                /* requeue to issue a notification when space is there */
+                rc = pending_requeue(dst_d, ring_info, src_id.domain_id, len);
+                if ( rc )
+                    ret = rc;
+            }
         }
 
         spin_unlock(&ring_info->L3_lock);
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/2] argo: correctly report pending message length

[Andrew Cooper]

On 12/06/2019 13:34, Nicholas Tsirakis wrote:
> When a message is requeue'd in Xen's internal queue, the queue
> entry contains the length of the message so that Xen knows to
> send a VIRQ to the respective domain when enough space frees up
> in the ring. Due to a small bug, however, Xen doesn't populate
> the length of the msg if a given write fails, so this length is
> always reported as zero. This causes Xen to spurriously wake up
> a domain even when the ring doesn't have enough space.
>
> This patch makes sure that the msg len is properly reported by
> populating it in the event of a write failure.
>
> Signed-off-by: Nicholas Tsirakis <tsirakisn@ainfosec.com>
> ---

Just as a note for future patches, it is expected to have a changelog
here so reviewers can see what has changed from the previous version.

As this is mainly between you and Chris, its probably fine for now, but
https://lists.xenproject.org/archives/html/xen-devel/2019-06/msg00765.html
is a good recent example.  It is freeform text, and as long as its clear
to follow, it should be fine however you chose to format it.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/2] argo: correctly report pending message length

[Nicholas Tsirakis]

On Wed, Jun 12, 2019 at 8:47 AM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 12/06/2019 13:34, Nicholas Tsirakis wrote:
> > When a message is requeue'd in Xen's internal queue, the queue
> > entry contains the length of the message so that Xen knows to
> > send a VIRQ to the respective domain when enough space frees up
> > in the ring. Due to a small bug, however, Xen doesn't populate
> > the length of the msg if a given write fails, so this length is
> > always reported as zero. This causes Xen to spurriously wake up
> > a domain even when the ring doesn't have enough space.
> >
> > This patch makes sure that the msg len is properly reported by
> > populating it in the event of a write failure.
> >
> > Signed-off-by: Nicholas Tsirakis <tsirakisn@ainfosec.com>
> > ---
>
> Just as a note for future patches, it is expected to have a changelog
> here so reviewers can see what has changed from the previous version.
>
> As this is mainly between you and Chris, its probably fine for now, but
> https://lists.xenproject.org/archives/html/xen-devel/2019-06/msg00765.html
> is a good recent example.  It is freeform text, and as long as its clear
> to follow, it should be fine however you chose to format it.
>
> ~Andrew

Thank you for the link. I will make sure to do so in the future.

Adding the changelog here for reference:
v3:
- Only run ringbuf_insert() block if iov_count() succeeded. Do nothing
otherwise.

v2:
- Move iov_count() out of ringbuf_insert() and into sendv().
- Pass len from iov_count() into ringbuf_insert().
- Change len var in sendv() from unsigned long to unsigned int.

--Niko

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/2] argo: correctly report pending message length

[Christopher Clark]

On Wed, Jun 12, 2019 at 5:35 AM Nicholas Tsirakis
<niko.tsirakis@gmail.com> wrote:
>
> When a message is requeue'd in Xen's internal queue, the queue
> entry contains the length of the message so that Xen knows to
> send a VIRQ to the respective domain when enough space frees up
> in the ring. Due to a small bug, however, Xen doesn't populate
> the length of the msg if a given write fails, so this length is
> always reported as zero. This causes Xen to spurriously wake up

I should've caught this earlier and it would be nice to fix on commit:
only a single R in "spuriously"

> a domain even when the ring doesn't have enough space.
>
> This patch makes sure that the msg len is properly reported by
> populating it in the event of a write failure.
>
> Signed-off-by: Nicholas Tsirakis <tsirakisn@ainfosec.com>
Reviewed-by: Christopher Clark <christopher.w.clark@gmail.com>

> ---
>  xen/common/argo.c | 39 +++++++++++++++++++--------------------
>  1 file changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/xen/common/argo.c b/xen/common/argo.c
> index 2f874a570d..c8f4302963 100644
> --- a/xen/common/argo.c
> +++ b/xen/common/argo.c
> @@ -765,27 +765,20 @@ iov_count(const xen_argo_iov_t *piov, unsigned int niov,
>  static int
>  ringbuf_insert(const struct domain *d, struct argo_ring_info *ring_info,
>                 const struct argo_ring_id *src_id, xen_argo_iov_t *iovs,
> -               unsigned int niov, uint32_t message_type,
> -               unsigned long *out_len)
> +               unsigned int niov, uint32_t message_type, unsigned int len)
>  {
>      xen_argo_ring_t ring;
>      struct xen_argo_ring_message_header mh = { };
>      int sp, ret;
> -    unsigned int len = 0;
>      xen_argo_iov_t *piov;
>      XEN_GUEST_HANDLE(uint8) NULL_hnd = { };
>
>      ASSERT(LOCKING_L3(d, ring_info));
>
>      /*
> -     * Obtain the total size of data to transmit -- sets the 'len' variable
> -     * -- and sanity check that the iovs conform to size and number limits.
>       * Enforced below: no more than 'len' bytes of guest data
>       * (plus the message header) will be sent in this operation.
>       */
> -    ret = iov_count(iovs, niov, &len);
> -    if ( ret )
> -        return ret;
>
>      /*
>       * Upper bound check the message len against the ring size.
> @@ -983,8 +976,6 @@ ringbuf_insert(const struct domain *d, struct argo_ring_info *ring_info,
>       * versus performance cost could be added to decide that here.
>       */
>
> -    *out_len = len;
> -
>      return ret;
>  }
>
> @@ -1976,7 +1967,7 @@ sendv(struct domain *src_d, xen_argo_addr_t *src_addr,
>      struct argo_ring_id src_id;
>      struct argo_ring_info *ring_info;
>      int ret = 0;
> -    unsigned long len = 0;
> +    unsigned int len = 0;
>
>      argo_dprintk("sendv: (%u:%x)->(%u:%x) niov:%u type:%x\n",
>                   src_addr->domain_id, src_addr->aport, dst_addr->domain_id,
> @@ -2044,17 +2035,25 @@ sendv(struct domain *src_d, xen_argo_addr_t *src_addr,
>      {
>          spin_lock(&ring_info->L3_lock);
>
> -        ret = ringbuf_insert(dst_d, ring_info, &src_id, iovs, niov,
> -                             message_type, &len);
> -        if ( ret == -EAGAIN )
> +        /*
> +         * Obtain the total size of data to transmit -- sets the 'len' variable
> +         * -- and sanity check that the iovs conform to size and number limits.
> +         */
> +        ret = iov_count(iovs, niov, &len);
> +        if ( !ret )
>          {
> -            int rc;
> +            ret = ringbuf_insert(dst_d, ring_info, &src_id, iovs, niov,
> +                                 message_type, len);
> +            if ( ret == -EAGAIN )
> +            {
> +                int rc;
>
> -            argo_dprintk("argo_ringbuf_sendv failed, EAGAIN\n");
> -            /* requeue to issue a notification when space is there */
> -            rc = pending_requeue(dst_d, ring_info, src_id.domain_id, len);
> -            if ( rc )
> -                ret = rc;
> +                argo_dprintk("argo_ringbuf_sendv failed, EAGAIN\n");
> +                /* requeue to issue a notification when space is there */
> +                rc = pending_requeue(dst_d, ring_info, src_id.domain_id, len);
> +                if ( rc )
> +                    ret = rc;
> +            }
>          }
>
>          spin_unlock(&ring_info->L3_lock);
> --
> 2.17.1
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/2] argo: correctly report pending message length

[Andrew Cooper]

On 12/06/2019 20:58, Christopher Clark wrote:
> On Wed, Jun 12, 2019 at 5:35 AM Nicholas Tsirakis
> <niko.tsirakis@gmail.com> wrote:
>> When a message is requeue'd in Xen's internal queue, the queue
>> entry contains the length of the message so that Xen knows to
>> send a VIRQ to the respective domain when enough space frees up
>> in the ring. Due to a small bug, however, Xen doesn't populate
>> the length of the msg if a given write fails, so this length is
>> always reported as zero. This causes Xen to spurriously wake up
> I should've caught this earlier and it would be nice to fix on commit:
> only a single R in "spuriously"
>
>> a domain even when the ring doesn't have enough space.
>>
>> This patch makes sure that the msg len is properly reported by
>> populating it in the event of a write failure.
>>
>> Signed-off-by: Nicholas Tsirakis <tsirakisn@ainfosec.com>
> Reviewed-by: Christopher Clark <christopher.w.clark@gmail.com>

Typo fixed and committed.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel