resend patch: xfrm policybyid

resend patch: xfrm policybyid

[jamal]

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



I dont think we reached any agreement last time, Herbert - so here is a
resend. 

 
A rule is unique by both selector(which it was already and this patch
doesnt change) and index which could be user specified(new) .
I spent at least 30 minutes testing it and infact captured some of my
testcases at the last minutes - also attached.

cheers,
jamal

[-- Attachment #2: polid_p5 --]
[-- Type: text/plain, Size: 3109 bytes --]

--- a/include/net/xfrm.h	2005/04/28 14:05:00	1.1
+++ b/include/net/xfrm.h	2005/04/28 14:05:48
@@ -302,6 +302,7 @@
 	struct dst_entry       *bundles;
 	__u16			family;
 	__u8			action;
+	__u8			dir;
 	__u8			flags;
 	__u8			dead;
 	__u8			xfrm_nr;
--- a/net/xfrm/xfrm_user.c	2005/04/28 13:59:27	1.1
+++ b/net/xfrm/xfrm_user.c	2005/04/28 14:01:58
@@ -653,6 +653,7 @@
 	memcpy(&xp->selector, &p->sel, sizeof(xp->selector));
 	memcpy(&xp->lft, &p->lft, sizeof(xp->lft));
 	xp->action = p->action;
+	xp->dir = p->dir;
 	xp->flags = p->flags;
 	xp->family = p->sel.family;
 	/* XXX xp->share = p->share; */
--- a/net/xfrm/xfrm_policy.c	2005/04/27 11:32:13	1.1
+++ b/net/xfrm/xfrm_policy.c	2005/04/29 23:07:38
@@ -163,7 +163,7 @@
 	if (xp->dead)
 		goto out;
 
-	dir = xp->index & 7;
+	dir = xp->dir;
 
 	if (xp->lft.hard_add_expires_seconds) {
 		long tmo = xp->lft.hard_add_expires_seconds +
@@ -341,17 +341,35 @@
 {
 	struct xfrm_policy *pol, **p;
 	struct xfrm_policy *delpol = NULL;
+	struct xfrm_policy *delpol2 = NULL;
+	struct xfrm_policy *delp = NULL;
 	struct xfrm_policy **newpos = NULL;
+	int ret = -EINVAL;
+
+	if (policy->index)
+		delpol = xfrm_policy_byid(dir, policy->index, 0);
+	delpol2 = xfrm_policy_bysel(dir, &policy->selector, 0);
+
+	/* must be unique in both index and selector */
+	if (delpol && delpol2) 
+		if (delpol != delpol2) 
+			goto pol_err;
 
+	if (delpol) 
+		delp = delpol;
+	else 
+		delp = delpol2;
+			
+	if (delp && excl) { 
+		ret = -EEXIST;
+		goto pol_err;
+	}
+
+	/* insert, sorted by prio*/
 	write_lock_bh(&xfrm_policy_lock);
 	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);
-				return -EEXIST;
-			}
+		if (pol == delp) {
 			*p = pol->next;
-			delpol = pol;
 			if (policy->priority > pol->priority)
 				continue;
 		} else if (policy->priority >= pol->priority) {
@@ -360,27 +378,36 @@
 		}
 		if (!newpos)
 			newpos = p;
-		if (delpol)
-			break;
 		p = &pol->next;
 	}
+
 	if (newpos)
 		p = newpos;
+	
 	xfrm_pol_hold(policy);
 	policy->next = *p;
 	*p = policy;
 	atomic_inc(&flow_cache_genid);
-	policy->index = delpol ? delpol->index : xfrm_gen_index(dir);
+	if (!policy->index)
+		policy->index = delp ? delp->index : xfrm_gen_index(dir);
+
 	policy->curlft.add_time = (unsigned long)xtime.tv_sec;
 	policy->curlft.use_time = 0;
 	if (!mod_timer(&policy->timer, jiffies + HZ))
 		xfrm_pol_hold(policy);
 	write_unlock_bh(&xfrm_policy_lock);
 
-	if (delpol) {
-		xfrm_policy_kill(delpol);
+	if (delp) {
+		xfrm_policy_kill(delp);
 	}
-	return 0;
+	ret = 0;
+
+pol_err:
+	if (delpol)
+		xfrm_pol_put(delpol);
+	if (delpol2)
+		xfrm_pol_put(delpol2);
+	return ret;
 }
 EXPORT_SYMBOL(xfrm_policy_insert);
 
@@ -413,7 +440,7 @@
 	struct xfrm_policy *pol, **p;
 
 	write_lock_bh(&xfrm_policy_lock);
-	for (p = &xfrm_policy_list[id & 7]; (pol=*p)!=NULL; p = &pol->next) {
+	for (p = &xfrm_policy_list[dir]; (pol=*p)!=NULL; p = &pol->next) {
 		if (pol->index == id) {
 			xfrm_pol_hold(pol);
 			if (delete)

[-- Attachment #3: ipsec-spd-priotst --]
[-- Type: text/plain, Size: 23452 bytes --]

IP=./root/iproute-mod/ip/ip 
root@jzny2: $IP x p flush
root@jzny2: $IP -s x p ls
root@jzny2: $IP x policy add dir in index 1 priority 10 src 12.0.0.10/24 dst 12.0.0.2/24
root@jzny2: $IP x policy add dir in index 2 priority 100 src 11.0.0.10/24 dst 12.0.0.2/24
root@jzny2: $IP x policy add dir in index 4 priority 200 src 11.0.0.10/24 dst 11.0.0.2/24
root@jzny2: $IP x policy add dir in index 5 priority 400 src 13.0.0.10/24 dst 11.0.0.2/24
root@jzny2: $IP -s x p ls
src 12.0.0.10/24 dst 12.0.0.2/24 uid 0
	dir in action allow index 1 priority 10 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:54 use -
src 11.0.0.10/24 dst 12.0.0.2/24 uid 0
	dir in action allow index 2 priority 100 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:54 use -
src 11.0.0.10/24 dst 11.0.0.2/24 uid 0
	dir in action allow index 4 priority 200 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:54 use -
src 13.0.0.10/24 dst 11.0.0.2/24 uid 0
	dir in action allow index 5 priority 400 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:54 use -

root@jzny2: $IP x policy update dir in priority 120 src 12.0.0.10/24 dst 12.0.0.2/24
root@jzny2: $IP -s x p ls
src 11.0.0.10/24 dst 12.0.0.2/24 uid 0
	dir in action allow index 2 priority 100 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:54 use -
src 12.0.0.10/24 dst 12.0.0.2/24 uid 0
	dir in action allow index 1 priority 120 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:54 use -
src 11.0.0.10/24 dst 11.0.0.2/24 uid 0
	dir in action allow index 4 priority 200 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:54 use -
src 13.0.0.10/24 dst 11.0.0.2/24 uid 0
	dir in action allow index 5 priority 400 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:54 use -

root@jzny2: $IP x policy update dir in priority 220 src 12.0.0.10/24 dst 12.0.0.2/24
root@jzny2: $IP -s x p ls
src 11.0.0.10/24 dst 12.0.0.2/24 uid 0
	dir in action allow index 2 priority 100 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:54 use -
src 11.0.0.10/24 dst 11.0.0.2/24 uid 0
	dir in action allow index 4 priority 200 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:54 use -
src 12.0.0.10/24 dst 12.0.0.2/24 uid 0
	dir in action allow index 1 priority 220 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:55 use -
src 13.0.0.10/24 dst 11.0.0.2/24 uid 0
	dir in action allow index 5 priority 400 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:54 use -

root@jzny2: $IP x policy update dir in priority 420 src 12.0.0.10/24 dst 12.0.0.2/24
root@jzny2: $IP -s x p ls
src 11.0.0.10/24 dst 12.0.0.2/24 uid 0
	dir in action allow index 2 priority 100 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:54 use -
src 11.0.0.10/24 dst 11.0.0.2/24 uid 0
	dir in action allow index 4 priority 200 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:54 use -
src 13.0.0.10/24 dst 11.0.0.2/24 uid 0
	dir in action allow index 5 priority 400 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:54 use -
src 12.0.0.10/24 dst 12.0.0.2/24 uid 0
	dir in action allow index 1 priority 420 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:55 use -

root@jzny2: $IP x policy update dir in priority 20 src 12.0.0.10/24 dst 12.0.0.2/24
root@jzny2: $IP -s x p ls
src 12.0.0.10/24 dst 12.0.0.2/24 uid 0
	dir in action allow index 1 priority 20 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:55 use -
src 11.0.0.10/24 dst 12.0.0.2/24 uid 0
	dir in action allow index 2 priority 100 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:54 use -
src 11.0.0.10/24 dst 11.0.0.2/24 uid 0
	dir in action allow index 4 priority 200 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:54 use -
src 13.0.0.10/24 dst 11.0.0.2/24 uid 0
	dir in action allow index 5 priority 400 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:54 use -

root@jzny2: $IP x policy add dir in priority 30 src 13.0.0.10/24 dst 11.0.0.2/24
RTNETLINK answers: File exists

root@jzny2: $IP -s x p ls
src 12.0.0.10/24 dst 12.0.0.2/24 uid 0
	dir in action allow index 1 priority 20 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:55 use -
src 11.0.0.10/24 dst 12.0.0.2/24 uid 0
	dir in action allow index 2 priority 100 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:54 use -
src 11.0.0.10/24 dst 11.0.0.2/24 uid 0
	dir in action allow index 4 priority 200 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:54 use -
src 13.0.0.10/24 dst 11.0.0.2/24 uid 0
	dir in action allow index 5 priority 400 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:54 use -

root@jzny2: $IP x policy update dir in priority 700 src 12.0.0.10/24 dst 12.0.0.2/24
root@jzny2: $IP -s x p ls
src 11.0.0.10/24 dst 12.0.0.2/24 uid 0
	dir in action allow index 2 priority 100 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:54 use -
src 11.0.0.10/24 dst 11.0.0.2/24 uid 0
	dir in action allow index 4 priority 200 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:54 use -
src 13.0.0.10/24 dst 11.0.0.2/24 uid 0
	dir in action allow index 5 priority 400 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:54 use -
src 12.0.0.10/24 dst 12.0.0.2/24 uid 0
	dir in action allow index 1 priority 700 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:56 use -

root@jzny2: $IP x p flush
root@jzny2: $IP -s x p ls
root@jzny2: $IP x policy add dir in index 1 priority 10 src 12.0.0.10/24 dst 12.0.0.2/24
root@jzny2: $IP x policy add dir in index 2 priority 100 src 11.0.0.10/24 dst 12.0.0.2/24
root@jzny2: $IP x policy add dir in index 3 priority 200 src 11.0.0.10/24 dst 11.0.0.2/24
root@jzny2: $IP x policy add dir in index 4 priority 400 src 13.0.0.10/24 dst 11.0.0.2/24
root@jzny2: $IP -s x p ls
src 12.0.0.10/24 dst 12.0.0.2/24 uid 0
	dir in action allow index 1 priority 10 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:59 use -
src 11.0.0.10/24 dst 12.0.0.2/24 uid 0
	dir in action allow index 2 priority 100 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:59 use -
src 11.0.0.10/24 dst 11.0.0.2/24 uid 0
	dir in action allow index 3 priority 200 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:59 use -
src 13.0.0.10/24 dst 11.0.0.2/24 uid 0
	dir in action allow index 4 priority 400 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:59 use -

root@jzny2: $IP x policy update dir in priority 120 index 1
root@jzny2: $IP -s x p ls
src 11.0.0.10/24 dst 12.0.0.2/24 uid 0
	dir in action allow index 2 priority 100 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:59 use -
src 0.0.0.0/0 dst 0.0.0.0/0 uid 0
	dir in action allow index 1 priority 120 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:59 use 2005-04-29 22:16:59
src 11.0.0.10/24 dst 11.0.0.2/24 uid 0
	dir in action allow index 3 priority 200 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:59 use -
src 13.0.0.10/24 dst 11.0.0.2/24 uid 0
	dir in action allow index 4 priority 400 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:59 use -

root@jzny2: $IP x policy update dir in priority 220 index 1
root@jzny2: $IP -s x p ls
src 11.0.0.10/24 dst 12.0.0.2/24 uid 0
	dir in action allow index 2 priority 100 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:59 use -
src 11.0.0.10/24 dst 11.0.0.2/24 uid 0
	dir in action allow index 3 priority 200 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:59 use -
src 0.0.0.0/0 dst 0.0.0.0/0 uid 0
	dir in action allow index 1 priority 220 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:17:00 use 2005-04-29 22:17:00
src 13.0.0.10/24 dst 11.0.0.2/24 uid 0
	dir in action allow index 4 priority 400 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:59 use -

root@jzny2: $IP x policy update dir in priority 420 index 1
root@jzny2: $IP -s x p ls
src 11.0.0.10/24 dst 12.0.0.2/24 uid 0
	dir in action allow index 2 priority 100 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:59 use -
src 11.0.0.10/24 dst 11.0.0.2/24 uid 0
	dir in action allow index 3 priority 200 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:59 use -
src 13.0.0.10/24 dst 11.0.0.2/24 uid 0
	dir in action allow index 4 priority 400 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:59 use -
src 0.0.0.0/0 dst 0.0.0.0/0 uid 0
	dir in action allow index 1 priority 420 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:17:00 use 2005-04-29 22:17:00

root@jzny2: $IP x policy update dir in priority 20 index 1
root@jzny2: $IP -s x p ls
src 0.0.0.0/0 dst 0.0.0.0/0 uid 0
	dir in action allow index 1 priority 20 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:17:01 use 2005-04-29 22:17:01
src 11.0.0.10/24 dst 12.0.0.2/24 uid 0
	dir in action allow index 2 priority 100 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:59 use -
src 11.0.0.10/24 dst 11.0.0.2/24 uid 0
	dir in action allow index 3 priority 200 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:59 use -
src 13.0.0.10/24 dst 11.0.0.2/24 uid 0
	dir in action allow index 4 priority 400 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:59 use -

root@jzny2: $IP x policy add dir in priority 30 index 4
RTNETLINK answers: Invalid argument
root@jzny2: $IP -s x p ls
src 0.0.0.0/0 dst 0.0.0.0/0 uid 0
	dir in action allow index 1 priority 20 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:17:01 use 2005-04-29 22:17:01
src 11.0.0.10/24 dst 12.0.0.2/24 uid 0
	dir in action allow index 2 priority 100 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:59 use -
src 11.0.0.10/24 dst 11.0.0.2/24 uid 0
	dir in action allow index 3 priority 200 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:59 use -
src 13.0.0.10/24 dst 11.0.0.2/24 uid 0
	dir in action allow index 4 priority 400 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:59 use -

root@jzny2: $IP x policy update dir in priority 700 index 1
root@jzny2: $IP -s x p ls
src 11.0.0.10/24 dst 12.0.0.2/24 uid 0
	dir in action allow index 2 priority 100 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:59 use -
src 11.0.0.10/24 dst 11.0.0.2/24 uid 0
	dir in action allow index 3 priority 200 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:59 use -
src 13.0.0.10/24 dst 11.0.0.2/24 uid 0
	dir in action allow index 4 priority 400 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:16:59 use -
src 0.0.0.0/0 dst 0.0.0.0/0 uid 0
	dir in action allow index 1 priority 700 share any flag 0x00000000
	lifetime config:
	  limit: soft (INF)(bytes), hard (INF)(bytes)
	  limit: soft (INF)(packets), hard (INF)(packets)
	  expire add: soft 0(sec), hard 0(sec)
	  expire use: soft 0(sec), hard 0(sec)
	lifetime current:
	  0(bytes), 0(packets)
	  add 2005-04-29 22:17:02 use 2005-04-29 22:17:02

Re: resend patch: xfrm policybyid

[Herbert Xu]

On Thu, May 05, 2005 at 09:14:36AM -0400, jamal wrote:
>
> +	if (policy->index)
> +		delpol = xfrm_policy_byid(dir, policy->index, 0);
> +	delpol2 = xfrm_policy_bysel(dir, &policy->selector, 0);
> +
> +	/* must be unique in both index and selector */
> +	if (delpol && delpol2) 
> +		if (delpol != delpol2) 
> +			goto pol_err;
>  
> +	if (delpol) 
> +		delp = delpol;
> +	else 
> +		delp = delpol2;
> +			
> +	if (delp && excl) { 
> +		ret = -EEXIST;
> +		goto pol_err;
> +	}
> +
> +	/* insert, sorted by prio*/
>  	write_lock_bh(&xfrm_policy_lock);

This is still racy since delp can be killed by timers before you get
the lock.

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

Re: resend patch: xfrm policybyid

[jamal]

On Fri, 2005-06-05 at 07:32 +1000, Herbert Xu wrote:
			
> > +	if (delp && excl) { 
> > +		ret = -EEXIST;
> > +		goto pol_err;
> > +	}
> > +
> > +	/* insert, sorted by prio*/
> >  	write_lock_bh(&xfrm_policy_lock);
> 
> This is still racy since delp can be killed by timers before you get
> the lock.

Ok, Herbert - this is fixable: I take it moving the lock one up is
sufficient; i dont mind if you fix it and add it to your list.

BTW, are the event patches in Davems queue somewhere?
I have a few ipsec ones (cosmetic) that i will send later.

cheers,
jamal

Re: resend patch: xfrm policybyid

[David S. Miller]

On Thu, 05 May 2005 18:17:15 -0400
jamal <hadi@cyberus.ca> wrote:

> BTW, are the event patches in Davems queue somewhere?
> I have a few ipsec ones (cosmetic) that i will send later.

I'm not queueing 2.6.13 material yet.
2.6.12 bug fixes are a full time job yet :-)

Re: resend patch: xfrm policybyid

[Herbert Xu]

On Thu, May 05, 2005 at 03:18:20PM -0700, David S. Miller wrote:
> On Thu, 05 May 2005 18:17:15 -0400
> jamal <hadi@cyberus.ca> wrote:
> 
> > BTW, are the event patches in Davems queue somewhere?
> > I have a few ipsec ones (cosmetic) that i will send later.
> 
> I'm not queueing 2.6.13 material yet.
> 2.6.12 bug fixes are a full time job yet :-)

Dave, would you mind if I pushed these event patches into mm for
some testing?

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

Re: resend patch: xfrm policybyid

[David S. Miller]

On Fri, 6 May 2005 23:28:29 +1000
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> On Thu, May 05, 2005 at 03:18:20PM -0700, David S. Miller wrote:
> > On Thu, 05 May 2005 18:17:15 -0400
> > jamal <hadi@cyberus.ca> wrote:
> > 
> > > BTW, are the event patches in Davems queue somewhere?
> > > I have a few ipsec ones (cosmetic) that i will send later.
> > 
> > I'm not queueing 2.6.13 material yet.
> > 2.6.12 bug fixes are a full time job yet :-)
> 
> Dave, would you mind if I pushed these event patches into mm for
> some testing?

Feel free.

Re: resend patch: xfrm policybyid

[Herbert Xu]

On Thu, May 05, 2005 at 06:17:15PM -0400, jamal wrote:
>
> > This is still racy since delp can be killed by timers before you get
> > the lock.
> 
> Ok, Herbert - this is fixable: I take it moving the lock one up is
> sufficient; i dont mind if you fix it and add it to your list.

I know it's fixable, but the problem is that the fix is likely to
make this function even uglier :)

What I still don't get is who would be using this feature.  No I
don't mean an example of how the ip command can do this :) I mean
a real-world scenario why someone or some KM would want do this and
why it can't be done easily with what we've already got.

BTW I very much like part where you added the dir to the policy
structure.  So if that can be split off I'd have no objections
in seeing it included.

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

Re: resend patch: xfrm policybyid

[jamal]

On Fri, 2005-06-05 at 09:12 +1000, Herbert Xu wrote:
> On Thu, May 05, 2005 at 06:17:15PM -0400, jamal wrote:
> >
> > > This is still racy since delp can be killed by timers before you get
> > > the lock.
> > 
> > Ok, Herbert - this is fixable: I take it moving the lock one up is
> > sufficient; i dont mind if you fix it and add it to your list.
> 
> I know it's fixable, but the problem is that the fix is likely to
> make this function even uglier :)
> 

I am giving you the opportunity now to fix it your way;->  
Rewrite if you want. I promise not to hassle you ;->

> What I still don't get is who would be using this feature.  
> No I
> don't mean an example of how the ip command can do this :) 

Oh, wait, ip doesnt count as an app?;-> Now, lets say it doesnt.
The incosistency i see is not something i have seen in any table logic
in the kernel or anywhere else.
a) I am allowed to set the id to say a 1 only to be  overruled by the
kernel which sets it to 8? 
b) The table is searchable by id - except i just cant set the search
key.

So for the sake of correctness, at least, this needs to be fixed.

> I mean
> a real-world scenario why someone or some KM would want do this and
> why it can't be done easily with what we've already got.
> 

Thats a moot point really Herbert. I can think of a few apps that can
use this, but it shouldnt matter: The main point is correctness.

You know what else you can do is get rid of the index totaly - that
would be fine with me. What do you say to that?


cheers,
jamal

Re: resend patch: xfrm policybyid

[Herbert Xu]

On Thu, May 05, 2005 at 09:15:22PM -0400, jamal wrote:
> 
> Thats a moot point really Herbert. I can think of a few apps that can
> use this, but it shouldnt matter: The main point is correctness.

Ah, that's why we're talking past each other :) You're looking at
it as a bug where we aren't setting the index when it is provided
by the user.

The way I'm seeing it is that the index is simply a read-only value
that's only provided by the kernel to the user as an aid in locating
policies.

In this respect it's just like xfrm_policy->curlft, it can be read
by the user by it's only ever written by the kernel.  So whatever
value the user provides us when adding/updating a policy is simply
ignored.

Another way to look at it is that it's a handle that we're returning
to the user so that they can talk about policies in an unambiguous way.

Does this make sense?

> You know what else you can do is get rid of the index totaly - that
> would be fine with me. What do you say to that?

That would break user API/ABI so no :)

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

Re: resend patch: xfrm policybyid

[jamal]

On Fri, 2005-06-05 at 11:31 +1000, Herbert Xu wrote:
> On Thu, May 05, 2005 at 09:15:22PM -0400, jamal wrote:
> > 
> > Thats a moot point really Herbert. I can think of a few apps that can
> > use this, but it shouldnt matter: The main point is correctness.
> 
> Ah, that's why we're talking past each other :) You're looking at
> it as a bug where we aren't setting the index when it is provided
> by the user.

It is a bug based on what common usage of what indices are used for in
tables. They are search keys.

> The way I'm seeing it is that the index is simply a read-only value
> that's only provided by the kernel to the user as an aid in locating
> policies.
> 

IOW, they are search keys. 
Search keys are unique and read-write. You cant just invent you rules
Herbert ;->
The typical rules are simple for indices:
a) you specify the index - if it doesnt conflict, it is accepted. You
can then use that key to search in the future
b) you dont specifiy the index, the kernel specifies one for you which
doesnt conflict.

We do b) only and override a). 

Look at these as array subscripts if you want an analogy. You can
reference an empty array slot but not one thats being used. If you dont
specify the array index then the kernel will provide one for you.

> In this respect it's just like xfrm_policy->curlft, it can be read
> by the user by it's only ever written by the kernel.  So whatever
> value the user provides us when adding/updating a policy is simply
> ignored.
> 

Theres a difference;-> you cant search by curlft i.e its not a key.

> Another way to look at it is that it's a handle that we're returning
> to the user so that they can talk about policies in an unambiguous way.
> 
> Does this make sense?
> 

You are emphasizing the uniqueuness feature of a search key ;->

> > You know what else you can do is get rid of the index totaly - that
> > would be fine with me. What do you say to that?
> 
> That would break user API/ABI so no :)

Understood.
There are a lot of things i think need fixing in both the SPD and SAD
that i am not touching - this is certainly one that i dont want to leave
alone. 

The other issues i will leave to another discussion  without distracting
this one;->

cheers,
jamal

Re: resend patch: xfrm policybyid

[jamal]

On Thu, 2005-05-05 at 22:10 -0400, jamal wrote:

> 
> Look at these as array subscripts if you want an analogy. You can
> reference an empty array slot but not one thats being used. If you dont
> specify the array index then the kernel will provide one for you.

It wasnt clear - but the above description is for adding.

cheers,
jamal

Re: resend patch: xfrm policybyid

[Herbert Xu]

On Thu, May 05, 2005 at 10:10:03PM -0400, jamal wrote:
> 
> > The way I'm seeing it is that the index is simply a read-only value
> > that's only provided by the kernel to the user as an aid in locating
> > policies.
> 
> IOW, they are search keys. 

I'm afraid I still disagree.  There should only be one key that the
user gets to set when adding policies that is guaranteed to be unique.
As it is it's the selector.

Letting the user specify two independent keys which need to be unique
is what is making your patch ugly and what IMHO will make the code
unmaintainable.

Here is an analogy.  Think of the policies as files and the indicies
as file descriptors.  The index is only valid while the policy is in
the policy list (the file descriptor is only valid while the file is
open).  When you add the policy, you don't get to specify what the
index is (when you open a file, you don't get to specify what the
file descriptor is).  Once the policy has been added, you will be
given an index that you can use to read/delete the policy (once the
file has been opened, you will be given a file descriptor that you
can use to access/close the file).

Now you may argue that file descriptors can be set to whatever value
you want using dup.  However, the difference here is that you can
only do this on an open file.  Similarly, changing the index of a
policy that's already in the list can be done cleanly since we
already have verified that its primary key (the selector) is unique.

However, I must say that I still have absolutely no idea why anyone
would need to set the index to arbitrary values.

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

Re: resend patch: xfrm policybyid

[jamal]

On Fri, 2005-06-05 at 18:54 +1000, Herbert Xu wrote:
> On Thu, May 05, 2005 at 10:10:03PM -0400, jamal wrote:
> > 
> > > The way I'm seeing it is that the index is simply a read-only value
> > > that's only provided by the kernel to the user as an aid in locating
> > > policies.
> > 
> > IOW, they are search keys. 
> 
> I'm afraid I still disagree.  

You are a reasonable man, so i am hoping you will end agreeing 
or agreeing to disagree;->

> There should only be one key that the
> user gets to set when adding policies that is guaranteed to be unique.
> As it is it's the selector.
> 

Note: The index was already guaranteed to be unique even without my
patch.

Just to make sure we are not speaking past each other:
A key is a column in a table that can be uniquely used to identify said
row. 
A table can have more than one key (a lot of relational databases do),
although it adds a lot of gunk in the kernel.
In this case actually the justification exists: The selector is needed
for data/packet path lookup key. The index for manager side
manageability. 

> Letting the user specify two independent keys which need to be unique
> is what is making your patch ugly and what IMHO will make the code
> unmaintainable.
> 

I did not add the code for index, Herbert ;->
whoever did missed some basic details - I am rectifying that. 
It will be much easir to get rid of the index as a key - at which point
it has no real value  or fix it. You cant have it both ways.

In my opinion the only key that should be used by the manager is the
index. User space should be able to map the selector to an index 
- same with the SAD. Its a lot more manageable that way. But again thats
a separate discussion (amongst a few other issues i have distate for in
the SPD/SAD). This actually is the simple one ;->

> Here is an analogy.  Think of the policies as files and the indicies
> as file descriptors.  The index is only valid while the policy is in
> the policy list (the file descriptor is only valid while the file is
> open).  When you add the policy, you don't get to specify what the
> index is (when you open a file, you don't get to specify what the
> file descriptor is).  

Of course you can specify the index at creation time! it does make sense
to. It's user space that manages these tables - much easier to specify
an index that makes sense to the manager so it can manage its
side of things easier. 
Otherwise all the MIBS and PIBS and policy management code that exists
and uses the concept of row indices in the world is wrong.

> Once the policy has been added, you will be
> given an index that you can use to read/delete the policy (once the
> file has been opened, you will be given a file descriptor that you
> can use to access/close the file).
> 

Good example that serves to make my point actually.
Indeed you can look at the fd as a key that the kernel uses.
The manager of the table is the kernel. The kernel dictates what the
fd could be (with some exceptions) because it is easier that way to
manage the addition, lookup and removal of these files.
 
The SPD, OTOH, is a table of policies managed by some entity in user
space like a KM (and not in the kernel). 
Standard approaches have long ago (probably before you and i were born)
to use indices to manage tables.  Look at SNMP, COPS, etc. There are
some exceptions to this, but this is in cases where they cant be
avoided.

> However, I must say that I still have absolutely no idea why anyone
> would need to set the index to arbitrary values.
> 

But you do know why someone would want to search or delete by it,
right?;->

cheers,
jamal

Re: resend patch: xfrm policybyid

[Herbert Xu]

On Fri, May 06, 2005 at 07:53:00AM -0400, jamal wrote:
> 
> You are a reasonable man, so i am hoping you will end agreeing 
> or agreeing to disagree;->

If it weren't for the fact that the only way of achieving what you
want here is through ugly code then I wouldn't have any problems with
it at all.
 
> > There should only be one key that the
> > user gets to set when adding policies that is guaranteed to be unique.
> > As it is it's the selector.
> 
> Note: The index was already guaranteed to be unique even without my
> patch.

The difference is that the uniqueness is easy when we (the kernel) are
the only ones setting it.  Once you let the user choose the value for
index, that's where the horror starts.

> Just to make sure we are not speaking past each other:
> A key is a column in a table that can be uniquely used to identify said
> row. 

Yep.

> In this case actually the justification exists: The selector is needed
> for data/packet path lookup key. The index for manager side
> manageability. 

I have no argument with the existence of the index per se.  What I am
yet to be convinced of is the need for the user to set its value.
 
> > However, I must say that I still have absolutely no idea why anyone
> > would need to set the index to arbitrary values.
> 
> But you do know why someone would want to search or delete by it,
> right?;->

What difference does it make if you can set the value of index? What
things can't I do if I have to use the index value given by the kernel?

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

Re: resend patch: xfrm policybyid

[jamal]

On Sat, 2005-07-05 at 20:55 +1000, Herbert Xu wrote:
> On Fri, May 06, 2005 at 07:53:00AM -0400, jamal wrote:
> > 
> > You are a reasonable man, so i am hoping you will end agreeing 
> > or agreeing to disagree;->
> 
> If it weren't for the fact that the only way of achieving what you
> want here is through ugly code then I wouldn't have any problems with
> it at all.
>  

Rewrite it Herbert ;-> I have no qualms as long as the feature is
available. I even promised not to harass you ;->


> > Note: The index was already guaranteed to be unique even without my
> > patch.
> 
> The difference is that the uniqueness is easy when we (the kernel) are
> the only ones setting it.  Once you let the user choose the value for
> index, that's where the horror starts.
> 

But the uniqueness is still maintained. Nothing changes there.

>  this case actually the justification exists: The selector is needed
> > for data/packet path lookup key. The index for manager side
> > manageability. 
> 
> I have no argument with the existence of the index per se.  What I am
> yet to be convinced of is the need for the user to set its value.

As i said in the last email, a lot of known management apps out there
typically deal with row indices when managing tables: examples include
SNMP, COPS, etc - and i am not sure how Racoon or pluto store their
policies but they would be a good fit as well. 
If you can specify what indices get used, then you can do some fast
lookups when need to (because you specify then based on how things are
laid out in your current tables). This is the way things are - i am not
trying to innovate anything here. 
How fast you add or delete these rules is a performance metric that
affects your setup/teardown rates.
 
Theres also another use of being able to use indices:  HA
synchronization in active/backup. If the manager wants to make sure the
active and backup are exactly the same and it(the manager) maintains
less amount of data it will ensure that both nodes have exactly the same
indices as well for the same policy.

Having said the above, the SAD as well oughta have an index; infact one
exists (the reqid) but is quiet bizzare. I dont wanna touch the SAD,
yet ;-> not for the next few months until we talk at netconf probably.
Actually this is a digression, so  just ignore i said it ;->

cheers,
jamal

Re: resend patch: xfrm policybyid

[Herbert Xu]

On Sat, May 07, 2005 at 08:38:16AM -0400, jamal wrote:
> 
> Rewrite it Herbert ;-> I have no qualms as long as the feature is
> available. I even promised not to harass you ;->

Sorry, I can't think of any good ways to implement what you want.

> > The difference is that the uniqueness is easy when we (the kernel) are
> > the only ones setting it.  Once you let the user choose the value for
> > index, that's where the horror starts.
> 
> But the uniqueness is still maintained. Nothing changes there.

The difference is that we now have to guarantee the uniqueness of
two unrelated fields during insertion: the index as well as the
selector.  This is where the complexity of your patch comes from.

> As i said in the last email, a lot of known management apps out there
> typically deal with row indices when managing tables: examples include
> SNMP, COPS, etc - and i am not sure how Racoon or pluto store their
> policies but they would be a good fit as well. 
> If you can specify what indices get used, then you can do some fast
> lookups when need to (because you specify then based on how things are
> laid out in your current tables). This is the way things are - i am not
> trying to innovate anything here. 
> How fast you add or delete these rules is a performance metric that
> affects your setup/teardown rates.

I'm afraid I don't buy this.  The policies are not stored in a
linear random-access data structure.  So setting the index
doesn't help performance one tiny bit.

In future it'll probably become a hash table or a tree, in neither
case will there be a linear index that could be used to determine
insertion/deletion.

> Theres also another use of being able to use indices:  HA
> synchronization in active/backup. If the manager wants to make sure the
> active and backup are exactly the same and it(the manager) maintains
> less amount of data it will ensure that both nodes have exactly the same
> indices as well for the same policy.

Please elaborate by giving an example of how the index is actually
used.  Sorry, but as it is I'm too thick to see your point :)

> Having said the above, the SAD as well oughta have an index; infact one
> exists (the reqid) but is quiet bizzare. I dont wanna touch the SAD,
> yet ;-> not for the next few months until we talk at netconf probably.

The reqid is definitely not an index.  In fact it is not unique at all.
It's a way of condensing all the fields in a SA template into one u32.

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

Re: resend patch: xfrm policybyid

[jamal]

On Sun, 2005-08-05 at 18:07 +1000, Herbert Xu wrote:
> On Sat, May 07, 2005 at 08:38:16AM -0400, jamal wrote:
> > 
> > Rewrite it Herbert ;-> I have no qualms as long as the feature is
> > available. I even promised not to harass you ;->
> 
> Sorry, I can't think of any good ways to implement what you want.
> 

There is really nothing in my (small) patch that you have pointed out
that is unresolvable;->
You just dont like the idea, period.

> > > The difference is that the uniqueness is easy when we (the kernel) are
> > > the only ones setting it.  Once you let the user choose the value for
> > > index, that's where the horror starts.
> > 
> > But the uniqueness is still maintained. Nothing changes there.
> 
> The difference is that we now have to guarantee the uniqueness of
> two unrelated fields during insertion: the index as well as the
> selector.  This is where the complexity of your patch comes from.

Both the index and the selector MUST be unique, no question about that.
What i did is introduce setting of the index - and that makes the
checking do a little more. It would be worng not to do the checking.

> I'm afraid I don't buy this.  The policies are not stored in a
> linear random-access data structure.  So setting the index
> doesn't help performance one tiny bit.
> 
> In future it'll probably become a hash table or a tree, in neither
> case will there be a linear index that could be used to determine
> insertion/deletion.
> 

I am talking about user space apps - not the kernel.
Have you ever looked at IPSEC related MIBs and PIBs? or apps that
implement those sorts of entities? I suspect you have not otherwise we
would have closed this ages ago.

> Please elaborate by giving an example of how the index is actually
> used.  Sorry, but as it is I'm too thick to see your point :)
> 

I have given you enough info that i am concluding this is now becoming a
debate for the sake of one;->

Sorry, Herbert, I strongly disagree with your views on this topic. This
is one of those moments when it becomes obvious there can be no
compromise. So I am hoping that someone following this discussion or
writing management apps would speak up.

> > Having said the above, the SAD as well oughta have an index; infact one
> > exists (the reqid) but is quiet bizzare. I dont wanna touch the SAD,
> > yet ;-> not for the next few months until we talk at netconf probably.
> 
> The reqid is definitely not an index.  In fact it is not unique at all.
> It's a way of condensing all the fields in a SA template into one u32.
> 

I dont wanna talk about this right now; 

cheers,
jamal

Re: resend patch: xfrm policybyid

[Patrick McHardy]

jamal wrote:
> On Sun, 2005-08-05 at 18:07 +1000, Herbert Xu wrote:
> 
>>Please elaborate by giving an example of how the index is actually
>>used.  Sorry, but as it is I'm too thick to see your point :)
>
> I have given you enough info that i am concluding this is now becoming a
> debate for the sake of one;->
> 
> Sorry, Herbert, I strongly disagree with your views on this topic. This
> is one of those moments when it becomes obvious there can be no
> compromise. So I am hoping that someone following this discussion or
> writing management apps would speak up.

Allowing the user to freely set indices breaks racoon:

#ifdef __linux__
        /* bsd skips over per-socket policies because there will be no
         * src and dst extensions in spddump messages. On Linux the only
         * way to achieve the same is check for policy id.
         */
        if (xpl->sadb_x_policy_id % 8 >= 3) return 0;
#endif

So how could we handle this?

Regards
Patrick

Re: resend patch: xfrm policybyid

[jamal]

On Sun, 2005-08-05 at 17:23 +0200, Patrick McHardy wrote:

> Allowing the user to freely set indices breaks racoon:
> 
> #ifdef __linux__
>         /* bsd skips over per-socket policies because there will be no
>          * src and dst extensions in spddump messages. On Linux the only
>          * way to achieve the same is check for policy id.
>          */
>         if (xpl->sadb_x_policy_id % 8 >= 3) return 0;
> #endif
> 

I can see where the %8 >= 3 comes from.
[per socket creation with calls xfrm_gen_index(XFRM_POLICY_MAX+dir)
and the kernel does things in increments of 8]

I didnt quiet understand that check in racoon: Why this guess work? if
per-socket policies need to be identified, why dont they get explicitly
marked as per-socket somehow? I am actually curious why that check is
needed. Sorry have never stared at the racoon code. Do other IKE/ISAKMP
daemons depend on it?

> So how could we handle this?
> 

We can disallow the explicit setting of any index which passes test
(index % 8 >= 3) - but it does seem to me the whole concept of reserving
those indices for per-socket policies is a bit of a hack and may need a
rethinking. Maybe we need to maintain a mark in the kernel for
per-socket polices and do the same as BSD?

cheers,
jamal

Re: resend patch: xfrm policybyid

[Patrick McHardy]

jamal wrote:
> I can see where the %8 >= 3 comes from.
> [per socket creation with calls xfrm_gen_index(XFRM_POLICY_MAX+dir)
> and the kernel does things in increments of 8]
> 
> I didnt quiet understand that check in racoon: Why this guess work? if
> per-socket policies need to be identified, why dont they get explicitly
> marked as per-socket somehow? I am actually curious why that check is
> needed. Sorry have never stared at the racoon code. Do other IKE/ISAKMP
> daemons depend on it?

Not sure why they're not marked as per-socket. Probably because
sadb_x_policy_id is a KAME extension and KAME pf_key doesn't dump
these policies with SADB_X_SPDDUMP. Racoon needs to skip them
to avoid adding them to its internal SPD, they could conflict
with global policies.

>>So how could we handle this?
>>
> We can disallow the explicit setting of any index which passes test
> (index % 8 >= 3) - but it does seem to me the whole concept of reserving
> those indices for per-socket policies is a bit of a hack and may need a
> rethinking. Maybe we need to maintain a mark in the kernel for
> per-socket polices and do the same as BSD?

Disallowing this special case seems a bit inconsistent to me. We can
deduce which are per-socket from the list they are contained in. We
don't notify on per-socket policy change, perhaps we should also skip
them when dumping in pf_key.

Regards
Patrick

Re: resend patch: xfrm policybyid

[jamal]

On Mon, 2005-09-05 at 13:45 +0200, Patrick McHardy wrote:

> Not sure why they're not marked as per-socket. Probably because
> sadb_x_policy_id is a KAME extension and KAME pf_key doesn't dump
> these policies with SADB_X_SPDDUMP. Racoon needs to skip them
> to avoid adding them to its internal SPD, they could conflict
> with global policies.
> 

But as you can see without having some KAME extension or explicit flag
it resorts to some hack. I have a feeling they may have to put a
different hack for each OS that is not BSD derived.

> >>So how could we handle this?
> >>
> > We can disallow the explicit setting of any index which passes test
> > (index % 8 >= 3) - but it does seem to me the whole concept of reserving
> > those indices for per-socket policies is a bit of a hack and may need a
> > rethinking. Maybe we need to maintain a mark in the kernel for
> > per-socket polices and do the same as BSD?
> 
> Disallowing this special case seems a bit inconsistent to me. 

Well, those indices are "reserved"  in a sense; so if we can get rid of
that speacial casing even better.

> We can
> deduce which are per-socket from the list they are contained in. We
> don't notify on per-socket policy change, perhaps we should also skip
> them when dumping in pf_key.

this sounds reasonable and would remove the necessity of special-casing
those indices. 

cheers,
jamal

Re: resend patch: xfrm policybyid

[Herbert Xu]

On Thu, May 05, 2005 at 06:17:15PM -0400, jamal wrote:
> 
> BTW, are the event patches in Davems queue somewhere?
> I have a few ipsec ones (cosmetic) that i will send later.

I can queue them up for you and bounce them to Dave once 2.6.13
opens up.

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

Re: resend patch: xfrm policybyid

[jamal]

On Fri, 2005-06-05 at 21:04 +1000, Herbert Xu wrote:

> I can queue them up for you and bounce them to Dave once 2.6.13
> opens up.

I would appreciate that - i am famous for losing patches. Now queue that
policybyid patch too ;->

cheers,
jamal