Re: [ovs-dev] [PATCH] net: openvswitch: Check vport name

[Eelco Chaudron <echaudro@redhat.com>]

On 16 Apr 2024, at 9:57, Jun Gu wrote:

> Hi Chaudron, thanks for your reply. Considerthe following scenario:
>
> - set a net device alias name (such as *enpxx*) into ovs.
>
> - |OVS_VPORT_CMD_NEW -> ||dev_get_by_name can query the net device using *enpxx* name, but the dev->name that return is *ensxx* name. the |net_device|struct including: ``` |struct net_device {
>     char			name[IFNAMSIZ];
>     RH_KABI_REPLACE(struct hlist_node name_hlist,
>     		struct netdev_name_node *name_node)
> ...
> |``` name_hlist include enpxx and ensxx name and both of them point to the same net_device, the net_device's name is ensxx. So when using *enpxx* name to query net device, the ||net device's name that return is *ensxx* name.|
> Then, openvswitch.ko will save the net device that name is*ensxx*  to a hash table. So this will cause the below process:
> - ovs can'tobtain the enpxx net device by |OVS_VPORT_CMD_GET.|
> -|dpif_netlink_port_poll will get a |notification after |OVS_VPORT_CMD_NEW|, but the vport's name is ensxx. ovs will query the ensxx net device from interface table, but nothing. So ovs will run the port_del operation.
> - this will cause|OVS_VPORT_CMD_NEW|  and OVS_VPORT_CMD_DEL operation to run repeatedly.
> So the above issue can be avoided by the patch.

Thanks for the clarification, I forgot about the alias case. And you are right, OVS userspace does not support using interface aliases.

Maybe you can update the commit message, and add a code comment to clarify this in your next revision?

Cheers,

Eelco


> 在 4/15/24 18:04, Eelco Chaudron 写道:
>>
>> On 13 Apr 2024, at 10:48, jun.gu wrote:
>>
>>> Check vport name from dev_get_by_name, this can avoid to add and remove
>>> NIC repeatedly when NIC rename failed at system startup.
>>>
>>> Signed-off-by: Jun Gu<jun.gu@easystack.cn>
>>> ---
>>>   net/openvswitch/vport-netdev.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/net/openvswitch/vport-netdev.c b/net/openvswitch/vport-netdev.c
>>> index 903537a5da22..de8977d7f329 100644
>>> --- a/net/openvswitch/vport-netdev.c
>>> +++ b/net/openvswitch/vport-netdev.c
>>> @@ -78,7 +78,7 @@ struct vport *ovs_netdev_link(struct vport *vport, const char *name)
>>>   	int err;
>>>
>>>   	vport->dev = dev_get_by_name(ovs_dp_get_net(vport->dp), name);
>>> -	if (!vport->dev) {
>>> +	if (!vport->dev) || strcmp(name, ovs_vport_name(vport)) {
>> Hi Jun, not sure if I get the point here, as ovs_vport_name() translates into vport->dev->name.
>>
>> So are we trying to catch the interface rename between the dev_get_by_name(), and the code below? This rename could happen at any other place, so this check does not guarantee anything. Or am I missing something?
>>
>>>   		err = -ENODEV;
>>>   		goto error_free_vport;
>>>   	}
>>> -- 
>>> 2.25.1
>>>
>>> _______________________________________________
>>> dev mailing list
>>> dev@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>