> -----Original Message-----
> From: Xen-devel <xen-devel-boun...@lists.xenproject.org> On Behalf Of Dongli
> Zhang
> Sent: 30 January 2021 05:09
> To: p...@xen.org; 'Jürgen Groß' <jgr...@suse.com>;
> xen-devel@lists.xenproject.org; linux-
> bl...@vger.kernel.org; linux-ker...@vger.kernel.org
> Cc: 'Paul Durrant' <pdurr...@amazon.com>; 'Konrad Rzeszutek Wilk'
> <konrad.w...@oracle.com>; 'Roger Pau
> Monné' <roger....@citrix.com>; 'Jens Axboe' <ax...@kernel.dk>
> 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ß <jgr...@suse.com>
> >> Sent: 29 January 2021 07:35
> >> To: Dongli Zhang <dongli.zh...@oracle.com>; Paul Durrant <p...@xen.org>;
> >> xen-
> >> de...@lists.xenproject.org; linux-bl...@vger.kernel.org;
> >> linux-ker...@vger.kernel.org
> >> Cc: Paul Durrant <pdurr...@amazon.com>; Konrad Rzeszutek Wilk
> >> <konrad.w...@oracle.com>; Roger Pau
> >> Monné <roger....@citrix.com>; Jens Axboe <ax...@kernel.dk>
> >> 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 <pdurr...@amazon.com>
> >>>>
> >>>> 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 <pdurr...@amazon.com>
> >>>> ---
> >>>> Cc: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
> >>>> Cc: "Roger Pau Monné" <roger....@citrix.com>
> >>>> Cc: Jens Axboe <ax...@kernel.dk>
> >>>> Cc: Dongli Zhang <dongli.zh...@oracle.com>
> >>>>
> >>>> 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.zh...@oracle.com>
>
Thanks.
Roger, Konrad, can I get a maintainer ack or otherwise, please?
Paul
