2023-01-22 10:44:05

by Laurent Vivier

[permalink] [raw]
Subject: [PATCH 0/4] virtio_net: vdpa: update MAC address when it is generated by virtio-net

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




2023-01-22 10:44:18

by Laurent Vivier

[permalink] [raw]
Subject: [PATCH 2/4] virtio_net: add a timeout in virtnet_send_command()

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

Signed-off-by: Laurent Vivier <[email protected]>
---
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

2023-01-22 10:52:08

by Laurent Vivier

[permalink] [raw]
Subject: [PATCH 1/4] virtio_net: notify MAC address change on device initialization

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 <[email protected]>
---
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

2023-01-22 10:58:46

by Laurent Vivier

[permalink] [raw]
Subject: [PATCH 4/4] virtio_net: fix virtnet_send_command() with vdpa_sim_net

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 <[email protected]>
---
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

2023-01-22 11:08:57

by Michael S. Tsirkin

[permalink] [raw]
Subject: Re: [PATCH 0/4] virtio_net: vdpa: update MAC address when it is generated by virtio-net

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
>