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 <[email protected]> 代表 Ilya Maximets > <[email protected]> > 发送时间: 2021年12月17日 8:05 > 收件人: [email protected] > 抄送: 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 <[email protected]> > Reported-at: > https://mail.openvswitch.org/pipermail/ovs-dev/2021-December/390117.html > Signed-off-by: Ilya Maximets <[email protected]> > --- > 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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
