diff mbox

[v2,1/3] xen: fix quad word bufioreq handling

Message ID 58381B15020000780012226B@prv-mh.provo.novell.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jan Beulich Nov. 25, 2016, 10:05 a.m. UTC
We should not consume the second slot if it didn't get written yet.
Normal writers - i.e. Xen - would not update write_pointer between the
two writes, but the page may get fiddled with by the guest itself, and
we're better off avoiding to enter an infinite loop in that case.

Reported-by: yanghongke <yanghongke@huawei.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Bail (using hw_error()) instead of just breaking the loop.

Comments

Paul Durrant Nov. 25, 2016, 10:08 a.m. UTC | #1
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 25 November 2016 10:06
> To: qemu-devel@nongnu.org
> Cc: Anthony Perard <anthony.perard@citrix.com>; Paul Durrant
> <Paul.Durrant@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
> devel <xen-devel@lists.xenproject.org>
> Subject: [PATCH v2 1/3] xen: fix quad word bufioreq handling
> 
> We should not consume the second slot if it didn't get written yet.
> Normal writers - i.e. Xen - would not update write_pointer between the
> two writes, but the page may get fiddled with by the guest itself, and
> we're better off avoiding to enter an infinite loop in that case.
> 
> Reported-by: yanghongke <yanghongke@huawei.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Bail (using hw_error()) instead of just breaking the loop.

LGTM.

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> 
> --- a/xen-hvm.c
> +++ b/xen-hvm.c
> @@ -1021,6 +1021,9 @@ static int handle_buffered_iopage(XenIOS
>          xen_rmb();
>          qw = (req.size == 8);
>          if (qw) {
> +            if (rdptr + 1 == wrptr) {
> +                hw_error("Incomplete quad word buffered ioreq");
> +            }
>              buf_req = &buf_page->buf_ioreq[(rdptr + 1) %
>                                             IOREQ_BUFFER_SLOT_NUM];
>              req.data |= ((uint64_t)buf_req->data) << 32;
> 
>
Stefano Stabellini Nov. 28, 2016, 7:21 p.m. UTC | #2
On Fri, 25 Nov 2016, Jan Beulich wrote:
> We should not consume the second slot if it didn't get written yet.
> Normal writers - i.e. Xen - would not update write_pointer between the
> two writes, but the page may get fiddled with by the guest itself, and
> we're better off avoiding to enter an infinite loop in that case.
> 
> Reported-by: yanghongke <yanghongke@huawei.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> v2: Bail (using hw_error()) instead of just breaking the loop.
> 
> --- a/xen-hvm.c
> +++ b/xen-hvm.c
> @@ -1021,6 +1021,9 @@ static int handle_buffered_iopage(XenIOS
>          xen_rmb();
>          qw = (req.size == 8);
>          if (qw) {
> +            if (rdptr + 1 == wrptr) {
> +                hw_error("Incomplete quad word buffered ioreq");
> +            }
>              buf_req = &buf_page->buf_ioreq[(rdptr + 1) %
>                                             IOREQ_BUFFER_SLOT_NUM];
>              req.data |= ((uint64_t)buf_req->data) << 32;
> 
> 
>
diff mbox

Patch

--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -1021,6 +1021,9 @@  static int handle_buffered_iopage(XenIOS
         xen_rmb();
         qw = (req.size == 8);
         if (qw) {
+            if (rdptr + 1 == wrptr) {
+                hw_error("Incomplete quad word buffered ioreq");
+            }
             buf_req = &buf_page->buf_ioreq[(rdptr + 1) %
                                            IOREQ_BUFFER_SLOT_NUM];
             req.data |= ((uint64_t)buf_req->data) << 32;