Inconsistend lock state in inet_frag_find

PATCH Applied Eric Sesterhenn 3 participants 9 messages May 29, 2008 – Jun 2, 2008

Subsystems (3)

Networking Network drivers netfilter

lore

Share & tools

lore

Jarek Poplawski

2008-05-30 10:49 UTC #1 lore raw

Quoted 5 lines · Eric Sesterhenn

hi,
the following just popped up on my test box with
tcpsic6 -s ::1 -d ::1 -p 100000 -r 4995

Quoted 8 lines

[   63.616218] =================================
[   63.616456] [ INFO: inconsistent lock state ]
[   63.616456] 2.6.26-rc4 #5
[   63.616456] ---------------------------------
[   63.616456] inconsistent {softirq-on-W} -> {in-softirq-R} usage.
[   63.616456] tcpsic6/3869 [HC0[0]:SC1[1]:HE1:SE0] takes:
[   63.616456]  (&f->lock){---?}, at: [<c06be62e>]

inet_frag_find+0x1e/0x140

...

Hi,

Could you try this patch?

Regards,
Jarek P.

---

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

net/ipv6/netfilter/nf_conntrack_reasm.c +2 -0

index 2dccad4..f3c36d6 100644
@@ -209,7 +209,9 @@ fq_find(__be32 id, struct in6_addr *src, struct in6_addr *dst)
 	arg.dst = dst;
 	hash = ip6qhashfn(id, src, dst);
 
+	local_bh_disable();
 	q = inet_frag_find(&nf_init_frags, &nf_frags, &arg, hash);
+	local_bh_enable();
 	if (q == NULL)
 		goto oom;

Jarek Poplawski

2008-05-30 17:11 UTC #3 lore raw

On Fri, May 30, 2008 at 03:18:45PM +0200, Eric Sesterhenn wrote:
...

Quoted 15 lines

with the patch applied i get the following lockdep warning:

[   63.531438] =================================
[   63.531520] [ INFO: inconsistent lock state ]
[   63.531520] 2.6.26-rc4 #7
[   63.531520] ---------------------------------
[   63.531520] inconsistent {softirq-on-W} -> {in-softirq-W} usage.
[   63.531520] tcpsic6/3864 [HC0[0]:SC1[1]:HE1:SE0] takes:
[   63.531520]  (&q->lock#2){-+..}, at: [<c07175b0>]
ipv6_frag_rcv+0xd0/0xbd0
[   63.531520] {softirq-on-W} state was registered at:
[   63.531520]   [<c0143bba>] __lock_acquire+0x3aa/0x1080
[   63.531520]   [<c0144906>] lock_acquire+0x76/0xa0
[   63.531520]   [<c07a8f0b>] _spin_lock+0x2b/0x40
[   63.531520]   [<c0727636>] nf_ct_frag6_gather+0x3f6/0x910

...

I hope it's not a Pandora's Box: looks like there are still a few of these locks around, which could make more such reports, but since I don't know this code, I'd prefer not to go ahead of lockdep...

Thanks,
Jarek P.

(Take 2: please revert the previous patch before applying.)
---

 net/ipv6/netfilter/nf_conntrack_reasm.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

net/ipv6/netfilter/nf_conntrack_reasm.c +5 -3

index 2dccad4..e65e26e 100644
@@ -209,7 +209,9 @@ fq_find(__be32 id, struct in6_addr *src, struct in6_addr *dst)
 	arg.dst = dst;
 	hash = ip6qhashfn(id, src, dst);
 
+	local_bh_disable();
 	q = inet_frag_find(&nf_init_frags, &nf_frags, &arg, hash);
+	local_bh_enable();
 	if (q == NULL)
 		goto oom;

@@ -638,10 +640,10 @@ struct sk_buff *nf_ct_frag6_gather(struct sk_buff *skb)
 		goto ret_orig;
 	}
 
-	spin_lock(&fq->q.lock);
+	spin_lock_bh(&fq->q.lock);
 
 	if (nf_ct_frag6_queue(fq, clone, fhdr, nhoff) < 0) {
-		spin_unlock(&fq->q.lock);
+		spin_unlock_bh(&fq->q.lock);
 		pr_debug("Can't insert skb to queue\n");
 		fq_put(fq);
 		goto ret_orig;

@@ -653,7 +655,7 @@ struct sk_buff *nf_ct_frag6_gather(struct sk_buff *skb)
 		if (ret_skb == NULL)
 			pr_debug("Can't reassemble fragmented packets\n");
 	}
-	spin_unlock(&fq->q.lock);
+	spin_unlock_bh(&fq->q.lock);
 
 	fq_put(fq);
 	return ret_skb;

Eric Sesterhenn

author

2008-05-30 21:13 UTC #4 lore raw

In reply to Jarek Poplawski

Quoted 17 lines · * Jarek Poplawski ([email protected]) wrote:

On Fri, May 30, 2008 at 03:18:45PM +0200, Eric Sesterhenn wrote:
...

Quoted 15 lines

with the patch applied i get the following lockdep warning:

[   63.531438] =================================
[   63.531520] [ INFO: inconsistent lock state ]
[   63.531520] 2.6.26-rc4 #7
[   63.531520] ---------------------------------
[   63.531520] inconsistent {softirq-on-W} -> {in-softirq-W} usage.
[   63.531520] tcpsic6/3864 [HC0[0]:SC1[1]:HE1:SE0] takes:
[   63.531520]  (&q->lock#2){-+..}, at: [<c07175b0>]
ipv6_frag_rcv+0xd0/0xbd0
[   63.531520] {softirq-on-W} state was registered at:
[   63.531520]   [<c0143bba>] __lock_acquire+0x3aa/0x1080
[   63.531520]   [<c0144906>] lock_acquire+0x76/0xa0
[   63.531520]   [<c07a8f0b>] _spin_lock+0x2b/0x40
[   63.531520]   [<c0727636>] nf_ct_frag6_gather+0x3f6/0x910

Quoted 5 lines

...
I hope it's not a Pandora's Box: looks like there are still a few of these locks around, which could make more such reports, but since I don't know this code, I'd prefer not to go ahead of lockdep...

looks like we got lucky :-)

Quoted 1 line

(Take 2: please revert the previous patch before applying.)

This one does the trick, i let tcp6sic run a bit longer and didnt see any more lockdep warnings.

Thanks, Eric

Jarek Poplawski

2008-05-30 21:53 UTC #5 lore raw

PATCH 1/1 : fix inconsistent lock state in nf_ct_frag6_gather()

On Fri, May 30, 2008 at 11:13:15PM +0200, Eric Sesterhenn wrote:
...

Quoted 4 lines

This one does the trick, i let tcp6sic run a bit longer and didnt see any more lockdep warnings.
Thanks, Eric

David & Patrick,

It looks like this patch could be applied.

Thanks,
Jarek P.

-------------------------->

[NETFILTER]: fix inconsistent lock state in nf_ct_frag6_gather()

[   63.531438] =================================
[   63.531520] [ INFO: inconsistent lock state ]
[   63.531520] 2.6.26-rc4 #7
[   63.531520] ---------------------------------
[   63.531520] inconsistent {softirq-on-W} -> {in-softirq-W} usage.
[   63.531520] tcpsic6/3864 [HC0[0]:SC1[1]:HE1:SE0] takes:
[   63.531520]  (&q->lock#2){-+..}, at: [<c07175b0>] ipv6_frag_rcv+0xd0/0xbd0
[   63.531520] {softirq-on-W} state was registered at:
[   63.531520]   [<c0143bba>] __lock_acquire+0x3aa/0x1080
[   63.531520]   [<c0144906>] lock_acquire+0x76/0xa0
[   63.531520]   [<c07a8f0b>] _spin_lock+0x2b/0x40
[   63.531520]   [<c0727636>] nf_ct_frag6_gather+0x3f6/0x910
 ...

According to this and another similar lockdep report inet_fragment locks are taken from nf_ct_frag6_gather() with softirqs enabled, but these locks are mainly used in softirq context, so disabling BHs is necessary.

Reported-and-tested-by Eric Sesterhenn <[email protected]>

Signed-off-by Jarek Poplawski <[email protected]>

---

 net/ipv6/netfilter/nf_conntrack_reasm.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

net/ipv6/netfilter/nf_conntrack_reasm.c +5 -3

index 2dccad4..e65e26e 100644
@@ -209,7 +209,9 @@ fq_find(__be32 id, struct in6_addr *src, struct in6_addr *dst)
 	arg.dst = dst;
 	hash = ip6qhashfn(id, src, dst);
 
+	local_bh_disable();
 	q = inet_frag_find(&nf_init_frags, &nf_frags, &arg, hash);
+	local_bh_enable();
 	if (q == NULL)
 		goto oom;

@@ -638,10 +640,10 @@ struct sk_buff *nf_ct_frag6_gather(struct sk_buff *skb)
 		goto ret_orig;
 	}
 
-	spin_lock(&fq->q.lock);
+	spin_lock_bh(&fq->q.lock);
 
 	if (nf_ct_frag6_queue(fq, clone, fhdr, nhoff) < 0) {
-		spin_unlock(&fq->q.lock);
+		spin_unlock_bh(&fq->q.lock);
 		pr_debug("Can't insert skb to queue\n");
 		fq_put(fq);
 		goto ret_orig;

@@ -653,7 +655,7 @@ struct sk_buff *nf_ct_frag6_gather(struct sk_buff *skb)
 		if (ret_skb == NULL)
 			pr_debug("Can't reassemble fragmented packets\n");
 	}
-	spin_unlock(&fq->q.lock);
+	spin_unlock_bh(&fq->q.lock);
 
 	fq_put(fq);
 	return ret_skb;

Patrick McHardy

2008-06-02 10:43 UTC #6 lore raw

Re: [PATCH][NETFILTER]: fix inconsistent lock state in nf_ct_frag6_gather()

Quoted 4 lines · Jarek Poplawski

According to this and another similar lockdep report inet_fragment locks are taken from nf_ct_frag6_gather() with softirqs enabled, but these locks are mainly used in softirq context, so disabling BHs is necessary.

Yes, this can happen on the local output path,

Quoted 3 lines

David & Patrick,
It looks like this patch could be applied.

Looks mostly fine, but don't we also have to disable BHs for the inet_frag_find() call in nf_ct_frag6_gather()?

Jarek Poplawski

2008-06-02 12:41 UTC #7 lore raw

Re: [PATCH][NETFILTER]: fix inconsistent lock state in nf_ct_frag6_gather()

Quoted 6 lines · Patrick McHardy

Quoted 4 lines · Jarek Poplawski
According to this and another similar lockdep report inet_fragment locks are taken from nf_ct_frag6_gather() with softirqs enabled, but these locks are mainly used in softirq context, so disabling BHs is necessary.

Quoted 9 lines

Yes, this can happen on the local output path,
Quoted 3 lines
David & Patrick,
It looks like this patch could be applied.
Looks mostly fine, but don't we also have to disable BHs for the inet_frag_find() call in nf_ct_frag6_gather()?

Probably I miss something, but I think this patch seems to do it:
I can see only one such call in nf_ct_frag6_gather() - with fq_find()?

BTW, it looks like this lockdep warning points at some other possible
problem: inet_fragment.c uses an SMP "optimization" in inet_frag_intern(), probably assuming softirq context; so without such BH blocking around inet_frag_find(), there should be at least preemption disabled, but I don't know if it's a problem for nf_ct.

Regards,
Jarek P.

Patrick McHardy

Applied

2008-06-02 13:24 UTC #8 lore raw

Re: [PATCH][NETFILTER]: fix inconsistent lock state in nf_ct_frag6_gather()

Quoted 10 lines · Jarek Poplawski

Quoted 9 lines · Patrick McHardy
Quoted 4 lines · Jarek Poplawski
According to this and another similar lockdep report inet_fragment locks are taken from nf_ct_frag6_gather() with softirqs enabled, but these locks are mainly used in softirq context, so disabling BHs is necessary.
Yes, this can happen on the local output path,
Quoted 2 lines
David & Patrick,

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
It looks like this patch could be applied.
Looks mostly fine, but don't we also have to disable BHs for the inet_frag_find() call in nf_ct_frag6_gather()?
Probably I miss something, but I think this patch seems to do it:
I can see only one such call in nf_ct_frag6_gather() - with fq_find()?

Right, I missed that, sorry.

Quoted 5 lines

BTW, it looks like this lockdep warning points at some other possible
problem: inet_fragment.c uses an SMP "optimization" in inet_frag_intern(), probably assuming softirq context; so without such BH blocking around inet_frag_find(), there should be at least preemption disabled, but I don't know if it's a problem for nf_ct.

Yes, good catch. With your patch things should be fine however.

I've applied your patch and will also push it to -stable.