[Xen-devel] [PATCH v2 1/2] argo: warn sendv() caller when ring is full

[Xen-devel] [PATCH v2 1/2] argo: warn sendv() caller when ring is full

[Nicholas Tsirakis]

In its current state, if the destination ring is full, sendv()
will requeue the message and return the rc of pending_requeue(),
which will return 0 on success. This prevents the caller from
distinguishing the difference between a successful write and a
message that needs to be resent at a later time.

Instead, capture the -EAGAIN value returned from ringbuf_insert()
and *only* overwrite it if the rc of pending_requeue() is non-zero.
This allows the caller to make intelligent decisions on -EAGAIN and
still be alerted if the pending message fails to requeue.

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

diff --git a/xen/common/argo.c b/xen/common/argo.c
index 13052b9239..2f874a570d 100644
--- a/xen/common/argo.c
+++ b/xen/common/argo.c
@@ -2048,9 +2048,13 @@ sendv(struct domain *src_d, xen_argo_addr_t *src_addr,
                              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 */
-            ret = pending_requeue(dst_d, ring_info, src_id.domain_id, len);
+            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

[Xen-devel] [PATCH v2 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 | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/xen/common/argo.c b/xen/common/argo.c
index 2f874a570d..31baf4beed 100644
--- a/xen/common/argo.c
+++ b/xen/common/argo.c
@@ -766,26 +766,20 @@ 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 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 +977,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 +1968,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,8 +2036,16 @@ sendv(struct domain *src_d, xen_argo_addr_t *src_addr,
     {
         spin_lock(&ring_info->L3_lock);
 
+        /*
+         * 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 )
+            return ret;
+
         ret = ringbuf_insert(dst_d, ring_info, &src_id, iovs, niov,
-                             message_type, &len);
+                             message_type, len);
         if ( ret == -EAGAIN )
         {
             int rc;
-- 
2.17.1


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

Re: [Xen-devel] [PATCH v2 1/2] argo: warn sendv() caller when ring is full

[Christopher Clark]

On Tue, Jun 11, 2019 at 2:14 PM Nicholas Tsirakis
<niko.tsirakis@gmail.com> wrote:
>
> In its current state, if the destination ring is full, sendv()
> will requeue the message and return the rc of pending_requeue(),
> which will return 0 on success. This prevents the caller from
> distinguishing the difference between a successful write and a
> message that needs to be resent at a later time.
>
> Instead, capture the -EAGAIN value returned from ringbuf_insert()
> and *only* overwrite it if the rc of pending_requeue() is non-zero.
> This allows the caller to make intelligent decisions on -EAGAIN and
> still be alerted if the pending message fails to requeue.
>
> Signed-off-by: Nicholas Tsirakis <tsirakisn@ainfosec.com>

This patch has already been approved and applied; so no need to resend
it again, thanks.

Christopher

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

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

[Christopher Clark]

On Tue, Jun 11, 2019 at 2:14 PM 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
> 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 | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/xen/common/argo.c b/xen/common/argo.c
> index 2f874a570d..31baf4beed 100644
> --- a/xen/common/argo.c
> +++ b/xen/common/argo.c
> @@ -766,26 +766,20 @@ 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 len)

The above can be reflowed, with the len argument on the same line as
niov and message_type without exceeding the maximum line length.

>  {
>      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 +977,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 +1968,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,8 +2036,16 @@ sendv(struct domain *src_d, xen_argo_addr_t *src_addr,
>      {
>          spin_lock(&ring_info->L3_lock);
>
> +        /*
> +         * 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 )
> +            return ret;

Returning at this point here would be bad as the L3_lock has been
taken above and would not be released.

Perhaps testing for !ret instead, and only if that is true performing
the insert logic up to the existing unlock, would be better.

> +
>          ret = ringbuf_insert(dst_d, ring_info, &src_id, iovs, niov,
> -                             message_type, &len);
> +                             message_type, len);
>          if ( ret == -EAGAIN )
>          {
>              int rc;

Christopher

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