netfilter: ctnetlink: fix (really) race condition between dump_table and destroy

PATCH In review Pablo Neira Ayuso 4 participants 17 messages Jan 23, 2011 – Feb 20, 2011

Subsystems (2)

Networking netfilter

lore

Share & tools

lore

Pablo Neira Ayuso

author maintainer

2011-01-23 23:16 UTC #0 lore raw

PATCH 1/1 netfilter: ctnetlink: fix (really) race condition between dump_table and destroy

In 13ee6ac579574a2a95e982b19920fd2495dce8cd, we recovered spinlocks to protect the dump of the conntrack table according to reports from Stephen and acknowledgments on the issue from Eric.

However, Stephen removed the refcount bump in that patch that allows to keep a reference to the current ct object we are interating over. That code avoids race conditions between ct object destruction and the iteration itself. This patch reintroduces these lines since the ct object may vanish between two recvmgs() invocations.

This patch fixes ocasional crashes while dumping the conntrack table intensively.

Cc Stephen Hemminger <[email protected]>

Signed-off-by Pablo Neira Ayuso <[email protected]>

---
 net/netfilter/nf_conntrack_netlink.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

net/netfilter/nf_conntrack_netlink.c +6 -2

index 93297aa..519e245 100644
@@ -654,14 +654,16 @@ restart:
 			if (NF_CT_DIRECTION(h) != IP_CT_DIR_ORIGINAL)
 				continue;
 			ct = nf_ct_tuplehash_to_ctrack(h);
+			if (!atomic_inc_not_zero(&ct->ct_general.use))
+				continue;
 			/* Dump entries of a given L3 protocol number.
 			 * If it is not specified, ie. l3proto == 0,
 			 * then dump everything. */
 			if (l3proto && nf_ct_l3num(ct) != l3proto)
-				continue;
+				goto releasect;
 			if (cb->args[1]) {
 				if (ct != last)
-					continue;
+					goto releasect;
 				cb->args[1] = 0;
 			}
 			if (ctnetlink_fill_info(skb, NETLINK_CB(cb->skb).pid,

@@ -679,6 +681,8 @@ restart:
 				if (acct)
 					memset(acct, 0, sizeof(struct nf_conn_counter[IP_CT_DIR_MAX]));
 			}
+releasect:
+		nf_ct_put(ct);
 		}
 		if (cb->args[1]) {
 			cb->args[1] = 0;

Eric Dumazet

2011-01-24 06:41 UTC #1 lore raw

Quoted 4 lines · Pablo Neira Ayuso

In 13ee6ac579574a2a95e982b19920fd2495dce8cd, we recovered spinlocks to protect the dump of the conntrack table according to reports from Stephen and acknowledgments on the issue from Eric.

When a commit is referred, always give its title, and you can use a short id (12 digits are OK)

In 13ee6ac57957 (netfilter: fix race in conntrack between dump_table and destroy)

Quoted 10 lines

However, Stephen removed the refcount bump in that patch that allows to keep a reference to the current ct object we are interating over. That code avoids race conditions between ct object destruction and the iteration itself. This patch reintroduces these lines since the ct object may vanish between two recvmgs() invocations.
This patch fixes ocasional crashes while dumping the conntrack table intensively.
Cc Stephen Hemminger <[email protected]>

Thats not true, Stephen was not in CC on your mail

I add him right now.

Quoted 30 lines

Signed-off-by Pablo Neira Ayuso <[email protected]>

---
 net/netfilter/nf_conntrack_netlink.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

net/netfilter/nf_conntrack_netlink.c +4 -2

index 93297aa..519e245 100644
@@ -654,14 +654,16 @@ restart:
 			if (NF_CT_DIRECTION(h) != IP_CT_DIR_ORIGINAL)
 				continue;
 			ct = nf_ct_tuplehash_to_ctrack(h);
+			if (!atomic_inc_not_zero(&ct->ct_general.use))
+				continue;
 			/* Dump entries of a given L3 protocol number.
 			 * If it is not specified, ie. l3proto == 0,
 			 * then dump everything. */
 			if (l3proto && nf_ct_l3num(ct) != l3proto)
-				continue;
+				goto releasect;
 			if (cb->args[1]) {
 				if (ct != last)
-					continue;
+					goto releasect;
 				cb->args[1] = 0;
 			}
 			if (ctnetlink_fill_info(skb, NETLINK_CB(cb->skb).pid,

net/netfilter/nf_conntrack_netlink.c restart: · line 688

Quoted 8 lines

net/netfilter/nf_conntrack_netlink.c +2 -0

@@ -679,6 +681,8 @@ restart:
 				if (acct)
 					memset(acct, 0, sizeof(struct nf_conn_counter[IP_CT_DIR_MAX]));
 			}
+releasect:
+		nf_ct_put(ct);
 		}
 		if (cb->args[1]) {
 			cb->args[1] = 0;

Hmm...

Only the RCU lookup should need this extra check (atomic_inc_not_zero), not a writer. (And a dumper is a "writer" since it holds nf_conntrack spinlock)

In Stephen commit, we switched from RCU lookup to traditional one (spinlock protected), so, we should not need to touch individual objects refcount ?

I feel the right fix is not to increment refcount then decrement it.

Only to _test_ it being zero, and not dumping the ct, eventually ?

net/netfilter/nf_conntrack_netlink.c +2 -0

index 93297aa..a977cc7 100644
@@ -654,6 +654,8 @@ restart:
 			if (NF_CT_DIRECTION(h) != IP_CT_DIR_ORIGINAL)
 				continue;
 			ct = nf_ct_tuplehash_to_ctrack(h);
+			if (!atomic_read(&ct->ct_general.use))
+				continue;
 			/* Dump entries of a given L3 protocol number.
 			 * If it is not specified, ie. l3proto == 0,
 			 * then dump everything. */

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Eric Dumazet

2011-01-24 07:04 UTC #2 lore raw

Quoted 88 lines · Eric Dumazet

Quoted 4 lines · Pablo Neira Ayuso
In 13ee6ac579574a2a95e982b19920fd2495dce8cd, we recovered spinlocks to protect the dump of the conntrack table according to reports from Stephen and acknowledgments on the issue from Eric.
When a commit is referred, always give its title, and you can use a short id (12 digits are OK)
In 13ee6ac57957 (netfilter: fix race in conntrack between dump_table and destroy)
Quoted 10 lines
However, Stephen removed the refcount bump in that patch that allows to keep a reference to the current ct object we are interating over. That code avoids race conditions between ct object destruction and the iteration itself. This patch reintroduces these lines since the ct object may vanish between two recvmgs() invocations.
This patch fixes ocasional crashes while dumping the conntrack table intensively.
Cc Stephen Hemminger <[email protected]>
Thats not true, Stephen was not in CC on your mail
I add him right now.
Quoted 30 lines
Signed-off-by Pablo Neira Ayuso <[email protected]>
---
 net/netfilter/nf_conntrack_netlink.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)
net/netfilter/nf_conntrack_netlink.c +4 -2
index 93297aa..519e245 100644
@@ -654,14 +654,16 @@ restart:
 			if (NF_CT_DIRECTION(h) != IP_CT_DIR_ORIGINAL)
 				continue;
 			ct = nf_ct_tuplehash_to_ctrack(h);
+			if (!atomic_inc_not_zero(&ct->ct_general.use))
+				continue;
 			/* Dump entries of a given L3 protocol number.
 			 * If it is not specified, ie. l3proto == 0,
 			 * then dump everything. */
 			if (l3proto && nf_ct_l3num(ct) != l3proto)
-				continue;
+				goto releasect;
 			if (cb->args[1]) {
 				if (ct != last)
-					continue;
+					goto releasect;
 				cb->args[1] = 0;
 			}
 			if (ctnetlink_fill_info(skb, NETLINK_CB(cb->skb).pid,
net/netfilter/nf_conntrack_netlink.c restart: · line 688
Quoted 8 lines
net/netfilter/nf_conntrack_netlink.c +2 -0
@@ -679,6 +681,8 @@ restart:
 				if (acct)
 					memset(acct, 0, sizeof(struct nf_conn_counter[IP_CT_DIR_MAX]));
 			}
+releasect:
+		nf_ct_put(ct);
 		}
 		if (cb->args[1]) {
 			cb->args[1] = 0;
Hmm...
Only the RCU lookup should need this extra check (atomic_inc_not_zero), not a writer. (And a dumper is a "writer" since it holds nf_conntrack spinlock)
In Stephen commit, we switched from RCU lookup to traditional one (spinlock protected), so, we should not need to touch individual objects refcount ?
I feel the right fix is not to increment refcount then decrement it.
Only to _test_ it being zero, and not dumping the ct, eventually ?
net/netfilter/nf_conntrack_netlink.c +0 -0
index 93297aa..a977cc7 100644

net/netfilter/nf_conntrack_netlink.c restart: · line 661

Quoted 8 lines

net/netfilter/nf_conntrack_netlink.c +2 -0

@@ -654,6 +654,8 @@ restart:
 			if (NF_CT_DIRECTION(h) != IP_CT_DIR_ORIGINAL)
 				continue;
 			ct = nf_ct_tuplehash_to_ctrack(h);
+			if (!atomic_read(&ct->ct_general.use))
+				continue;
 			/* Dump entries of a given L3 protocol number.
 			 * If it is not specified, ie. l3proto == 0,
 			 * then dump everything. */

I read ea781f197d6a (netfilter: nf_conntrack: use SLAB_DESTROY_BY_RCU and get rid of call_rcu()) and can see previous code only had the spinlock, not the refcount inc/dec.

Are you saying that my commit included a bug fix ? ;)

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Pablo Neira Ayuso

author maintainer

2011-01-24 11:57 UTC #3 lore raw

In reply to Eric Dumazet

Quoted 7 lines · Eric Dumazet

Quoted 3 lines · Pablo Neira Ayuso
In 13ee6ac579574a2a95e982b19920fd2495dce8cd, we recovered spinlocks to protect the dump of the conntrack table according to reports from Stephen and acknowledgments on the issue from Eric.
When a commit is referred, always give its title, and you can use a short id (12 digits are OK)

I'm using the id that git log --oneline shows, is it OK?

Quoted 9 lines

In 13ee6ac57957 (netfilter: fix race in conntrack between dump_table and destroy)
Quoted 6 lines
However, Stephen removed the refcount bump in that patch that allows to keep a reference to the current ct object we are interating over. That code avoids race conditions between ct object destruction and the iteration itself. This patch reintroduces these lines since the ct object may vanish between two recvmgs() invocations.

Quoted 6 lines

Quoted 4 lines
This patch fixes ocasional crashes while dumping the conntrack table intensively.
Cc Stephen Hemminger <[email protected]>
Thats not true, Stephen was not in CC on your mail

I'm sorry, I forgot to add --cc to stgit.

Quoted 62 lines

I add him right now.
Quoted 30 lines
Signed-off-by Pablo Neira Ayuso <[email protected]>
---
 net/netfilter/nf_conntrack_netlink.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)
net/netfilter/nf_conntrack_netlink.c +4 -2
index 93297aa..519e245 100644
@@ -654,14 +654,16 @@ restart:
 			if (NF_CT_DIRECTION(h) != IP_CT_DIR_ORIGINAL)
 				continue;
 			ct = nf_ct_tuplehash_to_ctrack(h);
+			if (!atomic_inc_not_zero(&ct->ct_general.use))
+				continue;
 			/* Dump entries of a given L3 protocol number.
 			 * If it is not specified, ie. l3proto == 0,
 			 * then dump everything. */
 			if (l3proto && nf_ct_l3num(ct) != l3proto)
-				continue;
+				goto releasect;
 			if (cb->args[1]) {
 				if (ct != last)
-					continue;
+					goto releasect;
 				cb->args[1] = 0;
 			}
 			if (ctnetlink_fill_info(skb, NETLINK_CB(cb->skb).pid,
net/netfilter/nf_conntrack_netlink.c restart: · line 688
Quoted 8 lines
net/netfilter/nf_conntrack_netlink.c +2 -0
@@ -679,6 +681,8 @@ restart:
 				if (acct)
 					memset(acct, 0, sizeof(struct nf_conn_counter[IP_CT_DIR_MAX]));
 			}
+releasect:
+		nf_ct_put(ct);
 		}
 		if (cb->args[1]) {
 			cb->args[1] = 0;
Hmm...
Only the RCU lookup should need this extra check (atomic_inc_not_zero), not a writer. (And a dumper is a "writer" since it holds nf_conntrack spinlock)
In Stephen commit, we switched from RCU lookup to traditional one (spinlock protected), so, we should not need to touch individual objects refcount ?
I feel the right fix is not to increment refcount then decrement it.
Only to _test_ it being zero, and not dumping the ct, eventually ?
net/netfilter/nf_conntrack_netlink.c +0 -0
index 93297aa..a977cc7 100644

net/netfilter/nf_conntrack_netlink.c restart: · line 661

Quoted 8 lines

net/netfilter/nf_conntrack_netlink.c +2 -0

@@ -654,6 +654,8 @@ restart:
 			if (NF_CT_DIRECTION(h) != IP_CT_DIR_ORIGINAL)
 				continue;
 			ct = nf_ct_tuplehash_to_ctrack(h);
+			if (!atomic_read(&ct->ct_general.use))
+				continue;
 			/* Dump entries of a given L3 protocol number.
 			 * If it is not specified, ie. l3proto == 0,
 			 * then dump everything. */

No, that won't work.

Sorry, I didn't explain the problem appropriately. We still do a nf_ct_put() for last ct. In that patch, you forgot to add the refcount bump that we need to keep the reference to the object.

The following patch attached is smaller, it also fixes the problem and we don't have to increase the refcount for each object. Now it looks similar to how it was before the RCU patches.

netfilter: ctnetlink: fix missing refcount increment during dumps

From: Pablo Neira Ayuso <[email protected]>

In 13ee6ac netfilter: fix race in conntrack between dump_table and destroy, we recovered spinlocks to protect the dump of the conntrack table according to reports from Stephen and acknowledgments on the issue from Eric.

In that patch, the refcount bump that allows to keep a reference to the current ct object was removed. However, we still decrement the refcount for that object in the output path of ctnetlink_dump_table():

        if (last)
                nf_ct_put(last)

Cc Stephen Hemminger <[email protected]>

Cc Eric Dumazet <[email protected]>

Signed-off-by Pablo Neira Ayuso <[email protected]>

---
 net/netfilter/nf_conntrack_netlink.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

net/netfilter/nf_conntrack_netlink.c +2 -0

index 93297aa..6dd9296 100644
@@ -667,6 +667,8 @@ restart:
 			if (ctnetlink_fill_info(skb, NETLINK_CB(cb->skb).pid,
 						cb->nlh->nlmsg_seq,
 						IPCTNL_MSG_CT_NEW, ct) < 0) {
+				if (!atomic_inc_not_zero(&ct->ct_general.use))
+					continue;
 				cb->args[1] = (unsigned long)ct;
 				goto out;
 			}

Patrick McHardy

2011-01-24 12:46 UTC #4 lore raw

Quoted 102 lines · Pablo Neira Ayuso

Quoted 7 lines · Eric Dumazet
Quoted 3 lines · Pablo Neira Ayuso
In 13ee6ac579574a2a95e982b19920fd2495dce8cd, we recovered spinlocks to protect the dump of the conntrack table according to reports from Stephen and acknowledgments on the issue from Eric.
When a commit is referred, always give its title, and you can use a short id (12 digits are OK)
I'm using the id that git log --oneline shows, is it OK?
Quoted 9 lines
In 13ee6ac57957 (netfilter: fix race in conntrack between dump_table and destroy)
Quoted 6 lines
However, Stephen removed the refcount bump in that patch that allows to keep a reference to the current ct object we are interating over. That code avoids race conditions between ct object destruction and the iteration itself. This patch reintroduces these lines since the ct object may vanish between two recvmgs() invocations.
Quoted 6 lines
Quoted 4 lines
This patch fixes ocasional crashes while dumping the conntrack table intensively.
Cc Stephen Hemminger <[email protected]>
Thats not true, Stephen was not in CC on your mail
I'm sorry, I forgot to add --cc to stgit.
Quoted 62 lines
I add him right now.
Quoted 30 lines
Signed-off-by Pablo Neira Ayuso <[email protected]>
---
 net/netfilter/nf_conntrack_netlink.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)
net/netfilter/nf_conntrack_netlink.c +4 -2
index 93297aa..519e245 100644
@@ -654,14 +654,16 @@ restart:
 			if (NF_CT_DIRECTION(h) != IP_CT_DIR_ORIGINAL)
 				continue;
 			ct = nf_ct_tuplehash_to_ctrack(h);
+			if (!atomic_inc_not_zero(&ct->ct_general.use))
+				continue;
 			/* Dump entries of a given L3 protocol number.
 			 * If it is not specified, ie. l3proto == 0,
 			 * then dump everything. */
 			if (l3proto && nf_ct_l3num(ct) != l3proto)
-				continue;
+				goto releasect;
 			if (cb->args[1]) {
 				if (ct != last)
-					continue;
+					goto releasect;
 				cb->args[1] = 0;
 			}
 			if (ctnetlink_fill_info(skb, NETLINK_CB(cb->skb).pid,
net/netfilter/nf_conntrack_netlink.c restart: · line 688
Quoted 8 lines
net/netfilter/nf_conntrack_netlink.c +2 -0
@@ -679,6 +681,8 @@ restart:
 				if (acct)
 					memset(acct, 0, sizeof(struct nf_conn_counter[IP_CT_DIR_MAX]));
 			}
+releasect:
+		nf_ct_put(ct);
 		}
 		if (cb->args[1]) {
 			cb->args[1] = 0;
Hmm...
Only the RCU lookup should need this extra check (atomic_inc_not_zero), not a writer. (And a dumper is a "writer" since it holds nf_conntrack spinlock)
In Stephen commit, we switched from RCU lookup to traditional one (spinlock protected), so, we should not need to touch individual objects refcount ?
I feel the right fix is not to increment refcount then decrement it.
Only to _test_ it being zero, and not dumping the ct, eventually ?
net/netfilter/nf_conntrack_netlink.c +0 -0
index 93297aa..a977cc7 100644
net/netfilter/nf_conntrack_netlink.c restart: · line 661
Quoted 8 lines
net/netfilter/nf_conntrack_netlink.c +2 -0
@@ -654,6 +654,8 @@ restart:
 			if (NF_CT_DIRECTION(h) != IP_CT_DIR_ORIGINAL)
 				continue;
 			ct = nf_ct_tuplehash_to_ctrack(h);
+			if (!atomic_read(&ct->ct_general.use))
+				continue;
 			/* Dump entries of a given L3 protocol number.
 			 * If it is not specified, ie. l3proto == 0,
 			 * then dump everything. */
No, that won't work.

Quoted 7 lines

Sorry, I didn't explain the problem appropriately. We still do a nf_ct_put() for last ct. In that patch, you forgot to add the refcount bump that we need to keep the reference to the object.
The following patch attached is smaller, it also fixes the problem and we don't have to increase the refcount for each object. Now it looks similar to how it was before the RCU patches.

This looks correct to me, we need to keep a reference for continuations. Eric, do you agree with this patch?
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Eric Dumazet

2011-01-24 12:54 UTC #5 lore raw

Quoted 107 lines · Patrick McHardy

Quoted 106 lines · Pablo Neira Ayuso
Quoted 7 lines · Eric Dumazet
Quoted 3 lines · Pablo Neira Ayuso
In 13ee6ac579574a2a95e982b19920fd2495dce8cd, we recovered spinlocks to protect the dump of the conntrack table according to reports from Stephen and acknowledgments on the issue from Eric.
When a commit is referred, always give its title, and you can use a short id (12 digits are OK)
I'm using the id that git log --oneline shows, is it OK?
Quoted 9 lines
In 13ee6ac57957 (netfilter: fix race in conntrack between dump_table and destroy)
Quoted 6 lines
However, Stephen removed the refcount bump in that patch that allows to keep a reference to the current ct object we are interating over. That code avoids race conditions between ct object destruction and the iteration itself. This patch reintroduces these lines since the ct object may vanish between two recvmgs() invocations.
Quoted 6 lines
Quoted 4 lines
This patch fixes ocasional crashes while dumping the conntrack table intensively.
Cc Stephen Hemminger <[email protected]>
Thats not true, Stephen was not in CC on your mail
I'm sorry, I forgot to add --cc to stgit.
Quoted 62 lines
I add him right now.
Quoted 30 lines
Signed-off-by Pablo Neira Ayuso <[email protected]>
---
 net/netfilter/nf_conntrack_netlink.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)
net/netfilter/nf_conntrack_netlink.c +4 -2
index 93297aa..519e245 100644
@@ -654,14 +654,16 @@ restart:
 			if (NF_CT_DIRECTION(h) != IP_CT_DIR_ORIGINAL)
 				continue;
 			ct = nf_ct_tuplehash_to_ctrack(h);
+			if (!atomic_inc_not_zero(&ct->ct_general.use))
+				continue;
 			/* Dump entries of a given L3 protocol number.
 			 * If it is not specified, ie. l3proto == 0,
 			 * then dump everything. */
 			if (l3proto && nf_ct_l3num(ct) != l3proto)
-				continue;
+				goto releasect;
 			if (cb->args[1]) {
 				if (ct != last)
-					continue;
+					goto releasect;
 				cb->args[1] = 0;
 			}
 			if (ctnetlink_fill_info(skb, NETLINK_CB(cb->skb).pid,
net/netfilter/nf_conntrack_netlink.c restart: · line 688
Quoted 8 lines
net/netfilter/nf_conntrack_netlink.c +2 -0
@@ -679,6 +681,8 @@ restart:
 				if (acct)
 					memset(acct, 0, sizeof(struct nf_conn_counter[IP_CT_DIR_MAX]));
 			}
+releasect:
+		nf_ct_put(ct);
 		}
 		if (cb->args[1]) {
 			cb->args[1] = 0;
Hmm...
Only the RCU lookup should need this extra check (atomic_inc_not_zero), not a writer. (And a dumper is a "writer" since it holds nf_conntrack spinlock)
In Stephen commit, we switched from RCU lookup to traditional one (spinlock protected), so, we should not need to touch individual objects refcount ?
I feel the right fix is not to increment refcount then decrement it.
Only to _test_ it being zero, and not dumping the ct, eventually ?
net/netfilter/nf_conntrack_netlink.c +0 -0
index 93297aa..a977cc7 100644
net/netfilter/nf_conntrack_netlink.c restart: · line 661
Quoted 8 lines
net/netfilter/nf_conntrack_netlink.c +2 -0
@@ -654,6 +654,8 @@ restart:
 			if (NF_CT_DIRECTION(h) != IP_CT_DIR_ORIGINAL)
 				continue;
 			ct = nf_ct_tuplehash_to_ctrack(h);
+			if (!atomic_read(&ct->ct_general.use))
+				continue;
 			/* Dump entries of a given L3 protocol number.
 			 * If it is not specified, ie. l3proto == 0,
 			 * then dump everything. */
No, that won't work.
Sorry, I didn't explain the problem appropriately. We still do a nf_ct_put() for last ct. In that patch, you forgot to add the refcount bump that we need to keep the reference to the object.

Quoted 6 lines

Quoted 3 lines
The following patch attached is smaller, it also fixes the problem and we don't have to increase the refcount for each object. Now it looks similar to how it was before the RCU patches.
This looks correct to me, we need to keep a reference for continuations. Eric, do you agree with this patch?

Taking a reference I agree for sure, this is the full atomic_inc_not_zero(&ct->ct_general.use) I disagree.

Pablo, the refcount cannot be 0 at this point, or something is really wrong ?

Why not using

atomic_inc(&ct->ct_general.use);

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Pablo Neira Ayuso

author maintainer

2011-01-24 13:06 UTC #6 lore raw

Quoted 118 lines · Eric Dumazet

Quoted 107 lines · Patrick McHardy
Quoted 106 lines · Pablo Neira Ayuso
Quoted 7 lines · Eric Dumazet
Quoted 3 lines · Pablo Neira Ayuso
In 13ee6ac579574a2a95e982b19920fd2495dce8cd, we recovered spinlocks to protect the dump of the conntrack table according to reports from Stephen and acknowledgments on the issue from Eric.
When a commit is referred, always give its title, and you can use a short id (12 digits are OK)
I'm using the id that git log --oneline shows, is it OK?
Quoted 9 lines
In 13ee6ac57957 (netfilter: fix race in conntrack between dump_table and destroy)
Quoted 6 lines
However, Stephen removed the refcount bump in that patch that allows to keep a reference to the current ct object we are interating over. That code avoids race conditions between ct object destruction and the iteration itself. This patch reintroduces these lines since the ct object may vanish between two recvmgs() invocations.
Quoted 6 lines
Quoted 4 lines
This patch fixes ocasional crashes while dumping the conntrack table intensively.
Cc Stephen Hemminger <[email protected]>
Thats not true, Stephen was not in CC on your mail
I'm sorry, I forgot to add --cc to stgit.
Quoted 62 lines
I add him right now.
Quoted 30 lines
Signed-off-by Pablo Neira Ayuso <[email protected]>
---
 net/netfilter/nf_conntrack_netlink.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)
net/netfilter/nf_conntrack_netlink.c +4 -2
index 93297aa..519e245 100644
@@ -654,14 +654,16 @@ restart:
 			if (NF_CT_DIRECTION(h) != IP_CT_DIR_ORIGINAL)
 				continue;
 			ct = nf_ct_tuplehash_to_ctrack(h);
+			if (!atomic_inc_not_zero(&ct->ct_general.use))
+				continue;
 			/* Dump entries of a given L3 protocol number.
 			 * If it is not specified, ie. l3proto == 0,
 			 * then dump everything. */
 			if (l3proto && nf_ct_l3num(ct) != l3proto)
-				continue;
+				goto releasect;
 			if (cb->args[1]) {
 				if (ct != last)
-					continue;
+					goto releasect;
 				cb->args[1] = 0;
 			}
 			if (ctnetlink_fill_info(skb, NETLINK_CB(cb->skb).pid,
net/netfilter/nf_conntrack_netlink.c restart: · line 688
Quoted 8 lines
net/netfilter/nf_conntrack_netlink.c +2 -0
@@ -679,6 +681,8 @@ restart:
 				if (acct)
 					memset(acct, 0, sizeof(struct nf_conn_counter[IP_CT_DIR_MAX]));
 			}
+releasect:
+		nf_ct_put(ct);
 		}
 		if (cb->args[1]) {
 			cb->args[1] = 0;
Hmm...
Only the RCU lookup should need this extra check (atomic_inc_not_zero), not a writer. (And a dumper is a "writer" since it holds nf_conntrack spinlock)
In Stephen commit, we switched from RCU lookup to traditional one (spinlock protected), so, we should not need to touch individual objects refcount ?
I feel the right fix is not to increment refcount then decrement it.
Only to _test_ it being zero, and not dumping the ct, eventually ?
net/netfilter/nf_conntrack_netlink.c +0 -0
index 93297aa..a977cc7 100644
net/netfilter/nf_conntrack_netlink.c restart: · line 661
Quoted 8 lines
net/netfilter/nf_conntrack_netlink.c +2 -0
@@ -654,6 +654,8 @@ restart:
 			if (NF_CT_DIRECTION(h) != IP_CT_DIR_ORIGINAL)
 				continue;
 			ct = nf_ct_tuplehash_to_ctrack(h);
+			if (!atomic_read(&ct->ct_general.use))
+				continue;
 			/* Dump entries of a given L3 protocol number.
 			 * If it is not specified, ie. l3proto == 0,
 			 * then dump everything. */
No, that won't work.
Sorry, I didn't explain the problem appropriately. We still do a nf_ct_put() for last ct. In that patch, you forgot to add the refcount bump that we need to keep the reference to the object.
Quoted 6 lines
Quoted 3 lines
The following patch attached is smaller, it also fixes the problem and we don't have to increase the refcount for each object. Now it looks similar to how it was before the RCU patches.
This looks correct to me, we need to keep a reference for continuations. Eric, do you agree with this patch?
Taking a reference I agree for sure, this is the full atomic_inc_not_zero(&ct->ct_general.use) I disagree.

Quoted 8 lines

Pablo, the refcount cannot be 0 at this point, or something is really wrong ?
Why not using
atomic_inc(&ct->ct_general.use);
?

Yes, we can use nf_conntrack_get (which does atomic_inc) instead. New patch attached.

netfilter: ctnetlink: fix missing refcount increment during dumps

From: Pablo Neira Ayuso <[email protected]>

        if (last)
                nf_ct_put(last)

Cc Stephen Hemminger <[email protected]>

Cc Eric Dumazet <[email protected]>

Signed-off-by Pablo Neira Ayuso <[email protected]>

---
 net/netfilter/nf_conntrack_netlink.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

net/netfilter/nf_conntrack_netlink.c +1 -0

index 93297aa..eead9db 100644
@@ -667,6 +667,7 @@ restart:
 			if (ctnetlink_fill_info(skb, NETLINK_CB(cb->skb).pid,
 						cb->nlh->nlmsg_seq,
 						IPCTNL_MSG_CT_NEW, ct) < 0) {
+				nf_conntrack_get(&ct->ct_general);
 				cb->args[1] = (unsigned long)ct;
 				goto out;
 			}

Eric Dumazet

2011-01-24 13:12 UTC #7 lore raw

Quoted 2 lines · Pablo Neira Ayuso

Yes, we can use nf_conntrack_get (which does atomic_inc) instead. New patch attached.

I feel now a bit uncomfortable, sorry ;)

Are we sure the refcount cannot reach 0 while we hold nf_conntrack_lock ?

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Pablo Neira Ayuso

author maintainer

2011-01-24 13:25 UTC #8 lore raw

Quoted 9 lines · Eric Dumazet

Quoted 2 lines · Pablo Neira Ayuso
Yes, we can use nf_conntrack_get (which does atomic_inc) instead. New patch attached.
I feel now a bit uncomfortable, sorry ;)
Are we sure the refcount cannot reach 0 while we hold nf_conntrack_lock ?

the ct deletion from the hash list is protected by spin lock, so whatever deletion would wait until we have left the dump section.

with this patch, the code looks like it was in 2.6.24 before the rcu stuff. --
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Patrick McHardy

2011-01-24 13:37 UTC #9 lore raw

Quoted 8 lines · Pablo Neira Ayuso

Quoted 7 lines · Eric Dumazet
Quoted 2 lines · Pablo Neira Ayuso
Yes, we can use nf_conntrack_get (which does atomic_inc) instead. New patch attached.
I feel now a bit uncomfortable, sorry ;)

Quoted 7 lines

Quoted 2 lines
Are we sure the refcount cannot reach 0 while we hold nf_conntrack_lock ?
the ct deletion from the hash list is protected by spin lock, so whatever deletion would wait until we have left the dump section.
with this patch, the code looks like it was in 2.6.24 before the rcu stuff.

Yeah, we definitely have a reference while the conntrack is contained in the hash table, and removal requires taking nf_conntrack_lock, therefor using the conntrack entry while holding the lock is valid. --
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Eric Dumazet

2011-01-24 14:06 UTC #10 lore raw

Quoted 12 lines · Patrick McHardy

Quoted 11 lines · Pablo Neira Ayuso
Quoted 9 lines · Eric Dumazet
Quoted 2 lines · Pablo Neira Ayuso
Yes, we can use nf_conntrack_get (which does atomic_inc) instead. New patch attached.
I feel now a bit uncomfortable, sorry ;)
Are we sure the refcount cannot reach 0 while we hold nf_conntrack_lock ?

Quoted 8 lines

Quoted 4 lines
the ct deletion from the hash list is protected by spin lock, so whatever deletion would wait until we have left the dump section.
with this patch, the code looks like it was in 2.6.24 before the rcu stuff.
Yeah, we definitely have a reference while the conntrack is contained in the hash table, and removal requires taking nf_conntrack_lock, therefor using the conntrack entry while holding the lock is valid.

Yes, but to clarify my question, is the following possible ?

CPU 0 CPU1
dump()
spin_lock()
....
if (not enough room in skb)
decrement last refcount
increment_ref_count()
spin_unlock();
since refcount hit 0, spin_lock(nf_conntrack) remove ct from hashes

If yes we should use the atomic_inc_not_zero() form.

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Patrick McHardy

2011-01-24 17:53 UTC #11 lore raw

Am 24.01.2011 15:06, schrieb Eric Dumazet:

Quoted 24 lines · Patrick McHardy

Quoted 12 lines · Pablo Neira Ayuso
Quoted 11 lines · Eric Dumazet
Quoted 9 lines · Pablo Neira Ayuso
Quoted 3 lines
Yes, we can use nf_conntrack_get (which does atomic_inc) instead. New patch attached.
I feel now a bit uncomfortable, sorry ;)
Are we sure the refcount cannot reach 0 while we hold nf_conntrack_lock ?
Quoted 8 lines
Quoted 4 lines
the ct deletion from the hash list is protected by spin lock, so whatever deletion would wait until we have left the dump section.
with this patch, the code looks like it was in 2.6.24 before the rcu stuff.
Yeah, we definitely have a reference while the conntrack is contained in the hash table, and removal requires taking nf_conntrack_lock, therefor using the conntrack entry while holding the lock is valid.
Yes, but to clarify my question, is the following possible ?

Quoted 12 lines

CPU 0 CPU1
dump()
spin_lock()
....
if (not enough room in skb)
decrement last refcount
increment_ref_count()
spin_unlock();
since refcount hit 0, spin_lock(nf_conntrack) remove ct from hashes

No, that can't happen. Before the last refcount is released, the entry is removed from the hash, which requires to take the lock. Each entry contained in the hash table has a refcount of at least 1.

If there are no futher concerns, I'm going to apply Pablo's patch. --
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Eric Dumazet

Acked-by

2011-01-24 18:01 UTC #12 lore raw

Quoted 5 lines · Patrick McHardy

No, that can't happen. Before the last refcount is released, the entry is removed from the hash, which requires to take the lock. Each entry contained in the hash table has a refcount of at least 1.
If there are no futher concerns, I'm going to apply Pablo's patch.

Thanks, that's perfect by me

Acked-by Eric Dumazet <[email protected]>

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Patrick McHardy

2011-01-24 18:16 UTC #13 lore raw

Am 24.01.2011 19:01, schrieb Eric Dumazet:

Quoted 11 lines · Patrick McHardy

Quoted 6 lines
No, that can't happen. Before the last refcount is released, the entry is removed from the hash, which requires to take the lock. Each entry contained in the hash table has a refcount of at least 1.
If there are no futher concerns, I'm going to apply Pablo's patch.
Thanks, that's perfect by me
Acked-by Eric Dumazet <[email protected]>

Applied, thanks everyone.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Stephen Hemminger

2011-02-20 20:51 UTC #14 lore raw

In reply to Pablo Neira Ayuso

Quoted 10 lines · On Mon, 24 Jan 2011 00:16:02 +0100 Pablo Neira Ayuso <[email protected]>…

In 13ee6ac579574a2a95e982b19920fd2495dce8cd, we recovered spinlocks to protect the dump of the conntrack table according to reports from Stephen and acknowledgments on the issue from Eric.
However, Stephen removed the refcount bump in that patch that allows to keep a reference to the current ct object we are interating over. That code avoids race conditions between ct object destruction and the iteration itself. This patch reintroduces these lines since the ct object may vanish between two recvmgs() invocations.

Quoted 5 lines

This patch fixes ocasional crashes while dumping the conntrack table intensively.
Cc Stephen Hemminger <[email protected]>
Signed-off-by Pablo Neira Ayuso <[email protected]>

These two patches should have been submitted to the stable kernel.

Stephen Hemminger

2011-02-20 20:51 UTC #15 lore raw

PATCH 1/2 netfilter: fix race in conntrack between dump_table and destroy

The netlink interface to dump the connection tracking table has a race when entries are deleted at the same time. A customer reported a crash and the backtrace showed thatctnetlink_dump_table was running while a conntrack entry was being destroyed.
(see https://bugzilla.vyatta.com/show_bug.cgi?id=6402).

According to RCU documentation, when using hlist_nulls the reader must handle the case of seeing a deleted entry and not proceed further down the linked list. The old code would continue which caused the scan to walk into the free list.

This patch uses locking (rather than RCU) for this operation which is guaranteed safe, and no longer requires getting reference while doing dump operation.

Signed-off-by Stephen Hemminger <[email protected]>

Signed-off-by Pablo Neira Ayuso <[email protected]>

(cherry picked from commit 13ee6ac579574a2a95e982b19920fd2495dce8cd)

---
Patch against 2.6.37.1

 net/netfilter/nf_conntrack_netlink.c |   14 +++++---------
 1 files changed, 5 insertions(+), 9 deletions(-)

net/netfilter/nf_conntrack_netlink.c +5 -9

index b729ace..d57dcdb 100644
@@ -642,25 +642,23 @@ ctnetlink_dump_table(struct sk_buff *skb, struct netlink_callback *cb)
 	struct nfgenmsg *nfmsg = nlmsg_data(cb->nlh);
 	u_int8_t l3proto = nfmsg->nfgen_family;
 
-	rcu_read_lock();
+	spin_lock_bh(&nf_conntrack_lock);
 	last = (struct nf_conn *)cb->args[1];
 	for (; cb->args[0] < net->ct.htable_size; cb->args[0]++) {
 restart:
-		hlist_nulls_for_each_entry_rcu(h, n, &net->ct.hash[cb->args[0]],
+		hlist_nulls_for_each_entry(h, n, &net->ct.hash[cb->args[0]],
 					 hnnode) {
 			if (NF_CT_DIRECTION(h) != IP_CT_DIR_ORIGINAL)
 				continue;
 			ct = nf_ct_tuplehash_to_ctrack(h);
-			if (!atomic_inc_not_zero(&ct->ct_general.use))
-				continue;
 			/* Dump entries of a given L3 protocol number.
 			 * If it is not specified, ie. l3proto == 0,
 			 * then dump everything. */
 			if (l3proto && nf_ct_l3num(ct) != l3proto)
-				goto releasect;
+				continue;
 			if (cb->args[1]) {
 				if (ct != last)
-					goto releasect;
+					continue;
 				cb->args[1] = 0;
 			}
 			if (ctnetlink_fill_info(skb, NETLINK_CB(cb->skb).pid,

@@ -678,8 +676,6 @@ restart:
 				if (acct)
 					memset(acct, 0, sizeof(struct nf_conn_counter[IP_CT_DIR_MAX]));
 			}
-releasect:
-		nf_ct_put(ct);
 		}
 		if (cb->args[1]) {
 			cb->args[1] = 0;

@@ -687,7 +683,7 @@ releasect:
 		}
 	}
 out:
-	rcu_read_unlock();
+	spin_unlock_bh(&nf_conntrack_lock);
 	if (last)
 		nf_ct_put(last);

Signature · 1 line

1.7.1

Stephen Hemminger

2011-02-20 20:51 UTC #16 lore raw

PATCH 2/2 netfilter: ctnetlink: fix missing refcount increment during dumps

In reply to Stephen Hemminger

Quoted 2 lines

From Pablo Neira Ayuso <[email protected]>

        if (last)
                nf_ct_put(last)

Cc Stephen Hemminger <[email protected]>

Signed-off-by Pablo Neira Ayuso <[email protected]>

Acked-by Eric Dumazet <[email protected]>

Signed-off-by Patrick McHardy <[email protected]>

(cherry picked from commit c71caf4114a0e1da3451cc92fba6a152929cd4c2)

---
Patch against v2.6.37.1

 net/netfilter/nf_conntrack_netlink.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

net/netfilter/nf_conntrack_netlink.c +1 -0

index d57dcdb..742a6dc 100644
@@ -664,6 +664,7 @@ restart:
 			if (ctnetlink_fill_info(skb, NETLINK_CB(cb->skb).pid,
 						cb->nlh->nlmsg_seq,
 						IPCTNL_MSG_CT_NEW, ct) < 0) {
+				nf_conntrack_get(&ct->ct_general);
 				cb->args[1] = (unsigned long)ct;
 				goto out;
 			}

Signature · 1 line

1.7.1