Linux Virtualization - Dec 2013 - [PATCH net-next 1/3] virtio_net: set multicast filter list to host

The virtio_net driver never sends the multicast address list to
the host. This is because send command takes a pointer to scatter list
to send but only inserts that one entry into the outgoing scatter list.

This bug has been there since:
commit f565a7c259d71cc186753653d978c646d2354b36
Author: Alex Williamson <alex.williamson at hp.com>
Date:   Wed Feb 4 09:02:45 2009 +0000

    virtio_net: Add a MAC filter table

Signed-off-by: Stephen Hemminger <stephen at networkplumber.org>

--- a/drivers/net/virtio_net.c	2013-12-09 16:12:03.897891975 -0800
+++ b/drivers/net/virtio_net.c	2013-12-09 16:12:36.353164803 -0800
@@ -893,7 +893,7 @@ static bool virtnet_send_command(struct
 	sg_init_one(&hdr, &ctrl, sizeof(ctrl));
 	sgs[out_num++] = &hdr;
 
-	if (out)
+	for (; out; out = sg_next(out))
 		sgs[out_num++] = out;
 	if (in)
 		sgs[out_num + in_num++] = in;

All the code passes NULL for the last sg list (in).
Simplify by just removing it.

Signed-off-by: Stephen Hemminger <stephen at networkplumber.org>


--- a/drivers/net/virtio_net.c	2013-12-09 16:12:36.353164803 -0800
+++ b/drivers/net/virtio_net.c	2013-12-09 16:12:41.409051865 -0800
@@ -876,13 +876,12 @@ static netdev_tx_t start_xmit(struct sk_
  * never fail unless improperly formated.
  */
 static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
-				 struct scatterlist *out,
-				 struct scatterlist *in)
+				 struct scatterlist *out)
 {
 	struct scatterlist *sgs[4], hdr, stat;
 	struct virtio_net_ctrl_hdr ctrl;
 	virtio_net_ctrl_ack status = ~0;
-	unsigned out_num = 0, in_num = 0, tmp;
+	unsigned out_num = 0, tmp;
 
 	/* Caller should know better */
 	BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ));
@@ -895,16 +894,13 @@ static bool virtnet_send_command(struct
 
 	for (; out; out = sg_next(out))
 		sgs[out_num++] = out;
-	if (in)
-		sgs[out_num + in_num++] = in;
 
 	/* Add return status. */
 	sg_init_one(&stat, &status, sizeof(status));
-	sgs[out_num + in_num++] = &stat;
+	sgs[out_num] = &stat;
 
-	BUG_ON(out_num + in_num > ARRAY_SIZE(sgs));
-	BUG_ON(virtqueue_add_sgs(vi->cvq, sgs, out_num, in_num, vi, GFP_ATOMIC)
-	       < 0);
+	BUG_ON(out_num + 1 > ARRAY_SIZE(sgs));
+	BUG_ON(virtqueue_add_sgs(vi->cvq, sgs, out_num, 1, vi, GFP_ATOMIC) < 0);
 
 	if (unlikely(!virtqueue_kick(vi->cvq)))
 		return status == VIRTIO_NET_OK;
@@ -934,8 +930,7 @@ static int virtnet_set_mac_address(struc
 	if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)) {
 		sg_init_one(&sg, addr->sa_data, dev->addr_len);
 		if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC,
-					  VIRTIO_NET_CTRL_MAC_ADDR_SET,
-					  &sg, NULL)) {
+					  VIRTIO_NET_CTRL_MAC_ADDR_SET, &sg)) {
 			dev_warn(&vdev->dev,
 				 "Failed to set mac address by vq command.\n");
 			return -EINVAL;
@@ -1008,7 +1003,7 @@ static void virtnet_ack_link_announce(st
 {
 	rtnl_lock();
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_ANNOUNCE,
-				  VIRTIO_NET_CTRL_ANNOUNCE_ACK, NULL, NULL))
+				  VIRTIO_NET_CTRL_ANNOUNCE_ACK, NULL))
 		dev_warn(&vi->dev->dev, "Failed to ack link
announce.\n");
 	rtnl_unlock();
 }
@@ -1026,7 +1021,7 @@ static int virtnet_set_queues(struct vir
 	sg_init_one(&sg, &s, sizeof(s));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MQ,
-				  VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET, &sg, NULL)) {
+				  VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET, &sg)) {
 		dev_warn(&dev->dev, "Fail to set num of queue pairs to
%d\n",
 			 queue_pairs);
 		return -EINVAL;
@@ -1076,16 +1071,14 @@ static void virtnet_set_rx_mode(struct n
 	sg_init_one(sg, &promisc, sizeof(promisc));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
-				  VIRTIO_NET_CTRL_RX_PROMISC,
-				  sg, NULL))
+				  VIRTIO_NET_CTRL_RX_PROMISC, sg))
 		dev_warn(&dev->dev, "Failed to %sable promisc mode.\n",
 			 promisc ? "en" : "dis");
 
 	sg_init_one(sg, &allmulti, sizeof(allmulti));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
-				  VIRTIO_NET_CTRL_RX_ALLMULTI,
-				  sg, NULL))
+				  VIRTIO_NET_CTRL_RX_ALLMULTI, sg))
 		dev_warn(&dev->dev, "Failed to %sable allmulti mode.\n",
 			 allmulti ? "en" : "dis");
 
@@ -1121,8 +1114,7 @@ static void virtnet_set_rx_mode(struct n
 		   sizeof(mac_data->entries) + (mc_count * ETH_ALEN));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC,
-				  VIRTIO_NET_CTRL_MAC_TABLE_SET,
-				  sg, NULL))
+				  VIRTIO_NET_CTRL_MAC_TABLE_SET, sg))
 		dev_warn(&dev->dev, "Failed to set MAC filter table.\n");
 
 	kfree(buf);
@@ -1137,7 +1129,7 @@ static int virtnet_vlan_rx_add_vid(struc
 	sg_init_one(&sg, &vid, sizeof(vid));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
-				  VIRTIO_NET_CTRL_VLAN_ADD, &sg, NULL))
+				  VIRTIO_NET_CTRL_VLAN_ADD, &sg))
 		dev_warn(&dev->dev, "Failed to add VLAN ID %d.\n", vid);
 	return 0;
 }
@@ -1151,7 +1143,7 @@ static int virtnet_vlan_rx_kill_vid(stru
 	sg_init_one(&sg, &vid, sizeof(vid));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
-				  VIRTIO_NET_CTRL_VLAN_DEL, &sg, NULL))
+				  VIRTIO_NET_CTRL_VLAN_DEL, &sg))
 		dev_warn(&dev->dev, "Failed to kill VLAN ID %d.\n", vid);
 	return 0;
 }

Signed-off-by: Stephen Hemminger <stephen at networkplumber.org>


--- a/drivers/net/virtio_net.c	2013-12-09 16:12:41.409051865 -0800
+++ b/drivers/net/virtio_net.c	2013-12-09 16:12:43.872996856 -0800
@@ -873,7 +873,7 @@ static netdev_tx_t start_xmit(struct sk_
 /*
  * Send command via the control virtqueue and check status.  Commands
  * supported by the hypervisor, as indicated by feature bits, should
- * never fail unless improperly formated.
+ * never fail unless improperly formatted.
  */
 static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
 				 struct scatterlist *out)
@@ -1061,7 +1061,7 @@ static void virtnet_set_rx_mode(struct n
 	void *buf;
 	int i;
 
-	/* We can't dynamicaly set ndo_set_rx_mode, so return gracefully */
+	/* We can't dynamically set ndo_set_rx_mode, so return gracefully */
 	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX))
 		return;

On Mon, Dec 09, 2013 at 04:16:32PM -0800, Stephen Hemminger
wrote:
> The virtio_net driver never sends the multicast address list to
> the host. This is because send command takes a pointer to scatter list
> to send but only inserts that one entry into the outgoing scatter list.
> 
> This bug has been there since:
> commit f565a7c259d71cc186753653d978c646d2354b36
> Author: Alex Williamson <alex.williamson at hp.com>
> Date:   Wed Feb 4 09:02:45 2009 +0000
> 
>     virtio_net: Add a MAC filter table
> 
> Signed-off-by: Stephen Hemminger <stephen at networkplumber.org>
Acked-by: Michael S. Tsirkin <mst at redhat.com>

Question is, can we detect a broken driver in the host somehow?
If not we'll need a feature bit for hosts to actually do
MC filtering.
> --- a/drivers/net/virtio_net.c	2013-12-09 16:12:03.897891975 -0800
> +++ b/drivers/net/virtio_net.c	2013-12-09 16:12:36.353164803 -0800
> @@ -893,7 +893,7 @@ static bool virtnet_send_command(struct
>  	sg_init_one(&hdr, &ctrl, sizeof(ctrl));
>  	sgs[out_num++] = &hdr;
>  
> -	if (out)
> +	for (; out; out = sg_next(out))
>  		sgs[out_num++] = out;
>  	if (in)
>  		sgs[out_num + in_num++] = in;

On Mon, Dec 09, 2013 at 04:17:40PM -0800, Stephen Hemminger
wrote:
> All the code passes NULL for the last sg list (in).
> Simplify by just removing it.
> 
> Signed-off-by: Stephen Hemminger <stephen at networkplumber.org>
When we need it we can re-add it.

Acked-by: Michael S. Tsirkin <mst at redhat.com>
> 
> --- a/drivers/net/virtio_net.c	2013-12-09 16:12:36.353164803 -0800
> +++ b/drivers/net/virtio_net.c	2013-12-09 16:12:41.409051865 -0800
> @@ -876,13 +876,12 @@ static netdev_tx_t start_xmit(struct sk_
>   * never fail unless improperly formated.
>   */
>  static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8
cmd,
> -				 struct scatterlist *out,
> -				 struct scatterlist *in)
> +				 struct scatterlist *out)
>  {
>  	struct scatterlist *sgs[4], hdr, stat;
>  	struct virtio_net_ctrl_hdr ctrl;
>  	virtio_net_ctrl_ack status = ~0;
> -	unsigned out_num = 0, in_num = 0, tmp;
> +	unsigned out_num = 0, tmp;
>  
>  	/* Caller should know better */
>  	BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ));
> @@ -895,16 +894,13 @@ static bool virtnet_send_command(struct
>  
>  	for (; out; out = sg_next(out))
>  		sgs[out_num++] = out;
> -	if (in)
> -		sgs[out_num + in_num++] = in;
>  
>  	/* Add return status. */
>  	sg_init_one(&stat, &status, sizeof(status));
> -	sgs[out_num + in_num++] = &stat;
> +	sgs[out_num] = &stat;
>  
> -	BUG_ON(out_num + in_num > ARRAY_SIZE(sgs));
> -	BUG_ON(virtqueue_add_sgs(vi->cvq, sgs, out_num, in_num, vi,
GFP_ATOMIC)
> -	       < 0);
> +	BUG_ON(out_num + 1 > ARRAY_SIZE(sgs));
> +	BUG_ON(virtqueue_add_sgs(vi->cvq, sgs, out_num, 1, vi, GFP_ATOMIC)
< 0);
>  
>  	if (unlikely(!virtqueue_kick(vi->cvq)))
>  		return status == VIRTIO_NET_OK;
> @@ -934,8 +930,7 @@ static int virtnet_set_mac_address(struc
>  	if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)) {
>  		sg_init_one(&sg, addr->sa_data, dev->addr_len);
>  		if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC,
> -					  VIRTIO_NET_CTRL_MAC_ADDR_SET,
> -					  &sg, NULL)) {
> +					  VIRTIO_NET_CTRL_MAC_ADDR_SET, &sg)) {
>  			dev_warn(&vdev->dev,
>  				 "Failed to set mac address by vq command.\n");
>  			return -EINVAL;
> @@ -1008,7 +1003,7 @@ static void virtnet_ack_link_announce(st
>  {
>  	rtnl_lock();
>  	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_ANNOUNCE,
> -				  VIRTIO_NET_CTRL_ANNOUNCE_ACK, NULL, NULL))
> +				  VIRTIO_NET_CTRL_ANNOUNCE_ACK, NULL))
>  		dev_warn(&vi->dev->dev, "Failed to ack link
announce.\n");
>  	rtnl_unlock();
>  }
> @@ -1026,7 +1021,7 @@ static int virtnet_set_queues(struct vir
>  	sg_init_one(&sg, &s, sizeof(s));
>  
>  	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MQ,
> -				  VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET, &sg, NULL)) {
> +				  VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET, &sg)) {
>  		dev_warn(&dev->dev, "Fail to set num of queue pairs to
%d\n",
>  			 queue_pairs);
>  		return -EINVAL;
> @@ -1076,16 +1071,14 @@ static void virtnet_set_rx_mode(struct n
>  	sg_init_one(sg, &promisc, sizeof(promisc));
>  
>  	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
> -				  VIRTIO_NET_CTRL_RX_PROMISC,
> -				  sg, NULL))
> +				  VIRTIO_NET_CTRL_RX_PROMISC, sg))
>  		dev_warn(&dev->dev, "Failed to %sable promisc mode.\n",
>  			 promisc ? "en" : "dis");
>  
>  	sg_init_one(sg, &allmulti, sizeof(allmulti));
>  
>  	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
> -				  VIRTIO_NET_CTRL_RX_ALLMULTI,
> -				  sg, NULL))
> +				  VIRTIO_NET_CTRL_RX_ALLMULTI, sg))
>  		dev_warn(&dev->dev, "Failed to %sable allmulti
mode.\n",
>  			 allmulti ? "en" : "dis");
>  
> @@ -1121,8 +1114,7 @@ static void virtnet_set_rx_mode(struct n
>  		   sizeof(mac_data->entries) + (mc_count * ETH_ALEN));
>  
>  	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC,
> -				  VIRTIO_NET_CTRL_MAC_TABLE_SET,
> -				  sg, NULL))
> +				  VIRTIO_NET_CTRL_MAC_TABLE_SET, sg))
>  		dev_warn(&dev->dev, "Failed to set MAC filter
table.\n");
>  
>  	kfree(buf);
> @@ -1137,7 +1129,7 @@ static int virtnet_vlan_rx_add_vid(struc
>  	sg_init_one(&sg, &vid, sizeof(vid));
>  
>  	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
> -				  VIRTIO_NET_CTRL_VLAN_ADD, &sg, NULL))
> +				  VIRTIO_NET_CTRL_VLAN_ADD, &sg))
>  		dev_warn(&dev->dev, "Failed to add VLAN ID %d.\n",
vid);
>  	return 0;
>  }
> @@ -1151,7 +1143,7 @@ static int virtnet_vlan_rx_kill_vid(stru
>  	sg_init_one(&sg, &vid, sizeof(vid));
>  
>  	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
> -				  VIRTIO_NET_CTRL_VLAN_DEL, &sg, NULL))
> +				  VIRTIO_NET_CTRL_VLAN_DEL, &sg))
>  		dev_warn(&dev->dev, "Failed to kill VLAN ID %d.\n",
vid);
>  	return 0;
>  }

On Mon, Dec 09, 2013 at 04:18:45PM -0800, Stephen Hemminger
wrote:
> 
> 
> Signed-off-by: Stephen Hemminger <stephen at networkplumber.org>
> 
Acked-by: Michael S. Tsirkin <mst at redhat.com>

> --- a/drivers/net/virtio_net.c	2013-12-09 16:12:41.409051865 -0800
> +++ b/drivers/net/virtio_net.c	2013-12-09 16:12:43.872996856 -0800
> @@ -873,7 +873,7 @@ static netdev_tx_t start_xmit(struct sk_
>  /*
>   * Send command via the control virtqueue and check status.  Commands
>   * supported by the hypervisor, as indicated by feature bits, should
> - * never fail unless improperly formated.
> + * never fail unless improperly formatted.
>   */
>  static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8
cmd,
>  				 struct scatterlist *out)
> @@ -1061,7 +1061,7 @@ static void virtnet_set_rx_mode(struct n
>  	void *buf;
>  	int i;
>  
> -	/* We can't dynamicaly set ndo_set_rx_mode, so return gracefully */
> +	/* We can't dynamically set ndo_set_rx_mode, so return gracefully */
>  	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX))
>  		return;
>

On 12/09/2013 07:16 PM, Stephen Hemminger wrote:
> The virtio_net driver never sends the multicast address list to
> the host. This is because send command takes a pointer to scatter list
> to send but only inserts that one entry into the outgoing scatter list.
> 
> This bug has been there since:
> commit f565a7c259d71cc186753653d978c646d2354b36
> Author: Alex Williamson <alex.williamson at hp.com>
> Date:   Wed Feb 4 09:02:45 2009 +0000
> 
>     virtio_net: Add a MAC filter table
> 
> Signed-off-by: Stephen Hemminger <stephen at networkplumber.org>
> 
> --- a/drivers/net/virtio_net.c	2013-12-09 16:12:03.897891975 -0800
> +++ b/drivers/net/virtio_net.c	2013-12-09 16:12:36.353164803 -0800
> @@ -893,7 +893,7 @@ static bool virtnet_send_command(struct
>  	sg_init_one(&hdr, &ctrl, sizeof(ctrl));
>  	sgs[out_num++] = &hdr;
>  
> -	if (out)
> +	for (; out; out = sg_next(out))
>  		sgs[out_num++] = out;
>  	if (in)
>  		sgs[out_num + in_num++] = in;
Hi Stephen

Amos Kong pointed out in your original thread, that this doesn't seem to
be needed and is working for him.  I just checked my VMs and it's
working for me as well without this.  Have you seen it fail?

Amos did a very good job explaining, but it basically boils down to the
fact that virtqueue_add_sgs() treats each entry of the sgs array
as a list and walks each list using exactly the same code as this patch
proposes :).  virtqueue_add() also walks each array element as a list.

-vlad

On Tue, Dec 10, 2013 at 05:24:29PM +0200, Michael S. Tsirkin
wrote:
> On Mon, Dec 09, 2013 at 04:16:32PM -0800, Stephen Hemminger wrote:
> > The virtio_net driver never sends the multicast address list to
> > the host. This is because send command takes a pointer to scatter list
> > to send but only inserts that one entry into the outgoing scatter
list.
> > 
> > This bug has been there since:
> > commit f565a7c259d71cc186753653d978c646d2354b36
> > Author: Alex Williamson <alex.williamson at hp.com>
> > Date:   Wed Feb 4 09:02:45 2009 +0000
> > 
> >     virtio_net: Add a MAC filter table
> > 
> > Signed-off-by: Stephen Hemminger <stephen at networkplumber.org>
> 
> Acked-by: Michael S. Tsirkin <mst at redhat.com>
> 
> Question is, can we detect a broken driver in the host somehow?
> If not we'll need a feature bit for hosts to actually do
> MC filtering.

Hmm Vlad and Amos made some good points about this patch.
Dave, pls don't apply yet until these are resolved.
> > --- a/drivers/net/virtio_net.c	2013-12-09 16:12:03.897891975 -0800
> > +++ b/drivers/net/virtio_net.c	2013-12-09 16:12:36.353164803 -0800
> > @@ -893,7 +893,7 @@ static bool virtnet_send_command(struct
> >  	sg_init_one(&hdr, &ctrl, sizeof(ctrl));
> >  	sgs[out_num++] = &hdr;
> >  
> > -	if (out)
> > +	for (; out; out = sg_next(out))
> >  		sgs[out_num++] = out;
> >  	if (in)
> >  		sgs[out_num + in_num++] = in;

From: Stephen Hemminger <stephen at networkplumber.org>
Date: Mon, 9 Dec 2013 16:17:40 -0800
> All the code passes NULL for the last sg list (in).
> Simplify by just removing it.
> 
> Signed-off-by: Stephen Hemminger <stephen at networkplumber.org>
Applied.

From: Stephen Hemminger <stephen at networkplumber.org>
Date: Mon, 9 Dec 2013 16:18:45 -0800
> Signed-off-by: Stephen Hemminger <stephen at networkplumber.org>
Applied.

From: Stephen Hemminger <stephen at networkplumber.org>
Date: Mon, 9 Dec 2013 16:16:32 -0800
> The virtio_net driver never sends the multicast address list to
> the host. This is because send command takes a pointer to scatter list
> to send but only inserts that one entry into the outgoing scatter list.
> 
> This bug has been there since:
> commit f565a7c259d71cc186753653d978c646d2354b36
> Author: Alex Williamson <alex.williamson at hp.com>
> Date:   Wed Feb 4 09:02:45 2009 +0000
> 
>     virtio_net: Add a MAC filter table
> 
> Signed-off-by: Stephen Hemminger <stephen at networkplumber.org>
This seems to still be under discussion so I'll hold off on this
one for now.

Thanks.

From: Stephen Hemminger <stephen at networkplumber.org>
Date: Mon, 9 Dec 2013 16:16:32 -0800
> The virtio_net driver never sends the multicast address list to
> the host. This is because send command takes a pointer to scatter list
> to send but only inserts that one entry into the outgoing scatter list.
> 
> This bug has been there since:
> commit f565a7c259d71cc186753653d978c646d2354b36
> Author: Alex Williamson <alex.williamson at hp.com>
> Date:   Wed Feb 4 09:02:45 2009 +0000
> 
>     virtio_net: Add a MAC filter table
> 
> Signed-off-by: Stephen Hemminger <stephen at networkplumber.org>
I'm just going to assume for now that you agree with the anaylsis of
others that the sg lists are processed properly as-is, and therefore
your patch isn't necessary.

Let me know if that is not the case.

Thanks.