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

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

[Laurent Vivier]

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

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

[Laurent Vivier]

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@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

Re: [PATCH 1/4] virtio_net: notify MAC address change on device initialization

[Eli Cohen]

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 <lvivier@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 &&
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:

Re: [PATCH 1/4] virtio_net: notify MAC address change on device initialization

[Laurent Vivier]

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 <lvivier@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 &&
> 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

Re: [PATCH 1/4] virtio_net: notify MAC address change on device initialization

[Eli Cohen]

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 <lvivier@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 &&
>> 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
>

Re: [PATCH 1/4] virtio_net: notify MAC address change on device initialization

[Jakub Kicinski]

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

Re: [PATCH 1/4] virtio_net: notify MAC address change on device initialization

[Laurent Vivier]

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/20230123120022.2364889-2-lvivier@redhat.com/T/#me9211516e12771001e0346818255c9fb48a2bf4a

Thanks,
Laurent

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

[Laurent Vivier]

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

Signed-off-by: Laurent Vivier <lvivier@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

[PATCH 3/4] vdpa_sim_net: don't always set VIRTIO_NET_F_MAC

[Laurent Vivier]

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@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

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

[Laurent Vivier]

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@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

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

[Michael S. Tsirkin]

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
> 

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

[Laurent Vivier]

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