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