Michael S. Tsirkin
2022-Jan-14 20:09 UTC
[PATCH] virtio: acknowledge all features before access
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.
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.
Cc: stable at 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 at linux.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
Halil, I thought hard about our situation with transitional and
today I finally thought of something I am happy with.
Pls let me know what you think. Testing on big endian would
also be much appreciated!
drivers/virtio/virtio.c | 31 +++++++++++++++++--------------
1 file changed, 17 insertions(+), 14 deletions(-)
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index d891b0a354b0..2ed6e2451fd8 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -168,12 +168,10 @@ EXPORT_SYMBOL_GPL(virtio_add_status);
static int virtio_finalize_features(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 +242,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,10 +252,22 @@ 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;
+
+ if (features != dev->features) {
+ err = dev->config->finalize_features(dev);
+ if (err)
+ goto err;
+ }
}
err = virtio_finalize_features(dev);
@@ -495,6 +494,10 @@ int virtio_device_restore(struct virtio_device *dev)
/* We have a driver! */
virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);
+ ret = dev->config->finalize_features(dev);
+ if (ret)
+ goto err;
+
ret = virtio_finalize_features(dev);
if (ret)
goto err;
--
MST
? 2022/1/15 ??4:09, 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. > > 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. > > Cc: stable at 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 at linux.ibm.com> > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > --- > > Halil, I thought hard about our situation with transitional and > today I finally thought of something I am happy with. > Pls let me know what you think. Testing on big endian would > also be much appreciated! > > drivers/virtio/virtio.c | 31 +++++++++++++++++-------------- > 1 file changed, 17 insertions(+), 14 deletions(-) > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c > index d891b0a354b0..2ed6e2451fd8 100644 > --- a/drivers/virtio/virtio.c > +++ b/drivers/virtio/virtio.c > @@ -168,12 +168,10 @@ EXPORT_SYMBOL_GPL(virtio_add_status); > > static int virtio_finalize_features(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 +242,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,10 +252,22 @@ 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; > + > + if (features != dev->features) { > + err = dev->config->finalize_features(dev); > + if (err) > + goto err; > + } > } > > err = virtio_finalize_features(dev); > @@ -495,6 +494,10 @@ int virtio_device_restore(struct virtio_device *dev) > /* We have a driver! */ > virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER); > > + ret = dev->config->finalize_features(dev); > + if (ret) > + goto err;Is this part of code related? Thanks> + > ret = virtio_finalize_features(dev); > if (ret) > goto err;
On Fri, 14 Jan 2022 15:09:14 -0500 "Michael S. Tsirkin" <mst at 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. > > 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. > > Cc: stable at 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 at linux.ibm.com> > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > --- > > Halil, I thought hard about our situation with transitional and > today I finally thought of something I am happy with. > Pls let me know what you think. Testing on big endian would > also be much appreciated!Hi Michael! I was just about to have a look into this. But it does not apply cleanly to Linus master (fetched a couple of minutes ago). I also tride with d9679d0013a66849~1 but no luck. What is a suitable base for this patch? Regards, Halil> drivers/virtio/virtio.c | 31 +++++++++++++++++-------------- > 1 file changed, 17 insertions(+), 14 deletions(-) > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c > index d891b0a354b0..2ed6e2451fd8 100644 > --- a/drivers/virtio/virtio.c > +++ b/drivers/virtio/virtio.c > @@ -168,12 +168,10 @@ EXPORT_SYMBOL_GPL(virtio_add_status); > > static int virtio_finalize_features(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 +242,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,10 +252,22 @@ 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; > + > + if (features != dev->features) { > + err = dev->config->finalize_features(dev); > + if (err) > + goto err; > + } > } > > err = virtio_finalize_features(dev); > @@ -495,6 +494,10 @@ int virtio_device_restore(struct virtio_device *dev) > /* We have a driver! */ > virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER); > > + ret = dev->config->finalize_features(dev); > + if (ret) > + goto err; > + > ret = virtio_finalize_features(dev); > if (ret) > goto err;
On Fri, 14 Jan 2022 15:09:14 -0500 "Michael S. Tsirkin" <mst at 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.I agree with that. The crux is what does "acknowledge features" exactly mean. Is it "write features" or "complete the feature negotiation, including setting FEATURES_OK". My understanding is, that we should not rely on that the device is going to act according to the negotiated feature set unless FEATURES_OK was set successfully. That would mean, that this change ain't guaranteed to help with the stated problem. We simply don't know if the fact that features were written is going to have a side-effect or not. Also see below.> > 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.My understanding is that this patch basically does for all the features, what commit 2f9a174f918e ("virtio: write back F_VERSION_1 before validate") did only for F_VERSION_1 and under certain conditions to be minimally invasive. I don't like when s390 is the oddball, so I'm very happy to see us moving away from that.> > Cc: stable at 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 at linux.ibm.com> > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > --- > > Halil, I thought hard about our situation with transitional and > today I finally thought of something I am happy with. > Pls let me know what you think. Testing on big endian would > also be much appreciated!Thanks! I will first provide some comments, and I intend to come back with the test results later.> > drivers/virtio/virtio.c | 31 +++++++++++++++++-------------- > 1 file changed, 17 insertions(+), 14 deletions(-) > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c > index d891b0a354b0..2ed6e2451fd8 100644 > --- a/drivers/virtio/virtio.c > +++ b/drivers/virtio/virtio.c > @@ -168,12 +168,10 @@ EXPORT_SYMBOL_GPL(virtio_add_status); > > static int virtio_finalize_features(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 +242,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,10 +252,22 @@ static int virtio_dev_probe(struct device *_d) > if (device_features & (1ULL << i)) > __virtio_set_bit(dev, i); > > + err = dev->config->finalize_features(dev);A side note: config->finalize_features() ain't the best name for what the thing does. After config->finalize_features() the features are not final. Unlike after virtio_finalize_features(). IMHO filter_and_write_features() would be a more accurate, although longer name. After this point, the features aren't final yet, and one can not say that a some feature X has been negotiated. But with regards to features, the spec does not really consider this limbo state. Should this change? Do we want to say: the device SHOULD pick up, and act upon the new features *before* FEATURES_OK is set? ...> + if (err) > + goto err; > + > if (drv->validate) { > + u64 features = dev->features; > + > err = drv->validate(dev);... Consider the "we would like to introduce a new config space format" example. Here, I guess we would like to use the new format. But let's say _F_CFG_FMT_V2 aint negotiated yet. So to be sure about the format, we would need to specify, that the behavior of the device needs to change after the feature has been written, but before FEATURES_OK is set, at least for _F_CFG_FMT_V2. Please also consider the QEMU implementation of the vhost-user stuff. We push the features to the back-end only when FEATURES_OK status is written.> if (err) > goto err; > + > + if (features != dev->features) { > + err = dev->config->finalize_features(dev);It is fine to call it again, because the features aren't finalized yet. And re-doing any transport-level filtering and validation is fine as well.> + if (err) > + goto err; > + } > } > > err = virtio_finalize_features(dev);Here the features are finally negotiated and final.> @@ -495,6 +494,10 @@ int virtio_device_restore(struct virtio_device *dev) > /* We have a driver! */ > virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER); > > + ret = dev->config->finalize_features(dev); > + if (ret) > + goto err; > + > ret = virtio_finalize_features(dev);Looks a little weird, because virtio_finalize_features() used to include filter + write + set FEATURES_OK. But it ain't too bad. Better names would benefit readability though, if we can come up with some. Regards, Halil> if (ret) > goto err;