[PATCH 2.6]: Fix policy update bug when increasing priority of last policy

[PATCH 2.6]: Fix policy update bug when increasing priority of last policy

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 1555 bytes --]

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


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 881 bytes --]

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2004/10/18 21:57:18+02:00 kaber@coreworks.de 
#   [XFRM]: Fix policy update bug when increasing priority of last policy
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
# net/xfrm/xfrm_policy.c
#   2004/10/18 21:56:41+02:00 kaber@coreworks.de +1 -1
#   [XFRM]: Fix policy update bug when increasing priority of last policy
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
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;

Re: [PATCH 2.6]: Fix policy update bug when increasing priority of last policy

[Herbert Xu]

Patrick McHardy <kaber@trash.net> wrote:
> 
> 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 :)
 
> 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.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
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)

Re: [PATCH 2.6]: Fix policy update bug when increasing priority of last policy

[Patrick McHardy]

Herbert Xu wrote:

>Patrick McHardy <kaber@trash.net> wrote:
>
>>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

Re: [PATCH 2.6]: Fix policy update bug when increasing priority of last policy

[David S. Miller]

On Tue, 19 Oct 2004 16:24:12 +0200
Patrick McHardy <kaber@trash.net> wrote:

> Herbert Xu wrote:
> 
> >Patrick McHardy <kaber@trash.net> wrote:
> >
> >>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.

I applied Herbert's version of the fix, thanks guys.