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;
MIME-Version: 1.0
References: <20220118170225.30620-1-mst@redhat.com> <20220118170225.30620-2-mst@redhat.com>
In-Reply-To: <20220118170225.30620-2-mst@redhat.com>
From:   Jason Wang <jasowang@redhat.com>
Date:   Wed, 19 Jan 2022 10:52:34 +0800
Message-ID: <CACGkMEt-Q7baDDUM8aC_Lki1aeO36xi02AE7kEapi5NVqkGErg@mail.gmail.com>
Subject: Re: [PATCH v2 2/2] virtio: acknowledge all features before access
To:     "Michael S. Tsirkin" <mst@redhat.com>
Cc:     linux-kernel <linux-kernel@vger.kernel.org>,
        Cornelia Huck <cohuck@redhat.com>,
        virtualization <virtualization@lists.linux-foundation.org>,
        stable@vger.kernel.org, Halil Pasic <pasic@linux.ibm.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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
>