On Mon, Oct 04 2021, "Michael S. Tsirkin" <mst@xxxxxxxxxx> wrote:
> On Mon, Oct 04, 2021 at 04:23:23AM +0200, Halil Pasic wrote:
>> --------------------------8<---------------------
>>
>> From: Halil Pasic <pasic@xxxxxxxxxxxxx>
>> 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@xxxxxxxxxxxxx>
>> Fixes: 82e89ea077b9 ("virtio-blk: Add validation for block size in config space")
>> Reported-by: markver@xxxxxxxxxx
>
> 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@xxxxxxxxxx>
> 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.
