Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754051AbaKRLCm (ORCPT ); Tue, 18 Nov 2014 06:02:42 -0500 Received: from mx1.redhat.com ([209.132.183.28]:38385 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753160AbaKRLCl (ORCPT ); Tue, 18 Nov 2014 06:02:41 -0500 Date: Tue, 18 Nov 2014 13:02:21 +0200 From: "Michael S. Tsirkin" To: Jason Wang Cc: rusty@rustcorp.com.au, virtualization@lists.linux-foundation.org, linux-kernel@vger.kernel.org, Cornelia Huck , Wanlong Gao Subject: Re: [PATCH V3 2/2] virtio-net: sanitize buggy features advertised by host Message-ID: <20141118110221.GA21664@redhat.com> References: <1416215838-21700-1-git-send-email-jasowang@redhat.com> <1416215838-21700-2-git-send-email-jasowang@redhat.com> <20141117100838.GD20133@redhat.com> <546AB704.4010904@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <546AB704.4010904@redhat.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Nov 18, 2014 at 11:03:32AM +0800, Jason Wang wrote: > On 11/17/2014 06:08 PM, Michael S. Tsirkin wrote: > > On Mon, Nov 17, 2014 at 05:17:18PM +0800, Jason Wang wrote: > >> This patch tries to detect the possible buggy features advertised by host > >> and sanitize them. One example is booting virtio-net with only ctrl_vq > >> disabled, qemu may still advertise many features which depends on it. This > >> will trigger several BUG()s in virtnet_send_command(). > >> > >> This patch utilizes the sanitize_features() method, and disables all > >> features that depends on ctrl_vq if it was not advertised. > >> > >> This fixes the crash when booting with ctrl_vq=off using qemu. > >> > >> Cc: Rusty Russell > >> Cc: Michael S. Tsirkin > >> Cc: Cornelia Huck > >> Cc: Wanlong Gao > >> Signed-off-by: Jason Wang > > > > So I'm not sure this is useful. > > The spec says: > > The device MUST NOT offer a feature which requires another feature which > > was not offered. > > We can't guarantee that hypervisor's implementation are 100% correct. > > So this is a buggy hypervisor, and I believe we should just fail probe. > > This can be done without crashing, and is generally a better > > idea that second-guessing what hypervisor wants us to do. > > > > So we still need something like this patch to detect the wrong > dependencies. And the features fixing like this is not something new, > see how net device fix the features through ndo_fix_features(). I think that's different. If linux refuses to work with a broken hardware NIC, you can not do anything, and hardware is mass-produced so we know many people are affected. If linux refuses to work with a misconfigured hypervisor, it's just one user who misconfigured it, and hypervisor can be fixed. So let's not work around bugs at least unless we know that many people already use a broken hypervisor, something prevents vendor from fixing it. > > > > > > > > However, assuming that we do want this change: > > This can be replaced with a table driven design in virtio core, but > > since you chose to open code it, I would drop table below altogether. > > > > > > Just make it > > if (!virtio_has_feature(dev, VIRTIO_NET_F_CTRL_VQ)) { > > virtio_disable_feature(dev, VIRTIO_NET_F_CTRL_RX); > > virtio_disable_feature(dev, VIRTIO_NET_F_CTRL_VLAN); > > virtio_disable_feature(dev, VIRTIO_NET_F_GUEST_ANNOUNCE); > > virtio_disable_feature(dev, VIRTIO_NET_F_MQ); > > virtio_disable_feature(dev, VIRTIO_NET_F_CTRL_MAC_ADDR); > > } > > > > This is similar to what I did in v1 and v2. Either are ok for me. > > > > > >> --- > >> Changes from V1: > >> - fix the cut-and-paste error > >> Changes from V2: > >> - loop through an array of feature bits > >> - switch to use dev_warn() > >> --- > >> drivers/net/virtio_net.c | 26 ++++++++++++++++++++++++++ > >> 1 file changed, 26 insertions(+) > >> > >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > >> index ec2a8b4..6fadd8c 100644 > >> --- a/drivers/net/virtio_net.c > >> +++ b/drivers/net/virtio_net.c > >> @@ -1948,6 +1948,31 @@ static int virtnet_restore(struct virtio_device *vdev) > >> } > >> #endif > >> > >> +static void virtnet_sanitize_features(struct virtio_device *dev) > >> +{ > >> + unsigned int features_for_ctrl_vq[] = { > >> + VIRTIO_NET_F_CTRL_RX, > >> + VIRTIO_NET_F_CTRL_VLAN, > >> + VIRTIO_NET_F_GUEST_ANNOUNCE, > >> + VIRTIO_NET_F_MQ, > >> + VIRTIO_NET_F_CTRL_MAC_ADDR > >> + }; > > This is not the only dependency: checksums > > have dependencies too. See virtio 1.0 spec. > > > > I see ,and this kind of check could be added. But we're really safe > since qemu handle such cases and won't advertise any offload feature is > csum is not supported. > > > >> + int i; > >> + > >> + if (!virtio_has_feature(dev, VIRTIO_NET_F_CTRL_VQ)) { > >> + for (i = 0; i < ARRAY_SIZE(features_for_ctrl_vq); i++) { > >> + unsigned int f = features_for_ctrl_vq[i]; > >> + if (virtio_has_feature(dev, f)) { > >> + virtio_disable_feature(dev, f); > >> + dev_warn(&dev->dev, > >> + "buggy hyperviser: disable feature " > >> + "0x%x since VIRTIO_NET_F_CTRL_VQ was " > >> + "not advertised.\n", f); > >> + } > >> + } > >> + } > >> +} > >> + > >> static struct virtio_device_id id_table[] = { > >> { VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID }, > >> { 0 }, > >> @@ -1975,6 +2000,7 @@ static struct virtio_driver virtio_net_driver = { > >> .probe = virtnet_probe, > >> .remove = virtnet_remove, > >> .config_changed = virtnet_config_changed, > >> + .sanitize_features = virtnet_sanitize_features, > >> #ifdef CONFIG_PM_SLEEP > >> .freeze = virtnet_freeze, > >> .restore = virtnet_restore, > >> -- > >> 1.9.1 > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/