From: "Michael S. Tsirkin" <mst@redhat.com> To: linux-kernel@vger.kernel.org Cc: stable@vger.kernel.org, Halil Pasic <pasic@linux.ibm.com>, Jason Wang <jasowang@redhat.com>, Cornelia Huck <cohuck@redhat.com>, virtualization@lists.linux-foundation.org Subject: [PATCH] virtio: acknowledge all features before access Date: Fri, 14 Jan 2022 15:09:14 -0500 [thread overview] Message-ID: <20220114200744.150325-1-mst@redhat.com> (raw) 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@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> --- 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
WARNING: multiple messages have this Message-ID (diff)
From: "Michael S. Tsirkin" <mst@redhat.com> To: linux-kernel@vger.kernel.org Cc: Halil Pasic <pasic@linux.ibm.com>, Cornelia Huck <cohuck@redhat.com>, stable@vger.kernel.org, virtualization@lists.linux-foundation.org Subject: [PATCH] virtio: acknowledge all features before access Date: Fri, 14 Jan 2022 15:09:14 -0500 [thread overview] Message-ID: <20220114200744.150325-1-mst@redhat.com> (raw) 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@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> --- 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 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next reply other threads:[~2022-01-14 20:09 UTC|newest] Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-01-14 20:09 Michael S. Tsirkin [this message] 2022-01-14 20:09 ` [PATCH] virtio: acknowledge all features before access Michael S. Tsirkin 2022-01-17 6:31 ` Jason Wang 2022-01-17 6:31 ` Jason Wang 2022-01-17 8:26 ` Michael S. Tsirkin 2022-01-17 8:26 ` Michael S. Tsirkin 2022-01-17 12:38 ` Cornelia Huck 2022-01-17 12:38 ` Cornelia Huck 2022-01-17 23:13 ` Michael S. Tsirkin 2022-01-17 23:13 ` Michael S. Tsirkin 2022-01-18 15:43 ` Michael S. Tsirkin 2022-01-18 15:43 ` Michael S. Tsirkin 2022-01-18 17:10 ` Cornelia Huck 2022-01-18 17:10 ` Cornelia Huck 2022-01-18 12:48 ` Halil Pasic 2022-01-18 12:48 ` Halil Pasic 2022-01-18 15:11 ` Michael S. Tsirkin 2022-01-18 15:11 ` Michael S. Tsirkin 2022-01-18 14:43 ` Halil Pasic 2022-01-18 14:43 ` Halil Pasic 2022-01-18 15:20 ` Michael S. Tsirkin 2022-01-18 15:20 ` Michael S. Tsirkin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20220114200744.150325-1-mst@redhat.com \ --to=mst@redhat.com \ --cc=cohuck@redhat.com \ --cc=jasowang@redhat.com \ --cc=linux-kernel@vger.kernel.org \ --cc=pasic@linux.ibm.com \ --cc=stable@vger.kernel.org \ --cc=virtualization@lists.linux-foundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.