Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Paul Durrant <xadimgnik@gmail.com>
Reply-To: <paul@xen.org>
To:     "'Jan Beulich'" <jbeulich@suse.com>
Cc:     "'Paul Durrant'" <pdurrant@amazon.com>,
        "'Konrad Rzeszutek Wilk'" <konrad.wilk@oracle.com>,
        =?utf-8?Q?'Roger_Pau_Monn=C3=A9'?= <roger.pau@citrix.com>,
        "'Jens Axboe'" <axboe@kernel.dk>,
        "'Dongli Zhang'" <dongli.zhang@oracle.com>,
        <linux-kernel@vger.kernel.org>, <linux-block@vger.kernel.org>,
        <xen-devel@lists.xenproject.org>
References: <20210127103034.2559-1-paul@xen.org> <cd70ae5e-a389-7521-8caf-15650a276152@suse.com> <026001d6f49c$eab982b0$c02c8810$@xen.org> <ed1988d9-131a-daf1-787f-3f49269b91aa@suse.com>
In-Reply-To: <ed1988d9-131a-daf1-787f-3f49269b91aa@suse.com>
Subject: RE: [PATCH] xen-blkback: fix compatibility bug with single page rings
Date:   Wed, 27 Jan 2021 11:33:05 -0000
Message-ID: <026101d6f4a0$2e3de000$8ab9a000$@xen.org>
MIME-Version: 1.0
Content-Type: text/plain;
        charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Thread-Index: AQH8tEXaaRm8hTZRIcgVeZub79k21gHW+9qTAdEPDm8C4a9si6m7hw/A
Content-Language: en-gb
Precedence: bulk

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 27 January 2021 11:21
> To: paul@xen.org
> Cc: 'Paul Durrant' <pdurrant@amazon.com>; 'Konrad Rzeszutek Wilk' =
<konrad.wilk@oracle.com>; 'Roger Pau
> Monn=C3=A9' <roger.pau@citrix.com>; 'Jens Axboe' <axboe@kernel.dk>; =
'Dongli Zhang'
> <dongli.zhang@oracle.com>; linux-kernel@vger.kernel.org; =
linux-block@vger.kernel.org; xen-
> devel@lists.xenproject.org
> Subject: Re: [PATCH] xen-blkback: fix compatibility bug with single =
page rings
>=20
> On 27.01.2021 12:09, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: 27 January 2021 10:57
> >> To: Paul Durrant <paul@xen.org>
> >> Cc: Paul Durrant <pdurrant@amazon.com>; Konrad Rzeszutek Wilk =
<konrad.wilk@oracle.com>; Roger Pau
> >> Monn=C3=A9 <roger.pau@citrix.com>; Jens Axboe <axboe@kernel.dk>; =
Dongli Zhang <dongli.zhang@oracle.com>;
> >> linux-kernel@vger.kernel.org; linux-block@vger.kernel.org; =
xen-devel@lists.xenproject.org
> >> Subject: Re: [PATCH] xen-blkback: fix compatibility bug with single =
page rings
> >>
> >> On 27.01.2021 11:30, Paul Durrant wrote:
> >>> From: Paul Durrant <pdurrant@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
> >>> - 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.
> >>
> >> Isn't this only the 2nd of a pair of fixes that's needed, the
> >> first being the drivers, upon being unloaded, to fully clean up
> >> after itself? Any stale key left may lead to confusion upon
> >> re-use of the containing directory.
> >
> > In a backend we shouldn't be relying on, nor really expect IMO, a =
frontend to clean up after itself.
> Any backend should know *exactly* what xenstore nodes it=E2=80=99s =
looking for from a frontend.
>=20
> But the backend can't know whether a node exists because the present
> frontend has written it, or because an earlier instance forgot to
> delete it. It can only honor what's there. (In fact the other day I
> was wondering whether some of the writes of boolean "false" nodes
> wouldn't better be xenbus_rm() instead.)

In the particular case this patch is fixing for me, the frontends are =
the Windows XENVBD driver and the Windows crash version of the same =
driver (actually built from different code). The 'normal' instance is =
multi-page aware and the crash instance is not quite, i.e. it uses the =
old ring-ref but knows to clean up 'ring-page-order'.
Clearly, in a crash situation, we cannot rely on frontend to clean up so =
what you say does highlight that there indeed needs to be a second patch =
to xen-blkback to make sure it removes 'ring-page-order' itself as =
'state' cycles through Closed and back to InitWait. I think this patch =
does still stand on its own though.

  Paul

>=20
> Jan