On 26 May 2023, at 10:17, Chris Mi wrote:
> On 5/10/2023 5:54 PM, Eelco Chaudron wrote:
>> On 26 Apr 2023, at 4:36, Chris Mi wrote:
>>
>>> On 4/12/2023 10:06 PM, Eelco Chaudron wrote:
>> <SNIP>
>>
>>>>> +{
>>>>> + new->type = old->type;
>>>>> + new->action = xmemdup(old->action, old->action->nla_len);
>>>>> + new->userspace_actions = old->userspace_actions
>>>>> + ? xmemdup(old->userspace_actions,
>>>>> + old->userspace_actions->nla_len)
>>>>> + : NULL;
>>>>> + new->userdata = xmemdup(old->userdata, old->userdata->nla_len);
>>>>> + new->tunnel = old->tunnel
>>>>> + ? xmemdup(old->tunnel, sizeof *old->tunnel)
>>>>> + : NULL;
>>>> For all 4 types of xmemdup, we should use a newly to be created
>>>> nullable_xmemdup(), see nullable_xstrdup() how/where to define it.
>>> But we need to get nla_len from the pointer, we need to check null pointer
>>> even if we create nullable_xmemdup().
>>> So it seems creating new api will not make code simple and clean. Maybe
>>> just add the null check for all pointers?
>> I think the code looks way cleaner like this (not tested):
>>
>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
>> index c48b85257..db8361ec5 100644
>> --- a/lib/netdev-offload-tc.c
>> +++ b/lib/netdev-offload-tc.c
>> @@ -153,15 +153,13 @@ offload_sample_clone(struct offload_sample *new,
>> const struct offload_sample *old)
>> {
>> new->type = old->type;
>> - new->action = xmemdup(old->action, old->action->nla_len);
>> - new->userspace_actions = old->userspace_actions
>> - ? xmemdup(old->userspace_actions,
>> - old->userspace_actions->nla_len)
>> - : NULL;
>> - new->userdata = xmemdup(old->userdata, old->userdata->nla_len);
>> - new->tunnel = old->tunnel
>> - ? xmemdup(old->tunnel, sizeof *old->tunnel)
>> - : NULL;
>> + new->action = nullable_xmemdup(old->action, old->action->nla_len);
>> + new->userdata = nullable_xmemdup(old->userdata, old->userdata->nla_len);
>> + new->tunnel = nullable_xmemdup(old->tunnel, sizeof *old->tunnel);
>> + new->userspace_actions = nullable_xmemdup(old->userspace_actions,
>> + old->userspace_actions ?
>> +
>> old->userspace_actions->nla_len |
>> + 0);
>> }
>>
>> /* Allocate a unique group id for the given set of flow metadata. The id
>> diff --git a/lib/util.c b/lib/util.c
>> index 96a71550d..53ca304dd 100644
>> --- a/lib/util.c
>> +++ b/lib/util.c
>> @@ -190,6 +190,12 @@ xmemdup(const void *p_, size_t size)
>> return p;
>> }
>>
>> +void *
>> +nullable_xmemdup(const void *p_, size_t size)
>> +{
>> + return *_p || size == 0 ? xmemdup(p_, size) : NULL;
>> +}
>> +
>> char *
>> xmemdup0(const char *p_, size_t length)
>> {
>> diff --git a/lib/util.h b/lib/util.h
>> index 62801e85f..90bb5b42b 100644
>> --- a/lib/util.h
>> +++ b/lib/util.h
>> @@ -168,6 +168,7 @@ void *xzalloc(size_t) MALLOC_LIKE;
>> void *xrealloc(void *, size_t);
>> void *xmemdup(const void *, size_t) MALLOC_LIKE;
>> char *xmemdup0(const char *, size_t) MALLOC_LIKE;
>> +void *nullable_xmemdup(const void *p_, size_t size) MALLOC_LIKE;
>> char *xstrdup(const char *) MALLOC_LIKE;
>> char *nullable_xstrdup(const char *) MALLOC_LIKE;
>> bool nullable_string_is_equal(const char *a, const char *b);
>>
> Done. And please see next reply.
>> <SNIP>
>>
>>>>> @@ -2128,6 +2239,11 @@ netdev_tc_parse_nl_actions(struct netdev *netdev,
>>>>> struct tc_flower *flower,
>>>>> if (err) {
>>>>> return err;
>>>>> }
>>>>> + } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_SAMPLE) {
>>>>> + struct offload_sample sample;
>>>>> +
>>>>> + memset(&sample, 0, sizeof sample);
>>>> We need the nullable_xmemdup() above, or the below sgid_alloc() would
>>>> crash.
>>> I added check for every pointer in offload_sample_clone(). It will not
>>> crash.
>> See comment above.
>>
>>>>> + sgid_alloc(&sample);
> Because in this patch all members in struct offload_sample are NULL, I'll
> change offload_sample_clone() to:
>
> static void
> offload_sample_clone(struct offload_sample *dst,
> const struct offload_sample *src)
> {
> dst->type = src->type;
> dst->action = nullable_xmemdup(src->action, src->action ?
> src->action->nla_len : 0);
> if (src->next_actions) {
> uint16_t len = src->next_actions_len;
>
> dst->userspace_actions = xmalloc(len + NLA_HDRLEN);
> nullable_memcpy((char *) dst->userspace_actions + NLA_HDRLEN,
> src->next_actions, len);
> dst->userspace_actions->nla_len = len + NLA_HDRLEN;
> }
> dst->userdata = nullable_xmemdup(src->userdata, src->userdata ?
> src->userdata->nla_len :
> 0);
> dst->tunnel = nullable_xmemdup(src->tunnel, sizeof *src->tunnel);
> dst->ifindex = src->ifindex;
> }
Thanks for fixing the nullable_memcopy(), see comments on patch7.
//Eelco
>>>> Maybe add 'return EOPNOTSUPP;' here because if people are doing a git
>>>> bisect this might result in tests failing.
>>> Done. I also added sgid_free() after sgid_alloc() to avoid possible memory
>>> leaks.
>> Thanks!
>>
>>>>> } else {
>>>>> VLOG_DBG_RL(&rl, "unsupported put action type: %d",
>>>>> nl_attr_type(nla));
>>>>> @@ -2871,6 +2987,9 @@ netdev_tc_init_flow_api(struct netdev *netdev)
>>>>> tc_cleanup_policer_actions(meter_police_ids,
>>>>> METER_POLICE_IDS_BASE,
>>>>> METER_POLICE_IDS_MAX);
>>>>> ovs_mutex_unlock(&meter_police_ids_mutex);
>>>>> + ovs_mutex_lock(&sgid_lock);
>>>>> + sample_group_ids = id_pool_create(1, UINT32_MAX - 1);
>>>>> + ovs_mutex_unlock(&sgid_lock);
>>>>>
>>>>> ovsthread_once_done(&once);
>>>>> }
>>>>> --
>>>>> 2.26.3
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
