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 <ru...@rustcorp.com.au> > >> Cc: Michael S. Tsirkin <m...@redhat.com> > >> Cc: Cornelia Huck <cornelia.h...@de.ibm.com> > >> Cc: Wanlong Gao <gaowanl...@cn.fujitsu.com> > >> Signed-off-by: Jason Wang <jasow...@redhat.com> > > > > 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 majord...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/