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 <rusty@xxxxxxxxxxxxxxx>
> >> Cc: Michael S. Tsirkin <mst@xxxxxxxxxx>
> >> Cc: Cornelia Huck <cornelia.huck@xxxxxxxxxx>
> >> Cc: Wanlong Gao <gaowanlong@xxxxxxxxxxxxxx>
> >> Signed-off-by: Jason Wang <jasowang@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
