* [PATCH] net: openvswitch: Check vport name @ 2024-04-13 8:48 jun.gu 2024-04-15 10:04 ` [ovs-dev] " Eelco Chaudron 2024-04-16 13:35 ` [PATCH] net: openvswitch: Check vport name Paolo Abeni 0 siblings, 2 replies; 11+ messages in thread From: jun.gu @ 2024-04-13 8:48 UTC (permalink / raw) To: pshelar; +Cc: netdev, dev, linux-kernel, jun.gu 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)) { err = -ENODEV; goto error_free_vport; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [ovs-dev] [PATCH] net: openvswitch: Check vport name 2024-04-13 8:48 [PATCH] net: openvswitch: Check vport name jun.gu @ 2024-04-15 10:04 ` Eelco Chaudron [not found] ` <671d3c3b-a5c4-4dbd-800b-fbfec0fbe4dc@easystack.cn> 2024-04-16 13:35 ` [PATCH] net: openvswitch: Check vport name Paolo Abeni 1 sibling, 1 reply; 11+ messages in thread From: Eelco Chaudron @ 2024-04-15 10:04 UTC (permalink / raw) To: jun.gu; +Cc: pshelar, dev, netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <671d3c3b-a5c4-4dbd-800b-fbfec0fbe4dc@easystack.cn>]
* Re: [ovs-dev] [PATCH] net: openvswitch: Check vport name [not found] ` <671d3c3b-a5c4-4dbd-800b-fbfec0fbe4dc@easystack.cn> @ 2024-04-16 8:04 ` Eelco Chaudron 2024-04-16 9:20 ` [PATCH v2] net: openvswitch: Check vport net device name jun.gu 0 siblings, 1 reply; 11+ messages in thread From: Eelco Chaudron @ 2024-04-16 8:04 UTC (permalink / raw) To: Jun Gu; +Cc: pshelar, dev, netdev, linux-kernel 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 >> ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] net: openvswitch: Check vport net device name 2024-04-16 8:04 ` Eelco Chaudron @ 2024-04-16 9:20 ` jun.gu 2024-04-16 11:19 ` Eelco Chaudron 0 siblings, 1 reply; 11+ messages in thread From: jun.gu @ 2024-04-16 9:20 UTC (permalink / raw) To: echaudro; +Cc: dev, jun.gu, linux-kernel, netdev, pshelar Check vport net device name to avoid the name that be used to query is inconsistent the retured name. Consider net device supports alias, the alias can be set to interface table in ovs userspace. Consider the following process: - set a net device alias to interface table. - ovs userspace run OVS_VPORT_CMD_NEW cmd to kernel, kernel will use net device alias to query net device by dev_get_by_name, but the net device name that return is inconsistent the name used to query. - the returned net device name is saved a hash table. - ovs userspace found that the name saved to interface table is inconsistent the name saved kernel hash table, it will run OVS_VPORT_CMD_DEL cmd to kernel and remove vport. ovs userspace will run OVS_VPORT_CMD_NEW and OVS_VPORT_CMD_DEL cmd repeatedly. So the patch will check vport net device name from dev_get_by_name to avoid the above issue. 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)) { err = -ENODEV; goto error_free_vport; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] net: openvswitch: Check vport net device name 2024-04-16 9:20 ` [PATCH v2] net: openvswitch: Check vport net device name jun.gu @ 2024-04-16 11:19 ` Eelco Chaudron 2024-04-17 4:20 ` [PATCH net-next v3] " jun.gu 0 siblings, 1 reply; 11+ messages in thread From: Eelco Chaudron @ 2024-04-16 11:19 UTC (permalink / raw) To: jun.gu; +Cc: dev, linux-kernel, netdev, pshelar On 16 Apr 2024, at 11:20, jun.gu wrote: > Check vport net device name to avoid the name that be used to query is > inconsistent the retured name. > > Consider net device supports alias, the alias can be set to interface > table in ovs userspace. Consider the following process: > - set a net device alias to interface table. > - ovs userspace run OVS_VPORT_CMD_NEW cmd to kernel, kernel will use net > device alias to query net device by dev_get_by_name, but the net device > name that return is inconsistent the name used to query. > - the returned net device name is saved a hash table. > - ovs userspace found that the name saved to interface table is > inconsistent the name saved kernel hash table, it will run > OVS_VPORT_CMD_DEL cmd to kernel and remove vport. > > ovs userspace will run OVS_VPORT_CMD_NEW and OVS_VPORT_CMD_DEL cmd > repeatedly. So the patch will check vport net device name from > dev_get_by_name to avoid the above issue. Maybe the commit message needs a rewrite to be more clear (shorter)? I’ll leave this up to the maintainers to judge. > 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); I was eluding to a comment here, something like: /* Ensure that the device exists and that the provided * name is not one of its aliases. */ > - if (!vport->dev) { > + if (!vport->dev) || strcmp(name, ovs_vport_name(vport)) { > err = -ENODEV; > goto error_free_vport; > } > -- > 2.25.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH net-next v3] net: openvswitch: Check vport net device name 2024-04-16 11:19 ` Eelco Chaudron @ 2024-04-17 4:20 ` jun.gu 2024-04-17 10:48 ` Eelco Chaudron 0 siblings, 1 reply; 11+ messages in thread From: jun.gu @ 2024-04-17 4:20 UTC (permalink / raw) To: echaudro; +Cc: dev, jun.gu, linux-kernel, netdev Check vport net device name to ensure the provided name is not one of its aliases. This can avoid that ovs userspace create and destroy vport repeatedly. Signed-off-by: Jun Gu <jun.gu@easystack.cn> --- net/openvswitch/vport-netdev.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/net/openvswitch/vport-netdev.c b/net/openvswitch/vport-netdev.c index 903537a5da22..7003e76b8172 100644 --- a/net/openvswitch/vport-netdev.c +++ b/net/openvswitch/vport-netdev.c @@ -78,7 +78,10 @@ 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) { + /* Ensure that the device exists and that the provided + * name is not one of its aliases. + */ + if ((!vport->dev) || strcmp(name, ovs_vport_name(vport))) { err = -ENODEV; goto error_free_vport; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net-next v3] net: openvswitch: Check vport net device name 2024-04-17 4:20 ` [PATCH net-next v3] " jun.gu @ 2024-04-17 10:48 ` Eelco Chaudron 2024-04-18 2:32 ` [PATCH net-next v4] net: openvswitch: Check vport netdev name jun.gu 0 siblings, 1 reply; 11+ messages in thread From: Eelco Chaudron @ 2024-04-17 10:48 UTC (permalink / raw) To: jun.gu; +Cc: dev, linux-kernel, netdev On 17 Apr 2024, at 6:20, jun.gu wrote: > Check vport net device name to ensure the provided name is not one of > its aliases. This can avoid that ovs userspace create and destroy vport > repeatedly. How about changing the text to the following? Ensure that the provided netdev name is not one of its aliases to prevent unnecessary creation and destruction of the vport by ovs-vswitchd. Maybe the maintainers are fine with the current text, in that case: Acked-by: Eelco Chaudron <echaudro@redhat.com> > > Signed-off-by: Jun Gu <jun.gu@easystack.cn> > --- > net/openvswitch/vport-netdev.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/net/openvswitch/vport-netdev.c b/net/openvswitch/vport-netdev.c > index 903537a5da22..7003e76b8172 100644 > --- a/net/openvswitch/vport-netdev.c > +++ b/net/openvswitch/vport-netdev.c > @@ -78,7 +78,10 @@ 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) { > + /* Ensure that the device exists and that the provided > + * name is not one of its aliases. > + */ > + if ((!vport->dev) || strcmp(name, ovs_vport_name(vport))) { > err = -ENODEV; > goto error_free_vport; > } > -- > 2.25.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH net-next v4] net: openvswitch: Check vport netdev name 2024-04-17 10:48 ` Eelco Chaudron @ 2024-04-18 2:32 ` jun.gu 2024-04-18 15:38 ` Jakub Kicinski [not found] ` <20240419043133.117295-1-jun.gu@easystack.cn> 0 siblings, 2 replies; 11+ messages in thread From: jun.gu @ 2024-04-18 2:32 UTC (permalink / raw) To: echaudro; +Cc: dev, jun.gu, linux-kernel, netdev Ensure that the provided netdev name is not one of its aliases to prevent unnecessary creation and destruction of the vport by ovs-vswitchd. Signed-off-by: Jun Gu <jun.gu@easystack.cn> Acked-by: Eelco Chaudron <echaudro@redhat.com> --- net/openvswitch/vport-netdev.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/net/openvswitch/vport-netdev.c b/net/openvswitch/vport-netdev.c index 903537a5da22..7003e76b8172 100644 --- a/net/openvswitch/vport-netdev.c +++ b/net/openvswitch/vport-netdev.c @@ -78,7 +78,10 @@ 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) { + /* Ensure that the device exists and that the provided + * name is not one of its aliases. + */ + if ((!vport->dev) || strcmp(name, ovs_vport_name(vport))) { err = -ENODEV; goto error_free_vport; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net-next v4] net: openvswitch: Check vport netdev name 2024-04-18 2:32 ` [PATCH net-next v4] net: openvswitch: Check vport netdev name jun.gu @ 2024-04-18 15:38 ` Jakub Kicinski [not found] ` <20240419043133.117295-1-jun.gu@easystack.cn> 1 sibling, 0 replies; 11+ messages in thread From: Jakub Kicinski @ 2024-04-18 15:38 UTC (permalink / raw) To: jun.gu; +Cc: echaudro, dev, linux-kernel, netdev On Thu, 18 Apr 2024 10:32:42 +0800 jun.gu wrote: > + if ((!vport->dev) || strcmp(name, ovs_vport_name(vport))) { Please drop the unnecessary brackets. When you repost, start a new thread, do not post new version in-reply-to. -- pw-bot: cr ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <20240419043133.117295-1-jun.gu@easystack.cn>]
* Re: [PATCH net-next v4] net: openvswitch: Check vport netdev name [not found] ` <20240419043133.117295-1-jun.gu@easystack.cn> @ 2024-04-20 3:01 ` Jakub Kicinski 0 siblings, 0 replies; 11+ messages in thread From: Jakub Kicinski @ 2024-04-20 3:01 UTC (permalink / raw) To: Jun Gu; +Cc: dev, echaudro, linux-kernel, netdev On Fri, 19 Apr 2024 12:31:33 +0800 Jun Gu wrote: > From: "jun.gu" <jun.gu@easystack.cn> > > Ensure that the provided netdev name is not one of its aliases to > prevent unnecessary creation and destruction of the vport by > ovs-vswitchd. > > Signed-off-by: jun.gu <jun.gu@easystack.cn> > Acked-by: Eelco Chaudron <echaudro@redhat.com> I said: When you repost, start a new thread, do not post new version in-reply-to. If you don't understand what something means - ask :| Now try again. -- pw-bot: cr ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net: openvswitch: Check vport name 2024-04-13 8:48 [PATCH] net: openvswitch: Check vport name jun.gu 2024-04-15 10:04 ` [ovs-dev] " Eelco Chaudron @ 2024-04-16 13:35 ` Paolo Abeni 1 sibling, 0 replies; 11+ messages in thread From: Paolo Abeni @ 2024-04-16 13:35 UTC (permalink / raw) To: jun.gu, pshelar; +Cc: netdev, dev, linux-kernel On Sat, 2024-04-13 at 16:48 +0800, 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)) { ^^ Missing open bracket here, and close bracket towards the end. Please build and test your patch before submitting the next revision. Please also include the target tree ('net-next') in the subj prefix when you will submit it. And update the commit message as per Eelco's request. Thanks, Paolo ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2024-04-20 3:01 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-13 8:48 [PATCH] net: openvswitch: Check vport name jun.gu
2024-04-15 10:04 ` [ovs-dev] " Eelco Chaudron
[not found] ` <671d3c3b-a5c4-4dbd-800b-fbfec0fbe4dc@easystack.cn>
2024-04-16 8:04 ` Eelco Chaudron
2024-04-16 9:20 ` [PATCH v2] net: openvswitch: Check vport net device name jun.gu
2024-04-16 11:19 ` Eelco Chaudron
2024-04-17 4:20 ` [PATCH net-next v3] " jun.gu
2024-04-17 10:48 ` Eelco Chaudron
2024-04-18 2:32 ` [PATCH net-next v4] net: openvswitch: Check vport netdev name jun.gu
2024-04-18 15:38 ` Jakub Kicinski
[not found] ` <20240419043133.117295-1-jun.gu@easystack.cn>
2024-04-20 3:01 ` Jakub Kicinski
2024-04-16 13:35 ` [PATCH] net: openvswitch: Check vport name Paolo Abeni
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).