Re: [ovs-dev] [PATCH] conntrack: Do not use {0} to initialize unions.
Hi Paolo
Thanks for the review.
On Wed, May 8, 2024 at 5:43 PM Paolo Valerio 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
>
> Xavier Simonart 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
> > ---
> > 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(_ip, 0, sizeof zero_ip);
> >
> > /* All-zero case. */
> > if (!memcmp(min, _ip, sizeof *min)) {
> > @@ -2394,7 +2395,7 @@ nat_get_unique_tuple(struct conntrack *ct, struct
> conn *conn,
> > {
> > struct conn_key *fwd_key = >key_node[CT_DIR_FWD].key;
> > struct conn_key *rev_key = >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(_addr, 0, sizeof min_addr);
> > +memset(_addr, 0, sizeof max_addr);
> > +memset(, 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
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] conntrack: Do not use {0} to initialize unions.
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
Xavier Simonart 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
> ---
> 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(_ip, 0, sizeof zero_ip);
>
> /* All-zero case. */
> if (!memcmp(min, _ip, sizeof *min)) {
> @@ -2394,7 +2395,7 @@ nat_get_unique_tuple(struct conntrack *ct, struct conn
> *conn,
> {
> struct conn_key *fwd_key = >key_node[CT_DIR_FWD].key;
> struct conn_key *rev_key = >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(_addr, 0, sizeof min_addr);
> +memset(_addr, 0, sizeof max_addr);
> +memset(, 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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] conntrack: Do not use {0} to initialize unions.
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
---
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(_ip, 0, sizeof zero_ip);
/* All-zero case. */
if (!memcmp(min, _ip, sizeof *min)) {
@@ -2394,7 +2395,7 @@ nat_get_unique_tuple(struct conntrack *ct, struct conn
*conn,
{
struct conn_key *fwd_key = >key_node[CT_DIR_FWD].key;
struct conn_key *rev_key = >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(_addr, 0, sizeof min_addr);
+memset(_addr, 0, sizeof max_addr);
+memset(, 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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
