From: Cornelia Huck <cohuck@redhat.com>
To: "Michael S. Tsirkin" <mst@redhat.com>, linux-kernel@vger.kernel.org
Cc: Halil Pasic <pasic@linux.ibm.com>,
stable@vger.kernel.org,
virtualization@lists.linux-foundation.org
Subject: Re: [PATCH v2 2/2] virtio: acknowledge all features before access
Date: Thu, 20 Jan 2022 15:35:01 +0100 [thread overview]
Message-ID: <87h79ycw6i.fsf@redhat.com> (raw)
In-Reply-To: <20220118170225.30620-2-mst@redhat.com>
On Tue, Jan 18 2022, "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.
>
> 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
Leftover from rebasing?
> ---
> drivers/virtio/virtio.c | 39 ++++++++++++++++++++---------------
> include/linux/virtio_config.h | 3 ++-
> 2 files changed, 24 insertions(+), 18 deletions(-)
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Would like to see a quick sanity test from Halil, though.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
WARNING: multiple messages have this Message-ID (diff)
From: Cornelia Huck <cohuck@redhat.com>
To: "Michael S. Tsirkin" <mst@redhat.com>, linux-kernel@vger.kernel.org
Cc: Jason Wang <jasowang@redhat.com>,
virtualization@lists.linux-foundation.org,
stable@vger.kernel.org, Halil Pasic <pasic@linux.ibm.com>
Subject: Re: [PATCH v2 2/2] virtio: acknowledge all features before access
Date: Thu, 20 Jan 2022 15:35:01 +0100 [thread overview]
Message-ID: <87h79ycw6i.fsf@redhat.com> (raw)
In-Reply-To: <20220118170225.30620-2-mst@redhat.com>
On Tue, Jan 18 2022, "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.
>
> 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
Leftover from rebasing?
> ---
> drivers/virtio/virtio.c | 39 ++++++++++++++++++++---------------
> include/linux/virtio_config.h | 3 ++-
> 2 files changed, 24 insertions(+), 18 deletions(-)
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Would like to see a quick sanity test from Halil, though.
next prev parent reply other threads:[~2022-01-20 14:35 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-18 17:03 [PATCH v2 1/2] virtio: unexport virtio_finalize_features Michael S. Tsirkin
2022-01-18 17:03 ` Michael S. Tsirkin
2022-01-18 17:03 ` [PATCH v2 2/2] virtio: acknowledge all features before access Michael S. Tsirkin
2022-01-18 17:03 ` Michael S. Tsirkin
2022-01-19 2:52 ` Jason Wang
2022-01-19 2:52 ` Jason Wang
2022-01-19 9:18 ` Michael S. Tsirkin
2022-01-19 9:18 ` Michael S. Tsirkin
2022-01-20 14:35 ` Cornelia Huck [this message]
2022-01-20 14:35 ` Cornelia Huck
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=87h79ycw6i.fsf@redhat.com \
--to=cohuck@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mst@redhat.com \
--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: link
Be 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.