On 25.07.2019 19:31, Ilya Maximets wrote:
> Designated initializers are not suitable for initializing non-packed
> structures and unions which are subjects for comparison by memcmp().
>
> Whole memory for 'struct user_action_cookie' must be explicitly cleared
> before using because it will be copied with memcpy and later compared
> by memcmp in ofpbuf_equal().
>
> Few issues found be valgrind:
>
> Thread 13 revalidator11:
> Conditional jump or move depends on uninitialised value(s)
> at 0x4C35D96: __memcmp_sse4_1 (in vgpreload_memcheck.so)
> by 0x9D4404: ofpbuf_equal (ofpbuf.h:273)
> by 0x9D4404: revalidate_ukey__ (ofproto-dpif-upcall.c:2219)
> by 0x9D4404: revalidate_ukey (ofproto-dpif-upcall.c:2286)
> by 0x9D62AC: revalidate (ofproto-dpif-upcall.c:2685)
> by 0x9D62AC: udpif_revalidator (ofproto-dpif-upcall.c:942)
> by 0xA9C732: ovsthread_wrapper (ovs-thread.c:383)
> by 0x5FF86DA: start_thread (pthread_create.c:463)
> by 0x6AF488E: clone (clone.S:95)
> Uninitialised value was created by a stack allocation
> at 0x9D4450: compose_slow_path (ofproto-dpif-upcall.c:1062)
>
> Thread 11 revalidator16:
> Conditional jump or move depends on uninitialised value(s)
> at 0x4C35D96: __memcmp_sse4_1 (in vgpreload_memcheck.so)
> by 0x9D4404: ofpbuf_equal (ofpbuf.h:273)
> by 0x9D4404: revalidate_ukey__ (ofproto-dpif-upcall.c:2220)
> by 0x9D4404: revalidate_ukey (ofproto-dpif-upcall.c:2287)
> by 0x9D62BC: revalidate (ofproto-dpif-upcall.c:2686)
> by 0x9D62BC: udpif_revalidator (ofproto-dpif-upcall.c:942)
> by 0xA9C6D2: ovsthread_wrapper (ovs-thread.c:383)
> by 0x5FF86DA: start_thread (pthread_create.c:463)
> by 0x6AF488E: clone (clone.S:95)
> Uninitialised value was created by a stack allocation
> at 0x9DC4E0: compose_sflow_action (ofproto-dpif-xlate.c:3211)
>
> The struct was never marked as 'packed', however it was manually
> adjusted to be so in practice.
> Old IPFIX related commit first made the structure non-contiguous.
> Commit 8de6ff3ea864 ("ofproto-dpif: Use a fixed size userspace cookie.")
> added uninitialized parts of the additional union space and the next
> one introduced new holes between structure fields for all cases.
>
> CC: Justin Pettit <[email protected]>
> Fixes: 8b7ea2d48033 ("Extend OVS IPFIX exporter to export tunnel headers")
> Fixes: 8de6ff3ea864 ("ofproto-dpif: Use a fixed size userspace cookie.")
> Fixes: fcb9579be3c7 ("ofproto: Add 'ofproto_uuid' and 'ofp_in_port' to user
> action cookie.")
> Signed-off-by: Ilya Maximets <[email protected]>
> ---
Kind reminder.
It'll be good to have this before the official 2.12 release.
Best regards, Ilya Maximets.
> ofproto/ofproto-dpif-upcall.c | 1 +
> ofproto/ofproto-dpif-xlate.c | 52 +++++++++++++++++++----------------
> 2 files changed, 29 insertions(+), 24 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 731ee7280..f46cdf213 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -1064,6 +1064,7 @@ compose_slow_path(struct udpif *udpif, struct xlate_out
> *xout,
> odp_port_t port;
> uint32_t pid;
>
> + memset(&cookie, 0, sizeof cookie);
> cookie.type = USER_ACTION_COOKIE_SLOW_PATH;
> cookie.ofp_in_port = ofp_in_port;
> cookie.ofproto_uuid = *ofproto_uuid;
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 28a7fdd84..b7da8cead 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -3214,11 +3214,13 @@ compose_sflow_action(struct xlate_ctx *ctx)
> return 0;
> }
>
> - struct user_action_cookie cookie = {
> - .type = USER_ACTION_COOKIE_SFLOW,
> - .ofp_in_port = ctx->xin->flow.in_port.ofp_port,
> - .ofproto_uuid = ctx->xbridge->ofproto->uuid
> - };
> + struct user_action_cookie cookie;
> +
> + memset(&cookie, 0, sizeof cookie);
> + cookie.type = USER_ACTION_COOKIE_SFLOW;
> + cookie.ofp_in_port = ctx->xin->flow.in_port.ofp_port;
> + cookie.ofproto_uuid = ctx->xbridge->ofproto->uuid;
> +
> return compose_sample_action(ctx, dpif_sflow_get_probability(sflow),
> &cookie, ODPP_NONE, true);
> }
> @@ -3258,12 +3260,14 @@ compose_ipfix_action(struct xlate_ctx *ctx,
> odp_port_t output_odp_port)
> }
> }
>
> - struct user_action_cookie cookie = {
> - .type = USER_ACTION_COOKIE_IPFIX,
> - .ofp_in_port = ctx->xin->flow.in_port.ofp_port,
> - .ofproto_uuid = ctx->xbridge->ofproto->uuid,
> - .ipfix.output_odp_port = output_odp_port
> - };
> + struct user_action_cookie cookie;
> +
> + memset(&cookie, 0, sizeof cookie);
> + cookie.type = USER_ACTION_COOKIE_IPFIX;
> + cookie.ofp_in_port = ctx->xin->flow.in_port.ofp_port;
> + cookie.ofproto_uuid = ctx->xbridge->ofproto->uuid;
> + cookie.ipfix.output_odp_port = output_odp_port;
> +
> compose_sample_action(ctx,
> dpif_ipfix_get_bridge_exporter_probability(ipfix),
> &cookie, tunnel_out_port, false);
> @@ -5521,19 +5525,19 @@ xlate_sample_action(struct xlate_ctx *ctx,
> }
> }
>
> - struct user_action_cookie cookie = {
> - .type = USER_ACTION_COOKIE_FLOW_SAMPLE,
> - .ofp_in_port = ctx->xin->flow.in_port.ofp_port,
> - .ofproto_uuid = ctx->xbridge->ofproto->uuid,
> - .flow_sample = {
> - .probability = os->probability,
> - .collector_set_id = os->collector_set_id,
> - .obs_domain_id = os->obs_domain_id,
> - .obs_point_id = os->obs_point_id,
> - .output_odp_port = output_odp_port,
> - .direction = os->direction,
> - }
> - };
> + struct user_action_cookie cookie;
> +
> + memset(&cookie, 0, sizeof cookie);
> + cookie.type = USER_ACTION_COOKIE_FLOW_SAMPLE;
> + cookie.ofp_in_port = ctx->xin->flow.in_port.ofp_port;
> + cookie.ofproto_uuid = ctx->xbridge->ofproto->uuid;
> + cookie.flow_sample.probability = os->probability;
> + cookie.flow_sample.collector_set_id = os->collector_set_id;
> + cookie.flow_sample.obs_domain_id = os->obs_domain_id;
> + cookie.flow_sample.obs_point_id = os->obs_point_id;
> + cookie.flow_sample.output_odp_port = output_odp_port;
> + cookie.flow_sample.direction = os->direction;
> +
> compose_sample_action(ctx, probability, &cookie, tunnel_out_port, false);
> }
>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
