| Message ID | 20211217000557.1563487-1-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] bridge: Fix incorrect configuration of netdev's dpif type. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 2021-12-17 2:05 AM, Ilya Maximets wrote: > 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; Acked-by: Roi Dayan <roid@nvidia.com>
On 12/17/21 09:04, Roi Dayan wrote: > > > On 2021-12-17 2:05 AM, Ilya Maximets wrote: >> 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(-) >> > > Acked-by: Roi Dayan <roid@nvidia.com> Thanks! Applied. Best regards, Ilya Maximets.
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;
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(-)