* Fw:[Bug 70471] xfrm policy node will double unlink.
@ 2014-02-18 2:55 Xianpeng Zhao
2014-02-18 8:37 ` Steffen Klassert
0 siblings, 1 reply; 6+ messages in thread
From: Xianpeng Zhao @ 2014-02-18 2:55 UTC (permalink / raw)
To: netdev; +Cc: alan
Hi Group,
I found a problem about xfrm policy.
In corner case, xfrm policy node will be double unlinked from the list.
The scenario like this:
In thread context, After removed the node from list, before remove the xfrm policy expire timer. At this point, a timer interrupt come, and call the run_timer_softirq to execute the xfrm_policy_timer to remove the expired policy node; because this policy node had already removed from list. this remove will cause the node double unlinked.
I have done such patch to protect this case. I am not sure here the policy->walk.dead means. From the name, I think it mark the policy node was dead. Maybe I use this flag is not correct, please the expert correct me if I am wrong. More detail information can reference bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=70471
my patch:
--- net/xfrm/xfrm_policy.c_old 2014-02-10 10:18:28.421504317 +0800
+++ net/xfrm/xfrm_policy.c 2014-02-10 10:19:01.661503334 +0800
@@ -330,7 +330,6 @@ static void xfrm_queue_purge(struct sk_b
static void xfrm_policy_kill(struct xfrm_policy *policy)
{
- policy->walk.dead = 1;
atomic_inc(&policy->genid);
@@ -1156,6 +1155,7 @@ static struct xfrm_policy *__xfrm_policy
if (hlist_unhashed(&pol->bydst))
return NULL;
+ pol->walk.dead = 1;
hlist_del(&pol->bydst);
hlist_del(&pol->byidx);
list_del(&pol->walk.all);
------------------
Best Regards
Xianpeng
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Fw:[Bug 70471] xfrm policy node will double unlink.
2014-02-18 2:55 Fw:[Bug 70471] xfrm policy node will double unlink Xianpeng Zhao
@ 2014-02-18 8:37 ` Steffen Klassert
2014-02-19 2:07 ` Xianpeng Zhao
0 siblings, 1 reply; 6+ messages in thread
From: Steffen Klassert @ 2014-02-18 8:37 UTC (permalink / raw)
To: Xianpeng Zhao; +Cc: netdev, alan
On Tue, Feb 18, 2014 at 10:55:57AM +0800, Xianpeng Zhao wrote:
> Hi Group,
> I found a problem about xfrm policy.
>
> In corner case, xfrm policy node will be double unlinked from the list.
>
> The scenario like this:
> In thread context, After removed the node from list, before remove the xfrm policy expire timer. At this point, a timer interrupt come, and call the run_timer_softirq to execute the xfrm_policy_timer to remove the expired policy node; because this policy node had already removed from list. this remove will cause the node double unlinked.
Good catch!
I wonder why I've never seen this. Do you have a reproducer for this bug?
Looks like it is sufficient to reinitialize the bydst hlist in
__xfrm_policy_unlink(). Then hlist_unhashed() will notice that
this policy is not linked.
Does the patch below help?
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 121399d..225f439 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -1156,7 +1156,7 @@ static struct xfrm_policy *__xfrm_policy_unlink(struct xfrm_policy *pol,
if (hlist_unhashed(&pol->bydst))
return NULL;
- hlist_del(&pol->bydst);
+ hlist_del_init(&pol->bydst);
hlist_del(&pol->byidx);
list_del(&pol->walk.all);
net->xfrm.policy_count[dir]--;
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: Fw:[Bug 70471] xfrm policy node will double unlink.
2014-02-18 8:37 ` Steffen Klassert
@ 2014-02-19 2:07 ` Xianpeng Zhao
2014-02-19 11:43 ` Steffen Klassert
0 siblings, 1 reply; 6+ messages in thread
From: Xianpeng Zhao @ 2014-02-19 2:07 UTC (permalink / raw)
To: Steffen Klassert; +Cc: netdev, alan
Hi Steffen,
This problem is happened when running stress test; Very little chance can get this case.
As you say, add a long time sleep in function xfrm_policy_bysel_ctx between __xfrm_policy_unlink and
xfrm_policy_kill, will reproduce this issue manually.
About my patch, I am not sure it is OK, because after it patched, the issue had reproduced once, but after some days test recently, have not reproduced again.
But I can make sure when __xfrm_policy_unlink find the node had been removed, return NULL instead of delete again will fix this problem.
What's your suggestions?
------------------
Best Regards
Xianpeng
------------------ Original ------------------
From: "Steffen Klassert";<steffen.klassert@secunet.com>;
Date: Tue, Feb 18, 2014 04:37 PM
To: "Xianpeng Zhao"<673321875@qq.com>;
Cc: "netdev"<netdev@vger.kernel.org>; "alan"<alan@lxorguk.ukuu.org.uk>;
Subject: Re: Fw:[Bug 70471] xfrm policy node will double unlink.
On Tue, Feb 18, 2014 at 10:55:57AM +0800, Xianpeng Zhao wrote:
> Hi Group,
> I found a problem about xfrm policy.
>
> In corner case, xfrm policy node will be double unlinked from the list.
>
> The scenario like this:
> In thread context, After removed the node from list, before remove the xfrm policy expire timer. At this point, a timer interrupt come, and call the run_timer_softirq to execute the xfrm_policy_timer to remove the expired policy node; because this policy node had already removed from list. this remove will cause the node double unlinked.
Good catch!
I wonder why I've never seen this. Do you have a reproducer for this bug?
Looks like it is sufficient to reinitialize the bydst hlist in
__xfrm_policy_unlink(). Then hlist_unhashed() will notice that
this policy is not linked.
Does the patch below help?
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 121399d..225f439 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -1156,7 +1156,7 @@ static struct xfrm_policy *__xfrm_policy_unlink(struct xfrm_policy *pol,
if (hlist_unhashed(&pol->bydst))
return NULL;
- hlist_del(&pol->bydst);
+ hlist_del_init(&pol->bydst);
hlist_del(&pol->byidx);
list_del(&pol->walk.all);
net->xfrm.policy_count[dir]--;
.
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: Fw:[Bug 70471] xfrm policy node will double unlink.
2014-02-19 2:07 ` Xianpeng Zhao
@ 2014-02-19 11:43 ` Steffen Klassert
2014-02-20 2:01 ` Xianpeng Zhao
0 siblings, 1 reply; 6+ messages in thread
From: Steffen Klassert @ 2014-02-19 11:43 UTC (permalink / raw)
To: Xianpeng Zhao; +Cc: netdev, alan
On Wed, Feb 19, 2014 at 10:07:14AM +0800, Xianpeng Zhao wrote:
> Hi Steffen,
>
> This problem is happened when running stress test; Very little chance can get this case.
>
> As you say, add a long time sleep in function xfrm_policy_bysel_ctx between __xfrm_policy_unlink and
> xfrm_policy_kill, will reproduce this issue manually.
>
> About my patch, I am not sure it is OK, because after it patched, the issue had reproduced once, but after some days test recently, have not reproduced again.
>
> But I can make sure when __xfrm_policy_unlink find the node had been removed, return NULL instead of delete again will fix this problem.
>
> What's your suggestions?
>
Please test the patch I've sent with the last mail.
Thanks!
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Fw:[Bug 70471] xfrm policy node will double unlink.
2014-02-19 11:43 ` Steffen Klassert
@ 2014-02-20 2:01 ` Xianpeng Zhao
2014-02-21 7:35 ` Steffen Klassert
0 siblings, 1 reply; 6+ messages in thread
From: Xianpeng Zhao @ 2014-02-20 2:01 UTC (permalink / raw)
To: Steffen Klassert; +Cc: netdev, alan
Sorry, I misunderstood your means in last mail.
I get your patch now. Later I will use your patch.
Now, I am using my patch doing the test, my patch is like this; till now, everything is OK. I think my patch almost same with your patch.
diff --git a/include/linux/list.h b/include/linux/list.h
index cc6d2aa..3a8b95a 100644
--- a/include/linux/list.h
+++ b/include/linux/list.h
@@ -578,6 +578,11 @@ static inline int hlist_unhashed(const struct hlist_node *h)
return !h->pprev;
}
+static inline int hlist_removed(const struct hlist_node *h)
+{
+ return (h->pprev == LIST_POISON2 && h->next == LIST_POISON1);
+}
+
static inline int hlist_empty(const struct hlist_head *h)
{
return !h->first;
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 41eabc4..005be47 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -1073,7 +1073,7 @@ static struct xfrm_policy *__xfrm_policy_unlink(struct xfrm_policy *pol,
{
struct net *net = xp_net(pol);
- if (hlist_unhashed(&pol->bydst))
+ if (hlist_unhashed(&pol->bydst) || hlist_removed(&pol->bydst))
return NULL;
hlist_del(&pol->bydst);
------------------ Original ------------------
From: "Steffen Klassert";<steffen.klassert@secunet.com>;
Date: Wed, Feb 19, 2014 07:43 PM
To: "Xianpeng Zhao"<673321875@qq.com>;
Cc: "netdev"<netdev@vger.kernel.org>; "alan"<alan@lxorguk.ukuu.org.uk>;
Subject: Re: Fw:[Bug 70471] xfrm policy node will double unlink.
On Wed, Feb 19, 2014 at 10:07:14AM +0800, Xianpeng Zhao wrote:
> Hi Steffen,
>
> This problem is happened when running stress test; Very little chance can get this case.
>
> As you say, add a long time sleep in function xfrm_policy_bysel_ctx between __xfrm_policy_unlink and
> xfrm_policy_kill, will reproduce this issue manually.
>
> About my patch, I am not sure it is OK, because after it patched, the issue had reproduced once, but after some days test recently, have not reproduced again.
>
> But I can make sure when __xfrm_policy_unlink find the node had been removed, return NULL instead of delete again will fix this problem.
>
> What's your suggestions?
>
Please test the patch I've sent with the last mail.
Thanks!
.
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: Fw:[Bug 70471] xfrm policy node will double unlink.
2014-02-20 2:01 ` Xianpeng Zhao
@ 2014-02-21 7:35 ` Steffen Klassert
0 siblings, 0 replies; 6+ messages in thread
From: Steffen Klassert @ 2014-02-21 7:35 UTC (permalink / raw)
To: Xianpeng Zhao; +Cc: netdev, alan
On Thu, Feb 20, 2014 at 10:01:19AM +0800, Xianpeng Zhao wrote:
> Sorry, I misunderstood your means in last mail.
> I get your patch now. Later I will use your patch.
>
The patch below is what I plan to commit to the ipsec tree,
please test it. Thanks!
Subject: [PATCH] xfrm: Fix unlink race when policies are deleted.
When a policy is unlinked from the lists in thread context,
the xfrm timer can fire before we can mark this policy as dead.
So reinitialize the bydst hlist, then hlist_unhashed() will
notice that this policy is not linked and will avoid a
doulble unlink of that policy.
Reported-by: Xianpeng Zhao <673321875@qq.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
net/xfrm/xfrm_policy.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 4b98b25..1d5c7bf 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -1158,7 +1158,7 @@ static struct xfrm_policy *__xfrm_policy_unlink(struct xfrm_policy *pol,
if (hlist_unhashed(&pol->bydst))
return NULL;
- hlist_del(&pol->bydst);
+ hlist_del_init(&pol->bydst);
hlist_del(&pol->byidx);
list_del(&pol->walk.all);
net->xfrm.policy_count[dir]--;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-02-21 7:35 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-02-18 2:55 Fw:[Bug 70471] xfrm policy node will double unlink Xianpeng Zhao
2014-02-18 8:37 ` Steffen Klassert
2014-02-19 2:07 ` Xianpeng Zhao
2014-02-19 11:43 ` Steffen Klassert
2014-02-20 2:01 ` Xianpeng Zhao
2014-02-21 7:35 ` Steffen Klassert
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).