On 3/8/2020 10:11 AM, Paul Blakey wrote:

iirc I did the spin lock bh because we can be called from queue work rcu handler , so I wanted to disable soft irq.

I got a possible deadlock splat for that.

Here I meant this call rcu:

static void tcf_ct_cleanup(struct tc_action *a)
{

-------struct tcf_ct_params *params;
-------struct tcf_ct *c = to_ct(a);

-------params = rcu_dereference_protected(c->params, 1);
-------if (params)
------->-------call_rcu(&params->rcu, tcf_ct_params_free);

}

static void tcf_ct_params_free(struct rcu_head *head)
{

-------struct tcf_ct_params *params = container_of(head,
------->------->------->------->------->-------    struct tcf_ct_params, rcu);

-------tcf_ct_flow_table_put(params);

...

On 3/7/2020 10:53 PM, Eric Dumazet wrote:

quoted

On 3/7/20 12:12 PM, Eric Dumazet wrote:

quoted

On 3/3/20 7:57 AM, Paul Blakey wrote:

quoted

Use the NF flow tables infrastructure for CT offload.

Create a nf flow table per zone.

Next patches will add FT entries to this table, and do
the software offload.

Signed-off-by: Paul Blakey <redacted>
Reviewed-by: Jiri Pirko <redacted>
---
  v4->v5:
    Added reviewed by Jiri, thanks!
  v3->v4:
    Alloc GFP_ATOMIC
  v2->v3:
    Ditch re-locking to alloc, and use atomic allocation
  v1->v2:
    Use spin_lock_bh instead of spin_lock, and unlock for alloc (as it can sleep)
    Free ft on last tc act instance instead of last instance + last offloaded tuple,
    this removes cleanup cb and netfilter patches, and is simpler
    Removed accidental mlx5/core/en_tc.c change
    Removed reviewed by Jiri - patch changed

+	err = nf_flow_table_init(&ct_ft->nf_ft);

This call is going to allocate a rhashtable (GFP_KERNEL allocations that might sleep)

Since you still hold zones_lock spinlock, a splat should occur.

"BUG: sleeping function called from invalid context in  ..."

DEBUG_ATOMIC_SLEEP=y is your friend.

And it is always a good thing to make sure a patch does not trigger a lockdep splat

CONFIG_PROVE_LOCKING=y

Also abusing a spinlock and GFP_ATOMIC allocations in control path is highly discouraged.

I can not test the following fix, any objections before I submit this officially ?

diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
index 23eba61f0f819212a3522c3c63b938d0b8d997e2..3d9e678d7d5336f1746035745b091bea0dcb5fdd 100644
--- a/net/sched/act_ct.c
+++ b/net/sched/act_ct.c

@@ -35,15 +35,15 @@
 
 static struct workqueue_struct *act_ct_wq;
 static struct rhashtable zones_ht;
-static DEFINE_SPINLOCK(zones_lock);
+static DEFINE_MUTEX(zones_mutex);
 
 struct tcf_ct_flow_table {
        struct rhash_head node; /* In zones tables */
 
        struct rcu_work rwork;
        struct nf_flowtable nf_ft;
+       refcount_t ref;
        u16 zone;
-       u32 ref;
 
        bool dying;
 };

@@ -64,14 +64,15 @@ static int tcf_ct_flow_table_get(struct tcf_ct_params *params)
        struct tcf_ct_flow_table *ct_ft;
        int err = -ENOMEM;
 
-       spin_lock_bh(&zones_lock);
+       mutex_lock(&zones_mutex);
        ct_ft = rhashtable_lookup_fast(&zones_ht, &params->zone, zones_params);
-       if (ct_ft)
-               goto take_ref;
+       if (ct_ft && refcount_inc_not_zero(&ct_ft->ref))
+               goto out_unlock;
 
-       ct_ft = kzalloc(sizeof(*ct_ft), GFP_ATOMIC);
+       ct_ft = kzalloc(sizeof(*ct_ft), GFP_KERNEL);
        if (!ct_ft)
                goto err_alloc;
+       refcount_set(&ct_ft->ref, 1);
 
        ct_ft->zone = params->zone;
        err = rhashtable_insert_fast(&zones_ht, &ct_ft->node, zones_params);

@@ -84,10 +85,9 @@ static int tcf_ct_flow_table_get(struct tcf_ct_params *params)
                goto err_init;
 
        __module_get(THIS_MODULE);
-take_ref:
+out_unlock:
        params->ct_ft = ct_ft;
-       ct_ft->ref++;
-       spin_unlock_bh(&zones_lock);
+       mutex_unlock(&zones_mutex);
 
        return 0;
 

@@ -96,7 +96,7 @@ static int tcf_ct_flow_table_get(struct tcf_ct_params *params)
 err_insert:
        kfree(ct_ft);
 err_alloc:
-       spin_unlock_bh(&zones_lock);
+       mutex_unlock(&zones_mutex);
        return err;
 }
 

@@ -116,13 +116,11 @@ static void tcf_ct_flow_table_put(struct tcf_ct_params *params)
 {
        struct tcf_ct_flow_table *ct_ft = params->ct_ft;
 
-       spin_lock_bh(&zones_lock);
-       if (--params->ct_ft->ref == 0) {
+       if (refcount_dec_and_test(&params->ct_ft->ref)) {
                rhashtable_remove_fast(&zones_ht, &ct_ft->node, zones_params);
                INIT_RCU_WORK(&ct_ft->rwork, tcf_ct_flow_table_cleanup_work);
                queue_rcu_work(act_ct_wq, &ct_ft->rwork);
        }
-       spin_unlock_bh(&zones_lock);
 }
 
 static void tcf_ct_flow_table_add(struct tcf_ct_flow_table *ct_ft,