| Message ID | 20240508084510.1578039-1-xsimonar@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] conntrack: Do not use {0} to initialize unions. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
Hello Xavier, just curious, based on your tests, is clang 18.1.1 the only compiler/version known so far to lead to the problem, right? Anyways, only a small cosmetic nit below. Other than that: Acked-by: Paolo Valerio <pvalerio@redhat.com> Xavier Simonart <xsimonar@redhat.com> writes: > In the following case: > union ct_addr { > unsigned int ipv4; > struct in6_addr ipv6; > }; > union ct_addr zero_ip = {0}; > > The ipv6 field might not be properly initialized. > For instance, clang 18.1.1 does not initialize the ipv6 field. > > Reported-at: https://issues.redhat.com/browse/FDP-608 > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > --- > lib/conntrack.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/lib/conntrack.c b/lib/conntrack.c > index 16e1c8bb5..ff4a17abc 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -2302,7 +2302,8 @@ find_addr(const struct conn_key *key, union ct_addr *min, > uint32_t hash, bool ipv4, > const struct nat_action_info_t *nat_info) > { > - const union ct_addr zero_ip = {0}; > + union ct_addr zero_ip; > + memset(&zero_ip, 0, sizeof zero_ip); > > /* All-zero case. */ > if (!memcmp(min, &zero_ip, sizeof *min)) { > @@ -2394,7 +2395,7 @@ nat_get_unique_tuple(struct conntrack *ct, struct conn *conn, > { > struct conn_key *fwd_key = &conn->key_node[CT_DIR_FWD].key; > struct conn_key *rev_key = &conn->key_node[CT_DIR_REV].key; > - union ct_addr min_addr = {0}, max_addr = {0}, addr = {0}; > + union ct_addr min_addr, max_addr, addr; nit: please keep the reverse xmas tree > bool pat_proto = fwd_key->nw_proto == IPPROTO_TCP || > fwd_key->nw_proto == IPPROTO_UDP || > fwd_key->nw_proto == IPPROTO_SCTP; > @@ -2402,6 +2403,10 @@ nat_get_unique_tuple(struct conntrack *ct, struct conn *conn, > uint16_t min_sport, max_sport, curr_sport; > uint32_t hash, port_off, basis; > > + memset(&min_addr, 0, sizeof min_addr); > + memset(&max_addr, 0, sizeof max_addr); > + memset(&addr, 0, sizeof addr); > + > basis = (nat_info->nat_flags & NAT_PERSISTENT) ? 0 : ct->hash_basis; > hash = nat_range_hash(fwd_key, basis, nat_info); > > -- > 2.31.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Hi Paolo Thanks for the review. On Wed, May 8, 2024 at 5:43 PM Paolo Valerio <pvalerio@redhat.com> wrote: > Hello Xavier, > > just curious, based on your tests, is clang 18.1.1 the only > compiler/version known so far to lead to the problem, right? > Yes. We have not seen any issues (so far) with gcc. Older clang versions were ok, and clang 18.1.3 is fine as well. But, as this is not a clang 18.1.1 issue (it's ok not to initialize ipv6 extra bits in this case), we still fix it in ovs, as it might come back... > > Anyways, only a small cosmetic nit below. Other than that: > > Acked-by: Paolo Valerio <pvalerio@redhat.com> > > Xavier Simonart <xsimonar@redhat.com> writes: > > > In the following case: > > union ct_addr { > > unsigned int ipv4; > > struct in6_addr ipv6; > > }; > > union ct_addr zero_ip = {0}; > > > > The ipv6 field might not be properly initialized. > > For instance, clang 18.1.1 does not initialize the ipv6 field. > > > > Reported-at: https://issues.redhat.com/browse/FDP-608 > > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > > --- > > lib/conntrack.c | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/lib/conntrack.c b/lib/conntrack.c > > index 16e1c8bb5..ff4a17abc 100644 > > --- a/lib/conntrack.c > > +++ b/lib/conntrack.c > > @@ -2302,7 +2302,8 @@ find_addr(const struct conn_key *key, union > ct_addr *min, > > uint32_t hash, bool ipv4, > > const struct nat_action_info_t *nat_info) > > { > > - const union ct_addr zero_ip = {0}; > > + union ct_addr zero_ip; > > + memset(&zero_ip, 0, sizeof zero_ip); > > > > /* All-zero case. */ > > if (!memcmp(min, &zero_ip, sizeof *min)) { > > @@ -2394,7 +2395,7 @@ nat_get_unique_tuple(struct conntrack *ct, struct > conn *conn, > > { > > struct conn_key *fwd_key = &conn->key_node[CT_DIR_FWD].key; > > struct conn_key *rev_key = &conn->key_node[CT_DIR_REV].key; > > - union ct_addr min_addr = {0}, max_addr = {0}, addr = {0}; > > + union ct_addr min_addr, max_addr, addr; > > nit: please keep the reverse xmas tree > Will send v2 > > > bool pat_proto = fwd_key->nw_proto == IPPROTO_TCP || > > fwd_key->nw_proto == IPPROTO_UDP || > > fwd_key->nw_proto == IPPROTO_SCTP; > > @@ -2402,6 +2403,10 @@ nat_get_unique_tuple(struct conntrack *ct, struct > conn *conn, > > uint16_t min_sport, max_sport, curr_sport; > > uint32_t hash, port_off, basis; > > > > + memset(&min_addr, 0, sizeof min_addr); > > + memset(&max_addr, 0, sizeof max_addr); > > + memset(&addr, 0, sizeof addr); > > + > > basis = (nat_info->nat_flags & NAT_PERSISTENT) ? 0 : ct->hash_basis; > > hash = nat_range_hash(fwd_key, basis, nat_info); > > > > -- > > 2.31.1 > > > Thanks Xavier > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
diff --git a/lib/conntrack.c b/lib/conntrack.c index 16e1c8bb5..ff4a17abc 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -2302,7 +2302,8 @@ find_addr(const struct conn_key *key, union ct_addr *min, uint32_t hash, bool ipv4, const struct nat_action_info_t *nat_info) { - const union ct_addr zero_ip = {0}; + union ct_addr zero_ip; + memset(&zero_ip, 0, sizeof zero_ip); /* All-zero case. */ if (!memcmp(min, &zero_ip, sizeof *min)) { @@ -2394,7 +2395,7 @@ nat_get_unique_tuple(struct conntrack *ct, struct conn *conn, { struct conn_key *fwd_key = &conn->key_node[CT_DIR_FWD].key; struct conn_key *rev_key = &conn->key_node[CT_DIR_REV].key; - union ct_addr min_addr = {0}, max_addr = {0}, addr = {0}; + union ct_addr min_addr, max_addr, addr; bool pat_proto = fwd_key->nw_proto == IPPROTO_TCP || fwd_key->nw_proto == IPPROTO_UDP || fwd_key->nw_proto == IPPROTO_SCTP; @@ -2402,6 +2403,10 @@ nat_get_unique_tuple(struct conntrack *ct, struct conn *conn, uint16_t min_sport, max_sport, curr_sport; uint32_t hash, port_off, basis; + memset(&min_addr, 0, sizeof min_addr); + memset(&max_addr, 0, sizeof max_addr); + memset(&addr, 0, sizeof addr); + basis = (nat_info->nat_flags & NAT_PERSISTENT) ? 0 : ct->hash_basis; hash = nat_range_hash(fwd_key, basis, nat_info);
In the following case: union ct_addr { unsigned int ipv4; struct in6_addr ipv6; }; union ct_addr zero_ip = {0}; The ipv6 field might not be properly initialized. For instance, clang 18.1.1 does not initialize the ipv6 field. Reported-at: https://issues.redhat.com/browse/FDP-608 Signed-off-by: Xavier Simonart <xsimonar@redhat.com> --- lib/conntrack.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)