virtio_net: MAC address releated breakage if there is no MAC area in config

virtio_net: MAC address releated breakage if there is no MAC area in config

[Christian Borntraeger]

Hello,

commit 9c46f6d42f1b5627c49a5906cb5b315ad8716ff0
Author: Alex Williamson <alex.williamson@hp.com>
Date:   Wed Feb 4 16:36:34 2009 -0800
virtio_net: Allow setting the MAC address of the NIC

Introduced an unconditional config->set to the MAC address field of
the device config.


+       } else {
                random_ether_addr(dev->dev_addr);
+               vdev->config->set(vdev, offsetof(struct virtio_net_config, mac),
+                                 dev->dev_addr, dev->addr_len);
+       }

Since our kuli userspace sample does not set VIRTIO_NET_F_MAC, there is no
config space assigned for this device. When virtio_net tries to overwrite
the non-existing field, this triggers a bug.

virtio_net.h specifies:
struct virtio_net_config
{
        /* The config defining mac address (if VIRTIO_NET_F_MAC) */
        __u8 mac[6];
[...]

I read this as the mac config field is optional (similar to all the optional
fields we added in virtio_blk later).

I see two options:
1. Change our sample userspace to always allocate the config (like lguest and
qemu)
2. Change the kernel code to not write into the config unless a specific feature
bit is set. (e.g. VIRTIO_NET_F_SETMAC)


Opinions?

PS: The same is true for virtnet_set_mac_address. it crashes as well

Re: virtio_net: MAC address releated breakage if there is no MAC area in config

[Alex Williamson]

On Thu, 2009-04-02 at 13:33 +0200, Christian Borntraeger wrote:
> I read this as the mac config field is optional (similar to all the optional
> fields we added in virtio_blk later).
> 
> I see two options:
> 1. Change our sample userspace to always allocate the config (like lguest and
> qemu)
> 2. Change the kernel code to not write into the config unless a specific feature
> bit is set. (e.g. VIRTIO_NET_F_SETMAC)
> 
> 
> Opinions?

Hi Christian,

Sorry for the breakage.  My interpretation of the virtio-net config
space was that the mac field was always present and the host had
programmed a valid value when the F_MAC feature is available.  However,
from the history of the flag, it seems like you're interpretation is
likely correct.  Setting the config value from the randomly generated
mac was largely opportunistic since there's no userspace that doesn't
provide a mac by default.  So perhaps we can drop that and gate the
set_mac_address entry point as shown below.  How does this look?
Thanks,

Alex


virtio_net: Set the mac config only when VIRITO_NET_F_MAC

VIRTIO_NET_F_MAC indicates the presence of the mac field in config
space, not the validity of the value it contains.  Allow the mac to be
changed at runtime, but only push the change into config space with the
VIRTIO_NET_F_MAC feature present.

Signed-off-by: Alex Williamson <alex.williamson@hp.com>
--

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index a6f1e19..9c82a39 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -575,8 +575,9 @@ static int virtnet_set_mac_address(struct net_device *dev, void *p)
 	if (ret)
 		return ret;
 
-	vdev->config->set(vdev, offsetof(struct virtio_net_config, mac),
-			  dev->dev_addr, dev->addr_len);
+	if (virtio_has_feature(vdev, VIRTIO_NET_F_MAC))
+		vdev->config->set(vdev, offsetof(struct virtio_net_config, mac),
+		                  dev->dev_addr, dev->addr_len);
 
 	return 0;
 }
@@ -876,11 +877,8 @@ static int virtnet_probe(struct virtio_device *vdev)
 		vdev->config->get(vdev,
 				  offsetof(struct virtio_net_config, mac),
 				  dev->dev_addr, dev->addr_len);
-	} else {
+	} else
 		random_ether_addr(dev->dev_addr);
-		vdev->config->set(vdev, offsetof(struct virtio_net_config, mac),
-				  dev->dev_addr, dev->addr_len);
-	}
 
 	/* Set up our device-specific information */
 	vi = netdev_priv(dev);

Re: virtio_net: MAC address releated breakage if there is no MAC area in config

[Christian Borntraeger]

Am Thursday 02 April 2009 18:06:25 schrieb Alex Williamson:
> On Thu, 2009-04-02 at 13:33 +0200, Christian Borntraeger wrote:
> > I read this as the mac config field is optional (similar to all the optional
> > fields we added in virtio_blk later).
[...]
> Sorry for the breakage.  My interpretation of the virtio-net config
> space was that the mac field was always present and the host had
> programmed a valid value when the F_MAC feature is available.  However,
> from the history of the flag, it seems like you're interpretation is
> likely correct.  Setting the config value from the randomly generated
> mac was largely opportunistic since there's no userspace that doesn't
> provide a mac by default.  So perhaps we can drop that and gate the
> set_mac_address entry point as shown below.  How does this look?
> Thanks,

that patch would solve the my problem. Thanks.

In addition, I will change our hypervisor sample code, to provide the
config space even if we do not set a MAC address in the host. Better
safe than sorry.

[...]
> virtio_net: Set the mac config only when VIRITO_NET_F_MAC
> 
> VIRTIO_NET_F_MAC indicates the presence of the mac field in config
> space, not the validity of the value it contains.  Allow the mac to be
> changed at runtime, but only push the change into config space with the
> VIRTIO_NET_F_MAC feature present.
> 
> Signed-off-by: Alex Williamson <alex.williamson@hp.com>

Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>

> --
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index a6f1e19..9c82a39 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -575,8 +575,9 @@ static int virtnet_set_mac_address(struct net_device *dev, void *p)
>  	if (ret)
>  		return ret;
> 
> -	vdev->config->set(vdev, offsetof(struct virtio_net_config, mac),
> -			  dev->dev_addr, dev->addr_len);
> +	if (virtio_has_feature(vdev, VIRTIO_NET_F_MAC))
> +		vdev->config->set(vdev, offsetof(struct virtio_net_config, mac),
> +		                  dev->dev_addr, dev->addr_len);
> 
>  	return 0;
>  }
> @@ -876,11 +877,8 @@ static int virtnet_probe(struct virtio_device *vdev)
>  		vdev->config->get(vdev,
>  				  offsetof(struct virtio_net_config, mac),
>  				  dev->dev_addr, dev->addr_len);
> -	} else {
> +	} else
>  		random_ether_addr(dev->dev_addr);
> -		vdev->config->set(vdev, offsetof(struct virtio_net_config, mac),
> -				  dev->dev_addr, dev->addr_len);
> -	}
> 
>  	/* Set up our device-specific information */
>  	vi = netdev_priv(dev);

Re: virtio_net: MAC address releated breakage if there is no MAC area in config

[David Miller]

From: Christian Borntraeger <borntraeger@de.ibm.com>
Date: Thu, 2 Apr 2009 19:23:48 +0200

> Am Thursday 02 April 2009 18:06:25 schrieb Alex Williamson:
> > virtio_net: Set the mac config only when VIRITO_NET_F_MAC
> > 
> > VIRTIO_NET_F_MAC indicates the presence of the mac field in config
> > space, not the validity of the value it contains.  Allow the mac to be
> > changed at runtime, but only push the change into config space with the
> > VIRTIO_NET_F_MAC feature present.
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@hp.com>
> 
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>

Applied, thanks!

Re: virtio_net: MAC address releated breakage if there is no MAC area in config

[Rusty Russell]

On Thursday 02 April 2009 22:03:24 Christian Borntraeger wrote:
> I read this as the mac config field is optional (similar to all the optional
> fields we added in virtio_blk later).
> 
> I see two options:
> 1. Change our sample userspace to always allocate the config (like lguest and
> qemu)
> 2. Change the kernel code to not write into the config unless a specific feature
> bit is set. (e.g. VIRTIO_NET_F_SETMAC)

I had assumed it would always exist.  Agreed it's unclear; I'm currently
writing down the actual standard to avoid such mistakes in the future.

Cheers,
Rusty.

Re: virtio_net: MAC address releated breakage if there is no MAC area in config

[Rusty Russell]

On Sunday 05 April 2009 09:10:31 David Miller wrote:
> > Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> Applied, thanks!

Hmm, doesn't really matter, but it's unnecessary.

Cheers,
Rusty.