git sha: 12583b97a68bfc2efd1ec5cbea591c25f926ec3a
Author: Eelco Chaudron <[email protected]>
Subject: dpif-netlink: Add API to get offloaded netdev from port_id.
This patch adds an API to retrieve the netdev by port_id from either
the dpif or a specific offload provider.
> diff --git a/lib/dpif-offload-dpdk.c b/lib/dpif-offload-dpdk.c
> index c8ae1adb2..7a4cc7292 100644
> --- a/lib/dpif-offload-dpdk.c
> +++ b/lib/dpif-offload-dpdk.c
> @@ -103,6 +103,20 @@ dpif_offload_dpdk_port_del(struct dpif_offload
> *offload_, odp_port_t port_no)
> return ret;
> }
>
> +static struct netdev *
> +dpif_offload_dpdk_get_netdev(struct dpif_offload *offload_, odp_port_t
> port_no)
This function name has a whitespace issue - the line is getting fairly
long at 95 characters. Consider breaking after the return type.
> +{
> + struct dpif_offload_dpdk *offload = dpif_offload_dpdk_cast(offload_);
> + struct dpif_offload_port_mgr_port *port;
> +
> + port = dpif_offload_port_mgr_find_by_odp_port(offload->port_mgr,
> port_no);
> + if (!port) {
> + return NULL;
> + }
> +
> + return port->netdev;
Is this function missing a reference increment? The retrieved netdev
might be freed while the caller is using it if no additional reference
is held.
> +}
> +
> static int
> dpif_offload_dpdk_open(const struct dpif_offload_class *offload_class,
> struct dpif *dpif, struct dpif_offload **offload_)
[ ... ]
> diff --git a/lib/dpif-offload-dummy.c b/lib/dpif-offload-dummy.c
> index 4b7a292bb..96d589b9f 100644
> --- a/lib/dpif-offload-dummy.c
> +++ b/lib/dpif-offload-dummy.c
> @@ -97,6 +97,24 @@ dpif_offload_dummy_port_del(struct dpif_offload
> *dpif_offload,
> return 0;
> }
>
> +static struct netdev *
> +dpif_offload_dummy_get_netdev(struct dpif_offload *dpif_offload,
> + odp_port_t port_no)
> +{
> + struct dpif_offload_dummy *offload_dummy;
> + struct dpif_offload_port_mgr_port *port;
> +
> + offload_dummy = dpif_offload_dummy_cast(dpif_offload);
> +
> + port = dpif_offload_port_mgr_find_by_odp_port(offload_dummy->port_mgr,
> + port_no);
> + if (!port) {
> + return NULL;
> + }
> +
> + return port->netdev;
Same issue here - is this returning a netdev without incrementing its
reference count? This could lead to use-after-free if the netdev is
destroyed while the caller is using it.
> +}
[ ... ]
> diff --git a/lib/dpif-offload-tc.c b/lib/dpif-offload-tc.c
> index e2e18b5b1..961953961 100644
> --- a/lib/dpif-offload-tc.c
> +++ b/lib/dpif-offload-tc.c
> @@ -127,6 +127,22 @@ dpif_offload_tc_port_del(struct dpif_offload
> *dpif_offload,
> return ret;
> }
>
> +static struct netdev *
> +dpif_offload_tc_get_netdev(struct dpif_offload *dpif_offload,
> + odp_port_t port_no)
> +{
> + struct dpif_offload_tc *offload_tc = dpif_offload_tc_cast(dpif_offload);
> + struct dpif_offload_port_mgr_port *port;
> +
> + port = dpif_offload_port_mgr_find_by_odp_port(offload_tc->port_mgr,
> + port_no);
> + if (!port) {
> + return NULL;
> + }
> +
> + return port->netdev;
This function also returns port->netdev directly. Should this be taking
a reference on the netdev before returning it to ensure the caller can
safely use it?
> +}
[ ... ]
> diff --git a/lib/dpif-offload.c b/lib/dpif-offload.c
> index 28b3c1d21..77229c861 100644
> --- a/lib/dpif-offload.c
> +++ b/lib/dpif-offload.c
> @@ -1009,6 +1010,44 @@ dpif_offload_flow_dump_thread_destroy(struct
> dpif_flow_dump_thread *thread)
> free(thread->offload_threads);
> }
>
> +struct netdev *
> +dpif_offload_offload_get_netdev_by_port_id(struct dpif_offload *offload,
> + odp_port_t port_no)
Function name has "offload_offload" - is this duplication intentional?
> +{
> + if (!dpif_offload_is_offload_enabled() || !offload) {
> + return NULL;
> + }
> +
> + return offload->class->get_netdev(offload, port_no);
The call to offload->class->get_netdev potentially returns a netdev
without an incremented reference. Should callers of this function be
expected to unref the returned netdev?
> +}
> +
> +struct netdev *
> +dpif_offload_get_netdev_by_port_id(struct dpif *dpif,
> + struct dpif_offload **offload,
> + odp_port_t port_no)
> +{
> + struct dp_offload *dp_offload = dpif_offload_get_dp_offload(dpif);
> + struct dpif_offload *tmp_offload;
> + struct netdev *netdev = NULL;
> +
> + if (!dp_offload || !dpif_offload_is_offload_enabled()) {
> + return NULL;
> + }
> +
> + LIST_FOR_EACH (tmp_offload, dpif_list_node,
> + &dp_offload->offload_providers) {
> + netdev = tmp_offload->class->get_netdev(tmp_offload, port_no);
> + if (netdev) {
> + if (offload) {
> + *offload = tmp_offload;
> + }
> + break;
> + }
> + }
> +
> + return netdev;
The returned netdev here comes from the same get_netdev calls that may
not increment references. Are callers expected to handle this
consistently?
> +}
[ ... ]
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 3fa96b673..d0c537bc9 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -2710,7 +2710,6 @@ ukey_netdev_unref(struct udpif_key *ukey)
> static void
> ukey_to_flow_netdev(struct udpif *udpif, struct udpif_key *ukey)
> {
> - const char *dpif_type_str = dpif_normalize_type(dpif_type(udpif->dpif));
> const struct nlattr *k;
> unsigned int left;
>
> @@ -2722,8 +2721,8 @@ ukey_to_flow_netdev(struct udpif *udpif, struct
> udpif_key *ukey)
> enum ovs_key_attr type = nl_attr_type(k);
>
> if (type == OVS_KEY_ATTR_IN_PORT) {
> - ukey->in_netdev = netdev_ports_get(nl_attr_get_odp_port(k),
> - dpif_type_str);
> + ukey->in_netdev = dpif_offload_get_netdev_by_port_id(
> + udpif->dpif, NULL, nl_attr_get_odp_port(k));
This change replaces netdev_ports_get with
dpif_offload_get_netdev_by_port_id. Does netdev_ports_get increment the
reference count while the new function does not? This could create a
reference counting mismatch with the existing ukey_netdev_unref function.
> } else if (type == OVS_KEY_ATTR_TUNNEL) {
> struct flow_tnl tnl;
> enum odp_key_fitness res;
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
