[Xen-changelog] [linux-2.6.18-xen] blkfront: add logic to deal with misbehaving backends

[Xen patchbot-linux-2.6.18-xen <patchbot@xxxxxxx>]

# HG changeset patch
# User Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
# Date 1339505813 -7200
# Node ID 607eb9bebe75b171c790eb104fbcc33a47d7aa35
# Parent  b72a53f5d7563ac9b1d8e13723c5a5bc9b08e19d
blkfront: add logic 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 it 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
end_that_request_{first,last}(), otherwise they crash (and all the
operations that the response is dealing with end up calling these
functions).

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

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Acked-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
Committed-by: Jan Beulich <jbeulich@xxxxxxxx>
---


diff -r b72a53f5d756 -r 607eb9bebe75 drivers/xen/blkfront/blkfront.c
--- a/drivers/xen/blkfront/blkfront.c   Mon Jun 04 10:55:00 2012 +0200
+++ b/drivers/xen/blkfront/blkfront.c   Tue Jun 12 14:56:53 2012 +0200
@@ -453,12 +453,33 @@ static inline int GET_ID_FROM_FREELIST(
        return free;
 }
 
-static inline void ADD_ID_TO_FREELIST(
+static inline int ADD_ID_TO_FREELIST(
        struct blkfront_info *info, unsigned long id)
 {
+       if (info->shadow[id].req.id != id)
+               return -EINVAL;
+       if (!info->shadow[id].request)
+               return -ENXIO;
        info->shadow[id].req.id  = info->shadow_free;
        info->shadow[id].request = 0;
        info->shadow_free = id;
+       return 0;
+}
+
+static const char *op_name(unsigned 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 >= ARRAY_SIZE(names))
+               return "unknown";
+
+       return names[op] ?: "reserved";
 }
 
 static inline void flush_requests(struct blkfront_info *info)
@@ -772,19 +793,40 @@ static irqreturn_t blkif_int(int irq, vo
                int ret;
 
                bret = RING_GET_RESPONSE(&info->ring, i);
+               if (unlikely(bret->id >= BLK_RING_SIZE)) {
+                       /*
+                        * 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 - see GET_ID_FROM_FREELIST()).
+                        */
+                       printk(KERN_WARNING
+                              "%s: response to %s has incorrect id (%#Lx)\n",
+                              info->gd->disk_name, op_name(bret->operation),
+                              (unsigned long long)bret->id);
+                       continue;
+               }
                id   = bret->id;
                req  = (struct request *)info->shadow[id].request;
 
                blkif_completion(&info->shadow[id]);
 
-               ADD_ID_TO_FREELIST(info, id);
+               ret = ADD_ID_TO_FREELIST(info, id);
+               if (unlikely(ret)) {
+                       printk(KERN_WARNING
+                              "%s: id %#lx (response to %s) couldn't be 
recycled (%d)!\n",
+                              info->gd->disk_name, id,
+                              op_name(bret->operation), ret);
+                       continue;
+               }
 
                uptodate = (bret->status == BLKIF_RSP_OKAY);
                switch (bret->operation) {
                case BLKIF_OP_WRITE_BARRIER:
                        if (unlikely(bret->status == BLKIF_RSP_EOPNOTSUPP)) {
-                               printk("blkfront: %s: write barrier op 
failed\n",
-                                      info->gd->disk_name);
+                               printk(KERN_WARNING
+                                      "blkfront: %s: %s op failed\n",
+                                      info->gd->disk_name,
+                                      op_name(bret->operation));
                                uptodate = -EOPNOTSUPP;
                                info->feature_barrier = 0;
                                xlvbd_barrier(info);
@@ -793,8 +835,9 @@ static irqreturn_t blkif_int(int irq, vo
                case BLKIF_OP_READ:
                case BLKIF_OP_WRITE:
                        if (unlikely(bret->status != BLKIF_RSP_OKAY))
-                               DPRINTK("Bad return from blkdev data "
-                                       "request: %x\n", bret->status);
+                               DPRINTK("Bad return from blkdev %s "
+                                       "request: %x\n", bret->status,
+                                       op_name(bret->operation));
 
                        ret = end_that_request_first(req, uptodate,
                                req->hard_nr_sectors);

_______________________________________________
Xen-changelog mailing list
Xen-changelog@xxxxxxxxxxxxx
http://lists.xensource.com/xen-changelog