: Fix policy update bug when increasing priority of last policy
Subsystems (2)
PATCH 1/1 : Fix policy update bug when increasing priority of last policy
When the last policy for a direction is replaced by a policy with equal selector but a higher priority, insertion of the new policy fails.
in xfrm_policy_insert:
for (p = &xfrm_policy_list[dir]; (pol=*p)!=NULL; p = &pol->next) {
if (!delpol && memcmp(&policy->selector, &pol->selector,
sizeof(pol->selector)) == 0) {
if (excl) {
write_unlock_bh(&xfrm_policy_lock);
return -EEXIST;
}
*p = pol->next;
delpol = pol;
X if (policy->priority > pol->priority)
X continue;
} else if (policy->priority >= pol->priority)
continue;
if (!newpos)
newpos = p;
if (delpol)
break;
}If the new policy has a higher priority than the old one, the loop will be continued in the lines marked with X, but because there are no further elements, it will leave the loop without setting newpos.
The problem can be verified with ip xfrm:
# ip xfrm policy list
# ip xfrm policy update dir fwd src 10.0.0.1 dst 10.0.0.2 action allow priority 0
# ip xfrm policy list
src 10.0.0.1/32 dst 10.0.0.2/32
dir fwd priority 0
# ip xfrm policy update dir fwd src 10.0.0.1 dst 10.0.0.2 action allow priority 1
# ip xfrm policy list
#This patch checks for *p != NULL before continuing the loop.
Regards
Patrick
# This is a BitKeeper generated diff -Nru style patch. #
# ChangeSet
# 2004/10/18 21:57:18+02:00 [email protected]
# [XFRM]: Fix policy update bug when increasing priority of last policy
#
# Signed-off-by: Patrick McHardy <[email protected]>
#
# net/xfrm/xfrm_policy.c # 2004/10/18 21:56:41+02:00 [email protected] +1 -1 # [XFRM]: Fix policy update bug when increasing priority of last policy # # Signed-off-by: Patrick McHardy <[email protected]> # diff -Nru a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c --- a/net/xfrm/xfrm_policy.c 2004-10-18 21:58:24 +02:00 +++ b/net/xfrm/xfrm_policy.c 2004-10-18 21:58:24 +02:00 @@ -340,7 +340,7 @@ } *p = pol->next; delpol = pol; - if (policy->priority > pol->priority) + if (policy->priority > pol->priority && *p != NULL) continue; } else if (policy->priority >= pol->priority) continue;
Quoted 4 lines · Patrick McHardy
When the last policy for a direction is replaced by a policy with equal selector but a higher priority, insertion of the new policy fails.
Well spotted, that's two bugs of my creation in one day :)
Quoted 1 line
This patch checks for *p != NULL before continuing the loop.
Unfortunately that doesn't fix it completely. The real bug is the fact that we continue with a bogus p pointing to the deleted element. So what we should do is continue without updating p at all.
Cheers,
Signature · 29 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
===== net/xfrm/xfrm_policy.c 1.54 vs edited =====
--- 1.54/net/xfrm/xfrm_policy.c 2004-09-18 08:16:56 +10:00
+++ edited/net/xfrm/xfrm_policy.c 2004-10-19 09:36:47 +10:00
@@ -332,7 +332,7 @@
struct xfrm_policy **newpos = NULL;
write_lock_bh(&xfrm_policy_lock);
- for (p = &xfrm_policy_list[dir]; (pol=*p)!=NULL; p = &pol->next) {
+ for (p = &xfrm_policy_list[dir]; (pol=*p)!=NULL;) {
if (!delpol && memcmp(&policy->selector, &pol->selector, sizeof(pol->selector)) == 0) {
if (excl) {
write_unlock_bh(&xfrm_policy_lock);
@@ -342,8 +342,10 @@
delpol = pol;
if (policy->priority > pol->priority)
continue;
- } else if (policy->priority >= pol->priority)
+ } else if (policy->priority >= pol->priority) {
+ p = &pol->next;
continue;
+ }
if (!newpos)
newpos = p;
if (delpol)Quoted 12 lines · Herbert Xu
Quoted 3 lines · Patrick McHardy
This patch checks for *p != NULL before continuing the loop.
Unfortunately that doesn't fix it completely. The real bug is the fact that we continue with a bogus p pointing to the deleted element. So what we should do is continue without updating p at all.
You're right, your patch is better. I've tested
it and it works fine.
Regards
Patrick
Quoted 8 lines · On Tue, 19 Oct 2004 16:24:12 +0200 Patrick McHardy <[email protected]> wrote:
Quoted 6 lines · Herbert Xu
Quoted 3 lines · Patrick McHardy
This patch checks for *p != NULL before continuing the loop.
Quoted 8 lines
Quoted 6 lines
Unfortunately that doesn't fix it completely. The real bug is the fact that we continue with a bogus p pointing to the deleted element. So what we should do is continue without updating p at all.
You're right, your patch is better. I've tested
it and it works fine.
I applied Herbert's version of the fix, thanks guys.