From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id D281FC433F5 for ; Mon, 14 Mar 2022 11:38:30 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236215AbiCNLjh (ORCPT ); Mon, 14 Mar 2022 07:39:37 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40010 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S239669AbiCNLih (ORCPT ); Mon, 14 Mar 2022 07:38:37 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 27A5041F98; Mon, 14 Mar 2022 04:37:28 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id B1E486116E; Mon, 14 Mar 2022 11:37:27 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 6BCB0C340E9; Mon, 14 Mar 2022 11:37:25 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1647257847; bh=p1iJ87UGATnWTBq8cygUcgNcTog8coD3PNue1smeTcg=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=KzSpxhXkQIvfZOqHsiAjbN761ektPdxX69dCEQI494NJa5FCJsu4ZmFJScYF2z8eH moiijHN8gLLjkIsgdL4ZUSqKr/j2yCGn7ADX68NGRgaOjYj1cQmPo3MKaCMHCQ1Rhc CgJq5RJLZWOhbkYa3bI5CcFHBPf8UVmYOC9o5AIY= From: Greg Kroah-Hartman To: linux-kernel@vger.kernel.org Cc: Greg Kroah-Hartman , stable@vger.kernel.org, Jason Wang , "Halil Pasic" , "Michael S. Tsirkin" Subject: [PATCH 4.14 20/23] virtio: acknowledge all features before access Date: Mon, 14 Mar 2022 12:34:33 +0100 Message-Id: <20220314112731.642622443@linuxfoundation.org> X-Mailer: git-send-email 2.35.1 In-Reply-To: <20220314112731.050583127@linuxfoundation.org> References: <20220314112731.050583127@linuxfoundation.org> User-Agent: quilt/0.66 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org From: Michael S. Tsirkin commit 4fa59ede95195f267101a1b8916992cf3f245cdb upstream. 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). IRC I think that this was more or less always the intent in the spec but unfortunately the way the spec is worded does not say this explicitly, I plan to address this at the spec level, too. Acked-by: Jason Wang 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" Signed-off-by: Michael S. Tsirkin Signed-off-by: Greg Kroah-Hartman --- drivers/virtio/virtio.c | 39 +++++++++++++++++++++------------------ include/linux/virtio_config.h | 3 ++- 2 files changed, 23 insertions(+), 19 deletions(-) --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -165,14 +165,11 @@ void virtio_add_status(struct virtio_dev } EXPORT_SYMBOL_GPL(virtio_add_status); -static int virtio_finalize_features(struct virtio_device *dev) +/* Do some validation, then set FEATURES_OK */ +static int virtio_features_ok(struct virtio_device *dev) { - int ret = dev->config->finalize_features(dev); unsigned status; - if (ret) - return ret; - if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) return 0; @@ -221,17 +218,6 @@ static int virtio_dev_probe(struct devic 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 @@ -242,13 +228,26 @@ static int virtio_dev_probe(struct devic 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; + + /* Did validation change any features? Then write them again. */ + if (features != dev->features) { + err = dev->config->finalize_features(dev); + if (err) + goto err; + } } - err = virtio_finalize_features(dev); + err = virtio_features_ok(dev); if (err) goto err; @@ -400,7 +399,11 @@ int virtio_device_restore(struct virtio_ /* We have a driver! */ virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER); - ret = virtio_finalize_features(dev); + ret = dev->config->finalize_features(dev); + if (ret) + goto err; + + ret = virtio_features_ok(dev); if (ret) goto err; --- a/include/linux/virtio_config.h +++ b/include/linux/virtio_config.h @@ -51,8 +51,9 @@ struct irq_affinity; * Returns the first 32 feature bits (all we currently need). * @finalize_features: confirm what device features we'll be using. * vdev: the virtio_device - * This gives the final feature bits for the device: it can change + * This sends the driver feature bits to the device: it can change * the dev->feature bits if it wants. + * Note: despite the name this can be called any number of times. * Returns 0 on success or error status * @bus_name: return the bus name associated with the device * vdev: the virtio_device