Re: [RFC PATCH 1/1] virtio: write back features before verify

[Cornelia Huck]

On Mon, Oct 04 2021, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Oct 04, 2021 at 04:23:23AM +0200, Halil Pasic wrote:
>> --------------------------8<---------------------
>> 
>> From: Halil Pasic <pasic@linux.ibm.com>
>> Date: Thu, 30 Sep 2021 02:38:47 +0200
>> Subject: [PATCH] virtio: write back feature VERSION_1 before verify
>> 
>> This patch fixes a regression introduced by commit 82e89ea077b9
>> ("virtio-blk: Add validation for block size in config space") and
>> enables similar checks in verify() on big endian platforms.
>> 
>> The problem with checking multi-byte config fields in the verify
>> callback, on big endian platforms, and with a possibly transitional
>> device is the following. The verify() callback is called between
>> config->get_features() and virtio_finalize_features(). That we have a
>> device that offered F_VERSION_1 then we have the following options
>> either the device is transitional, and then it has to present the legacy
>> interface, i.e. a big endian config space until F_VERSION_1 is
>> negotiated, or we have a non-transitional device, which makes
>> F_VERSION_1 mandatory, and only implements the non-legacy interface and
>> thus presents a little endian config space. Because at this point we
>> can't know if the device is transitional or non-transitional, we can't
>> know do we need to byte swap or not.
>
> Well we established that we can know. Here's an alternative explanation:
>
>       The virtio specification virtio-v1.1-cs01 states:
>
>       Transitional devices MUST detect Legacy drivers by detecting that
>       VIRTIO_F_VERSION_1 has not been acknowledged by the driver.
>       This is exactly what QEMU as of 6.1 has done relying solely
>       on VIRTIO_F_VERSION_1 for detecting that.
>
>       However, the specification also says:
>       driver MAY read (but MUST NOT write) the device-specific
>       configuration fields to check that it can support the device before
>       accepting it.
>
>       In that case, any device relying solely on VIRTIO_F_VERSION_1
>       for detecting legacy drivers will return data in legacy format.
>       In particular, this implies that it is in big endian format
>       for big endian guests. This naturally confuses the driver
>       which expects little endian in the modern mode.
>
>       It is probably a good idea to amend the spec to clarify that
>       VIRTIO_F_VERSION_1 can only be relied on after the feature negotiation
>       is complete. However, we already have regression so let's
>       try to address it.

I prefer that explanation.

>
>
>> 
>> The virtio spec explicitly states that the driver MAY read config
>> between reading and writing the features so saying that first accessing
>> the config before feature negotiation is done is not an option. The
>> specification ain't clear about setting the features multiple times
>> before FEATURES_OK, so I guess that should be fine to set F_VERSION_1
>> since at this point we already know that we are about to negotiate
>> F_VERSION_1.
>> 
>> I don't consider this patch super clean, but frankly I don't think we
>> have a ton of options. Another option that may or man not be cleaner,
>> but is also IMHO much uglier is to figure out whether the device is
>> transitional by rejecting _F_VERSION_1, then resetting it and proceeding
>> according tho what we have figured out, hoping that the characteristics
>> of the device didn't change.
>
> An empty line before tags.
>
>> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
>> Fixes: 82e89ea077b9 ("virtio-blk: Add validation for block size in config 
>> space")
>> Reported-by: markver@us.ibm.com
>
> Let's add more commits that are affected. E.g. virtio-net with MTU
> feature bit set is affected too.
>
> So let's add Fixes tag for:
> commit 14de9d114a82a564b94388c95af79a701dc93134
> Author: Aaron Conole <aconole@redhat.com>
> Date:   Fri Jun 3 16:57:12 2016 -0400
>
>     virtio-net: Add initial MTU advice feature
>     
> I think that's all, but pls double check me.

I could not find anything else after a quick check.

>
>
>> ---
>>  drivers/virtio/virtio.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>> 
>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>> index 0a5b54034d4b..2b9358f2e22a 100644
>> --- a/drivers/virtio/virtio.c
>> +++ b/drivers/virtio/virtio.c
>> @@ -239,6 +239,12 @@ static int virtio_dev_probe(struct device *_d)
>>              driver_features_legacy = driver_features;
>>      }
>>  
>> +    /* Write F_VERSION_1 feature to pin down endianness */
>> +    if (device_features & (1ULL << VIRTIO_F_VERSION_1) & driver_features) {
>> +            dev->features = (1ULL << VIRTIO_F_VERSION_1);
>> +            dev->config->finalize_features(dev);
>> +    }
>> +
>>      if (device_features & (1ULL << VIRTIO_F_VERSION_1))
>>              dev->features = driver_features & device_features;
>>      else
>> -- 
>> 2.31.1

I think we should go with this just to fix the nasty regression for now.