RE: [PATCH v2] xen-blkback: fix compatibility bug with single page rings

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Sent: 02 February 2021 16:29
> To: Paul Durrant <paul@xxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; linux-block@xxxxxxxxxxxxxxx; 
> linux-kernel@xxxxxxxxxxxxxxx; Paul
> Durrant <pdurrant@xxxxxxxxxx>; Konrad Rzeszutek Wilk 
> <konrad.wilk@xxxxxxxxxx>; Jens Axboe
> <axboe@xxxxxxxxx>; Dongli Zhang <dongli.zhang@xxxxxxxxxx>
> Subject: Re: [PATCH v2] xen-blkback: fix compatibility bug with single page 
> rings
> 
> On Thu, Jan 28, 2021 at 01:04:41PM +0000, 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;
> 
> You seem to have used spaces between the type and the variable name
> here, while neighbors also use hard tabs.
> 

Oops. Xen vs. Linux coding style :-( I'll send a v3 with the whitespace fixed.

> The rest LGTM:
> 
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> We should have forbidden the usage of ring-page-order = 0 and we could
> have avoided having to add the multi_ref variable, but that's too late
> now.

Thanks. Yes, that cat is out of the bag and has been for a while unfortunately.

  Paul

> 
> Thanks, Roger.