Xen devel - Jun 2013 - [PATCH] 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 (rsp_prod gets set by rsp_prod_pvt when
pushing the responses on the ring).  Both values can
wrap and are modulo the size of the ring (in block case that is 32).
Please see 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 until the ''rc == rp'' - which means 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. The astute reader will
notice that the macro RING_REQUEST_CONS_OVERFLOW provides two
arguments - more on this later.

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(&blk_rings->common,
blk_rings->common.req_cons)
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).

If we re-use said macro RING_REQUEST_CONS_OVERFLOW and pass in the rp
instead (sring->req_prod) of rc, the this macro can do the check:

     req_prod - sring->rsp_prov_pvt >= 32

Which is,
       X + 31415 - X >= 32 , or 31415 >= 32

which is true, so we can error out and break out of the function.

Cc: stable@vger.kernel.org
[v1: Move the check outside the loop]
[v2: Add a pr_warn as suggested by David]
[v3: Use RING_REQUEST_CONS_OVERFLOW as suggested by Jan]
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/block/xen-blkback/blkback.c | 14 ++++++++++++--
 drivers/block/xen-blkback/common.h  |  2 ++
 drivers/block/xen-blkback/xenbus.c  |  2 ++
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/block/xen-blkback/blkback.c
b/drivers/block/xen-blkback/blkback.c
index d81dfca..c4b23be 100644
--- a/drivers/block/xen-blkback/blkback.c
+++ b/drivers/block/xen-blkback/blkback.c
@@ -397,7 +397,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()) {
@@ -417,8 +417,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);
@@ -778,6 +782,12 @@ __do_block_io_op(struct xen_blkif *blkif)
 	rp = blk_rings->common.sring->req_prod;
 	rmb(); /* Ensure we see queued requests up to ''rp''. */
 
+	/* N.B. ''rp'', not ''rc''. */
+	if (RING_REQUEST_CONS_OVERFLOW(&blk_rings->common, rp)) {
+		pr_warn(DRV_PFX "Frontend provided bogus ring requests (%d - %d = %d).
Halting ring processing on dev=%04x\n",
+			rp, rc, rp - rc, blkif->vbd.pdevice);
+		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 60103e2..43ef7f2 100644
--- a/drivers/block/xen-blkback/common.h
+++ b/drivers/block/xen-blkback/common.h
@@ -231,6 +231,8 @@ struct xen_blkif {
 	unsigned long long			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 8bfd1bc..00f2573 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -118,6 +118,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;
 }
@@ -177,6 +178,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.1.4

>>> On 04.06.13 at 21:57, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> 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
The "expect ..." here doesn''t really seem to have a proper
termination later in the sentence (or I''m having problems parsing
the whole construct), so if I didn''t know what the patch is about
I would have a hard time following.
> 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 (rsp_prod gets set by rsp_prod_pvt when
> pushing the responses on the ring).  Both values can
> wrap and are modulo the size of the ring (in block case that is 32).
> Please see 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 until the ''rc == rp'' - which means 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. The astute reader will
> notice that the macro RING_REQUEST_CONS_OVERFLOW provides two
> arguments - more on this later.
> 
> 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(&blk_rings->common,
blk_rings->common.req_cons)
> 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).
> 
> If we re-use said macro RING_REQUEST_CONS_OVERFLOW and pass in the rp
> instead (sring->req_prod) of rc, the this macro can do the check:
> 
>      req_prod - sring->rsp_prov_pvt >= 32
> 
> Which is,
>        X + 31415 - X >= 32 , or 31415 >= 32
> 
> which is true, so we can error out and break out of the function.
> 
> Cc: stable@vger.kernel.org 
> [v1: Move the check outside the loop]
> [v2: Add a pr_warn as suggested by David]
> [v3: Use RING_REQUEST_CONS_OVERFLOW as suggested by Jan]
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>

albeit with a minor nit:
> --- a/drivers/block/xen-blkback/blkback.c
> +++ b/drivers/block/xen-blkback/blkback.c
> @@ -397,7 +397,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()) {
Removing the blank line here isn''t nice imo, and the initializer is
unnecessary (and admittedly confused me a little when looking at
the hunks further down, thinking the initialization here is to have
"rc" initialized below, until I realized that those are in a different
function).

Jan
> @@ -417,8 +417,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);
> @@ -778,6 +782,12 @@ __do_block_io_op(struct xen_blkif *blkif)
>  	rp = blk_rings->common.sring->req_prod;
>  	rmb(); /* Ensure we see queued requests up to ''rp''. */
>  
> +	/* N.B. ''rp'', not ''rc''. */
> +	if (RING_REQUEST_CONS_OVERFLOW(&blk_rings->common, rp)) {
> +		pr_warn(DRV_PFX "Frontend provided bogus ring requests (%d - %d =
%d).
> Halting ring processing on dev=%04x\n",
> +			rp, rc, rp - rc, blkif->vbd.pdevice);
> +		return -EACCES;
> +	}
>  	while (rc != rp) {
>  
>  		if (RING_REQUEST_CONS_OVERFLOW(&blk_rings->common, rc))

> > Looking at the code, one would expect that the existing check
> 
> The "expect ..." here doesn''t really seem to have a
proper
> termination later in the sentence (or I''m having problems parsing
> the whole construct), so if I didn''t know what the patch is about
> I would have a hard time following.
Oh gosh, that is very badly worded.

.. snip..

I took the advice and altered the patch and hopefully fixed
the commit description to make more sense:


From 8aa353ba3eb706e89a311645a3eeff9e27b61e33 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] 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.

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 whether said difference is greater or
equal to the size of the ring, does not catch this case.

Wha the condition does check if there is a need to process more
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 is in place.

There are four entries: req_prod and rsp_prod; req_event and rsp_event
to track the ring entries. 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 (rsp_prod gets set by rsp_prod_pvt when
pushing the responses on the ring).  Both values can
wrap and are modulo the size of the ring (in block case that is 32).
Please see 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 until the ''rc == rp'' - which means 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. The astute reader will
notice that the macro RING_REQUEST_CONS_OVERFLOW provides two
arguments - more on this later.

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(&blk_rings->common,
blk_rings->common.req_cons)
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).

If we re-use said macro RING_REQUEST_CONS_OVERFLOW and pass in the rp
instead (sring->req_prod) of rc, the this macro can do the check:

     req_prod - sring->rsp_prov_pvt >= 32

Which is,
       X + 31415 - X >= 32 , or 31415 >= 32

which is true, so we can error out and break out of the function.

Cc: stable@vger.kernel.org
[v1: Move the check outside the loop]
[v2: Add a pr_warn as suggested by David]
[v3: Use RING_REQUEST_CONS_OVERFLOW as suggested by Jan]
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
 drivers/block/xen-blkback/blkback.c | 13 ++++++++++++-
 drivers/block/xen-blkback/common.h  |  2 ++
 drivers/block/xen-blkback/xenbus.c  |  2 ++
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/block/xen-blkback/blkback.c
b/drivers/block/xen-blkback/blkback.c
index 4119bcd..11ca405 100644
--- a/drivers/block/xen-blkback/blkback.c
+++ b/drivers/block/xen-blkback/blkback.c
@@ -571,6 +571,7 @@ int xen_blkif_schedule(void *arg)
 	struct xen_blkif *blkif = arg;
 	struct xen_vbd *vbd = &blkif->vbd;
 	unsigned long timeout;
+	int ret;
 
 	xen_blkif_get(blkif);
 
@@ -599,8 +600,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))
+		ret = do_block_io_op(blkif);
+		if (ret > 0)
 			blkif->waiting_reqs = 1;
+		if (ret == -EACCES)
+			wait_event_interruptible(blkif->shutdown_wq,
+						 kthread_should_stop());
 
 purge_gnt_list:
 		if (blkif->vbd.feature_gnt_persistent &&
@@ -1009,6 +1014,12 @@ __do_block_io_op(struct xen_blkif *blkif)
 	rp = blk_rings->common.sring->req_prod;
 	rmb(); /* Ensure we see queued requests up to ''rp''. */
 
+	/* N.B. ''rp'', not ''rc''. */
+	if (RING_REQUEST_CONS_OVERFLOW(&blk_rings->common, rp)) {
+		pr_warn(DRV_PFX "Frontend provided bogus ring requests (%d - %d = %d).
Halting ring processing on dev=%04x\n",
+			rp, rc, rp - rc, blkif->vbd.pdevice);
+		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 c6b4cb9..8d88075 100644
--- a/drivers/block/xen-blkback/common.h
+++ b/drivers/block/xen-blkback/common.h
@@ -314,6 +314,8 @@ struct xen_blkif {
 	unsigned long long			st_wr_sect;
 
 	wait_queue_head_t	waiting_to_free;
+	/* Thread shutdown wait queue. */
+	wait_queue_head_t	shutdown_wq;
 };
 
 struct seg_buf {
diff --git a/drivers/block/xen-blkback/xenbus.c
b/drivers/block/xen-blkback/xenbus.c
index 4a4749c..c9695fe 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -151,6 +151,7 @@ static struct xen_blkif *xen_blkif_alloc(domid_t domid)
 	}
 	spin_lock_init(&blkif->pending_free_lock);
 	init_waitqueue_head(&blkif->pending_free_wq);
+	init_waitqueue_head(&blkif->shutdown_wq);
 
 	return blkif;
 
@@ -230,6 +231,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.1.4

>>> On 05.06.13 at 19:35, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
>> > Looking at the code, one would expect that the existing check
>> 
>> The "expect ..." here doesn''t really seem to have a
proper
>> termination later in the sentence (or I''m having problems
parsing
>> the whole construct), so if I didn''t know what the patch is
about
>> I would have a hard time following.
> 
> Oh gosh, that is very badly worded.
> 
> .. snip..
> 
> I took the advice and altered the patch and hopefully fixed
> the commit description to make more sense:
Thanks, this reads quite a bit better.

Jan

>>> On 05.06.13 at 19:35, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> @@ -230,6 +231,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;
>  	}
Btw., wouldn''t the wake_up() better be done after the kthread_stop(),
so that when the corresponding wait_event_interruptible() checks
whether to exit the terminating kthread_should_stop() is guaranteed
to evaluate to true (otherwise I think there''s potential for it to
never
exit)?

Jan

On Thu, Jun 06, 2013 at 12:47:35PM +0100, Jan Beulich
wrote:
> >>> On 05.06.13 at 19:35, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> > @@ -230,6 +231,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;
> >  	}
> 
> Btw., wouldn''t the wake_up() better be done after the
kthread_stop(),
> so that when the corresponding wait_event_interruptible() checks
> whether to exit the terminating kthread_should_stop() is guaranteed
> to evaluate to true (otherwise I think there''s potential for it to
never
> exit)?
I think you are right. I did the change and the tests were
OK.
> 
> Jan
>

On Tue, Jun 04, 2013 at 03:57:06PM -0400, Konrad Rzeszutek Wilk
wrote:
> 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 (rsp_prod gets set by rsp_prod_pvt when
> pushing the responses on the ring).  Both values can
> wrap and are modulo the size of the ring (in block case that is 32).
> Please see 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 until the ''rc == rp'' - which means 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. The astute reader will
> notice that the macro RING_REQUEST_CONS_OVERFLOW provides two
> arguments - more on this later.
> 
> 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(&blk_rings->common,
blk_rings->common.req_cons)
> 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).
> 
> If we re-use said macro RING_REQUEST_CONS_OVERFLOW and pass in the rp
> instead (sring->req_prod) of rc, the this macro can do the check:
> 
>      req_prod - sring->rsp_prov_pvt >= 32
> 
> Which is,
>        X + 31415 - X >= 32 , or 31415 >= 32
> 
> which is true, so we can error out and break out of the function.
> 
> Cc: stable@vger.kernel.org
> [v1: Move the check outside the loop]
> [v2: Add a pr_warn as suggested by David]
> [v3: Use RING_REQUEST_CONS_OVERFLOW as suggested by Jan]
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  drivers/block/xen-blkback/blkback.c | 14 ++++++++++++--
>  drivers/block/xen-blkback/common.h  |  2 ++
>  drivers/block/xen-blkback/xenbus.c  |  2 ++
>  3 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/block/xen-blkback/blkback.c
b/drivers/block/xen-blkback/blkback.c
> index d81dfca..c4b23be 100644
> --- a/drivers/block/xen-blkback/blkback.c
> +++ b/drivers/block/xen-blkback/blkback.c
> @@ -397,7 +397,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()) {
> @@ -417,8 +417,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);
> @@ -778,6 +782,12 @@ __do_block_io_op(struct xen_blkif *blkif)
>  	rp = blk_rings->common.sring->req_prod;
>  	rmb(); /* Ensure we see queued requests up to ''rp''. */
>  
> +	/* N.B. ''rp'', not ''rc''. */
> +	if (RING_REQUEST_CONS_OVERFLOW(&blk_rings->common, rp)) {
> +		pr_warn(DRV_PFX "Frontend provided bogus ring requests (%d - %d =
%d). Halting ring processing on dev=%04x\n",
> +			rp, rc, rp - rc, blkif->vbd.pdevice);
Hm, I seem to be able to get:

[  189.398095] xen-blkback:Frontend provided bogus ring requests (125 - 115 =
10). Halting ring processing on dev=800011
or:
[  478.558699] xen-blkback:Frontend provided bogus ring requests (95 - 94 = 1).
Halting ring processing on dev=800011

Which is clearly wrong. Piggybacking on the rsp_prod_pvt does not seem to cut
it.

>>> On 07.06.13 at 22:11, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Tue, Jun 04, 2013 at 03:57:06PM -0400, Konrad Rzeszutek Wilk wrote:
>> +	/* N.B. ''rp'', not ''rc''. */
>> +	if (RING_REQUEST_CONS_OVERFLOW(&blk_rings->common, rp)) {
>> +		pr_warn(DRV_PFX "Frontend provided bogus ring requests (%d - %d
= %d).
> Halting ring processing on dev=%04x\n",
>> +			rp, rc, rp - rc, blkif->vbd.pdevice);
> 
> Hm, I seem to be able to get:
> 
> [  189.398095] xen-blkback:Frontend provided bogus ring requests (125 - 115
=
> 10). Halting ring processing on dev=800011
> or:
> [  478.558699] xen-blkback:Frontend provided bogus ring requests (95 - 94 =
> 1). Halting ring processing on dev=800011
> 
> Which is clearly wrong. Piggybacking on the rsp_prod_pvt does not seem to 
> cut it.
We see that too, but not very frequently. One thing is that
rsp_prod_pvt doesn''t get printed along with rc and rp, thus
making it not immediately obvious how this can be off in any way.

Among the instance there are cases where the printed
difference is 32, which makes me wonder whether part of the
problem is the >= in the macro (we may want > here).

And then we might have been living with some sort of issue in the
past, because the existing use of the macro just causes the loop
to be exited, with it getting re-entered subsequently (i.e. at worst
causing performance issues).

Jan

On Mon, Jun 10, 2013 at 04:52:35PM +0100, Jan Beulich
wrote:
> >>> On 07.06.13 at 22:11, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> > On Tue, Jun 04, 2013 at 03:57:06PM -0400, Konrad Rzeszutek Wilk wrote:
> >> +	/* N.B. ''rp'', not ''rc''. */
> >> +	if (RING_REQUEST_CONS_OVERFLOW(&blk_rings->common, rp)) {
> >> +		pr_warn(DRV_PFX "Frontend provided bogus ring requests (%d
- %d = %d).
> > Halting ring processing on dev=%04x\n",
> >> +			rp, rc, rp - rc, blkif->vbd.pdevice);
> > 
> > Hm, I seem to be able to get:
> > 
> > [  189.398095] xen-blkback:Frontend provided bogus ring requests (125
- 115 =
> > 10). Halting ring processing on dev=800011
> > or:
> > [  478.558699] xen-blkback:Frontend provided bogus ring requests (95 -
94 =
> > 1). Halting ring processing on dev=800011
> > 
> > Which is clearly wrong. Piggybacking on the rsp_prod_pvt does not seem
to
> > cut it.
> 
> We see that too, but not very frequently. One thing is that
> rsp_prod_pvt doesn''t get printed along with rc and rp, thus
> making it not immediately obvious how this can be off in any way.
> 
> Among the instance there are cases where the printed
> difference is 32, which makes me wonder whether part of the
> problem is the >= in the macro (we may want > here).
> 
> And then we might have been living with some sort of issue in the
> past, because the existing use of the macro just causes the loop
> to be exited, with it getting re-entered subsequently (i.e. at worst
> causing performance issues).
My observation was that the rsp_prod_pvt was lagging behind b/c the 
READ requests weren''t completed. In other words, the processing
of the ring was stalled b/c ''make_response'' hadn''t
been called yet.
Which meant that rsp_prod was not updated to rsp_prod_pvt (backend
does not care about that value, only frontend does).

Going back to the rc an rp check solves the immediate ''insane ring
check''.

>>> On 10.06.13 at 18:43, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Mon, Jun 10, 2013 at 04:52:35PM +0100, Jan Beulich wrote:
>> >>> On 07.06.13 at 22:11, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
>> > On Tue, Jun 04, 2013 at 03:57:06PM -0400, Konrad Rzeszutek Wilk
wrote:
>> >> +	/* N.B. ''rp'', not ''rc''.
*/
>> >> +	if (RING_REQUEST_CONS_OVERFLOW(&blk_rings->common,
rp)) {
>> >> +		pr_warn(DRV_PFX "Frontend provided bogus ring requests
(%d - %d = %d).
>> > Halting ring processing on dev=%04x\n",
>> >> +			rp, rc, rp - rc, blkif->vbd.pdevice);
>> > 
>> > Hm, I seem to be able to get:
>> > 
>> > [  189.398095] xen-blkback:Frontend provided bogus ring requests
(125 - 115 =
> 
>> > 10). Halting ring processing on dev=800011
>> > or:
>> > [  478.558699] xen-blkback:Frontend provided bogus ring requests
(95 - 94 =
>> > 1). Halting ring processing on dev=800011
>> > 
>> > Which is clearly wrong. Piggybacking on the rsp_prod_pvt does not
seem to
>> > cut it.
>> 
>> We see that too, but not very frequently. One thing is that
>> rsp_prod_pvt doesn''t get printed along with rc and rp, thus
>> making it not immediately obvious how this can be off in any way.
>> 
>> Among the instance there are cases where the printed
>> difference is 32, which makes me wonder whether part of the
>> problem is the >= in the macro (we may want > here).
>> 
>> And then we might have been living with some sort of issue in the
>> past, because the existing use of the macro just causes the loop
>> to be exited, with it getting re-entered subsequently (i.e. at worst
>> causing performance issues).
> 
> My observation was that the rsp_prod_pvt was lagging behind b/c the 
> READ requests weren''t completed. In other words, the processing
> of the ring was stalled b/c ''make_response''
hadn''t been called yet.
> Which meant that rsp_prod was not updated to rsp_prod_pvt (backend
> does not care about that value, only frontend does).
I don''t buy this: rsp_prod is being updated by the backend just for
the frontend''s sake, so this value really doesn''t need looking
at (or
else we''d become susceptible to the guest maliciously writing that
field).

rsp_prod_pvt, otoh, is never behind rsp_prod, and if the guest
produces requests that don''t have matching space for responses,
the guest is doing something bogus (and perhaps malicious).
> Going back to the rc an rp check solves the immediate ''insane ring
> check''. 
Consequently, while this check is better than none at all, I think it
is still too lax, and we really want to check against the produced
responses. Just that other than for the rc check using >=, we''d
need > for the rp one.

But first of all let me see if I can get the original broken check to
trigger wrongly here (so far only our stage testing caught these),
and look at by how much rsp_prod_pvt really lags.

Jan

On 6/11/2013 3:42 AM, Jan Beulich wrote:
>>>> On 10.06.13 at 18:43, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
>> On Mon, Jun 10, 2013 at 04:52:35PM +0100, Jan Beulich wrote:
>>>>>> On 07.06.13 at 22:11, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
>>>> On Tue, Jun 04, 2013 at 03:57:06PM -0400, Konrad Rzeszutek Wilk
wrote:
>>>>> +	/* N.B. ''rp'', not
''rc''. */
>>>>> +	if (RING_REQUEST_CONS_OVERFLOW(&blk_rings->common,
rp)) {
>>>>> +		pr_warn(DRV_PFX "Frontend provided bogus ring
requests (%d - %d = %d).
>>>> Halting ring processing on dev=%04x\n",
>>>>> +			rp, rc, rp - rc, blkif->vbd.pdevice);
>>>> Hm, I seem to be able to get:
>>>>
>>>> [  189.398095] xen-blkback:Frontend provided bogus ring
requests (125 - 115 >>>> 10). Halting ring processing on dev=800011
>>>> or:
>>>> [  478.558699] xen-blkback:Frontend provided bogus ring
requests (95 - 94 >>>> 1). Halting ring processing on dev=800011
>>>>
>>>> Which is clearly wrong. Piggybacking on the rsp_prod_pvt does
not seem to
>>>> cut it.
>>> We see that too, but not very frequently. One thing is that
>>> rsp_prod_pvt doesn''t get printed along with rc and rp,
thus
>>> making it not immediately obvious how this can be off in any way.
>>>
>>> Among the instance there are cases where the printed
>>> difference is 32, which makes me wonder whether part of the
>>> problem is the >= in the macro (we may want > here).
>>>
>>> And then we might have been living with some sort of issue in the
>>> past, because the existing use of the macro just causes the loop
>>> to be exited, with it getting re-entered subsequently (i.e. at
worst
>>> causing performance issues).
>> My observation was that the rsp_prod_pvt was lagging behind b/c the
>> READ requests weren''t completed. In other words, the
processing
>> of the ring was stalled b/c ''make_response''
hadn''t been called yet.
>> Which meant that rsp_prod was not updated to rsp_prod_pvt (backend
>> does not care about that value, only frontend does).
> I don''t buy this: rsp_prod is being updated by the backend just
for
> the frontend''s sake, so this value really doesn''t need
looking at (or
> else we''d become susceptible to the guest maliciously writing that
> field).
>
> rsp_prod_pvt, otoh, is never behind rsp_prod, and if the guest
> produces requests that don''t have matching space for responses,
> the guest is doing something bogus (and perhaps malicious).
I believe this is what I saw with the rsp_prod_pvt added in the printk. 
Unfortunately I did not save the logs.
>
>> Going back to the rc an rp check solves the immediate ''insane
ring
>> check''.
> Consequently, while this check is better than none at all, I think it
> is still too lax, and we really want to check against the produced
> responses. Just that other than for the rc check using >=, we''d
> need > for the rp one.
>
> But first of all let me see if I can get the original broken check to
> trigger wrongly here (so far only our stage testing caught these),
> and look at by how much rsp_prod_pvt really lags.
OK.
>
> Jan
>