| Message ID | 7dafb0d7608ca1d245967382da5eb2e6cbe2c5b6.1560342869.git.tsirakisn@ainfosec.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
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
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
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 >
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
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);
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(-)