On Thu, Aug 30, 2018 at 1:13 AM, Zang MingJie <[email protected]> wrote:
> I don't think the patch will resolve the problem. Once ctb->lock is > released, other thread may have chance to acquire the lock and modify > ctb. In general, ctb->lock can not be released in this function, > another approach is needed. > It is expected that other threads can acquire the lock - thats intended to be ok. There is an existing comment that implies that already in the function. The function checks whether a race has occurred. Please give the patch a try. If you think there is still an issue, then please provide the stacktrace. Also, if you allude to other issues, please mention them specifically ? Darrell > > On Wed, Aug 29, 2018 at 3:31 PM Darrell Ball <[email protected]> wrote: > > > > nat_clean has a defunct optimization for calculating a hash outside the > > scope of a bucket lock which can lead to a race in referencing a freed > > conntrack entry. Adjust to avoid this. Needs backporting to 2.8. > > > > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2018-August/ > 351629.html > > Fixes: 286de2729955 ("dpdk: Userspace Datapath: Introduce NAT Support.") > > Signed-off-by: Darrell Ball <[email protected]> > > --- > > lib/conntrack.c | 14 ++++++++------ > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > diff --git a/lib/conntrack.c b/lib/conntrack.c > > index be8debb..692f2b8 100644 > > --- a/lib/conntrack.c > > +++ b/lib/conntrack.c > > @@ -778,20 +778,22 @@ nat_clean(struct conntrack *ct, struct conn *conn, > > { > > ct_rwlock_wrlock(&ct->resources_lock); > > nat_conn_keys_remove(&ct->nat_conn_keys, &conn->rev_key, > ct->hash_basis); > > - ct_rwlock_unlock(&ct->resources_lock); > > - ct_lock_unlock(&ctb->lock); > > unsigned bucket_rev_conn = > > hash_to_bucket(conn_key_hash(&conn->rev_key, ct->hash_basis)); > > + struct conn_key rev_key = conn->rev_key; > > + ct_rwlock_unlock(&ct->resources_lock); > > + ct_lock_unlock(&ctb->lock); > > + > > ct_lock_lock(&ct->buckets[bucket_rev_conn].lock); > > ct_rwlock_wrlock(&ct->resources_lock); > > long long now = time_msec(); > > - struct conn *rev_conn = conn_lookup(ct, &conn->rev_key, now); > > + struct conn *rev_conn = conn_lookup(ct, &rev_key, now); > > struct nat_conn_key_node *nat_conn_key_node = > > - nat_conn_keys_lookup(&ct->nat_conn_keys, &conn->rev_key, > > + nat_conn_keys_lookup(&ct->nat_conn_keys, &rev_key, > > ct->hash_basis); > > > > - /* In the unlikely event, rev conn was recreated, then skip > > - * rev_conn cleanup. */ > > + /* In the unlikely event, 'rev_conn' was recreated, then skip > > + * 'rev_conn' cleanup. */ > > if (rev_conn && (!nat_conn_key_node || > > conn_key_cmp(&nat_conn_key_node->value, > > &rev_conn->rev_key))) { > > -- > > 1.9.1 > > > > _______________________________________________ > > dev mailing list > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
