nfnetlink_queue crashes kernel

[Florian Westphal <fw@strlen.de>]

Hello,

nfnetlink_queue is currently broken in at least two ways.
1). kernel will crash under pressure because
    struct nf_queue_entry is freed via kfree, but parallel
    cpu can still observe this while walking the (global) rhashtable.

2) a4400a5b343d ("netfilter: nfnetlink_queue: nfqnl_instance GFP_ATOMIC -> GFP_KERNEL_ACCOUNT allocation") should have updated the spinlock to use the _bh variant, if the queue exists we risk deadlock via softirq recursion.

Minimal fix, that I am not a fan of:

diff --git a/include/net/netfilter/nf_queue.h b/include/net/netfilter/nf_queue.h
--- a/include/net/netfilter/nf_queue.h
+++ b/include/net/netfilter/nf_queue.h
@@ -24,6 +24,7 @@ struct nf_queue_entry {
 	bool			nf_ct_is_unconfirmed;
 	u16			size; /* sizeof(entry) + saved route keys */
 	u16			queue_num;
+	struct rcu_head		head;
 
 	/* extra space to store route keys */
 };
diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
index 7f12e56e6e52..385d1fe704ae 100644
--- a/net/netfilter/nf_queue.c
+++ b/net/netfilter/nf_queue.c
@@ -74,7 +74,7 @@ static void nf_queue_entry_release_refs(struct nf_queue_entry *entry)
 void nf_queue_entry_free(struct nf_queue_entry *entry)
 {
 	nf_queue_entry_release_refs(entry);
-	kfree(entry);
+	kfree_rcu(entry, head);
 }
 EXPORT_SYMBOL_GPL(nf_queue_entry_free);
 
diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index 47f7f62906e2..fc44ea4e5128 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -190,7 +190,7 @@ instance_create(struct nfnl_queue_net *q, u_int16_t queue_num, u32 portid)
 	spin_lock_init(&inst->lock);
 	INIT_LIST_HEAD(&inst->queue_list);
 
-	spin_lock(&q->instances_lock);
+	spin_lock_bh(&q->instances_lock);
 	if (instance_lookup(q, queue_num)) {
 		err = -EEXIST;
 		goto out_unlock;
@@ -204,7 +204,7 @@ instance_create(struct nfnl_queue_net *q, u_int16_t queue_num, u32 portid)
 	h = instance_hashfn(queue_num);
 	hlist_add_head_rcu(&inst->hlist, &q->instance_table[h]);
 
-	spin_unlock(&q->instances_lock);
+	spin_unlock_bh(&q->instances_lock);
 
 	return inst;
 



A probably better fix is to make the rhashtable perqueue, which is
much more intrusive at this late stage.

I prefer to revert both changes and not accept a reworked hashtable
until we have an extension to nft_queue.sh selftest.

Anything using queue balancing (pacing packets to n queues) with
enough streams to have parallel invocation of the rhashtable should
demonstrate the crash.

Thoughts?