linux-kernel - Re: [PATCH] vdpa/mlx5: Use random MAC address when no nic vport MAC set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <cfece74e-a979-4f74-8a6a-fc8869e354f7@nvidia.com>
Date: Fri, 30 Aug 2024 15:52:54 +0200
From: Dragos Tatulea <dtatulea@...dia.com>
To: Cindy Lu <lulu@...hat.com>
Cc: Jason Wang <jasowang@...hat.com>, "Michael S. Tsirkin" <mst@...hat.com>,
 Xuan Zhuo <xuanzhuo@...ux.alibaba.com>, Eugenio Pérez
 <eperezma@...hat.com>, si-wei.liu@...cle.com, Jiri Pirko <jiri@...dia.com>,
 virtualization@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] vdpa/mlx5: Use random MAC address when no nic vport MAC
 set



On 30.08.24 11:12, Cindy Lu wrote:
> On Thu, 29 Aug 2024 at 18:00, Dragos Tatulea <dtatulea@...dia.com> wrote:
>>
>>
>>
>> On 29.08.24 11:05, Cindy Lu wrote:
>>> On Wed, 28 Aug 2024 at 17:37, Dragos Tatulea <dtatulea@...dia.com> wrote:
>>>>
>>>>
>>>>
>>>> On 28.08.24 11:00, Cindy Lu wrote:
>>>>> On Wed, 28 Aug 2024 at 09:51, Jason Wang <jasowang@...hat.com> wrote:
>>>>>>
>>>>>> On Wed, Aug 28, 2024 at 12:03 AM Dragos Tatulea <dtatulea@...dia.com> wrote:
>>>>>>>
>>>>>>> When the vdpa device is configured without a specific MAC
>>>>>>> address, the vport MAC address is used. However, this
>>>>>>> address can be 0 which prevents the driver from properly
>>>>>>> configuring the MPFS and breaks steering.
>>>>>>>
>>>>>>> The solution is to simply generate a random MAC address
>>>>>>> when no MAC is set on the nic vport.
>>>>>>>
>>>>>>> Now it's possible to create a vdpa device without a
>>>>>>> MAC address and run qemu with this device without needing
>>>>>>> to configure an explicit MAC address.
>>>>>>>
>>>>>>> Signed-off-by: Dragos Tatulea <dtatulea@...dia.com>
>>>>>>> Reviewed-by: Jiri Pirko <jiri@...dia.com>
>>>>>>
>>>>>> Acked-by: Jason Wang <jasowang@...hat.com>
>>>>>>
>>>>>> (Adding Cindy for double checking if it has any side effect on Qemu side)
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>> But Now there is a bug in QEMU: if the hardware MAC address does not
>>>>> match the one in the QEMU command line, it will cause traffic loss.
>>>>>
>>>> Why is this a new issue in qemu? qemu in it's current state won't work
>>>> with a different mac address that the one that is set in HW anyway.
>>>>
>>> this is not a new bug. We are trying to fix it because it will cause
>>> traffic lose without any warning.
>>> in my fix , this setting (different mac in device and Qemu) will fail
>>> to load the VM.
>> Which is a good thing, right? Some feedback to the user that there is
>> a misconfig. I got bitten by this so many times... Thank you for adding it.
>>
>>>
>>>>> So, Just an FYI here: if your patch merged, it may cause traffic loss.
>>>>> and now I'm working in the fix it in qemu, the link is
>>>>> https://patchew.org/QEMU/20240716011349.821777-1-lulu@redhat.com/
>>>>> The idea of this fix is
>>>>> There are will only two acceptable situations for qemu:
>>>>> 1. The hardware MAC address is the same as the MAC address specified
>>>>> in the QEMU command line, and both MAC addresses are not 0.
>>>>> 2. The hardware MAC address is not 0, and the MAC address in the QEMU
>>>>> command line is 0. In this situation, the hardware MAC address will
>>>>> overwrite the QEMU command line address.
>>>>>
>>>> Why would this not work with this patch? This patch simply sets a MAC
>>>> if the vport doesn't have one set. Which allows for more scenarios to
>>>> work.
>>>>
>>> I do not mean your patch will not work, I just want to make some
>>> clarify here.Your patch + my fix may cause the VM to fail to load in
>>> some situations, and this is as expected.
>>> Your patch is good to merge.
>> Ack. Thank you for the clarification.
>>
>> Thanks,
>> Dragos
>>
> Hi Dragos，
>  I think we need to hold this patch. Because it may not be working
> with upstream qemu.
> 
> MLX will create a random MAC address for your patch. Additionally, if
> there is no specific MAC in the QEMU command line, QEMU will also
> generate a random MAC.
> these two MAC are not the same. and this will cause traffic loss.
Ahaa, it turns out that qemu 8.x and 9.x have different behaviour.

Initially I was testing this scenario (vdpa device created with no mac
and no mac set in qemu cli) with qemu 8.x. There, qemu was not being
able to set the qemu generated random mac addres because .set_config()
is a nop in mlx5_vdpa.

Then I moved to qemu 9.x and saw that this scenario was working because
now the CVQ was used instead to configure the mac on the device.

So this patch should definitely not be applied.

I was thinking if there are ways to fix this for 8.x. The only feasible
way is to implement .set_config() in mlx5_vdpa for the mac
configuration. But as you previousy said, this is discouraged.

Thanks,
Dragos