Re: [PATCH V3 2/2] virtio-net: sanitize buggy features advertised by host

[Michael S. Tsirkin]

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/
--
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/