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
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
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
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
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
>
On 22/01/2023 12:05, Laurent Vivier wrote:
> 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 &&
Maybe it's better to not count on addr_assign_type and use a local
variable to indicate that virtnet_probe assigned random MAC. The reason
is that the hardware driver might have done that as well and does not
need notification.
> + 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:
On 1/22/23 11:23, Michael S. Tsirkin wrote:
> 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.
We needed to fix virtio_net before fixing vdpa_sim_net otherwise the
virtnet_send_command() hangs when we define the vdpa device with "vdpa dev" but without a
MAC address.
> 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.
I was not aware of the Jason's series.
It seems to address better the problem, except it triggers the ASSERT_RTNL() in
virtnet_send_command() when it is called from virtnet_probe().
I will remove patches 2 and 4 from my series.
PATCH 3 can be sent on independently too.
Thanks,
Laurent
On 1/22/23 14:47, Eli Cohen wrote:
>
> On 22/01/2023 12:05, Laurent Vivier wrote:
>> 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 &&
> Maybe it's better to not count on addr_assign_type and use a local variable to indicate
> that virtnet_probe assigned random MAC. The reason is that the hardware driver might have
> done that as well and does not need notification.
eth_hw_addr_random() sets explicitly NET_ADDR_RANDOM, while eth_hw_addr_set() doesn't
change addr_assign_type so it doesn't seem this value is set by the hardware driver.
So I guess it's the default value (NET_ADDR_PERM) in this case (even if it's a random
address from the point of view of the hardware).
If you prefer I can replace it by "!virtio_has_feature(vdev, VIRTIO_NET_F_MAC)"?
Thanks,
Laurent
On 23/01/2023 11:52, Laurent Vivier wrote:
> On 1/22/23 14:47, Eli Cohen wrote:
>>
>> On 22/01/2023 12:05, Laurent Vivier wrote:
>>> 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 &&
>> Maybe it's better to not count on addr_assign_type and use a local
>> variable to indicate that virtnet_probe assigned random MAC. The
>> reason is that the hardware driver might have done that as well and
>> does not need notification.
>
> eth_hw_addr_random() sets explicitly NET_ADDR_RANDOM, while
> eth_hw_addr_set() doesn't change addr_assign_type so it doesn't seem
> this value is set by the hardware driver.
>
> So I guess it's the default value (NET_ADDR_PERM) in this case (even
> if it's a random address from the point of view of the hardware).
>
> If you prefer I can replace it by "!virtio_has_feature(vdev,
> VIRTIO_NET_F_MAC)"?
>
My point is this. What if the hardware driver decides to choose a random
address by calling eth_hw_addr_random(). In this case
dev->addr_assign_type = NET_ADDR_RANDOM is executed unconditionally. But
now you will needlessly execute the control command because the hardware
address already knows about this address.
> Thanks,
> Laurent
>
On Sun, 22 Jan 2023 15:47:08 +0200 Eli Cohen wrote:
> > @@ -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 &&
> Maybe it's better to not count on addr_assign_type and use a local
> variable to indicate that virtnet_probe assigned random MAC.
+1, FWIW
On 1/24/23 04:31, Jakub Kicinski wrote:
> On Sun, 22 Jan 2023 15:47:08 +0200 Eli Cohen wrote:
>>> @@ -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 &&
>> Maybe it's better to not count on addr_assign_type and use a local
>> variable to indicate that virtnet_probe assigned random MAC.
>
> +1, FWIW
>
v2 sent, but I rely on virtio_has_feature(vdev, VIRTIO_NET_F_MAC) to know if the MAC
address is provided by the device or not:
https://lore.kernel.org/lkml/[email protected]/T/#me9211516e12771001e0346818255c9fb48a2bf4a
Thanks,
Laurent