[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] RE: [PATCH v2] xen-blkback: fix compatibility bug with single page rings
> -----Original Message----- > From: Jürgen Groß <jgross@xxxxxxxx> > Sent: 29 January 2021 07:35 > To: Dongli Zhang <dongli.zhang@xxxxxxxxxx>; 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 v2] xen-blkback: fix compatibility bug with single page > rings > > On 29.01.21 07:20, Dongli Zhang wrote: > > > > > > On 1/28/21 5:04 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 (i.e. ring-page-order = 0) > >> - 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> > >> > >> v2: > >> - Remove now-spurious error path special-case when nr_grefs == 1 > >> --- > >> drivers/block/xen-blkback/common.h | 1 + > >> drivers/block/xen-blkback/xenbus.c | 38 +++++++++++++----------------- > >> 2 files changed, 17 insertions(+), 22 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; > > > > Is it really necessary to introduce 'multi_ref' here or we may just re-use > > 'nr_ring_pages'? > > > > According to blkfront code, 'ring-page-order' is set only when it is not > > zero, > > that is, only when (info->nr_ring_pages > 1). > That's how it is *supposed* to be. Windows certainly behaves that way too. > Did you look into all other OS's (Windows, OpenBSD, FreebSD, NetBSD, > Solaris, Netware, other proprietary systems) implementations to verify > that claim? > > I don't think so. So better safe than sorry. > Indeed. It was unfortunate that the commit to blkif.h documenting multi-page (829f2a9c6dfae) was not crystal clear and (possibly as a consequence) blkback was implemented to read ring-ref0 rather than ring-ref if ring-page-order was present and 0. Hence the only safe thing to do is to restore that behaviour. Paul > > Juergen
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |