Virtualization - Jan 2023 - [PATCH 2/4] virtio_net: add a timeout in virtnet_send_command()

When the MAC address is not provided by the vdpa device virtio_net
driver assigns a random one without notifying the device.
The consequence, in the case of mlx5_vdpa, is the internal routing
tables of the device are not updated and this can block the
communication between two namespaces.

To fix this problem, use virtnet_send_command(VIRTIO_NET_CTRL_MAC)
to set the address from virtnet_probe() when the MAC address is
randomly assigned from virtio_net.

While I was testing this change I found 3 other bugs in vdpa_sim_net:

- vdpa_sim_net sets the VIRTIO_NET_F_MAC even if no MAC address is
  provided. So virtio_net doesn't generate a random MAC address and
  the MAC address appears to be 00:00:00:00:00:00

- vdpa_sim_net never processes the command and virtnet_send_command()
  hangs in an infinite loop. To avoid a kernel crash add a timeout
  in the loop.

- To allow vdpa_sim_net to process the command, replace the cpu_relax()
  in the loop by a schedule(). vdpa_sim_net uses a workqueue to process
  the queue, and if we don't allow the kernel to schedule, the queue
  is not processed and the loop is infinite.

Laurent Vivier (4):
  virtio_net: notify MAC address change on device initialization
  virtio_net: add a timeout in virtnet_send_command()
  vdpa_sim_net: don't always set VIRTIO_NET_F_MAC
  virtio_net: fix virtnet_send_command() with vdpa_sim_net

 drivers/net/virtio_net.c             | 21 +++++++++++++++++++--
 drivers/vdpa/vdpa_sim/vdpa_sim_net.c |  6 ++++++
 2 files changed, 25 insertions(+), 2 deletions(-)

-- 
2.39.0

In virtnet_probe(), if the device doesn't provide a MAC address the
driver assigns a random one.
As we modify the MAC address we need to notify the device to allow it
to update all the related information.

The problem can be seen with vDPA and mlx5_vdpa driver as it doesn't
assign a MAC address by default. The virtio_net device uses a random
MAC address (we can see it with "ip link"), but we can't ping a
net
namespace from another one using the virtio-vdpa device because the
new MAC address has not been provided to the hardware.

Signed-off-by: Laurent Vivier <lvivier at redhat.com>
---
 drivers/net/virtio_net.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 7723b2a49d8e..25511a86590e 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -3800,6 +3800,8 @@ static int virtnet_probe(struct virtio_device *vdev)
 		eth_hw_addr_set(dev, addr);
 	} else {
 		eth_hw_addr_random(dev);
+		dev_info(&vdev->dev, "Assigned random MAC address %pM\n",
+			 dev->dev_addr);
 	}
 
 	/* Set up our device-specific information */
@@ -3956,6 +3958,18 @@ static int virtnet_probe(struct virtio_device *vdev)
 	pr_debug("virtnet: registered device %s with %d RX and TX
vq's\n",
 		 dev->name, max_queue_pairs);
 
+	/* a random MAC address has been assigned, notify the device */
+	if (dev->addr_assign_type == NET_ADDR_RANDOM &&
+	    virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)) {
+		struct scatterlist sg;
+
+		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)) {
+			dev_warn(&vdev->dev, "Failed to update MAC address.\n");
+		}
+	}
+
 	return 0;
 
 free_unregister_netdev:
-- 
2.39.0

if the device control queue is buggy, don't crash the kernel by
waiting for ever the response.

Signed-off-by: Laurent Vivier <lvivier at redhat.com>
---
 drivers/net/virtio_net.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 25511a86590e..29b3cc72082d 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1974,6 +1974,7 @@ static bool virtnet_send_command(struct virtnet_info *vi,
u8 class, u8 cmd,
 	struct scatterlist *sgs[4], hdr, stat;
 	unsigned out_num = 0, tmp;
 	int ret;
+	unsigned long timeout;
 
 	/* Caller should know better */
 	BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ));
@@ -2006,8 +2007,10 @@ static bool virtnet_send_command(struct virtnet_info *vi,
u8 class, u8 cmd,
 	/* Spin for a response, the kick causes an ioport write, trapping
 	 * into the hypervisor, so the request should be handled immediately.
 	 */
+	timeout = jiffies + 20 * HZ;
 	while (!virtqueue_get_buf(vi->cvq, &tmp) &&
-	       !virtqueue_is_broken(vi->cvq))
+	       !virtqueue_is_broken(vi->cvq) &&
+	       !time_after(jiffies, timeout))
 		cpu_relax();
 
 	return vi->ctrl->status == VIRTIO_NET_OK;
-- 
2.39.0

if vdpa dev command doesn't set a MAC address, don't report
VIRTIO_NET_F_MAC.

As vdpa_sim_net sets VIRTIO_NET_F_MAC without setting the MAC address,
virtio-net doesn't set a random one and the address appears to be the
zero MAC address.

Signed-off-by: Laurent Vivier <lvivier at redhat.com>
---
 drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
index 584b975a98a7..28e858659b85 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
@@ -257,6 +257,12 @@ static int vdpasim_net_dev_add(struct vdpa_mgmt_dev *mdev,
const char *name,
 	dev_attr.work_fn = vdpasim_net_work;
 	dev_attr.buffer_size = PAGE_SIZE;
 
+	/* if vdpa dev doesn't provide a MAC address,
+	 * don't report we have one
+	 */
+	if (!(config->mask & (1 << VDPA_ATTR_DEV_NET_CFG_MACADDR)))
+		dev_attr.supported_features &= ~(1ULL << VIRTIO_NET_F_MAC);
+
 	simdev = vdpasim_create(&dev_attr, config);
 	if (IS_ERR(simdev))
 		return PTR_ERR(simdev);
-- 
2.39.0

virtnet_send_command() sends a command to the control virtqueue
by adding the command to the virtqueue, kicking the queue and waiting
in a loop.

The vdpa simulator simulates the control virqueue using a work queue:
the virqueue_kick() calls schedule_work() to start the queue processing.
But as virtnet_send_command() uses a loop, the scheduler cannot schedule
the workqueue and the virtqueue is never processed (and the command
never executed).

To fix that, replace in the loop the cpu_relax() by a schedule().

Signed-off-by: Laurent Vivier <lvivier at redhat.com>
---
 drivers/net/virtio_net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 29b3cc72082d..546c0b2baaca 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2011,7 +2011,7 @@ static bool virtnet_send_command(struct virtnet_info *vi,
u8 class, u8 cmd,
 	while (!virtqueue_get_buf(vi->cvq, &tmp) &&
 	       !virtqueue_is_broken(vi->cvq) &&
 	       !time_after(jiffies, timeout))
-		cpu_relax();
+		schedule();
 
 	return vi->ctrl->status == VIRTIO_NET_OK;
 }
-- 
2.39.0

On Sun, Jan 22, 2023 at 11:05:22AM +0100, Laurent Vivier
wrote:
> When the MAC address is not provided by the vdpa device virtio_net
> driver assigns a random one without notifying the device.
> The consequence, in the case of mlx5_vdpa, is the internal routing
> tables of the device are not updated and this can block the
> communication between two namespaces.
> 
> To fix this problem, use virtnet_send_command(VIRTIO_NET_CTRL_MAC)
> to set the address from virtnet_probe() when the MAC address is
> randomly assigned from virtio_net.
> 
> While I was testing this change I found 3 other bugs in vdpa_sim_net:
> 
> - vdpa_sim_net sets the VIRTIO_NET_F_MAC even if no MAC address is
>   provided. So virtio_net doesn't generate a random MAC address and
>   the MAC address appears to be 00:00:00:00:00:00
> 
> - vdpa_sim_net never processes the command and virtnet_send_command()
>   hangs in an infinite loop. To avoid a kernel crash add a timeout
>   in the loop.
> 
> - To allow vdpa_sim_net to process the command, replace the cpu_relax()
>   in the loop by a schedule(). vdpa_sim_net uses a workqueue to process
>   the queue, and if we don't allow the kernel to schedule, the queue
>   is not processed and the loop is infinite.
I'd split these things out as opposed to a series unless there's
a dependency I missed.

All this reminds me of
https://lore.kernel.org/r/20221226074908.8154-5-jasowang%40redhat.com

how is this patch different/better?
Pls also CC people involved in that original discussion.

Thanks!
> Laurent Vivier (4):
>   virtio_net: notify MAC address change on device initialization
>   virtio_net: add a timeout in virtnet_send_command()
>   vdpa_sim_net: don't always set VIRTIO_NET_F_MAC
>   virtio_net: fix virtnet_send_command() with vdpa_sim_net
> 
>  drivers/net/virtio_net.c             | 21 +++++++++++++++++++--
>  drivers/vdpa/vdpa_sim/vdpa_sim_net.c |  6 ++++++
>  2 files changed, 25 insertions(+), 2 deletions(-)
> 
> -- 
> 2.39.0
>