netdev - Re: [PATCH v3 2/2] virtio-net: introduce a new control to set macaddr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20130120022027.GA2184@t430s.redhat.com>
Date:	Sun, 20 Jan 2013 10:20:27 +0800
From:	Amos Kong <akong@...hat.com>
To:	Stefan Hajnoczi <stefanha@...il.com>
Cc:	mst@...hat.com, kvm@...r.kernel.org, netdev@...r.kernel.org,
	qemu-devel@...gnu.org, virtualization@...ts.linux-foundation.org,
	davem@...emloft.net
Subject: Re: [PATCH v3 2/2] virtio-net: introduce a new control to set macaddr

On Fri, Jan 18, 2013 at 12:00:42PM +0100, Stefan Hajnoczi wrote:
> On Thu, Jan 17, 2013 at 06:40:12PM +0800, akong@...hat.com wrote:
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 395ab4f..837c978 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -802,14 +802,32 @@ static int virtnet_set_mac_address(struct net_device *dev, void *p)
> >  	struct virtnet_info *vi = netdev_priv(dev);
> >  	struct virtio_device *vdev = vi->vdev;
> >  	int ret;
> > +	struct scatterlist sg;
> > +	char save_addr[ETH_ALEN];
> > +	unsigned char save_aatype;
> > +
> > +	memcpy(save_addr, dev->dev_addr, ETH_ALEN);
> > +	save_aatype = dev->addr_assign_type;
> >  
> >  	ret = eth_mac_addr(dev, p);
> >  	if (ret)
> >  		return ret;
> >  
> > -	if (virtio_has_feature(vdev, VIRTIO_NET_F_MAC))
> > +	if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)) {
> > +		sg_init_one(&sg, dev->dev_addr, dev->addr_len);
> > +		if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC,
> > +					  VIRTIO_NET_CTRL_MAC_ADDR_SET,
> > +					  &sg, 1, 0)) {
> > +			dev_warn(&vdev->dev,
> > +				 "Failed to set mac address by vq command.\n");
> > +			memcpy(dev->dev_addr, save_addr, ETH_ALEN);
> > +			dev->addr_assign_type = save_aatype;
> > +			return -EINVAL;
> > +		}
> 
> eth_mac_addr() doesn't allow callers to implement error handling nicely.
> Although you didn't duplicate it's code directly, this patch still leaks
> internals of eth_mac_addr().
> 
> How about splitting eth_mac_addr() in a separate patch:

Agree, then we can have nice error handling.
I will send a V4 and include this patch.
Thanks.
 
> int eth_prepare_mac_addr_change(struct net_device *dev, void *p)
> {
>         struct sockaddr *addr = p;
>         if (!(dev->priv_flags & IFF_LIVE_ADDR_CHANGE) && netif_running(dev))
>                 return -EBUSY;
>         if (!is_valid_ether_addr(addr->sa_data))
>                 return -EADDRNOTAVAIL;
> 	return 0;
> }

...

> Now virtio_net.c does:
> 
> ret = eth_prepare_mac_addr_change(dev, p);
> if (ret < 0)
> 	return ret;
> 
> if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)) {
> 	sg_init_one(&sg, dev->dev_addr, dev->addr_len);

         trivial issue, %s/dev->dev_addr/addr->sa_data

> 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC,
> 				  VIRTIO_NET_CTRL_MAC_ADDR_SET,
> 				  &sg, 1, 0)) {
> 		dev_warn(&vdev->dev,
> 			 "Failed to set mac address by vq command.\n");
> 		return -EINVAL;
> 	}
> } ...
> 
> eth_commit_mac_addr_change(dev, p);
> return 0;
> 
> Stefan
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html