Git Commit: 37763099533158d94670df8cc6882e782b0eb3be
Author: Eelco Chaudron <[email protected]>
Subject: dpif-offload: Add inline flow APIs for userspace datapaths.
This patch introduces new APIs in dpif-offload to allow userspace
datapaths to directly manage flows in the offloaded datapath,
providing efficient fast-path operations.
> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
> index 285d0e43f..df441c6aa 100644
> --- a/lib/dp-packet.h
> +++ b/lib/dp-packet.h
> @@ -48,6 +48,7 @@ enum OVS_PACKED_ENUM dp_packet_source {
> };
>
> #define DP_PACKET_CONTEXT_SIZE 64
> +#define INVALID_FLOW_MARK 0
Should this define be moved to dpif-offload.h where it's actually used?
The constant appears to be specific to flow marking rather than general
packet handling.
> diff --git a/lib/dpif-offload.c b/lib/dpif-offload.c
> index 0cff74a5d..a9ccadce9 100644
> --- a/lib/dpif-offload.c
> +++ b/lib/dpif-offload.c
[ ... ]
> @@ -1186,19 +1186,14 @@ dpif_offload_offload_get_netdev_by_port_id(struct
> dpif_offload *offload,
> return offload->class->get_netdev(offload, port_no);
> }
>
> -struct netdev *
> -dpif_offload_get_netdev_by_port_id(struct dpif *dpif,
> - struct dpif_offload **offload,
> - odp_port_t port_no)
> +static struct netdev *
> +dpif_offload_get_netdev_by_port_id_(struct dp_offload *dp_offload,
> + 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);
Does the removal of the dp_offload null check create a potential crash
in dpif_offload_get_netdev_by_port_id_? The LIST_FOR_EACH macro will
dereference dp_offload->offload_providers without checking if dp_offload
is null.
> +int
> +dpif_offload_datapath_flow_put(const char *dpif_name,
> + struct dpif_offload_flow_put *put,
> + uint32_t *flow_mark)
> +{
> + struct dpif_offload *offload;
> + struct dp_offload *dp_offload;
> + struct netdev *netdev;
> +
> + ovs_mutex_lock(&dpif_offload_mutex);
> + /* XXX: Implement a faster solution than the current dpif_name lookup. */
> + dp_offload = shash_find_data(&dpif_offload_providers, dpif_name);
> + ovs_mutex_unlock(&dpif_offload_mutex);
> +
> + if (OVS_UNLIKELY(!dp_offload)) {
> + if (flow_mark) {
> + *flow_mark = INVALID_FLOW_MARK;
> + }
> + return 0;
> + }
> +
> + netdev = dpif_offload_get_netdev_by_port_id_(dp_offload, &offload,
> + put->in_port);
Will dpif_offload_get_netdev_by_port_id_ crash if dp_offload is null?
The function no longer checks for null dp_offload internally.
> +
> + if (OVS_LIKELY(netdev && offload->class->netdev_flow_put)) {
> + return offload->class->netdev_flow_put(offload, netdev, put,
> + flow_mark);
> + }
Can 'offload' be null here if netdev is found but offload is not set by
dpif_offload_get_netdev_by_port_id_? This could cause a dereference of
null offload->class.
> +int
> +dpif_offload_datapath_flow_del(const char *dpif_name,
> + struct dpif_offload_flow_del *del,
> + uint32_t *flow_mark)
> +{
> + struct dpif_offload *offload;
> + struct dp_offload *dp_offload;
> + struct netdev *netdev;
> +
> + ovs_mutex_lock(&dpif_offload_mutex);
> + /* XXX: Implement a faster solution than the current dpif_name lookup. */
> + dp_offload = shash_find_data(&dpif_offload_providers, dpif_name);
> + ovs_mutex_unlock(&dpif_offload_mutex);
Should there be a null check on dpif_name parameter before calling
shash_find_data? The function could crash if dpif_name is null.
> +bool
> +dpif_offload_datapath_flow_stats(const char *dpif_name, odp_port_t in_port,
> + const ovs_u128 *ufid,
> + struct dpif_flow_stats *stats,
> + struct dpif_flow_attrs *attrs)
> +{
Are there null checks needed for the ufid, stats, or attrs parameters
before they're passed to the provider implementation? These pointers
could be dereferenced in the provider code.
> diff --git a/lib/dpif-offload.h b/lib/dpif-offload.h
> index 0de077919..b7d437c94 100644
> --- a/lib/dpif-offload.h
> +++ b/lib/dpif-offload.h
[ ... ]
> +static inline void dpif_offload_datapath_flow_op_continue(
> + struct dpif_offload_flow_cb_data *cb, struct dpif_flow_stats *stats,
> + uint32_t flow_mark, int error)
> +{
> + if (cb && cb->callback) {
> + cb->callback(cb->callback_aux_dp, cb->callback_aux_flow,
> + stats, flow_mark, error);
> + }
> +}
Does this function need to check if stats is null before passing it to
the callback? The callback might dereference stats without checking.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
