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

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----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