[PATCH] fixes to xen-blk[back|front] to deal with 32/64 host/guest combination with BLKIF_DISCARD (v1).

[PATCH] fixes to xen-blk[back|front] to deal with 32/64 host/guest combination with BLKIF_DISCARD (v1).

[Konrad Rzeszutek Wilk]

These two patches came out of hitting https://bugzilla.redhat.com/show_bug.cgi?id=824641
where a 32-bit guest would send a BLKIF_DISCARD request to a 64-bit
host. The xen-blkback did not copy the 'id' field into the response (it ended
up with a random value), which the frontend uses to lookup in its array to
find the 'struct request' that corresponded to this BLKIF_DISCARD.

The result was the __blk_end_request_all ended being called with a NULL
pointer and crashed the guest.

The fix is easy enough - make xen-blkback copy the 'id' field when
dealing with 32/64 or 64/32 host/guest combinations. The problem
does not exist if we are using a 64/64 combination.

I also added two BUG_ON to xen-blkfront to detect this misuse of 'id'
field in case there are other backends that do this.

If there are no problems with these patches I was thinking to put them
in my stable/for-jens-3.5 tree and ask Jens to pull them.

 drivers/block/xen-blkback/common.h |    2 ++
 drivers/block/xen-blkfront.c       |    7 +++++++
 2 files changed, 9 insertions(+), 0 deletions(-)

[PATCH 1/2] xen/blkback: Copy id field when doing BLKIF_DISCARD.

[Konrad Rzeszutek Wilk]

We weren't copying the id field so when we sent the response
back to the frontend (especially with a 64-bit host and 32-bit
guest), we ended up using a random value. This lead to the
frontend crashing as it would try to pass to __blk_end_request_all
a NULL 'struct request' (b/c it would use the 'id' to find the
proper 'struct request' in its shadow array) and end up crashing:

BUG: unable to handle kernel NULL pointer dereference at 000000e4
IP: [<c0646d4c>] __blk_end_request_all+0xc/0x40
.. snip..
EIP is at __blk_end_request_all+0xc/0x40
.. snip..
 [<ed95db72>] blkif_interrupt+0x172/0x330 [xen_blkfront]

This fixes the bug by passing in the proper id for the response.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=824641
CC: stable@kernel.org
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/block/xen-blkback/common.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/block/xen-blkback/common.h b/drivers/block/xen-blkback/common.h
index 773cf27..9ad3b5e 100644
--- a/drivers/block/xen-blkback/common.h
+++ b/drivers/block/xen-blkback/common.h
@@ -257,6 +257,7 @@ static inline void blkif_get_x86_32_req(struct blkif_request *dst,
 		break;
 	case BLKIF_OP_DISCARD:
 		dst->u.discard.flag = src->u.discard.flag;
+		dst->u.discard.id = src->u.discard.id;
 		dst->u.discard.sector_number = src->u.discard.sector_number;
 		dst->u.discard.nr_sectors = src->u.discard.nr_sectors;
 		break;
@@ -287,6 +288,7 @@ static inline void blkif_get_x86_64_req(struct blkif_request *dst,
 		break;
 	case BLKIF_OP_DISCARD:
 		dst->u.discard.flag = src->u.discard.flag;
+		dst->u.discard.id = src->u.discard.id;
 		dst->u.discard.sector_number = src->u.discard.sector_number;
 		dst->u.discard.nr_sectors = src->u.discard.nr_sectors;
 		break;
-- 
1.7.7.6

Re: [Xen-devel] [PATCH 1/2] xen/blkback: Copy id field when doing BLKIF_DISCARD.

[Stefano Stabellini]

On Fri, 25 May 2012, Konrad Rzeszutek Wilk wrote:
> We weren't copying the id field so when we sent the response
> back to the frontend (especially with a 64-bit host and 32-bit
> guest), we ended up using a random value. This lead to the
> frontend crashing as it would try to pass to __blk_end_request_all
> a NULL 'struct request' (b/c it would use the 'id' to find the
> proper 'struct request' in its shadow array) and end up crashing:
> 
> BUG: unable to handle kernel NULL pointer dereference at 000000e4
> IP: [<c0646d4c>] __blk_end_request_all+0xc/0x40
> .. snip..
> EIP is at __blk_end_request_all+0xc/0x40
> .. snip..
>  [<ed95db72>] blkif_interrupt+0x172/0x330 [xen_blkfront]
> 
> This fixes the bug by passing in the proper id for the response.
> 
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=824641
> CC: stable@kernel.org
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

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

>  drivers/block/xen-blkback/common.h |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/block/xen-blkback/common.h b/drivers/block/xen-blkback/common.h
> index 773cf27..9ad3b5e 100644
> --- a/drivers/block/xen-blkback/common.h
> +++ b/drivers/block/xen-blkback/common.h
> @@ -257,6 +257,7 @@ static inline void blkif_get_x86_32_req(struct blkif_request *dst,
>  		break;
>  	case BLKIF_OP_DISCARD:
>  		dst->u.discard.flag = src->u.discard.flag;
> +		dst->u.discard.id = src->u.discard.id;
>  		dst->u.discard.sector_number = src->u.discard.sector_number;
>  		dst->u.discard.nr_sectors = src->u.discard.nr_sectors;
>  		break;
> @@ -287,6 +288,7 @@ static inline void blkif_get_x86_64_req(struct blkif_request *dst,
>  		break;
>  	case BLKIF_OP_DISCARD:
>  		dst->u.discard.flag = src->u.discard.flag;
> +		dst->u.discard.id = src->u.discard.id;
>  		dst->u.discard.sector_number = src->u.discard.sector_number;
>  		dst->u.discard.nr_sectors = src->u.discard.nr_sectors;
>  		break;
> -- 
> 1.7.7.6
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

Re: [Xen-devel] [PATCH 1/2] xen/blkback: Copy id field when doing BLKIF_DISCARD.

[William Dauchy]

On Mon, May 28, 2012 at 12:19 PM, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
>> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=824641
>> CC: stable@kernel.org
>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Tested-by: William Dauchy <wdauchy@gmail.com>

-- 
William

Re: [Xen-devel] [PATCH 1/2] xen/blkback: Copy id field when doing BLKIF_DISCARD.

[Konrad Rzeszutek Wilk]

On Wed, May 30, 2012 at 10:39:10PM +0200, William Dauchy wrote:
> On Mon, May 28, 2012 at 12:19 PM, Stefano Stabellini
> <stefano.stabellini@eu.citrix.com> wrote:
> >> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=824641
> >> CC: stable@kernel.org
> >> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >
> > Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> Tested-by: William Dauchy <wdauchy@gmail.com>

Heh. That is what I get from reading emails in sequence. Thanks!

[PATCH 2/2] xen/blkfront: Add BUG_ON to deal with misbehaving backends.

[Konrad Rzeszutek Wilk]

Part of the ring structure is the 'id' field which is under
control of the frontend. The frontend stamps it with "some"
value (this some in this implementation being a value less
than BLK_RING_SIZE), and when it gets a response expects
said value to be in the response structure. We have a check
for the id field when spolling new requests but not when
de-spolling responses.

We also add an extra check in add_id_to_freelist to make
sure that the 'struct request' was not NULL - as we cannot
pass a NULL to __blk_end_request_all, otherwise that crashes
(and all the operations that the response is dealing with
end up with __blk_end_request_all).

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/block/xen-blkfront.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 60eed4b..8e177ca 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -145,6 +145,7 @@ static void add_id_to_freelist(struct blkfront_info *info,
 			       unsigned long id)
 {
 	info->shadow[id].req.u.rw.id  = info->shadow_free;
+	BUG_ON(info->shadow[id].request == NULL);
 	info->shadow[id].request = NULL;
 	info->shadow_free = id;
 }
@@ -746,6 +747,12 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
 
 		bret = RING_GET_RESPONSE(&info->ring, i);
 		id   = bret->id;
+		/*
+		 * The backend has messed up and given us an id that we would
+		 * never have given to it (we stamp it up to BLK_RING_SIZE -
+		 * look in get_id_from_freelist.
+		 */
+		BUG_ON(id >= BLK_RING_SIZE);
 		req  = info->shadow[id].request;
 
 		if (bret->operation != BLKIF_OP_DISCARD)
-- 
1.7.7.6

Re: [Xen-devel] [PATCH 2/2] xen/blkfront: Add BUG_ON to deal with misbehaving backends.

[Stefano Stabellini]

On Fri, 25 May 2012, Konrad Rzeszutek Wilk wrote:
> Part of the ring structure is the 'id' field which is under
> control of the frontend. The frontend stamps it with "some"
> value (this some in this implementation being a value less
> than BLK_RING_SIZE), and when it gets a response expects
> said value to be in the response structure. We have a check
> for the id field when spolling new requests but not when
> de-spolling responses.
> 
> We also add an extra check in add_id_to_freelist to make
> sure that the 'struct request' was not NULL - as we cannot
> pass a NULL to __blk_end_request_all, otherwise that crashes
> (and all the operations that the response is dealing with
> end up with __blk_end_request_all).
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  drivers/block/xen-blkfront.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index 60eed4b..8e177ca 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -145,6 +145,7 @@ static void add_id_to_freelist(struct blkfront_info *info,
>  			       unsigned long id)
>  {
>  	info->shadow[id].req.u.rw.id  = info->shadow_free;
> +	BUG_ON(info->shadow[id].request == NULL);
>  	info->shadow[id].request = NULL;
>  	info->shadow_free = id;
>  }
> @@ -746,6 +747,12 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
>  
>  		bret = RING_GET_RESPONSE(&info->ring, i);
>  		id   = bret->id;
> +		/*
> +		 * The backend has messed up and given us an id that we would
> +		 * never have given to it (we stamp it up to BLK_RING_SIZE -
> +		 * look in get_id_from_freelist.
> +		 */
> +		BUG_ON(id >= BLK_RING_SIZE);
>  		req  = info->shadow[id].request;
>  
>  		if (bret->operation != BLKIF_OP_DISCARD)

While we should certainly check whether bret->id is valid before
using it, is it actually correct that the frontend BUGs in response of a
backend bug?

If the IO doesn't involve the root disk, the guest might be able to
function correctly without communicating with the backend at all.
I think we should WARN and return error. Maybe also call blkfront_remove
if we can.

Re: [Xen-devel] [PATCH 2/2] xen/blkfront: Add BUG_ON to deal with misbehaving backends.

[Jan Beulich]

>>> On 28.05.12 at 12:18, Stefano Stabellini <stefano.stabellini@eu.citrix.com>
wrote:
> On Fri, 25 May 2012, Konrad Rzeszutek Wilk wrote:
>> Part of the ring structure is the 'id' field which is under
>> control of the frontend. The frontend stamps it with "some"
>> value (this some in this implementation being a value less
>> than BLK_RING_SIZE), and when it gets a response expects
>> said value to be in the response structure. We have a check
>> for the id field when spolling new requests but not when
>> de-spolling responses.
>> 
>> We also add an extra check in add_id_to_freelist to make
>> sure that the 'struct request' was not NULL - as we cannot
>> pass a NULL to __blk_end_request_all, otherwise that crashes
>> (and all the operations that the response is dealing with
>> end up with __blk_end_request_all).
>> 
>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> ---
>>  drivers/block/xen-blkfront.c |    7 +++++++
>>  1 files changed, 7 insertions(+), 0 deletions(-)
>> 
>> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
>> index 60eed4b..8e177ca 100644
>> --- a/drivers/block/xen-blkfront.c
>> +++ b/drivers/block/xen-blkfront.c
>> @@ -145,6 +145,7 @@ static void add_id_to_freelist(struct blkfront_info 
> *info,
>>  			       unsigned long id)
>>  {
>>  	info->shadow[id].req.u.rw.id  = info->shadow_free;
>> +	BUG_ON(info->shadow[id].request == NULL);

This only catches a small sub-portion of possible bad backend
behavior. Checking (as the very first thing in the function) e.g.

info->shadow[id].req.u.rw.id == id

would seem to cover a broader set (based on my recent looking
at similar mismatches apparently resulting from the qdisk
backend occasionally sending bad/duplicate responses).

But take this with the below applied here too.

>>  	info->shadow[id].request = NULL;
>>  	info->shadow_free = id;
>>  }
>> @@ -746,6 +747,12 @@ static irqreturn_t blkif_interrupt(int irq, void 
> *dev_id)
>>  
>>  		bret = RING_GET_RESPONSE(&info->ring, i);
>>  		id   = bret->id;
>> +		/*
>> +		 * The backend has messed up and given us an id that we would
>> +		 * never have given to it (we stamp it up to BLK_RING_SIZE -
>> +		 * look in get_id_from_freelist.
>> +		 */
>> +		BUG_ON(id >= BLK_RING_SIZE);
>>  		req  = info->shadow[id].request;
>>  
>>  		if (bret->operation != BLKIF_OP_DISCARD)
> 
> While we should certainly check whether bret->id is valid before
> using it, is it actually correct that the frontend BUGs in response of a
> backend bug?
> 
> If the IO doesn't involve the root disk, the guest might be able to
> function correctly without communicating with the backend at all.
> I think we should WARN and return error. Maybe also call blkfront_remove
> if we can.

I very much agree to this.

Jan

Re: [Xen-devel] [PATCH 2/2] xen/blkfront: Add BUG_ON to deal with misbehaving backends.

[Konrad Rzeszutek Wilk]

> >> @@ -145,6 +145,7 @@ static void add_id_to_freelist(struct blkfront_info 
> > *info,
> >>  			       unsigned long id)
> >>  {
> >>  	info->shadow[id].req.u.rw.id  = info->shadow_free;
> >> +	BUG_ON(info->shadow[id].request == NULL);
> 
> This only catches a small sub-portion of possible bad backend
> behavior. Checking (as the very first thing in the function) e.g.
> 
> info->shadow[id].req.u.rw.id == id
> 
> would seem to cover a broader set (based on my recent looking
> at similar mismatches apparently resulting from the qdisk
> backend occasionally sending bad/duplicate responses).
> 
> But take this with the below applied here too.
> 
> >>  	info->shadow[id].request = NULL;
> >>  	info->shadow_free = id;
> >>  }
> >> @@ -746,6 +747,12 @@ static irqreturn_t blkif_interrupt(int irq, void 
> > *dev_id)
> >>  
> >>  		bret = RING_GET_RESPONSE(&info->ring, i);
> >>  		id   = bret->id;
> >> +		/*
> >> +		 * The backend has messed up and given us an id that we would
> >> +		 * never have given to it (we stamp it up to BLK_RING_SIZE -
> >> +		 * look in get_id_from_freelist.
> >> +		 */
> >> +		BUG_ON(id >= BLK_RING_SIZE);
> >>  		req  = info->shadow[id].request;
> >>  
> >>  		if (bret->operation != BLKIF_OP_DISCARD)
> > 
> > While we should certainly check whether bret->id is valid before
> > using it, is it actually correct that the frontend BUGs in response of a
> > backend bug?

The 'id' is used to get the 'struct request' and to do do the grant unmaps.
Since it would be outside the shadow structure it would fetch garbage as
'struct request'.

> > 
> > If the IO doesn't involve the root disk, the guest might be able to
> > function correctly without communicating with the backend at all.
> > I think we should WARN and return error. Maybe also call blkfront_remove
> > if we can.
> 
> I very much agree to this.

The blkfront_remove part is .. that is going to take some surgery to do
and I don't think I am going to be able to attempt that within the next couple
of weeks. So lets put that on the TODO list and just do this one?

>From 4aabb5b44778fc0c0b8d4f5a2e2cd8e8490064d7 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Fri, 25 May 2012 17:34:51 -0400
Subject: [PATCH] xen/blkfront: Add WARN to deal with misbehaving backends.

Part of the ring structure is the 'id' field which is under
control of the frontend. The frontend stamps it with "some"
value (this some in this implementation being a value less
than BLK_RING_SIZE), and when it gets a response expects
said value to be in the response structure. We have a check
for the id field when spolling new requests but not when
de-spolling responses.

We also add an extra check in add_id_to_freelist to make
sure that the 'struct request' was not NULL - as we cannot
pass a NULL to __blk_end_request_all, otherwise that crashes
(and all the operations that the response is dealing with
end up with __blk_end_request_all).

Lastly we also print the name of the operation that failed.

[v1: s/BUG/WARN/ suggested by Stefano]
[v2: Add extra check in add_id_to_freelist]
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/block/xen-blkfront.c |   39 +++++++++++++++++++++++++++++----------
 1 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 60eed4b..c7ef8a4 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -144,11 +144,22 @@ static int get_id_from_freelist(struct blkfront_info *info)
 static void add_id_to_freelist(struct blkfront_info *info,
 			       unsigned long id)
 {
+	BUG_ON(info->shadow[id].req.u.rw.id != id);
 	info->shadow[id].req.u.rw.id  = info->shadow_free;
+	BUG_ON(info->shadow[id].request == NULL);
 	info->shadow[id].request = NULL;
 	info->shadow_free = id;
 }
 
+static const char *op_name(int op)
+{
+	const char *names[BLKIF_OP_DISCARD+1] = {
+		"read" , "write", "barrier", "flush", "reserved", "discard"};
+
+	if (op > BLKIF_OP_DISCARD)
+		return "unknown";
+	return names[op];
+}
 static int xlbd_reserve_minors(unsigned int minor, unsigned int nr)
 {
 	unsigned int end = minor + nr;
@@ -746,6 +757,18 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
 
 		bret = RING_GET_RESPONSE(&info->ring, i);
 		id   = bret->id;
+		/*
+		 * The backend has messed up and given us an id that we would
+		 * never have given to it (we stamp it up to BLK_RING_SIZE -
+		 * look in get_id_from_freelist.
+		 */
+		if (id >= BLK_RING_SIZE) {
+			/* We can't safely get the 'struct request' as
+			 * the id is busted. So limp along. */
+			WARN(1, "%s: response to %s has incorrect id (%d)!\n",
+			     info->gd->disk_name, op_name(bret->operation), id);
+			continue;
+		}
 		req  = info->shadow[id].request;
 
 		if (bret->operation != BLKIF_OP_DISCARD)
@@ -758,8 +781,8 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
 		case BLKIF_OP_DISCARD:
 			if (unlikely(bret->status == BLKIF_RSP_EOPNOTSUPP)) {
 				struct request_queue *rq = info->rq;
-				printk(KERN_WARNING "blkfront: %s: discard op failed\n",
-					   info->gd->disk_name);
+				printk(KERN_WARNING "blkfront: %s: %s op failed\n",
+					   info->gd->disk_name, op_name(bret->operation));
 				error = -EOPNOTSUPP;
 				info->feature_discard = 0;
 				info->feature_secdiscard = 0;
@@ -771,18 +794,14 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
 		case BLKIF_OP_FLUSH_DISKCACHE:
 		case BLKIF_OP_WRITE_BARRIER:
 			if (unlikely(bret->status == BLKIF_RSP_EOPNOTSUPP)) {
-				printk(KERN_WARNING "blkfront: %s: write %s op failed\n",
-				       info->flush_op == BLKIF_OP_WRITE_BARRIER ?
-				       "barrier" :  "flush disk cache",
-				       info->gd->disk_name);
+				printk(KERN_WARNING "blkfront: %s: %s op failed\n",
+				       info->gd->disk_name, op_name(bret->operation));
 				error = -EOPNOTSUPP;
 			}
 			if (unlikely(bret->status == BLKIF_RSP_ERROR &&
 				     info->shadow[id].req.u.rw.nr_segments == 0)) {
-				printk(KERN_WARNING "blkfront: %s: empty write %s op failed\n",
-				       info->flush_op == BLKIF_OP_WRITE_BARRIER ?
-				       "barrier" :  "flush disk cache",
-				       info->gd->disk_name);
+				printk(KERN_WARNING "blkfront: %s: empty %s op failed\n",
+				       info->gd->disk_name, op_name(bret->operation));
 				error = -EOPNOTSUPP;
 			}
 			if (unlikely(error)) {
-- 
1.7.7.6

Re: [Xen-devel] [PATCH 2/2] xen/blkfront: Add BUG_ON to deal with misbehaving backends.

[Jan Beulich]

>>> On 31.05.12 at 20:17, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> The blkfront_remove part is .. that is going to take some surgery to do
> and I don't think I am going to be able to attempt that within the next 
> couple
> of weeks. So lets put that on the TODO list and just do this one?

That sounds okay to me.

> From 4aabb5b44778fc0c0b8d4f5a2e2cd8e8490064d7 Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date: Fri, 25 May 2012 17:34:51 -0400
> Subject: [PATCH] xen/blkfront: Add WARN to deal with misbehaving backends.
> 
> Part of the ring structure is the 'id' field which is under
> control of the frontend. The frontend stamps it with "some"
> value (this some in this implementation being a value less
> than BLK_RING_SIZE), and when it gets a response expects
> said value to be in the response structure. We have a check
> for the id field when spolling new requests but not when
> de-spolling responses.
> 
> We also add an extra check in add_id_to_freelist to make
> sure that the 'struct request' was not NULL - as we cannot
> pass a NULL to __blk_end_request_all, otherwise that crashes
> (and all the operations that the response is dealing with
> end up with __blk_end_request_all).
> 
> Lastly we also print the name of the operation that failed.
> 
> [v1: s/BUG/WARN/ suggested by Stefano]

That's only partly true, since ...

> [v2: Add extra check in add_id_to_freelist]
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  drivers/block/xen-blkfront.c |   39 +++++++++++++++++++++++++++++----------
>  1 files changed, 29 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index 60eed4b..c7ef8a4 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -144,11 +144,22 @@ static int get_id_from_freelist(struct blkfront_info 
> *info)
>  static void add_id_to_freelist(struct blkfront_info *info,
>  			       unsigned long id)
>  {
> +	BUG_ON(info->shadow[id].req.u.rw.id != id);
>  	info->shadow[id].req.u.rw.id  = info->shadow_free;
> +	BUG_ON(info->shadow[id].request == NULL);

... there's now even two BUG_ON()s here. I would think these
checks should either happen at the call site, or the function
should return an error indication; in any case should these just
result in warnings just like the out-of-range check you added.

>  	info->shadow[id].request = NULL;
>  	info->shadow_free = id;
>  }
>  
> +static const char *op_name(int op)
> +{
> +	const char *names[BLKIF_OP_DISCARD+1] = {

	static const char *const names[] = {

> +		"read" , "write", "barrier", "flush", "reserved", "discard"};

Perhaps using dedicated initializers would be preferable here?

> +
> +	if (op > BLKIF_OP_DISCARD)

	if (op >= ARRAY_SIZE(names))

(all three adjustments making future additions less intrusive -
they could be single-line then).

Jan

> +		return "unknown";
> +	return names[op];
> +}
>  static int xlbd_reserve_minors(unsigned int minor, unsigned int nr)
>  {
>  	unsigned int end = minor + nr;
> @@ -746,6 +757,18 @@ static irqreturn_t blkif_interrupt(int irq, void 
> *dev_id)
>  
>  		bret = RING_GET_RESPONSE(&info->ring, i);
>  		id   = bret->id;
> +		/*
> +		 * The backend has messed up and given us an id that we would
> +		 * never have given to it (we stamp it up to BLK_RING_SIZE -
> +		 * look in get_id_from_freelist.
> +		 */
> +		if (id >= BLK_RING_SIZE) {
> +			/* We can't safely get the 'struct request' as
> +			 * the id is busted. So limp along. */
> +			WARN(1, "%s: response to %s has incorrect id (%d)!\n",
> +			     info->gd->disk_name, op_name(bret->operation), id);
> +			continue;
> +		}
>  		req  = info->shadow[id].request;
>  
>  		if (bret->operation != BLKIF_OP_DISCARD)
> @@ -758,8 +781,8 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
>  		case BLKIF_OP_DISCARD:
>  			if (unlikely(bret->status == BLKIF_RSP_EOPNOTSUPP)) {
>  				struct request_queue *rq = info->rq;
> -				printk(KERN_WARNING "blkfront: %s: discard op failed\n",
> -					   info->gd->disk_name);
> +				printk(KERN_WARNING "blkfront: %s: %s op failed\n",
> +					   info->gd->disk_name, op_name(bret->operation));
>  				error = -EOPNOTSUPP;
>  				info->feature_discard = 0;
>  				info->feature_secdiscard = 0;
> @@ -771,18 +794,14 @@ static irqreturn_t blkif_interrupt(int irq, void 
> *dev_id)
>  		case BLKIF_OP_FLUSH_DISKCACHE:
>  		case BLKIF_OP_WRITE_BARRIER:
>  			if (unlikely(bret->status == BLKIF_RSP_EOPNOTSUPP)) {
> -				printk(KERN_WARNING "blkfront: %s: write %s op failed\n",
> -				       info->flush_op == BLKIF_OP_WRITE_BARRIER ?
> -				       "barrier" :  "flush disk cache",
> -				       info->gd->disk_name);
> +				printk(KERN_WARNING "blkfront: %s: %s op failed\n",
> +				       info->gd->disk_name, op_name(bret->operation));
>  				error = -EOPNOTSUPP;
>  			}
>  			if (unlikely(bret->status == BLKIF_RSP_ERROR &&
>  				     info->shadow[id].req.u.rw.nr_segments == 0)) {
> -				printk(KERN_WARNING "blkfront: %s: empty write %s op failed\n",
> -				       info->flush_op == BLKIF_OP_WRITE_BARRIER ?
> -				       "barrier" :  "flush disk cache",
> -				       info->gd->disk_name);
> +				printk(KERN_WARNING "blkfront: %s: empty %s op failed\n",
> +				       info->gd->disk_name, op_name(bret->operation));
>  				error = -EOPNOTSUPP;
>  			}
>  			if (unlikely(error)) {
> -- 
> 1.7.7.6

Re: [Xen-devel] [PATCH 2/2] xen/blkfront: Add BUG_ON to deal with misbehaving backends.

[Stefano Stabellini]

On Thu, 31 May 2012, Konrad Rzeszutek Wilk wrote:
> The blkfront_remove part is .. that is going to take some surgery to do
> and I don't think I am going to be able to attempt that within the next couple
> of weeks. So lets put that on the TODO list and just do this one?

OK

> >From 4aabb5b44778fc0c0b8d4f5a2e2cd8e8490064d7 Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date: Fri, 25 May 2012 17:34:51 -0400
> Subject: [PATCH] xen/blkfront: Add WARN to deal with misbehaving backends.
> 
> Part of the ring structure is the 'id' field which is under
> control of the frontend. The frontend stamps it with "some"
> value (this some in this implementation being a value less
> than BLK_RING_SIZE), and when it gets a response expects
> said value to be in the response structure. We have a check
> for the id field when spolling new requests but not when
> de-spolling responses.
> 
> We also add an extra check in add_id_to_freelist to make
> sure that the 'struct request' was not NULL - as we cannot
> pass a NULL to __blk_end_request_all, otherwise that crashes
> (and all the operations that the response is dealing with
> end up with __blk_end_request_all).
> 
> Lastly we also print the name of the operation that failed.
> 
> [v1: s/BUG/WARN/ suggested by Stefano]
> [v2: Add extra check in add_id_to_freelist]
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  drivers/block/xen-blkfront.c |   39 +++++++++++++++++++++++++++++----------
>  1 files changed, 29 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index 60eed4b..c7ef8a4 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -144,11 +144,22 @@ static int get_id_from_freelist(struct blkfront_info *info)
>  static void add_id_to_freelist(struct blkfront_info *info,
>  			       unsigned long id)
>  {
> +	BUG_ON(info->shadow[id].req.u.rw.id != id);
>  	info->shadow[id].req.u.rw.id  = info->shadow_free;
> +	BUG_ON(info->shadow[id].request == NULL);
>  	info->shadow[id].request = NULL;
>  	info->shadow_free = id;
>  }

Like Jan said, it would be best to change the two BUG_ON into WARN_ON
and return an error.


> +static const char *op_name(int op)
> +{
> +	const char *names[BLKIF_OP_DISCARD+1] = {
> +		"read" , "write", "barrier", "flush", "reserved", "discard"};
> +
> +	if (op > BLKIF_OP_DISCARD)
> +		return "unknown";
> +	return names[op];
> +}

Considering that op is an int, shoudn't we check for negative values
too?


>  static int xlbd_reserve_minors(unsigned int minor, unsigned int nr)
>  {
>  	unsigned int end = minor + nr;
> @@ -746,6 +757,18 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
>  
>  		bret = RING_GET_RESPONSE(&info->ring, i);
>  		id   = bret->id;
> +		/*
> +		 * The backend has messed up and given us an id that we would
> +		 * never have given to it (we stamp it up to BLK_RING_SIZE -
> +		 * look in get_id_from_freelist.
> +		 */
> +		if (id >= BLK_RING_SIZE) {
> +			/* We can't safely get the 'struct request' as
> +			 * the id is busted. So limp along. */
> +			WARN(1, "%s: response to %s has incorrect id (%d)!\n",
> +			     info->gd->disk_name, op_name(bret->operation), id);
> +			continue;
> +		}
>  		req  = info->shadow[id].request;

Do you think it would be better to goto error_out, instead of continue?
I guess that depends on whether we expect the other requests to be in a
good shape or not...

Re: [Xen-devel] [PATCH 2/2] xen/blkfront: Add BUG_ON to deal with misbehaving backends.

[Konrad Rzeszutek Wilk]

On Fri, Jun 01, 2012 at 11:16:29AM +0100, Stefano Stabellini wrote:
> On Thu, 31 May 2012, Konrad Rzeszutek Wilk wrote:
> > The blkfront_remove part is .. that is going to take some surgery to do
> > and I don't think I am going to be able to attempt that within the next couple
> > of weeks. So lets put that on the TODO list and just do this one?
> 
> OK
> 
> > >From 4aabb5b44778fc0c0b8d4f5a2e2cd8e8490064d7 Mon Sep 17 00:00:00 2001
> > From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Date: Fri, 25 May 2012 17:34:51 -0400
> > Subject: [PATCH] xen/blkfront: Add WARN to deal with misbehaving backends.
> > 
> > Part of the ring structure is the 'id' field which is under
> > control of the frontend. The frontend stamps it with "some"
> > value (this some in this implementation being a value less
> > than BLK_RING_SIZE), and when it gets a response expects
> > said value to be in the response structure. We have a check
> > for the id field when spolling new requests but not when
> > de-spolling responses.
> > 
> > We also add an extra check in add_id_to_freelist to make
> > sure that the 'struct request' was not NULL - as we cannot
> > pass a NULL to __blk_end_request_all, otherwise that crashes
> > (and all the operations that the response is dealing with
> > end up with __blk_end_request_all).
> > 
> > Lastly we also print the name of the operation that failed.
> > 
> > [v1: s/BUG/WARN/ suggested by Stefano]
> > [v2: Add extra check in add_id_to_freelist]
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  drivers/block/xen-blkfront.c |   39 +++++++++++++++++++++++++++++----------
> >  1 files changed, 29 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> > index 60eed4b..c7ef8a4 100644
> > --- a/drivers/block/xen-blkfront.c
> > +++ b/drivers/block/xen-blkfront.c
> > @@ -144,11 +144,22 @@ static int get_id_from_freelist(struct blkfront_info *info)
> >  static void add_id_to_freelist(struct blkfront_info *info,
> >  			       unsigned long id)
> >  {
> > +	BUG_ON(info->shadow[id].req.u.rw.id != id);
> >  	info->shadow[id].req.u.rw.id  = info->shadow_free;
> > +	BUG_ON(info->shadow[id].request == NULL);
> >  	info->shadow[id].request = NULL;
> >  	info->shadow_free = id;
> >  }
> 
> Like Jan said, it would be best to change the two BUG_ON into WARN_ON
> and return an error.

Yes. I missed that.
> 
> 
> > +static const char *op_name(int op)
> > +{
> > +	const char *names[BLKIF_OP_DISCARD+1] = {
> > +		"read" , "write", "barrier", "flush", "reserved", "discard"};
> > +
> > +	if (op > BLKIF_OP_DISCARD)
> > +		return "unknown";
> > +	return names[op];
> > +}
> 
> Considering that op is an int, shoudn't we check for negative values
> too?

Yes! I also converted this per Jan's excellent idea.

Please see:


>From 3877611c3096423a7741e99e9c9b9e63a9f2e557 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Fri, 25 May 2012 17:34:51 -0400
Subject: [PATCH] xen/blkfront: Add WARN to deal with misbehaving backends.

Part of the ring structure is the 'id' field which is under
control of the frontend. The frontend stamps it with "some"
value (this some in this implementation being a value less
than BLK_RING_SIZE), and when it gets a response expects
said value to be in the response structure. We have a check
for the id field when spolling new requests but not when
de-spolling responses.

We also add an extra check in add_id_to_freelist to make
sure that the 'struct request' was not NULL - as we cannot
pass a NULL to __blk_end_request_all, otherwise that crashes
(and all the operations that the response is dealing with
end up with __blk_end_request_all).

Lastly we also print the name of the operation that failed.

[v1: s/BUG/WARN/ suggested by Stefano]
[v2: Add extra check in add_id_to_freelist]
[v3: Redid op_name per Jan's suggestion]
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/block/xen-blkfront.c     |   58 +++++++++++++++++++++++++++++--------
 include/xen/interface/io/blkif.h |    6 ++++
 2 files changed, 51 insertions(+), 13 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 2f22874..ae8e3b7 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -141,14 +141,33 @@ static int get_id_from_freelist(struct blkfront_info *info)
 	return free;
 }
 
-static void add_id_to_freelist(struct blkfront_info *info,
+static int add_id_to_freelist(struct blkfront_info *info,
 			       unsigned long id)
 {
+	if (info->shadow[id].req.u.rw.id != id)
+		return -EINVAL;
+	if (info->shadow[id].request == NULL)
+		return -EINVAL;
 	info->shadow[id].req.u.rw.id  = info->shadow_free;
 	info->shadow[id].request = NULL;
 	info->shadow_free = id;
+	return 0;
 }
 
+static const char *op_name(int op)
+{
+	static const char *names[] = {
+		[BLKIF_OP_READ] = "read",
+		[BLKIF_OP_WRITE] = "write",
+		[BLKIF_OP_WRITE_BARRIER] = "barrier",
+		[BLKIF_OP_FLUSH_DISKCACHE] = "flush",
+		[BLKIF_OP_RESERVED_1] = "reserved",
+		[BLKIF_OP_DISCARD] = "discard" };
+
+	if (op < 0 || op >= ARRAY_SIZE(names))
+		return "unknown";
+	return names[op];
+}
 static int xlbd_reserve_minors(unsigned int minor, unsigned int nr)
 {
 	unsigned int end = minor + nr;
@@ -744,20 +763,31 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
 
 		bret = RING_GET_RESPONSE(&info->ring, i);
 		id   = bret->id;
+		/*
+		 * The backend has messed up and given us an id that we would
+		 * never have given to it (we stamp it up to BLK_RING_SIZE -
+		 * look in get_id_from_freelist.
+		 */
+		if (id >= BLK_RING_SIZE)
+			/* We can't safely get the 'struct request' as
+			 * the id is busted. So limp along. */
+			goto err_out;
+
 		req  = info->shadow[id].request;
 
 		if (bret->operation != BLKIF_OP_DISCARD)
 			blkif_completion(&info->shadow[id]);
 
-		add_id_to_freelist(info, id);
+		if (add_id_to_freelist(info, id))
+			goto err_out;
 
 		error = (bret->status == BLKIF_RSP_OKAY) ? 0 : -EIO;
 		switch (bret->operation) {
 		case BLKIF_OP_DISCARD:
 			if (unlikely(bret->status == BLKIF_RSP_EOPNOTSUPP)) {
 				struct request_queue *rq = info->rq;
-				printk(KERN_WARNING "blkfront: %s: discard op failed\n",
-					   info->gd->disk_name);
+				printk(KERN_WARNING "blkfront: %s: %s op failed\n",
+					   info->gd->disk_name, op_name(bret->operation));
 				error = -EOPNOTSUPP;
 				info->feature_discard = 0;
 				info->feature_secdiscard = 0;
@@ -769,18 +799,14 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
 		case BLKIF_OP_FLUSH_DISKCACHE:
 		case BLKIF_OP_WRITE_BARRIER:
 			if (unlikely(bret->status == BLKIF_RSP_EOPNOTSUPP)) {
-				printk(KERN_WARNING "blkfront: %s: write %s op failed\n",
-				       info->flush_op == BLKIF_OP_WRITE_BARRIER ?
-				       "barrier" :  "flush disk cache",
-				       info->gd->disk_name);
+				printk(KERN_WARNING "blkfront: %s: %s op failed\n",
+				       info->gd->disk_name, op_name(bret->operation));
 				error = -EOPNOTSUPP;
 			}
 			if (unlikely(bret->status == BLKIF_RSP_ERROR &&
 				     info->shadow[id].req.u.rw.nr_segments == 0)) {
-				printk(KERN_WARNING "blkfront: %s: empty write %s op failed\n",
-				       info->flush_op == BLKIF_OP_WRITE_BARRIER ?
-				       "barrier" :  "flush disk cache",
-				       info->gd->disk_name);
+				printk(KERN_WARNING "blkfront: %s: empty %s op failed\n",
+				       info->gd->disk_name, op_name(bret->operation));
 				error = -EOPNOTSUPP;
 			}
 			if (unlikely(error)) {
@@ -813,12 +839,18 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
 			goto again;
 	} else
 		info->ring.sring->rsp_event = i + 1;
-
 	kick_pending_request_queues(info);
 
 	spin_unlock_irqrestore(&blkif_io_lock, flags);
 
 	return IRQ_HANDLED;
+err_out:
+	WARN(1, "%s: response to %s has incorrect id\n",
+	     info->gd->disk_name, op_name(bret->operation));
+
+	spin_unlock_irqrestore(&info->io_lock, flags);
+
+	return IRQ_HANDLED;
 }
 
 
diff --git a/include/xen/interface/io/blkif.h b/include/xen/interface/io/blkif.h
index ee338bf..bc75c75 100644
--- a/include/xen/interface/io/blkif.h
+++ b/include/xen/interface/io/blkif.h
@@ -59,6 +59,12 @@ typedef uint64_t blkif_sector_t;
 #define BLKIF_OP_FLUSH_DISKCACHE   3
 
 /*
+ * Used in SLES sources for device specific command packet
+ * contained within the request. Reserved for that purpose.
+ */
+#define BLKIF_OP_RESERVED_1        4
+
+/*
  * Recognised only if "feature-discard" is present in backend xenbus info.
  * The "feature-discard" node contains a boolean indicating whether trim
  * (ATA) or unmap (SCSI) - conviently called discard requests are likely
-- 
1.7.7.6

Re: [Xen-devel] [PATCH 2/2] xen/blkfront: Add BUG_ON to deal with misbehaving backends.

[Jan Beulich]

>>> On 07.06.12 at 23:59, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -141,14 +141,33 @@ static int get_id_from_freelist(struct blkfront_info 
> *info)
>  	return free;
>  }
>  
> -static void add_id_to_freelist(struct blkfront_info *info,
> +static int add_id_to_freelist(struct blkfront_info *info,
>  			       unsigned long id)
>  {
> +	if (info->shadow[id].req.u.rw.id != id)
> +		return -EINVAL;
> +	if (info->shadow[id].request == NULL)
> +		return -EINVAL;
>  	info->shadow[id].req.u.rw.id  = info->shadow_free;
>  	info->shadow[id].request = NULL;
>  	info->shadow_free = id;
> +	return 0;
>  }
>  
> +static const char *op_name(int op)
> +{
> +	static const char *names[] = {

Could/should really be "static const char *const names[]".

> +		[BLKIF_OP_READ] = "read",
> +		[BLKIF_OP_WRITE] = "write",
> +		[BLKIF_OP_WRITE_BARRIER] = "barrier",
> +		[BLKIF_OP_FLUSH_DISKCACHE] = "flush",
> +		[BLKIF_OP_RESERVED_1] = "reserved",
> +		[BLKIF_OP_DISCARD] = "discard" };
> +
> +	if (op < 0 || op >= ARRAY_SIZE(names))
> +		return "unknown";
> +	return names[op];
> +}
>  static int xlbd_reserve_minors(unsigned int minor, unsigned int nr)
>  {
>  	unsigned int end = minor + nr;
> @@ -744,20 +763,31 @@ static irqreturn_t blkif_interrupt(int irq, void 
> *dev_id)
>  
>  		bret = RING_GET_RESPONSE(&info->ring, i);
>  		id   = bret->id;
> +		/*
> +		 * The backend has messed up and given us an id that we would
> +		 * never have given to it (we stamp it up to BLK_RING_SIZE -
> +		 * look in get_id_from_freelist.
> +		 */
> +		if (id >= BLK_RING_SIZE)
> +			/* We can't safely get the 'struct request' as
> +			 * the id is busted. So limp along. */
> +			goto err_out;

Getting out of the loop here isn't really what I'd call "limp along" -
it'd likely get the communication to a halt (if there are more
responses ready). I'd rather see this print something, and then
continue the loop. Or alternatively, if we really want to stop
communication in such a case, initiate tear down of the device.

> +
>  		req  = info->shadow[id].request;
>  
>  		if (bret->operation != BLKIF_OP_DISCARD)
>  			blkif_completion(&info->shadow[id]);
>  
> -		add_id_to_freelist(info, id);
> +		if (add_id_to_freelist(info, id))
> +			goto err_out;

Same here, obviously.

>  
>  		error = (bret->status == BLKIF_RSP_OKAY) ? 0 : -EIO;
>  		switch (bret->operation) {
>  		case BLKIF_OP_DISCARD:
>  			if (unlikely(bret->status == BLKIF_RSP_EOPNOTSUPP)) {
>  				struct request_queue *rq = info->rq;
> -				printk(KERN_WARNING "blkfront: %s: discard op failed\n",
> -					   info->gd->disk_name);
> +				printk(KERN_WARNING "blkfront: %s: %s op failed\n",
> +					   info->gd->disk_name, op_name(bret->operation));
>  				error = -EOPNOTSUPP;
>  				info->feature_discard = 0;
>  				info->feature_secdiscard = 0;
> @@ -769,18 +799,14 @@ static irqreturn_t blkif_interrupt(int irq, void 
> *dev_id)
>  		case BLKIF_OP_FLUSH_DISKCACHE:
>  		case BLKIF_OP_WRITE_BARRIER:
>  			if (unlikely(bret->status == BLKIF_RSP_EOPNOTSUPP)) {
> -				printk(KERN_WARNING "blkfront: %s: write %s op failed\n",
> -				       info->flush_op == BLKIF_OP_WRITE_BARRIER ?
> -				       "barrier" :  "flush disk cache",
> -				       info->gd->disk_name);
> +				printk(KERN_WARNING "blkfront: %s: %s op failed\n",
> +				       info->gd->disk_name, op_name(bret->operation));
>  				error = -EOPNOTSUPP;
>  			}
>  			if (unlikely(bret->status == BLKIF_RSP_ERROR &&
>  				     info->shadow[id].req.u.rw.nr_segments == 0)) {
> -				printk(KERN_WARNING "blkfront: %s: empty write %s op failed\n",
> -				       info->flush_op == BLKIF_OP_WRITE_BARRIER ?
> -				       "barrier" :  "flush disk cache",
> -				       info->gd->disk_name);
> +				printk(KERN_WARNING "blkfront: %s: empty %s op failed\n",
> +				       info->gd->disk_name, op_name(bret->operation));
>  				error = -EOPNOTSUPP;
>  			}
>  			if (unlikely(error)) {
> @@ -813,12 +839,18 @@ static irqreturn_t blkif_interrupt(int irq, void 
> *dev_id)
>  			goto again;
>  	} else
>  		info->ring.sring->rsp_event = i + 1;
> -
>  	kick_pending_request_queues(info);
>  
>  	spin_unlock_irqrestore(&blkif_io_lock, flags);
>  
>  	return IRQ_HANDLED;
> +err_out:
> +	WARN(1, "%s: response to %s has incorrect id\n",
> +	     info->gd->disk_name, op_name(bret->operation));
> +
> +	spin_unlock_irqrestore(&info->io_lock, flags);
> +
> +	return IRQ_HANDLED;
>  }
>  
>  
> diff --git a/include/xen/interface/io/blkif.h 
> b/include/xen/interface/io/blkif.h
> index ee338bf..bc75c75 100644
> --- a/include/xen/interface/io/blkif.h
> +++ b/include/xen/interface/io/blkif.h
> @@ -59,6 +59,12 @@ typedef uint64_t blkif_sector_t;
>  #define BLKIF_OP_FLUSH_DISKCACHE   3
>  
>  /*
> + * Used in SLES sources for device specific command packet
> + * contained within the request. Reserved for that purpose.
> + */
> +#define BLKIF_OP_RESERVED_1        4

If you really want to give this a numeric tag, wouldn't it be better
to make this _4? But maybe you could leave out the definition here
altogether, and leave the corresponding names[] slot set to NULL?

Jan

> +
> +/*
>   * Recognised only if "feature-discard" is present in backend xenbus info.
>   * The "feature-discard" node contains a boolean indicating whether trim
>   * (ATA) or unmap (SCSI) - conviently called discard requests are likely
> -- 
> 1.7.7.6

Re: [Xen-devel] [PATCH 2/2] xen/blkfront: Add BUG_ON to deal with misbehaving backends.

[Konrad Rzeszutek Wilk]

> > +static const char *op_name(int op)
> > +{
> > +	static const char *names[] = {
> 
> Could/should really be "static const char *const names[]".

It should. I forgot the extra 'const' 
> 
> > +		[BLKIF_OP_READ] = "read",
> > +		[BLKIF_OP_WRITE] = "write",
> > +		[BLKIF_OP_WRITE_BARRIER] = "barrier",
> > +		[BLKIF_OP_FLUSH_DISKCACHE] = "flush",
> > +		[BLKIF_OP_RESERVED_1] = "reserved",
> > +		[BLKIF_OP_DISCARD] = "discard" };
> > +
.. snip..
> > +		if (id >= BLK_RING_SIZE)
> > +			/* We can't safely get the 'struct request' as
> > +			 * the id is busted. So limp along. */
> > +			goto err_out;
> 
> Getting out of the loop here isn't really what I'd call "limp along" -
> it'd likely get the communication to a halt (if there are more
> responses ready). I'd rather see this print something, and then
> continue the loop. Or alternatively, if we really want to stop
> communication in such a case, initiate tear down of the device.
> 
> > +
> >  		req  = info->shadow[id].request;
> >  
> >  		if (bret->operation != BLKIF_OP_DISCARD)
> >  			blkif_completion(&info->shadow[id]);
> >  
> > -		add_id_to_freelist(info, id);
> > +		if (add_id_to_freelist(info, id))
> > +			goto err_out;
> 
> Same here, obviously.

<nods> Will do.
.. snip.k
> > +#define BLKIF_OP_RESERVED_1        4
> 
> If you really want to give this a numeric tag, wouldn't it be better
> to make this _4? But maybe you could leave out the definition here
> altogether, and leave the corresponding names[] slot set to NULL?

That would work too. Please see
>From 98e01250daadc66c1476e3d14603df69ff2a9e14 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Fri, 25 May 2012 17:34:51 -0400
Subject: [PATCH] xen/blkfront: Add WARN to deal with misbehaving backends.

Part of the ring structure is the 'id' field which is under
control of the frontend. The frontend stamps it with "some"
value (this some in this implementation being a value less
than BLK_RING_SIZE), and when it gets a response expects
said value to be in the response structure. We have a check
for the id field when spolling new requests but not when
de-spolling responses.

We also add an extra check in add_id_to_freelist to make
sure that the 'struct request' was not NULL - as we cannot
pass a NULL to __blk_end_request_all, otherwise that crashes
(and all the operations that the response is dealing with
end up with __blk_end_request_all).

Lastly we also print the name of the operation that failed.

[v1: s/BUG/WARN/ suggested by Stefano]
[v2: Add extra check in add_id_to_freelist]
[v3: Redid op_name per Jan's suggestion]
[v4: add const * and add WARN on failure returns]
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/block/xen-blkfront.c |   62 +++++++++++++++++++++++++++++++++--------
 1 files changed, 50 insertions(+), 12 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 2f22874..88510e4 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -141,14 +141,36 @@ static int get_id_from_freelist(struct blkfront_info *info)
 	return free;
 }
 
-static void add_id_to_freelist(struct blkfront_info *info,
+static int add_id_to_freelist(struct blkfront_info *info,
 			       unsigned long id)
 {
+	if (info->shadow[id].req.u.rw.id != id)
+		return -EINVAL;
+	if (info->shadow[id].request == NULL)
+		return -EINVAL;
 	info->shadow[id].req.u.rw.id  = info->shadow_free;
 	info->shadow[id].request = NULL;
 	info->shadow_free = id;
+	return 0;
 }
 
+static const char *op_name(int op)
+{
+	static const char *const names[] = {
+		[BLKIF_OP_READ] = "read",
+		[BLKIF_OP_WRITE] = "write",
+		[BLKIF_OP_WRITE_BARRIER] = "barrier",
+		[BLKIF_OP_FLUSH_DISKCACHE] = "flush",
+		[BLKIF_OP_DISCARD] = "discard" };
+
+	if (op < 0 || op >= ARRAY_SIZE(names))
+		return "unknown";
+
+	if (!names[op])
+		return "reserved";
+
+	return names[op];
+}
 static int xlbd_reserve_minors(unsigned int minor, unsigned int nr)
 {
 	unsigned int end = minor + nr;
@@ -744,20 +766,36 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
 
 		bret = RING_GET_RESPONSE(&info->ring, i);
 		id   = bret->id;
+		/*
+		 * The backend has messed up and given us an id that we would
+		 * never have given to it (we stamp it up to BLK_RING_SIZE -
+		 * look in get_id_from_freelist.
+		 */
+		if (id >= BLK_RING_SIZE) {
+			WARN(1, "%s: response to %s has incorrect id (%ld)\n",
+			     info->gd->disk_name, op_name(bret->operation), id);
+			/* We can't safely get the 'struct request' as
+			 * the id is busted. So quit. */
+			goto err_out;
+		}
 		req  = info->shadow[id].request;
 
 		if (bret->operation != BLKIF_OP_DISCARD)
 			blkif_completion(&info->shadow[id]);
 
-		add_id_to_freelist(info, id);
+		if (add_id_to_freelist(info, id)) {
+			WARN(1, "%s: response to %s (id %ld) couldn't be recycled!\n",
+			     info->gd->disk_name, op_name(bret->operation), id);
+			goto err_out;
+		}
 
 		error = (bret->status == BLKIF_RSP_OKAY) ? 0 : -EIO;
 		switch (bret->operation) {
 		case BLKIF_OP_DISCARD:
 			if (unlikely(bret->status == BLKIF_RSP_EOPNOTSUPP)) {
 				struct request_queue *rq = info->rq;
-				printk(KERN_WARNING "blkfront: %s: discard op failed\n",
-					   info->gd->disk_name);
+				printk(KERN_WARNING "blkfront: %s: %s op failed\n",
+					   info->gd->disk_name, op_name(bret->operation));
 				error = -EOPNOTSUPP;
 				info->feature_discard = 0;
 				info->feature_secdiscard = 0;
@@ -769,18 +807,14 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
 		case BLKIF_OP_FLUSH_DISKCACHE:
 		case BLKIF_OP_WRITE_BARRIER:
 			if (unlikely(bret->status == BLKIF_RSP_EOPNOTSUPP)) {
-				printk(KERN_WARNING "blkfront: %s: write %s op failed\n",
-				       info->flush_op == BLKIF_OP_WRITE_BARRIER ?
-				       "barrier" :  "flush disk cache",
-				       info->gd->disk_name);
+				printk(KERN_WARNING "blkfront: %s: %s op failed\n",
+				       info->gd->disk_name, op_name(bret->operation));
 				error = -EOPNOTSUPP;
 			}
 			if (unlikely(bret->status == BLKIF_RSP_ERROR &&
 				     info->shadow[id].req.u.rw.nr_segments == 0)) {
-				printk(KERN_WARNING "blkfront: %s: empty write %s op failed\n",
-				       info->flush_op == BLKIF_OP_WRITE_BARRIER ?
-				       "barrier" :  "flush disk cache",
-				       info->gd->disk_name);
+				printk(KERN_WARNING "blkfront: %s: empty %s op failed\n",
+				       info->gd->disk_name, op_name(bret->operation));
 				error = -EOPNOTSUPP;
 			}
 			if (unlikely(error)) {
@@ -819,6 +853,10 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
 	spin_unlock_irqrestore(&blkif_io_lock, flags);
 
 	return IRQ_HANDLED;
+err_out:
+	spin_unlock_irqrestore(&info->io_lock, flags);
+
+	return IRQ_HANDLED;
 }
 
 
-- 
1.7.7.6

Re: [Xen-devel] [PATCH 2/2] xen/blkfront: Add BUG_ON to deal with misbehaving backends.

[Konrad Rzeszutek Wilk]

> > Getting out of the loop here isn't really what I'd call "limp along" -
> > it'd likely get the communication to a halt (if there are more
> > responses ready). I'd rather see this print something, and then
> > continue the loop. Or alternatively, if we really want to stop
> > communication in such a case, initiate tear down of the device.

And this is what I get from reading without coffee.. This patch
does the 'continue' instead of tear down of the device. Long-term
tear-down is proper but that is a bit more complex.

>From 24a400057010ef280c3ebe33d1c4d74de43dcbea Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Fri, 25 May 2012 17:34:51 -0400
Subject: [PATCH] xen/blkfront: Add WARN to deal with misbehaving backends.

Part of the ring structure is the 'id' field which is under
control of the frontend. The frontend stamps it with "some"
value (this some in this implementation being a value less
than BLK_RING_SIZE), and when it gets a response expects
said value to be in the response structure. We have a check
for the id field when spolling new requests but not when
de-spolling responses.

We also add an extra check in add_id_to_freelist to make
sure that the 'struct request' was not NULL - as we cannot
pass a NULL to __blk_end_request_all, otherwise that crashes
(and all the operations that the response is dealing with
end up with __blk_end_request_all).

Lastly we also print the name of the operation that failed.

[v1: s/BUG/WARN/ suggested by Stefano]
[v2: Add extra check in add_id_to_freelist]
[v3: Redid op_name per Jan's suggestion]
[v4: add const * and add WARN on failure returns]
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/block/xen-blkfront.c |   58 +++++++++++++++++++++++++++++++++--------
 1 files changed, 46 insertions(+), 12 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 60eed4b..e4fb337 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -141,14 +141,36 @@ static int get_id_from_freelist(struct blkfront_info *info)
 	return free;
 }
 
-static void add_id_to_freelist(struct blkfront_info *info,
+static int add_id_to_freelist(struct blkfront_info *info,
 			       unsigned long id)
 {
+	if (info->shadow[id].req.u.rw.id != id)
+		return -EINVAL;
+	if (info->shadow[id].request == NULL)
+		return -EINVAL;
 	info->shadow[id].req.u.rw.id  = info->shadow_free;
 	info->shadow[id].request = NULL;
 	info->shadow_free = id;
+	return 0;
 }
 
+static const char *op_name(int op)
+{
+	static const char *const names[] = {
+		[BLKIF_OP_READ] = "read",
+		[BLKIF_OP_WRITE] = "write",
+		[BLKIF_OP_WRITE_BARRIER] = "barrier",
+		[BLKIF_OP_FLUSH_DISKCACHE] = "flush",
+		[BLKIF_OP_DISCARD] = "discard" };
+
+	if (op < 0 || op >= ARRAY_SIZE(names))
+		return "unknown";
+
+	if (!names[op])
+		return "reserved";
+
+	return names[op];
+}
 static int xlbd_reserve_minors(unsigned int minor, unsigned int nr)
 {
 	unsigned int end = minor + nr;
@@ -746,20 +768,36 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
 
 		bret = RING_GET_RESPONSE(&info->ring, i);
 		id   = bret->id;
+		/*
+		 * The backend has messed up and given us an id that we would
+		 * never have given to it (we stamp it up to BLK_RING_SIZE -
+		 * look in get_id_from_freelist.
+		 */
+		if (id >= BLK_RING_SIZE) {
+			WARN(1, "%s: response to %s has incorrect id (%ld)\n",
+			     info->gd->disk_name, op_name(bret->operation), id);
+			/* We can't safely get the 'struct request' as
+			 * the id is busted. */
+			continue;
+		}
 		req  = info->shadow[id].request;
 
 		if (bret->operation != BLKIF_OP_DISCARD)
 			blkif_completion(&info->shadow[id]);
 
-		add_id_to_freelist(info, id);
+		if (add_id_to_freelist(info, id)) {
+			WARN(1, "%s: response to %s (id %ld) couldn't be recycled!\n",
+			     info->gd->disk_name, op_name(bret->operation), id);
+			continue;
+		}
 
 		error = (bret->status == BLKIF_RSP_OKAY) ? 0 : -EIO;
 		switch (bret->operation) {
 		case BLKIF_OP_DISCARD:
 			if (unlikely(bret->status == BLKIF_RSP_EOPNOTSUPP)) {
 				struct request_queue *rq = info->rq;
-				printk(KERN_WARNING "blkfront: %s: discard op failed\n",
-					   info->gd->disk_name);
+				printk(KERN_WARNING "blkfront: %s: %s op failed\n",
+					   info->gd->disk_name, op_name(bret->operation));
 				error = -EOPNOTSUPP;
 				info->feature_discard = 0;
 				info->feature_secdiscard = 0;
@@ -771,18 +809,14 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
 		case BLKIF_OP_FLUSH_DISKCACHE:
 		case BLKIF_OP_WRITE_BARRIER:
 			if (unlikely(bret->status == BLKIF_RSP_EOPNOTSUPP)) {
-				printk(KERN_WARNING "blkfront: %s: write %s op failed\n",
-				       info->flush_op == BLKIF_OP_WRITE_BARRIER ?
-				       "barrier" :  "flush disk cache",
-				       info->gd->disk_name);
+				printk(KERN_WARNING "blkfront: %s: %s op failed\n",
+				       info->gd->disk_name, op_name(bret->operation));
 				error = -EOPNOTSUPP;
 			}
 			if (unlikely(bret->status == BLKIF_RSP_ERROR &&
 				     info->shadow[id].req.u.rw.nr_segments == 0)) {
-				printk(KERN_WARNING "blkfront: %s: empty write %s op failed\n",
-				       info->flush_op == BLKIF_OP_WRITE_BARRIER ?
-				       "barrier" :  "flush disk cache",
-				       info->gd->disk_name);
+				printk(KERN_WARNING "blkfront: %s: empty %s op failed\n",
+				       info->gd->disk_name, op_name(bret->operation));
 				error = -EOPNOTSUPP;
 			}
 			if (unlikely(error)) {
-- 
1.7.7.6

Re: [Xen-devel] [PATCH 2/2] xen/blkfront: Add BUG_ON to deal with misbehaving backends.

[Jan Beulich]

>>> On 11.06.12 at 20:52, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date: Fri, 25 May 2012 17:34:51 -0400
> Subject: [PATCH] xen/blkfront: Add WARN to deal with misbehaving backends.
> 
> Part of the ring structure is the 'id' field which is under
> control of the frontend. The frontend stamps it with "some"
> value (this some in this implementation being a value less
> than BLK_RING_SIZE), and when it gets a response expects
> said value to be in the response structure. We have a check
> for the id field when spolling new requests but not when
> de-spolling responses.
> 
> We also add an extra check in add_id_to_freelist to make
> sure that the 'struct request' was not NULL - as we cannot
> pass a NULL to __blk_end_request_all, otherwise that crashes
> (and all the operations that the response is dealing with
> end up with __blk_end_request_all).
> 
> Lastly we also print the name of the operation that failed.
> 
> [v1: s/BUG/WARN/ suggested by Stefano]
> [v2: Add extra check in add_id_to_freelist]
> [v3: Redid op_name per Jan's suggestion]
> [v4: add const * and add WARN on failure returns]
> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

> ---
>  drivers/block/xen-blkfront.c |   58 +++++++++++++++++++++++++++++++++--------
>  1 files changed, 46 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index 60eed4b..e4fb337 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -141,14 +141,36 @@ static int get_id_from_freelist(struct blkfront_info 
> *info)
>  	return free;
>  }
>  
> -static void add_id_to_freelist(struct blkfront_info *info,
> +static int add_id_to_freelist(struct blkfront_info *info,
>  			       unsigned long id)
>  {
> +	if (info->shadow[id].req.u.rw.id != id)
> +		return -EINVAL;
> +	if (info->shadow[id].request == NULL)
> +		return -EINVAL;
>  	info->shadow[id].req.u.rw.id  = info->shadow_free;
>  	info->shadow[id].request = NULL;
>  	info->shadow_free = id;
> +	return 0;
>  }
>  
> +static const char *op_name(int op)
> +{
> +	static const char *const names[] = {
> +		[BLKIF_OP_READ] = "read",
> +		[BLKIF_OP_WRITE] = "write",
> +		[BLKIF_OP_WRITE_BARRIER] = "barrier",
> +		[BLKIF_OP_FLUSH_DISKCACHE] = "flush",
> +		[BLKIF_OP_DISCARD] = "discard" };
> +
> +	if (op < 0 || op >= ARRAY_SIZE(names))
> +		return "unknown";
> +
> +	if (!names[op])
> +		return "reserved";
> +
> +	return names[op];
> +}
>  static int xlbd_reserve_minors(unsigned int minor, unsigned int nr)
>  {
>  	unsigned int end = minor + nr;
> @@ -746,20 +768,36 @@ static irqreturn_t blkif_interrupt(int irq, void 
> *dev_id)
>  
>  		bret = RING_GET_RESPONSE(&info->ring, i);
>  		id   = bret->id;
> +		/*
> +		 * The backend has messed up and given us an id that we would
> +		 * never have given to it (we stamp it up to BLK_RING_SIZE -
> +		 * look in get_id_from_freelist.
> +		 */
> +		if (id >= BLK_RING_SIZE) {
> +			WARN(1, "%s: response to %s has incorrect id (%ld)\n",
> +			     info->gd->disk_name, op_name(bret->operation), id);
> +			/* We can't safely get the 'struct request' as
> +			 * the id is busted. */
> +			continue;
> +		}
>  		req  = info->shadow[id].request;
>  
>  		if (bret->operation != BLKIF_OP_DISCARD)
>  			blkif_completion(&info->shadow[id]);
>  
> -		add_id_to_freelist(info, id);
> +		if (add_id_to_freelist(info, id)) {
> +			WARN(1, "%s: response to %s (id %ld) couldn't be recycled!\n",
> +			     info->gd->disk_name, op_name(bret->operation), id);
> +			continue;
> +		}
>  
>  		error = (bret->status == BLKIF_RSP_OKAY) ? 0 : -EIO;
>  		switch (bret->operation) {
>  		case BLKIF_OP_DISCARD:
>  			if (unlikely(bret->status == BLKIF_RSP_EOPNOTSUPP)) {
>  				struct request_queue *rq = info->rq;
> -				printk(KERN_WARNING "blkfront: %s: discard op failed\n",
> -					   info->gd->disk_name);
> +				printk(KERN_WARNING "blkfront: %s: %s op failed\n",
> +					   info->gd->disk_name, op_name(bret->operation));
>  				error = -EOPNOTSUPP;
>  				info->feature_discard = 0;
>  				info->feature_secdiscard = 0;
> @@ -771,18 +809,14 @@ static irqreturn_t blkif_interrupt(int irq, void 
> *dev_id)
>  		case BLKIF_OP_FLUSH_DISKCACHE:
>  		case BLKIF_OP_WRITE_BARRIER:
>  			if (unlikely(bret->status == BLKIF_RSP_EOPNOTSUPP)) {
> -				printk(KERN_WARNING "blkfront: %s: write %s op failed\n",
> -				       info->flush_op == BLKIF_OP_WRITE_BARRIER ?
> -				       "barrier" :  "flush disk cache",
> -				       info->gd->disk_name);
> +				printk(KERN_WARNING "blkfront: %s: %s op failed\n",
> +				       info->gd->disk_name, op_name(bret->operation));
>  				error = -EOPNOTSUPP;
>  			}
>  			if (unlikely(bret->status == BLKIF_RSP_ERROR &&
>  				     info->shadow[id].req.u.rw.nr_segments == 0)) {
> -				printk(KERN_WARNING "blkfront: %s: empty write %s op failed\n",
> -				       info->flush_op == BLKIF_OP_WRITE_BARRIER ?
> -				       "barrier" :  "flush disk cache",
> -				       info->gd->disk_name);
> +				printk(KERN_WARNING "blkfront: %s: empty %s op failed\n",
> +				       info->gd->disk_name, op_name(bret->operation));
>  				error = -EOPNOTSUPP;
>  			}
>  			if (unlikely(error)) {
> -- 
> 1.7.7.6

Re: [Xen-devel] [PATCH 2/2] xen/blkfront: Add BUG_ON to deal with misbehaving backends.

[Stefano Stabellini]

On Thu, 7 Jun 2012, Konrad Rzeszutek Wilk wrote:
> On Fri, Jun 01, 2012 at 11:16:29AM +0100, Stefano Stabellini wrote:
> > On Thu, 31 May 2012, Konrad Rzeszutek Wilk wrote:
> > > The blkfront_remove part is .. that is going to take some surgery to do
> > > and I don't think I am going to be able to attempt that within the next couple
> > > of weeks. So lets put that on the TODO list and just do this one?
> > 
> > OK
> > 
> > > >From 4aabb5b44778fc0c0b8d4f5a2e2cd8e8490064d7 Mon Sep 17 00:00:00 2001
> > > From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > > Date: Fri, 25 May 2012 17:34:51 -0400
> > > Subject: [PATCH] xen/blkfront: Add WARN to deal with misbehaving backends.
> > > 
> > > Part of the ring structure is the 'id' field which is under
> > > control of the frontend. The frontend stamps it with "some"
> > > value (this some in this implementation being a value less
> > > than BLK_RING_SIZE), and when it gets a response expects
> > > said value to be in the response structure. We have a check
> > > for the id field when spolling new requests but not when
> > > de-spolling responses.
> > > 
> > > We also add an extra check in add_id_to_freelist to make
> > > sure that the 'struct request' was not NULL - as we cannot
> > > pass a NULL to __blk_end_request_all, otherwise that crashes
> > > (and all the operations that the response is dealing with
> > > end up with __blk_end_request_all).
> > > 
> > > Lastly we also print the name of the operation that failed.
> > > 
> > > [v1: s/BUG/WARN/ suggested by Stefano]
> > > [v2: Add extra check in add_id_to_freelist]
> > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > > ---
> > >  drivers/block/xen-blkfront.c |   39 +++++++++++++++++++++++++++++----------
> > >  1 files changed, 29 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> > > index 60eed4b..c7ef8a4 100644
> > > --- a/drivers/block/xen-blkfront.c
> > > +++ b/drivers/block/xen-blkfront.c
> > > @@ -144,11 +144,22 @@ static int get_id_from_freelist(struct blkfront_info *info)
> > >  static void add_id_to_freelist(struct blkfront_info *info,
> > >  			       unsigned long id)
> > >  {
> > > +	BUG_ON(info->shadow[id].req.u.rw.id != id);
> > >  	info->shadow[id].req.u.rw.id  = info->shadow_free;
> > > +	BUG_ON(info->shadow[id].request == NULL);
> > >  	info->shadow[id].request = NULL;
> > >  	info->shadow_free = id;
> > >  }
> > 
> > Like Jan said, it would be best to change the two BUG_ON into WARN_ON
> > and return an error.
> 
> Yes. I missed that.
> > 
> > 
> > > +static const char *op_name(int op)
> > > +{
> > > +	const char *names[BLKIF_OP_DISCARD+1] = {
> > > +		"read" , "write", "barrier", "flush", "reserved", "discard"};
> > > +
> > > +	if (op > BLKIF_OP_DISCARD)
> > > +		return "unknown";
> > > +	return names[op];
> > > +}
> > 
> > Considering that op is an int, shoudn't we check for negative values
> > too?
> 
> Yes! I also converted this per Jan's excellent idea.
> 
> Please see:
> 
> 
> >From 3877611c3096423a7741e99e9c9b9e63a9f2e557 Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date: Fri, 25 May 2012 17:34:51 -0400
> Subject: [PATCH] xen/blkfront: Add WARN to deal with misbehaving backends.
> 
> Part of the ring structure is the 'id' field which is under
> control of the frontend. The frontend stamps it with "some"
> value (this some in this implementation being a value less
> than BLK_RING_SIZE), and when it gets a response expects
> said value to be in the response structure. We have a check
> for the id field when spolling new requests but not when
> de-spolling responses.
> 
> We also add an extra check in add_id_to_freelist to make
> sure that the 'struct request' was not NULL - as we cannot
> pass a NULL to __blk_end_request_all, otherwise that crashes
> (and all the operations that the response is dealing with
> end up with __blk_end_request_all).
> 
> Lastly we also print the name of the operation that failed.
> 
> [v1: s/BUG/WARN/ suggested by Stefano]
> [v2: Add extra check in add_id_to_freelist]
> [v3: Redid op_name per Jan's suggestion]
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

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


>  drivers/block/xen-blkfront.c     |   58 +++++++++++++++++++++++++++++--------
>  include/xen/interface/io/blkif.h |    6 ++++
>  2 files changed, 51 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index 2f22874..ae8e3b7 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -141,14 +141,33 @@ static int get_id_from_freelist(struct blkfront_info *info)
>  	return free;
>  }
>  
> -static void add_id_to_freelist(struct blkfront_info *info,
> +static int add_id_to_freelist(struct blkfront_info *info,
>  			       unsigned long id)
>  {
> +	if (info->shadow[id].req.u.rw.id != id)
> +		return -EINVAL;
> +	if (info->shadow[id].request == NULL)
> +		return -EINVAL;
>  	info->shadow[id].req.u.rw.id  = info->shadow_free;
>  	info->shadow[id].request = NULL;
>  	info->shadow_free = id;
> +	return 0;
>  }
>  
> +static const char *op_name(int op)
> +{
> +	static const char *names[] = {
> +		[BLKIF_OP_READ] = "read",
> +		[BLKIF_OP_WRITE] = "write",
> +		[BLKIF_OP_WRITE_BARRIER] = "barrier",
> +		[BLKIF_OP_FLUSH_DISKCACHE] = "flush",
> +		[BLKIF_OP_RESERVED_1] = "reserved",
> +		[BLKIF_OP_DISCARD] = "discard" };
> +
> +	if (op < 0 || op >= ARRAY_SIZE(names))
> +		return "unknown";
> +	return names[op];
> +}
>  static int xlbd_reserve_minors(unsigned int minor, unsigned int nr)
>  {
>  	unsigned int end = minor + nr;
> @@ -744,20 +763,31 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
>  
>  		bret = RING_GET_RESPONSE(&info->ring, i);
>  		id   = bret->id;
> +		/*
> +		 * The backend has messed up and given us an id that we would
> +		 * never have given to it (we stamp it up to BLK_RING_SIZE -
> +		 * look in get_id_from_freelist.
> +		 */
> +		if (id >= BLK_RING_SIZE)
> +			/* We can't safely get the 'struct request' as
> +			 * the id is busted. So limp along. */
> +			goto err_out;
> +
>  		req  = info->shadow[id].request;
>  
>  		if (bret->operation != BLKIF_OP_DISCARD)
>  			blkif_completion(&info->shadow[id]);
>  
> -		add_id_to_freelist(info, id);
> +		if (add_id_to_freelist(info, id))
> +			goto err_out;
>  
>  		error = (bret->status == BLKIF_RSP_OKAY) ? 0 : -EIO;
>  		switch (bret->operation) {
>  		case BLKIF_OP_DISCARD:
>  			if (unlikely(bret->status == BLKIF_RSP_EOPNOTSUPP)) {
>  				struct request_queue *rq = info->rq;
> -				printk(KERN_WARNING "blkfront: %s: discard op failed\n",
> -					   info->gd->disk_name);
> +				printk(KERN_WARNING "blkfront: %s: %s op failed\n",
> +					   info->gd->disk_name, op_name(bret->operation));
>  				error = -EOPNOTSUPP;
>  				info->feature_discard = 0;
>  				info->feature_secdiscard = 0;
> @@ -769,18 +799,14 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
>  		case BLKIF_OP_FLUSH_DISKCACHE:
>  		case BLKIF_OP_WRITE_BARRIER:
>  			if (unlikely(bret->status == BLKIF_RSP_EOPNOTSUPP)) {
> -				printk(KERN_WARNING "blkfront: %s: write %s op failed\n",
> -				       info->flush_op == BLKIF_OP_WRITE_BARRIER ?
> -				       "barrier" :  "flush disk cache",
> -				       info->gd->disk_name);
> +				printk(KERN_WARNING "blkfront: %s: %s op failed\n",
> +				       info->gd->disk_name, op_name(bret->operation));
>  				error = -EOPNOTSUPP;
>  			}
>  			if (unlikely(bret->status == BLKIF_RSP_ERROR &&
>  				     info->shadow[id].req.u.rw.nr_segments == 0)) {
> -				printk(KERN_WARNING "blkfront: %s: empty write %s op failed\n",
> -				       info->flush_op == BLKIF_OP_WRITE_BARRIER ?
> -				       "barrier" :  "flush disk cache",
> -				       info->gd->disk_name);
> +				printk(KERN_WARNING "blkfront: %s: empty %s op failed\n",
> +				       info->gd->disk_name, op_name(bret->operation));
>  				error = -EOPNOTSUPP;
>  			}
>  			if (unlikely(error)) {
> @@ -813,12 +839,18 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
>  			goto again;
>  	} else
>  		info->ring.sring->rsp_event = i + 1;
> -
>  	kick_pending_request_queues(info);
>  
>  	spin_unlock_irqrestore(&blkif_io_lock, flags);
>  
>  	return IRQ_HANDLED;
> +err_out:
> +	WARN(1, "%s: response to %s has incorrect id\n",
> +	     info->gd->disk_name, op_name(bret->operation));
> +
> +	spin_unlock_irqrestore(&info->io_lock, flags);
> +
> +	return IRQ_HANDLED;
>  }
>  
>  
> diff --git a/include/xen/interface/io/blkif.h b/include/xen/interface/io/blkif.h
> index ee338bf..bc75c75 100644
> --- a/include/xen/interface/io/blkif.h
> +++ b/include/xen/interface/io/blkif.h
> @@ -59,6 +59,12 @@ typedef uint64_t blkif_sector_t;
>  #define BLKIF_OP_FLUSH_DISKCACHE   3
>  
>  /*
> + * Used in SLES sources for device specific command packet
> + * contained within the request. Reserved for that purpose.
> + */
> +#define BLKIF_OP_RESERVED_1        4
> +
> +/*
>   * Recognised only if "feature-discard" is present in backend xenbus info.
>   * The "feature-discard" node contains a boolean indicating whether trim
>   * (ATA) or unmap (SCSI) - conviently called discard requests are likely
> -- 
> 1.7.7.6
>