Linux Virtualization - Mar 2017 - [PATCH 1/2] virtio: allow drivers to validate features

Some drivers can't support all features in all configurations.  At the
moment we blindly set FEATURES_OK and later FAILED.  Support this better
by adding a callback drivers can use to do some early checks.

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 drivers/virtio/virtio.c | 6 ++++++
 include/linux/virtio.h  | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 400d70b..48230a5 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -232,6 +232,12 @@ static int virtio_dev_probe(struct device *_d)
 		if (device_features & (1ULL << i))
 			__virtio_set_bit(dev, i);
 
+	if (drv->validate) {
+		err = drv->validate(dev);
+		if (err)
+			goto err;
+	}
+
 	err = virtio_finalize_features(dev);
 	if (err)
 		goto err;
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 193fea9..ed04753 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -176,6 +176,7 @@ struct virtio_driver {
 	unsigned int feature_table_size;
 	const unsigned int *feature_table_legacy;
 	unsigned int feature_table_size_legacy;
+	int (*validate)(struct virtio_device *dev);
 	int (*probe)(struct virtio_device *dev);
 	void (*scan)(struct virtio_device *dev);
 	void (*remove)(struct virtio_device *dev);
-- 
MST

virtio attempts to clear the MTU feature bit if the value is out of the
supported range, but this has no real effect since FEATURES_OK has
already been set.

Fix this up by checking the MTU in the new validate callback.

Fixes: 14de9d114a82 ("virtio-net: Add initial MTU advice feature")
Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 drivers/net/virtio_net.c | 41 ++++++++++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 11 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index f6a379d..6aba098 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2312,14 +2312,8 @@ static bool virtnet_validate_features(struct
virtio_device *vdev)
 #define MIN_MTU ETH_MIN_MTU
 #define MAX_MTU ETH_MAX_MTU
 
-static int virtnet_probe(struct virtio_device *vdev)
+static int virtnet_validate(struct virtio_device *vdev)
 {
-	int i, err;
-	struct net_device *dev;
-	struct virtnet_info *vi;
-	u16 max_queue_pairs;
-	int mtu;
-
 	if (!vdev->config->get) {
 		dev_err(&vdev->dev, "%s failure: config access disabled\n",
 			__func__);
@@ -2329,6 +2323,25 @@ static int virtnet_probe(struct virtio_device *vdev)
 	if (!virtnet_validate_features(vdev))
 		return -EINVAL;
 
+	if (virtio_has_feature(vdev, VIRTIO_NET_F_MTU)) {
+		int mtu = virtio_cread16(vdev,
+					 offsetof(struct virtio_net_config,
+						  mtu));
+		if (mtu < MIN_MTU)
+			__virtio_clear_bit(vdev, VIRTIO_NET_F_MTU);
+	}
+
+	return 0;
+}
+
+static int virtnet_probe(struct virtio_device *vdev)
+{
+	int i, err;
+	struct net_device *dev;
+	struct virtnet_info *vi;
+	u16 max_queue_pairs;
+	int mtu;
+
 	/* Find if host supports multiqueue virtio_net device */
 	err = virtio_cread_feature(vdev, VIRTIO_NET_F_MQ,
 				   struct virtio_net_config,
@@ -2444,12 +2457,17 @@ static int virtnet_probe(struct virtio_device *vdev)
 				     offsetof(struct virtio_net_config,
 					      mtu));
 		if (mtu < dev->min_mtu) {
-			__virtio_clear_bit(vdev, VIRTIO_NET_F_MTU);
-		} else {
-			dev->mtu = mtu;
-			dev->max_mtu = mtu;
+			/* Should never trigger: MTU was previously validated
+			 * in virtnet_validate.
+			 */
+			dev_err(&vdev->dev, "device MTU appears to have changed "
+				"it is now %d < %d", mtu, dev->min_mtu);
+			goto free_stats;
 		}
 
+		dev->mtu = mtu;
+		dev->max_mtu = mtu;
+
 		/* TODO: size buffers correctly in this case. */
 		if (dev->mtu > ETH_DATA_LEN)
 			vi->big_packets = true;
@@ -2630,6 +2648,7 @@ static struct virtio_driver virtio_net_driver = {
 	.driver.name =	KBUILD_MODNAME,
 	.driver.owner =	THIS_MODULE,
 	.id_table =	id_table,
+	.validate =	virtnet_validate,
 	.probe =	virtnet_probe,
 	.remove =	virtnet_remove,
 	.config_changed = virtnet_config_changed,
-- 
MST

On Wed, 29 Mar 2017 20:14:44 +0300
"Michael S. Tsirkin" <mst at redhat.com> wrote:
> Some drivers can't support all features in all configurations.  At the
> moment we blindly set FEATURES_OK and later FAILED.  Support this better
> by adding a callback drivers can use to do some early checks.
Looks reasonable. Do we need to document that the driver must not do
anything beyond dealing with features and reading the config space that
early?
> 
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> ---
>  drivers/virtio/virtio.c | 6 ++++++
>  include/linux/virtio.h  | 1 +
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 400d70b..48230a5 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -232,6 +232,12 @@ static int virtio_dev_probe(struct device *_d)
>  		if (device_features & (1ULL << i))
>  			__virtio_set_bit(dev, i);
>  
> +	if (drv->validate) {
> +		err = drv->validate(dev);
> +		if (err)
> +			goto err;
> +	}
> +
>  	err = virtio_finalize_features(dev);
>  	if (err)
>  		goto err;
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index 193fea9..ed04753 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -176,6 +176,7 @@ struct virtio_driver {
>  	unsigned int feature_table_size;
>  	const unsigned int *feature_table_legacy;
>  	unsigned int feature_table_size_legacy;
> +	int (*validate)(struct virtio_device *dev);
>  	int (*probe)(struct virtio_device *dev);
>  	void (*scan)(struct virtio_device *dev);
>  	void (*remove)(struct virtio_device *dev);
Would be good to add some doc; but other members are undocumented here
already...

On Thu, Mar 30, 2017 at 11:06:27AM +0200, Cornelia Huck
wrote:
> On Wed, 29 Mar 2017 20:14:44 +0300
> "Michael S. Tsirkin" <mst at redhat.com> wrote:
> 
> > Some drivers can't support all features in all configurations.  At
the
> > moment we blindly set FEATURES_OK and later FAILED.  Support this
better
> > by adding a callback drivers can use to do some early checks.
> 
> Looks reasonable. Do we need to document that the driver must not do
> anything beyond dealing with features and reading the config space that
> early?
It's up to the driver - we probably should document that on failure
neither probe nor remove will be called. On success we proceed
to probe.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> > ---
> >  drivers/virtio/virtio.c | 6 ++++++
> >  include/linux/virtio.h  | 1 +
> >  2 files changed, 7 insertions(+)
> > 
> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > index 400d70b..48230a5 100644
> > --- a/drivers/virtio/virtio.c
> > +++ b/drivers/virtio/virtio.c
> > @@ -232,6 +232,12 @@ static int virtio_dev_probe(struct device *_d)
> >  		if (device_features & (1ULL << i))
> >  			__virtio_set_bit(dev, i);
> >  
> > +	if (drv->validate) {
> > +		err = drv->validate(dev);
> > +		if (err)
> > +			goto err;
> > +	}
> > +
> >  	err = virtio_finalize_features(dev);
> >  	if (err)
> >  		goto err;
> > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > index 193fea9..ed04753 100644
> > --- a/include/linux/virtio.h
> > +++ b/include/linux/virtio.h
> > @@ -176,6 +176,7 @@ struct virtio_driver {
> >  	unsigned int feature_table_size;
> >  	const unsigned int *feature_table_legacy;
> >  	unsigned int feature_table_size_legacy;
> > +	int (*validate)(struct virtio_device *dev);
> >  	int (*probe)(struct virtio_device *dev);
> >  	void (*scan)(struct virtio_device *dev);
> >  	void (*remove)(struct virtio_device *dev);
> 
> Would be good to add some doc; but other members are undocumented here
> already...
True. Patches welcome.

From: "Michael S. Tsirkin" <mst at redhat.com>
Date: Wed, 29 Mar 2017 20:14:44 +0300
> Some drivers can't support all features in all configurations.  At the
> moment we blindly set FEATURES_OK and later FAILED.  Support this better
> by adding a callback drivers can use to do some early checks.
> 
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
Michael do you want me to take these virtio networking fixes into my
tree directly or are you going to send me a pull request or something
after it all settles down?

Thanks.

On Thu, Mar 30, 2017 at 12:39:31PM -0700, David Miller
wrote:
> From: "Michael S. Tsirkin" <mst at redhat.com>
> Date: Wed, 29 Mar 2017 20:14:44 +0300
> 
> > Some drivers can't support all features in all configurations.  At
the
> > moment we blindly set FEATURES_OK and later FAILED.  Support this
better
> > by adding a callback drivers can use to do some early checks.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> 
> Michael do you want me to take these virtio networking fixes into my
> tree directly or are you going to send me a pull request or something
> after it all settles down?
> 
> Thanks.
I think I'll send a pull request.

Thanks,

-- 
MST