Re: [ovs-dev] [branch-2.9 3/3] conntrack: Replace structure copy by memcpy().

[Ben Pfaff]

On Wed, May 08, 2019 at 11:41:16AM +0300, Ilya Maximets wrote:
> > On Thu, May 02, 2019 at 05:29:16PM -0700, Darrell Ball wrote:
> >> There are a few cases where structure copy can be replaced by
> >> memcpy(), for possible portability benefit.  This is because
> >> the structures involved have padding and elements of the
> >> structure are used to generate hashes.
> >> 
> >> Signed-off-by: Darrell Ball 
> > 
> > Thanks for the backports.  I applied them to branch-2.9.
> 
> Hi. These patches broke the clang build on branch-2.9 and it looks
> like not only the build:
> 
> lib/conntrack.c:2563:26: error: expecting mutex 'ctb->lock' to be held at 
> start
>   of each loop [-Werror,-Wthread-safety-analysis]
> for (unsigned i = 0; i < CONNTRACK_BUCKETS; i++) {

Oops.  I always test that master builds with clang, but I only test
backports with GCC.  Maybe I should do both for backports too.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [branch-2.9 3/3] conntrack: Replace structure copy by memcpy().

[Ilya Maximets]

On 08.05.2019 12:20, Darrell Ball wrote:
> 
> 
> On Wed, May 8, 2019 at 1:41 AM Ilya Maximets  > wrote:
> 
> > On Thu, May 02, 2019 at 05:29:16PM -0700, Darrell Ball wrote:
> >> There are a few cases where structure copy can be replaced by
> >> memcpy(), for possible portability benefit.  This is because
> >> the structures involved have padding and elements of the
> >> structure are used to generate hashes.
> >>
> >> Signed-off-by: Darrell Ball http://gmail.com>>
> >
> > Thanks for the backports.  I applied them to branch-2.9.
> 
> Hi. These patches broke the clang build on branch-2.9 and it looks
> like not only the build:
> 
> 
> Thanks for the report Ilya
> 
> Besides the clang false positive, what else was broken ?

Nothing. Was an issue with -Wno-null-pointer-arithmetic on branch-2.9 that I
mistakenly treated as some other problem. We, probably, may want to backport
a7021b08b0d5 ("configure: Disable -Wnull-pointer-arithmetic Clang warning.")
because branches <= 2.9 fails to build with relatively new clang.

Thanks for the fix for false positives. I'll check it.

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [branch-2.9 3/3] conntrack: Replace structure copy by memcpy().

[Darrell Ball]

On Wed, May 8, 2019 at 1:41 AM Ilya Maximets  wrote:

> > On Thu, May 02, 2019 at 05:29:16PM -0700, Darrell Ball wrote:
> >> There are a few cases where structure copy can be replaced by
> >> memcpy(), for possible portability benefit.  This is because
> >> the structures involved have padding and elements of the
> >> structure are used to generate hashes.
> >>
> >> Signed-off-by: Darrell Ball 
> >
> > Thanks for the backports.  I applied them to branch-2.9.
>
> Hi. These patches broke the clang build on branch-2.9 and it looks
> like not only the build:
>

Thanks for the report Ilya

Besides the clang false positive, what else was broken ?



>
> lib/conntrack.c:2563:26: error: expecting mutex 'ctb->lock' to be held at
> start
>   of each loop [-Werror,-Wthread-safety-analysis]
> for (unsigned i = 0; i < CONNTRACK_BUCKETS; i++) {
>  ^
> lib/conntrack.c:2566:9: note: mutex acquired here
> ct_lock_lock(&ctb->lock);
> ^
> lib/conntrack.c:2575:9: error: releasing mutex '->buckets[i].lock' that
> was not
>   held [-Werror,-Wthread-safety-analysis]
> ct_lock_unlock(&ct->buckets[i].lock);
> ^
> 2 errors generated.
> make[2]: *** [lib/conntrack.lo] Error 1
>
> https://travis-ci.org/openvswitch/ovs/builds/528984734
>
>
> Best regards, Ilya Maximets.
> ___
> 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] [branch-2.9 3/3] conntrack: Replace structure copy by memcpy().

[Ilya Maximets]

> On Thu, May 02, 2019 at 05:29:16PM -0700, Darrell Ball wrote:
>> There are a few cases where structure copy can be replaced by
>> memcpy(), for possible portability benefit.  This is because
>> the structures involved have padding and elements of the
>> structure are used to generate hashes.
>> 
>> Signed-off-by: Darrell Ball 
> 
> Thanks for the backports.  I applied them to branch-2.9.

Hi. These patches broke the clang build on branch-2.9 and it looks
like not only the build:

lib/conntrack.c:2563:26: error: expecting mutex 'ctb->lock' to be held at start
  of each loop [-Werror,-Wthread-safety-analysis]
for (unsigned i = 0; i < CONNTRACK_BUCKETS; i++) {
 ^
lib/conntrack.c:2566:9: note: mutex acquired here
ct_lock_lock(&ctb->lock);
^
lib/conntrack.c:2575:9: error: releasing mutex '->buckets[i].lock' that was not
  held [-Werror,-Wthread-safety-analysis]
ct_lock_unlock(&ct->buckets[i].lock);
^
2 errors generated.
make[2]: *** [lib/conntrack.lo] Error 1

https://travis-ci.org/openvswitch/ovs/builds/528984734


Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [branch-2.9 3/3] conntrack: Replace structure copy by memcpy().

[Ben Pfaff]

On Thu, May 02, 2019 at 05:29:16PM -0700, Darrell Ball wrote:
> There are a few cases where structure copy can be replaced by
> memcpy(), for possible portability benefit.  This is because
> the structures involved have padding and elements of the
> structure are used to generate hashes.
> 
> Signed-off-by: Darrell Ball 

Thanks for the backports.  I applied them to branch-2.9.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [branch-2.9 3/3] conntrack: Replace structure copy by memcpy().

[Darrell Ball]

There are a few cases where structure copy can be replaced by
memcpy(), for possible portability benefit.  This is because
the structures involved have padding and elements of the
structure are used to generate hashes.

Signed-off-by: Darrell Ball 
---
 lib/conntrack.c | 21 -
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/lib/conntrack.c b/lib/conntrack.c
index 042b94e..0d71195 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -934,7 +934,7 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
 nc = &connl;
 memset(nc, 0, sizeof *nc);
 memcpy(&nc->key, &ctx->key, sizeof nc->key);
-nc->rev_key = nc->key;
+memcpy(&nc->rev_key, &nc->key, sizeof nc->rev_key);
 conn_key_reverse(&nc->rev_key);
 
 if (ct_verify_helper(helper, ct_alg_ctl)) {
@@ -985,7 +985,7 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
 
 /* Update nc with nat adjustments made to
  * conn_for_un_nat_copy by nat_select_range_tuple(). */
-*nc = *conn_for_un_nat_copy;
+memcpy(nc, conn_for_un_nat_copy, sizeof *nc);
 }
 conn_for_un_nat_copy->conn_type = CT_CONN_TYPE_UN_NAT;
 conn_for_un_nat_copy->nat_info = NULL;
@@ -1076,8 +1076,8 @@ create_un_nat_conn(struct conntrack *ct, struct conn 
*conn_for_un_nat_copy,
long long now, bool alg_un_nat)
 {
 struct conn *nc = xmemdup(conn_for_un_nat_copy, sizeof *nc);
-nc->key = conn_for_un_nat_copy->rev_key;
-nc->rev_key = conn_for_un_nat_copy->key;
+memcpy(&nc->key, &conn_for_un_nat_copy->rev_key, sizeof nc->key);
+memcpy(&nc->rev_key, &conn_for_un_nat_copy->key, sizeof nc->rev_key);
 uint32_t un_nat_hash = conn_key_hash(&nc->key, ct->hash_basis);
 unsigned un_nat_conn_bucket = hash_to_bucket(un_nat_hash);
 ct_lock_lock(&ct->buckets[un_nat_conn_bucket].lock);
@@ -1266,7 +1266,7 @@ process_one(struct conntrack *ct, struct dp_packet *pkt,
 
 struct conn_lookup_ctx ctx2;
 ctx2.conn = NULL;
-ctx2.key = conn->rev_key;
+memcpy(&ctx2.key, &conn->rev_key, sizeof ctx2.key);
 ctx2.hash = conn_key_hash(&conn->rev_key, ct->hash_basis);
 
 ct_lock_unlock(&ct->buckets[bucket].lock);
@@ -1352,7 +1352,7 @@ process_one(struct conntrack *ct, struct dp_packet *pkt,
 
 struct conn conn_for_expectation;
 if (OVS_UNLIKELY((ct_alg_ctl != CT_ALG_CTL_NONE) && conn)) {
-conn_for_expectation = *conn;
+memcpy(&conn_for_expectation, conn, sizeof conn_for_expectation);
 }
 
 ct_lock_unlock(&ct->buckets[bucket].lock);
@@ -2334,8 +2334,10 @@ nat_conn_keys_insert(struct hmap *nat_conn_keys, const 
struct conn *nat_conn,
 
 if (!nat_conn_key_node) {
 struct nat_conn_key_node *nat_conn_key = xzalloc(sizeof *nat_conn_key);
-nat_conn_key->key = nat_conn->rev_key;
-nat_conn_key->value = nat_conn->key;
+memcpy(&nat_conn_key->key, &nat_conn->rev_key,
+   sizeof nat_conn_key->key);
+memcpy(&nat_conn_key->value, &nat_conn->key,
+   sizeof nat_conn_key->value);
 hmap_insert(nat_conn_keys, &nat_conn_key->node,
 conn_key_hash(&nat_conn_key->key, basis));
 return true;
@@ -2741,7 +2743,8 @@ expectation_create(struct conntrack *ct, ovs_be16 
dst_port,
 alg_exp_node->key.dst.port = dst_port;
 alg_exp_node->master_mark = master_conn->mark;
 alg_exp_node->master_label = master_conn->label;
-alg_exp_node->master_key = master_conn->key;
+memcpy(&alg_exp_node->master_key, &master_conn->key,
+   sizeof alg_exp_node->master_key);
 /* Take the write lock here because it is almost 100%
  * likely that the lookup will fail and
  * expectation_create() will be called below. */
-- 
1.9.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev