[PATCH 1/3] xen/blkback: Don't trust the handle from the frontend.

[PATCH 1/3] xen/blkback: Don't trust the handle from the frontend.

[Konrad Rzeszutek Wilk]

The 'handle' is the device that the request is from. For the life-time
of the ring we copy it from a request to a response so that the frontend
is not surprised by it. But we do not need it - when we start processing
I/Os we have our own 'struct phys_req' which has only most essential
information about the request. In fact the 'vbd_translate' ends up
over-writing the preq.dev with a value from the backend.

This assignment of preq.dev with the 'handle' value is superfluous
so lets not do it.

Cc: stable@vger.kernel.org
Acked-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/block/xen-blkback/blkback.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
index 742871d..2de3da9 100644
--- a/drivers/block/xen-blkback/blkback.c
+++ b/drivers/block/xen-blkback/blkback.c
@@ -890,7 +890,6 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 		goto fail_response;
 	}
 
-	preq.dev           = req->u.rw.handle;
 	preq.sector_number = req->u.rw.sector_number;
 	preq.nr_sects      = 0;
 
-- 
1.8.0.2

Re: [Xen-devel] [PATCH 1/3] xen/blkback: Don't trust the handle from the frontend.

[Jan Beulich]

>>> On 25.01.13 at 18:32, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> The 'handle' is the device that the request is from. For the life-time
> of the ring we copy it from a request to a response so that the frontend
> is not surprised by it. But we do not need it - when we start processing
> I/Os we have our own 'struct phys_req' which has only most essential
> information about the request. In fact the 'vbd_translate' ends up
> over-writing the preq.dev with a value from the backend.
> 
> This assignment of preq.dev with the 'handle' value is superfluous
> so lets not do it.
> 
> Cc: stable@vger.kernel.org 

Is this really stable material? It fixes nothing but the (wrong)
impression that the driver might be using frontend provided data
without checking.

Jan

> Acked-by: Jan Beulich <jbeulich@suse.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  drivers/block/xen-blkback/blkback.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/block/xen-blkback/blkback.c 
> b/drivers/block/xen-blkback/blkback.c
> index 742871d..2de3da9 100644
> --- a/drivers/block/xen-blkback/blkback.c
> +++ b/drivers/block/xen-blkback/blkback.c
> @@ -890,7 +890,6 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
>  		goto fail_response;
>  	}
>  
> -	preq.dev           = req->u.rw.handle;
>  	preq.sector_number = req->u.rw.sector_number;
>  	preq.nr_sects      = 0;
>  
> -- 
> 1.8.0.2
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 

[PATCH 2/3] xen/ring: Add a new macro to detect whether there is an overflow in requests and response.

[Konrad Rzeszutek Wilk]

We want to be able to exit if the difference between the request
produced (what the frontend tells us) and the requests consumed
(what we have so far processed) is greater than the ring size.

If so, we should terminate the loop as the request produced
is not trusted and it means it is bogus.

Cc: stable@vger.kernel.org
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 include/xen/interface/io/ring.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/xen/interface/io/ring.h b/include/xen/interface/io/ring.h
index 75271b9..1114dde 100644
--- a/include/xen/interface/io/ring.h
+++ b/include/xen/interface/io/ring.h
@@ -188,6 +188,12 @@ struct __name##_back_ring {						\
 #define RING_REQUEST_CONS_OVERFLOW(_r, _cons)				\
     (((_cons) - (_r)->rsp_prod_pvt) >= RING_SIZE(_r))
 
+/* Loop termination condition: Is the difference between request produced
+ * and request consumed greater than the ring size. If so, terminate the
+ * loop. */
+#define RING_REQUEST_PROD_OVERFLOW(_r, _cons, _prod)                     \
+    (((_prod) - (_prod)) > RING_SIZE(_r))
+
 #define RING_PUSH_REQUESTS(_r) do {					\
     wmb(); /* back sees requests /before/ updated producer index */	\
     (_r)->sring->req_prod = (_r)->req_prod_pvt;				\
-- 
1.8.0.2

Re: [Xen-devel] [PATCH 2/3] xen/ring: Add a new macro to detect whether there is an overflow in requests and response.

[Roger Pau Monné]

On 25/01/13 18:32, Konrad Rzeszutek Wilk wrote:
> We want to be able to exit if the difference between the request
> produced (what the frontend tells us) and the requests consumed
> (what we have so far processed) is greater than the ring size.
> 
> If so, we should terminate the loop as the request produced
> is not trusted and it means it is bogus.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  include/xen/interface/io/ring.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/xen/interface/io/ring.h b/include/xen/interface/io/ring.h
> index 75271b9..1114dde 100644
> --- a/include/xen/interface/io/ring.h
> +++ b/include/xen/interface/io/ring.h
> @@ -188,6 +188,12 @@ struct __name##_back_ring {						\
>  #define RING_REQUEST_CONS_OVERFLOW(_r, _cons)				\
>      (((_cons) - (_r)->rsp_prod_pvt) >= RING_SIZE(_r))
>  
> +/* Loop termination condition: Is the difference between request produced
> + * and request consumed greater than the ring size. If so, terminate the
> + * loop. */
> +#define RING_REQUEST_PROD_OVERFLOW(_r, _cons, _prod)                     \
> +    (((_prod) - (_prod)) > RING_SIZE(_r))

Shouldn't the second _prod be _cons?

> +
>  #define RING_PUSH_REQUESTS(_r) do {					\
>      wmb(); /* back sees requests /before/ updated producer index */	\
>      (_r)->sring->req_prod = (_r)->req_prod_pvt;				\
> 

Re: [Xen-devel] [PATCH 2/3] xen/ring: Add a new macro to detect whether there is an overflow in requests and response.

[Konrad Rzeszutek Wilk]

On Fri, Jan 25, 2013 at 06:45:29PM +0100, Roger Pau Monn� wrote:
> On 25/01/13 18:32, Konrad Rzeszutek Wilk wrote:
> > We want to be able to exit if the difference between the request
> > produced (what the frontend tells us) and the requests consumed
> > (what we have so far processed) is greater than the ring size.
> > 
> > If so, we should terminate the loop as the request produced
> > is not trusted and it means it is bogus.
> > 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  include/xen/interface/io/ring.h | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/include/xen/interface/io/ring.h b/include/xen/interface/io/ring.h
> > index 75271b9..1114dde 100644
> > --- a/include/xen/interface/io/ring.h
> > +++ b/include/xen/interface/io/ring.h
> > @@ -188,6 +188,12 @@ struct __name##_back_ring {						\
> >  #define RING_REQUEST_CONS_OVERFLOW(_r, _cons)				\
> >      (((_cons) - (_r)->rsp_prod_pvt) >= RING_SIZE(_r))
> >  
> > +/* Loop termination condition: Is the difference between request produced
> > + * and request consumed greater than the ring size. If so, terminate the
> > + * loop. */
> > +#define RING_REQUEST_PROD_OVERFLOW(_r, _cons, _prod)                     \
> > +    (((_prod) - (_prod)) > RING_SIZE(_r))
> 
> Shouldn't the second _prod be _cons?

Yes :-) Was just testing everybody :-)

> 
> > +
> >  #define RING_PUSH_REQUESTS(_r) do {					\
> >      wmb(); /* back sees requests /before/ updated producer index */	\
> >      (_r)->sring->req_prod = (_r)->req_prod_pvt;				\
> > 
> 

Re: [Xen-devel] [PATCH 2/3] xen/ring: Add a new macro to detect whether there is an overflow in requests and response.

[Konrad Rzeszutek Wilk]

On Fri, Jan 25, 2013 at 06:45:29PM +0100, Roger Pau Monn� wrote:
> On 25/01/13 18:32, Konrad Rzeszutek Wilk wrote:
> > We want to be able to exit if the difference between the request
> > produced (what the frontend tells us) and the requests consumed
> > (what we have so far processed) is greater than the ring size.
> > 
> > If so, we should terminate the loop as the request produced
> > is not trusted and it means it is bogus.
> > 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  include/xen/interface/io/ring.h | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/include/xen/interface/io/ring.h b/include/xen/interface/io/ring.h
> > index 75271b9..1114dde 100644
> > --- a/include/xen/interface/io/ring.h
> > +++ b/include/xen/interface/io/ring.h
> > @@ -188,6 +188,12 @@ struct __name##_back_ring {						\
> >  #define RING_REQUEST_CONS_OVERFLOW(_r, _cons)				\
> >      (((_cons) - (_r)->rsp_prod_pvt) >= RING_SIZE(_r))
> >  
> > +/* Loop termination condition: Is the difference between request produced
> > + * and request consumed greater than the ring size. If so, terminate the
> > + * loop. */
> > +#define RING_REQUEST_PROD_OVERFLOW(_r, _cons, _prod)                     \
> > +    (((_prod) - (_prod)) > RING_SIZE(_r))
> 
> Shouldn't the second _prod be _cons?
> 
> > +
> >  #define RING_PUSH_REQUESTS(_r) do {					\
> >      wmb(); /* back sees requests /before/ updated producer index */	\
> >      (_r)->sring->req_prod = (_r)->req_prod_pvt;				\
> > 
> 

Here is an update of it:

>From a84ea207716486515f083a6d708414dd3ad3aaf6 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Fri, 25 Jan 2013 12:29:51 -0500
Subject: [PATCH] ring/macro: Add a new macro to detect whether there is an
 overflow in requests and response.

We want to be able to exit if the difference between the request
produced (what the frontend tells us) and the requests consumed
(what we have so far processed) is greater than the ring size.

If so, we should terminate the loop as the request produced
is not trusted and it means it is bogus.

[v1:  Make the macro do the ring thing, swap arguments around to
match the comment]
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/include/public/io/ring.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/xen/include/public/io/ring.h b/xen/include/public/io/ring.h
index 38b7051..440ef9a 100644
--- a/xen/include/public/io/ring.h
+++ b/xen/include/public/io/ring.h
@@ -234,6 +234,12 @@ typedef struct __name##_back_ring __name##_back_ring_t
 #define RING_REQUEST_CONS_OVERFLOW(_r, _cons)                           \
     (((_cons) - (_r)->rsp_prod_pvt) >= RING_SIZE(_r))
 
+/* Loop termination condition: Is the difference between request produced
+ * and request consumed greater than the ring size. If so, terminate the
+ * loop. */
+#define RING_REQUEST_PROD_OVERFLOW(_r, _prod, _cons)                    \
+    (((_prod) - (_cons)) > RING_SIZE(_r))
+
 #define RING_PUSH_REQUESTS(_r) do {                                     \
     xen_wmb(); /* back sees requests /before/ updated producer index */ \
     (_r)->sring->req_prod = (_r)->req_prod_pvt;                         \
-- 
1.8.0.2

Re: [PATCH 2/3] xen/ring: Add a new macro to detect whether there is an overflow in requests and response.

[Konrad Rzeszutek Wilk]

On Fri, Jan 25, 2013 at 12:32:31PM -0500, Konrad Rzeszutek Wilk wrote:
> We want to be able to exit if the difference between the request
> produced (what the frontend tells us) and the requests consumed
> (what we have so far processed) is greater than the ring size.
> 
> If so, we should terminate the loop as the request produced
> is not trusted and it means it is bogus.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  include/xen/interface/io/ring.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/xen/interface/io/ring.h b/include/xen/interface/io/ring.h
> index 75271b9..1114dde 100644
> --- a/include/xen/interface/io/ring.h
> +++ b/include/xen/interface/io/ring.h
> @@ -188,6 +188,12 @@ struct __name##_back_ring {						\
>  #define RING_REQUEST_CONS_OVERFLOW(_r, _cons)				\
>      (((_cons) - (_r)->rsp_prod_pvt) >= RING_SIZE(_r))
>  
> +/* Loop termination condition: Is the difference between request produced
> + * and request consumed greater than the ring size. If so, terminate the
> + * loop. */
> +#define RING_REQUEST_PROD_OVERFLOW(_r, _cons, _prod)                     \
> +    (((_prod) - (_prod)) > RING_SIZE(_r))
> +
>  #define RING_PUSH_REQUESTS(_r) do {					\
>      wmb(); /* back sees requests /before/ updated producer index */	\
>      (_r)->sring->req_prod = (_r)->req_prod_pvt;				\
> -- 
> 1.8.0.2
>

Per Roger sharp eyes:
 
>From 567782db60a5e410924751fb7c2e021095720b93 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Wed, 23 Jan 2013 16:53:54 -0500
Subject: [PATCH 1/2] xen/ring: Add a new macro to detect whether there is an
 overflow in requests and response.

We want to be able to exit if the difference between the request
produced (what the frontend tells us) and the requests consumed
(what we have so far processed) is greater than the ring size.

If so, we should terminate the loop as the request produced
is not trusted and it means it is bogus.

Cc: stable@vger.kernel.org
[v1: Make the macro do the ring thing, swap arguments around to
match the comment]
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 include/xen/interface/io/ring.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/xen/interface/io/ring.h b/include/xen/interface/io/ring.h
index 75271b9..94aa885 100644
--- a/include/xen/interface/io/ring.h
+++ b/include/xen/interface/io/ring.h
@@ -188,6 +188,12 @@ struct __name##_back_ring {						\
 #define RING_REQUEST_CONS_OVERFLOW(_r, _cons)				\
     (((_cons) - (_r)->rsp_prod_pvt) >= RING_SIZE(_r))
 
+/* Loop termination condition: Is the difference between request produced
+ * and request consumed greater than the ring size. If so, terminate the
+ * loop. */
+#define RING_REQUEST_PROD_OVERFLOW(_r, _prod, _cons)                    \
+    (((_prod) - (_cons)) > RING_SIZE(_r))
+
 #define RING_PUSH_REQUESTS(_r) do {					\
     wmb(); /* back sees requests /before/ updated producer index */	\
     (_r)->sring->req_prod = (_r)->req_prod_pvt;				\
-- 
1.8.0.2

[PATCH 3/3] xen/blkback: Check for insane amounts of request on the ring.

[Konrad Rzeszutek Wilk]

Check that the ring does not have an insane amount of requests
(more than there could fit on the ring).

If we detect this case we will stop processing the requests
and wait until the XenBus disconnects the ring.

Looking at the code, one would expect that the existing check
RING_REQUEST_CONS_OVERFLOW which checks for how many responses we
have created in the past (rsp_prod_pvt) vs requests consumed (req_cons)
and that difference between is greater or equal to the size of the
ring. If that condition is satisfied that means we should not be
processing more requests as we still have a backlog of responses
to finish. Note that both of those values (rsp_prod_pvt and req_cons)
are not exposed on the shared ring.

To understand this problem a mini crash course in ring protocol
response/request updates.

There are four entries: req_prod and rsp_prod; req_event and rsp_event.
We are only concerned about the first two - which set the tone of
this bug. The req_prod is a value incremented by frontend for each
request put on the ring. Conversely the rsp_prod is a value incremented
by the backend for each response put on the ring. Both values can
wrap and are modulo the size of the ring (in block case that is 32).
Look in RING_GET_REQUEST and RING_GET_RESPONSE for the more details.

The culprit here is that if the difference between the
req_prod and req_cons is greater than the ring size we have a problem.
Fortunately for us, the '__do_block_io_op' loop:

	rc = blk_rings->common.req_cons;
	rp = blk_rings->common.sring->req_prod;

	while (rc != rp) {

		..
		blk_rings->common.req_cons = ++rc; /* before make_response() */

	}

will loop up to the point when rc == rp. The macros inside of the
loop (RING_GET_REQUEST) is smart and is indexing based on the modulo
of the ring size. If the frontend has provided a bogus req_prod value
we will loop here until the 'rc == rp' - so we could be processing
already processed requests (or responses) often.

The reason the RING_REQUEST_CONS_OVERFLOW is not helping here is
b/c it only tracks how many responses we have internally produced
and whether we would should process more.

For example, if we were to enter this function with these values:

       	blk_rings->common.sring->req_prod =  X+31415 (X is the value from
		the last time __do_block_io_op was called).
        blk_rings->common.req_cons = X
        blk_rings->common.rsp_prod_pvt = X

The RING_REQUEST_CONS_OVERFLOW is doing:

	req_cons - sring->rsp_prod_pvt >= 32

Which is,
	X - X >= 32 or 0 >= 32

And that is false, so we continue on looping (this bug).

The new macro (RING_REQUEST_PROD_OVERFLOW) will help us determine this
condition, and checks the difference between request consumed and
request produced and whether that difference is greater than the ring size.

	req_cons - sring->req.prod > 32

	X - X+31415 > 32

And that is true, so we break out of the loop.

Cc: stable@vger.kernel.org
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/block/xen-blkback/blkback.c | 11 +++++++++--
 drivers/block/xen-blkback/common.h  |  2 ++
 drivers/block/xen-blkback/xenbus.c  |  2 ++
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
index 2de3da9..4e9b028 100644
--- a/drivers/block/xen-blkback/blkback.c
+++ b/drivers/block/xen-blkback/blkback.c
@@ -395,7 +395,7 @@ int xen_blkif_schedule(void *arg)
 {
 	struct xen_blkif *blkif = arg;
 	struct xen_vbd *vbd = &blkif->vbd;
-
+	int rc = 0;
 	xen_blkif_get(blkif);
 
 	while (!kthread_should_stop()) {
@@ -415,8 +415,12 @@ int xen_blkif_schedule(void *arg)
 		blkif->waiting_reqs = 0;
 		smp_mb(); /* clear flag *before* checking for work */
 
-		if (do_block_io_op(blkif))
+		rc = do_block_io_op(blkif);
+		if (rc > 0)
 			blkif->waiting_reqs = 1;
+		if (rc == -EACCES)
+			wait_event_interruptible(blkif->shutdown_wq,
+						 kthread_should_stop());
 
 		if (log_stats && time_after(jiffies, blkif->st_print))
 			print_stats(blkif);
@@ -780,6 +784,9 @@ __do_block_io_op(struct xen_blkif *blkif)
 		if (RING_REQUEST_CONS_OVERFLOW(&blk_rings->common, rc))
 			break;
 
+		if (RING_REQUEST_PROD_OVERFLOW(&blk_rings->common, rc, rp))
+			return -EACCES;
+
 		if (kthread_should_stop()) {
 			more_to_do = 1;
 			break;
diff --git a/drivers/block/xen-blkback/common.h b/drivers/block/xen-blkback/common.h
index ae7951f..8fd211b 100644
--- a/drivers/block/xen-blkback/common.h
+++ b/drivers/block/xen-blkback/common.h
@@ -218,6 +218,8 @@ struct xen_blkif {
 	int			st_wr_sect;
 
 	wait_queue_head_t	waiting_to_free;
+	/* Thread shutdown wait queue. */
+	wait_queue_head_t	shutdown_wq;
 };
 
 
diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
index 60558a5..333ebe9 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -119,6 +119,7 @@ static struct xen_blkif *xen_blkif_alloc(domid_t domid)
 	blkif->st_print = jiffies;
 	init_waitqueue_head(&blkif->waiting_to_free);
 	blkif->persistent_gnts.rb_node = NULL;
+	init_waitqueue_head(&blkif->shutdown_wq);
 
 	return blkif;
 }
@@ -178,6 +179,7 @@ static int xen_blkif_map(struct xen_blkif *blkif, unsigned long shared_page,
 static void xen_blkif_disconnect(struct xen_blkif *blkif)
 {
 	if (blkif->xenblkd) {
+		wake_up(&blkif->shutdown_wq);
 		kthread_stop(blkif->xenblkd);
 		blkif->xenblkd = NULL;
 	}
-- 
1.8.0.2

Re: [PATCH 3/3] xen/blkback: Check for insane amounts of request on the ring.

[Konrad Rzeszutek Wilk]

On Fri, Jan 25, 2013 at 12:32:32PM -0500, Konrad Rzeszutek Wilk wrote:
> Check that the ring does not have an insane amount of requests
> (more than there could fit on the ring).
.. snip..
> diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
> index 2de3da9..4e9b028 100644
> --- a/drivers/block/xen-blkback/blkback.c
> +++ b/drivers/block/xen-blkback/blkback.c
> @@ -395,7 +395,7 @@ int xen_blkif_schedule(void *arg)
>  {
>  	struct xen_blkif *blkif = arg;
>  	struct xen_vbd *vbd = &blkif->vbd;
> -
> +	int rc = 0;
>  	xen_blkif_get(blkif);
>  
>  	while (!kthread_should_stop()) {
> @@ -415,8 +415,12 @@ int xen_blkif_schedule(void *arg)
>  		blkif->waiting_reqs = 0;
>  		smp_mb(); /* clear flag *before* checking for work */
>  
> -		if (do_block_io_op(blkif))
> +		rc = do_block_io_op(blkif);
> +		if (rc > 0)
>  			blkif->waiting_reqs = 1;
> +		if (rc == -EACCES)
> +			wait_event_interruptible(blkif->shutdown_wq,
> +						 kthread_should_stop());
>  
>  		if (log_stats && time_after(jiffies, blkif->st_print))
>  			print_stats(blkif);
> @@ -780,6 +784,9 @@ __do_block_io_op(struct xen_blkif *blkif)
>  		if (RING_REQUEST_CONS_OVERFLOW(&blk_rings->common, rc))
>  			break;
>  
> +		if (RING_REQUEST_PROD_OVERFLOW(&blk_rings->common, rc, rp))
> +			return -EACCES;
> +


With the new patch for RING_REQUEST_PROD_OVERFLOW I realized that the check
can actually be right before the loop, so:

>From c35dd7a41960038c5defbf8b40541a0d7f206d54 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Wed, 23 Jan 2013 16:54:32 -0500
Subject: [PATCH 2/2] xen/blkback: Check for insane amounts of request on the
 ring.

Check that the ring does not have an insane amount of requests
(more than there could fit on the ring).

If we detect this case we will stop processing the requests
and wait until the XenBus disconnects the ring.

Looking at the code, one would expect that the existing check
RING_REQUEST_CONS_OVERFLOW which checks for how many responses we
have created in the past (rsp_prod_pvt) vs requests consumed (req_cons)
and that difference between is greater or equal to the size of the
ring. If that condition is satisfied that means we should not be
processing more requests as we still have a backlog of responses
to finish. Note that both of those values (rsp_prod_pvt and req_cons)
are not exposed on the shared ring.

To understand this problem a mini crash course in ring protocol
response/request updates.

There are four entries: req_prod and rsp_prod; req_event and rsp_event.
We are only concerned about the first two - which set the tone of
this bug. The req_prod is a value incremented by frontend for each
request put on the ring. Conversely the rsp_prod is a value incremented
by the backend for each response put on the ring. Both values can
wrap and are modulo the size of the ring (in block case that is 32).
Look in RING_GET_REQUEST and RING_GET_RESPONSE for the more details.

The culprit here is that if the difference between the
req_prod and req_cons is greater than the ring size we have a problem.
Fortunately for us, the '__do_block_io_op' loop:

	rc = blk_rings->common.req_cons;
	rp = blk_rings->common.sring->req_prod;

	while (rc != rp) {

		..
		blk_rings->common.req_cons = ++rc; /* before make_response() */

	}

will loop up to the point when rc == rp. The macros inside of the
loop (RING_GET_REQUEST) is smart and is indexing based on the modulo
of the ring size. If the frontend has provided a bogus req_prod value
we will loop here until the 'rc == rp' - so we could be processing
already processed requests (or responses) often.

The reason the RING_REQUEST_CONS_OVERFLOW is not helping here is
b/c it only tracks how many responses we have internally produced
and whether we would should process more.

For example, if we were to enter this function with these values:

       	blk_rings->common.sring->req_prod =  X+31415 (X is the value from
		the last time __do_block_io_op was called).
        blk_rings->common.req_cons = X
        blk_rings->common.rsp_prod_pvt = X

The RING_REQUEST_CONS_OVERFLOW is doing:

	req_cons - sring->rsp_prod_pvt >= 32

Which is,
	X - X >= 32 or 0 >= 32

And that is false, so we continue on looping (this bug).

The new macro (RING_REQUEST_PROD_OVERFLOW) will help us determine this
condition, and checks the difference between request consumed and
request produced and whether that difference is greater than the ring size.

	req_cons - sring->req.prod > 32

	X - X+31415 > 32

And that is true, so we break out of the loop.

Cc: stable@vger.kernel.org
[v1: Move the check outside the loop]
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/block/xen-blkback/blkback.c | 11 +++++++++--
 drivers/block/xen-blkback/common.h  |  2 ++
 drivers/block/xen-blkback/xenbus.c  |  2 ++
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
index 1966a7c..cc9c48f 100644
--- a/drivers/block/xen-blkback/blkback.c
+++ b/drivers/block/xen-blkback/blkback.c
@@ -395,7 +395,7 @@ int xen_blkif_schedule(void *arg)
 {
 	struct xen_blkif *blkif = arg;
 	struct xen_vbd *vbd = &blkif->vbd;
-
+	int rc = 0;
 	xen_blkif_get(blkif);
 
 	while (!kthread_should_stop()) {
@@ -415,8 +415,12 @@ int xen_blkif_schedule(void *arg)
 		blkif->waiting_reqs = 0;
 		smp_mb(); /* clear flag *before* checking for work */
 
-		if (do_block_io_op(blkif))
+		rc = do_block_io_op(blkif);
+		if (rc > 0)
 			blkif->waiting_reqs = 1;
+		if (rc == -EACCES)
+			wait_event_interruptible(blkif->shutdown_wq,
+						 kthread_should_stop());
 
 		if (log_stats && time_after(jiffies, blkif->st_print))
 			print_stats(blkif);
@@ -764,6 +768,9 @@ __do_block_io_op(struct xen_blkif *blkif)
 	rp = blk_rings->common.sring->req_prod;
 	rmb(); /* Ensure we see queued requests up to 'rp'. */
 
+	if (RING_REQUEST_PROD_OVERFLOW(&blk_rings->common, rp, rc))
+		return -EACCES;
+
 	while (rc != rp) {
 
 		if (RING_REQUEST_CONS_OVERFLOW(&blk_rings->common, rc))
diff --git a/drivers/block/xen-blkback/common.h b/drivers/block/xen-blkback/common.h
index ae7951f..8fd211b 100644
--- a/drivers/block/xen-blkback/common.h
+++ b/drivers/block/xen-blkback/common.h
@@ -218,6 +218,8 @@ struct xen_blkif {
 	int			st_wr_sect;
 
 	wait_queue_head_t	waiting_to_free;
+	/* Thread shutdown wait queue. */
+	wait_queue_head_t	shutdown_wq;
 };
 
 
diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
index 60558a5..333ebe9 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -119,6 +119,7 @@ static struct xen_blkif *xen_blkif_alloc(domid_t domid)
 	blkif->st_print = jiffies;
 	init_waitqueue_head(&blkif->waiting_to_free);
 	blkif->persistent_gnts.rb_node = NULL;
+	init_waitqueue_head(&blkif->shutdown_wq);
 
 	return blkif;
 }
@@ -178,6 +179,7 @@ static int xen_blkif_map(struct xen_blkif *blkif, unsigned long shared_page,
 static void xen_blkif_disconnect(struct xen_blkif *blkif)
 {
 	if (blkif->xenblkd) {
+		wake_up(&blkif->shutdown_wq);
 		kthread_stop(blkif->xenblkd);
 		blkif->xenblkd = NULL;
 	}
-- 
1.8.0.2

Re: [Xen-devel] [PATCH 3/3] xen/blkback: Check for insane amounts of request on the ring.

[Jan Beulich]

>>> On 25.01.13 at 19:43, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> @@ -764,6 +768,9 @@ __do_block_io_op(struct xen_blkif *blkif)
>  	rp = blk_rings->common.sring->req_prod;
>  	rmb(); /* Ensure we see queued requests up to 'rp'. */
>  
> +	if (RING_REQUEST_PROD_OVERFLOW(&blk_rings->common, rp, rc))
> +		return -EACCES;

Actually I wonder whether we need the new macro at all: It seems
to me that using RING_REQUEST_CONS_OVERFLOW(&blk_rings->common, rp)
here would achieve the same effect.

Jan

> +
>  	while (rc != rp) {
>  
>  		if (RING_REQUEST_CONS_OVERFLOW(&blk_rings->common, rc))

Re: [Xen-devel] [PATCH 3/3] xen/blkback: Check for insane amounts of request on the ring.

[Konrad Rzeszutek Wilk]

On Mon, Jan 28, 2013 at 11:07:46AM +0000, Jan Beulich wrote:
> >>> On 25.01.13 at 19:43, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> > @@ -764,6 +768,9 @@ __do_block_io_op(struct xen_blkif *blkif)
> >  	rp = blk_rings->common.sring->req_prod;
> >  	rmb(); /* Ensure we see queued requests up to 'rp'. */
> >  
> > +	if (RING_REQUEST_PROD_OVERFLOW(&blk_rings->common, rp, rc))
> > +		return -EACCES;
> 
> Actually I wonder whether we need the new macro at all: It seems
> to me that using RING_REQUEST_CONS_OVERFLOW(&blk_rings->common, rp)
> here would achieve the same effect.

But it would not. The RING_REQUEST_CONS_OVERFLOW only check that the
non-shared ring entries (rsp_prod and rsp_prod_pvt) are less than
the size of the ring (32). In other words - they check whether we want
to process more requests as we still have a back-log of responses to
deal with.

This new macro would check for the req_prod being bogus and out of
bounds. Bounds being the difference between rsp_prod and req_prod
being bigger than ring (32).

> 
> Jan
> 
> > +
> >  	while (rc != rp) {
> >  
> >  		if (RING_REQUEST_CONS_OVERFLOW(&blk_rings->common, rc))
> 
> 

Re: [Xen-devel] [PATCH 3/3] xen/blkback: Check for insane amounts of request on the ring.

[Jan Beulich]

>>> On 28.01.13 at 16:42, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Mon, Jan 28, 2013 at 11:07:46AM +0000, Jan Beulich wrote:
>> >>> On 25.01.13 at 19:43, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
>> > @@ -764,6 +768,9 @@ __do_block_io_op(struct xen_blkif *blkif)
>> >  	rp = blk_rings->common.sring->req_prod;
>> >  	rmb(); /* Ensure we see queued requests up to 'rp'. */
>> >  
>> > +	if (RING_REQUEST_PROD_OVERFLOW(&blk_rings->common, rp, rc))
>> > +		return -EACCES;
>> 
>> Actually I wonder whether we need the new macro at all: It seems
>> to me that using RING_REQUEST_CONS_OVERFLOW(&blk_rings->common, rp)
>> here would achieve the same effect.
> 
> But it would not. The RING_REQUEST_CONS_OVERFLOW only check that the
> non-shared ring entries (rsp_prod and rsp_prod_pvt) are less than
> the size of the ring (32). In other words - they check whether we want
> to process more requests as we still have a back-log of responses to
> deal with.

So did you not notice that here 'rp' (i.e. req_prod) is being passed,
not 'rc'?

Jan

Re: [Xen-devel] [PATCH 3/3] xen/blkback: Check for insane amounts of request on the ring.

[Konrad Rzeszutek Wilk]

On Mon, Jan 28, 2013 at 04:18:59PM +0000, Jan Beulich wrote:
> >>> On 28.01.13 at 16:42, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> > On Mon, Jan 28, 2013 at 11:07:46AM +0000, Jan Beulich wrote:
> >> >>> On 25.01.13 at 19:43, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> >> > @@ -764,6 +768,9 @@ __do_block_io_op(struct xen_blkif *blkif)
> >> >  	rp = blk_rings->common.sring->req_prod;
> >> >  	rmb(); /* Ensure we see queued requests up to 'rp'. */
> >> >  
> >> > +	if (RING_REQUEST_PROD_OVERFLOW(&blk_rings->common, rp, rc))
> >> > +		return -EACCES;
> >> 
> >> Actually I wonder whether we need the new macro at all: It seems
> >> to me that using RING_REQUEST_CONS_OVERFLOW(&blk_rings->common, rp)
> >> here would achieve the same effect.
> > 
> > But it would not. The RING_REQUEST_CONS_OVERFLOW only check that the
> > non-shared ring entries (rsp_prod and rsp_prod_pvt) are less than
> > the size of the ring (32). In other words - they check whether we want
> > to process more requests as we still have a back-log of responses to
> > deal with.
> 
> So did you not notice that here 'rp' (i.e. req_prod) is being passed,
> not 'rc'?

Oh, no I did not :-(

That ought to do the trick too.

Re: [Xen-devel] [PATCH 3/3] xen/blkback: Check for insane amounts of request on the ring.

[David Vrabel]

On 25/01/13 18:43, Konrad Rzeszutek Wilk wrote:
> 
> Check that the ring does not have an insane amount of requests
> (more than there could fit on the ring).
[...]
> --- a/drivers/block/xen-blkback/blkback.c
> +++ b/drivers/block/xen-blkback/blkback.c
[...]
> @@ -415,8 +415,12 @@ int xen_blkif_schedule(void *arg)
>  		blkif->waiting_reqs = 0;
>  		smp_mb(); /* clear flag *before* checking for work */
>  
> -		if (do_block_io_op(blkif))
> +		rc = do_block_io_op(blkif);
> +		if (rc > 0)
>  			blkif->waiting_reqs = 1;
> +		if (rc == -EACCES)
> +			wait_event_interruptible(blkif->shutdown_wq,
> +						 kthread_should_stop());

So if this happens then the backend silently stops processing any
further requests?  I think it should print an error and disconnect from
the front end.

David

Re: [Xen-devel] [PATCH 3/3] xen/blkback: Check for insane amounts of request on the ring.

[Konrad Rzeszutek Wilk]

On Mon, Jan 28, 2013 at 11:38:50AM +0000, David Vrabel wrote:
> On 25/01/13 18:43, Konrad Rzeszutek Wilk wrote:
> > 
> > Check that the ring does not have an insane amount of requests
> > (more than there could fit on the ring).
> [...]
> > --- a/drivers/block/xen-blkback/blkback.c
> > +++ b/drivers/block/xen-blkback/blkback.c
> [...]
> > @@ -415,8 +415,12 @@ int xen_blkif_schedule(void *arg)
> >  		blkif->waiting_reqs = 0;
> >  		smp_mb(); /* clear flag *before* checking for work */
> >  
> > -		if (do_block_io_op(blkif))
> > +		rc = do_block_io_op(blkif);
> > +		if (rc > 0)
> >  			blkif->waiting_reqs = 1;
> > +		if (rc == -EACCES)
> > +			wait_event_interruptible(blkif->shutdown_wq,
> > +						 kthread_should_stop());
> 
> So if this happens then the backend silently stops processing any
> further requests?  I think it should print an error and disconnect from
> the front end.

I was not sure what it should print?

"Frontend provided bogus ring requests. Halting ring processing"?

It does eventually disconnect from the front-end as the XenBus enters
the Disconnect phase. It seemed the strike the balance - so that if
the frontend negotiates again the ring can start again and penalize
the frontend for giving bogus data.

> 
> David

Re: [Xen-devel] [PATCH 3/3] xen/blkback: Check for insane amounts of request on the ring.

[David Vrabel]

On 28/01/13 15:44, Konrad Rzeszutek Wilk wrote:
> On Mon, Jan 28, 2013 at 11:38:50AM +0000, David Vrabel wrote:
>> On 25/01/13 18:43, Konrad Rzeszutek Wilk wrote:
>>>
>>> Check that the ring does not have an insane amount of requests
>>> (more than there could fit on the ring).
>> [...]
>>> --- a/drivers/block/xen-blkback/blkback.c
>>> +++ b/drivers/block/xen-blkback/blkback.c
>> [...]
>>> @@ -415,8 +415,12 @@ int xen_blkif_schedule(void *arg)
>>>  		blkif->waiting_reqs = 0;
>>>  		smp_mb(); /* clear flag *before* checking for work */
>>>  
>>> -		if (do_block_io_op(blkif))
>>> +		rc = do_block_io_op(blkif);
>>> +		if (rc > 0)
>>>  			blkif->waiting_reqs = 1;
>>> +		if (rc == -EACCES)
>>> +			wait_event_interruptible(blkif->shutdown_wq,
>>> +						 kthread_should_stop());
>>
>> So if this happens then the backend silently stops processing any
>> further requests?  I think it should print an error and disconnect from
>> the front end.
> 
> I was not sure what it should print?
> 
> "Frontend provided bogus ring requests. Halting ring processing"?

That would be fine but you may want to print the bad values, in case
they're useful for tracking down bugs in frontends.

> It does eventually disconnect from the front-end as the XenBus enters
> the Disconnect phase. It seemed the strike the balance - so that if
> the frontend negotiates again the ring can start again and penalize
> the frontend for giving bogus data.

Ok.

David

Re: [Xen-devel] [PATCH 3/3] xen/blkback: Check for insane amounts of request on the ring.

[Konrad Rzeszutek Wilk]

On Mon, Jan 28, 2013 at 04:03:49PM +0000, David Vrabel wrote:
> On 28/01/13 15:44, Konrad Rzeszutek Wilk wrote:
> > On Mon, Jan 28, 2013 at 11:38:50AM +0000, David Vrabel wrote:
> >> On 25/01/13 18:43, Konrad Rzeszutek Wilk wrote:
> >>>
> >>> Check that the ring does not have an insane amount of requests
> >>> (more than there could fit on the ring).
> >> [...]
> >>> --- a/drivers/block/xen-blkback/blkback.c
> >>> +++ b/drivers/block/xen-blkback/blkback.c
> >> [...]
> >>> @@ -415,8 +415,12 @@ int xen_blkif_schedule(void *arg)
> >>>  		blkif->waiting_reqs = 0;
> >>>  		smp_mb(); /* clear flag *before* checking for work */
> >>>  
> >>> -		if (do_block_io_op(blkif))
> >>> +		rc = do_block_io_op(blkif);
> >>> +		if (rc > 0)
> >>>  			blkif->waiting_reqs = 1;
> >>> +		if (rc == -EACCES)
> >>> +			wait_event_interruptible(blkif->shutdown_wq,
> >>> +						 kthread_should_stop());
> >>
> >> So if this happens then the backend silently stops processing any
> >> further requests?  I think it should print an error and disconnect from
> >> the front end.
> > 
> > I was not sure what it should print?
> > 
> > "Frontend provided bogus ring requests. Halting ring processing"?
> 
> That would be fine but you may want to print the bad values, in case
> they're useful for tracking down bugs in frontends.

OK.
> 
> > It does eventually disconnect from the front-end as the XenBus enters
> > the Disconnect phase. It seemed the strike the balance - so that if
> > the frontend negotiates again the ring can start again and penalize
> > the frontend for giving bogus data.
> 
> Ok.
> 
> David