[PATCH v2 0/3] xenfb bug fixes and cleanup

[PATCH v2 0/3] xenfb bug fixes and cleanup

[Wei Liu]

Wei Liu (3):
  xenfb: use the correct condition to avoid excessive looping
  xenfb: move xen_rmb to the correct location
  xenfb: remove out_cons in xenfb_handle_events

 hw/display/xenfb.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v2 1/3] xenfb: use the correct condition to avoid excessive looping

[Wei Liu]

In commit ac0487e1 ("xenfb.c: avoid expensive loops when prod <=
out_cons"), ">=" was used. In fact, a full ring is a legit state.
Correct the test to use ">".

Reported-by: "Hao, Xudong" <xudong.hao@intel.com>
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Tested-by: "Hao, Xudong" <xudong.hao@intel.com>
Acked-by: Anthony Perard <anthony.perard@citrix.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>

Backport candidate to our own tree.
---
 hw/display/xenfb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
index 40b096a..9866dfd 100644
--- a/hw/display/xenfb.c
+++ b/hw/display/xenfb.c
@@ -775,7 +775,7 @@ static void xenfb_handle_events(struct XenFB *xenfb)
 
     prod = page->out_prod;
     out_cons = page->out_cons;
-    if (prod - out_cons >= XENFB_OUT_RING_LEN) {
+    if (prod - out_cons > XENFB_OUT_RING_LEN) {
         return;
     }
     xen_rmb();		/* ensure we see ring contents up to prod */
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v2 2/3] xenfb: move xen_rmb to the correct location

[Wei Liu]

It should be placed before first time producer and consumer are used.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>

Backport candidate to our own tree.
---
 hw/display/xenfb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
index 9866dfd..7f4fad7 100644
--- a/hw/display/xenfb.c
+++ b/hw/display/xenfb.c
@@ -775,10 +775,10 @@ static void xenfb_handle_events(struct XenFB *xenfb)
 
     prod = page->out_prod;
     out_cons = page->out_cons;
+    xen_rmb();
     if (prod - out_cons > XENFB_OUT_RING_LEN) {
         return;
     }
-    xen_rmb();		/* ensure we see ring contents up to prod */
     for (cons = out_cons; cons != prod; cons++) {
 	union xenfb_out_event *event = &XENFB_OUT_RING_REF(page, cons);
         uint8_t type = event->type;
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 2/3] xenfb: move xen_rmb to the correct location

[David Vrabel]

On 12/04/16 11:43, Wei Liu wrote:
> It should be placed before first time producer and consumer are used.

This change isn't necessary and is confusing as this is not what this
barrier is for.

The barrier needs to be between the load of prod and the load of the
ring contents (there's even a comment that says this).  This pairs with
the corresponding write barrier between the store of the ring contents
and the store of prod (in the other end).

David

> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> 
> Backport candidate to our own tree.
> ---
>  hw/display/xenfb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> index 9866dfd..7f4fad7 100644
> --- a/hw/display/xenfb.c
> +++ b/hw/display/xenfb.c
> @@ -775,10 +775,10 @@ static void xenfb_handle_events(struct XenFB *xenfb)
>  
>      prod = page->out_prod;
>      out_cons = page->out_cons;
> +    xen_rmb();
>      if (prod - out_cons > XENFB_OUT_RING_LEN) {
>          return;
>      }
> -    xen_rmb();		/* ensure we see ring contents up to prod */
>      for (cons = out_cons; cons != prod; cons++) {
>  	union xenfb_out_event *event = &XENFB_OUT_RING_REF(page, cons);
>          uint8_t type = event->type;
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 2/3] xenfb: move xen_rmb to the correct location

[Andrew Cooper]

On 12/04/16 13:57, David Vrabel wrote:
> On 12/04/16 11:43, Wei Liu wrote:
>> It should be placed before first time producer and consumer are used.
> This change isn't necessary and is confusing as this is not what this
> barrier is for.
>
> The barrier needs to be between the load of prod and the load of the
> ring contents (there's even a comment that says this).  This pairs with
> the corresponding write barrier between the store of the ring contents
> and the store of prod (in the other end).

Looking further, this code will compile to multiple reads of the page,
because there is no ACCESS_ONCE().  This code is still vulnerable to
XSA-155.

~Andrew

>
> David
>
>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>> ---
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Anthony Perard <anthony.perard@citrix.com>
>>
>> Backport candidate to our own tree.
>> ---
>>  hw/display/xenfb.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
>> index 9866dfd..7f4fad7 100644
>> --- a/hw/display/xenfb.c
>> +++ b/hw/display/xenfb.c
>> @@ -775,10 +775,10 @@ static void xenfb_handle_events(struct XenFB *xenfb)
>>  
>>      prod = page->out_prod;
>>      out_cons = page->out_cons;
>> +    xen_rmb();
>>      if (prod - out_cons > XENFB_OUT_RING_LEN) {
>>          return;
>>      }
>> -    xen_rmb();		/* ensure we see ring contents up to prod */
>>      for (cons = out_cons; cons != prod; cons++) {
>>  	union xenfb_out_event *event = &XENFB_OUT_RING_REF(page, cons);
>>          uint8_t type = event->type;
>>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 2/3] xenfb: move xen_rmb to the correct location

[Wei Liu]

On Tue, Apr 12, 2016 at 02:38:13PM +0100, Andrew Cooper wrote:
> On 12/04/16 13:57, David Vrabel wrote:
> > On 12/04/16 11:43, Wei Liu wrote:
> >> It should be placed before first time producer and consumer are used.
> > This change isn't necessary and is confusing as this is not what this
> > barrier is for.
> >
> > The barrier needs to be between the load of prod and the load of the
> > ring contents (there's even a comment that says this).  This pairs with
> > the corresponding write barrier between the store of the ring contents
> > and the store of prod (in the other end).
> 
> Looking further, this code will compile to multiple reads of the page,
> because there is no ACCESS_ONCE().  This code is still vulnerable to
> XSA-155.
> 

Oops, accidentally kicked over a can of worms. Should have just sent
patch 1. :-)

Jokes aside, more time is needed to fix this properly. So maybe we
should just upstream patch #1 first. Stefano? Anthony?

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 2/3] xenfb: move xen_rmb to the correct location

[Stefano Stabellini]

On Tue, 12 Apr 2016, Wei Liu wrote:
> On Tue, Apr 12, 2016 at 02:38:13PM +0100, Andrew Cooper wrote:
> > On 12/04/16 13:57, David Vrabel wrote:
> > > On 12/04/16 11:43, Wei Liu wrote:
> > >> It should be placed before first time producer and consumer are used.
> > > This change isn't necessary and is confusing as this is not what this
> > > barrier is for.
> > >
> > > The barrier needs to be between the load of prod and the load of the
> > > ring contents (there's even a comment that says this).  This pairs with
> > > the corresponding write barrier between the store of the ring contents
> > > and the store of prod (in the other end).
> > 
> > Looking further, this code will compile to multiple reads of the page,
> > because there is no ACCESS_ONCE().  This code is still vulnerable to
> > XSA-155.

There is no ACCESS_ONCE in QEMU, the closest thing to it is atomic_read.


> Oops, accidentally kicked over a can of worms. Should have just sent
> patch 1. :-)
> 
> Jokes aside, more time is needed to fix this properly. So maybe we
> should just upstream patch #1 first. Stefano? Anthony?

Sure

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v2 3/3] xenfb: remove out_cons in xenfb_handle_events

[Wei Liu]

The variable out_cons was only used to temporarily hold the consumer
index. Use cons directly to simplify code a bit.

No functional change introduced.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
---
 hw/display/xenfb.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
index 7f4fad7..7d50efb 100644
--- a/hw/display/xenfb.c
+++ b/hw/display/xenfb.c
@@ -770,16 +770,16 @@ static void xenfb_invalidate(void *opaque)
 
 static void xenfb_handle_events(struct XenFB *xenfb)
 {
-    uint32_t prod, cons, out_cons;
+    uint32_t prod, cons;
     struct xenfb_page *page = xenfb->c.page;
 
     prod = page->out_prod;
-    out_cons = page->out_cons;
+    cons = page->out_cons;
     xen_rmb();
-    if (prod - out_cons > XENFB_OUT_RING_LEN) {
+    if (prod - cons > XENFB_OUT_RING_LEN) {
         return;
     }
-    for (cons = out_cons; cons != prod; cons++) {
+    for ( ; cons != prod; cons++) {
 	union xenfb_out_event *event = &XENFB_OUT_RING_REF(page, cons);
         uint8_t type = event->type;
 	int x, y, w, h;
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 1/3] xenfb: use the correct condition to avoid excessive looping

[Stefano Stabellini]

On Tue, 12 Apr 2016, Wei Liu wrote:
> In commit ac0487e1 ("xenfb.c: avoid expensive loops when prod <=
> out_cons"), ">=" was used. In fact, a full ring is a legit state.
> Correct the test to use ">".
> 
> Reported-by: "Hao, Xudong" <xudong.hao@intel.com>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Tested-by: "Hao, Xudong" <xudong.hao@intel.com>
> Acked-by: Anthony Perard <anthony.perard@citrix.com>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

I'll add it to my queue


> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> 
> Backport candidate to our own tree.
> ---
>  hw/display/xenfb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> index 40b096a..9866dfd 100644
> --- a/hw/display/xenfb.c
> +++ b/hw/display/xenfb.c
> @@ -775,7 +775,7 @@ static void xenfb_handle_events(struct XenFB *xenfb)
>  
>      prod = page->out_prod;
>      out_cons = page->out_cons;
> -    if (prod - out_cons >= XENFB_OUT_RING_LEN) {
> +    if (prod - out_cons > XENFB_OUT_RING_LEN) {
>          return;
>      }
>      xen_rmb();		/* ensure we see ring contents up to prod */
> -- 
> 2.1.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 1/3] xenfb: use the correct condition to avoid excessive looping

[Anthony PERARD]

On Tue, Apr 12, 2016 at 10:12:28AM -0700, Stefano Stabellini wrote:
> On Tue, 12 Apr 2016, Wei Liu wrote:
> > In commit ac0487e1 ("xenfb.c: avoid expensive loops when prod <=
> > out_cons"), ">=" was used. In fact, a full ring is a legit state.
> > Correct the test to use ">".
> > 
> > Reported-by: "Hao, Xudong" <xudong.hao@intel.com>
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > Tested-by: "Hao, Xudong" <xudong.hao@intel.com>
> > Acked-by: Anthony Perard <anthony.perard@citrix.com>
> 
> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> I'll add it to my queue
> 
> 
> > Cc: Stefano Stabellini <sstabellini@kernel.org>
> > Cc: Anthony Perard <anthony.perard@citrix.com>
> > 
> > Backport candidate to our own tree.

Patch backported to qemu-xen.git:master.

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 1/3] xenfb: use the correct condition to avoid excessive looping

[Wei Liu]

On Fri, Apr 22, 2016 at 12:16:09PM +0100, Anthony PERARD wrote:
> On Tue, Apr 12, 2016 at 10:12:28AM -0700, Stefano Stabellini wrote:
> > On Tue, 12 Apr 2016, Wei Liu wrote:
> > > In commit ac0487e1 ("xenfb.c: avoid expensive loops when prod <=
> > > out_cons"), ">=" was used. In fact, a full ring is a legit state.
> > > Correct the test to use ">".
> > > 
> > > Reported-by: "Hao, Xudong" <xudong.hao@intel.com>
> > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > Tested-by: "Hao, Xudong" <xudong.hao@intel.com>
> > > Acked-by: Anthony Perard <anthony.perard@citrix.com>
> > 
> > Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > I'll add it to my queue
> > 
> > 
> > > Cc: Stefano Stabellini <sstabellini@kernel.org>
> > > Cc: Anthony Perard <anthony.perard@citrix.com>
> > > 
> > > Backport candidate to our own tree.
> 
> Patch backported to qemu-xen.git:master.
> 

I think you mean staging. :-)

Wei.

> -- 
> Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 3/3] xenfb: remove out_cons in xenfb_handle_events

[Stefano Stabellini]

On Tue, 12 Apr 2016, Wei Liu wrote:
> The variable out_cons was only used to temporarily hold the consumer
> index. Use cons directly to simplify code a bit.
> 
> No functional change introduced.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Except for the fact that it is based on patch #2, which is wrong, this
looks OK.

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


> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> ---
>  hw/display/xenfb.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> index 7f4fad7..7d50efb 100644
> --- a/hw/display/xenfb.c
> +++ b/hw/display/xenfb.c
> @@ -770,16 +770,16 @@ static void xenfb_invalidate(void *opaque)
>  
>  static void xenfb_handle_events(struct XenFB *xenfb)
>  {
> -    uint32_t prod, cons, out_cons;
> +    uint32_t prod, cons;
>      struct xenfb_page *page = xenfb->c.page;
>  
>      prod = page->out_prod;
> -    out_cons = page->out_cons;
> +    cons = page->out_cons;
>      xen_rmb();
> -    if (prod - out_cons > XENFB_OUT_RING_LEN) {
> +    if (prod - cons > XENFB_OUT_RING_LEN) {
>          return;
>      }
> -    for (cons = out_cons; cons != prod; cons++) {
> +    for ( ; cons != prod; cons++) {
>  	union xenfb_out_event *event = &XENFB_OUT_RING_REF(page, cons);
>          uint8_t type = event->type;
>  	int x, y, w, h;
> -- 
> 2.1.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel