Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758279Ab2EaSYS (ORCPT ); Thu, 31 May 2012 14:24:18 -0400 Received: from rcsinet15.oracle.com ([148.87.113.117]:43020 "EHLO rcsinet15.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757174Ab2EaSYQ (ORCPT ); Thu, 31 May 2012 14:24:16 -0400 Date: Thu, 31 May 2012 14:17:12 -0400 From: Konrad Rzeszutek Wilk To: Jan Beulich Cc: Stefano Stabellini , "axboe@kernel.dk" , "xen-devel@lists.xensource.com" , "linux-kernel@vger.kernel.org" Subject: Re: [Xen-devel] [PATCH 2/2] xen/blkfront: Add BUG_ON to deal with misbehaving backends. Message-ID: <20120531181712.GA19700@phenom.dumpdata.com> References: <1337982603-15692-1-git-send-email-konrad.wilk@oracle.com> <1337982603-15692-3-git-send-email-konrad.wilk@oracle.com> <4FC4A10F0200007800086842@nat28.tlf.novell.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4FC4A10F0200007800086842@nat28.tlf.novell.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Source-IP: ucsinet21.oracle.com [156.151.31.93] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6653 Lines: 172 > >> @@ -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 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 --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/