[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] RE: [PATCH] xen-blkback: fix compatibility bug with single page rings
> -----Original Message----- > From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of Dongli > Zhang > Sent: 27 January 2021 19:57 > To: Paul Durrant <paul@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx; > linux-block@xxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx > Cc: Paul Durrant <pdurrant@xxxxxxxxxx>; Konrad Rzeszutek Wilk > <konrad.wilk@xxxxxxxxxx>; Roger Pau > Monné <roger.pau@xxxxxxxxxx>; Jens Axboe <axboe@xxxxxxxxx> > Subject: Re: [PATCH] xen-blkback: fix compatibility bug with single page rings > > > > On 1/27/21 2:30 AM, Paul Durrant wrote: > > From: Paul Durrant <pdurrant@xxxxxxxxxx> > > > > Prior to commit 4a8c31a1c6f5 ("xen/blkback: rework connect_ring() to avoid > > inconsistent xenstore 'ring-page-order' set by malicious blkfront"), the > > behaviour of xen-blkback when connecting to a frontend was: > > > > - read 'ring-page-order' > > - if not present then expect a single page ring specified by 'ring-ref' > > - else expect a ring specified by 'ring-refX' where X is between 0 and > > 1 << ring-page-order > > > > This was correct behaviour, but was broken by the afforementioned commit to > > become: > > > > - read 'ring-page-order' > > - if not present then expect a single page ring > > - expect a ring specified by 'ring-refX' where X is between 0 and > > 1 << ring-page-order > > - if that didn't work then see if there's a single page ring specified by > > 'ring-ref' > > > > This incorrect behaviour works most of the time but fails when a frontend > > that sets 'ring-page-order' is unloaded and replaced by one that does not > > because, instead of reading 'ring-ref', xen-blkback will read the stale > > 'ring-ref0' left around by the previous frontend will try to map the wrong > > grant reference. > > > > This patch restores the original behaviour. > > > > Fixes: 4a8c31a1c6f5 ("xen/blkback: rework connect_ring() to avoid > > inconsistent xenstore 'ring-page- > order' set by malicious blkfront") > > Signed-off-by: Paul Durrant <pdurrant@xxxxxxxxxx> > > --- > > Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> > > Cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx> > > Cc: Jens Axboe <axboe@xxxxxxxxx> > > Cc: Dongli Zhang <dongli.zhang@xxxxxxxxxx> > > --- > > drivers/block/xen-blkback/common.h | 1 + > > drivers/block/xen-blkback/xenbus.c | 36 +++++++++++++----------------- > > 2 files changed, 17 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/block/xen-blkback/common.h > > b/drivers/block/xen-blkback/common.h > > index b0c71d3a81a0..524a79f10de6 100644 > > --- a/drivers/block/xen-blkback/common.h > > +++ b/drivers/block/xen-blkback/common.h > > @@ -313,6 +313,7 @@ struct xen_blkif { > > > > struct work_struct free_work; > > unsigned int nr_ring_pages; > > + bool multi_ref; > > /* All rings for this device. */ > > struct xen_blkif_ring *rings; > > unsigned int nr_rings; > > diff --git a/drivers/block/xen-blkback/xenbus.c > > b/drivers/block/xen-blkback/xenbus.c > > index 9860d4842f36..4c1541cde68c 100644 > > --- a/drivers/block/xen-blkback/xenbus.c > > +++ b/drivers/block/xen-blkback/xenbus.c > > @@ -998,10 +998,15 @@ static int read_per_ring_refs(struct xen_blkif_ring > > *ring, const char *dir) > > for (i = 0; i < nr_grefs; i++) { > > char ring_ref_name[RINGREF_NAME_LEN]; > > > > - snprintf(ring_ref_name, RINGREF_NAME_LEN, "ring-ref%u", i); > > + if (blkif->multi_ref) > > + snprintf(ring_ref_name, RINGREF_NAME_LEN, "ring-ref%u", > > i); > > + else { > > + WARN_ON(i != 0); > > + snprintf(ring_ref_name, RINGREF_NAME_LEN, "ring-ref"); > > + } > > + > > err = xenbus_scanf(XBT_NIL, dir, ring_ref_name, > > "%u", &ring_ref[i]); > > - > > if (err != 1) { > > if (nr_grefs == 1) > > break; > > I think we should not simply break here because the failure can be due to when > (nr_grefs == 1) and reading from legacy "ring-ref". > Yes, you're quite right. This special case is no longer correct. > Should we do something as below? > > err = -EINVAL; > xenbus_dev_fatal(dev, err, "reading %s/ring-ref", dir); > return err; > I think simply removing the 'if (nr_grefs == 1)' will be sufficient. Paul > Dongli Zhang > > > > @@ -1013,18 +1018,6 @@ static int read_per_ring_refs(struct xen_blkif_ring > > *ring, const char *dir) > > } > > } > > > > - if (err != 1) { > > - WARN_ON(nr_grefs != 1); > > - > > - err = xenbus_scanf(XBT_NIL, dir, "ring-ref", "%u", > > - &ring_ref[0]); > > - if (err != 1) { > > - err = -EINVAL; > > - xenbus_dev_fatal(dev, err, "reading %s/ring-ref", dir); > > - return err; > > - } > > - } > > - > > err = -ENOMEM; > > for (i = 0; i < nr_grefs * XEN_BLKIF_REQS_PER_PAGE; i++) { > > req = kzalloc(sizeof(*req), GFP_KERNEL); > > @@ -1129,10 +1122,15 @@ static int connect_ring(struct backend_info *be) > > blkif->nr_rings, blkif->blk_protocol, protocol, > > blkif->vbd.feature_gnt_persistent ? "persistent grants" : ""); > > > > - ring_page_order = xenbus_read_unsigned(dev->otherend, > > - "ring-page-order", 0); > > - > > - if (ring_page_order > xen_blkif_max_ring_order) { > > + err = xenbus_scanf(XBT_NIL, dev->otherend, "ring-page-order", "%u", > > + &ring_page_order); > > + if (err != 1) { > > + blkif->nr_ring_pages = 1; > > + blkif->multi_ref = false; > > + } else if (ring_page_order <= xen_blkif_max_ring_order) { > > + blkif->nr_ring_pages = 1 << ring_page_order; > > + blkif->multi_ref = true; > > + } else { > > err = -EINVAL; > > xenbus_dev_fatal(dev, err, > > "requested ring page order %d exceed max:%d", > > @@ -1141,8 +1139,6 @@ static int connect_ring(struct backend_info *be) > > return err; > > } > > > > - blkif->nr_ring_pages = 1 << ring_page_order; > > - > > if (blkif->nr_rings == 1) > > return read_per_ring_refs(&blkif->rings[0], dev->otherend); > > else { > >
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |