[ovs-dev] [patch v2 1/2] conntrack: Fix nat_clean.

[Zang MingJie]

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.

On Wed, Aug 29, 2018 at 3:31 PM Darrell Ball <dlu998 at gmail.com> 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 <dlu998 at gmail.com>
> ---
>  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
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev