linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] virtio: allow drivers to validate features
@ 2017-03-29 17:14 Michael S. Tsirkin
  2017-03-29 17:14 ` [PATCH 2/2] virtio_net: clear MTU when out of range Michael S. Tsirkin
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2017-03-29 17:14 UTC (permalink / raw)
  To: linux-kernel; +Cc: Jason Wang, virtualization, netdev

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH 2/2] virtio_net: clear MTU when out of range
  2017-03-29 17:14 [PATCH 1/2] virtio: allow drivers to validate features Michael S. Tsirkin
@ 2017-03-29 17:14 ` Michael S. Tsirkin
  2017-03-30  9:06 ` [PATCH 1/2] virtio: allow drivers to validate features Cornelia Huck
  2017-03-30 19:39 ` David Miller
  2 siblings, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2017-03-29 17:14 UTC (permalink / raw)
  To: linux-kernel; +Cc: Jason Wang, virtualization, netdev

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] virtio: allow drivers to validate features
  2017-03-29 17:14 [PATCH 1/2] virtio: allow drivers to validate features Michael S. Tsirkin
  2017-03-29 17:14 ` [PATCH 2/2] virtio_net: clear MTU when out of range Michael S. Tsirkin
@ 2017-03-30  9:06 ` Cornelia Huck
  2017-03-30 14:57   ` Michael S. Tsirkin
  2017-03-30 19:39 ` David Miller
  2 siblings, 1 reply; 6+ messages in thread
From: Cornelia Huck @ 2017-03-30  9:06 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: linux-kernel, netdev, virtualization

On Wed, 29 Mar 2017 20:14:44 +0300
"Michael S. Tsirkin" <mst@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@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...

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] virtio: allow drivers to validate features
  2017-03-30  9:06 ` [PATCH 1/2] virtio: allow drivers to validate features Cornelia Huck
@ 2017-03-30 14:57   ` Michael S. Tsirkin
  0 siblings, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2017-03-30 14:57 UTC (permalink / raw)
  To: Cornelia Huck; +Cc: linux-kernel, netdev, virtualization

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@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@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.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] virtio: allow drivers to validate features
  2017-03-29 17:14 [PATCH 1/2] virtio: allow drivers to validate features Michael S. Tsirkin
  2017-03-29 17:14 ` [PATCH 2/2] virtio_net: clear MTU when out of range Michael S. Tsirkin
  2017-03-30  9:06 ` [PATCH 1/2] virtio: allow drivers to validate features Cornelia Huck
@ 2017-03-30 19:39 ` David Miller
  2017-03-31  3:27   ` Michael S. Tsirkin
  2 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2017-03-30 19:39 UTC (permalink / raw)
  To: mst; +Cc: linux-kernel, jasowang, virtualization, netdev

From: "Michael S. Tsirkin" <mst@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@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.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] virtio: allow drivers to validate features
  2017-03-30 19:39 ` David Miller
@ 2017-03-31  3:27   ` Michael S. Tsirkin
  0 siblings, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2017-03-31  3:27 UTC (permalink / raw)
  To: David Miller; +Cc: linux-kernel, jasowang, virtualization, netdev

On Thu, Mar 30, 2017 at 12:39:31PM -0700, David Miller wrote:
> From: "Michael S. Tsirkin" <mst@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@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

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-03-31  3:27 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-29 17:14 [PATCH 1/2] virtio: allow drivers to validate features Michael S. Tsirkin
2017-03-29 17:14 ` [PATCH 2/2] virtio_net: clear MTU when out of range Michael S. Tsirkin
2017-03-30  9:06 ` [PATCH 1/2] virtio: allow drivers to validate features Cornelia Huck
2017-03-30 14:57   ` Michael S. Tsirkin
2017-03-30 19:39 ` David Miller
2017-03-31  3:27   ` Michael S. Tsirkin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).