On 8/11/21 1:25 PM, Ilya Maximets wrote:
> On 8/11/21 7:05 AM, Sriharsha Basavapatna via dev wrote:
>> On Wed, Aug 11, 2021 at 6:21 AM Gaëtan Rivet <gr...@u256.net> wrote:
>>>
>>> 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.basavapa...@broadcom.com>
>>>
>>> Hello Harsha,
>>>
>>> Thanks for the fix. I have a remark below.
>>
>> Thanks Gaetan, please see my response inline.
>>
>>>
>>>> ---
>>>> 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.
>>
>> Are you suggesting something like this in ds_clone():
>>
>> if (source->string)
>> memcpy(dst->string, source->string, dst->allocated + 1);
>>
>> I suspect that's inconsistent with other functions in the ds library.
>> The expectation seems to be that callers initialize their strings with
>> a call to ds_cstr() before using a ds string.>
>> Here's the comments from the header file for reference:
>>
>> * The 'string' member does not always point to a null-terminated string.
>> * Initially it is NULL, and even when it is nonnull, some operations do not
>> * ensure that it is null-terminated. Use ds_cstr() to ensure that memory is
>> * allocated for the string and that it is null-terminated. */
>
> This part talks about Null-termination. ds_cstr() should not be needed
> for the initialization of a dynamic string. The problem here, I think,
> that ds_clone doesn't check if memory was allocated, so it incorrectly
> clones the empty dynamic string. How about this (not tested):
>
> diff --git a/lib/dynamic-string.c b/lib/dynamic-string.c
> index 6f7b610a9..5e3f46bb5 100644
> --- a/lib/dynamic-string.c
> +++ b/lib/dynamic-string.c
> @@ -460,6 +460,10 @@ ds_chomp(struct ds *ds, int c)
> void
> ds_clone(struct ds *dst, struct ds *source)
> {
> + if (!source->allocated) {
> + ds_init(dst);
> + return;
> + }
> dst->length = source->length;
> dst->allocated = dst->length;
> dst->string = xmalloc(dst->allocated + 1);
> ---
And, I think, the fix for dynamic string library should be a separate
patch, so we can backport it further.
>
> ?
>
> ds_clone seems to be a fairly new function and it was used only in one
> place in OVN until now, so it wasn't tested enough. We also seems to
> not have any unit tests for dynamic-string library...
>
>> struct ds {
>> char *string; /* Null-terminated string. */
>>
>> Let me know what you think.
>>
>> Thanks,
>> -Harsha
>>>
>>> --
>>> Gaetan Rivet
>>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
