On 5/21/21 9:23 AM, Mark Gray wrote: > On 20/05/2021 19:24, Ilya Maximets wrote: >> On 5/20/21 7:46 PM, Ilya Maximets wrote: >>> On 5/20/21 6:55 PM, Mark Gray wrote: >>>> On 04/04/2021 18:31, Ilya Maximets wrote: >>>>> ct limit requests never initializes the whole 'struct ovs_zone_limit' >>>>> sending uninitialized stack memory to kernel: >>>>> >>>>> Syscall param sendmsg(msg.msg_iov[0]) points to uninitialised byte(s) >>>>> at 0x5E23867: sendmsg (in /usr/lib64/libpthread-2.28.so) >>>>> by 0x54F761: nl_sock_transact_multiple__ (netlink-socket.c:858) >>>>> by 0x54FB6E: nl_sock_transact_multiple.part.9 (netlink-socket.c:1079) >>>>> by 0x54FCC0: nl_sock_transact_multiple (netlink-socket.c:1044) >>>>> by 0x54FCC0: nl_sock_transact (netlink-socket.c:1108) >>>>> by 0x550B6F: nl_transact (netlink-socket.c:1804) >>>>> by 0x53BEA2: dpif_netlink_ct_get_limits (dpif-netlink.c:3052) >>>>> by 0x588B57: dpctl_ct_get_limits (dpctl.c:2178) >>>>> by 0x586FF2: dpctl_unixctl_handler (dpctl.c:2870) >>>>> by 0x52C241: process_command (unixctl.c:310) >>>>> by 0x52C241: run_connection (unixctl.c:344) >>>>> by 0x52C241: unixctl_server_run (unixctl.c:395) >>>>> by 0x407526: main (ovs-vswitchd.c:128) >>>>> Address 0x10b87480 is 32 bytes inside a block of size 4,096 alloc'd >>>>> at 0x4C30F0B: malloc (vg_replace_malloc.c:307) >>>>> by 0x52CDE4: xmalloc (util.c:138) >>>>> by 0x4F7E07: ofpbuf_init (ofpbuf.c:123) >>>>> by 0x4F7E07: ofpbuf_new (ofpbuf.c:151) >>>>> by 0x53BDE3: dpif_netlink_ct_get_limits (dpif-netlink.c:3025) >>>>> by 0x588B57: dpctl_ct_get_limits (dpctl.c:2178) >>>>> by 0x586FF2: dpctl_unixctl_handler (dpctl.c:2870) >>>>> by 0x52C241: process_command (unixctl.c:310) >>>>> by 0x52C241: run_connection (unixctl.c:344) >>>>> by 0x52C241: unixctl_server_run (unixctl.c:395) >>>>> by 0x407526: main (ovs-vswitchd.c:128) >>>>> Uninitialised value was created by a stack allocation >>>>> at 0x46AAA0: ct_dpif_get_limits (ct-dpif.c:197) >>>>> >>>>> Fix that by using designated initializers that will clear all the >>>>> non-specified fields. >>>>> >>>>> CC: Yi-Hung Wei <[email protected]> >>>>> Fixes: 906ff9d229ee ("dpif-netlink: Implement conntrack zone limit") >>>>> Signed-off-by: Ilya Maximets <[email protected]> >>>>> --- >>>>> lib/dpif-netlink.c | 24 ++++++++++++++---------- >>>>> 1 file changed, 14 insertions(+), 10 deletions(-) >>>>> >>>>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c >>>>> index ceb56c685..ee96f4011 100644 >>>>> --- a/lib/dpif-netlink.c >>>>> +++ b/lib/dpif-netlink.c >>>>> @@ -2923,8 +2923,6 @@ dpif_netlink_ct_set_limits(struct dpif *dpif >>>>> OVS_UNUSED, >>>>> const uint32_t *default_limits, >>>>> const struct ovs_list *zone_limits) >>>>> { >>>>> - struct ovs_zone_limit req_zone_limit; >>>>> - >>>>> if (ovs_ct_limit_family < 0) { >>>>> return EOPNOTSUPP; >>>>> } >>>>> @@ -2941,8 +2939,10 @@ dpif_netlink_ct_set_limits(struct dpif *dpif >>>>> OVS_UNUSED, >>>>> size_t opt_offset; >>>>> opt_offset = nl_msg_start_nested(request, >>>>> OVS_CT_LIMIT_ATTR_ZONE_LIMIT); >>>>> if (default_limits) { >>>>> - req_zone_limit.zone_id = OVS_ZONE_LIMIT_DEFAULT_ZONE; >>>>> - req_zone_limit.limit = *default_limits; >>>>> + struct ovs_zone_limit req_zone_limit = { >>>>> + .zone_id = OVS_ZONE_LIMIT_DEFAULT_ZONE, >>>>> + .limit = *default_limits, >>>>> + }; >>>>> nl_msg_put(request, &req_zone_limit, sizeof req_zone_limit); >>>>> } >>>>> >>>>> @@ -2950,8 +2950,10 @@ dpif_netlink_ct_set_limits(struct dpif *dpif >>>>> OVS_UNUSED, >>>>> struct ct_dpif_zone_limit *zone_limit; >>>>> >>>>> LIST_FOR_EACH (zone_limit, node, zone_limits) { >>>>> - req_zone_limit.zone_id = zone_limit->zone; >>>>> - req_zone_limit.limit = zone_limit->limit; >>>>> + struct ovs_zone_limit req_zone_limit = { >>>>> + .zone_id = zone_limit->zone, >>>>> + .limit = zone_limit->limit, >>>>> + }; >>>>> nl_msg_put(request, &req_zone_limit, sizeof req_zone_limit); >>>>> } >>>>> } >>>>> @@ -3035,8 +3037,9 @@ dpif_netlink_ct_get_limits(struct dpif *dpif >>>>> OVS_UNUSED, >>>>> size_t opt_offset = nl_msg_start_nested(request, >>>>> >>>>> OVS_CT_LIMIT_ATTR_ZONE_LIMIT); >>>>> >>>>> - struct ovs_zone_limit req_zone_limit; >>>>> - req_zone_limit.zone_id = OVS_ZONE_LIMIT_DEFAULT_ZONE; >>>>> + struct ovs_zone_limit req_zone_limit = { >>>>> + .zone_id = OVS_ZONE_LIMIT_DEFAULT_ZONE, >>>>> + }; >>>>> nl_msg_put(request, &req_zone_limit, sizeof req_zone_limit); >>>>> >>>>> struct ct_dpif_zone_limit *zone_limit; >>>>> @@ -3086,8 +3089,9 @@ dpif_netlink_ct_del_limits(struct dpif *dpif >>>>> OVS_UNUSED, >>>>> >>>>> struct ct_dpif_zone_limit *zone_limit; >>>>> LIST_FOR_EACH (zone_limit, node, zone_limits) { >>>>> - struct ovs_zone_limit req_zone_limit; >>>>> - req_zone_limit.zone_id = zone_limit->zone; >>>>> + struct ovs_zone_limit req_zone_limit = { >>>>> + .zone_id = zone_limit->zone, >>>> >>>> Does this set 'limit' and 'count' automatically to 0? Is this behaviour >>>> specified by the relevant c standard or is it compiler dependent? >>> >>> Yes. C standard says following: >>> >>> If there are fewer initializers in a list than there are members of an >>> aggregate, >>> the remainder of the aggregate shall be initialized implicitly the same >>> as objects >>> that have static storage duration. >>> >>> This means that they will be zeroed. This doesn't work for initialization >>> of unions with fields that have different sizes, but it's not the case here. >> >> To be more correct, this also doesn't work well with paddings inside the >> structure, i.e. doesn't clear them. But this structure has no paddings. >> >>> >>> Best regards, Ilya Maximets. >>> >> > > Thanks for checking. LGTM > > Acked-by: Mark D. Gray <[email protected]> >
Thanks! Applied to master and backported down to 2.12. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
