[PATCH net 0/2] small tc conntrack fixes

[PATCH net 0/2] small tc conntrack fixes

[Simon Horman]

Louis Peens says:

The first patch makes sure that any callbacks registered to
the ct->nf_tables table are cleaned up before freeing.

The second patch removes what was effectively a workaround
in the nfp driver because of the missing cleanup in patch 1.

Louis Peens (2):
  net/sched: act_ct: remove and free nf_table callbacks
  nfp: flower-ct: remove callback delete deadlock

 .../net/ethernet/netronome/nfp/flower/conntrack.c   | 13 -------------
 net/sched/act_ct.c                                  | 11 +++++++++++
 2 files changed, 11 insertions(+), 13 deletions(-)

-- 
2.20.1

[PATCH net 1/2] net/sched: act_ct: remove and free nf_table callbacks

[Simon Horman]

From: Louis Peens <louis.peens@corigine.com>

When cleaning up the nf_table in tcf_ct_flow_table_cleanup_work
there is no guarantee that the callback list, added to by
nf_flow_table_offload_add_cb, is empty. This means that it is
possible that the flow_block_cb memory allocated will be lost.

Fix this by iterating the list and freeing the flow_block_cb entries
before freeing the nf_table entry (via freeing ct_ft).

Fixes: 978703f42549 ("netfilter: flowtable: Add API for registering to flow table events")
Signed-off-by: Louis Peens <louis.peens@corigine.com>
Signed-off-by: Yinjun Zhang <yinjun.zhang@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
 net/sched/act_ct.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
index a656baa321fe..7ec443ca48d6 100644
--- a/net/sched/act_ct.c
+++ b/net/sched/act_ct.c
@@ -322,11 +322,22 @@ static int tcf_ct_flow_table_get(struct tcf_ct_params *params)
 
 static void tcf_ct_flow_table_cleanup_work(struct work_struct *work)
 {
+	struct flow_block_cb *block_cb, *tmp_cb;
 	struct tcf_ct_flow_table *ct_ft;
+	struct flow_block *block;
 
 	ct_ft = container_of(to_rcu_work(work), struct tcf_ct_flow_table,
 			     rwork);
 	nf_flow_table_free(&ct_ft->nf_ft);
+
+	/* Remove any remaining callbacks before cleanup */
+	block = &ct_ft->nf_ft.flow_block;
+	down_write(&ct_ft->nf_ft.flow_block_lock);
+	list_for_each_entry_safe(block_cb, tmp_cb, &block->cb_list, list) {
+		list_del(&block_cb->list);
+		flow_block_cb_free(block_cb);
+	}
+	up_write(&ct_ft->nf_ft.flow_block_lock);
 	kfree(ct_ft);
 
 	module_put(THIS_MODULE);
-- 
2.20.1

[PATCH net 2/2] nfp: flower-ct: remove callback delete deadlock

[Simon Horman]

From: Louis Peens <louis.peens@corigine.com>

The current location of the callback delete can lead to a race
condition where deleting the callback requires a write_lock on
the nf_table, but at the same time another thread from netfilter
could have taken a read lock on the table before trying to offload.
Since the driver is taking a rtnl_lock this can lead into a deadlock
situation, where the netfilter offload will wait for the cls_flower
rtnl_lock to be released, but this cannot happen since this is
waiting for the nf_table read_lock to be released before it can
delete the callback.

Solve this by completely removing the nf_flow_table_offload_del_cb
call, as this will now be cleaned up by act_ct itself when cleaning
up the specific nf_table.

Fixes: 62268e78145f ("nfp: flower-ct: add nft callback stubs")
Signed-off-by: Louis Peens <louis.peens@corigine.com>
Signed-off-by: Yinjun Zhang <yinjun.zhang@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
 .../net/ethernet/netronome/nfp/flower/conntrack.c   | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/flower/conntrack.c b/drivers/net/ethernet/netronome/nfp/flower/conntrack.c
index 273d529d43c2..128020b1573e 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/conntrack.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/conntrack.c
@@ -1141,20 +1141,7 @@ int nfp_fl_ct_del_flow(struct nfp_fl_ct_map_entry *ct_map_ent)
 		nfp_fl_ct_clean_flow_entry(ct_entry);
 		kfree(ct_map_ent);
 
-		/* If this is the last pre_ct_rule it means that it is
-		 * very likely that the nft table will be cleaned up next,
-		 * as this happens on the removal of the last act_ct flow.
-		 * However we cannot deregister the callback on the removal
-		 * of the last nft flow as this runs into a deadlock situation.
-		 * So deregister the callback on removal of the last pre_ct flow
-		 * and remove any remaining nft flow entries. We also cannot
-		 * save this state and delete the callback later since the
-		 * nft table would already have been freed at that time.
-		 */
 		if (!zt->pre_ct_count) {
-			nf_flow_table_offload_del_cb(zt->nft,
-						     nfp_fl_ct_handle_nft_flow,
-						     zt);
 			zt->nft = NULL;
 			nfp_fl_ct_clean_nft_entries(zt);
 		}
-- 
2.20.1

Re: [PATCH net 0/2] small tc conntrack fixes

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (refs/heads/master):

On Fri,  2 Jul 2021 11:21:37 +0200 you wrote:
> Louis Peens says:
> 
> The first patch makes sure that any callbacks registered to
> the ct->nf_tables table are cleaned up before freeing.
> 
> The second patch removes what was effectively a workaround
> in the nfp driver because of the missing cleanup in patch 1.
> 
> [...]

Here is the summary with links:
  - [net,1/2] net/sched: act_ct: remove and free nf_table callbacks
    https://git.kernel.org/netdev/net/c/77ac5e40c44e
  - [net,2/2] nfp: flower-ct: remove callback delete deadlock
    https://git.kernel.org/netdev/net/c/7cc93d888df7

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html