| Message ID | 1556843356-46485-3-git-send-email-dlu998@gmail.com |
|---|---|
| State | Accepted |
| Commit | edb1e021dff43b0ca0af1724a1c95e4e517c9b2f |
| Headers | show |
| Series | [ovs-dev,branch-2.9,1/3] conntrack: Fix race for NAT cleanup. | expand |
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 <dlu998@gmail.com> Thanks for the backports. I applied them to branch-2.9.
> 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 <dlu998 at 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: 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.
On Wed, May 8, 2019 at 1:41 AM Ilya Maximets <i.maximets@samsung.com> 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 <dlu998 at 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 ? > > 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 08.05.2019 12:20, Darrell Ball wrote: > > > On Wed, May 8, 2019 at 1:41 AM Ilya Maximets <i.maximets@samsung.com <mailto:i.maximets@samsung.com>> 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 <dlu998 at gmail.com <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.
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 <dlu998 at 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: > > 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.
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. */
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 <dlu998@gmail.com> --- lib/conntrack.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)