[PATCH v2 2/2] virtio: acknowledge all features before access

[PATCH v2 2/2] virtio: acknowledge all features before access

[Michael S. Tsirkin]

The feature negotiation was designed in a way that
makes it possible for devices to know which config
fields will be accessed by drivers.

This is broken since commit 404123c2db79 ("virtio: allow drivers to
validate features") with fallout in at least block and net.  We have a
partial work-around in commit 2f9a174f918e ("virtio: write back
F_VERSION_1 before validate") which at least lets devices find out which
format should config space have, but this is a partial fix: guests
should not access config space without acknowledging features since
otherwise we'll never be able to change the config space format.

To fix, split finalize_features from virtio_finalize_features and
call finalize_features with all feature bits before validation,
and then - if validation changed any bits - once again after.

Since virtio_finalize_features no longer writes out features
rename it to virtio_features_ok - since that is what it does:
checks that features are ok with the device.

As a side effect, this also reduces the amount of hypervisor accesses -
we now only acknowledge features once unless we are clearing any
features when validating (which is uncommon).

Cc: stable@vger.kernel.org
Fixes: 404123c2db79 ("virtio: allow drivers to validate features")
Fixes: 2f9a174f918e ("virtio: write back F_VERSION_1 before validate")
Cc: "Halil Pasic" <pasic@linux.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

fixup! virtio: acknowledge all features before access
---
 drivers/virtio/virtio.c       | 39 ++++++++++++++++++++---------------
 include/linux/virtio_config.h |  3 ++-
 2 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index d891b0a354b0..d6396be0ea83 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -166,14 +166,13 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status)
 }
 EXPORT_SYMBOL_GPL(virtio_add_status);
 
-static int virtio_finalize_features(struct virtio_device *dev)
+/* Do some validation, then set FEATURES_OK */
+static int virtio_features_ok(struct virtio_device *dev)
 {
-	int ret = dev->config->finalize_features(dev);
 	unsigned status;
+	int ret;
 
 	might_sleep();
-	if (ret)
-		return ret;
 
 	ret = arch_has_restricted_virtio_memory_access();
 	if (ret) {
@@ -244,17 +243,6 @@ static int virtio_dev_probe(struct device *_d)
 		driver_features_legacy = driver_features;
 	}
 
-	/*
-	 * Some devices detect legacy solely via F_VERSION_1. Write
-	 * F_VERSION_1 to force LE config space accesses before FEATURES_OK for
-	 * these when needed.
-	 */
-	if (drv->validate && !virtio_legacy_is_little_endian()
-			  && device_features & BIT_ULL(VIRTIO_F_VERSION_1)) {
-		dev->features = BIT_ULL(VIRTIO_F_VERSION_1);
-		dev->config->finalize_features(dev);
-	}
-
 	if (device_features & (1ULL << VIRTIO_F_VERSION_1))
 		dev->features = driver_features & device_features;
 	else
@@ -265,13 +253,26 @@ static int virtio_dev_probe(struct device *_d)
 		if (device_features & (1ULL << i))
 			__virtio_set_bit(dev, i);
 
+	err = dev->config->finalize_features(dev);
+	if (err)
+		goto err;
+
 	if (drv->validate) {
+		u64 features = dev->features;
+
 		err = drv->validate(dev);
 		if (err)
 			goto err;
+
+		/* Did validation change any features? Then write them again. */
+		if (features != dev->features) {
+			err = dev->config->finalize_features(dev);
+			if (err)
+				goto err;
+		}
 	}
 
-	err = virtio_finalize_features(dev);
+	err = virtio_features_ok(dev);
 	if (err)
 		goto err;
 
@@ -495,7 +496,11 @@ int virtio_device_restore(struct virtio_device *dev)
 	/* We have a driver! */
 	virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);
 
-	ret = virtio_finalize_features(dev);
+	ret = dev->config->finalize_features(dev);
+	if (ret)
+		goto err;
+
+	ret = virtio_features_ok(dev);
 	if (ret)
 		goto err;
 
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index 4d107ad31149..dafdc7f48c01 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -64,8 +64,9 @@ struct virtio_shm_region {
  *	Returns the first 64 feature bits (all we currently need).
  * @finalize_features: confirm what device features we'll be using.
  *	vdev: the virtio_device
- *	This gives the final feature bits for the device: it can change
+ *	This sends the driver feature bits to the device: it can change
  *	the dev->feature bits if it wants.
+ * Note: despite the name this can be called any number of times.
  *	Returns 0 on success or error status
  * @bus_name: return the bus name associated with the device (optional)
  *	vdev: the virtio_device
-- 
MST

Re: [PATCH v2 2/2] virtio: acknowledge all features before access

[Jason Wang]

On Wed, Jan 19, 2022 at 1:04 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> The feature negotiation was designed in a way that
> makes it possible for devices to know which config
> fields will be accessed by drivers.
>
> This is broken since commit 404123c2db79 ("virtio: allow drivers to
> validate features") with fallout in at least block and net.  We have a
> partial work-around in commit 2f9a174f918e ("virtio: write back
> F_VERSION_1 before validate") which at least lets devices find out which
> format should config space have, but this is a partial fix: guests
> should not access config space without acknowledging features since
> otherwise we'll never be able to change the config space format.

So I guess this is for this part of the spec 3.1.1:

"""
4. Read device feature bits, and write the subset of feature bits
understood by the OS and driver to the device. During this step the
driver MAY read (but MUST NOT write) the device-specific configuration
fields to check that it can support the device before accepting it.
"""

If it is, is this better to quote in the change log?

Other than this,

Acked-by: Jason Wang <jasowang@redhat.com>

>
> To fix, split finalize_features from virtio_finalize_features and
> call finalize_features with all feature bits before validation,
> and then - if validation changed any bits - once again after.
>
> Since virtio_finalize_features no longer writes out features
> rename it to virtio_features_ok - since that is what it does:
> checks that features are ok with the device.
>
> As a side effect, this also reduces the amount of hypervisor accesses -
> we now only acknowledge features once unless we are clearing any
> features when validating (which is uncommon).
>
> Cc: stable@vger.kernel.org
> Fixes: 404123c2db79 ("virtio: allow drivers to validate features")
> Fixes: 2f9a174f918e ("virtio: write back F_VERSION_1 before validate")
> Cc: "Halil Pasic" <pasic@linux.ibm.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> fixup! virtio: acknowledge all features before access
> ---
>  drivers/virtio/virtio.c       | 39 ++++++++++++++++++++---------------
>  include/linux/virtio_config.h |  3 ++-
>  2 files changed, 24 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index d891b0a354b0..d6396be0ea83 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -166,14 +166,13 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status)
>  }
>  EXPORT_SYMBOL_GPL(virtio_add_status);
>
> -static int virtio_finalize_features(struct virtio_device *dev)
> +/* Do some validation, then set FEATURES_OK */
> +static int virtio_features_ok(struct virtio_device *dev)
>  {
> -       int ret = dev->config->finalize_features(dev);
>         unsigned status;
> +       int ret;
>
>         might_sleep();
> -       if (ret)
> -               return ret;
>
>         ret = arch_has_restricted_virtio_memory_access();
>         if (ret) {
> @@ -244,17 +243,6 @@ static int virtio_dev_probe(struct device *_d)
>                 driver_features_legacy = driver_features;
>         }
>
> -       /*
> -        * Some devices detect legacy solely via F_VERSION_1. Write
> -        * F_VERSION_1 to force LE config space accesses before FEATURES_OK for
> -        * these when needed.
> -        */
> -       if (drv->validate && !virtio_legacy_is_little_endian()
> -                         && device_features & BIT_ULL(VIRTIO_F_VERSION_1)) {
> -               dev->features = BIT_ULL(VIRTIO_F_VERSION_1);
> -               dev->config->finalize_features(dev);
> -       }
> -
>         if (device_features & (1ULL << VIRTIO_F_VERSION_1))
>                 dev->features = driver_features & device_features;
>         else
> @@ -265,13 +253,26 @@ static int virtio_dev_probe(struct device *_d)
>                 if (device_features & (1ULL << i))
>                         __virtio_set_bit(dev, i);
>
> +       err = dev->config->finalize_features(dev);
> +       if (err)
> +               goto err;
> +
>         if (drv->validate) {
> +               u64 features = dev->features;
> +
>                 err = drv->validate(dev);
>                 if (err)
>                         goto err;
> +
> +               /* Did validation change any features? Then write them again. */
> +               if (features != dev->features) {
> +                       err = dev->config->finalize_features(dev);
> +                       if (err)
> +                               goto err;
> +               }
>         }
>
> -       err = virtio_finalize_features(dev);
> +       err = virtio_features_ok(dev);
>         if (err)
>                 goto err;
>
> @@ -495,7 +496,11 @@ int virtio_device_restore(struct virtio_device *dev)
>         /* We have a driver! */
>         virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);
>
> -       ret = virtio_finalize_features(dev);
> +       ret = dev->config->finalize_features(dev);
> +       if (ret)
> +               goto err;
> +
> +       ret = virtio_features_ok(dev);
>         if (ret)
>                 goto err;
>
> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> index 4d107ad31149..dafdc7f48c01 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -64,8 +64,9 @@ struct virtio_shm_region {
>   *     Returns the first 64 feature bits (all we currently need).
>   * @finalize_features: confirm what device features we'll be using.
>   *     vdev: the virtio_device
> - *     This gives the final feature bits for the device: it can change
> + *     This sends the driver feature bits to the device: it can change
>   *     the dev->feature bits if it wants.
> + * Note: despite the name this can be called any number of times.
>   *     Returns 0 on success or error status
>   * @bus_name: return the bus name associated with the device (optional)
>   *     vdev: the virtio_device
> --
> MST
>

Re: [PATCH v2 2/2] virtio: acknowledge all features before access

[Michael S. Tsirkin]

On Wed, Jan 19, 2022 at 10:52:34AM +0800, Jason Wang wrote:
> On Wed, Jan 19, 2022 at 1:04 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > The feature negotiation was designed in a way that
> > makes it possible for devices to know which config
> > fields will be accessed by drivers.
> >
> > This is broken since commit 404123c2db79 ("virtio: allow drivers to
> > validate features") with fallout in at least block and net.  We have a
> > partial work-around in commit 2f9a174f918e ("virtio: write back
> > F_VERSION_1 before validate") which at least lets devices find out which
> > format should config space have, but this is a partial fix: guests
> > should not access config space without acknowledging features since
> > otherwise we'll never be able to change the config space format.
> 
> So I guess this is for this part of the spec 3.1.1:
> 
> """
> 4. Read device feature bits, and write the subset of feature bits
> understood by the OS and driver to the device. During this step the
> driver MAY read (but MUST NOT write) the device-specific configuration
> fields to check that it can support the device before accepting it.
> """
> 
> If it is, is this better to quote in the change log?

I don't think this spec actually clarifies anything.
Sent some spec patches to improve the situation.

> Other than this,
> 
> Acked-by: Jason Wang <jasowang@redhat.com>
> 
> >
> > To fix, split finalize_features from virtio_finalize_features and
> > call finalize_features with all feature bits before validation,
> > and then - if validation changed any bits - once again after.
> >
> > Since virtio_finalize_features no longer writes out features
> > rename it to virtio_features_ok - since that is what it does:
> > checks that features are ok with the device.
> >
> > As a side effect, this also reduces the amount of hypervisor accesses -
> > we now only acknowledge features once unless we are clearing any
> > features when validating (which is uncommon).
> >
> > Cc: stable@vger.kernel.org
> > Fixes: 404123c2db79 ("virtio: allow drivers to validate features")
> > Fixes: 2f9a174f918e ("virtio: write back F_VERSION_1 before validate")
> > Cc: "Halil Pasic" <pasic@linux.ibm.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > fixup! virtio: acknowledge all features before access
> > ---
> >  drivers/virtio/virtio.c       | 39 ++++++++++++++++++++---------------
> >  include/linux/virtio_config.h |  3 ++-
> >  2 files changed, 24 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > index d891b0a354b0..d6396be0ea83 100644
> > --- a/drivers/virtio/virtio.c
> > +++ b/drivers/virtio/virtio.c
> > @@ -166,14 +166,13 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status)
> >  }
> >  EXPORT_SYMBOL_GPL(virtio_add_status);
> >
> > -static int virtio_finalize_features(struct virtio_device *dev)
> > +/* Do some validation, then set FEATURES_OK */
> > +static int virtio_features_ok(struct virtio_device *dev)
> >  {
> > -       int ret = dev->config->finalize_features(dev);
> >         unsigned status;
> > +       int ret;
> >
> >         might_sleep();
> > -       if (ret)
> > -               return ret;
> >
> >         ret = arch_has_restricted_virtio_memory_access();
> >         if (ret) {
> > @@ -244,17 +243,6 @@ static int virtio_dev_probe(struct device *_d)
> >                 driver_features_legacy = driver_features;
> >         }
> >
> > -       /*
> > -        * Some devices detect legacy solely via F_VERSION_1. Write
> > -        * F_VERSION_1 to force LE config space accesses before FEATURES_OK for
> > -        * these when needed.
> > -        */
> > -       if (drv->validate && !virtio_legacy_is_little_endian()
> > -                         && device_features & BIT_ULL(VIRTIO_F_VERSION_1)) {
> > -               dev->features = BIT_ULL(VIRTIO_F_VERSION_1);
> > -               dev->config->finalize_features(dev);
> > -       }
> > -
> >         if (device_features & (1ULL << VIRTIO_F_VERSION_1))
> >                 dev->features = driver_features & device_features;
> >         else
> > @@ -265,13 +253,26 @@ static int virtio_dev_probe(struct device *_d)
> >                 if (device_features & (1ULL << i))
> >                         __virtio_set_bit(dev, i);
> >
> > +       err = dev->config->finalize_features(dev);
> > +       if (err)
> > +               goto err;
> > +
> >         if (drv->validate) {
> > +               u64 features = dev->features;
> > +
> >                 err = drv->validate(dev);
> >                 if (err)
> >                         goto err;
> > +
> > +               /* Did validation change any features? Then write them again. */
> > +               if (features != dev->features) {
> > +                       err = dev->config->finalize_features(dev);
> > +                       if (err)
> > +                               goto err;
> > +               }
> >         }
> >
> > -       err = virtio_finalize_features(dev);
> > +       err = virtio_features_ok(dev);
> >         if (err)
> >                 goto err;
> >
> > @@ -495,7 +496,11 @@ int virtio_device_restore(struct virtio_device *dev)
> >         /* We have a driver! */
> >         virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);
> >
> > -       ret = virtio_finalize_features(dev);
> > +       ret = dev->config->finalize_features(dev);
> > +       if (ret)
> > +               goto err;
> > +
> > +       ret = virtio_features_ok(dev);
> >         if (ret)
> >                 goto err;
> >
> > diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> > index 4d107ad31149..dafdc7f48c01 100644
> > --- a/include/linux/virtio_config.h
> > +++ b/include/linux/virtio_config.h
> > @@ -64,8 +64,9 @@ struct virtio_shm_region {
> >   *     Returns the first 64 feature bits (all we currently need).
> >   * @finalize_features: confirm what device features we'll be using.
> >   *     vdev: the virtio_device
> > - *     This gives the final feature bits for the device: it can change
> > + *     This sends the driver feature bits to the device: it can change
> >   *     the dev->feature bits if it wants.
> > + * Note: despite the name this can be called any number of times.
> >   *     Returns 0 on success or error status
> >   * @bus_name: return the bus name associated with the device (optional)
> >   *     vdev: the virtio_device
> > --
> > MST
> >
> 
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> 

Re: [PATCH v2 2/2] virtio: acknowledge all features before access

[Cornelia Huck]

On Tue, Jan 18 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> The feature negotiation was designed in a way that
> makes it possible for devices to know which config
> fields will be accessed by drivers.
>
> This is broken since commit 404123c2db79 ("virtio: allow drivers to
> validate features") with fallout in at least block and net.  We have a
> partial work-around in commit 2f9a174f918e ("virtio: write back
> F_VERSION_1 before validate") which at least lets devices find out which
> format should config space have, but this is a partial fix: guests
> should not access config space without acknowledging features since
> otherwise we'll never be able to change the config space format.
>
> To fix, split finalize_features from virtio_finalize_features and
> call finalize_features with all feature bits before validation,
> and then - if validation changed any bits - once again after.
>
> Since virtio_finalize_features no longer writes out features
> rename it to virtio_features_ok - since that is what it does:
> checks that features are ok with the device.
>
> As a side effect, this also reduces the amount of hypervisor accesses -
> we now only acknowledge features once unless we are clearing any
> features when validating (which is uncommon).
>
> Cc: stable@vger.kernel.org
> Fixes: 404123c2db79 ("virtio: allow drivers to validate features")
> Fixes: 2f9a174f918e ("virtio: write back F_VERSION_1 before validate")
> Cc: "Halil Pasic" <pasic@linux.ibm.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> fixup! virtio: acknowledge all features before access

Leftover from rebasing?

> ---
>  drivers/virtio/virtio.c       | 39 ++++++++++++++++++++---------------
>  include/linux/virtio_config.h |  3 ++-
>  2 files changed, 24 insertions(+), 18 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Would like to see a quick sanity test from Halil, though.