Received-SPF: pass (google.com: domain of linux-kernel+bounces-35511-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Tue, 23 Jan 2024 10:09:24 -0500
From: Stefan Hajnoczi <stefanha@redhat.com>
To: mst@redhat.com
Cc: Yi Sun <yi.sun@unisoc.com>, axboe@kernel.dk,
	yi sun <sunyibuaa@gmail.com>, jasowang@redhat.com,
	xuanzhuo@linux.alibaba.com, pbonzini@redhat.com,
	virtualization@lists.linux.dev, linux-block@vger.kernel.org,
	linux-kernel@vger.kernel.org, zhiguo.niu@unisoc.com,
	hongyu.jin@unisoc.com
Subject: Re: [PATCH 2/2] virtio-blk: Ensure no requests in virtqueues before
 deleting vqs.
Message-ID: <20240123150924.GD484337@fedora>
References: <20240122110722.690223-1-yi.sun@unisoc.com>
 <20240122110722.690223-3-yi.sun@unisoc.com>
 <20240122154255.GA389442@fedora>
 <CALpufv0h-sQ4Qfp-Sxd7wME4onMNAMop_gi-np6Dk2R96sba0Q@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="5XZvsWUA6IsHlc+K"
Content-Disposition: inline
In-Reply-To: <CALpufv0h-sQ4Qfp-Sxd7wME4onMNAMop_gi-np6Dk2R96sba0Q@mail.gmail.com>


--5XZvsWUA6IsHlc+K
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hi Michael,
This could potentially affect other VIRTIO drivers too. Please see
below.

On Tue, Jan 23, 2024 at 11:27:40AM +0800, yi sun wrote:
> On Mon, Jan 22, 2024 at 11:43=E2=80=AFPM Stefan Hajnoczi <stefanha@redhat=
=2Ecom> wrote:
> >
> > On Mon, Jan 22, 2024 at 07:07:22PM +0800, Yi Sun wrote:
> > > Ensure no remaining requests in virtqueues before resetting vdev and
> > > deleting virtqueues. Otherwise these requests will never be completed.
> > > It may cause the system to become unresponsive. So it is better to pl=
ace
> > > blk_mq_quiesce_queue() in front of virtio_reset_device().
> >
> > QEMU's virtio-blk device implementation completes all requests during
> > device reset. Most device implementations have to do the same to avoid
> > leaving dangling requests in flight across device reset.
> >
> > Which device implementation are you using and why is it safe for the
> > device is simply drop requests across device reset?
> >
> > Stefan
>=20
> Virtio-blk device implementation completes all requests during device res=
et, but
> this can only ensure that the device has finished using virtqueue. We sho=
uld
> also consider the driver's use of virtqueue.
>=20
> I caught such an example. Before del_vqs, the request had been processed =
by
> the device, but it had not been processed by the driver. Although I am
> using kernel5.4,
> I think this problem may also occur with the latest version of kernel.
>=20
> The debug code I added is as follows:
> virtblk_freeze()
> {
>         vdev reset();
>         quiesce queue();
>         if (virtqueue->num_free !=3D 1024) //1024 is the init value.
>                 BUG_ON(1);
>         vdev del_vqs();
> }
>=20
> BUG_ON triggered the dump, the analysis is as follows:
>=20
> There is one request left in request_queue.
> crash_arm64> struct request ffffff81f0560000 | grep -e state -e __data_len
>   __data_len =3D 20480,
>   state =3D MQ_RQ_IN_FLIGHT,
>=20
> crash_arm64> vring_virtqueue.packed,last_used_idx,broken,vq 0xffffff8086f=
92900 |
> grep -e num -e used_wrap_counter -e last_used_idx -e broken -e
> num_free -e desc_state -e "desc =3D"
>         num =3D 1024,
>         desc =3D 0xffffff8085ff8000,
>       used_wrap_counter =3D false,
>       desc_state =3D 0xffffff8085610000,
>   last_used_idx =3D 487,
>   broken =3D false,
>     num_free =3D 1017,
>=20
> Find desc based on last_used_idx. Through flags, we can know that the req=
uest
> has been processed by the device, but it is still in flight state
> because it has not
> had time to run virtblk_done().
> crash_arm> vring_packed_desc ffffff8085ff9e70
> struct vring_packed_desc {
>   addr =3D 10474619192,
>   len =3D 20481,
>   id =3D 667,
>   flags =3D 2
> }
>=20
> I'm using a closed source virtual machine, so I can't see the source
> code for it,
> but I'm guessing it's similar to qemu.
>=20
> After the device completes the request, we must also ensure that the driv=
er can
> complete the request in virtblk_done().
>=20

Okay, I think your approach of waiting for requests before
virtio_device_reset() makes sense. blk_mq_complete_request() is async
(might be deferred to an IPI or softirq) so it's not enough for
virtblk_done() to run before virtio_device_reset() returns. There is no
guarantee that virtblk_request_done() runs before virtio_device_reset()
returns.

A separate issue about virtio_device_reset():

Are you using virtio-mmio? virtio-mmio's vm_reset() doesn't offer the
same guarantees as virtio-pci's reset functions. virtio-pci guarantees
that irqs sent by the device during the reset operation complete and the
irq handlers run before virtio_device_reset() returns. virtio-mmio does
not.

(On top of this, virtio_device_reset() now has
CONFIG_VIRTIO_HARDEN_NOTIFICATION which changes the semantics. Drivers
cannot expect to complete any in-flight requests in
virtio_device_reset() when complied with this config option.)

Other drivers may be affected by these inconsistent
virtio_device_reset() semantics. I haven't audited the code.

Stefan

--5XZvsWUA6IsHlc+K
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEEhpWov9P5fNqsNXdanKSrs4Grc8gFAmWv1qQACgkQnKSrs4Gr
c8jpgAf+NXHJgW/fRiaSNygOJ4hNh3a2hy8o6bZssqK9v3TAl8S3iIWCBF+IWuCA
apnBdRbOu6nWpp9DtMjEwKzt3JAzdwRGkEpK/PbGP53Y07KihDTeJU6FhssuUxLn
idP+j15guyV/m4+wVQJgNR6c3k9rFc+P4pH97CcxkLH7SX0uUA5dMQEBUgBN3sVb
V+nx/vm0jTUe3BJ3mJaUzB5G4iECMAeaKTLQQj3fOoKMJ1vt8+HQGTLeHaHL9pOW
3qeHUov5iMlp2CZpMtn0DDjqs3mX9uj3uhXGHW95BfkU1QRVHVWQypHGHE5xzAmq
PgAw13jcz8svFhI7lztiQBHmJ5N9ug==
=cz+B
-----END PGP SIGNATURE-----

--5XZvsWUA6IsHlc+K--