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);
<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);
>> 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
