[ovs-dev] [PATCH] netdev-offload-dpdk: fix a crash in dump_flow_attr()

[Gaëtan Rivet]

On Wed, Aug 4, 2021, at 14:37, Sriharsha Basavapatna via dev wrote:
> In netdev_offload_dpdk_flow_create() when an offload request fails,
> dump_flow() is called to log a warning message. The 's_tnl' string
> in flow_patterns gets initialized in vport_to_rte_tunnel() conditionally
> via ds_put_format(). If it is not initialized, it crashes later in
> dump_flow_attr()->ds_clone()->memcpy() while dereferencing this string.
> 
> Fix this by initializing s_tnl using ds_cstr(). Fix a similar
> issue with actions->s_tnl.
> 
> Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapatna at broadcom.com>

Hello Harsha,

Thanks for the fix. I have a remark below.

> ---
>  lib/netdev-offload-dpdk.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
> index f6706ee0c..243e2c430 100644
> --- a/lib/netdev-offload-dpdk.c
> +++ b/lib/netdev-offload-dpdk.c
> @@ -788,6 +788,7 @@ free_flow_patterns(struct flow_patterns *patterns)
>              free(CONST_CAST(void *, patterns->items[i].mask));
>          }
>      }
> +    ds_destroy(&patterns->s_tnl);
>      free(patterns->items);
>      patterns->items = NULL;
>      patterns->cnt = 0;
> @@ -1334,6 +1335,7 @@ netdev_offload_dpdk_mark_rss(struct flow_patterns 
> *patterns,
>      struct rte_flow_error error;
>      struct rte_flow *flow;
>  
> +    ds_cstr(&actions.s_tnl);

You are forced to use 'ds_cstr' because 'ds_clone' crashes on properly initialized
ds. I think this is an issue with 'ds_clone'.

I would expect all ds_ functions to work with strings that were initialized with
'ds_init' or set to 'DS_EMPTY_INITIALIZER'.

In this case, I think it's better to use .s_tnl = DS_EMPTY_INITIALIZER in the actions
initializer list and have a fix for 'ds_clone' so that it won't crash with correct strings.

-- 
Gaetan Rivet