diff mbox series

[ovs-dev] 回复: [PATCH] bridge: Fix incorrect configuration of netdev's dpif type.

Message ID MEYP282MB3302F88B0EDFD7B5E1C131B1CD789@MEYP282MB3302.AUSP282.PROD.OUTLOOK.COM
State Not Applicable
Headers show
Series [ovs-dev] 回复: [PATCH] bridge: Fix incorrect configuration of netdev's dpif type. | expand

Checks

Context Check Description
ovsrobot/apply-robot fail apply and check: fail

Commit Message

miter Dec. 17, 2021, 6:44 a.m. UTC 
Hi Ilya,

I am sorry for what I have done.

This patch looks good to me.

Comments

Ilya Maximets Dec. 17, 2021, 9:09 p.m. UTC | #1 
On 12/17/21 07:44, lin huang wrote:
> Hi Ilya,
> 
> I am sorry for what I have done.

No worries.  It happens. :)

> 
> This patch looks good to me.

Thanks!

> 
> ________________________________________
> 发件人: dev <ovs-dev-bounces@openvswitch.org> 代表 Ilya Maximets <i.maximets@ovn.org>
> 发送时间: 2021年12月17日 8:05
> 收件人: ovs-dev@openvswitch.org
> 抄送: Lin Huang; Ilya Maximets
> 主题: [ovs-dev] [PATCH] bridge: Fix incorrect configuration of netdev's   dpif type.
> 
> netdev_set_dpif_type() can only be used with a normalized dpif type
> as an argument, which is a constant static string derived from a type
> of a dpif_class or a constant string "system".  Usage of a same
> constant string allows netdev-offload module to compare types by
> simply comparing pointers.
> 
> OTOH, 'br->ofproto->type' is a dynamic string that:
> a. Can be NULL.
> b. Even if not NULL and equal, can be a different dynamically
>    allocated string.
> 
> Both these qualities breaks assumptions made by all other modules
> related to HW offload, breaking the functionality.
> 
> Fix that by moving netdev_set_dpif_type() to dpif.c and calling with
> a correct constant string as an argument.
> 
> The call moved from bridge.c to dpif.c, because we need to have access
> to the dpif class, but bridge.c should not.
> 
> Not trying to set the dpif_type inside the netdev_ports_insert(),
> because it's used now outside the offloading context.  So, it's
> cleaner to move the netdev_set_dpif_type() call outside of the
> netdev-offload module.
> 
> Additionally removed the redundant call from the netdev_ports_insert()
> and refactored the function, since it doesn't need an extra argument
> anymore.
> 
> Fixes: 4f19a78a61c5 ("netdev-vport: Fix userspace tunnel ioctl(SIOCGIFINDEX) info logs.")
> Reported-by: Roi Dayan <roid@nvidia.com>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2021-December/390117.html
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---
>  lib/dpif.c           | 7 +++++--
>  lib/netdev-offload.c | 8 ++++----
>  lib/netdev-offload.h | 3 +--
>  vswitchd/bridge.c    | 2 --
>  4 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/dpif.c b/lib/dpif.c
> index 38bcb47cb..cff0bc2db 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -364,7 +364,8 @@ do_open(const char *name, const char *type, bool create, struct dpif **dpifp)
>              err = netdev_open(dpif_port.name, dpif_port.type, &netdev);
> 
>              if (!err) {
> -                netdev_ports_insert(netdev, dpif_type_str, &dpif_port);
> +                netdev_set_dpif_type(netdev, dpif_type_str);
> +                netdev_ports_insert(netdev, &dpif_port);
>                  netdev_close(netdev);
>              } else {
>                  VLOG_WARN("could not open netdev %s type %s: %s",
> @@ -602,10 +603,12 @@ dpif_port_add(struct dpif *dpif, struct netdev *netdev, odp_port_t *port_nop)
>              const char *dpif_type_str = dpif_normalize_type(dpif_type(dpif));
>              struct dpif_port dpif_port;
> 
> +            netdev_set_dpif_type(netdev, dpif_type_str);
> +
>              dpif_port.type = CONST_CAST(char *, netdev_get_type(netdev));
>              dpif_port.name = CONST_CAST(char *, netdev_name);
>              dpif_port.port_no = port_no;
> -            netdev_ports_insert(netdev, dpif_type_str, &dpif_port);
> +            netdev_ports_insert(netdev, &dpif_port);
>          }
>      } else {
>          VLOG_WARN_RL(&error_rl, "%s: failed to add %s as port: %s",
> diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c
> index 8075cfbd8..b8dc108f3 100644
> --- a/lib/netdev-offload.c
> +++ b/lib/netdev-offload.c
> @@ -573,12 +573,14 @@ netdev_ports_lookup(odp_port_t port_no, const char *dpif_type)
>  }
> 
>  int
> -netdev_ports_insert(struct netdev *netdev, const char *dpif_type,
> -                    struct dpif_port *dpif_port)
> +netdev_ports_insert(struct netdev *netdev, struct dpif_port *dpif_port)
>  {
> +    const char *dpif_type = netdev_get_dpif_type(netdev);
>      struct port_to_netdev_data *data;
>      int ifindex = netdev_get_ifindex(netdev);
> 
> +    ovs_assert(dpif_type);
> +
>      ovs_rwlock_wrlock(&netdev_hmap_rwlock);
>      if (netdev_ports_lookup(dpif_port->port_no, dpif_type)) {
>          ovs_rwlock_unlock(&netdev_hmap_rwlock);
> @@ -596,8 +598,6 @@ netdev_ports_insert(struct netdev *netdev, const char *dpif_type,
>          data->ifindex = -1;
>      }
> 
> -    netdev_set_dpif_type(netdev, dpif_type);
> -
>      hmap_insert(&port_to_netdev, &data->portno_node,
>                  netdev_ports_hash(dpif_port->port_no, dpif_type));
>      ovs_rwlock_unlock(&netdev_hmap_rwlock);
> diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h
> index e7fcedae9..43a98d499 100644
> --- a/lib/netdev-offload.h
> +++ b/lib/netdev-offload.h
> @@ -108,8 +108,7 @@ bool netdev_is_offload_rebalance_policy_enabled(void);
>  int netdev_flow_get_n_flows(struct netdev *netdev, uint64_t *n_flows);
> 
>  struct dpif_port;
> -int netdev_ports_insert(struct netdev *, const char *dpif_type,
> -                        struct dpif_port *);
> +int netdev_ports_insert(struct netdev *, struct dpif_port *);
>  struct netdev *netdev_ports_get(odp_port_t port, const char *dpif_type);
>  int netdev_ports_remove(odp_port_t port, const char *dpif_type);
>  odp_port_t netdev_ifindex_to_odp_port(int ifindex);
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 513ef7ea9..5223aa897 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -2052,8 +2052,6 @@ iface_do_create(const struct bridge *br,
>          goto error;
>      }
> 
> -    netdev_set_dpif_type(netdev, br->ofproto->type);
> -
>      error = iface_set_netdev_config(iface_cfg, netdev, errp);
>      if (error) {
>          goto error;
> --
> 2.31.1
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
diff mbox series

Patch

diff --git a/lib/dpif.c b/lib/dpif.c
index 38bcb47cb..cff0bc2db 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -364,7 +364,8 @@  do_open(const char *name, const char *type, bool create, struct dpif **dpifp)
             err = netdev_open(dpif_port.name, dpif_port.type, &netdev);

             if (!err) {
-                netdev_ports_insert(netdev, dpif_type_str, &dpif_port);
+                netdev_set_dpif_type(netdev, dpif_type_str);
+                netdev_ports_insert(netdev, &dpif_port);
                 netdev_close(netdev);
             } else {
                 VLOG_WARN("could not open netdev %s type %s: %s",
@@ -602,10 +603,12 @@  dpif_port_add(struct dpif *dpif, struct netdev *netdev, odp_port_t *port_nop)
             const char *dpif_type_str = dpif_normalize_type(dpif_type(dpif));
             struct dpif_port dpif_port;

+            netdev_set_dpif_type(netdev, dpif_type_str);
+
             dpif_port.type = CONST_CAST(char *, netdev_get_type(netdev));
             dpif_port.name = CONST_CAST(char *, netdev_name);
             dpif_port.port_no = port_no;
-            netdev_ports_insert(netdev, dpif_type_str, &dpif_port);
+            netdev_ports_insert(netdev, &dpif_port);
         }
     } else {
         VLOG_WARN_RL(&error_rl, "%s: failed to add %s as port: %s",
diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c
index 8075cfbd8..b8dc108f3 100644
--- a/lib/netdev-offload.c
+++ b/lib/netdev-offload.c
@@ -573,12 +573,14 @@  netdev_ports_lookup(odp_port_t port_no, const char *dpif_type)
 }

 int
-netdev_ports_insert(struct netdev *netdev, const char *dpif_type,
-                    struct dpif_port *dpif_port)
+netdev_ports_insert(struct netdev *netdev, struct dpif_port *dpif_port)
 {
+    const char *dpif_type = netdev_get_dpif_type(netdev);
     struct port_to_netdev_data *data;
     int ifindex = netdev_get_ifindex(netdev);

+    ovs_assert(dpif_type);
+
     ovs_rwlock_wrlock(&netdev_hmap_rwlock);
     if (netdev_ports_lookup(dpif_port->port_no, dpif_type)) {
         ovs_rwlock_unlock(&netdev_hmap_rwlock);
@@ -596,8 +598,6 @@  netdev_ports_insert(struct netdev *netdev, const char *dpif_type,
         data->ifindex = -1;
     }

-    netdev_set_dpif_type(netdev, dpif_type);
-
     hmap_insert(&port_to_netdev, &data->portno_node,
                 netdev_ports_hash(dpif_port->port_no, dpif_type));
     ovs_rwlock_unlock(&netdev_hmap_rwlock);
diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h
index e7fcedae9..43a98d499 100644
--- a/lib/netdev-offload.h
+++ b/lib/netdev-offload.h
@@ -108,8 +108,7 @@  bool netdev_is_offload_rebalance_policy_enabled(void);
 int netdev_flow_get_n_flows(struct netdev *netdev, uint64_t *n_flows);

 struct dpif_port;
-int netdev_ports_insert(struct netdev *, const char *dpif_type,
-                        struct dpif_port *);
+int netdev_ports_insert(struct netdev *, struct dpif_port *);
 struct netdev *netdev_ports_get(odp_port_t port, const char *dpif_type);
 int netdev_ports_remove(odp_port_t port, const char *dpif_type);
 odp_port_t netdev_ifindex_to_odp_port(int ifindex);
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 513ef7ea9..5223aa897 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -2052,8 +2052,6 @@  iface_do_create(const struct bridge *br,
         goto error;
     }

-    netdev_set_dpif_type(netdev, br->ofproto->type);
-
     error = iface_set_netdev_config(iface_cfg, netdev, errp);
     if (error) {
         goto error;