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

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of Dongli 
> Zhang
> Sent: 30 January 2021 05:09
> To: paul@xxxxxxx; 'Jürgen Groß' <jgross@xxxxxxxx>; 
> 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 1/29/21 12:13 AM, Paul Durrant wrote:
> >> -----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.
> >
> 
> Thank you very much for the explanation!
> 
> Reviewed-by: Dongli Zhang <dongli.zhang@xxxxxxxxxx>
> 

Thanks.

Roger, Konrad, can I get a maintainer ack or otherwise, please?

  Paul