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

[Darrell Ball]

On Thu, Aug 30, 2018 at 1:13 AM, Zang MingJie <zealot0630 at gmail.com> 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 <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
>