Received-SPF: pass (google.com: domain of linux-kernel+bounces-146459-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: Eelco Chaudron <echaudro@redhat.com>
To: Jun Gu <jun.gu@easystack.cn>
Cc: pshelar@ovn.org, dev@openvswitch.org, netdev@vger.kernel.org,
 linux-kernel@vger.kernel.org
Subject: Re: [ovs-dev] [PATCH] net: openvswitch: Check vport name
Date: Tue, 16 Apr 2024 10:04:29 +0200
Message-ID: <084E7217-6290-46D2-A47A-14ACB60EBBCA@redhat.com>
In-Reply-To: <671d3c3b-a5c4-4dbd-800b-fbfec0fbe4dc@easystack.cn>
References: <20240413084826.52417-1-jun.gu@easystack.cn>
 <9D534A61-4CC2-4374-B2D8-761216745EDD@redhat.com>
 <671d3c3b-a5c4-4dbd-800b-fbfec0fbe4dc@easystack.cn>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable


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 usin=
g *enpxx* name, but the dev->name that return is *ensxx* name. the |net_d=
evice|struct including: ``` |struct=C2=A0net_device=C2=A0{
>     char			name[IFNAMSIZ];
>     RH_KABI_REPLACE(struct=C2=A0hlist_node=C2=A0name_hlist,
>     		struct=C2=A0netdev_name_node=C2=A0*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=
=2E
> - this will cause|OVS_VPORT_CMD_NEW|  and OVS_VPORT_CMD_DEL operation t=
o 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 clarif=
y this in your next revision?

Cheers,

Eelco


> =E5=9C=A8 4/15/24 18:04, Eelco Chaudron =E5=86=99=E9=81=93:
>>
>> On 13 Apr 2024, at 10:48, jun.gu wrote:
>>
>>> Check vport name from dev_get_by_name, this can avoid to add and remo=
ve
>>> 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-n=
etdev.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 =3D 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() translat=
es 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 =3D -ENODEV;
>>>   		goto error_free_vport;
>>>   	}
>>> -- =

>>> 2.25.1
>>>
>>> _______________________________________________
>>> dev mailing list
>>> dev@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>