[PATCH net] virtio-net: validate features during probe

[PATCH net] virtio-net: validate features during probe

[Jason Wang]

This patch validates feature dependencies during probe and fail the probing
if a dependency is missed. This fixes the issues of hitting BUG()
when qemu fails to advertise features correctly. One example is booting
guest with ctrl_vq=off through qemu.

Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
Cc: Wanlong Gao <gaowanlong@cn.fujitsu.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/net/virtio_net.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 93 insertions(+)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index ec2a8b4..4a0ad46 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1673,6 +1673,95 @@ static const struct attribute_group virtio_net_mrg_rx_group = {
 };
 #endif
 
+static int virtnet_validate_features(struct virtio_device *dev,
+				     unsigned int *table,
+				     int table_size,
+				     unsigned int feature)
+{
+	int i;
+
+	if (!virtio_has_feature(dev, feature)) {
+		for (i = 0; i < table_size; i++) {
+			unsigned int f = table[i];
+
+			if (virtio_has_feature(dev, f)) {
+				dev_err(&dev->dev,
+					"buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x was not",
+					f, feature);
+				return -EINVAL;
+			}
+		}
+	}
+
+	return 0;
+}
+
+static int virtnet_check_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
+	};
+	unsigned int features_for_guest_csum[] = {
+		VIRTIO_NET_F_GUEST_TSO4,
+		VIRTIO_NET_F_GUEST_TSO6,
+		VIRTIO_NET_F_GUEST_ECN,
+		VIRTIO_NET_F_GUEST_UFO,
+	};
+	unsigned int features_for_host_csum[] = {
+		VIRTIO_NET_F_HOST_TSO4,
+		VIRTIO_NET_F_HOST_TSO6,
+		VIRTIO_NET_F_HOST_ECN,
+		VIRTIO_NET_F_HOST_UFO,
+	};
+	int err;
+
+	err = virtnet_validate_features(dev, features_for_ctrl_vq,
+					ARRAY_SIZE(features_for_ctrl_vq),
+					VIRTIO_NET_F_CTRL_VQ);
+	if (err)
+		return err;
+
+	err = virtnet_validate_features(dev, features_for_guest_csum,
+					ARRAY_SIZE(features_for_guest_csum),
+					VIRTIO_NET_F_GUEST_CSUM);
+	if (err)
+		return err;
+
+	err = virtnet_validate_features(dev, features_for_host_csum,
+					ARRAY_SIZE(features_for_host_csum),
+					VIRTIO_NET_F_CSUM);
+	if (err)
+		return err;
+
+	if (virtio_has_feature(dev, VIRTIO_NET_F_GUEST_ECN) &&
+	    (!virtio_has_feature(dev, VIRTIO_NET_F_GUEST_TSO4) ||
+	     !virtio_has_feature(dev, VIRTIO_NET_F_GUEST_TSO6))) {
+		dev_err(&dev->dev,
+			"buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x or 0x%x was not",
+			VIRTIO_NET_F_GUEST_ECN,
+			VIRTIO_NET_F_GUEST_TSO4,
+			VIRTIO_NET_F_GUEST_TSO6);
+		return -EINVAL;
+	}
+
+	if (virtio_has_feature(dev, VIRTIO_NET_F_HOST_ECN) &&
+	    (!virtio_has_feature(dev, VIRTIO_NET_F_HOST_TSO4) ||
+	     !virtio_has_feature(dev, VIRTIO_NET_F_HOST_TSO6))) {
+		dev_err(&dev->dev,
+			"buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x or 0x%x was not",
+			VIRTIO_NET_F_HOST_ECN,
+			VIRTIO_NET_F_HOST_TSO4,
+			VIRTIO_NET_F_HOST_TSO6);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int virtnet_probe(struct virtio_device *vdev)
 {
 	int i, err;
@@ -1680,6 +1769,10 @@ static int virtnet_probe(struct virtio_device *vdev)
 	struct virtnet_info *vi;
 	u16 max_queue_pairs;
 
+	err = virtnet_check_features(vdev);
+	if (err)
+		return -EINVAL;
+
 	/* Find if host supports multiqueue virtio_net device */
 	err = virtio_cread_feature(vdev, VIRTIO_NET_F_MQ,
 				   struct virtio_net_config,
-- 
1.9.1

Re: [PATCH net] virtio-net: validate features during probe

[Jason Wang]

On 11/19/2014 02:35 PM, Jason Wang wrote:
> This patch validates feature dependencies during probe and fail the probing
> if a dependency is missed. This fixes the issues of hitting BUG()
> when qemu fails to advertise features correctly. One example is booting
> guest with ctrl_vq=off through qemu.
>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---

Just notice UFO has been disabled after
3d0ad09412ffe00c9afa201d01effdb6023d09b4 "drivers/net: Disable UFO
through virtio". Will post V2 and drop VIRTIO_NET_F_*_UFO from the
checklist.

Re: [PATCH net] virtio-net: validate features during probe

[Michael S. Tsirkin]

On Wed, Nov 19, 2014 at 02:35:39PM +0800, Jason Wang wrote:
> This patch validates feature dependencies during probe and fail the probing
> if a dependency is missed. This fixes the issues of hitting BUG()
> when qemu fails to advertise features correctly. One example is booting
> guest with ctrl_vq=off through qemu.
> 
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/net/virtio_net.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 93 insertions(+)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index ec2a8b4..4a0ad46 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1673,6 +1673,95 @@ static const struct attribute_group virtio_net_mrg_rx_group = {
>  };
>  #endif
>  
> +static int virtnet_validate_features(struct virtio_device *dev,
> +				     unsigned int *table,
> +				     int table_size,
> +				     unsigned int feature)
> +{
> +	int i;
> +
> +	if (!virtio_has_feature(dev, feature)) {
> +		for (i = 0; i < table_size; i++) {
> +			unsigned int f = table[i];
> +
> +			if (virtio_has_feature(dev, f)) {
> +				dev_err(&dev->dev,
> +					"buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x was not",


This line's way too long.

> +					f, feature);
> +				return -EINVAL;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int virtnet_check_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
> +	};
> +	unsigned int features_for_guest_csum[] = {
> +		VIRTIO_NET_F_GUEST_TSO4,
> +		VIRTIO_NET_F_GUEST_TSO6,
> +		VIRTIO_NET_F_GUEST_ECN,
> +		VIRTIO_NET_F_GUEST_UFO,
> +	};
> +	unsigned int features_for_host_csum[] = {
> +		VIRTIO_NET_F_HOST_TSO4,
> +		VIRTIO_NET_F_HOST_TSO6,
> +		VIRTIO_NET_F_HOST_ECN,
> +		VIRTIO_NET_F_HOST_UFO,
> +	};
> +	int err;
> +
> +	err = virtnet_validate_features(dev, features_for_ctrl_vq,
> +					ARRAY_SIZE(features_for_ctrl_vq),
> +					VIRTIO_NET_F_CTRL_VQ);
> +	if (err)
> +		return err;
> +
> +	err = virtnet_validate_features(dev, features_for_guest_csum,
> +					ARRAY_SIZE(features_for_guest_csum),
> +					VIRTIO_NET_F_GUEST_CSUM);
> +	if (err)
> +		return err;
> +
> +	err = virtnet_validate_features(dev, features_for_host_csum,
> +					ARRAY_SIZE(features_for_host_csum),
> +					VIRTIO_NET_F_CSUM);
> +	if (err)
> +		return err;
> +
> +	if (virtio_has_feature(dev, VIRTIO_NET_F_GUEST_ECN) &&
> +	    (!virtio_has_feature(dev, VIRTIO_NET_F_GUEST_TSO4) ||
> +	     !virtio_has_feature(dev, VIRTIO_NET_F_GUEST_TSO6))) {
> +		dev_err(&dev->dev,
> +			"buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x or 0x%x was not",
> +			VIRTIO_NET_F_GUEST_ECN,
> +			VIRTIO_NET_F_GUEST_TSO4,
> +			VIRTIO_NET_F_GUEST_TSO6);
> +		return -EINVAL;
> +	}
> +
> +	if (virtio_has_feature(dev, VIRTIO_NET_F_HOST_ECN) &&
> +	    (!virtio_has_feature(dev, VIRTIO_NET_F_HOST_TSO4) ||
> +	     !virtio_has_feature(dev, VIRTIO_NET_F_HOST_TSO6))) {
> +		dev_err(&dev->dev,
> +			"buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x or 0x%x was not",
> +			VIRTIO_NET_F_HOST_ECN,
> +			VIRTIO_NET_F_HOST_TSO4,
> +			VIRTIO_NET_F_HOST_TSO6);
> +		return -EINVAL;
> +	}
> +
> +	return 0;

> +}
> +
>  static int virtnet_probe(struct virtio_device *vdev)
>  {
>  	int i, err;
> @@ -1680,6 +1769,10 @@ static int virtnet_probe(struct virtio_device *vdev)
>  	struct virtnet_info *vi;
>  	u16 max_queue_pairs;
>  
> +	err = virtnet_check_features(vdev);
> +	if (err)
> +		return -EINVAL;
> +
>  	/* Find if host supports multiqueue virtio_net device */
>  	err = virtio_cread_feature(vdev, VIRTIO_NET_F_MQ,
>  				   struct virtio_net_config,

The API seems too complex, and you still had to open-code ECN logic.
Just open-code most of it.  You can use a helper macro to output a
friendly message without code duplication.
For example like the below (completely untested)?


I would also like to split things: dependencies on
VIRTIO_NET_F_CTRL_VQ might go into this kernel,
since they help fix BUG.

Others should wait since they don't fix any crashes, and there's a small
chance of a regression for some hypervisor in the field.


-->

virtio-net: friendlier handling of misconfigured hypervisors

We currently trigger BUG when VIRTIO_NET_F_CTRL_VQ
is not set but one of features depending on it is.
That's not a friendly way to report errors to
hypervisors.
Let's check, and fail probe instead.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 26e1330..7a7d1a3 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1673,6 +1673,21 @@ static const struct attribute_group virtio_net_mrg_rx_group = {
 };
 #endif
 
+bool __virtnet_fail_on_feature(struct virtio_device *vdev, unsigned int fbit,
+			       const char *fname)
+{
+	if (!virtio_has_feature(vdev, fbit))
+		return false;
+
+        dev_err(&dev->dev, "missing requirements for feature bit %d: %s\n",
+		fbit, fname);
+
+	return true;
+}
+
+#define VIRTNET_FAIL_ON(vdev, fbit) \
+	__virtnet_fail_on_feature(vdev, fbit, #fbit)
+
 static int virtnet_probe(struct virtio_device *vdev)
 {
 	int i, err;
@@ -1680,6 +1695,14 @@ static int virtnet_probe(struct virtio_device *vdev)
 	struct virtnet_info *vi;
 	u16 max_queue_pairs;
 
+	if (!virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ) &&
+		(VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_RX) ||
+		 VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_VLAN) ||
+		 VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE) ||
+		 VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_MQ) ||
+		 VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)))
+		return -EINVAL;
+
 	/* Find if host supports multiqueue virtio_net device */
 	err = virtio_cread_feature(vdev, VIRTIO_NET_F_MQ,
 				   struct virtio_net_config,

Re: [PATCH net] virtio-net: validate features during probe

[Cornelia Huck]

On Wed, 19 Nov 2014 10:59:39 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:


> +bool __virtnet_fail_on_feature(struct virtio_device *vdev, unsigned int fbit,
> +			       const char *fname)
> +{
> +	if (!virtio_has_feature(vdev, fbit))
> +		return false;
> +
> +        dev_err(&dev->dev, "missing requirements for feature bit %d: %s\n",
> +		fbit, fname);

I'd like the message to point out that this is a hypervisor problem: I
can imagine that a user would be stumped about what to do about this.
And printing what requirements are missing would probably be helpful to
someone trying to fix the hypervisor.

> +
> +	return true;
> +}
> +
>

Re: [PATCH net] virtio-net: validate features during probe

[Jason Wang]

On 11/19/2014 04:59 PM, Michael S. Tsirkin wrote:
> On Wed, Nov 19, 2014 at 02:35:39PM +0800, Jason Wang wrote:
>> This patch validates feature dependencies during probe and fail the probing
>> if a dependency is missed. This fixes the issues of hitting BUG()
>> when qemu fails to advertise features correctly. One example is booting
>> guest with ctrl_vq=off through qemu.
>>
>> Cc: Rusty Russell <rusty@rustcorp.com.au>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
>> Cc: Wanlong Gao <gaowanlong@cn.fujitsu.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>  drivers/net/virtio_net.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 93 insertions(+)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index ec2a8b4..4a0ad46 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -1673,6 +1673,95 @@ static const struct attribute_group virtio_net_mrg_rx_group = {
>>  };
>>  #endif
>>  
>> +static int virtnet_validate_features(struct virtio_device *dev,
>> +				     unsigned int *table,
>> +				     int table_size,
>> +				     unsigned int feature)
>> +{
>> +	int i;
>> +
>> +	if (!virtio_has_feature(dev, feature)) {
>> +		for (i = 0; i < table_size; i++) {
>> +			unsigned int f = table[i];
>> +
>> +			if (virtio_has_feature(dev, f)) {
>> +				dev_err(&dev->dev,
>> +					"buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x was not",
>
> This line's way too long.

Yes. (Anyway it pass checkpatch.pl since it forbids quoted string to be
split)
>
>> +					f, feature);
>> +				return -EINVAL;
>> +			}
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int virtnet_check_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
>> +	};
>> +	unsigned int features_for_guest_csum[] = {
>> +		VIRTIO_NET_F_GUEST_TSO4,
>> +		VIRTIO_NET_F_GUEST_TSO6,
>> +		VIRTIO_NET_F_GUEST_ECN,
>> +		VIRTIO_NET_F_GUEST_UFO,
>> +	};
>> +	unsigned int features_for_host_csum[] = {
>> +		VIRTIO_NET_F_HOST_TSO4,
>> +		VIRTIO_NET_F_HOST_TSO6,
>> +		VIRTIO_NET_F_HOST_ECN,
>> +		VIRTIO_NET_F_HOST_UFO,
>> +	};
>> +	int err;
>> +
>> +	err = virtnet_validate_features(dev, features_for_ctrl_vq,
>> +					ARRAY_SIZE(features_for_ctrl_vq),
>> +					VIRTIO_NET_F_CTRL_VQ);
>> +	if (err)
>> +		return err;
>> +
>> +	err = virtnet_validate_features(dev, features_for_guest_csum,
>> +					ARRAY_SIZE(features_for_guest_csum),
>> +					VIRTIO_NET_F_GUEST_CSUM);
>> +	if (err)
>> +		return err;
>> +
>> +	err = virtnet_validate_features(dev, features_for_host_csum,
>> +					ARRAY_SIZE(features_for_host_csum),
>> +					VIRTIO_NET_F_CSUM);
>> +	if (err)
>> +		return err;
>> +
>> +	if (virtio_has_feature(dev, VIRTIO_NET_F_GUEST_ECN) &&
>> +	    (!virtio_has_feature(dev, VIRTIO_NET_F_GUEST_TSO4) ||
>> +	     !virtio_has_feature(dev, VIRTIO_NET_F_GUEST_TSO6))) {
>> +		dev_err(&dev->dev,
>> +			"buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x or 0x%x was not",
>> +			VIRTIO_NET_F_GUEST_ECN,
>> +			VIRTIO_NET_F_GUEST_TSO4,
>> +			VIRTIO_NET_F_GUEST_TSO6);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (virtio_has_feature(dev, VIRTIO_NET_F_HOST_ECN) &&
>> +	    (!virtio_has_feature(dev, VIRTIO_NET_F_HOST_TSO4) ||
>> +	     !virtio_has_feature(dev, VIRTIO_NET_F_HOST_TSO6))) {
>> +		dev_err(&dev->dev,
>> +			"buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x or 0x%x was not",
>> +			VIRTIO_NET_F_HOST_ECN,
>> +			VIRTIO_NET_F_HOST_TSO4,
>> +			VIRTIO_NET_F_HOST_TSO6);
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static int virtnet_probe(struct virtio_device *vdev)
>>  {
>>  	int i, err;
>> @@ -1680,6 +1769,10 @@ static int virtnet_probe(struct virtio_device *vdev)
>>  	struct virtnet_info *vi;
>>  	u16 max_queue_pairs;
>>  
>> +	err = virtnet_check_features(vdev);
>> +	if (err)
>> +		return -EINVAL;
>> +
>>  	/* Find if host supports multiqueue virtio_net device */
>>  	err = virtio_cread_feature(vdev, VIRTIO_NET_F_MQ,
>>  				   struct virtio_net_config,
> The API seems too complex, and you still had to open-code ECN logic.
> Just open-code most of it. 

Yes, the ECN could be done through the same way as ctrl_vq did.

How about move those checking into virtio core by just letting device
export its dependency table?
>  You can use a helper macro to output a
> friendly message without code duplication.
> For example like the below (completely untested)?
>
>
> I would also like to split things: dependencies on
> VIRTIO_NET_F_CTRL_VQ might go into this kernel,
> since they help fix BUG.
>
> Others should wait since they don't fix any crashes, and there's a small
> chance of a regression for some hypervisor in the field.

Probably ok but not sure, since the rest features are all related to
csum and offloading, we are in fact depends on network core to fix them.
>
> -->
>
> virtio-net: friendlier handling of misconfigured hypervisors
>
> We currently trigger BUG when VIRTIO_NET_F_CTRL_VQ
> is not set but one of features depending on it is.
> That's not a friendly way to report errors to
> hypervisors.
> Let's check, and fail probe instead.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> ---
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 26e1330..7a7d1a3 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1673,6 +1673,21 @@ static const struct attribute_group virtio_net_mrg_rx_group = {
>  };
>  #endif
>  
> +bool __virtnet_fail_on_feature(struct virtio_device *vdev, unsigned int fbit,
> +			       const char *fname)
> +{
> +	if (!virtio_has_feature(vdev, fbit))
> +		return false;
> +
> +        dev_err(&dev->dev, "missing requirements for feature bit %d: %s\n",
> +		fbit, fname);
> +
> +	return true;
> +}
> +
> +#define VIRTNET_FAIL_ON(vdev, fbit) \
> +	__virtnet_fail_on_feature(vdev, fbit, #fbit)
> +
>  static int virtnet_probe(struct virtio_device *vdev)
>  {
>  	int i, err;
> @@ -1680,6 +1695,14 @@ static int virtnet_probe(struct virtio_device *vdev)
>  	struct virtnet_info *vi;
>  	u16 max_queue_pairs;
>  
> +	if (!virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ) &&
> +		(VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_RX) ||
> +		 VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_VLAN) ||
> +		 VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE) ||
> +		 VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_MQ) ||
> +		 VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)))
> +		return -EINVAL;
> +
>  	/* Find if host supports multiqueue virtio_net device */
>  	err = virtio_cread_feature(vdev, VIRTIO_NET_F_MQ,
>  				   struct virtio_net_config,
>

Patch looks good, but consider we may check more dependencies in the
future, may need a helper instead. Or just use this and switch to
dependency table in 3.19. 

Re: [PATCH net] virtio-net: validate features during probe

[Michael S. Tsirkin]

On Wed, Nov 19, 2014 at 05:33:26PM +0800, Jason Wang wrote:
> On 11/19/2014 04:59 PM, Michael S. Tsirkin wrote:
> > On Wed, Nov 19, 2014 at 02:35:39PM +0800, Jason Wang wrote:
> >> This patch validates feature dependencies during probe and fail the probing
> >> if a dependency is missed. This fixes the issues of hitting BUG()
> >> when qemu fails to advertise features correctly. One example is booting
> >> guest with ctrl_vq=off through qemu.
> >>
> >> Cc: Rusty Russell <rusty@rustcorp.com.au>
> >> Cc: Michael S. Tsirkin <mst@redhat.com>
> >> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> >> Cc: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >> ---
> >>  drivers/net/virtio_net.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 93 insertions(+)
> >>
> >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >> index ec2a8b4..4a0ad46 100644
> >> --- a/drivers/net/virtio_net.c
> >> +++ b/drivers/net/virtio_net.c
> >> @@ -1673,6 +1673,95 @@ static const struct attribute_group virtio_net_mrg_rx_group = {
> >>  };
> >>  #endif
> >>  
> >> +static int virtnet_validate_features(struct virtio_device *dev,
> >> +				     unsigned int *table,
> >> +				     int table_size,
> >> +				     unsigned int feature)
> >> +{
> >> +	int i;
> >> +
> >> +	if (!virtio_has_feature(dev, feature)) {
> >> +		for (i = 0; i < table_size; i++) {
> >> +			unsigned int f = table[i];
> >> +
> >> +			if (virtio_has_feature(dev, f)) {
> >> +				dev_err(&dev->dev,
> >> +					"buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x was not",
> >
> > This line's way too long.
> 
> Yes. (Anyway it pass checkpatch.pl since it forbids quoted string to be
> split)
> >
> >> +					f, feature);
> >> +				return -EINVAL;
> >> +			}
> >> +		}
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int virtnet_check_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
> >> +	};
> >> +	unsigned int features_for_guest_csum[] = {
> >> +		VIRTIO_NET_F_GUEST_TSO4,
> >> +		VIRTIO_NET_F_GUEST_TSO6,
> >> +		VIRTIO_NET_F_GUEST_ECN,
> >> +		VIRTIO_NET_F_GUEST_UFO,
> >> +	};
> >> +	unsigned int features_for_host_csum[] = {
> >> +		VIRTIO_NET_F_HOST_TSO4,
> >> +		VIRTIO_NET_F_HOST_TSO6,
> >> +		VIRTIO_NET_F_HOST_ECN,
> >> +		VIRTIO_NET_F_HOST_UFO,
> >> +	};
> >> +	int err;
> >> +
> >> +	err = virtnet_validate_features(dev, features_for_ctrl_vq,
> >> +					ARRAY_SIZE(features_for_ctrl_vq),
> >> +					VIRTIO_NET_F_CTRL_VQ);
> >> +	if (err)
> >> +		return err;
> >> +
> >> +	err = virtnet_validate_features(dev, features_for_guest_csum,
> >> +					ARRAY_SIZE(features_for_guest_csum),
> >> +					VIRTIO_NET_F_GUEST_CSUM);
> >> +	if (err)
> >> +		return err;
> >> +
> >> +	err = virtnet_validate_features(dev, features_for_host_csum,
> >> +					ARRAY_SIZE(features_for_host_csum),
> >> +					VIRTIO_NET_F_CSUM);
> >> +	if (err)
> >> +		return err;
> >> +
> >> +	if (virtio_has_feature(dev, VIRTIO_NET_F_GUEST_ECN) &&
> >> +	    (!virtio_has_feature(dev, VIRTIO_NET_F_GUEST_TSO4) ||
> >> +	     !virtio_has_feature(dev, VIRTIO_NET_F_GUEST_TSO6))) {
> >> +		dev_err(&dev->dev,
> >> +			"buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x or 0x%x was not",
> >> +			VIRTIO_NET_F_GUEST_ECN,
> >> +			VIRTIO_NET_F_GUEST_TSO4,
> >> +			VIRTIO_NET_F_GUEST_TSO6);
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	if (virtio_has_feature(dev, VIRTIO_NET_F_HOST_ECN) &&
> >> +	    (!virtio_has_feature(dev, VIRTIO_NET_F_HOST_TSO4) ||
> >> +	     !virtio_has_feature(dev, VIRTIO_NET_F_HOST_TSO6))) {
> >> +		dev_err(&dev->dev,
> >> +			"buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x or 0x%x was not",
> >> +			VIRTIO_NET_F_HOST_ECN,
> >> +			VIRTIO_NET_F_HOST_TSO4,
> >> +			VIRTIO_NET_F_HOST_TSO6);
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  static int virtnet_probe(struct virtio_device *vdev)
> >>  {
> >>  	int i, err;
> >> @@ -1680,6 +1769,10 @@ static int virtnet_probe(struct virtio_device *vdev)
> >>  	struct virtnet_info *vi;
> >>  	u16 max_queue_pairs;
> >>  
> >> +	err = virtnet_check_features(vdev);
> >> +	if (err)
> >> +		return -EINVAL;
> >> +
> >>  	/* Find if host supports multiqueue virtio_net device */
> >>  	err = virtio_cread_feature(vdev, VIRTIO_NET_F_MQ,
> >>  				   struct virtio_net_config,
> > The API seems too complex, and you still had to open-code ECN logic.
> > Just open-code most of it. 
> 
> Yes, the ECN could be done through the same way as ctrl_vq did.
> 
> How about move those checking into virtio core by just letting device
> export its dependency table?

So far we only have this for net, let's not add
one-off APIs.

> >  You can use a helper macro to output a
> > friendly message without code duplication.
> > For example like the below (completely untested)?
> >
> >
> > I would also like to split things: dependencies on
> > VIRTIO_NET_F_CTRL_VQ might go into this kernel,
> > since they help fix BUG.
> >
> > Others should wait since they don't fix any crashes, and there's a small
> > chance of a regression for some hypervisor in the field.
> 
> Probably ok but not sure, since the rest features are all related to
> csum and offloading, we are in fact depends on network core to fix them.

Well it does fix them so ... there's no bug, is there?


> >
> > -->
> >
> > virtio-net: friendlier handling of misconfigured hypervisors
> >
> > We currently trigger BUG when VIRTIO_NET_F_CTRL_VQ
> > is not set but one of features depending on it is.
> > That's not a friendly way to report errors to
> > hypervisors.
> > Let's check, and fail probe instead.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > ---
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 26e1330..7a7d1a3 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -1673,6 +1673,21 @@ static const struct attribute_group virtio_net_mrg_rx_group = {
> >  };
> >  #endif
> >  
> > +bool __virtnet_fail_on_feature(struct virtio_device *vdev, unsigned int fbit,
> > +			       const char *fname)
> > +{
> > +	if (!virtio_has_feature(vdev, fbit))
> > +		return false;
> > +
> > +        dev_err(&dev->dev, "missing requirements for feature bit %d: %s\n",
> > +		fbit, fname);
> > +
> > +	return true;
> > +}
> > +
> > +#define VIRTNET_FAIL_ON(vdev, fbit) \
> > +	__virtnet_fail_on_feature(vdev, fbit, #fbit)
> > +
> >  static int virtnet_probe(struct virtio_device *vdev)
> >  {
> >  	int i, err;
> > @@ -1680,6 +1695,14 @@ static int virtnet_probe(struct virtio_device *vdev)
> >  	struct virtnet_info *vi;
> >  	u16 max_queue_pairs;
> >  
> > +	if (!virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ) &&
> > +		(VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_RX) ||
> > +		 VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_VLAN) ||
> > +		 VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE) ||
> > +		 VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_MQ) ||
> > +		 VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)))
> > +		return -EINVAL;
> > +
> >  	/* Find if host supports multiqueue virtio_net device */
> >  	err = virtio_cread_feature(vdev, VIRTIO_NET_F_MQ,
> >  				   struct virtio_net_config,
> >
> 
> Patch looks good, but consider we may check more dependencies in the
> future, may need a helper instead. Or just use this and switch to
> dependency table in 3.19. 

Pls note this is just pseudo-code - I didn't even compile it.
If you want something like this merged, go ahead and
post it, probably addressing Cornelia's request to
print the dependency too. Maybe:

> >		(VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_RX, "ctrl_vq") ||
> >		 VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_VLAN, "ctrl_vq") ||
> >		 VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE, "ctrl_vq") ||
> >		 VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_MQ, "ctrl_vq") ||
> >		 VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_MAC_ADDR, "ctrl_vq")))

-- 
MST

Re: [PATCH net] virtio-net: validate features during probe

[Jason Wang]

On 11/19/2014 05:39 PM, Michael S. Tsirkin wrote:
> On Wed, Nov 19, 2014 at 05:33:26PM +0800, Jason Wang wrote:
>> On 11/19/2014 04:59 PM, Michael S. Tsirkin wrote:
>>> On Wed, Nov 19, 2014 at 02:35:39PM +0800, Jason Wang wrote:
>>>> This patch validates feature dependencies during probe and fail the probing
>>>> if a dependency is missed. This fixes the issues of hitting BUG()
>>>> when qemu fails to advertise features correctly. One example is booting
>>>> guest with ctrl_vq=off through qemu.
>>>>
>>>> Cc: Rusty Russell <rusty@rustcorp.com.au>
>>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>>> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
>>>> Cc: Wanlong Gao <gaowanlong@cn.fujitsu.com>
>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>> ---
>>>>  drivers/net/virtio_net.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 93 insertions(+)
>>>>
>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>> index ec2a8b4..4a0ad46 100644
>>>> --- a/drivers/net/virtio_net.c
>>>> +++ b/drivers/net/virtio_net.c
>>>> @@ -1673,6 +1673,95 @@ static const struct attribute_group virtio_net_mrg_rx_group = {
>>>>  };
>>>>  #endif
>>>>  
>>>> +static int virtnet_validate_features(struct virtio_device *dev,
>>>> +				     unsigned int *table,
>>>> +				     int table_size,
>>>> +				     unsigned int feature)
>>>> +{
>>>> +	int i;
>>>> +
>>>> +	if (!virtio_has_feature(dev, feature)) {
>>>> +		for (i = 0; i < table_size; i++) {
>>>> +			unsigned int f = table[i];
>>>> +
>>>> +			if (virtio_has_feature(dev, f)) {
>>>> +				dev_err(&dev->dev,
>>>> +					"buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x was not",
>>> This line's way too long.
>> Yes. (Anyway it pass checkpatch.pl since it forbids quoted string to be
>> split)
[...]
>>>> +
>>>>  static int virtnet_probe(struct virtio_device *vdev)
>>>>  {
>>>>  	int i, err;
>>>> @@ -1680,6 +1769,10 @@ static int virtnet_probe(struct virtio_device *vdev)
>>>>  	struct virtnet_info *vi;
>>>>  	u16 max_queue_pairs;
>>>>  
>>>> +	err = virtnet_check_features(vdev);
>>>> +	if (err)
>>>> +		return -EINVAL;
>>>> +
>>>>  	/* Find if host supports multiqueue virtio_net device */
>>>>  	err = virtio_cread_feature(vdev, VIRTIO_NET_F_MQ,
>>>>  				   struct virtio_net_config,
>>> The API seems too complex, and you still had to open-code ECN logic.
>>> Just open-code most of it. 
>> Yes, the ECN could be done through the same way as ctrl_vq did.
>>
>> How about move those checking into virtio core by just letting device
>> export its dependency table?
> So far we only have this for net, let's not add
> one-off APIs.
>
>>>  You can use a helper macro to output a
>>> friendly message without code duplication.
>>> For example like the below (completely untested)?
>>>
>>>
>>> I would also like to split things: dependencies on
>>> VIRTIO_NET_F_CTRL_VQ might go into this kernel,
>>> since they help fix BUG.
>>>
>>> Others should wait since they don't fix any crashes, and there's a small
>>> chance of a regression for some hypervisor in the field.
>> Probably ok but not sure, since the rest features are all related to
>> csum and offloading, we are in fact depends on network core to fix them.
> Well it does fix them so ... there's no bug, is there?
>

No.
>>> -->
>>>
>>> virtio-net: friendlier handling of misconfigured hypervisors
>>>
>>> We currently trigger BUG when VIRTIO_NET_F_CTRL_VQ
>>> is not set but one of features depending on it is.
>>> That's not a friendly way to report errors to
>>> hypervisors.
>>> Let's check, and fail probe instead.
>>>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>
>>> ---
>>>
>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>> index 26e1330..7a7d1a3 100644
>>> --- a/drivers/net/virtio_net.c
>>> +++ b/drivers/net/virtio_net.c
>>> @@ -1673,6 +1673,21 @@ static const struct attribute_group virtio_net_mrg_rx_group = {
>>>  };
>>>  #endif
>>>  
>>> +bool __virtnet_fail_on_feature(struct virtio_device *vdev, unsigned int fbit,
>>> +			       const char *fname)
>>> +{
>>> +	if (!virtio_has_feature(vdev, fbit))
>>> +		return false;
>>> +
>>> +        dev_err(&dev->dev, "missing requirements for feature bit %d: %s\n",
>>> +		fbit, fname);
>>> +
>>> +	return true;
>>> +}
>>> +
>>> +#define VIRTNET_FAIL_ON(vdev, fbit) \
>>> +	__virtnet_fail_on_feature(vdev, fbit, #fbit)
>>> +
>>>  static int virtnet_probe(struct virtio_device *vdev)
>>>  {
>>>  	int i, err;
>>> @@ -1680,6 +1695,14 @@ static int virtnet_probe(struct virtio_device *vdev)
>>>  	struct virtnet_info *vi;
>>>  	u16 max_queue_pairs;
>>>  
>>> +	if (!virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ) &&
>>> +		(VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_RX) ||
>>> +		 VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_VLAN) ||
>>> +		 VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE) ||
>>> +		 VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_MQ) ||
>>> +		 VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)))
>>> +		return -EINVAL;
>>> +
>>>  	/* Find if host supports multiqueue virtio_net device */
>>>  	err = virtio_cread_feature(vdev, VIRTIO_NET_F_MQ,
>>>  				   struct virtio_net_config,
>>>
>> Patch looks good, but consider we may check more dependencies in the
>> future, may need a helper instead. Or just use this and switch to
>> dependency table in 3.19. 
> Pls note this is just pseudo-code - I didn't even compile it.
> If you want something like this merged, go ahead and
> post it, probably addressing Cornelia's request to
> print the dependency too. Maybe:

Ok, will post v3.
>
>>> 		(VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_RX, "ctrl_vq") ||
>>> 		 VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_VLAN, "ctrl_vq") ||
>>> 		 VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE, "ctrl_vq") ||
>>> 		 VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_MQ, "ctrl_vq") ||
>>> 		 VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_MAC_ADDR, "ctrl_vq")))