* [Xen-devel] [PATCH v3 2/2] argo: correctly report pending message length
[not found] <fb83896f3b399c7ace3292f38506812bc4616f6d.1560342869.git.tsirakisn@ainfosec.com>
@ 2019-06-12 12:34 ` Nicholas Tsirakis
2019-06-12 12:47 ` Andrew Cooper
2019-06-12 19:58 ` Christopher Clark
0 siblings, 2 replies; 5+ messages in thread
From: Nicholas Tsirakis @ 2019-06-12 12:34 UTC (permalink / raw)
To: xen-devel; +Cc: Nicholas Tsirakis, Christopher Clark
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Xen-devel] [PATCH v3 2/2] argo: correctly report pending message length
2019-06-12 12:34 ` [Xen-devel] [PATCH v3 2/2] argo: correctly report pending message length Nicholas Tsirakis
@ 2019-06-12 12:47 ` Andrew Cooper
2019-06-12 13:42 ` Nicholas Tsirakis
2019-06-12 19:58 ` Christopher Clark
1 sibling, 1 reply; 5+ messages in thread
From: Andrew Cooper @ 2019-06-12 12:47 UTC (permalink / raw)
To: Nicholas Tsirakis, xen-devel; +Cc: Nicholas Tsirakis, Christopher Clark
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Xen-devel] [PATCH v3 2/2] argo: correctly report pending message length
2019-06-12 12:47 ` Andrew Cooper
@ 2019-06-12 13:42 ` Nicholas Tsirakis
0 siblings, 0 replies; 5+ messages in thread
From: Nicholas Tsirakis @ 2019-06-12 13:42 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Nicholas Tsirakis, Christopher Clark
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Xen-devel] [PATCH v3 2/2] argo: correctly report pending message length
2019-06-12 12:34 ` [Xen-devel] [PATCH v3 2/2] argo: correctly report pending message length Nicholas Tsirakis
2019-06-12 12:47 ` Andrew Cooper
@ 2019-06-12 19:58 ` Christopher Clark
2019-06-12 20:08 ` Andrew Cooper
1 sibling, 1 reply; 5+ messages in thread
From: Christopher Clark @ 2019-06-12 19:58 UTC (permalink / raw)
To: Nicholas Tsirakis; +Cc: xen-devel, Nicholas Tsirakis, Andrew Cooper
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Xen-devel] [PATCH v3 2/2] argo: correctly report pending message length
2019-06-12 19:58 ` Christopher Clark
@ 2019-06-12 20:08 ` Andrew Cooper
0 siblings, 0 replies; 5+ messages in thread
From: Andrew Cooper @ 2019-06-12 20:08 UTC (permalink / raw)
To: Christopher Clark, Nicholas Tsirakis; +Cc: xen-devel, Nicholas Tsirakis
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-06-12 20:09 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <fb83896f3b399c7ace3292f38506812bc4616f6d.1560342869.git.tsirakisn@ainfosec.com>
2019-06-12 12:34 ` [Xen-devel] [PATCH v3 2/2] argo: correctly report pending message length Nicholas Tsirakis
2019-06-12 12:47 ` Andrew Cooper
2019-06-12 13:42 ` Nicholas Tsirakis
2019-06-12 19:58 ` Christopher Clark
2019-06-12 20:08 ` Andrew Cooper
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.