PATCH: IPSEC xfrm events

PATCH: IPSEC xfrm events

[jamal]

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

Herbert et al,

Ok, heres the final patch with all the changes discussed.

 include/linux/xfrm.h   |    2 
 include/net/xfrm.h     |   29 ++++++-
 net/key/af_key.c       |   24 +++++-
 net/xfrm/xfrm_policy.c |   25 ++++--
 net/xfrm/xfrm_state.c  |   84 +++++++++++++++++++--
 net/xfrm/xfrm_user.c   |  188
++++++++++++++++++++++++++++++++++++++++++++++++-
 6 files changed, 323 insertions(+), 29 deletions(-)

I have tested this with both setkey and iproute2 (about 10 scenarios or
so). Masahide-san is doing a lot more thorough testing with key servers
as well. He has not tested this patch yet (time difference) but it is
based on the last one he tested.

Let me know what you think. If no issues i would like to push to Dave.
There are comments in the patch that are intentional so they can be
commented on.

cheers,
jamal

[-- Attachment #2: ipsec-xfrm-ev-p --]
[-- Type: text/plain, Size: 15659 bytes --]

--- a/include/net/xfrm.h	2005-03-25 22:28:26.000000000 -0500
+++ b/include/net/xfrm.h	2005-03-31 19:26:24.000000000 -0500
@@ -157,6 +157,25 @@
 	XFRM_STATE_DEAD
 };
 
+/* events that could be sent by kernel */
+enum {
+	XFRM_SAP_INVALID,
+	XFRM_SAP_EXPIRED,
+	XFRM_SAP_ADDED,
+	XFRM_SAP_UPDATED,
+	XFRM_SAP_DELETED,
+	XFRM_SAP_FLUSHED,
+	__XFRM_SAP_MAX
+};
+#define XFRM_SAP_MAX (__XFRM_SAP_MAX - 1)
+
+/* callback structure passed from either netlink or pfkey */
+struct km_cb
+{
+	u32	data; /* callee to caller */
+};
+
+
 struct xfrm_type;
 struct xfrm_dst;
 struct xfrm_policy_afinfo {
@@ -178,6 +197,8 @@
 
 extern int xfrm_policy_register_afinfo(struct xfrm_policy_afinfo *afinfo);
 extern int xfrm_policy_unregister_afinfo(struct xfrm_policy_afinfo *afinfo);
+extern void km_policy_notify(struct xfrm_policy *xp, int dir, int event, struct km_cb *c);
+
 
 #define XFRM_ACQ_EXPIRES	30
 
@@ -283,17 +304,17 @@
 	struct xfrm_tmpl       	xfrm_vec[XFRM_MAX_DEPTH];
 };
 
-#define XFRM_KM_TIMEOUT		30
 
+#define XFRM_KM_TIMEOUT		30
 struct xfrm_mgr
 {
 	struct list_head	list;
 	char			*id;
-	int			(*notify)(struct xfrm_state *x, int event);
+	int			(*notify)(struct xfrm_state *x, int event, struct km_cb *c);
 	int			(*acquire)(struct xfrm_state *x, struct xfrm_tmpl *, struct xfrm_policy *xp, int dir);
 	struct xfrm_policy	*(*compile_policy)(u16 family, int opt, u8 *data, int len, int *dir);
 	int			(*new_mapping)(struct xfrm_state *x, xfrm_address_t *ipaddr, u16 sport);
-	int			(*notify_policy)(struct xfrm_policy *x, int dir, int event);
+	int			(*notify_policy)(struct xfrm_policy *x, int dir, int event, struct km_cb *c);
 };
 
 extern int xfrm_register_km(struct xfrm_mgr *km);
@@ -860,7 +881,7 @@
 
 extern wait_queue_head_t km_waitq;
 extern int km_new_mapping(struct xfrm_state *x, xfrm_address_t *ipaddr, u16 sport);
-extern void km_policy_expired(struct xfrm_policy *pol, int dir, int hard);
+extern void km_policy_expired(struct xfrm_policy *pol, int dir, int event);
 
 extern void xfrm_input_init(void);
 extern int xfrm_parse_spi(struct sk_buff *skb, u8 nexthdr, u32 *spi, u32 *seq);
--- a/include/linux/xfrm.h	2005-03-25 22:28:39.000000000 -0500
+++ b/include/linux/xfrm.h	2005-03-31 19:26:24.000000000 -0500
@@ -254,5 +254,7 @@
 
 #define XFRMGRP_ACQUIRE		1
 #define XFRMGRP_EXPIRE		2
+#define XFRMGRP_SA		4
+#define XFRMGRP_POLICY		8
 
 #endif /* _LINUX_XFRM_H */
--- a/net/xfrm/xfrm_state.c	2005-03-25 22:28:25.000000000 -0500
+++ b/net/xfrm/xfrm_state.c	2005-03-31 19:26:24.000000000 -0500
@@ -239,10 +239,56 @@
 	}
 }
 
+static DEFINE_RWLOCK(xfrm_km_lock);
+static struct list_head xfrm_km_list = LIST_HEAD_INIT(xfrm_km_list);
+
+void km_policy_notify(struct xfrm_policy *xp, int dir, int event, struct km_cb *c)
+{
+	struct xfrm_mgr *km;
+
+	read_lock(&xfrm_km_lock);
+	list_for_each_entry(km, &xfrm_km_list, list)
+		if (km && km->notify_policy)
+			km->notify_policy(xp, dir, event, c);
+	read_unlock(&xfrm_km_lock);
+}
+EXPORT_SYMBOL(km_policy_notify);
+
+void km_state_notify(struct xfrm_state *x, struct km_cb *c, int event)
+{
+	struct xfrm_mgr *km;
+	read_lock(&xfrm_km_lock);
+	list_for_each_entry(km, &xfrm_km_list, list)
+		km->notify(x, event, c);
+	read_unlock(&xfrm_km_lock);
+}
+
+void xfrm_state_del_flush(struct xfrm_state *x)
+{
+	spin_lock_bh(&x->lock);
+	__xfrm_state_delete(x);
+	spin_unlock_bh(&x->lock);
+}
+
 void xfrm_state_delete(struct xfrm_state *x)
 {
+	int notif = 0;
 	spin_lock_bh(&x->lock);
+	/* 
+	 * its unfortunate we have to freeze gc for this
+	 * one moment - the other alternative would involve
+	 * memcopying the state and then announcing that.
+	 * think SMP where theres an iota where this could mess
+	 * up - JHS
+	*/
+	spin_lock_bh(&xfrm_state_gc_lock);
+	if (x->km.state != XFRM_STATE_DEAD)
+		notif = 1;
 	__xfrm_state_delete(x);
+
+	if (notif)
+		km_state_notify(x, NULL, XFRM_SAP_DELETED);
+	spin_unlock_bh(&xfrm_state_gc_lock);
 	spin_unlock_bh(&x->lock);
 }
 EXPORT_SYMBOL(xfrm_state_delete);
@@ -251,6 +297,8 @@
 {
 	int i;
 	struct xfrm_state *x;
+	struct km_cb c;
+	int count = 0;
 
 	spin_lock_bh(&xfrm_state_lock);
 	for (i = 0; i < XFRM_DST_HSIZE; i++) {
@@ -261,7 +309,8 @@
 				xfrm_state_hold(x);
 				spin_unlock_bh(&xfrm_state_lock);
 
-				xfrm_state_delete(x);
+				xfrm_state_del_flush(x);
+				count++;
 				xfrm_state_put(x);
 
 				spin_lock_bh(&xfrm_state_lock);
@@ -270,6 +319,10 @@
 		}
 	}
 	spin_unlock_bh(&xfrm_state_lock);
+	if (count) {
+		c.data = proto;
+		km_state_notify(NULL, &c, XFRM_SAP_FLUSHED);
+	}
 	wake_up(&km_waitq);
 }
 EXPORT_SYMBOL(xfrm_state_flush);
@@ -402,6 +455,7 @@
 
 static struct xfrm_state *__xfrm_find_acq_byseq(u32 seq);
 
+
 int xfrm_state_add(struct xfrm_state *x)
 {
 	struct xfrm_state_afinfo *afinfo;
@@ -438,6 +492,7 @@
 			&x->id.daddr, &x->props.saddr, 0);
 
 	__xfrm_state_insert(x);
+	km_state_notify(x, NULL, XFRM_SAP_ADDED);
 	err = 0;
 
 out:
@@ -478,6 +533,7 @@
 
 	if (x1->km.state == XFRM_STATE_ACQ) {
 		__xfrm_state_insert(x);
+		km_state_notify(x, NULL, XFRM_SAP_ADDED);
 		x = NULL;
 	}
 	err = 0;
@@ -509,6 +565,7 @@
 			xfrm_state_check_expire(x1);
 
 		err = 0;
+		km_state_notify(x, NULL, XFRM_SAP_UPDATED);
 	}
 	spin_unlock_bh(&x1->lock);
 
@@ -764,37 +821,41 @@
 }
 EXPORT_SYMBOL(xfrm_replay_advance);
 
-static struct list_head xfrm_km_list = LIST_HEAD_INIT(xfrm_km_list);
-static DEFINE_RWLOCK(xfrm_km_lock);
 
 static void km_state_expired(struct xfrm_state *x, int hard)
 {
 	struct xfrm_mgr *km;
+	struct km_cb c;
 
 	if (hard)
 		x->km.state = XFRM_STATE_EXPIRED;
 	else
 		x->km.dying = 1;
 
+	c.data = hard;
 	read_lock(&xfrm_km_lock);
 	list_for_each_entry(km, &xfrm_km_list, list)
-		km->notify(x, hard);
+		km->notify(x, XFRM_SAP_EXPIRED, &c);
 	read_unlock(&xfrm_km_lock);
 
 	if (hard)
 		wake_up(&km_waitq);
 }
 
+/*
+ * We send to all registered managers regardless of failure
+ * We are happy with one success
+*/
 static int km_query(struct xfrm_state *x, struct xfrm_tmpl *t, struct xfrm_policy *pol)
 {
-	int err = -EINVAL;
+	int err = -EINVAL, acqret;
 	struct xfrm_mgr *km;
 
 	read_lock(&xfrm_km_lock);
 	list_for_each_entry(km, &xfrm_km_list, list) {
-		err = km->acquire(x, t, pol, XFRM_POLICY_OUT);
-		if (!err)
-			break;
+		acqret = km->acquire(x, t, pol, XFRM_POLICY_OUT);
+		if (!acqret)
+			err = acqret;
 	}
 	read_unlock(&xfrm_km_lock);
 	return err;
@@ -820,11 +881,13 @@
 void km_policy_expired(struct xfrm_policy *pol, int dir, int hard)
 {
 	struct xfrm_mgr *km;
+	struct km_cb c;
 
 	read_lock(&xfrm_km_lock);
+	c.data = hard;
 	list_for_each_entry(km, &xfrm_km_list, list)
 		if (km->notify_policy)
-			km->notify_policy(pol, dir, hard);
+			km->notify_policy(pol, dir, XFRM_SAP_EXPIRED, &c);
 	read_unlock(&xfrm_km_lock);
 
 	if (hard)
@@ -957,8 +1020,9 @@
 	if (x->tunnel) {
 		struct xfrm_state *t = x->tunnel;
 
+		/* XXX: Avoid announce?? */
 		if (atomic_read(&t->tunnel_users) == 2)
-			xfrm_state_delete(t);
+			xfrm_state_del_flush(t);
 		atomic_dec(&t->tunnel_users);
 		xfrm_state_put(t);
 		x->tunnel = NULL;
--- a/net/xfrm/xfrm_policy.c	2005-03-25 22:28:21.000000000 -0500
+++ b/net/xfrm/xfrm_policy.c	2005-03-31 19:26:24.000000000 -0500
@@ -298,7 +298,7 @@
  * entry dead. The rule must be unlinked from lists to the moment.
  */
 
-static void xfrm_policy_kill(struct xfrm_policy *policy)
+static void xfrm_policy_kill(struct xfrm_policy *policy, int dir, int notif)
 {
 	write_lock_bh(&policy->lock);
 	if (policy->dead)
@@ -307,6 +307,9 @@
 	policy->dead = 1;
 
 	spin_lock(&xfrm_policy_gc_lock);
+	if (notif) {
+		km_policy_notify(policy, dir, XFRM_SAP_DELETED, NULL);
+	}
 	list_add(&policy->list, &xfrm_policy_gc_list);
 	spin_unlock(&xfrm_policy_gc_lock);
 	schedule_work(&xfrm_policy_gc_work);
@@ -375,10 +378,11 @@
 	policy->curlft.use_time = 0;
 	if (!mod_timer(&policy->timer, jiffies + HZ))
 		xfrm_pol_hold(policy);
+	km_policy_notify(policy, dir, XFRM_SAP_ADDED, NULL);
 	write_unlock_bh(&xfrm_policy_lock);
 
 	if (delpol) {
-		xfrm_policy_kill(delpol);
+		xfrm_policy_kill(delpol, dir, 1);
 	}
 	return 0;
 }
@@ -402,7 +406,7 @@
 
 	if (pol && delete) {
 		atomic_inc(&flow_cache_genid);
-		xfrm_policy_kill(pol);
+		xfrm_policy_kill(pol, dir, 1);
 	}
 	return pol;
 }
@@ -425,16 +429,16 @@
 
 	if (pol && delete) {
 		atomic_inc(&flow_cache_genid);
-		xfrm_policy_kill(pol);
+		xfrm_policy_kill(pol, dir, 1);
 	}
 	return pol;
 }
 EXPORT_SYMBOL(xfrm_policy_byid);
 
-void xfrm_policy_flush(void)
+void xfrm_policy_flush()
 {
 	struct xfrm_policy *xp;
-	int dir;
+	int dir, count = 0;
 
 	write_lock_bh(&xfrm_policy_lock);
 	for (dir = 0; dir < XFRM_POLICY_MAX; dir++) {
@@ -442,12 +446,15 @@
 			xfrm_policy_list[dir] = xp->next;
 			write_unlock_bh(&xfrm_policy_lock);
 
-			xfrm_policy_kill(xp);
+			xfrm_policy_kill(xp, dir, 0);
+			count++;
 
 			write_lock_bh(&xfrm_policy_lock);
 		}
 	}
 	atomic_inc(&flow_cache_genid);
+	if (count)
+		km_policy_notify(NULL, 0, XFRM_SAP_FLUSHED, NULL);
 	write_unlock_bh(&xfrm_policy_lock);
 }
 EXPORT_SYMBOL(xfrm_policy_flush);
@@ -558,7 +565,7 @@
 	if (pol) {
 		if (dir < XFRM_POLICY_MAX)
 			atomic_inc(&flow_cache_genid);
-		xfrm_policy_kill(pol);
+		xfrm_policy_kill(pol, dir, 1);
 	}
 }
 
@@ -579,7 +586,7 @@
 	write_unlock_bh(&xfrm_policy_lock);
 
 	if (old_pol) {
-		xfrm_policy_kill(old_pol);
+		xfrm_policy_kill(old_pol, dir, 1);
 	}
 	return 0;
 }
--- a/net/xfrm/xfrm_user.c	2005-03-25 22:28:22.000000000 -0500
+++ b/net/xfrm/xfrm_user.c	2005-03-31 19:26:24.000000000 -0500
@@ -683,6 +683,10 @@
 	if (!xp)
 		return err;
 
+	/* shouldnt excl be based on nlh flags?? 
+	 * Aha! this is anti-netlink really i.e  more pfkey derived
+	 * in netlink excl is a flag and you wouldnt need
+	 * a type XFRM_MSG_UPDPOLICY - JHS */
 	excl = nlh->nlmsg_type == XFRM_MSG_NEWPOLICY;
 	err = xfrm_policy_insert(p->dir, xp, excl);
 	if (err) {
@@ -1053,10 +1057,10 @@
 	return -1;
 }
 
-static int xfrm_send_state_notify(struct xfrm_state *x, int hard)
+static int xfrm_exp_state_notify(struct xfrm_state *x, u32 hard)
 {
 	struct sk_buff *skb;
-
+	/* fix to do alloc using NLM macros */
 	skb = alloc_skb(sizeof(struct xfrm_user_expire) + 16, GFP_ATOMIC);
 	if (skb == NULL)
 		return -ENOMEM;
@@ -1069,6 +1073,98 @@
 	return netlink_broadcast(xfrm_nl, skb, 0, XFRMGRP_EXPIRE, GFP_ATOMIC);
 }
 
+static int xfrm_notify_sa_flush(struct km_cb *c)
+{
+	struct xfrm_usersa_flush *p;
+	struct nlmsghdr *nlh;
+	struct sk_buff *skb;
+	unsigned char *b;
+	u32 ppid = 0;
+	int len = NLMSG_LENGTH(sizeof(struct xfrm_usersa_flush));
+
+	skb = alloc_skb(len, GFP_ATOMIC);
+	if (skb == NULL)
+		return -ENOMEM;
+	b = skb->tail;
+
+	nlh = NLMSG_PUT(skb, ppid, jiffies,
+			XFRM_MSG_FLUSHSA, sizeof(*p));
+	nlh->nlmsg_flags = 0;
+
+	p = NLMSG_DATA(nlh);
+	if (!c) {
+		printk("xfrm_notify_sa_flush NULL km cb\n");
+		p->proto = 0;
+	} else {
+		p->proto = c->data;
+	}
+
+	nlh->nlmsg_len = skb->tail - b;
+
+	return netlink_broadcast(xfrm_nl, skb, ppid, XFRMGRP_SA, GFP_ATOMIC);
+
+nlmsg_failure:
+	kfree_skb(skb);
+	return -1;
+}
+
+static int xfrm_notify_sa( struct xfrm_state *x, int event, struct km_cb *c)
+{
+	struct xfrm_usersa_info *p;
+	struct nlmsghdr *nlh;
+	struct sk_buff *skb;
+	u32 nlt;
+	unsigned char *b;
+	u32 ppid = 0;
+	int len = NLMSG_LENGTH(sizeof(struct xfrm_usersa_info));
+
+	skb = alloc_skb(len, GFP_ATOMIC);
+	if (skb == NULL)
+		return -ENOMEM;
+	b = skb->tail;
+
+	if (event == XFRM_SAP_ADDED)
+		nlt = XFRM_MSG_NEWSA;
+	else if (event == XFRM_SAP_UPDATED)
+		nlt = XFRM_MSG_UPDSA;
+	else if (event == XFRM_SAP_DELETED)
+		nlt = XFRM_MSG_DELSA;
+	else
+		goto nlmsg_failure;
+
+	nlh = NLMSG_PUT(skb, ppid, jiffies,
+			nlt, sizeof(*p));
+	nlh->nlmsg_flags = 0;
+
+	p = NLMSG_DATA(nlh);
+	copy_to_user_state(x, p);
+
+	nlh->nlmsg_len = skb->tail - b;
+
+	return netlink_broadcast(xfrm_nl, skb, ppid, XFRMGRP_SA, GFP_ATOMIC);
+
+nlmsg_failure:
+	kfree_skb(skb);
+	return -1;
+}
+
+static int xfrm_send_state_notify(struct xfrm_state *x, int event, struct km_cb *c)
+{
+
+	if ((event == XFRM_SAP_ADDED) || 
+		(event == XFRM_SAP_UPDATED) ||
+		(event == XFRM_SAP_DELETED))
+		return xfrm_notify_sa(x, event, c);
+
+	if (event == XFRM_SAP_FLUSHED) 
+		xfrm_notify_sa_flush(c);
+
+	if (event != XFRM_SAP_EXPIRED)
+		return 0;
+
+	return xfrm_exp_state_notify(x, c->data);
+}
+
 static int build_acquire(struct sk_buff *skb, struct xfrm_state *x,
 			 struct xfrm_tmpl *xt, struct xfrm_policy *xp,
 			 int dir)
@@ -1202,7 +1298,8 @@
 	return -1;
 }
 
-static int xfrm_send_policy_notify(struct xfrm_policy *xp, int dir, int hard)
+
+static int xfrm_exp_policy_notify(struct xfrm_policy *xp, int dir, int hard)
 {
 	struct sk_buff *skb;
 	size_t len;
@@ -1221,6 +1318,91 @@
 	return netlink_broadcast(xfrm_nl, skb, 0, XFRMGRP_EXPIRE, GFP_ATOMIC);
 }
 
+static int xfrm_notify_policy( struct xfrm_policy *xp, int dir, int event, struct km_cb *c)
+{
+	struct xfrm_userpolicy_info *p;
+	struct nlmsghdr *nlh;
+	struct sk_buff *skb;
+	u32 nlt = 0 ;
+	unsigned char *b;
+	int len = NLMSG_LENGTH(sizeof(struct xfrm_userpolicy_info));
+	u32 ppid = 0;
+
+	skb = alloc_skb(len, GFP_ATOMIC);
+	if (skb == NULL)
+		return -ENOMEM;
+	b = skb->tail;
+
+	if (event == XFRM_SAP_ADDED)
+		nlt = XFRM_MSG_NEWPOLICY;
+	else if (event == XFRM_SAP_UPDATED)
+		nlt = XFRM_MSG_UPDPOLICY;
+	else if (event == XFRM_SAP_DELETED)
+		nlt = XFRM_MSG_DELPOLICY;
+	else
+		goto nlmsg_failure;
+
+	nlh = NLMSG_PUT(skb, ppid, jiffies, nlt, sizeof(*p));
+
+	p = NLMSG_DATA(nlh);
+
+	nlh->nlmsg_flags = 0;
+
+	copy_to_user_policy(xp, p, dir);
+
+	nlh->nlmsg_len = skb->tail - b;
+
+	return netlink_broadcast(xfrm_nl, skb, ppid, XFRMGRP_POLICY, GFP_ATOMIC);
+
+nlmsg_failure:
+	kfree_skb(skb);
+	return -1;
+}
+
+static int xfrm_notify_policy_flush(struct km_cb *c)
+{
+	struct nlmsghdr *nlh;
+	struct sk_buff *skb;
+	unsigned char *b;
+	u32 ppid = 0;
+	int len = NLMSG_LENGTH(0);
+
+	skb = alloc_skb(len, GFP_ATOMIC);
+	if (skb == NULL)
+		return -ENOMEM;
+	b = skb->tail;
+
+
+	nlh = NLMSG_PUT(skb, 0, jiffies, XFRM_MSG_FLUSHPOLICY, 0);
+
+	nlh->nlmsg_len = skb->tail - b;
+
+	return netlink_broadcast(xfrm_nl, skb, ppid, XFRMGRP_POLICY, GFP_ATOMIC);
+
+nlmsg_failure:
+	kfree_skb(skb);
+	return -1;
+}
+
+static int xfrm_send_policy_notify(struct xfrm_policy *xp, int dir, int event, struct km_cb *c)
+{
+
+	if ((event == XFRM_SAP_ADDED) || 
+		(event == XFRM_SAP_UPDATED) ||
+		(event == XFRM_SAP_DELETED))
+		return xfrm_notify_policy(xp, dir, event, c);
+
+	if (event == XFRM_SAP_FLUSHED) 
+		return xfrm_notify_policy_flush(c);
+
+	if (event != XFRM_SAP_EXPIRED) {
+		printk("Netlink Unknown Policy event %d\n",event);
+		return 0;
+	}
+
+	return xfrm_exp_policy_notify(xp, dir, c->data);
+}
+
 static struct xfrm_mgr netlink_mgr = {
 	.id		= "netlink",
 	.notify		= xfrm_send_state_notify,
--- a/net/key/af_key.c	2005-03-25 22:28:39.000000000 -0500
+++ b/net/key/af_key.c	2005-03-31 19:26:24.000000000 -0500
@@ -1240,7 +1240,6 @@
 	return 0;
 }
 
-
 static int pfkey_add(struct sock *sk, struct sk_buff *skb, struct sadb_msg *hdr, void **ext_hdrs)
 {
 	struct sk_buff *out_skb;
@@ -2317,11 +2316,30 @@
 	}
 }
 
-static int pfkey_send_notify(struct xfrm_state *x, int hard)
+static int pfkey_send_notify(struct xfrm_state *x, int event, struct km_cb *c)
 {
 	struct sk_buff *out_skb;
 	struct sadb_msg *out_hdr;
-	int hsc = (hard ? 2 : 1);
+	int hard;
+	int hsc;
+
+	/* 
+	 * migrate pf_key later - for now only support is for
+	 * expire events 
+	*/
+	if (event != XFRM_SAP_EXPIRED)
+		return 0; 
+
+	if (!c) {
+		printk("pfkey_send_notify: bad CB data!\n");
+		return 0;
+	}
+
+	hard = c->data;
+	if (hard)
+		hsc = 2;
+	else
+		hsc = 1;
 
 	out_skb = pfkey_xfrm_state2msg(x, 0, hsc);
 	if (IS_ERR(out_skb))

Re: PATCH: IPSEC xfrm events

[Herbert Xu]

On Thu, Mar 31, 2005 at 08:37:21PM -0500, jamal wrote:
> 
> Ok, heres the final patch with all the changes discussed.

Thanks Jamal.  The patch looks good overall.  However, the
delete/flush code is new so I've got something to say again :)

> --- a/include/net/xfrm.h	2005-03-25 22:28:26.000000000 -0500
> +++ b/include/net/xfrm.h	2005-03-31 19:26:24.000000000 -0500
>
> +/* callback structure passed from either netlink or pfkey */
> +struct km_cb

This name is a bit non-specific.

> +{
> +	u32	data; /* callee to caller */
> +};

Might as well put the event into it if we're going to keep this
structure.  It'll help to shorten the function prototypes that
use it.

And then we can just call this structure km_event.

> -extern void km_policy_expired(struct xfrm_policy *pol, int dir, int hard);
> +extern void km_policy_expired(struct xfrm_policy *pol, int dir, int event);

Bogus prototype change.

> +void xfrm_state_del_flush(struct xfrm_state *x)
> +{
> +	spin_lock_bh(&x->lock);
> +	__xfrm_state_delete(x);
> +	spin_unlock_bh(&x->lock);
> +}

Sorry, I've changed my mind on this.  This demonstrates why the
km_notify_* calls should be made from af_key/xfrm_user directly
instead of here.

Some of these functions are called internally as you discovered.
Since the notifications should only be generated by user requests,
calls to km_notify_* should be made at the places where the user
requests are handled, which is in the KM itself.

Otherwise we'll have to add hacks like this to avoid the
notification for internal users.

>  void xfrm_state_delete(struct xfrm_state *x)
>  {
> +	int notif = 0;
>  	spin_lock_bh(&x->lock);
> +	/* 
> +	 * its unfortunate we have to freeze gc for this
> +	 * one moment - the other alternative would involve
> +	 * memcopying the state and then announcing that.
> +	 * think SMP where theres an iota where this could mess
> +	 * up - JHS
> +	*/
> +	spin_lock_bh(&xfrm_state_gc_lock);
> +	if (x->km.state != XFRM_STATE_DEAD)
> +		notif = 1;
>  	__xfrm_state_delete(x);
> +
> +	if (notif)
> +		km_state_notify(x, NULL, XFRM_SAP_DELETED);

You've caught a real bug for af_key here.  It's currently possible to
receive two delete notifications for the same state.

However, may I suggest that we code this differently.  Make
__xfrm_state_delete return 0 if the state was really deleted
and -ESRCH otherwise.

Then af_key/xfrm_user can simply call km_state_notify if the
return value was zero.

BTW there is no need to grab xfrm_state_gc_lock.  You've got
a reference count on the state from your caller.

> @@ -270,6 +319,10 @@
>  		}
>  	}
>  	spin_unlock_bh(&xfrm_state_lock);
> +	if (count) {
> +		c.data = proto;
> +		km_state_notify(NULL, &c, XFRM_SAP_FLUSHED);
> +	}

The notification should occur in all cases, even if count == 0.

> @@ -957,8 +1020,9 @@
>  	if (x->tunnel) {
>  		struct xfrm_state *t = x->tunnel;
>  
> +		/* XXX: Avoid announce?? */
>  		if (atomic_read(&t->tunnel_users) == 2)
> -			xfrm_state_delete(t);
> +			xfrm_state_del_flush(t);

That's right.  We don't want to announce internal states to the world.

> --- a/net/xfrm/xfrm_policy.c	2005-03-25 22:28:21.000000000 -0500
> +++ b/net/xfrm/xfrm_policy.c	2005-03-31 19:26:24.000000000 -0500
> @@ -298,7 +298,7 @@
>   * entry dead. The rule must be unlinked from lists to the moment.
>   */
>  
> -static void xfrm_policy_kill(struct xfrm_policy *policy)
> +static void xfrm_policy_kill(struct xfrm_policy *policy, int dir, int notif)

Again, had you done the km_* calls from af_key/xfrm_user, then there'd
be no need to check notif here.

BTW, as it is you're announcing expired policies twice.  Once as an
expire event and once as a delete event.  This problem will also go
away if you move the km_* calls into af_key/xfrm_user.

> @@ -579,7 +586,7 @@
>  	write_unlock_bh(&xfrm_policy_lock);
>  
>  	if (old_pol) {
> -		xfrm_policy_kill(old_pol);
> +		xfrm_policy_kill(old_pol, dir, 1);
>  	}

Please don't announce socket policies :)

> --- a/net/xfrm/xfrm_user.c	2005-03-25 22:28:22.000000000 -0500
> +++ b/net/xfrm/xfrm_user.c	2005-03-31 19:26:24.000000000 -0500
> @@ -683,6 +683,10 @@
>  	if (!xp)
>  		return err;
>  
> +	/* shouldnt excl be based on nlh flags?? 
> +	 * Aha! this is anti-netlink really i.e  more pfkey derived
> +	 * in netlink excl is a flag and you wouldnt need
> +	 * a type XFRM_MSG_UPDPOLICY - JHS */

Good point.  Care to provide a patch to treat NEW + NLM_F_REPLACE
as UPD?

> @@ -1053,10 +1057,10 @@
>  	return -1;
>  }
>  
> -static int xfrm_send_state_notify(struct xfrm_state *x, int hard)
> +static int xfrm_exp_state_notify(struct xfrm_state *x, u32 hard)

How about calling this xfrm_notify_sa_expired for consistency?
Ditto for the policy function.
  
> +static int xfrm_notify_sa_flush(struct km_cb *c)
> +{
> +	struct xfrm_usersa_flush *p;
> +	struct nlmsghdr *nlh;
> +	struct sk_buff *skb;
> +	unsigned char *b;
> +	u32 ppid = 0;
> +	int len = NLMSG_LENGTH(sizeof(struct xfrm_usersa_flush));
> +
> +	skb = alloc_skb(len, GFP_ATOMIC);
> +	if (skb == NULL)
> +		return -ENOMEM;
> +	b = skb->tail;
> +
> +	nlh = NLMSG_PUT(skb, ppid, jiffies,

If we're serious about providing sequence numbers then please
set it up as an atomic integer and use it throughout this file.

Otherwise just pop zero in there.

> +	p = NLMSG_DATA(nlh);
> +	if (!c) {
> +		printk("xfrm_notify_sa_flush NULL km cb\n");
> +		p->proto = 0;

Is anyone expected to call this with a NULL pointer? If not then
just let it OOPS.  Same comment applies to the cb checks later on.

> +static int xfrm_notify_sa( struct xfrm_state *x, int event, struct km_cb *c)

> +	if (event == XFRM_SAP_ADDED)
> +		nlt = XFRM_MSG_NEWSA;
> +	else if (event == XFRM_SAP_UPDATED)
> +		nlt = XFRM_MSG_UPDSA;
> +	else if (event == XFRM_SAP_DELETED)
> +		nlt = XFRM_MSG_DELSA;
> +	else
> +		goto nlmsg_failure;

Please use a switch.

> +static int xfrm_send_state_notify(struct xfrm_state *x, int event, struct km_cb *c)
> +{
> +
> +	if ((event == XFRM_SAP_ADDED) || 
> +		(event == XFRM_SAP_UPDATED) ||
> +		(event == XFRM_SAP_DELETED))
> +		return xfrm_notify_sa(x, event, c);
> +
> +	if (event == XFRM_SAP_FLUSHED) 
> +		xfrm_notify_sa_flush(c);
> +
> +	if (event != XFRM_SAP_EXPIRED)
> +		return 0;

Again a switch would be perfect.

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: PATCH: IPSEC xfrm events

[jamal]

On Thu, 2005-03-31 at 23:21, Herbert Xu wrote:
> On Thu, Mar 31, 2005 at 08:37:21PM -0500, jamal wrote:

> 
> > --- a/include/net/xfrm.h	2005-03-25 22:28:26.000000000 -0500
> > +++ b/include/net/xfrm.h	2005-03-31 19:26:24.000000000 -0500
> >
> > +/* callback structure passed from either netlink or pfkey */
> > +struct km_cb
> 
> This name is a bit non-specific.
> 

note: used by both SP/SA

> > +{
> > +	u32	data; /* callee to caller */
> > +};
> 
> Might as well put the event into it if we're going to keep this
> structure.  It'll help to shorten the function prototypes that
> use it.
> 
> And then we can just call this structure km_event.
> 

sure.

> > -extern void km_policy_expired(struct xfrm_policy *pol, int dir, int hard);
> > +extern void km_policy_expired(struct xfrm_policy *pol, int dir, int event);
> 
> Bogus prototype change.
> 

agreed.

> > +void xfrm_state_del_flush(struct xfrm_state *x)
> > +{
> > +	spin_lock_bh(&x->lock);
> > +	__xfrm_state_delete(x);
> > +	spin_unlock_bh(&x->lock);
> > +}
> 
> Sorry, I've changed my mind on this.  This demonstrates why the
> km_notify_* calls should be made from af_key/xfrm_user directly
> instead of here.
> 
>
> Some of these functions are called internally as you discovered.
> Since the notifications should only be generated by user requests,
> calls to km_notify_* should be made at the places where the user
> requests are handled, which is in the KM itself.
> 

You need to be able to generate events at every km not just the one that
generated the request. You also (most of the time) need to do it before
affected object dissapears. So I am missing your point on this one.

> Otherwise we'll have to add hacks like this to avoid the
> notification for internal users.
> 

I may be paranoid but i do this because x could be garbage collected way
before i send the km user message - and i need it to use it to generate
the event. I could take a copy of it ... 

> >  void xfrm_state_delete(struct xfrm_state *x)
> >  {
> > +	int notif = 0;
> >  	spin_lock_bh(&x->lock);
> > +	/* 
> > +	 * its unfortunate we have to freeze gc for this
> > +	 * one moment - the other alternative would involve
> > +	 * memcopying the state and then announcing that.
> > +	 * think SMP where theres an iota where this could mess
> > +	 * up - JHS
> > +	*/
> > +	spin_lock_bh(&xfrm_state_gc_lock);
> > +	if (x->km.state != XFRM_STATE_DEAD)
> > +		notif = 1;
> >  	__xfrm_state_delete(x);
> > +
> > +	if (notif)
> > +		km_state_notify(x, NULL, XFRM_SAP_DELETED);
> 
> You've caught a real bug for af_key here.  It's currently possible to
> receive two delete notifications for the same state.

Can you elaborate?

> However, may I suggest that we code this differently.  Make
> __xfrm_state_delete return 0 if the state was really deleted
> and -ESRCH otherwise.
> 
> Then af_key/xfrm_user can simply call km_state_notify if the
> return value was zero.
> 

Again like i said: I need to tell every km user about the event, not
just the originator.

> BTW there is no need to grab xfrm_state_gc_lock.  You've got
> a reference count on the state from your caller.
> 

Aha! I missed that - I will remove it.

> > @@ -270,6 +319,10 @@
> >  		}
> >  	}
> >  	spin_unlock_bh(&xfrm_state_lock);
> > +	if (count) {
> > +		c.data = proto;
> > +		km_state_notify(NULL, &c, XFRM_SAP_FLUSHED);
> > +	}
> 
> The notification should occur in all cases, even if count == 0.
> 


Well, Masahide-San and I actually did discuss this and he was of the
same opinion as you. My opinion: We only generate events when something
happens, not just because someone issues a command. If flush was issued
and there was nothing to flush why generate an event? does the PFKEY RFC
say anything on this?

> > @@ -957,8 +1020,9 @@
> >  	if (x->tunnel) {
> >  		struct xfrm_state *t = x->tunnel;
> >  
> > +		/* XXX: Avoid announce?? */
> >  		if (atomic_read(&t->tunnel_users) == 2)
> > -			xfrm_state_delete(t);
> > +			xfrm_state_del_flush(t);
> 
> That's right.  We don't want to announce internal states to the world.
> 

I will remove that comment. Thats achieved in the above code although
the called funtion may not have the appropriate name .

> > --- a/net/xfrm/xfrm_policy.c	2005-03-25 22:28:21.000000000 -0500
> > +++ b/net/xfrm/xfrm_policy.c	2005-03-31 19:26:24.000000000 -0500
> > @@ -298,7 +298,7 @@
> >   * entry dead. The rule must be unlinked from lists to the moment.
> >   */
> >  
> > -static void xfrm_policy_kill(struct xfrm_policy *policy)
> > +static void xfrm_policy_kill(struct xfrm_policy *policy, int dir, int notif)
> 
> Again, had you done the km_* calls from af_key/xfrm_user, then there'd
> be no need to check notif here.
> 

Refer to my comments above on being able to tell multiple managers about
the events originated by one. 
Actually, given that this function is being called in many places i
would say this is the exact central location you want to issue the
announce from.

> BTW, as it is you're announcing expired policies twice.  Once as an
> expire event and once as a delete event.  This problem will also go
> away if you move the km_* calls into af_key/xfrm_user.
> 

Theres an announcement only when policy goes dead ;->
So only one not two. Same with the state as well.

And again cant do it from af_key/xfrm_user if you want to have events
generated by one km to be sent to another as well. Its pf_key that needs
fixing.

> > @@ -579,7 +586,7 @@
> >  	write_unlock_bh(&xfrm_policy_lock);
> >  
> >  	if (old_pol) {
> > -		xfrm_policy_kill(old_pol);
> > +		xfrm_policy_kill(old_pol, dir, 1);
> >  	}
> 
> Please don't announce socket policies :)
> 

I missed this one - sorry.

> > --- a/net/xfrm/xfrm_user.c	2005-03-25 22:28:22.000000000 -0500
> > +++ b/net/xfrm/xfrm_user.c	2005-03-31 19:26:24.000000000 -0500
> > @@ -683,6 +683,10 @@
> >  	if (!xp)
> >  		return err;
> >  
> > +	/* shouldnt excl be based on nlh flags?? 
> > +	 * Aha! this is anti-netlink really i.e  more pfkey derived
> > +	 * in netlink excl is a flag and you wouldnt need
> > +	 * a type XFRM_MSG_UPDPOLICY - JHS */
> 
> Good point.  Care to provide a patch to treat NEW + NLM_F_REPLACE
> as UPD?
> 
> > @@ -1053,10 +1057,10 @@
> >  	return -1;
> >  }
> >  
> > -static int xfrm_send_state_notify(struct xfrm_state *x, int hard)
> > +static int xfrm_exp_state_notify(struct xfrm_state *x, u32 hard)
> 
> How about calling this xfrm_notify_sa_expired for consistency?
> Ditto for the policy function.

sure.

>   
> > +static int xfrm_notify_sa_flush(struct km_cb *c)
> > +{
> > +	struct xfrm_usersa_flush *p;
> > +	struct nlmsghdr *nlh;
> > +	struct sk_buff *skb;
> > +	unsigned char *b;
> > +	u32 ppid = 0;
> > +	int len = NLMSG_LENGTH(sizeof(struct xfrm_usersa_flush));
> > +
> > +	skb = alloc_skb(len, GFP_ATOMIC);
> > +	if (skb == NULL)
> > +		return -ENOMEM;
> > +	b = skb->tail;
> > +
> > +	nlh = NLMSG_PUT(skb, ppid, jiffies,
> 
> If we're serious about providing sequence numbers then please
> set it up as an atomic integer and use it throughout this file.
> 
> Otherwise just pop zero in there.
> 

I was just being lazy. I could send a 0 but whats wrong with using
jiffies?

> > +	p = NLMSG_DATA(nlh);
> > +	if (!c) {
> > +		printk("xfrm_notify_sa_flush NULL km cb\n");
> > +		p->proto = 0;
> 
> Is anyone expected to call this with a NULL pointer? If not then
> just let it OOPS.  Same comment applies to the cb checks later on.
> 

Will fix this.

> > +static int xfrm_notify_sa( struct xfrm_state *x, int event, struct km_cb *c)
> 
> > +	if (event == XFRM_SAP_ADDED)
> > +		nlt = XFRM_MSG_NEWSA;
> > +	else if (event == XFRM_SAP_UPDATED)
> > +		nlt = XFRM_MSG_UPDSA;
> > +	else if (event == XFRM_SAP_DELETED)
> > +		nlt = XFRM_MSG_DELSA;
> > +	else
> > +		goto nlmsg_failure;
> 
> Please use a switch.
> 

sure.

> > +static int xfrm_send_state_notify(struct xfrm_state *x, int event, struct km_cb *c)
> > +{
> > +
> > +	if ((event == XFRM_SAP_ADDED) || 
> > +		(event == XFRM_SAP_UPDATED) ||
> > +		(event == XFRM_SAP_DELETED))
> > +		return xfrm_notify_sa(x, event, c);
> > +
> > +	if (event == XFRM_SAP_FLUSHED) 
> > +		xfrm_notify_sa_flush(c);
> > +
> > +	if (event != XFRM_SAP_EXPIRED)
> > +		return 0;
> 
> Again a switch would be perfect.
> 

Will fix this.

BTW, Herbert, thanks for taking the time; appreciated.

cheers,
jamal

Re: PATCH: IPSEC xfrm events

[Herbert Xu]

On Fri, Apr 01, 2005 at 06:03:18AM -0500, jamal wrote:
>
> > Some of these functions are called internally as you discovered.
> > Since the notifications should only be generated by user requests,
> > calls to km_notify_* should be made at the places where the user
> > requests are handled, which is in the KM itself.
> 
> You need to be able to generate events at every km not just the one that
> generated the request. You also (most of the time) need to do it before

I understand.  However, that's not determined by where you put the
km_notify call itself.  Even when you call km_notify from af_key
or xfrm_user it will notify every km in the system.

It's the fact that we're calling km_notify instead of pfkey_broadcast
or netlink_broadcast that's important, not the location.

Having the km_notify call made in af_key/xfrm_user is convenient though
for the reason I outlined above.
 
> I may be paranoid but i do this because x could be garbage collected way
> before i send the km user message - and i need it to use it to generate
> the event. I could take a copy of it ... 

That's what the ref counter is for.

> > You've caught a real bug for af_key here.  It's currently possible to
> > receive two delete notifications for the same state.
> 
> Can you elaborate?

Imagine you've got a KM that's trying to delete a state via af_key that's
about to expire.  If pfkey_delete looks up the state successfully, and
then the timer triggers before the actual xfrm_state_delete, you will
get one event generated by the timer and another by pfkey_delete.

> Again like i said: I need to tell every km user about the event, not
> just the originator.

I'm suggesting that you add the km_notify calls to af_key and xfrm_user.
That will take care of notifying everyone.
 
> Well, Masahide-San and I actually did discuss this and he was of the
> same opinion as you. My opinion: We only generate events when something
> happens, not just because someone issues a command. If flush was issued
> and there was nothing to flush why generate an event? does the PFKEY RFC
> say anything on this?

RFC 2367 says that:

     The messaging behavior for SADB_FLUSH is:

           Send an SADB_FLUSH message from a user process to the kernel.

           <base>

           The kernel will return an SADB_FLUSH message to all listening
           sockets.

           <base>

As you can see, there is no exception for the case of an empty database.
So my interpretation would be that a broadcast is needed.

> Refer to my comments above on being able to tell multiple managers about
> the events originated by one. 

May I also refer you to my comment above about this being achieved
by calling km_notify, even if you do it from within af_key or
xfrm_user :)

> Actually, given that this function is being called in many places i
> would say this is the exact central location you want to issue the
> announce from.

Try this as an exercise.  List all the xfrm_policy_kills that need
notifications and all those that don't, you will find that the former
all originate from delete/flush commands in af_key/xfrm_user, while
the latter originate from other callers.

In other words, by placing the call in af_key/xfrm_user you simplify
the logic and make it more maintainable.

> > BTW, as it is you're announcing expired policies twice.  Once as an
> > expire event and once as a delete event.  This problem will also go
> > away if you move the km_* calls into af_key/xfrm_user.
> 
> Theres an announcement only when policy goes dead ;->
> So only one not two. Same with the state as well.

Well when the policy expires you will get one expire notification from
the current timer code and a new one from your patch since the timer
calls xfrm_policy_delete.

See my point? By putting the call in xfrm_policy.c you have to be
really careful in dividing the internal users which shouldn't
generate notifications and the external users which should.  By doing
it in af_key/xfrm_user you can avoid all this work.

> And again cant do it from af_key/xfrm_user if you want to have events
> generated by one km to be sent to another as well. Its pf_key that needs
> fixing.

Well I must repeat that if you were calling km_notify from
af_key/xfrm_user you will be sending these events to all km's
no matter what their affiliation is :)

> > If we're serious about providing sequence numbers then please
> > set it up as an atomic integer and use it throughout this file.
> > 
> > Otherwise just pop zero in there.
> 
> I was just being lazy. I could send a 0 but whats wrong with using
> jiffies?

Using jiffies means that you can have two successive messages that
share the same sequence number.  It's not a big deal of course.  But
if we're going to indicate ordering, we might as well go the full
length.

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: PATCH: IPSEC xfrm events

[jamal]

On Fri, 2005-04-01 at 06:42, Herbert Xu wrote:
> On Fri, Apr 01, 2005 at 06:03:18AM -0500, jamal wrote:
> >
> > > Some of these functions are called internally as you discovered.
> > > Since the notifications should only be generated by user requests,
> > > calls to km_notify_* should be made at the places where the user
> > > requests are handled, which is in the KM itself.
> > 
> > You need to be able to generate events at every km not just the one that
> > generated the request. You also (most of the time) need to do it before
> 
> I understand.  However, that's not determined by where you put the
> km_notify call itself.  Even when you call km_notify from af_key
> or xfrm_user it will notify every km in the system.
> 
> It's the fact that we're calling km_notify instead of pfkey_broadcast
> or netlink_broadcast that's important, not the location.
> 
> Having the km_notify call made in af_key/xfrm_user is convenient though
> for the reason I outlined above.

I think either scheme is fine really;-> I will definetely go back and
consider the approach you are suggesting and see if it results into
more maintanable code - then fair. Otherwise you realize its more work
for me ;->

> > > You've caught a real bug for af_key here.  It's currently possible to
> > > receive two delete notifications for the same state.
> > 
> > Can you elaborate?
> 
> Imagine you've got a KM that's trying to delete a state via af_key that's
> about to expire.  If pfkey_delete looks up the state successfully, and
> then the timer triggers before the actual xfrm_state_delete, you will
> get one event generated by the timer and another by pfkey_delete.
> 

I havent checked the state machine closely, but the following seems to
make sense:
The first thing that happens to delete the state/policy should win if
the state/policy is transitioned to dead. 


> RFC 2367 says that:
> 
>      The messaging behavior for SADB_FLUSH is:
> 
>            Send an SADB_FLUSH message from a user process to the kernel.
> 
>            <base>
> 
>            The kernel will return an SADB_FLUSH message to all listening
>            sockets.
> 
>            <base>
> 
> As you can see, there is no exception for the case of an empty database.
> So my interpretation would be that a broadcast is needed.
> 

Does it really make sense, Herbert? ;-> 
What is it that you just flushed that results in the event?
The RFC is ambigous in my opinion. Look at what it says about deleting
(same ambiguity).

----
3.1.4 SADB_DELETE

   The SADB_DELETE message causes the kernel to delete a Security
   Association from the key table.  The delete message consists of the
   base header followed by the association, and the source and
   destination sockaddrs in the address extension.  The kernel deletes
   the security association matching the type, spi, source address, and
   destination address in the message.

     The message behavior for SADB_DELETE is as follows:

        Send an SADB_DELETE message from a user process to the kernel.

        <base, SA(*), address(SD)>

        The kernel returns the SADB_DELETE message to all listening
        processes.

        <base, SA(*), address(SD)>
------

So why would you generate an event in the case when you didnt delete anything?


> > Actually, given that this function is being called in many places i
> > would say this is the exact central location you want to issue the
> > announce from.
> 
> Try this as an exercise.  List all the xfrm_policy_kills that need
> notifications and all those that don't, you will find that the former
> all originate from delete/flush commands in af_key/xfrm_user, while
> the latter originate from other callers.
> 
> In other words, by placing the call in af_key/xfrm_user you simplify
> the logic and make it more maintainable.
> 

I will go over the code and review. 
You may be absolutely right - thats the better approach to take.

>  BTW, as it is you're announcing expired policies twice.  Once as an
> > > expire event and once as a delete event.  This problem will also go
> > > away if you move the km_* calls into af_key/xfrm_user.
> > 
> > Theres an announcement only when policy goes dead ;->
> > So only one not two. Same with the state as well.
> 
> Well when the policy expires you will get one expire notification from
> the current timer code and a new one from your patch since the timer
> calls xfrm_policy_delete.
>
> See my point? By putting the call in xfrm_policy.c you have to be
> really careful in dividing the internal users which shouldn't
> generate notifications and the external users which should.  By doing
> it in af_key/xfrm_user you can avoid all this work.
> 

Thats a bug really which is being exposed now. So it has nothing to do
with the approach taken ;-> 
No expire should be sent if the policy has transitioned to dead. The bug
is trivial to fix - and actually should be fixed regardless of this
patch.


> > I was just being lazy. I could send a 0 but whats wrong with using
> > jiffies?
> 
> Using jiffies means that you can have two successive messages that
> share the same sequence number.  It's not a big deal of course.  But
> if we're going to indicate ordering, we might as well go the full
> length.
> 

Good point. I will stay lazy and just set a 0 ;->

cheers,
jamal

Re: PATCH: IPSEC xfrm events

[Herbert Xu]

On Fri, Apr 01, 2005 at 07:24:38AM -0500, jamal wrote:
> 
> I think either scheme is fine really;-> I will definetely go back and
> consider the approach you are suggesting and see if it results into
> more maintanable code - then fair. Otherwise you realize its more work
> for me ;->

Well I'm happy to code that part if you want :)
 
> I havent checked the state machine closely, but the following seems to
> make sense:
> The first thing that happens to delete the state/policy should win if
> the state/policy is transitioned to dead. 

Agreed.  That's what we'll get if we make __xfrm_state_delete return
success/failure.
 
> So why would you generate an event in the case when you didnt delete anything?

You're right that the RFC isn't very clear.

Let's forget about the RFC and simply consider the usefulness of this.
I contend that it is useful to see a FLUSH notification even when
it flushed nothing.

The reason is that this is an indication to all listeners that the
database is completely empty.

> > Well when the policy expires you will get one expire notification from
> > the current timer code and a new one from your patch since the timer
> > calls xfrm_policy_delete.
> >
> > See my point? By putting the call in xfrm_policy.c you have to be
> > really careful in dividing the internal users which shouldn't
> > generate notifications and the external users which should.  By doing
> > it in af_key/xfrm_user you can avoid all this work.
> 
> Thats a bug really which is being exposed now. So it has nothing to do
> with the approach taken ;-> 

You're right that it is a bug.  However, this bug would've never triggered
before because we simply didn't have delete policy notifications :)

> No expire should be sent if the policy has transitioned to dead. The bug
> is trivial to fix - and actually should be fixed regardless of this
> patch.

Yes the same fix to __xfrm_state_delete can be applied to
xfrm_policy_delete.

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: PATCH: IPSEC xfrm events

[jamal]

On Fri, 2005-04-01 at 07:35, Herbert Xu wrote:
> On Fri, Apr 01, 2005 at 07:24:38AM -0500, jamal wrote:
> > 
> > I think either scheme is fine really;-> I will definetely go back and
> > consider the approach you are suggesting and see if it results into
> > more maintanable code - then fair. Otherwise you realize its more work
> > for me ;->
> 
> Well I'm happy to code that part if you want :)
>  

Let me review first. If it is valuable (we may have to leave expire
alone). If i can get it done within next day or two fine - else if i get
busyed out elsewhere i will hand it to you. Actually if you have plenty
cycles and are very enthusiastic about this i can hand it to you right
now ;-> Masahide and myself have some momentum going right now but i
dont think this will be that disruptive.

> You're right that the RFC isn't very clear.
> 
> Let's forget about the RFC and simply consider the usefulness of this.
> I contend that it is useful to see a FLUSH notification even when
> it flushed nothing.
> 
> The reason is that this is an indication to all listeners that the
> database is completely empty.
> 

Ok, let me hear from Masahide-san: If he still holds the same opinion as
you then i will make the change.


> > Thats a bug really which is being exposed now. So it has nothing to do
> > with the approach taken ;-> 
> 
> You're right that it is a bug.  However, this bug would've never triggered
> before because we simply didn't have delete policy notifications :)
> 

indeed.

> > No expire should be sent if the policy has transitioned to dead. The bug
> > is trivial to fix - and actually should be fixed regardless of this
> > patch.
> 
> Yes the same fix to __xfrm_state_delete can be applied to
> xfrm_policy_delete.
> 

agreed.

cheers,
jamal

Re: PATCH: IPSEC xfrm events

[jamal]

On Fri, 2005-04-01 at 07:59, jamal wrote:

> Let me review first. If it is valuable (we may have to leave expire
> alone). 

Ok, from a first review I would agree with you the result of doing it in
km user will be more maintainable. It will result in a larger patch
but in the long run more maintainable.

> If i can get it done within next day or two fine - else if i get
> busyed out elsewhere i will hand it to you. 

Let me code away at it - The offer still stands though ;->

cheers,
jamal

Re: PATCH: IPSEC xfrm events

[Masahide NAKAMURA]

Hello Jamal and Herbert,


jamal wrote:
> Let me review first. If it is valuable (we may have to leave expire
> alone). If i can get it done within next day or two fine - else if i get
> busyed out elsewhere i will hand it to you. Actually if you have plenty
> cycles and are very enthusiastic about this i can hand it to you right
> now ;-> Masahide and myself have some momentum going right now but i
> dont think this will be that disruptive.
> 
> 
>>You're right that the RFC isn't very clear.
>>
>>Let's forget about the RFC and simply consider the usefulness of this.
>>I contend that it is useful to see a FLUSH notification even when
>>it flushed nothing.
>>
>>The reason is that this is an indication to all listeners that the
>>database is completely empty.
>>
> 
> 
> Ok, let me hear from Masahide-san: If he still holds the same opinion as
> you then i will make the change.

I think FLUSH should be sent in such case.
Because flushing empty SADB/SPD is not an error (at current code),
it is reasonable to broadcast it.

Regards,

-- 
Masahide NAKAMURA

Re: PATCH: IPSEC xfrm events

[Masahide NAKAMURA]

Jamal and Herbert,

jamal wrote:
> Herbert et al,
> 
> Ok, heres the final patch with all the changes discussed.
> 
>  include/linux/xfrm.h   |    2 
>  include/net/xfrm.h     |   29 ++++++-
>  net/key/af_key.c       |   24 +++++-
>  net/xfrm/xfrm_policy.c |   25 ++++--
>  net/xfrm/xfrm_state.c  |   84 +++++++++++++++++++--
>  net/xfrm/xfrm_user.c   |  188
> ++++++++++++++++++++++++++++++++++++++++++++++++-
>  6 files changed, 323 insertions(+), 29 deletions(-)
> 
> I have tested this with both setkey and iproute2 (about 10 scenarios or
> so). Masahide-san is doing a lot more thorough testing with key servers
> as well. He has not tested this patch yet (time difference) but it is
> based on the last one he tested.

Short report:
I've tested on this patched kernel and it works.

- add/del/flush for SA/SP and  allocspi/acquire/upd for SA
  through netlink socket
- racoon runs fine (pfkey works for normal operation)
  both without and with opening netlink socket to listen

Since we have discussion which is still going on about the patch,
the code will be change and I'll need to test again anyway.

Thanks,

-- 
Masahide NAKAMURA

Re: PATCH: IPSEC xfrm events

[jamal]

Herbert,

Staring at the code, obversation:

-> PFKEY is going to be interesting to have it actually generate events
as a result of some app using netlink such as ip x - the reverse is
actually easier to deal with. This problem doesnt exist with current
approach i am taking.

The issue is that pfkey echoes back a few things from the original
message - important ones being version, pid, seq, and msgtype (as a
sample take a look at pfkey_add()). So these need to be remembered...
Brings back the original behavior i had netlink doing which was similar
(but innacurate now that i stare at this). At the time i carried the
nlmsg header around in the cb. So we would have to do the same for
netlink[1].
The good news is all these fields happen to exist on netlink (except for
the version - to which, for netlink created events, we could pass a
hardcoded matching PFKEY2).
In other words the structure i called km_cb will now have to have these
fields i mentioned above. 


Thoughts before i start ?

cheers,
jamal

[1]I actually would have no problems using a pid/seq etc generated by
pfkey on a netlink header and viceversa. It shouldnt be an issue.

Re: PATCH: IPSEC xfrm events

[Herbert Xu]

Hi Jamal:

On Fri, Apr 01, 2005 at 08:04:05PM -0500, jamal wrote:
>
> The issue is that pfkey echoes back a few things from the original
> message - important ones being version, pid, seq, and msgtype (as a
> sample take a look at pfkey_add()). So these need to be remembered...

You're right.  The pid and seq should be stored in km_event by
af_key and xfrm_user before they call km_notify.  In fact bring
back that the km_type field too and put it in km_event.  That'll
become useful when we figure out a way to include it in the netlink
message so that the originator can be uniquely identified.

The version should always be set by the kernel though.  This is because
the packet we're broadcasting has been regenerated by the kernel.  If
we ever get PFKEY v3 then in order that all existing applications
understand these messages you'll have to reformat them as PFKEY v2
anyway.

msgtype should be derived from the event as you did in xfrm_user.

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: PATCH: IPSEC xfrm events

[jamal]

Herbert,

On Fri, 2005-04-01 at 20:28, Herbert Xu wrote:
> Hi Jamal:
> 
> On Fri, Apr 01, 2005 at 08:04:05PM -0500, jamal wrote:
> >
> > The issue is that pfkey echoes back a few things from the original
> > message - important ones being version, pid, seq, and msgtype (as a
> > sample take a look at pfkey_add()). So these need to be remembered...
> 
> You're right.  The pid and seq should be stored in km_event by
> af_key and xfrm_user before they call km_notify.  In fact bring
> back that the km_type field too and put it in km_event. 

Do we need km_type? Given we have: the event, seq, pid (regardless of
where it was generated) we have sufficient info to create eitehr a
netlink or pfkey message.

>  That'll
> become useful when we figure out a way to include it in the netlink
> message so that the originator can be uniquely identified.
> 

The pid seems pretty accurate to describe what process generated the
initial message.

hold on: Ah, I think i may get what you are trying to get to: You want
iproute to display something along the lines of "this was created by a
pfkey app pid 1534". Did i read you correctly?
 
> The version should always be set by the kernel though.  This is because
> the packet we're broadcasting has been regenerated by the kernel.  If
> we ever get PFKEY v3 then in order that all existing applications
> understand these messages you'll have to reformat them as PFKEY v2
> anyway.
> 

So always go v2?

> msgtype should be derived from the event as you did in xfrm_user.
> 

indeed.

cheers,
jamal

Re: PATCH: IPSEC xfrm events

[Herbert Xu]

On Fri, Apr 01, 2005 at 08:42:45PM -0500, jamal wrote:
> 
> hold on: Ah, I think i may get what you are trying to get to: You want
> iproute to display something along the lines of "this was created by a
> pfkey app pid 1534". Did i read you correctly?

That's right.  Someone with a pathological mind might do pfkey and
netlink from the same pid :)
-- 
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: PATCH: IPSEC xfrm events

[Herbert Xu]

On Fri, Apr 01, 2005 at 08:42:45PM -0500, jamal wrote:
> 
> So always go v2?

Yes since that's the only version that the kernel knows how to generate.

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

PATCH: IPSEC xfrm events

[jamal]

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


Dave,

Heres the final patch.
What this patch provides

- netlink xfrm events
- ability to have events generated by netlink propagated to pfkey
and vice versa.
- fixes the acquire lets-be-happy-with-one-success issue

cheers,
jamal

[-- Attachment #2: ipsec-event-take2-6 --]
[-- Type: text/plain, Size: 28779 bytes --]

--- a/include/net/xfrm.h	2005-04-05 07:19:11.000000000 -0400
+++ b/include/net/xfrm.h	2005-04-05 07:29:00.000000000 -0400
@@ -157,6 +157,28 @@
 	XFRM_STATE_DEAD
 };
 
+/* events that could be sent by kernel */
+enum {
+	XFRM_SAP_INVALID,
+	XFRM_SAP_EXPIRED,
+	XFRM_SAP_ADDED,
+	XFRM_SAP_UPDATED,
+	XFRM_SAP_DELETED,
+	XFRM_SAP_FLUSHED,
+	__XFRM_SAP_MAX
+};
+#define XFRM_SAP_MAX (__XFRM_SAP_MAX - 1)
+
+/* callback structure passed from either netlink or pfkey */
+struct km_event
+{
+	u32	data; 
+	u32	seq; 
+	u32	pid; 
+	u32	event; 
+};
+
+
 struct xfrm_type;
 struct xfrm_dst;
 struct xfrm_policy_afinfo {
@@ -178,6 +200,9 @@
 
 extern int xfrm_policy_register_afinfo(struct xfrm_policy_afinfo *afinfo);
 extern int xfrm_policy_unregister_afinfo(struct xfrm_policy_afinfo *afinfo);
+extern void km_policy_notify(struct xfrm_policy *xp, int dir, struct km_event *c);
+extern void km_state_notify(struct xfrm_state *x, struct km_event *c);
+
 
 #define XFRM_ACQ_EXPIRES	30
 
@@ -283,17 +308,17 @@
 	struct xfrm_tmpl       	xfrm_vec[XFRM_MAX_DEPTH];
 };
 
-#define XFRM_KM_TIMEOUT		30
 
+#define XFRM_KM_TIMEOUT		30
 struct xfrm_mgr
 {
 	struct list_head	list;
 	char			*id;
-	int			(*notify)(struct xfrm_state *x, int event);
+	int			(*notify)(struct xfrm_state *x, struct km_event *c);
 	int			(*acquire)(struct xfrm_state *x, struct xfrm_tmpl *, struct xfrm_policy *xp, int dir);
 	struct xfrm_policy	*(*compile_policy)(u16 family, int opt, u8 *data, int len, int *dir);
 	int			(*new_mapping)(struct xfrm_state *x, xfrm_address_t *ipaddr, u16 sport);
-	int			(*notify_policy)(struct xfrm_policy *x, int dir, int event);
+	int			(*notify_policy)(struct xfrm_policy *x, int dir, struct km_event *c);
 };
 
 extern int xfrm_register_km(struct xfrm_mgr *km);
@@ -805,7 +830,7 @@
 extern int xfrm_state_update(struct xfrm_state *x);
 extern struct xfrm_state *xfrm_state_lookup(xfrm_address_t *daddr, u32 spi, u8 proto, unsigned short family);
 extern struct xfrm_state *xfrm_find_acq_byseq(u32 seq);
-extern void xfrm_state_delete(struct xfrm_state *x);
+extern int xfrm_state_delete(struct xfrm_state *x);
 extern void xfrm_state_flush(u8 proto);
 extern int xfrm_replay_check(struct xfrm_state *x, u32 seq);
 extern void xfrm_replay_advance(struct xfrm_state *x, u32 seq);
--- a/include/linux/xfrm.h	2005-03-02 02:38:37.000000000 -0500
+++ b/include/linux/xfrm.h	2005-04-05 07:29:00.000000000 -0400
@@ -254,5 +254,7 @@
 
 #define XFRMGRP_ACQUIRE		1
 #define XFRMGRP_EXPIRE		2
+#define XFRMGRP_SA		4
+#define XFRMGRP_POLICY		8
 
 #endif /* _LINUX_XFRM_H */
--- a/net/xfrm/xfrm_state.c	2005-04-05 07:19:30.000000000 -0400
+++ b/net/xfrm/xfrm_state.c	2005-04-05 07:29:00.000000000 -0400
@@ -50,7 +50,7 @@
 
 static int xfrm_state_gc_flush_bundles;
 
-static void __xfrm_state_delete(struct xfrm_state *x);
+static int __xfrm_state_delete(struct xfrm_state *x);
 
 static struct xfrm_state_afinfo *xfrm_state_get_afinfo(unsigned short family);
 static void xfrm_state_put_afinfo(struct xfrm_state_afinfo *afinfo);
@@ -215,8 +215,10 @@
 }
 EXPORT_SYMBOL(__xfrm_state_destroy);
 
-static void __xfrm_state_delete(struct xfrm_state *x)
+static int __xfrm_state_delete(struct xfrm_state *x)
 {
+	int err = -ESRCH;
+
 	if (x->km.state != XFRM_STATE_DEAD) {
 		x->km.state = XFRM_STATE_DEAD;
 		spin_lock(&xfrm_state_lock);
@@ -245,14 +247,21 @@
 		 * is what we are dropping here.
 		 */
 		atomic_dec(&x->refcnt);
+		err = 0;
 	}
+
+	return err;
 }
 
-void xfrm_state_delete(struct xfrm_state *x)
+int xfrm_state_delete(struct xfrm_state *x)
 {
+	int err;
+
 	spin_lock_bh(&x->lock);
-	__xfrm_state_delete(x);
+	err = __xfrm_state_delete(x);
 	spin_unlock_bh(&x->lock);
+
+	return err;
 }
 EXPORT_SYMBOL(xfrm_state_delete);
 
@@ -430,6 +439,7 @@
 
 static struct xfrm_state *__xfrm_find_acq_byseq(u32 seq);
 
+
 int xfrm_state_add(struct xfrm_state *x)
 {
 	struct xfrm_state_afinfo *afinfo;
@@ -795,34 +805,60 @@
 static struct list_head xfrm_km_list = LIST_HEAD_INIT(xfrm_km_list);
 static DEFINE_RWLOCK(xfrm_km_lock);
 
-static void km_state_expired(struct xfrm_state *x, int hard)
+void km_policy_notify(struct xfrm_policy *xp, int dir, struct km_event *c)
 {
 	struct xfrm_mgr *km;
 
-	if (hard)
-		x->km.state = XFRM_STATE_EXPIRED;
-	else
-		x->km.dying = 1;
+	read_lock(&xfrm_km_lock);
+	list_for_each_entry(km, &xfrm_km_list, list)
+		if (km->notify_policy)
+			km->notify_policy(xp, dir, c);
+	read_unlock(&xfrm_km_lock);
+}
 
+void km_state_notify(struct xfrm_state *x, struct km_event *c)
+{
+	struct xfrm_mgr *km;
 	read_lock(&xfrm_km_lock);
 	list_for_each_entry(km, &xfrm_km_list, list)
-		km->notify(x, hard);
+		if (km->notify)
+			km->notify(x, c);
 	read_unlock(&xfrm_km_lock);
+}
+
+EXPORT_SYMBOL(km_policy_notify);
+EXPORT_SYMBOL(km_state_notify);
+
+static void km_state_expired(struct xfrm_state *x, int hard)
+{
+	struct km_event c;
+
+	if (hard)
+		x->km.state = XFRM_STATE_EXPIRED;
+	else
+		x->km.dying = 1;
+	c.data = hard;
+	c.event = XFRM_SAP_EXPIRED;
+	km_state_notify(x, &c);
 
 	if (hard)
 		wake_up(&km_waitq);
 }
 
+/*
+ * We send to all registered managers regardless of failure
+ * We are happy with one success
+*/
 static int km_query(struct xfrm_state *x, struct xfrm_tmpl *t, struct xfrm_policy *pol)
 {
-	int err = -EINVAL;
+	int err = -EINVAL, acqret;
 	struct xfrm_mgr *km;
 
 	read_lock(&xfrm_km_lock);
 	list_for_each_entry(km, &xfrm_km_list, list) {
-		err = km->acquire(x, t, pol, XFRM_POLICY_OUT);
-		if (!err)
-			break;
+		acqret = km->acquire(x, t, pol, XFRM_POLICY_OUT);
+		if (!acqret)
+			err = acqret;
 	}
 	read_unlock(&xfrm_km_lock);
 	return err;
@@ -847,13 +883,12 @@
 
 void km_policy_expired(struct xfrm_policy *pol, int dir, int hard)
 {
-	struct xfrm_mgr *km;
+	struct km_event c;
 
-	read_lock(&xfrm_km_lock);
-	list_for_each_entry(km, &xfrm_km_list, list)
-		if (km->notify_policy)
-			km->notify_policy(pol, dir, hard);
-	read_unlock(&xfrm_km_lock);
+	c.data = hard;
+	c.data = hard;
+	c.event = XFRM_SAP_EXPIRED;
+	km_policy_notify(pol, dir, &c);
 
 	if (hard)
 		wake_up(&km_waitq);
--- a/net/xfrm/xfrm_user.c	2005-03-02 02:38:10.000000000 -0500
+++ b/net/xfrm/xfrm_user.c	2005-04-05 07:47:45.000000000 -0400
@@ -268,6 +268,7 @@
 	struct xfrm_usersa_info *p = NLMSG_DATA(nlh);
 	struct xfrm_state *x;
 	int err;
+	struct km_event c;
 
 	err = verify_newsa_info(p, (struct rtattr **) xfrma);
 	if (err)
@@ -277,6 +278,7 @@
 	if (!x)
 		return err;
 
+	xfrm_state_hold(x);
 	if (nlh->nlmsg_type == XFRM_MSG_NEWSA)
 		err = xfrm_state_add(x);
 	else
@@ -285,14 +287,27 @@
 	if (err < 0) {
 		x->km.state = XFRM_STATE_DEAD;
 		xfrm_state_put(x);
+		return err;
 	}
 
+	c.seq = nlh->nlmsg_seq;
+	c.pid = nlh->nlmsg_pid;
+	if (nlh->nlmsg_type == XFRM_MSG_NEWSA)
+		c.event = XFRM_SAP_ADDED;
+	else
+		c.event = XFRM_SAP_UPDATED;
+
+	km_state_notify(x, &c);
+	xfrm_state_put(x);
+
 	return err;
 }
 
 static int xfrm_del_sa(struct sk_buff *skb, struct nlmsghdr *nlh, void **xfrma)
 {
 	struct xfrm_state *x;
+	int err;
+	struct km_event c;
 	struct xfrm_usersa_id *p = NLMSG_DATA(nlh);
 
 	x = xfrm_state_lookup(&p->daddr, p->spi, p->proto, p->family);
@@ -304,10 +319,19 @@
 		return -EPERM;
 	}
 
-	xfrm_state_delete(x);
+	err = xfrm_state_delete(x);
+	if (err < 0) {
+		xfrm_state_put(x);
+		return err;
+	}
+
+	c.seq = nlh->nlmsg_seq;
+	c.pid = nlh->nlmsg_pid;
+	c.event = XFRM_SAP_DELETED;
+	km_state_notify(x, &c);
 	xfrm_state_put(x);
 
-	return 0;
+	return err;
 }
 
 static void copy_to_user_state(struct xfrm_state *x, struct xfrm_usersa_info *p)
@@ -335,6 +359,7 @@
 	int this_idx;
 };
 
+
 static int dump_one_state(struct xfrm_state *x, int count, void *ptr)
 {
 	struct xfrm_dump_info *sp = ptr;
@@ -672,6 +697,7 @@
 {
 	struct xfrm_userpolicy_info *p = NLMSG_DATA(nlh);
 	struct xfrm_policy *xp;
+	struct km_event c;
 	int err;
 	int excl;
 
@@ -683,6 +709,10 @@
 	if (!xp)
 		return err;
 
+	/* shouldnt excl be based on nlh flags?? 
+	 * Aha! this is anti-netlink really i.e  more pfkey derived
+	 * in netlink excl is a flag and you wouldnt need
+	 * a type XFRM_MSG_UPDPOLICY - JHS */
 	excl = nlh->nlmsg_type == XFRM_MSG_NEWPOLICY;
 	err = xfrm_policy_insert(p->dir, xp, excl);
 	if (err) {
@@ -690,6 +720,16 @@
 		return err;
 	}
 
+
+	if (!excl)
+		c.event = XFRM_SAP_UPDATED;
+	else
+		c.event = XFRM_SAP_ADDED;
+
+	c.seq = nlh->nlmsg_seq;
+	c.pid = nlh->nlmsg_pid;
+	km_policy_notify(xp, p->dir, &c);
+
 	xfrm_pol_put(xp);
 
 	return 0;
@@ -807,8 +847,10 @@
 	struct xfrm_policy *xp;
 	struct xfrm_userpolicy_id *p;
 	int err;
+	struct km_event c;
 	int delete;
 
+
 	p = NLMSG_DATA(nlh);
 	delete = nlh->nlmsg_type == XFRM_MSG_DELPOLICY;
 
@@ -834,6 +876,11 @@
 					      NETLINK_CB(skb).pid,
 					      MSG_DONTWAIT);
 		}
+	} else {
+		c.event = XFRM_SAP_DELETED;
+		c.seq = nlh->nlmsg_seq;
+		c.pid = nlh->nlmsg_pid;
+		km_policy_notify(xp, p->dir, &c);
 	}
 
 	xfrm_pol_put(xp);
@@ -843,15 +890,28 @@
 
 static int xfrm_flush_sa(struct sk_buff *skb, struct nlmsghdr *nlh, void **xfrma)
 {
+	struct km_event c;
 	struct xfrm_usersa_flush *p = NLMSG_DATA(nlh);
 
 	xfrm_state_flush(p->proto);
+	c.data = p->proto;
+	c.event = XFRM_SAP_FLUSHED;
+	c.seq = nlh->nlmsg_seq;
+	c.pid = nlh->nlmsg_pid;
+	km_state_notify(NULL, &c);
+
 	return 0;
 }
 
 static int xfrm_flush_policy(struct sk_buff *skb, struct nlmsghdr *nlh, void **xfrma)
 {
+	struct km_event c;
+
 	xfrm_policy_flush();
+	c.event = XFRM_SAP_FLUSHED;
+	c.seq = nlh->nlmsg_seq;
+	c.pid = nlh->nlmsg_pid;
+	km_policy_notify(NULL, 0, &c);
 	return 0;
 }
 
@@ -1053,10 +1113,11 @@
 	return -1;
 }
 
-static int xfrm_send_state_notify(struct xfrm_state *x, int hard)
+static int xfrm_exp_state_notify(struct xfrm_state *x, struct km_event *c)
 {
 	struct sk_buff *skb;
-
+	int hard = c ->data;
+	/* fix to do alloc using NLM macros */
 	skb = alloc_skb(sizeof(struct xfrm_user_expire) + 16, GFP_ATOMIC);
 	if (skb == NULL)
 		return -ENOMEM;
@@ -1069,6 +1130,122 @@
 	return netlink_broadcast(xfrm_nl, skb, 0, XFRMGRP_EXPIRE, GFP_ATOMIC);
 }
 
+static int xfrm_notify_sa_flush(struct km_event *c)
+{
+	struct xfrm_usersa_flush *p;
+	struct nlmsghdr *nlh;
+	struct sk_buff *skb;
+	unsigned char *b;
+	int len = NLMSG_LENGTH(sizeof(struct xfrm_usersa_flush));
+
+	skb = alloc_skb(len, GFP_ATOMIC);
+	if (skb == NULL)
+		return -ENOMEM;
+	b = skb->tail;
+
+	nlh = NLMSG_PUT(skb, c->pid,  c->seq,
+			XFRM_MSG_FLUSHSA, sizeof(*p));
+	nlh->nlmsg_flags = 0;
+
+	p = NLMSG_DATA(nlh);
+	p->proto = c->data;
+
+	nlh->nlmsg_len = skb->tail - b;
+
+	return netlink_broadcast(xfrm_nl, skb, 0, XFRMGRP_SA, GFP_ATOMIC);
+
+nlmsg_failure:
+	kfree_skb(skb);
+	return -1;
+}
+
+static int inline xfrm_sa_len(struct xfrm_state *x)
+{
+	int l = NLMSG_LENGTH(sizeof(struct xfrm_usersa_info));
+	if (x->aalg)
+		l+= RTA_SPACE(sizeof(*(x->aalg))+(x->aalg->alg_key_len+7)/8);
+	if (x->ealg)
+		l+= RTA_SPACE(sizeof(*(x->ealg))+(x->ealg->alg_key_len+7)/8);
+	if (x->calg)
+		l+= RTA_SPACE(sizeof(*(x->calg)));
+	if (x->encap)
+		l+= RTA_SPACE(sizeof(*x->encap));
+
+	return l;
+}
+
+static int xfrm_notify_sa(struct xfrm_state *x, struct km_event *c)
+{
+	struct xfrm_usersa_info *p;
+	struct nlmsghdr *nlh;
+	struct sk_buff *skb;
+	u32 nlt;
+	unsigned char *b;
+	int len = xfrm_sa_len(x);
+
+	skb = alloc_skb(len, GFP_ATOMIC);
+	if (skb == NULL)
+		return -ENOMEM;
+	b = skb->tail;
+
+	if (c->event == XFRM_SAP_ADDED)
+		nlt = XFRM_MSG_NEWSA;
+	else if (c->event == XFRM_SAP_UPDATED)
+		nlt = XFRM_MSG_UPDSA;
+	else if (c->event == XFRM_SAP_DELETED)
+		nlt = XFRM_MSG_DELSA;
+	else
+		goto nlmsg_failure;
+
+	nlh = NLMSG_PUT(skb, c->pid, c->seq, nlt, sizeof(*p));
+	nlh->nlmsg_flags = 0;
+
+	p = NLMSG_DATA(nlh);
+	copy_to_user_state(x, p);
+
+	if (x->aalg)
+		RTA_PUT(skb, XFRMA_ALG_AUTH,
+			sizeof(*(x->aalg))+(x->aalg->alg_key_len+7)/8, x->aalg);
+	if (x->ealg)
+		RTA_PUT(skb, XFRMA_ALG_CRYPT,
+			sizeof(*(x->ealg))+(x->ealg->alg_key_len+7)/8, x->ealg);        
+	if (x->calg)
+		RTA_PUT(skb, XFRMA_ALG_COMP, sizeof(*(x->calg)), x->calg);
+
+	if (x->encap)
+		RTA_PUT(skb, XFRMA_ENCAP, sizeof(*x->encap), x->encap);
+
+	nlh->nlmsg_len = skb->tail - b;
+
+	return netlink_broadcast(xfrm_nl, skb, 0, XFRMGRP_SA, GFP_ATOMIC);
+
+nlmsg_failure:
+rtattr_failure:
+	kfree_skb(skb);
+	return -1;
+}
+
+static int xfrm_send_state_notify(struct xfrm_state *x, struct km_event *c)
+{
+
+	switch (c->event) {
+	case XFRM_SAP_EXPIRED:
+		return xfrm_exp_state_notify(x, c);
+	case XFRM_SAP_DELETED:
+	case XFRM_SAP_UPDATED:
+	case XFRM_SAP_ADDED:
+		return xfrm_notify_sa(x, c);
+	case XFRM_SAP_FLUSHED:
+		return xfrm_notify_sa_flush(c);
+	default:
+		 printk("xfrm_user: Unknown SA event %d\n",c->event);
+		 break;
+	}
+
+	return 0;
+
+}
+
 static int build_acquire(struct sk_buff *skb, struct xfrm_state *x,
 			 struct xfrm_tmpl *xt, struct xfrm_policy *xp,
 			 int dir)
@@ -1202,7 +1379,8 @@
 	return -1;
 }
 
-static int xfrm_send_policy_notify(struct xfrm_policy *xp, int dir, int hard)
+
+static int xfrm_exp_policy_notify(struct xfrm_policy *xp, int dir, struct km_event *c)
 {
 	struct sk_buff *skb;
 	size_t len;
@@ -1213,7 +1391,7 @@
 	if (skb == NULL)
 		return -ENOMEM;
 
-	if (build_polexpire(skb, xp, dir, hard) < 0)
+	if (build_polexpire(skb, xp, dir, c->data) < 0)
 		BUG();
 
 	NETLINK_CB(skb).dst_groups = XFRMGRP_EXPIRE;
@@ -1221,6 +1399,93 @@
 	return netlink_broadcast(xfrm_nl, skb, 0, XFRMGRP_EXPIRE, GFP_ATOMIC);
 }
 
+static int xfrm_notify_policy( struct xfrm_policy *xp, int dir, struct km_event *c)
+{
+	struct xfrm_userpolicy_info *p;
+	struct nlmsghdr *nlh;
+	struct sk_buff *skb;
+	u32 nlt = 0 ;
+	unsigned char *b;
+	int len = RTA_SPACE(sizeof(struct xfrm_user_tmpl) * xp->xfrm_nr);
+	len += NLMSG_SPACE(sizeof(struct xfrm_userpolicy_info));
+
+	skb = alloc_skb(len, GFP_ATOMIC);
+	if (skb == NULL)
+		return -ENOMEM;
+	b = skb->tail;
+
+	if (c->event == XFRM_SAP_ADDED)
+		nlt = XFRM_MSG_NEWPOLICY;
+	else if (c->event == XFRM_SAP_UPDATED)
+		nlt = XFRM_MSG_UPDPOLICY;
+	else if (c->event == XFRM_SAP_DELETED)
+		nlt = XFRM_MSG_DELPOLICY;
+	else
+		goto nlmsg_failure;
+
+	nlh = NLMSG_PUT(skb, c->pid, c->seq, nlt, sizeof(*p));
+
+	p = NLMSG_DATA(nlh);
+
+	nlh->nlmsg_flags = 0;
+
+	copy_to_user_policy(xp, p, dir);
+	if (copy_to_user_tmpl(xp, skb) < 0)
+		goto nlmsg_failure;
+
+	nlh->nlmsg_len = skb->tail - b;
+
+	return netlink_broadcast(xfrm_nl, skb, 0, XFRMGRP_POLICY, GFP_ATOMIC);
+
+nlmsg_failure:
+	kfree_skb(skb);
+	return -1;
+}
+
+static int xfrm_notify_policy_flush(struct km_event *c)
+{
+	struct nlmsghdr *nlh;
+	struct sk_buff *skb;
+	unsigned char *b;
+	int len = NLMSG_LENGTH(0);
+
+	skb = alloc_skb(len, GFP_ATOMIC);
+	if (skb == NULL)
+		return -ENOMEM;
+	b = skb->tail;
+
+
+	nlh = NLMSG_PUT(skb, c->pid, c->seq, XFRM_MSG_FLUSHPOLICY, 0);
+
+	nlh->nlmsg_len = skb->tail - b;
+
+	return netlink_broadcast(xfrm_nl, skb, 0, XFRMGRP_POLICY, GFP_ATOMIC);
+
+nlmsg_failure:
+	kfree_skb(skb);
+	return -1;
+}
+
+static int xfrm_send_policy_notify(struct xfrm_policy *xp, int dir, struct km_event *c)
+{
+
+	switch (c->event) {
+	case XFRM_SAP_ADDED:
+	case XFRM_SAP_UPDATED:
+	case XFRM_SAP_DELETED:
+		return xfrm_notify_policy(xp, dir, c);
+	case XFRM_SAP_FLUSHED:
+		return xfrm_notify_policy_flush(c);
+	case XFRM_SAP_EXPIRED:
+		return xfrm_exp_policy_notify(xp, dir, c);
+	default:
+		printk("xfrm_user: Unknown Policy event %d\n",c->event);
+	}
+
+	return 0;
+
+}
+
 static struct xfrm_mgr netlink_mgr = {
 	.id		= "netlink",
 	.notify		= xfrm_send_state_notify,
--- a/net/key/af_key.c	2005-04-05 07:19:26.000000000 -0400
+++ b/net/key/af_key.c	2005-04-05 07:48:31.000000000 -0400
@@ -1240,13 +1240,85 @@
 	return 0;
 }
 
+static inline int event2poltype (int event)
+{
+	switch (event) {
+	case XFRM_SAP_DELETED:
+		return SADB_X_SPDDELETE;
+	case XFRM_SAP_ADDED:
+		return SADB_X_SPDADD;
+	case XFRM_SAP_UPDATED:
+		return SADB_X_SPDUPDATE;
+	case XFRM_SAP_EXPIRED:
+	//	return SADB_X_SPDEXPIRE;
+	default:
+		printk("pfkey: Unknown policy event %d\n",event);
+		break;
+	}
+
+	return 0;
+}
+
+static inline int event2keytype (int event)
+{
+	switch (event) {
+	case XFRM_SAP_DELETED:
+		return SADB_DELETE;
+	case XFRM_SAP_ADDED:
+		return SADB_ADD;
+	case XFRM_SAP_UPDATED:
+		return SADB_UPDATE;
+	case XFRM_SAP_EXPIRED:
+		return SADB_EXPIRE;
+	default:
+		printk("pfkey: Unknown SA event %d\n",event);
+		break;
+	}
+
+	return 0;
+}
+
+/* ADD/UPD/DEL */
+static int key_notify_sa(struct xfrm_state *x, struct km_event *c)
+{
+	struct sk_buff *skb;
+	struct sadb_msg *hdr;
+	int hsc = 3;
+
+	if (c->event == XFRM_SAP_DELETED)
+		hsc = 0;
+
+	if (c->event == XFRM_SAP_EXPIRED) {
+		if (c->data)
+			hsc = 2;
+		else
+			hsc = 1;
+	}
+
+	skb = pfkey_xfrm_state2msg(x, 0, hsc);
+
+	if (IS_ERR(skb))
+		return  PTR_ERR(skb);
+
+	hdr = (struct sadb_msg *) skb->data;
+	hdr->sadb_msg_version = PF_KEY_V2;
+	hdr->sadb_msg_type = event2keytype(c->event);
+	hdr->sadb_msg_satype = pfkey_proto2satype(x->id.proto);
+	hdr->sadb_msg_errno = 0;
+	hdr->sadb_msg_reserved = 0;
+	hdr->sadb_msg_seq = c->seq;
+	hdr->sadb_msg_pid = c->pid;
+
+	pfkey_broadcast(skb, GFP_ATOMIC, BROADCAST_ALL, NULL);
+
+	return 0;
+}
 
 static int pfkey_add(struct sock *sk, struct sk_buff *skb, struct sadb_msg *hdr, void **ext_hdrs)
 {
-	struct sk_buff *out_skb;
-	struct sadb_msg *out_hdr;
 	struct xfrm_state *x;
 	int err;
+	struct km_event c;
 
 	xfrm_probe_algs();
 	
@@ -1254,6 +1326,7 @@
 	if (IS_ERR(x))
 		return PTR_ERR(x);
 
+	xfrm_state_hold(x);
 	if (hdr->sadb_msg_type == SADB_ADD)
 		err = xfrm_state_add(x);
 	else
@@ -1265,27 +1338,23 @@
 		return err;
 	}
 
-	out_skb = pfkey_xfrm_state2msg(x, 0, 3);
-	if (IS_ERR(out_skb))
-		return  PTR_ERR(out_skb); /* XXX Should we return 0 here ? */
-
-	out_hdr = (struct sadb_msg *) out_skb->data;
-	out_hdr->sadb_msg_version = hdr->sadb_msg_version;
-	out_hdr->sadb_msg_type = hdr->sadb_msg_type;
-	out_hdr->sadb_msg_satype = pfkey_proto2satype(x->id.proto);
-	out_hdr->sadb_msg_errno = 0;
-	out_hdr->sadb_msg_reserved = 0;
-	out_hdr->sadb_msg_seq = hdr->sadb_msg_seq;
-	out_hdr->sadb_msg_pid = hdr->sadb_msg_pid;
-
-	pfkey_broadcast(out_skb, GFP_ATOMIC, BROADCAST_ALL, sk);
+	if (hdr->sadb_msg_type == SADB_ADD)
+		c.event = XFRM_SAP_ADDED;
+	else
+		c.event = XFRM_SAP_UPDATED;
+	c.seq = hdr->sadb_msg_seq;
+	c.pid = hdr->sadb_msg_pid;
+	km_state_notify(x, &c);
+	xfrm_state_put(x);
 
-	return 0;
+	return err;
 }
 
 static int pfkey_delete(struct sock *sk, struct sk_buff *skb, struct sadb_msg *hdr, void **ext_hdrs)
 {
 	struct xfrm_state *x;
+	struct km_event c;
+	int err;
 
 	if (!ext_hdrs[SADB_EXT_SA-1] ||
 	    !present_and_same_family(ext_hdrs[SADB_EXT_ADDRESS_SRC-1],
@@ -1301,13 +1370,19 @@
 		return -EPERM;
 	}
 	
-	xfrm_state_delete(x);
-	xfrm_state_put(x);
+	err = xfrm_state_delete(x);
+	if (err < 0) {
+		xfrm_state_put(x);
+		return err;
+	}
 
-	pfkey_broadcast(skb_clone(skb, GFP_KERNEL), GFP_KERNEL, 
-			BROADCAST_ALL, sk);
+	c.seq = hdr->sadb_msg_seq;
+	c.pid = hdr->sadb_msg_pid;
+	c.event = XFRM_SAP_DELETED;
+	km_state_notify(x, &c);
+	xfrm_state_put(x);
 
-	return 0;
+	return err;
 }
 
 static int pfkey_get(struct sock *sk, struct sk_buff *skb, struct sadb_msg *hdr, void **ext_hdrs)
@@ -1445,28 +1520,42 @@
 	return 0;
 }
 
+static int key_notify_sa_flush(struct km_event *c)
+{
+	struct sk_buff *skb;
+	struct sadb_msg *hdr;
+
+	skb = alloc_skb(sizeof(struct sadb_msg) + 16, GFP_ATOMIC);
+	if (!skb)
+		return -ENOBUFS;
+	hdr = (struct sadb_msg *) skb_put(skb, sizeof(struct sadb_msg));
+	hdr->sadb_msg_satype = pfkey_proto2satype(c->data);
+	hdr->sadb_msg_seq = c->seq;
+	hdr->sadb_msg_pid = c->pid;
+	hdr->sadb_msg_version = PF_KEY_V2;
+	hdr->sadb_msg_errno = (uint8_t) 0;
+	hdr->sadb_msg_len = (sizeof(struct sadb_msg) / sizeof(uint64_t));
+
+	pfkey_broadcast(skb, GFP_ATOMIC, BROADCAST_ALL, NULL);
+
+	return 0;
+}
+
 static int pfkey_flush(struct sock *sk, struct sk_buff *skb, struct sadb_msg *hdr, void **ext_hdrs)
 {
 	unsigned proto;
-	struct sk_buff *skb_out;
-	struct sadb_msg *hdr_out;
+	struct km_event c;
 
 	proto = pfkey_satype2proto(hdr->sadb_msg_satype);
 	if (proto == 0)
 		return -EINVAL;
 
-	skb_out = alloc_skb(sizeof(struct sadb_msg) + 16, GFP_KERNEL);
-	if (!skb_out)
-		return -ENOBUFS;
-
 	xfrm_state_flush(proto);
-
-	hdr_out = (struct sadb_msg *) skb_put(skb_out, sizeof(struct sadb_msg));
-	pfkey_hdr_dup(hdr_out, hdr);
-	hdr_out->sadb_msg_errno = (uint8_t) 0;
-	hdr_out->sadb_msg_len = (sizeof(struct sadb_msg) / sizeof(uint64_t));
-
-	pfkey_broadcast(skb_out, GFP_KERNEL, BROADCAST_ALL, NULL);
+	c.data = proto;
+	c.seq = hdr->sadb_msg_seq;
+	c.pid = hdr->sadb_msg_pid;
+	c.event = XFRM_SAP_FLUSHED;
+	km_state_notify(NULL, &c);
 
 	return 0;
 }
@@ -1859,6 +1948,35 @@
 	hdr->sadb_msg_reserved = atomic_read(&xp->refcnt);
 }
 
+static int key_notify_policy( struct xfrm_policy *xp, int dir, struct km_event *c)
+{
+	struct sk_buff *out_skb;
+	struct sadb_msg *out_hdr;
+	int err;
+
+	out_skb = pfkey_xfrm_policy2msg_prep(xp);
+	if (IS_ERR(out_skb)) {
+		err =  PTR_ERR(out_skb);
+		goto out;
+	}
+	pfkey_xfrm_policy2msg(out_skb, xp, dir);
+
+	out_hdr = (struct sadb_msg *) out_skb->data;
+	out_hdr->sadb_msg_version =  PF_KEY_V2;
+
+	if (c->data && c->event == XFRM_SAP_DELETED)
+		out_hdr->sadb_msg_type = SADB_X_SPDDELETE2;
+	else
+		out_hdr->sadb_msg_type = event2poltype(c->event);
+	out_hdr->sadb_msg_errno = 0;
+	out_hdr->sadb_msg_seq = c->seq;
+	out_hdr->sadb_msg_pid = c->pid;
+	pfkey_broadcast(out_skb, GFP_ATOMIC, BROADCAST_ALL, NULL);
+out:
+	return 0;
+
+}
+
 static int pfkey_spdadd(struct sock *sk, struct sk_buff *skb, struct sadb_msg *hdr, void **ext_hdrs)
 {
 	int err;
@@ -1866,8 +1984,7 @@
 	struct sadb_address *sa;
 	struct sadb_x_policy *pol;
 	struct xfrm_policy *xp;
-	struct sk_buff *out_skb;
-	struct sadb_msg *out_hdr;
+	struct km_event c;
 
 	if (!present_and_same_family(ext_hdrs[SADB_EXT_ADDRESS_SRC-1],
 				     ext_hdrs[SADB_EXT_ADDRESS_DST-1]) ||
@@ -1935,31 +2052,25 @@
 	    (err = parse_ipsecrequests(xp, pol)) < 0)
 		goto out;
 
-	out_skb = pfkey_xfrm_policy2msg_prep(xp);
-	if (IS_ERR(out_skb)) {
-		err =  PTR_ERR(out_skb);
-		goto out;
-	}
 
 	err = xfrm_policy_insert(pol->sadb_x_policy_dir-1, xp,
 				 hdr->sadb_msg_type != SADB_X_SPDUPDATE);
+
 	if (err) {
-		kfree_skb(out_skb);
-		goto out;
+		kfree(xp);
+		return err;
 	}
 
-	pfkey_xfrm_policy2msg(out_skb, xp, pol->sadb_x_policy_dir-1);
+	if (hdr->sadb_msg_type == SADB_X_SPDUPDATE) 
+		c.event = XFRM_SAP_UPDATED;
+	else 
+		c.event = XFRM_SAP_ADDED;
 
-	xfrm_pol_put(xp);
+	c.seq = hdr->sadb_msg_seq;
+	c.pid = hdr->sadb_msg_pid;
 
-	out_hdr = (struct sadb_msg *) out_skb->data;
-	out_hdr->sadb_msg_version = hdr->sadb_msg_version;
-	out_hdr->sadb_msg_type = hdr->sadb_msg_type;
-	out_hdr->sadb_msg_satype = 0;
-	out_hdr->sadb_msg_errno = 0;
-	out_hdr->sadb_msg_seq = hdr->sadb_msg_seq;
-	out_hdr->sadb_msg_pid = hdr->sadb_msg_pid;
-	pfkey_broadcast(out_skb, GFP_ATOMIC, BROADCAST_ALL, sk);
+	km_policy_notify(xp, pol->sadb_x_policy_dir-1, &c);
+	xfrm_pol_put(xp);
 	return 0;
 
 out:
@@ -1973,9 +2084,8 @@
 	struct sadb_address *sa;
 	struct sadb_x_policy *pol;
 	struct xfrm_policy *xp;
-	struct sk_buff *out_skb;
-	struct sadb_msg *out_hdr;
 	struct xfrm_selector sel;
+	struct km_event c;
 
 	if (!present_and_same_family(ext_hdrs[SADB_EXT_ADDRESS_SRC-1],
 				     ext_hdrs[SADB_EXT_ADDRESS_DST-1]) ||
@@ -2010,25 +2120,41 @@
 
 	err = 0;
 
+	c.seq = hdr->sadb_msg_seq;
+	c.pid = hdr->sadb_msg_pid;
+	c.event = XFRM_SAP_DELETED;
+	km_policy_notify(xp, pol->sadb_x_policy_dir-1, &c);
+
+	xfrm_pol_put(xp);
+	return err;
+}
+
+
+static int key_pol_get_resp(struct sock *sk, struct xfrm_policy *xp, struct sadb_msg *hdr, int dir)
+{
+	int err;
+	struct sk_buff *out_skb;
+	struct sadb_msg *out_hdr;
+	err = 0;
+
 	out_skb = pfkey_xfrm_policy2msg_prep(xp);
 	if (IS_ERR(out_skb)) {
 		err =  PTR_ERR(out_skb);
 		goto out;
 	}
-	pfkey_xfrm_policy2msg(out_skb, xp, pol->sadb_x_policy_dir-1);
+	pfkey_xfrm_policy2msg(out_skb, xp, dir);
 
 	out_hdr = (struct sadb_msg *) out_skb->data;
 	out_hdr->sadb_msg_version = hdr->sadb_msg_version;
-	out_hdr->sadb_msg_type = SADB_X_SPDDELETE;
+	out_hdr->sadb_msg_type = hdr->sadb_msg_type;
 	out_hdr->sadb_msg_satype = 0;
 	out_hdr->sadb_msg_errno = 0;
 	out_hdr->sadb_msg_seq = hdr->sadb_msg_seq;
 	out_hdr->sadb_msg_pid = hdr->sadb_msg_pid;
-	pfkey_broadcast(out_skb, GFP_ATOMIC, BROADCAST_ALL, sk);
+	pfkey_broadcast(out_skb, GFP_ATOMIC, BROADCAST_ONE, sk);
 	err = 0;
 
 out:
-	xfrm_pol_put(xp);
 	return err;
 }
 
@@ -2037,8 +2163,7 @@
 	int err;
 	struct sadb_x_policy *pol;
 	struct xfrm_policy *xp;
-	struct sk_buff *out_skb;
-	struct sadb_msg *out_hdr;
+	struct km_event c;
 
 	if ((pol = ext_hdrs[SADB_X_EXT_POLICY-1]) == NULL)
 		return -EINVAL;
@@ -2050,24 +2175,16 @@
 
 	err = 0;
 
-	out_skb = pfkey_xfrm_policy2msg_prep(xp);
-	if (IS_ERR(out_skb)) {
-		err =  PTR_ERR(out_skb);
-		goto out;
+	c.seq = hdr->sadb_msg_seq;
+	c.pid = hdr->sadb_msg_pid;
+	if (hdr->sadb_msg_type == SADB_X_SPDDELETE2) {
+		c.data = 1; // to signal pfkey of SADB_X_SPDDELETE2
+		c.event = XFRM_SAP_DELETED;
+		km_policy_notify(xp, pol->sadb_x_policy_dir-1, &c);
+	} else {
+		err = key_pol_get_resp(sk, xp, hdr, pol->sadb_x_policy_dir-1);
 	}
-	pfkey_xfrm_policy2msg(out_skb, xp, pol->sadb_x_policy_dir-1);
 
-	out_hdr = (struct sadb_msg *) out_skb->data;
-	out_hdr->sadb_msg_version = hdr->sadb_msg_version;
-	out_hdr->sadb_msg_type = hdr->sadb_msg_type;
-	out_hdr->sadb_msg_satype = 0;
-	out_hdr->sadb_msg_errno = 0;
-	out_hdr->sadb_msg_seq = hdr->sadb_msg_seq;
-	out_hdr->sadb_msg_pid = hdr->sadb_msg_pid;
-	pfkey_broadcast(out_skb, GFP_ATOMIC, BROADCAST_ALL, sk);
-	err = 0;
-
-out:
 	xfrm_pol_put(xp);
 	return err;
 }
@@ -2102,22 +2219,33 @@
 	return xfrm_policy_walk(dump_sp, &data);
 }
 
-static int pfkey_spdflush(struct sock *sk, struct sk_buff *skb, struct sadb_msg *hdr, void **ext_hdrs)
+static int key_notify_policy_flush(struct km_event *c)
 {
 	struct sk_buff *skb_out;
-	struct sadb_msg *hdr_out;
-
-	skb_out = alloc_skb(sizeof(struct sadb_msg) + 16, GFP_KERNEL);
+	struct sadb_msg *hdr;
+	skb_out = alloc_skb(sizeof(struct sadb_msg) + 16, GFP_ATOMIC);
 	if (!skb_out)
 		return -ENOBUFS;
+	hdr = (struct sadb_msg *) skb_put(skb_out, sizeof(struct sadb_msg));
+	hdr->sadb_msg_seq = c->seq;
+	hdr->sadb_msg_pid = c->pid;
+	hdr->sadb_msg_version = PF_KEY_V2;
+	hdr->sadb_msg_errno = (uint8_t) 0;
+	hdr->sadb_msg_len = (sizeof(struct sadb_msg) / sizeof(uint64_t));
+	pfkey_broadcast(skb_out, GFP_ATOMIC, BROADCAST_ALL, NULL);
+	return 0;
 
-	xfrm_policy_flush();
+}
 
-	hdr_out = (struct sadb_msg *) skb_put(skb_out, sizeof(struct sadb_msg));
-	pfkey_hdr_dup(hdr_out, hdr);
-	hdr_out->sadb_msg_errno = (uint8_t) 0;
-	hdr_out->sadb_msg_len = (sizeof(struct sadb_msg) / sizeof(uint64_t));
-	pfkey_broadcast(skb_out, GFP_KERNEL, BROADCAST_ALL, NULL);
+static int pfkey_spdflush(struct sock *sk, struct sk_buff *skb, struct sadb_msg *hdr, void **ext_hdrs)
+{
+	struct km_event c;
+
+	xfrm_policy_flush();
+	c.event = XFRM_SAP_FLUSHED;
+	c.pid = hdr->sadb_msg_pid;
+	c.seq = hdr->sadb_msg_seq;
+	km_policy_notify(NULL, 0, &c);
 
 	return 0;
 }
@@ -2317,11 +2445,24 @@
 	}
 }
 
-static int pfkey_send_notify(struct xfrm_state *x, int hard)
+/* XXX: Noisy for now */
+static int key_notify_policy_expire(struct xfrm_policy *xp, struct km_event *c)
+{
+	return 0;
+}
+
+static int key_notify_sa_expire(struct xfrm_state *x, struct km_event *c)
 {
 	struct sk_buff *out_skb;
 	struct sadb_msg *out_hdr;
-	int hsc = (hard ? 2 : 1);
+	int hard;
+	int hsc;
+
+	hard = c->data;
+	if (hard)
+		hsc = 2;
+	else
+		hsc = 1;
 
 	out_skb = pfkey_xfrm_state2msg(x, 0, hsc);
 	if (IS_ERR(out_skb))
@@ -2340,6 +2481,43 @@
 	return 0;
 }
 
+static int pfkey_send_notify(struct xfrm_state *x, struct km_event *c)
+{
+	switch (c->event) {
+	case XFRM_SAP_EXPIRED:
+		return key_notify_sa_expire(x, c);
+	case XFRM_SAP_DELETED:
+	case XFRM_SAP_ADDED:
+	case XFRM_SAP_UPDATED:
+		return key_notify_sa(x, c);
+	case XFRM_SAP_FLUSHED:
+		return key_notify_sa_flush(c);
+	default:
+		printk("pfkey: Unknown SA event %d\n",c->event);
+		break;
+	}
+
+	return 0;
+}
+
+static int pfkey_send_policy_notify(struct xfrm_policy *xp, int dir, struct km_event *c)
+{
+	switch (c->event) {
+	case XFRM_SAP_EXPIRED:
+		return key_notify_policy_expire(xp, c);
+	case XFRM_SAP_DELETED:
+	case XFRM_SAP_ADDED:
+	case XFRM_SAP_UPDATED:
+		return key_notify_policy(xp, dir, c);
+	case XFRM_SAP_FLUSHED:
+		return key_notify_policy_flush(c);
+	default:
+		printk("pfkey: Unknown policy event %d\n",c->event);
+		break;
+	}
+
+	return 0;
+}
 static u32 get_acqseq(void)
 {
 	u32 res;
@@ -2856,6 +3034,7 @@
 	.acquire	= pfkey_send_acquire,
 	.compile_policy	= pfkey_compile_policy,
 	.new_mapping	= pfkey_send_new_mapping,
+	.notify_policy	= pfkey_send_policy_notify,
 };
 
 static void __exit ipsec_pfkey_exit(void)

Re: PATCH: IPSEC xfrm events

[Herbert Xu]

On Tue, Apr 05, 2005 at 08:03:24AM -0400, jamal wrote:
> 
> Heres the final patch.
> What this patch provides
> 
> - netlink xfrm events
> - ability to have events generated by netlink propagated to pfkey
> and vice versa.
> - fixes the acquire lets-be-happy-with-one-success issue

Jamal you forgot to sign off your own patch :)

Anyway this looks good to me.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
-- 
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: PATCH: IPSEC xfrm events

[jamal]

Gah, Ok - I guess i too can be famous

Signed-off-by: Jamal Hadi Salim <hadi@cyberus.ca>

cheers,
jamal

On Tue, 2005-04-05 at 08:07, Herbert Xu wrote:
> 
> Jamal you forgot to sign off your own patch :)
> 
> Anyway this looks good to me.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Re: PATCH: IPSEC xfrm events

[Arnaldo Carvalho de Melo]

On 05 Apr 2005 08:19:06 -0400, jamal <hadi@cyberus.ca> wrote:
> 
> Gah, Ok - I guess i too can be famous

Or funny! /me runs

- Arnaldo

[1/4] [IPSEC] Improve xfrm to pfkey SA state conversion

[Herbert Xu]

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

Hi:

This series of patches will fix the spurious state/policy expire
notification problem identified in the discussion regarding Jamal's
IPsec events patch.  It will also address other minor issues arising
from the events patch.

The first patch adjusts the SA state conversion in af_key such that
XFRM_STATE_ERROR/XFRM_STATE_DEAD will be converted to SADB_STATE_DEAD
instead of SADB_STATE_DYING.

According to RFC 2367, SADB_STATE_DYING SAs can be turned into
mature ones through updating their lifetime settings.  Since SAs
which are in the states XFRM_STATE_ERROR/XFRM_STATE_DEAD cannot
be resurrected, this value is unsuitable.

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

[-- Attachment #2: xfrm-event-1 --]
[-- Type: text/plain, Size: 920 bytes --]

===== net/key/af_key.c 1.73 vs edited =====
--- 1.73/net/key/af_key.c	2005-04-08 23:08:26 +10:00
+++ edited/net/key/af_key.c	2005-04-09 14:15:45 +10:00
@@ -656,13 +656,18 @@
 	sa->sadb_sa_exttype = SADB_EXT_SA;
 	sa->sadb_sa_spi = x->id.spi;
 	sa->sadb_sa_replay = x->props.replay_window;
-	sa->sadb_sa_state = SADB_SASTATE_DYING;
-	if (x->km.state == XFRM_STATE_VALID && !x->km.dying)
-		sa->sadb_sa_state = SADB_SASTATE_MATURE;
-	else if (x->km.state == XFRM_STATE_ACQ)
+	switch (x->km.state) {
+	case XFRM_STATE_VALID:
+		sa->sadb_sa_state = x->km.dying ?
+			SADB_SASTATE_DYING : SADB_SASTATE_MATURE;
+		break;
+	case XFRM_STATE_ACQ:
 		sa->sadb_sa_state = SADB_SASTATE_LARVAL;
-	else if (x->km.state == XFRM_STATE_EXPIRED)
+		break;
+	default:
 		sa->sadb_sa_state = SADB_SASTATE_DEAD;
+		break;
+	}
 	sa->sadb_sa_auth = 0;
 	if (x->aalg) {
 		struct xfrm_algo_desc *a = xfrm_aalg_get_byname(x->aalg->alg_name, 0);

[2/4] [IPSEC] Kill spurious hard expire messages

[Herbert Xu]

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

Hi:

This patch ensures that the hard state/policy expire notifications are
only sent when the state/policy is successfully removed from their
respective tables.

As it is, it's possible for a state/policy to both expire through
reaching a hard limit, as well as being deleted by the user.

Note that this behaviour isn't actually forbidden by RFC 2367.
However, it is a quality of implementation issue.

As an added bonus, the restructuring in this patch will help
eventually in moving the expire notifications from softirq
context into process context, thus improving their reliability.

One important side-effect from this change is that SAs reaching
their hard byte/packet limits are now deleted immediately, just
like SAs that have reached their hard time limits.

Previously they were announced immediately but only deleted after
30 seconds.

This is bad because it prevents the system from issuing an ACQUIRE
command until the existing state was deleted by the user or expires
after the time is up.

In the scenario where the expire notification was lost this introduces
a 30 second delay into the system for no good reason.
 
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

 include/net/xfrm.h     |    2 +-
 net/xfrm/xfrm_policy.c |    8 +++++---
 net/xfrm/xfrm_state.c  |   16 +++++++---------
 3 files changed, 13 insertions(+), 13 deletions(-)

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

[-- Attachment #2: xfrm-event-2 --]
[-- Type: text/plain, Size: 2560 bytes --]

===== include/net/xfrm.h 1.80 vs edited =====
--- 1.80/include/net/xfrm.h	2005-04-08 23:08:26 +10:00
+++ edited/include/net/xfrm.h	2005-04-09 17:55:24 +10:00
@@ -669,7 +669,7 @@
 	return 0;
 }
 
-extern void xfrm_policy_delete(struct xfrm_policy *pol, int dir);
+extern int xfrm_policy_delete(struct xfrm_policy *pol, int dir);
 
 static inline void xfrm_sk_free_policy(struct sock *sk)
 {
===== net/xfrm/xfrm_policy.c 1.75 vs edited =====
--- 1.75/net/xfrm/xfrm_policy.c	2005-04-01 16:24:20 +10:00
+++ edited/net/xfrm/xfrm_policy.c	2005-04-09 18:02:53 +10:00
@@ -216,8 +216,8 @@
 
 expired:
 	read_unlock(&xp->lock);
-	km_policy_expired(xp, dir, 1);
-	xfrm_policy_delete(xp, dir);
+	if (!xfrm_policy_delete(xp, dir))
+		km_policy_expired(xp, dir, 1);
 	xfrm_pol_put(xp);
 }
 
@@ -555,7 +555,7 @@
 	return NULL;
 }
 
-void xfrm_policy_delete(struct xfrm_policy *pol, int dir)
+int xfrm_policy_delete(struct xfrm_policy *pol, int dir)
 {
 	write_lock_bh(&xfrm_policy_lock);
 	pol = __xfrm_policy_unlink(pol, dir);
@@ -564,7 +564,9 @@
 		if (dir < XFRM_POLICY_MAX)
 			atomic_inc(&flow_cache_genid);
 		xfrm_policy_kill(pol);
+		return 0;
 	}
+	return -ENOENT;
 }
 
 int xfrm_sk_policy_insert(struct sock *sk, int dir, struct xfrm_policy *pol)
===== net/xfrm/xfrm_state.c 1.61 vs edited =====
--- 1.61/net/xfrm/xfrm_state.c	2005-04-08 23:08:26 +10:00
+++ edited/net/xfrm/xfrm_state.c	2005-04-09 19:55:48 +10:00
@@ -154,6 +154,7 @@
 			next = tmo;
 	}
 
+	x->km.dying = warn;
 	if (warn)
 		km_state_expired(x, 0);
 resched:
@@ -169,9 +170,8 @@
 		next = 2;
 		goto resched;
 	}
-	if (x->id.spi != 0)
+	if (!__xfrm_state_delete(x) && x->id.spi)
 		km_state_expired(x, 1);
-	__xfrm_state_delete(x);
 
 out:
 	spin_unlock(&x->lock);
@@ -566,16 +566,18 @@
 
 	if (x->curlft.bytes >= x->lft.hard_byte_limit ||
 	    x->curlft.packets >= x->lft.hard_packet_limit) {
-		km_state_expired(x, 1);
-		if (!mod_timer(&x->timer, jiffies + XFRM_ACQ_EXPIRES*HZ))
+		x->km.state = XFRM_STATE_EXPIRED;
+		if (!mod_timer(&x->timer, jiffies))
 			xfrm_state_hold(x);
 		return -EINVAL;
 	}
 
 	if (!x->km.dying &&
 	    (x->curlft.bytes >= x->lft.soft_byte_limit ||
-	     x->curlft.packets >= x->lft.soft_packet_limit))
+	     x->curlft.packets >= x->lft.soft_packet_limit)) {
+		x->km.dying = 1;
 		km_state_expired(x, 0);
+	}
 	return 0;
 }
 EXPORT_SYMBOL(xfrm_state_check_expire);
@@ -833,10 +835,6 @@
 {
 	struct km_event c;
 
-	if (hard)
-		x->km.state = XFRM_STATE_EXPIRED;
-	else
-		x->km.dying = 1;
 	c.data = hard;
 	c.event = XFRM_SAP_EXPIRED;
 	km_state_notify(x, &c);

[3/4] [IPSEC] Turn km_event.data into a union

[Herbert Xu]

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

Hi:

This patch turns km_event.data into a union.  This makes code that
uses it clearer.
  
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

 include/net/xfrm.h    |    7 ++++++-
 net/key/af_key.c      |   17 +++++------------
 net/xfrm/xfrm_state.c |    5 ++---
 net/xfrm/xfrm_user.c  |   10 +++++-----
 4 files changed, 18 insertions(+), 21 deletions(-)
 
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

[-- Attachment #2: xfrm-event-3 --]
[-- Type: text/plain, Size: 3729 bytes --]

===== include/net/xfrm.h 1.81 vs edited =====
--- 1.81/include/net/xfrm.h	2005-04-09 20:21:15 +10:00
+++ edited/include/net/xfrm.h	2005-04-09 20:37:05 +10:00
@@ -172,7 +172,12 @@
 /* callback structure passed from either netlink or pfkey */
 struct km_event
 {
-	u32	data; 
+	union {
+		u32 hard;
+		u32 proto; 
+		u32 byid;
+	} data;
+
 	u32	seq; 
 	u32	pid; 
 	u32	event; 
===== net/key/af_key.c 1.74 vs edited =====
--- 1.74/net/key/af_key.c	2005-04-09 14:39:58 +10:00
+++ edited/net/key/af_key.c	2005-04-09 20:32:54 +10:00
@@ -1293,13 +1293,6 @@
 	if (c->event == XFRM_SAP_DELETED)
 		hsc = 0;
 
-	if (c->event == XFRM_SAP_EXPIRED) {
-		if (c->data)
-			hsc = 2;
-		else
-			hsc = 1;
-	}
-
 	skb = pfkey_xfrm_state2msg(x, 0, hsc);
 
 	if (IS_ERR(skb))
@@ -1534,7 +1527,7 @@
 	if (!skb)
 		return -ENOBUFS;
 	hdr = (struct sadb_msg *) skb_put(skb, sizeof(struct sadb_msg));
-	hdr->sadb_msg_satype = pfkey_proto2satype(c->data);
+	hdr->sadb_msg_satype = pfkey_proto2satype(c->data.proto);
 	hdr->sadb_msg_seq = c->seq;
 	hdr->sadb_msg_pid = c->pid;
 	hdr->sadb_msg_version = PF_KEY_V2;
@@ -1556,7 +1549,7 @@
 		return -EINVAL;
 
 	xfrm_state_flush(proto);
-	c.data = proto;
+	c.data.proto = proto;
 	c.seq = hdr->sadb_msg_seq;
 	c.pid = hdr->sadb_msg_pid;
 	c.event = XFRM_SAP_FLUSHED;
@@ -1969,7 +1962,7 @@
 	out_hdr = (struct sadb_msg *) out_skb->data;
 	out_hdr->sadb_msg_version =  PF_KEY_V2;
 
-	if (c->data && c->event == XFRM_SAP_DELETED)
+	if (c->data.byid && c->event == XFRM_SAP_DELETED)
 		out_hdr->sadb_msg_type = SADB_X_SPDDELETE2;
 	else
 		out_hdr->sadb_msg_type = event2poltype(c->event);
@@ -2183,7 +2176,7 @@
 	c.seq = hdr->sadb_msg_seq;
 	c.pid = hdr->sadb_msg_pid;
 	if (hdr->sadb_msg_type == SADB_X_SPDDELETE2) {
-		c.data = 1; // to signal pfkey of SADB_X_SPDDELETE2
+		c.data.byid = 1;
 		c.event = XFRM_SAP_DELETED;
 		km_policy_notify(xp, pol->sadb_x_policy_dir-1, &c);
 	} else {
@@ -2463,7 +2456,7 @@
 	int hard;
 	int hsc;
 
-	hard = c->data;
+	hard = c->data.hard;
 	if (hard)
 		hsc = 2;
 	else
===== net/xfrm/xfrm_state.c 1.62 vs edited =====
--- 1.62/net/xfrm/xfrm_state.c	2005-04-09 20:21:15 +10:00
+++ edited/net/xfrm/xfrm_state.c	2005-04-09 20:38:54 +10:00
@@ -835,7 +835,7 @@
 {
 	struct km_event c;
 
-	c.data = hard;
+	c.data.hard = hard;
 	c.event = XFRM_SAP_EXPIRED;
 	km_state_notify(x, &c);
 
@@ -883,8 +883,7 @@
 {
 	struct km_event c;
 
-	c.data = hard;
-	c.data = hard;
+	c.data.hard = hard;
 	c.event = XFRM_SAP_EXPIRED;
 	km_policy_notify(pol, dir, &c);
 
===== net/xfrm/xfrm_user.c 1.53 vs edited =====
--- 1.53/net/xfrm/xfrm_user.c	2005-04-08 23:08:26 +10:00
+++ edited/net/xfrm/xfrm_user.c	2005-04-09 20:35:06 +10:00
@@ -894,7 +894,7 @@
 	struct xfrm_usersa_flush *p = NLMSG_DATA(nlh);
 
 	xfrm_state_flush(p->proto);
-	c.data = p->proto;
+	c.data.proto = p->proto;
 	c.event = XFRM_SAP_FLUSHED;
 	c.seq = nlh->nlmsg_seq;
 	c.pid = nlh->nlmsg_pid;
@@ -1116,13 +1116,13 @@
 static int xfrm_exp_state_notify(struct xfrm_state *x, struct km_event *c)
 {
 	struct sk_buff *skb;
-	int hard = c ->data;
+
 	/* fix to do alloc using NLM macros */
 	skb = alloc_skb(sizeof(struct xfrm_user_expire) + 16, GFP_ATOMIC);
 	if (skb == NULL)
 		return -ENOMEM;
 
-	if (build_expire(skb, x, hard) < 0)
+	if (build_expire(skb, x, c->data.hard) < 0)
 		BUG();
 
 	NETLINK_CB(skb).dst_groups = XFRMGRP_EXPIRE;
@@ -1148,7 +1148,7 @@
 	nlh->nlmsg_flags = 0;
 
 	p = NLMSG_DATA(nlh);
-	p->proto = c->data;
+	p->proto = c->data.proto;
 
 	nlh->nlmsg_len = skb->tail - b;
 
@@ -1391,7 +1391,7 @@
 	if (skb == NULL)
 		return -ENOMEM;
 
-	if (build_polexpire(skb, xp, dir, c->data) < 0)
+	if (build_polexpire(skb, xp, dir, c->data.hard) < 0)
 		BUG();
 
 	NETLINK_CB(skb).dst_groups = XFRMGRP_EXPIRE;

Re: [2/4] [IPSEC] Kill spurious hard expire messages

[jamal]

Herbert,
This is nice since it seems to go on top of the patch i posted.
I didnt get an ACK from Dave - so i assumed that patch is not on his 
tree yet. Which tree are you using?

Small comment below:

On Sat, 2005-04-09 at 07:12, Herbert Xu wrote:

> ===== net/xfrm/xfrm_policy.c 1.75 vs edited =====
> --- 1.75/net/xfrm/xfrm_policy.c 2005-04-01 16:24:20 +10:00
> +++ edited/net/xfrm/xfrm_policy.c       2005-04-09 18:02:53 +10:00
> @@ -216,8 +216,8 @@
> 
> expired:
>        read_unlock(&xp->lock);
> -       km_policy_expired(xp, dir, 1);
> -       xfrm_policy_delete(xp, dir);
> +       if (!xfrm_policy_delete(xp, dir))
> +               km_policy_expired(xp, dir, 1);
>        xfrm_pol_put(xp);>
> }
>

If the policy was already dead you will still return a 0 from
xfrm_policy_delete(). Fixable by policy_kill returning an int.

cheers,
jamal

Re: [2/4] [IPSEC] Kill spurious hard expire messages

[Herbert Xu]

Hi Jamal:

On Sat, Apr 09, 2005 at 08:30:44AM -0400, jamal wrote:
> 
> This is nice since it seems to go on top of the patch i posted.
> I didnt get an ACK from Dave - so i assumed that patch is not on his 
> tree yet. Which tree are you using?

The Linus tree plus your patch.

> On Sat, 2005-04-09 at 07:12, Herbert Xu wrote:
> 
> > ===== net/xfrm/xfrm_policy.c 1.75 vs edited =====
> > --- 1.75/net/xfrm/xfrm_policy.c 2005-04-01 16:24:20 +10:00
> > +++ edited/net/xfrm/xfrm_policy.c       2005-04-09 18:02:53 +10:00
> > @@ -216,8 +216,8 @@
> > 
> > expired:
> >        read_unlock(&xp->lock);
> > -       km_policy_expired(xp, dir, 1);
> > -       xfrm_policy_delete(xp, dir);
> > +       if (!xfrm_policy_delete(xp, dir))
> > +               km_policy_expired(xp, dir, 1);
> >        xfrm_pol_put(xp);>
> > }
> >
> 
> If the policy was already dead you will still return a 0 from
> xfrm_policy_delete(). Fixable by policy_kill returning an int.

You're right.  The other callers of xfrm_policy_kill needs to
check it too.

I'll fix this up.

Thanks,
-- 
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: [2/4] [IPSEC] Kill spurious hard expire messages

[Herbert Xu]

On Sun, Apr 10, 2005 at 05:29:26AM +1000, herbert wrote:
>
> > On Sat, 2005-04-09 at 07:12, Herbert Xu wrote:
> > 
> > > ===== net/xfrm/xfrm_policy.c 1.75 vs edited =====
> > > --- 1.75/net/xfrm/xfrm_policy.c 2005-04-01 16:24:20 +10:00
> > > +++ edited/net/xfrm/xfrm_policy.c       2005-04-09 18:02:53 +10:00
> > > @@ -216,8 +216,8 @@
> > > 
> > > expired:
> > >        read_unlock(&xp->lock);
> > > -       km_policy_expired(xp, dir, 1);
> > > -       xfrm_policy_delete(xp, dir);
> > > +       if (!xfrm_policy_delete(xp, dir))
> > > +               km_policy_expired(xp, dir, 1);
> > >        xfrm_pol_put(xp);>
> > > }
> > >
> > 
> > If the policy was already dead you will still return a 0 from
> > xfrm_policy_delete(). Fixable by policy_kill returning an int.
> 
> You're right.  The other callers of xfrm_policy_kill needs to
> check it too.

Actually, my code was right :)

xfrm_policy_kill can only be called by the one who removed the
policy from the linked list.  Therefore it can never fail.

If this logic is wrong we will be getting fat warnings from
xfrm_policy_kill.

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

[4/4] [IPSEC] Set byid for km_event in xfrm_get_policy

[Herbert Xu]

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

Hi:

This patch fixes policy deletion in xfrm_user so that it sets
km_event.data.byid.  This puts xfrm_user on par with what af_key
does in this case.
   
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

 xfrm_user.c |    1 +
 1 files changed, 1 insertion(+)

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

[-- Attachment #2: xfrm-event-4 --]
[-- Type: text/plain, Size: 339 bytes --]

===== net/xfrm/xfrm_user.c 1.54 vs edited =====
--- 1.54/net/xfrm/xfrm_user.c	2005-04-09 20:40:43 +10:00
+++ edited/net/xfrm/xfrm_user.c	2005-04-09 20:41:29 +10:00
@@ -877,6 +877,7 @@
 					      MSG_DONTWAIT);
 		}
 	} else {
+		c.data.byid = p->index;
 		c.event = XFRM_SAP_DELETED;
 		c.seq = nlh->nlmsg_seq;
 		c.pid = nlh->nlmsg_pid;

[5/*] [IPSEC] Use XFRM_MSG_* instead of XFRM_SAP_*

[Herbert Xu]

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

Hi:

I spotted some more issues while preparing the last patch of the
series.  So I'll extend it a little further.

This patch removes XFRM_SAP_* and converts them over to XFRM_MSG_*.
The netlink interface is meant to map directly onto the underlying
xfrm subsystem.  Therefore rather than using a new independent
representation for the events we can simply use the existing ones
from xfrm_user.

I also killed a couple of inline's in af_key since

1) They are not performance critical.
2) None of the other x2y functions here are inlined.

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

 include/net/xfrm.h    |   12 --------
 net/key/af_key.c      |   70 +++++++++++++++++++++++++-------------------------
 net/xfrm/xfrm_state.c |    4 +-
 net/xfrm/xfrm_user.c  |   63 ++++++++++++---------------------------------
 4 files changed, 55 insertions(+), 94 deletions(-)
 
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

[-- Attachment #2: xfrm-event-5 --]
[-- Type: text/plain, Size: 9546 bytes --]

===== include/net/xfrm.h 1.82 vs edited =====
--- 1.82/include/net/xfrm.h	2005-04-09 20:40:43 +10:00
+++ edited/include/net/xfrm.h	2005-04-10 18:01:39 +10:00
@@ -157,18 +157,6 @@
 	XFRM_STATE_DEAD
 };
 
-/* events that could be sent by kernel */
-enum {
-	XFRM_SAP_INVALID,
-	XFRM_SAP_EXPIRED,
-	XFRM_SAP_ADDED,
-	XFRM_SAP_UPDATED,
-	XFRM_SAP_DELETED,
-	XFRM_SAP_FLUSHED,
-	__XFRM_SAP_MAX
-};
-#define XFRM_SAP_MAX (__XFRM_SAP_MAX - 1)
-
 /* callback structure passed from either netlink or pfkey */
 struct km_event
 {
===== net/key/af_key.c 1.75 vs edited =====
--- 1.75/net/key/af_key.c	2005-04-09 20:40:43 +10:00
+++ edited/net/key/af_key.c	2005-04-10 18:51:51 +10:00
@@ -1245,38 +1245,38 @@
 	return 0;
 }
 
-static inline int event2poltype (int event)
+static int event2poltype(int event)
 {
 	switch (event) {
-	case XFRM_SAP_DELETED:
+	case XFRM_MSG_DELPOLICY:
 		return SADB_X_SPDDELETE;
-	case XFRM_SAP_ADDED:
+	case XFRM_MSG_NEWPOLICY:
 		return SADB_X_SPDADD;
-	case XFRM_SAP_UPDATED:
+	case XFRM_MSG_UPDPOLICY:
 		return SADB_X_SPDUPDATE;
-	case XFRM_SAP_EXPIRED:
+	case XFRM_MSG_POLEXPIRE:
 	//	return SADB_X_SPDEXPIRE;
 	default:
-		printk("pfkey: Unknown policy event %d\n",event);
+		printk("pfkey: Unknown policy event %d\n", event);
 		break;
 	}
 
 	return 0;
 }
 
-static inline int event2keytype (int event)
+static int event2keytype(int event)
 {
 	switch (event) {
-	case XFRM_SAP_DELETED:
+	case XFRM_MSG_DELSA:
 		return SADB_DELETE;
-	case XFRM_SAP_ADDED:
+	case XFRM_MSG_NEWSA:
 		return SADB_ADD;
-	case XFRM_SAP_UPDATED:
+	case XFRM_MSG_UPDSA:
 		return SADB_UPDATE;
-	case XFRM_SAP_EXPIRED:
+	case XFRM_MSG_EXPIRE:
 		return SADB_EXPIRE;
 	default:
-		printk("pfkey: Unknown SA event %d\n",event);
+		printk("pfkey: Unknown SA event %d\n", event);
 		break;
 	}
 
@@ -1290,7 +1290,7 @@
 	struct sadb_msg *hdr;
 	int hsc = 3;
 
-	if (c->event == XFRM_SAP_DELETED)
+	if (c->event == XFRM_MSG_DELSA)
 		hsc = 0;
 
 	skb = pfkey_xfrm_state2msg(x, 0, hsc);
@@ -1337,9 +1337,9 @@
 	}
 
 	if (hdr->sadb_msg_type == SADB_ADD)
-		c.event = XFRM_SAP_ADDED;
+		c.event = XFRM_MSG_NEWSA;
 	else
-		c.event = XFRM_SAP_UPDATED;
+		c.event = XFRM_MSG_UPDSA;
 	c.seq = hdr->sadb_msg_seq;
 	c.pid = hdr->sadb_msg_pid;
 	km_state_notify(x, &c);
@@ -1376,7 +1376,7 @@
 
 	c.seq = hdr->sadb_msg_seq;
 	c.pid = hdr->sadb_msg_pid;
-	c.event = XFRM_SAP_DELETED;
+	c.event = XFRM_MSG_DELSA;
 	km_state_notify(x, &c);
 	xfrm_state_put(x);
 
@@ -1552,7 +1552,7 @@
 	c.data.proto = proto;
 	c.seq = hdr->sadb_msg_seq;
 	c.pid = hdr->sadb_msg_pid;
-	c.event = XFRM_SAP_FLUSHED;
+	c.event = XFRM_MSG_FLUSHSA;
 	km_state_notify(NULL, &c);
 
 	return 0;
@@ -1962,7 +1962,7 @@
 	out_hdr = (struct sadb_msg *) out_skb->data;
 	out_hdr->sadb_msg_version =  PF_KEY_V2;
 
-	if (c->data.byid && c->event == XFRM_SAP_DELETED)
+	if (c->data.byid && c->event == XFRM_MSG_DELPOLICY)
 		out_hdr->sadb_msg_type = SADB_X_SPDDELETE2;
 	else
 		out_hdr->sadb_msg_type = event2poltype(c->event);
@@ -2060,9 +2060,9 @@
 	}
 
 	if (hdr->sadb_msg_type == SADB_X_SPDUPDATE) 
-		c.event = XFRM_SAP_UPDATED;
+		c.event = XFRM_MSG_UPDPOLICY;
 	else 
-		c.event = XFRM_SAP_ADDED;
+		c.event = XFRM_MSG_NEWPOLICY;
 
 	c.seq = hdr->sadb_msg_seq;
 	c.pid = hdr->sadb_msg_pid;
@@ -2120,7 +2120,7 @@
 
 	c.seq = hdr->sadb_msg_seq;
 	c.pid = hdr->sadb_msg_pid;
-	c.event = XFRM_SAP_DELETED;
+	c.event = XFRM_MSG_DELPOLICY;
 	km_policy_notify(xp, pol->sadb_x_policy_dir-1, &c);
 
 	xfrm_pol_put(xp);
@@ -2177,7 +2177,7 @@
 	c.pid = hdr->sadb_msg_pid;
 	if (hdr->sadb_msg_type == SADB_X_SPDDELETE2) {
 		c.data.byid = 1;
-		c.event = XFRM_SAP_DELETED;
+		c.event = XFRM_MSG_DELPOLICY;
 		km_policy_notify(xp, pol->sadb_x_policy_dir-1, &c);
 	} else {
 		err = key_pol_get_resp(sk, xp, hdr, pol->sadb_x_policy_dir-1);
@@ -2240,7 +2240,7 @@
 	struct km_event c;
 
 	xfrm_policy_flush();
-	c.event = XFRM_SAP_FLUSHED;
+	c.event = XFRM_MSG_FLUSHPOLICY;
 	c.pid = hdr->sadb_msg_pid;
 	c.seq = hdr->sadb_msg_seq;
 	km_policy_notify(NULL, 0, &c);
@@ -2482,16 +2482,16 @@
 static int pfkey_send_notify(struct xfrm_state *x, struct km_event *c)
 {
 	switch (c->event) {
-	case XFRM_SAP_EXPIRED:
+	case XFRM_MSG_EXPIRE:
 		return key_notify_sa_expire(x, c);
-	case XFRM_SAP_DELETED:
-	case XFRM_SAP_ADDED:
-	case XFRM_SAP_UPDATED:
+	case XFRM_MSG_DELSA:
+	case XFRM_MSG_NEWSA:
+	case XFRM_MSG_UPDSA:
 		return key_notify_sa(x, c);
-	case XFRM_SAP_FLUSHED:
+	case XFRM_MSG_FLUSHSA:
 		return key_notify_sa_flush(c);
 	default:
-		printk("pfkey: Unknown SA event %d\n",c->event);
+		printk("pfkey: Unknown SA event %d\n", c->event);
 		break;
 	}
 
@@ -2501,16 +2501,16 @@
 static int pfkey_send_policy_notify(struct xfrm_policy *xp, int dir, struct km_event *c)
 {
 	switch (c->event) {
-	case XFRM_SAP_EXPIRED:
+	case XFRM_MSG_POLEXPIRE:
 		return key_notify_policy_expire(xp, c);
-	case XFRM_SAP_DELETED:
-	case XFRM_SAP_ADDED:
-	case XFRM_SAP_UPDATED:
+	case XFRM_MSG_DELPOLICY:
+	case XFRM_MSG_NEWPOLICY:
+	case XFRM_MSG_UPDPOLICY:
 		return key_notify_policy(xp, dir, c);
-	case XFRM_SAP_FLUSHED:
+	case XFRM_MSG_FLUSHPOLICY:
 		return key_notify_policy_flush(c);
 	default:
-		printk("pfkey: Unknown policy event %d\n",c->event);
+		printk("pfkey: Unknown policy event %d\n", c->event);
 		break;
 	}
 
===== net/xfrm/xfrm_state.c 1.63 vs edited =====
--- 1.63/net/xfrm/xfrm_state.c	2005-04-09 20:40:43 +10:00
+++ edited/net/xfrm/xfrm_state.c	2005-04-10 18:55:01 +10:00
@@ -836,7 +836,7 @@
 	struct km_event c;
 
 	c.data.hard = hard;
-	c.event = XFRM_SAP_EXPIRED;
+	c.event = XFRM_MSG_EXPIRE;
 	km_state_notify(x, &c);
 
 	if (hard)
@@ -884,7 +884,7 @@
 	struct km_event c;
 
 	c.data.hard = hard;
-	c.event = XFRM_SAP_EXPIRED;
+	c.event = XFRM_MSG_POLEXPIRE;
 	km_policy_notify(pol, dir, &c);
 
 	if (hard)
===== net/xfrm/xfrm_user.c 1.55 vs edited =====
--- 1.55/net/xfrm/xfrm_user.c	2005-04-10 17:45:12 +10:00
+++ edited/net/xfrm/xfrm_user.c	2005-04-10 18:05:53 +10:00
@@ -292,10 +292,7 @@
 
 	c.seq = nlh->nlmsg_seq;
 	c.pid = nlh->nlmsg_pid;
-	if (nlh->nlmsg_type == XFRM_MSG_NEWSA)
-		c.event = XFRM_SAP_ADDED;
-	else
-		c.event = XFRM_SAP_UPDATED;
+	c.event = nlh->nlmsg_type;
 
 	km_state_notify(x, &c);
 	xfrm_state_put(x);
@@ -327,7 +324,7 @@
 
 	c.seq = nlh->nlmsg_seq;
 	c.pid = nlh->nlmsg_pid;
-	c.event = XFRM_SAP_DELETED;
+	c.event = nlh->nlmsg_type;
 	km_state_notify(x, &c);
 	xfrm_state_put(x);
 
@@ -721,11 +718,7 @@
 	}
 
 
-	if (!excl)
-		c.event = XFRM_SAP_UPDATED;
-	else
-		c.event = XFRM_SAP_ADDED;
-
+	c.event = nlh->nlmsg_type;
 	c.seq = nlh->nlmsg_seq;
 	c.pid = nlh->nlmsg_pid;
 	km_policy_notify(xp, p->dir, &c);
@@ -878,7 +871,7 @@
 		}
 	} else {
 		c.data.byid = p->index;
-		c.event = XFRM_SAP_DELETED;
+		c.event = nlh->nlmsg_type;
 		c.seq = nlh->nlmsg_seq;
 		c.pid = nlh->nlmsg_pid;
 		km_policy_notify(xp, p->dir, &c);
@@ -896,7 +889,7 @@
 
 	xfrm_state_flush(p->proto);
 	c.data.proto = p->proto;
-	c.event = XFRM_SAP_FLUSHED;
+	c.event = nlh->nlmsg_type;
 	c.seq = nlh->nlmsg_seq;
 	c.pid = nlh->nlmsg_pid;
 	km_state_notify(NULL, &c);
@@ -909,7 +902,7 @@
 	struct km_event c;
 
 	xfrm_policy_flush();
-	c.event = XFRM_SAP_FLUSHED;
+	c.event = nlh->nlmsg_type;
 	c.seq = nlh->nlmsg_seq;
 	c.pid = nlh->nlmsg_pid;
 	km_policy_notify(NULL, 0, &c);
@@ -1180,7 +1173,6 @@
 	struct xfrm_usersa_info *p;
 	struct nlmsghdr *nlh;
 	struct sk_buff *skb;
-	u32 nlt;
 	unsigned char *b;
 	int len = xfrm_sa_len(x);
 
@@ -1189,16 +1181,7 @@
 		return -ENOMEM;
 	b = skb->tail;
 
-	if (c->event == XFRM_SAP_ADDED)
-		nlt = XFRM_MSG_NEWSA;
-	else if (c->event == XFRM_SAP_UPDATED)
-		nlt = XFRM_MSG_UPDSA;
-	else if (c->event == XFRM_SAP_DELETED)
-		nlt = XFRM_MSG_DELSA;
-	else
-		goto nlmsg_failure;
-
-	nlh = NLMSG_PUT(skb, c->pid, c->seq, nlt, sizeof(*p));
+	nlh = NLMSG_PUT(skb, c->pid, c->seq, c->event, sizeof(*p));
 	nlh->nlmsg_flags = 0;
 
 	p = NLMSG_DATA(nlh);
@@ -1230,13 +1213,13 @@
 {
 
 	switch (c->event) {
-	case XFRM_SAP_EXPIRED:
+	case XFRM_MSG_EXPIRE:
 		return xfrm_exp_state_notify(x, c);
-	case XFRM_SAP_DELETED:
-	case XFRM_SAP_UPDATED:
-	case XFRM_SAP_ADDED:
+	case XFRM_MSG_DELSA:
+	case XFRM_MSG_UPDSA:
+	case XFRM_MSG_NEWSA:
 		return xfrm_notify_sa(x, c);
-	case XFRM_SAP_FLUSHED:
+	case XFRM_MSG_FLUSHSA:
 		return xfrm_notify_sa_flush(c);
 	default:
 		 printk("xfrm_user: Unknown SA event %d\n",c->event);
@@ -1405,7 +1388,6 @@
 	struct xfrm_userpolicy_info *p;
 	struct nlmsghdr *nlh;
 	struct sk_buff *skb;
-	u32 nlt = 0 ;
 	unsigned char *b;
 	int len = RTA_SPACE(sizeof(struct xfrm_user_tmpl) * xp->xfrm_nr);
 	len += NLMSG_SPACE(sizeof(struct xfrm_userpolicy_info));
@@ -1415,16 +1397,7 @@
 		return -ENOMEM;
 	b = skb->tail;
 
-	if (c->event == XFRM_SAP_ADDED)
-		nlt = XFRM_MSG_NEWPOLICY;
-	else if (c->event == XFRM_SAP_UPDATED)
-		nlt = XFRM_MSG_UPDPOLICY;
-	else if (c->event == XFRM_SAP_DELETED)
-		nlt = XFRM_MSG_DELPOLICY;
-	else
-		goto nlmsg_failure;
-
-	nlh = NLMSG_PUT(skb, c->pid, c->seq, nlt, sizeof(*p));
+	nlh = NLMSG_PUT(skb, c->pid, c->seq, c->event, sizeof(*p));
 
 	p = NLMSG_DATA(nlh);
 
@@ -1471,13 +1444,13 @@
 {
 
 	switch (c->event) {
-	case XFRM_SAP_ADDED:
-	case XFRM_SAP_UPDATED:
-	case XFRM_SAP_DELETED:
+	case XFRM_MSG_NEWPOLICY:
+	case XFRM_MSG_UPDPOLICY:
+	case XFRM_MSG_DELPOLICY:
 		return xfrm_notify_policy(xp, dir, c);
-	case XFRM_SAP_FLUSHED:
+	case XFRM_MSG_FLUSHPOLICY:
 		return xfrm_notify_policy_flush(c);
-	case XFRM_SAP_EXPIRED:
+	case XFRM_MSG_POLEXPIRE:
 		return xfrm_exp_policy_notify(xp, dir, c);
 	default:
 		printk("xfrm_user: Unknown Policy event %d\n",c->event);

[6/*] [IPSEC] Add xfrm_userpolicy_delete for xfrm_user notification

[Herbert Xu]

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

Hi:

This patch changes the format of the XFRM_MSG_DELPOLICY notification
so that we don't lose the byid information carried in km_event.

Since this user interface is introduced by Jamal's patch we can still
afford to change it.

I know that this is a bit trivial but as it is the information carried
in xfrm_user's notifications is a strict superset of that of af_key
and I'd like to keep it that way :)
 
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

[-- Attachment #2: xfrm-event-6 --]
[-- Type: text/plain, Size: 1435 bytes --]

===== include/linux/xfrm.h 1.24 vs edited =====
--- 1.24/include/linux/xfrm.h	2005-04-08 23:08:26 +10:00
+++ edited/include/linux/xfrm.h	2005-04-10 19:25:22 +10:00
@@ -227,6 +227,11 @@
 	__u8				dir;
 };
 
+struct xfrm_userpolicy_delete {
+	struct xfrm_userpolicy_info	pol;
+	__u8				byid;
+};
+
 struct xfrm_user_acquire {
 	struct xfrm_id			id;
 	xfrm_address_t			saddr;
===== net/xfrm/xfrm_user.c 1.56 vs edited =====
--- 1.56/net/xfrm/xfrm_user.c	2005-04-10 19:03:30 +10:00
+++ edited/net/xfrm/xfrm_user.c	2005-04-10 19:30:30 +10:00
@@ -1386,20 +1386,30 @@
 static int xfrm_notify_policy( struct xfrm_policy *xp, int dir, struct km_event *c)
 {
 	struct xfrm_userpolicy_info *p;
+	struct xfrm_userpolicy_delete *pdel;
 	struct nlmsghdr *nlh;
 	struct sk_buff *skb;
 	unsigned char *b;
 	int len = RTA_SPACE(sizeof(struct xfrm_user_tmpl) * xp->xfrm_nr);
-	len += NLMSG_SPACE(sizeof(struct xfrm_userpolicy_info));
+	int payload;
+
+	payload = (c->event == XFRM_MSG_DELPOLICY) ?
+		  sizeof(*pdel) : sizeof(*p);
+	len += NLMSG_SPACE(payload);
 
 	skb = alloc_skb(len, GFP_ATOMIC);
 	if (skb == NULL)
 		return -ENOMEM;
 	b = skb->tail;
 
-	nlh = NLMSG_PUT(skb, c->pid, c->seq, c->event, sizeof(*p));
+	nlh = NLMSG_PUT(skb, c->pid, c->seq, c->event, payload);
 
 	p = NLMSG_DATA(nlh);
+	if (c->event == XFRM_MSG_DELPOLICY) {
+		pdel = NLMSG_DATA(nlh);
+		pdel->byid = !!c->data.byid;
+		p = &pdel->pol;
+	}
 
 	nlh->nlmsg_flags = 0;
 

Re: [2/4] [IPSEC] Kill spurious hard expire messages

[jamal]

On Sat, 2005-04-09 at 16:03, Herbert Xu wrote:

> xfrm_policy_kill can only be called by the one who removed the
> policy from the linked list.  Therefore it can never fail.
> 
> If this logic is wrong we will be getting fat warnings from
> xfrm_policy_kill.
> 

The warning will kick in but it may be as rare as the issue of a delete
and expire happening on the same policy that your patch was supposed to
stop ;->

cheers,
jamal

Re: [5/*] [IPSEC] Use XFRM_MSG_* instead of XFRM_SAP_*

[jamal]

On Sun, 2005-04-10 at 05:02, Herbert Xu wrote:
> Hi:
> 
> I spotted some more issues while preparing the last patch of the
> series.  So I'll extend it a little further.
> 
> This patch removes XFRM_SAP_* and converts them over to XFRM_MSG_*.
> The netlink interface is meant to map directly onto the underlying
> xfrm subsystem.  Therefore rather than using a new independent
> representation for the events we can simply use the existing ones
> from xfrm_user.
> 

The main reason i did the XFRM_SAP_* is to be able to cover a case where
a message was relevant to one KM but not the other. That may not
exist now (actually it does with policy expiration that pfkey cant
handle - but thats easy to take care of).
Hopefully XFRM_MSG_xxx is the superset and will be sufficient.

Do you have anymore patches? If not i can give these a quick test;
Masahide has a better test setup and if he has time he should as well.

cheers,
jamal

Re: [2/4] [IPSEC] Kill spurious hard expire messages

[Herbert Xu]

On Sun, Apr 10, 2005 at 10:10:44AM -0400, jamal wrote:
> On Sat, 2005-04-09 at 16:03, Herbert Xu wrote:
> 
> > xfrm_policy_kill can only be called by the one who removed the
> > policy from the linked list.  Therefore it can never fail.
> > 
> > If this logic is wrong we will be getting fat warnings from
> > xfrm_policy_kill.
> 
> The warning will kick in but it may be as rare as the issue of a delete
> and expire happening on the same policy that your patch was supposed to
> stop ;->

What I am saying is that this is impossible.  If it really bothers you
we can turn the WARN_ON into a BUG.

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: [5/*] [IPSEC] Use XFRM_MSG_* instead of XFRM_SAP_*

[Herbert Xu]

On Sun, Apr 10, 2005 at 10:15:11AM -0400, jamal wrote:
> 
> The main reason i did the XFRM_SAP_* is to be able to cover a case where
> a message was relevant to one KM but not the other. That may not
> exist now (actually it does with policy expiration that pfkey cant
> handle - but thats easy to take care of).
> Hopefully XFRM_MSG_xxx is the superset and will be sufficient.

Since xfrm_user is meant to be the native interface, this should never
happen.

> Do you have anymore patches? If not i can give these a quick test;
> Masahide has a better test setup and if he has time he should as well.

That's it for now.

Thanks,
-- 
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: [5/*] [IPSEC] Use XFRM_MSG_* instead of XFRM_SAP_*

[Masahide NAKAMURA]

> Do you have anymore patches? If not i can give these a quick test;
> Masahide has a better test setup and if he has time he should as well.

I can, but about 24 hours later. I'll test it there is no update and
if it isn't too late then.

Thanks,

-- 
Masahide NAKAMURA


jamal wrote:
> On Sun, 2005-04-10 at 05:02, Herbert Xu wrote:
> 
>>Hi:
>>
>>I spotted some more issues while preparing the last patch of the
>>series.  So I'll extend it a little further.
>>
>>This patch removes XFRM_SAP_* and converts them over to XFRM_MSG_*.
>>The netlink interface is meant to map directly onto the underlying
>>xfrm subsystem.  Therefore rather than using a new independent
>>representation for the events we can simply use the existing ones
>>from xfrm_user.
>>
> 
> 
> The main reason i did the XFRM_SAP_* is to be able to cover a case where
> a message was relevant to one KM but not the other. That may not
> exist now (actually it does with policy expiration that pfkey cant
> handle - but thats easy to take care of).
> Hopefully XFRM_MSG_xxx is the superset and will be sufficient.
> 
> Do you have anymore patches? If not i can give these a quick test;
> Masahide has a better test setup and if he has time he should as well.
> 
> cheers,
> jamal
> 
> 

Re: [2/4] [IPSEC] Kill spurious hard expire messages

[jamal]

Herbert,

On Sun, 2005-04-10 at 17:27, Herbert Xu wrote:

> 
> What I am saying is that this is impossible.  If it really bothers you
> we can turn the WARN_ON into a BUG.
> 

We started this discussion by asserting that an expire may be issued on
a policy (or state) despite a delete event notification already having
happened. Thats what we were/are trying to stop, no?
What i am saying is you are not stopping that with this:
---
  if (!xfrm_policy_delete(xp, dir))
        km_policy_expired(xp, dir, 1);
---

xfrm_policy_delete will return 0 whether the policy is dead or not.
OTOH, if it returns <0 for the case where it is dead, then an expire
event will not be issued. 

cheers,
jamal

Re: [5/*] [IPSEC] Use XFRM_MSG_* instead of XFRM_SAP_*

[jamal]

On Mon, 2005-04-11 at 01:45, Masahide NAKAMURA wrote:
> > Do you have anymore patches? If not i can give these a quick test;
> > Masahide has a better test setup and if he has time he should as well.
> 
> I can, but about 24 hours later. I'll test it there is no update and
> if it isn't too late then.
> 

I will on my part test in about 12 hours.

cheers,
jamal

Re: [2/4] [IPSEC] Kill spurious hard expire messages

[Herbert Xu]

Hi Jamal:

On Mon, Apr 11, 2005 at 07:20:20AM -0400, jamal wrote:
>
> We started this discussion by asserting that an expire may be issued on
> a policy (or state) despite a delete event notification already having
> happened. Thats what we were/are trying to stop, no?

Yes.

> What i am saying is you are not stopping that with this:
> ---
>   if (!xfrm_policy_delete(xp, dir))
>         km_policy_expired(xp, dir, 1);
> ---
> 
> xfrm_policy_delete will return 0 whether the policy is dead or not.

xfrm_policy_delete returns 0 if and only if the policy was removed
from xfrm_policy_list.

A user-initiated delete policy action can only succeed if the policy
was removed from xfrm_policy_list.

The locking in xfrm_policy ensures that a given policy can only be
removed from the xfrm_policy_list once.

Therefore knowing that xfrm_policy_delete returned 0 implies that
no user-initiated delete action can succeed either before or after
the expire event.

Whether a policy is dead or not is equivalent to whether it's on
xfrm_policy_list.  That is, while it's on xfrm_policy_list it
must be alive.  As soon as it is removed from xfrm_policy_list it
is killed by the entity that performed the removal.

As a corollary, xfrm_policy_delete returns 0 if and only if the
policy was on xfrm_policy_list prior to the call and was successfully
removed by the call and subsequently killed.

> OTOH, if it returns <0 for the case where it is dead, then an expire
> event will not be issued. 

I'm not sure what you're trying to say here.

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: [2/4] [IPSEC] Kill spurious hard expire messages

[jamal]

Herbert,

On Mon, 2005-04-11 at 07:30, Herbert Xu wrote:
> Hi Jamal:

> Therefore knowing that xfrm_policy_delete returned 0 implies that
> no user-initiated delete action can succeed either before or after
> the expire event.
> 

Ok, what you are saying is sensible, but: does this mean there was never
an issue then there was never a possibility of delete and expire
intefering with each other then?

cheers,
jamal

Re: [2/4] [IPSEC] Kill spurious hard expire messages

[Herbert Xu]

On Mon, Apr 11, 2005 at 07:57:29AM -0400, jamal wrote:
> Herbert,
> 
> Ok, what you are saying is sensible, but: does this mean there was never
> an issue then there was never a possibility of delete and expire
> intefering with each other then?

The problem before was that the timer didn't check the return value
of xfrm_policy_delete (in fact the latter didn't return anything).
xfrm_policy_delete tself always checked whether it succeeded in
deleting the entry from xfrm_policy_list.  This is used to determine
whether it should call xfrm_policy_kill.

Not checking the return value in the timer allows the following
situation to occur:

CPU 0				CPU 1
xfrm_policy_timer
	goto expired
				xfrm_get_policy(delete == 1)
					xfrm_policy_byid() succeeds
					notification is sent
	xfrm_policy_delete
	km_policy_expired <--- BUG

So as long as xfrm_policy_delete returns the correct value and it's
checked by the xfrm_policy_timer then we're OK.

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: [5/*] [IPSEC] Use XFRM_MSG_* instead of XFRM_SAP_*

[Masahide NAKAMURA]

Hello Herbert and Jamal,

jamal wrote:
> On Mon, 2005-04-11 at 01:45, Masahide NAKAMURA wrote:
> 
>>>Do you have anymore patches? If not i can give these a quick test;
>>>Masahide has a better test setup and if he has time he should as well.
>>
>>I can, but about 24 hours later. I'll test it there is no update and
>>if it isn't too late then.
>>

short report:
My testing is not completed but, I've tested below and it is fine:
 add/del/flush SP and their notifications through netlink (using modified iproute2/ip).

 new "xfrm_userpolicy_delete" works fine on this case;
 used byid=1 when deleting SP with specifying SP index.

I'll test the rest case (17 hours later):
- using pfkey
- using both sockets

Thanks,


-- 
Masahide NAKAMURA

Re: [5/*] [IPSEC] Use XFRM_MSG_* instead of XFRM_SAP_*

[jamal]

On Tue, 2005-04-12 at 04:17, Masahide NAKAMURA wrote:

> short report:
> My testing is not completed but, I've tested below and it is fine:
>  add/del/flush SP and their notifications through netlink (using modified iproute2/ip).
> 
>  new "xfrm_userpolicy_delete" works fine on this case;
>  used byid=1 when deleting SP with specifying SP index.
> 
> I'll test the rest case (17 hours later):
> - using pfkey
> - using both sockets
> 

I did basic testing with pfkey generated events (policy/state) showing
up in both. The vice-versa also looks good.
Herbert, try to push forward to Dave.

cheers,
jamal

Re: [5/*] [IPSEC] Use XFRM_MSG_* instead of XFRM_SAP_*

[Masahide NAKAMURA]

Hello Jamal and Herbert,

jamal wrote:
> On Tue, 2005-04-12 at 04:17, Masahide NAKAMURA wrote:
> 
> 
>>short report:
>>My testing is not completed but, I've tested below and it is fine:
>> add/del/flush SP and their notifications through netlink (using modified iproute2/ip).
>>
>> new "xfrm_userpolicy_delete" works fine on this case;
>> used byid=1 when deleting SP with specifying SP index.
>>
>>I'll test the rest case (17 hours later):
>>- using pfkey
>>- using both sockets
>>
> 
> 
> I did basic testing with pfkey generated events (policy/state) showing
> up in both. The vice-versa also looks good.

Thanks, Jamal.
I'm sorry to be late, I've also tested pfkey, both sockets, and using racoon
(on 2.6.12-rc2 + Jamal's patch + Herbert's 6 patches).

# If somebody is interested in, modified iproute2 for the kernel can be found at:
# http://www.linux-ipv6.org/~nakam/iproute2-xfrm-event/iproute2-xfrm-event-20050413.tar.gz
# a patch against the latest iproute2 bk tree
# http://www.linux-ipv6.org/~nakam/iproute2-xfrm-event/iproute2-xfrm-event-20050413.diff
# This iproute2 change will be sent to Stephen after the kernel released.


> Herbert, try to push forward to Dave.

Agreed.

Regards,

-- 
Masahide NAKAMURA

[0/7] [IPSEC] IPsec event notification

[Herbert Xu]

Hi:

I'm reposting the series of IPsec events patches that I will be asking
Andrew to include in mm.  It's mostly the same as what's been posted
before with some new white space clean-up's and a change to the xfrm
delete sa/policy notification.

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

[1/7] [IPSEC] Add complete xfrm event notification

[Herbert Xu]

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

From: Jamal Hadi Salim <hadi@cyberus.ca>

Heres the final patch.
What this patch provides

- netlink xfrm events
- ability to have events generated by netlink propagated to pfkey
  and vice versa.
- fixes the acquire lets-be-happy-with-one-success issue

Signed-off-by: Jamal Hadi Salim <hadi@cyberus.ca>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

I've made some white space clean-up's.  I'll include an incremental
patch showing the changes in a reply to this message.

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

[-- Attachment #2: xfrm-event-1 --]
[-- Type: text/plain, Size: 29383 bytes --]

--- a/include/linux/xfrm.h
+++ b/include/linux/xfrm.h
@@ -257,5 +257,7 @@ struct xfrm_usersa_flush {
 
 #define XFRMGRP_ACQUIRE		1
 #define XFRMGRP_EXPIRE		2
+#define XFRMGRP_SA		4
+#define XFRMGRP_POLICY		8
 
 #endif /* _LINUX_XFRM_H */
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -158,6 +158,27 @@ enum {
 	XFRM_STATE_DEAD
 };
 
+/* events that could be sent by kernel */
+enum {
+	XFRM_SAP_INVALID,
+	XFRM_SAP_EXPIRED,
+	XFRM_SAP_ADDED,
+	XFRM_SAP_UPDATED,
+	XFRM_SAP_DELETED,
+	XFRM_SAP_FLUSHED,
+	__XFRM_SAP_MAX
+};
+#define XFRM_SAP_MAX (__XFRM_SAP_MAX - 1)
+
+/* callback structure passed from either netlink or pfkey */
+struct km_event
+{
+	u32	data;
+	u32	seq;
+	u32	pid;
+	u32	event;
+};
+
 struct xfrm_type;
 struct xfrm_dst;
 struct xfrm_policy_afinfo {
@@ -179,6 +200,8 @@ struct xfrm_policy_afinfo {
 
 extern int xfrm_policy_register_afinfo(struct xfrm_policy_afinfo *afinfo);
 extern int xfrm_policy_unregister_afinfo(struct xfrm_policy_afinfo *afinfo);
+extern void km_policy_notify(struct xfrm_policy *xp, int dir, struct km_event *c);
+extern void km_state_notify(struct xfrm_state *x, struct km_event *c);
 
 #define XFRM_ACQ_EXPIRES	30
 
@@ -290,11 +313,11 @@ struct xfrm_mgr
 {
 	struct list_head	list;
 	char			*id;
-	int			(*notify)(struct xfrm_state *x, int event);
+	int			(*notify)(struct xfrm_state *x, struct km_event *c);
 	int			(*acquire)(struct xfrm_state *x, struct xfrm_tmpl *, struct xfrm_policy *xp, int dir);
 	struct xfrm_policy	*(*compile_policy)(u16 family, int opt, u8 *data, int len, int *dir);
 	int			(*new_mapping)(struct xfrm_state *x, xfrm_address_t *ipaddr, u16 sport);
-	int			(*notify_policy)(struct xfrm_policy *x, int dir, int event);
+	int			(*notify_policy)(struct xfrm_policy *x, int dir, struct km_event *c);
 };
 
 extern int xfrm_register_km(struct xfrm_mgr *km);
@@ -815,7 +838,7 @@ extern int xfrm_state_add(struct xfrm_st
 extern int xfrm_state_update(struct xfrm_state *x);
 extern struct xfrm_state *xfrm_state_lookup(xfrm_address_t *daddr, u32 spi, u8 proto, unsigned short family);
 extern struct xfrm_state *xfrm_find_acq_byseq(u32 seq);
-extern void xfrm_state_delete(struct xfrm_state *x);
+extern int xfrm_state_delete(struct xfrm_state *x);
 extern void xfrm_state_flush(u8 proto);
 extern int xfrm_replay_check(struct xfrm_state *x, u32 seq);
 extern void xfrm_replay_advance(struct xfrm_state *x, u32 seq);
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -1240,13 +1240,85 @@ static int pfkey_acquire(struct sock *sk
 	return 0;
 }
 
+static inline int event2poltype(int event)
+{
+	switch (event) {
+	case XFRM_SAP_DELETED:
+		return SADB_X_SPDDELETE;
+	case XFRM_SAP_ADDED:
+		return SADB_X_SPDADD;
+	case XFRM_SAP_UPDATED:
+		return SADB_X_SPDUPDATE;
+	case XFRM_SAP_EXPIRED:
+	//	return SADB_X_SPDEXPIRE;
+	default:
+		printk("pfkey: Unknown policy event %d\n", event);
+		break;
+	}
+
+	return 0;
+}
+
+static inline int event2keytype(int event)
+{
+	switch (event) {
+	case XFRM_SAP_DELETED:
+		return SADB_DELETE;
+	case XFRM_SAP_ADDED:
+		return SADB_ADD;
+	case XFRM_SAP_UPDATED:
+		return SADB_UPDATE;
+	case XFRM_SAP_EXPIRED:
+		return SADB_EXPIRE;
+	default:
+		printk("pfkey: Unknown SA event %d\n", event);
+		break;
+	}
+
+	return 0;
+}
+
+/* ADD/UPD/DEL */
+static int key_notify_sa(struct xfrm_state *x, struct km_event *c)
+{
+	struct sk_buff *skb;
+	struct sadb_msg *hdr;
+	int hsc = 3;
+
+	if (c->event == XFRM_SAP_DELETED)
+		hsc = 0;
+
+	if (c->event == XFRM_SAP_EXPIRED) {
+		if (c->data)
+			hsc = 2;
+		else
+			hsc = 1;
+	}
+
+	skb = pfkey_xfrm_state2msg(x, 0, hsc);
+
+	if (IS_ERR(skb))
+		return PTR_ERR(skb);
+
+	hdr = (struct sadb_msg *) skb->data;
+	hdr->sadb_msg_version = PF_KEY_V2;
+	hdr->sadb_msg_type = event2keytype(c->event);
+	hdr->sadb_msg_satype = pfkey_proto2satype(x->id.proto);
+	hdr->sadb_msg_errno = 0;
+	hdr->sadb_msg_reserved = 0;
+	hdr->sadb_msg_seq = c->seq;
+	hdr->sadb_msg_pid = c->pid;
+
+	pfkey_broadcast(skb, GFP_ATOMIC, BROADCAST_ALL, NULL);
+
+	return 0;
+}
 
 static int pfkey_add(struct sock *sk, struct sk_buff *skb, struct sadb_msg *hdr, void **ext_hdrs)
 {
-	struct sk_buff *out_skb;
-	struct sadb_msg *out_hdr;
 	struct xfrm_state *x;
 	int err;
+	struct km_event c;
 
 	xfrm_probe_algs();
 	
@@ -1254,6 +1326,7 @@ static int pfkey_add(struct sock *sk, st
 	if (IS_ERR(x))
 		return PTR_ERR(x);
 
+	xfrm_state_hold(x);
 	if (hdr->sadb_msg_type == SADB_ADD)
 		err = xfrm_state_add(x);
 	else
@@ -1265,27 +1338,23 @@ static int pfkey_add(struct sock *sk, st
 		return err;
 	}
 
-	out_skb = pfkey_xfrm_state2msg(x, 0, 3);
-	if (IS_ERR(out_skb))
-		return  PTR_ERR(out_skb); /* XXX Should we return 0 here ? */
-
-	out_hdr = (struct sadb_msg *) out_skb->data;
-	out_hdr->sadb_msg_version = hdr->sadb_msg_version;
-	out_hdr->sadb_msg_type = hdr->sadb_msg_type;
-	out_hdr->sadb_msg_satype = pfkey_proto2satype(x->id.proto);
-	out_hdr->sadb_msg_errno = 0;
-	out_hdr->sadb_msg_reserved = 0;
-	out_hdr->sadb_msg_seq = hdr->sadb_msg_seq;
-	out_hdr->sadb_msg_pid = hdr->sadb_msg_pid;
-
-	pfkey_broadcast(out_skb, GFP_ATOMIC, BROADCAST_ALL, sk);
+	if (hdr->sadb_msg_type == SADB_ADD)
+		c.event = XFRM_SAP_ADDED;
+	else
+		c.event = XFRM_SAP_UPDATED;
+	c.seq = hdr->sadb_msg_seq;
+	c.pid = hdr->sadb_msg_pid;
+	km_state_notify(x, &c);
+	xfrm_state_put(x);
 
-	return 0;
+	return err;
 }
 
 static int pfkey_delete(struct sock *sk, struct sk_buff *skb, struct sadb_msg *hdr, void **ext_hdrs)
 {
 	struct xfrm_state *x;
+	struct km_event c;
+	int err;
 
 	if (!ext_hdrs[SADB_EXT_SA-1] ||
 	    !present_and_same_family(ext_hdrs[SADB_EXT_ADDRESS_SRC-1],
@@ -1301,13 +1370,19 @@ static int pfkey_delete(struct sock *sk,
 		return -EPERM;
 	}
 	
-	xfrm_state_delete(x);
-	xfrm_state_put(x);
+	err = xfrm_state_delete(x);
+	if (err < 0) {
+		xfrm_state_put(x);
+		return err;
+	}
 
-	pfkey_broadcast(skb_clone(skb, GFP_KERNEL), GFP_KERNEL, 
-			BROADCAST_ALL, sk);
+	c.seq = hdr->sadb_msg_seq;
+	c.pid = hdr->sadb_msg_pid;
+	c.event = XFRM_SAP_DELETED;
+	km_state_notify(x, &c);
+	xfrm_state_put(x);
 
-	return 0;
+	return err;
 }
 
 static int pfkey_get(struct sock *sk, struct sk_buff *skb, struct sadb_msg *hdr, void **ext_hdrs)
@@ -1445,28 +1520,42 @@ static int pfkey_register(struct sock *s
 	return 0;
 }
 
+static int key_notify_sa_flush(struct km_event *c)
+{
+	struct sk_buff *skb;
+	struct sadb_msg *hdr;
+
+	skb = alloc_skb(sizeof(struct sadb_msg) + 16, GFP_ATOMIC);
+	if (!skb)
+		return -ENOBUFS;
+	hdr = (struct sadb_msg *) skb_put(skb, sizeof(struct sadb_msg));
+	hdr->sadb_msg_satype = pfkey_proto2satype(c->data);
+	hdr->sadb_msg_seq = c->seq;
+	hdr->sadb_msg_pid = c->pid;
+	hdr->sadb_msg_version = PF_KEY_V2;
+	hdr->sadb_msg_errno = (uint8_t) 0;
+	hdr->sadb_msg_len = (sizeof(struct sadb_msg) / sizeof(uint64_t));
+
+	pfkey_broadcast(skb, GFP_ATOMIC, BROADCAST_ALL, NULL);
+
+	return 0;
+}
+
 static int pfkey_flush(struct sock *sk, struct sk_buff *skb, struct sadb_msg *hdr, void **ext_hdrs)
 {
 	unsigned proto;
-	struct sk_buff *skb_out;
-	struct sadb_msg *hdr_out;
+	struct km_event c;
 
 	proto = pfkey_satype2proto(hdr->sadb_msg_satype);
 	if (proto == 0)
 		return -EINVAL;
 
-	skb_out = alloc_skb(sizeof(struct sadb_msg) + 16, GFP_KERNEL);
-	if (!skb_out)
-		return -ENOBUFS;
-
 	xfrm_state_flush(proto);
-
-	hdr_out = (struct sadb_msg *) skb_put(skb_out, sizeof(struct sadb_msg));
-	pfkey_hdr_dup(hdr_out, hdr);
-	hdr_out->sadb_msg_errno = (uint8_t) 0;
-	hdr_out->sadb_msg_len = (sizeof(struct sadb_msg) / sizeof(uint64_t));
-
-	pfkey_broadcast(skb_out, GFP_KERNEL, BROADCAST_ALL, NULL);
+	c.data = proto;
+	c.seq = hdr->sadb_msg_seq;
+	c.pid = hdr->sadb_msg_pid;
+	c.event = XFRM_SAP_FLUSHED;
+	km_state_notify(NULL, &c);
 
 	return 0;
 }
@@ -1859,6 +1948,35 @@ static void pfkey_xfrm_policy2msg(struct
 	hdr->sadb_msg_reserved = atomic_read(&xp->refcnt);
 }
 
+static int key_notify_policy(struct xfrm_policy *xp, int dir, struct km_event *c)
+{
+	struct sk_buff *out_skb;
+	struct sadb_msg *out_hdr;
+	int err;
+
+	out_skb = pfkey_xfrm_policy2msg_prep(xp);
+	if (IS_ERR(out_skb)) {
+		err = PTR_ERR(out_skb);
+		goto out;
+	}
+	pfkey_xfrm_policy2msg(out_skb, xp, dir);
+
+	out_hdr = (struct sadb_msg *) out_skb->data;
+	out_hdr->sadb_msg_version = PF_KEY_V2;
+
+	if (c->data && c->event == XFRM_SAP_DELETED)
+		out_hdr->sadb_msg_type = SADB_X_SPDDELETE2;
+	else
+		out_hdr->sadb_msg_type = event2poltype(c->event);
+	out_hdr->sadb_msg_errno = 0;
+	out_hdr->sadb_msg_seq = c->seq;
+	out_hdr->sadb_msg_pid = c->pid;
+	pfkey_broadcast(out_skb, GFP_ATOMIC, BROADCAST_ALL, NULL);
+out:
+	return 0;
+
+}
+
 static int pfkey_spdadd(struct sock *sk, struct sk_buff *skb, struct sadb_msg *hdr, void **ext_hdrs)
 {
 	int err;
@@ -1866,8 +1984,7 @@ static int pfkey_spdadd(struct sock *sk,
 	struct sadb_address *sa;
 	struct sadb_x_policy *pol;
 	struct xfrm_policy *xp;
-	struct sk_buff *out_skb;
-	struct sadb_msg *out_hdr;
+	struct km_event c;
 
 	if (!present_and_same_family(ext_hdrs[SADB_EXT_ADDRESS_SRC-1],
 				     ext_hdrs[SADB_EXT_ADDRESS_DST-1]) ||
@@ -1935,31 +2052,23 @@ static int pfkey_spdadd(struct sock *sk,
 	    (err = parse_ipsecrequests(xp, pol)) < 0)
 		goto out;
 
-	out_skb = pfkey_xfrm_policy2msg_prep(xp);
-	if (IS_ERR(out_skb)) {
-		err =  PTR_ERR(out_skb);
-		goto out;
-	}
-
 	err = xfrm_policy_insert(pol->sadb_x_policy_dir-1, xp,
 				 hdr->sadb_msg_type != SADB_X_SPDUPDATE);
 	if (err) {
-		kfree_skb(out_skb);
-		goto out;
+		kfree(xp);
+		return err;
 	}
 
-	pfkey_xfrm_policy2msg(out_skb, xp, pol->sadb_x_policy_dir-1);
+	if (hdr->sadb_msg_type == SADB_X_SPDUPDATE)
+		c.event = XFRM_SAP_UPDATED;
+	else
+		c.event = XFRM_SAP_ADDED;
 
-	xfrm_pol_put(xp);
+	c.seq = hdr->sadb_msg_seq;
+	c.pid = hdr->sadb_msg_pid;
 
-	out_hdr = (struct sadb_msg *) out_skb->data;
-	out_hdr->sadb_msg_version = hdr->sadb_msg_version;
-	out_hdr->sadb_msg_type = hdr->sadb_msg_type;
-	out_hdr->sadb_msg_satype = 0;
-	out_hdr->sadb_msg_errno = 0;
-	out_hdr->sadb_msg_seq = hdr->sadb_msg_seq;
-	out_hdr->sadb_msg_pid = hdr->sadb_msg_pid;
-	pfkey_broadcast(out_skb, GFP_ATOMIC, BROADCAST_ALL, sk);
+	km_policy_notify(xp, pol->sadb_x_policy_dir-1, &c);
+	xfrm_pol_put(xp);
 	return 0;
 
 out:
@@ -1973,9 +2082,8 @@ static int pfkey_spddelete(struct sock *
 	struct sadb_address *sa;
 	struct sadb_x_policy *pol;
 	struct xfrm_policy *xp;
-	struct sk_buff *out_skb;
-	struct sadb_msg *out_hdr;
 	struct xfrm_selector sel;
+	struct km_event c;
 
 	if (!present_and_same_family(ext_hdrs[SADB_EXT_ADDRESS_SRC-1],
 				     ext_hdrs[SADB_EXT_ADDRESS_DST-1]) ||
@@ -2010,25 +2118,40 @@ static int pfkey_spddelete(struct sock *
 
 	err = 0;
 
+	c.seq = hdr->sadb_msg_seq;
+	c.pid = hdr->sadb_msg_pid;
+	c.event = XFRM_SAP_DELETED;
+	km_policy_notify(xp, pol->sadb_x_policy_dir-1, &c);
+
+	xfrm_pol_put(xp);
+	return err;
+}
+
+static int key_pol_get_resp(struct sock *sk, struct xfrm_policy *xp, struct sadb_msg *hdr, int dir)
+{
+	int err;
+	struct sk_buff *out_skb;
+	struct sadb_msg *out_hdr;
+	err = 0;
+
 	out_skb = pfkey_xfrm_policy2msg_prep(xp);
 	if (IS_ERR(out_skb)) {
 		err =  PTR_ERR(out_skb);
 		goto out;
 	}
-	pfkey_xfrm_policy2msg(out_skb, xp, pol->sadb_x_policy_dir-1);
+	pfkey_xfrm_policy2msg(out_skb, xp, dir);
 
 	out_hdr = (struct sadb_msg *) out_skb->data;
 	out_hdr->sadb_msg_version = hdr->sadb_msg_version;
-	out_hdr->sadb_msg_type = SADB_X_SPDDELETE;
+	out_hdr->sadb_msg_type = hdr->sadb_msg_type;
 	out_hdr->sadb_msg_satype = 0;
 	out_hdr->sadb_msg_errno = 0;
 	out_hdr->sadb_msg_seq = hdr->sadb_msg_seq;
 	out_hdr->sadb_msg_pid = hdr->sadb_msg_pid;
-	pfkey_broadcast(out_skb, GFP_ATOMIC, BROADCAST_ALL, sk);
+	pfkey_broadcast(out_skb, GFP_ATOMIC, BROADCAST_ONE, sk);
 	err = 0;
 
 out:
-	xfrm_pol_put(xp);
 	return err;
 }
 
@@ -2037,8 +2160,7 @@ static int pfkey_spdget(struct sock *sk,
 	int err;
 	struct sadb_x_policy *pol;
 	struct xfrm_policy *xp;
-	struct sk_buff *out_skb;
-	struct sadb_msg *out_hdr;
+	struct km_event c;
 
 	if ((pol = ext_hdrs[SADB_X_EXT_POLICY-1]) == NULL)
 		return -EINVAL;
@@ -2050,24 +2172,16 @@ static int pfkey_spdget(struct sock *sk,
 
 	err = 0;
 
-	out_skb = pfkey_xfrm_policy2msg_prep(xp);
-	if (IS_ERR(out_skb)) {
-		err =  PTR_ERR(out_skb);
-		goto out;
+	c.seq = hdr->sadb_msg_seq;
+	c.pid = hdr->sadb_msg_pid;
+	if (hdr->sadb_msg_type == SADB_X_SPDDELETE2) {
+		c.data = 1; // to signal pfkey of SADB_X_SPDDELETE2
+		c.event = XFRM_SAP_DELETED;
+		km_policy_notify(xp, pol->sadb_x_policy_dir-1, &c);
+	} else {
+		err = key_pol_get_resp(sk, xp, hdr, pol->sadb_x_policy_dir-1);
 	}
-	pfkey_xfrm_policy2msg(out_skb, xp, pol->sadb_x_policy_dir-1);
 
-	out_hdr = (struct sadb_msg *) out_skb->data;
-	out_hdr->sadb_msg_version = hdr->sadb_msg_version;
-	out_hdr->sadb_msg_type = hdr->sadb_msg_type;
-	out_hdr->sadb_msg_satype = 0;
-	out_hdr->sadb_msg_errno = 0;
-	out_hdr->sadb_msg_seq = hdr->sadb_msg_seq;
-	out_hdr->sadb_msg_pid = hdr->sadb_msg_pid;
-	pfkey_broadcast(out_skb, GFP_ATOMIC, BROADCAST_ALL, sk);
-	err = 0;
-
-out:
 	xfrm_pol_put(xp);
 	return err;
 }
@@ -2102,22 +2216,34 @@ static int pfkey_spddump(struct sock *sk
 	return xfrm_policy_walk(dump_sp, &data);
 }
 
-static int pfkey_spdflush(struct sock *sk, struct sk_buff *skb, struct sadb_msg *hdr, void **ext_hdrs)
+static int key_notify_policy_flush(struct km_event *c)
 {
 	struct sk_buff *skb_out;
-	struct sadb_msg *hdr_out;
+	struct sadb_msg *hdr;
 
-	skb_out = alloc_skb(sizeof(struct sadb_msg) + 16, GFP_KERNEL);
+	skb_out = alloc_skb(sizeof(struct sadb_msg) + 16, GFP_ATOMIC);
 	if (!skb_out)
 		return -ENOBUFS;
+	hdr = (struct sadb_msg *) skb_put(skb_out, sizeof(struct sadb_msg));
+	hdr->sadb_msg_seq = c->seq;
+	hdr->sadb_msg_pid = c->pid;
+	hdr->sadb_msg_version = PF_KEY_V2;
+	hdr->sadb_msg_errno = (uint8_t) 0;
+	hdr->sadb_msg_len = (sizeof(struct sadb_msg) / sizeof(uint64_t));
+	pfkey_broadcast(skb_out, GFP_ATOMIC, BROADCAST_ALL, NULL);
+	return 0;
 
-	xfrm_policy_flush();
+}
+
+static int pfkey_spdflush(struct sock *sk, struct sk_buff *skb, struct sadb_msg *hdr, void **ext_hdrs)
+{
+	struct km_event c;
 
-	hdr_out = (struct sadb_msg *) skb_put(skb_out, sizeof(struct sadb_msg));
-	pfkey_hdr_dup(hdr_out, hdr);
-	hdr_out->sadb_msg_errno = (uint8_t) 0;
-	hdr_out->sadb_msg_len = (sizeof(struct sadb_msg) / sizeof(uint64_t));
-	pfkey_broadcast(skb_out, GFP_KERNEL, BROADCAST_ALL, NULL);
+	xfrm_policy_flush();
+	c.event = XFRM_SAP_FLUSHED;
+	c.pid = hdr->sadb_msg_pid;
+	c.seq = hdr->sadb_msg_seq;
+	km_policy_notify(NULL, 0, &c);
 
 	return 0;
 }
@@ -2317,11 +2443,23 @@ static void dump_esp_combs(struct sk_buf
 	}
 }
 
-static int pfkey_send_notify(struct xfrm_state *x, int hard)
+static int key_notify_policy_expire(struct xfrm_policy *xp, struct km_event *c)
+{
+	return 0;
+}
+
+static int key_notify_sa_expire(struct xfrm_state *x, struct km_event *c)
 {
 	struct sk_buff *out_skb;
 	struct sadb_msg *out_hdr;
-	int hsc = (hard ? 2 : 1);
+	int hard;
+	int hsc;
+
+	hard = c->data;
+	if (hard)
+		hsc = 2;
+	else
+		hsc = 1;
 
 	out_skb = pfkey_xfrm_state2msg(x, 0, hsc);
 	if (IS_ERR(out_skb))
@@ -2340,6 +2478,44 @@ static int pfkey_send_notify(struct xfrm
 	return 0;
 }
 
+static int pfkey_send_notify(struct xfrm_state *x, struct km_event *c)
+{
+	switch (c->event) {
+	case XFRM_SAP_EXPIRED:
+		return key_notify_sa_expire(x, c);
+	case XFRM_SAP_DELETED:
+	case XFRM_SAP_ADDED:
+	case XFRM_SAP_UPDATED:
+		return key_notify_sa(x, c);
+	case XFRM_SAP_FLUSHED:
+		return key_notify_sa_flush(c);
+	default:
+		printk("pfkey: Unknown SA event %d\n", c->event);
+		break;
+	}
+
+	return 0;
+}
+
+static int pfkey_send_policy_notify(struct xfrm_policy *xp, int dir, struct km_event *c)
+{
+	switch (c->event) {
+	case XFRM_SAP_EXPIRED:
+		return key_notify_policy_expire(xp, c);
+	case XFRM_SAP_DELETED:
+	case XFRM_SAP_ADDED:
+	case XFRM_SAP_UPDATED:
+		return key_notify_policy(xp, dir, c);
+	case XFRM_SAP_FLUSHED:
+		return key_notify_policy_flush(c);
+	default:
+		printk("pfkey: Unknown policy event %d\n", c->event);
+		break;
+	}
+
+	return 0;
+}
+
 static u32 get_acqseq(void)
 {
 	u32 res;
@@ -2856,6 +3032,7 @@ static struct xfrm_mgr pfkeyv2_mgr =
 	.acquire	= pfkey_send_acquire,
 	.compile_policy	= pfkey_compile_policy,
 	.new_mapping	= pfkey_send_new_mapping,
+	.notify_policy	= pfkey_send_policy_notify,
 };
 
 static void __exit ipsec_pfkey_exit(void)
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -50,7 +50,7 @@ static DEFINE_SPINLOCK(xfrm_state_gc_loc
 
 static int xfrm_state_gc_flush_bundles;
 
-static void __xfrm_state_delete(struct xfrm_state *x);
+static int __xfrm_state_delete(struct xfrm_state *x);
 
 static struct xfrm_state_afinfo *xfrm_state_get_afinfo(unsigned short family);
 static void xfrm_state_put_afinfo(struct xfrm_state_afinfo *afinfo);
@@ -215,8 +215,10 @@ void __xfrm_state_destroy(struct xfrm_st
 }
 EXPORT_SYMBOL(__xfrm_state_destroy);
 
-static void __xfrm_state_delete(struct xfrm_state *x)
+static int __xfrm_state_delete(struct xfrm_state *x)
 {
+	int err = -ESRCH;
+
 	if (x->km.state != XFRM_STATE_DEAD) {
 		x->km.state = XFRM_STATE_DEAD;
 		spin_lock(&xfrm_state_lock);
@@ -245,14 +247,21 @@ static void __xfrm_state_delete(struct x
 		 * is what we are dropping here.
 		 */
 		atomic_dec(&x->refcnt);
+		err = 0;
 	}
+
+	return err;
 }
 
-void xfrm_state_delete(struct xfrm_state *x)
+int xfrm_state_delete(struct xfrm_state *x)
 {
+	int err;
+
 	spin_lock_bh(&x->lock);
-	__xfrm_state_delete(x);
+	err = __xfrm_state_delete(x);
 	spin_unlock_bh(&x->lock);
+
+	return err;
 }
 EXPORT_SYMBOL(xfrm_state_delete);
 
@@ -796,34 +805,60 @@ EXPORT_SYMBOL(xfrm_replay_advance);
 static struct list_head xfrm_km_list = LIST_HEAD_INIT(xfrm_km_list);
 static DEFINE_RWLOCK(xfrm_km_lock);
 
-static void km_state_expired(struct xfrm_state *x, int hard)
+void km_policy_notify(struct xfrm_policy *xp, int dir, struct km_event *c)
 {
 	struct xfrm_mgr *km;
 
-	if (hard)
-		x->km.state = XFRM_STATE_EXPIRED;
-	else
-		x->km.dying = 1;
+	read_lock(&xfrm_km_lock);
+	list_for_each_entry(km, &xfrm_km_list, list)
+		if (km->notify_policy)
+			km->notify_policy(xp, dir, c);
+	read_unlock(&xfrm_km_lock);
+}
 
+void km_state_notify(struct xfrm_state *x, struct km_event *c)
+{
+	struct xfrm_mgr *km;
 	read_lock(&xfrm_km_lock);
 	list_for_each_entry(km, &xfrm_km_list, list)
-		km->notify(x, hard);
+		if (km->notify)
+			km->notify(x, c);
 	read_unlock(&xfrm_km_lock);
+}
+
+EXPORT_SYMBOL(km_policy_notify);
+EXPORT_SYMBOL(km_state_notify);
+
+static void km_state_expired(struct xfrm_state *x, int hard)
+{
+	struct km_event c;
+
+	if (hard)
+		x->km.state = XFRM_STATE_EXPIRED;
+	else
+		x->km.dying = 1;
+	c.data = hard;
+	c.event = XFRM_SAP_EXPIRED;
+	km_state_notify(x, &c);
 
 	if (hard)
 		wake_up(&km_waitq);
 }
 
+/*
+ * We send to all registered managers regardless of failure
+ * We are happy with one success
+*/
 static int km_query(struct xfrm_state *x, struct xfrm_tmpl *t, struct xfrm_policy *pol)
 {
-	int err = -EINVAL;
+	int err = -EINVAL, acqret;
 	struct xfrm_mgr *km;
 
 	read_lock(&xfrm_km_lock);
 	list_for_each_entry(km, &xfrm_km_list, list) {
-		err = km->acquire(x, t, pol, XFRM_POLICY_OUT);
-		if (!err)
-			break;
+		acqret = km->acquire(x, t, pol, XFRM_POLICY_OUT);
+		if (!acqret)
+			err = acqret;
 	}
 	read_unlock(&xfrm_km_lock);
 	return err;
@@ -848,13 +883,12 @@ EXPORT_SYMBOL(km_new_mapping);
 
 void km_policy_expired(struct xfrm_policy *pol, int dir, int hard)
 {
-	struct xfrm_mgr *km;
+	struct km_event c;
 
-	read_lock(&xfrm_km_lock);
-	list_for_each_entry(km, &xfrm_km_list, list)
-		if (km->notify_policy)
-			km->notify_policy(pol, dir, hard);
-	read_unlock(&xfrm_km_lock);
+	c.data = hard;
+	c.data = hard;
+	c.event = XFRM_SAP_EXPIRED;
+	km_policy_notify(pol, dir, &c);
 
 	if (hard)
 		wake_up(&km_waitq);
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -268,6 +268,7 @@ static int xfrm_add_sa(struct sk_buff *s
 	struct xfrm_usersa_info *p = NLMSG_DATA(nlh);
 	struct xfrm_state *x;
 	int err;
+	struct km_event c;
 
 	err = verify_newsa_info(p, (struct rtattr **) xfrma);
 	if (err)
@@ -277,6 +278,7 @@ static int xfrm_add_sa(struct sk_buff *s
 	if (!x)
 		return err;
 
+	xfrm_state_hold(x);
 	if (nlh->nlmsg_type == XFRM_MSG_NEWSA)
 		err = xfrm_state_add(x);
 	else
@@ -285,14 +287,27 @@ static int xfrm_add_sa(struct sk_buff *s
 	if (err < 0) {
 		x->km.state = XFRM_STATE_DEAD;
 		xfrm_state_put(x);
+		return err;
 	}
 
+	c.seq = nlh->nlmsg_seq;
+	c.pid = nlh->nlmsg_pid;
+	if (nlh->nlmsg_type == XFRM_MSG_NEWSA)
+		c.event = XFRM_SAP_ADDED;
+	else
+		c.event = XFRM_SAP_UPDATED;
+
+	km_state_notify(x, &c);
+	xfrm_state_put(x);
+
 	return err;
 }
 
 static int xfrm_del_sa(struct sk_buff *skb, struct nlmsghdr *nlh, void **xfrma)
 {
 	struct xfrm_state *x;
+	int err;
+	struct km_event c;
 	struct xfrm_usersa_id *p = NLMSG_DATA(nlh);
 
 	x = xfrm_state_lookup(&p->daddr, p->spi, p->proto, p->family);
@@ -304,10 +319,19 @@ static int xfrm_del_sa(struct sk_buff *s
 		return -EPERM;
 	}
 
-	xfrm_state_delete(x);
+	err = xfrm_state_delete(x);
+	if (err < 0) {
+		xfrm_state_put(x);
+		return err;
+	}
+
+	c.seq = nlh->nlmsg_seq;
+	c.pid = nlh->nlmsg_pid;
+	c.event = XFRM_SAP_DELETED;
+	km_state_notify(x, &c);
 	xfrm_state_put(x);
 
-	return 0;
+	return err;
 }
 
 static void copy_to_user_state(struct xfrm_state *x, struct xfrm_usersa_info *p)
@@ -672,6 +696,7 @@ static int xfrm_add_policy(struct sk_buf
 {
 	struct xfrm_userpolicy_info *p = NLMSG_DATA(nlh);
 	struct xfrm_policy *xp;
+	struct km_event c;
 	int err;
 	int excl;
 
@@ -683,6 +708,10 @@ static int xfrm_add_policy(struct sk_buf
 	if (!xp)
 		return err;
 
+	/* shouldnt excl be based on nlh flags??
+	 * Aha! this is anti-netlink really i.e  more pfkey derived
+	 * in netlink excl is a flag and you wouldnt need
+	 * a type XFRM_MSG_UPDPOLICY - JHS */
 	excl = nlh->nlmsg_type == XFRM_MSG_NEWPOLICY;
 	err = xfrm_policy_insert(p->dir, xp, excl);
 	if (err) {
@@ -690,6 +719,15 @@ static int xfrm_add_policy(struct sk_buf
 		return err;
 	}
 
+	if (!excl)
+		c.event = XFRM_SAP_UPDATED;
+	else
+		c.event = XFRM_SAP_ADDED;
+
+	c.seq = nlh->nlmsg_seq;
+	c.pid = nlh->nlmsg_pid;
+	km_policy_notify(xp, p->dir, &c);
+
 	xfrm_pol_put(xp);
 
 	return 0;
@@ -807,6 +845,7 @@ static int xfrm_get_policy(struct sk_buf
 	struct xfrm_policy *xp;
 	struct xfrm_userpolicy_id *p;
 	int err;
+	struct km_event c;
 	int delete;
 
 	p = NLMSG_DATA(nlh);
@@ -834,6 +873,11 @@ static int xfrm_get_policy(struct sk_buf
 					      NETLINK_CB(skb).pid,
 					      MSG_DONTWAIT);
 		}
+	} else {
+		c.event = XFRM_SAP_DELETED;
+		c.seq = nlh->nlmsg_seq;
+		c.pid = nlh->nlmsg_pid;
+		km_policy_notify(xp, p->dir, &c);
 	}
 
 	xfrm_pol_put(xp);
@@ -843,15 +887,28 @@ static int xfrm_get_policy(struct sk_buf
 
 static int xfrm_flush_sa(struct sk_buff *skb, struct nlmsghdr *nlh, void **xfrma)
 {
+	struct km_event c;
 	struct xfrm_usersa_flush *p = NLMSG_DATA(nlh);
 
 	xfrm_state_flush(p->proto);
+	c.data = p->proto;
+	c.event = XFRM_SAP_FLUSHED;
+	c.seq = nlh->nlmsg_seq;
+	c.pid = nlh->nlmsg_pid;
+	km_state_notify(NULL, &c);
+
 	return 0;
 }
 
 static int xfrm_flush_policy(struct sk_buff *skb, struct nlmsghdr *nlh, void **xfrma)
 {
+	struct km_event c;
+
 	xfrm_policy_flush();
+	c.event = XFRM_SAP_FLUSHED;
+	c.seq = nlh->nlmsg_seq;
+	c.pid = nlh->nlmsg_pid;
+	km_policy_notify(NULL, 0, &c);
 	return 0;
 }
 
@@ -1060,10 +1117,12 @@ nlmsg_failure:
 	return -1;
 }
 
-static int xfrm_send_state_notify(struct xfrm_state *x, int hard)
+static int xfrm_exp_state_notify(struct xfrm_state *x, struct km_event *c)
 {
 	struct sk_buff *skb;
+	int hard = c ->data;
 
+	/* fix to do alloc using NLM macros */
 	skb = alloc_skb(sizeof(struct xfrm_user_expire) + 16, GFP_ATOMIC);
 	if (skb == NULL)
 		return -ENOMEM;
@@ -1076,6 +1135,122 @@ static int xfrm_send_state_notify(struct
 	return netlink_broadcast(xfrm_nl, skb, 0, XFRMGRP_EXPIRE, GFP_ATOMIC);
 }
 
+static int xfrm_notify_sa_flush(struct km_event *c)
+{
+	struct xfrm_usersa_flush *p;
+	struct nlmsghdr *nlh;
+	struct sk_buff *skb;
+	unsigned char *b;
+	int len = NLMSG_LENGTH(sizeof(struct xfrm_usersa_flush));
+
+	skb = alloc_skb(len, GFP_ATOMIC);
+	if (skb == NULL)
+		return -ENOMEM;
+	b = skb->tail;
+
+	nlh = NLMSG_PUT(skb, c->pid, c->seq,
+			XFRM_MSG_FLUSHSA, sizeof(*p));
+	nlh->nlmsg_flags = 0;
+
+	p = NLMSG_DATA(nlh);
+	p->proto = c->data;
+
+	nlh->nlmsg_len = skb->tail - b;
+
+	return netlink_broadcast(xfrm_nl, skb, 0, XFRMGRP_SA, GFP_ATOMIC);
+
+nlmsg_failure:
+	kfree_skb(skb);
+	return -1;
+}
+
+static int inline xfrm_sa_len(struct xfrm_state *x)
+{
+	int l = NLMSG_LENGTH(sizeof(struct xfrm_usersa_info));
+	if (x->aalg)
+		l += RTA_SPACE(sizeof(*x->aalg) + (x->aalg->alg_key_len+7)/8);
+	if (x->ealg)
+		l += RTA_SPACE(sizeof(*x->ealg) + (x->ealg->alg_key_len+7)/8);
+	if (x->calg)
+		l += RTA_SPACE(sizeof(*x->calg));
+	if (x->encap)
+		l += RTA_SPACE(sizeof(*x->encap));
+
+	return l;
+}
+
+static int xfrm_notify_sa(struct xfrm_state *x, struct km_event *c)
+{
+	struct xfrm_usersa_info *p;
+	struct nlmsghdr *nlh;
+	struct sk_buff *skb;
+	u32 nlt;
+	unsigned char *b;
+	int len = xfrm_sa_len(x);
+
+	skb = alloc_skb(len, GFP_ATOMIC);
+	if (skb == NULL)
+		return -ENOMEM;
+	b = skb->tail;
+
+	if (c->event == XFRM_SAP_ADDED)
+		nlt = XFRM_MSG_NEWSA;
+	else if (c->event == XFRM_SAP_UPDATED)
+		nlt = XFRM_MSG_UPDSA;
+	else if (c->event == XFRM_SAP_DELETED)
+		nlt = XFRM_MSG_DELSA;
+	else
+		goto nlmsg_failure;
+
+	nlh = NLMSG_PUT(skb, c->pid, c->seq, nlt, sizeof(*p));
+	nlh->nlmsg_flags = 0;
+
+	p = NLMSG_DATA(nlh);
+	copy_to_user_state(x, p);
+
+	if (x->aalg)
+		RTA_PUT(skb, XFRMA_ALG_AUTH,
+			sizeof(*(x->aalg))+(x->aalg->alg_key_len+7)/8, x->aalg);
+	if (x->ealg)
+		RTA_PUT(skb, XFRMA_ALG_CRYPT,
+			sizeof(*(x->ealg))+(x->ealg->alg_key_len+7)/8, x->ealg);
+	if (x->calg)
+		RTA_PUT(skb, XFRMA_ALG_COMP, sizeof(*(x->calg)), x->calg);
+
+	if (x->encap)
+		RTA_PUT(skb, XFRMA_ENCAP, sizeof(*x->encap), x->encap);
+
+	nlh->nlmsg_len = skb->tail - b;
+
+	return netlink_broadcast(xfrm_nl, skb, 0, XFRMGRP_SA, GFP_ATOMIC);
+
+nlmsg_failure:
+rtattr_failure:
+	kfree_skb(skb);
+	return -1;
+}
+
+static int xfrm_send_state_notify(struct xfrm_state *x, struct km_event *c)
+{
+
+	switch (c->event) {
+	case XFRM_SAP_EXPIRED:
+		return xfrm_exp_state_notify(x, c);
+	case XFRM_SAP_DELETED:
+	case XFRM_SAP_UPDATED:
+	case XFRM_SAP_ADDED:
+		return xfrm_notify_sa(x, c);
+	case XFRM_SAP_FLUSHED:
+		return xfrm_notify_sa_flush(c);
+	default:
+		 printk("xfrm_user: Unknown SA event %d\n", c->event);
+		 break;
+	}
+
+	return 0;
+
+}
+
 static int build_acquire(struct sk_buff *skb, struct xfrm_state *x,
 			 struct xfrm_tmpl *xt, struct xfrm_policy *xp,
 			 int dir)
@@ -1209,7 +1384,7 @@ nlmsg_failure:
 	return -1;
 }
 
-static int xfrm_send_policy_notify(struct xfrm_policy *xp, int dir, int hard)
+static int xfrm_exp_policy_notify(struct xfrm_policy *xp, int dir, struct km_event *c)
 {
 	struct sk_buff *skb;
 	size_t len;
@@ -1220,7 +1395,7 @@ static int xfrm_send_policy_notify(struc
 	if (skb == NULL)
 		return -ENOMEM;
 
-	if (build_polexpire(skb, xp, dir, hard) < 0)
+	if (build_polexpire(skb, xp, dir, c->data) < 0)
 		BUG();
 
 	NETLINK_CB(skb).dst_groups = XFRMGRP_EXPIRE;
@@ -1228,6 +1403,93 @@ static int xfrm_send_policy_notify(struc
 	return netlink_broadcast(xfrm_nl, skb, 0, XFRMGRP_EXPIRE, GFP_ATOMIC);
 }
 
+static int xfrm_notify_policy(struct xfrm_policy *xp, int dir, struct km_event *c)
+{
+	struct xfrm_userpolicy_info *p;
+	struct nlmsghdr *nlh;
+	struct sk_buff *skb;
+	u32 nlt = 0 ;
+	unsigned char *b;
+	int len = RTA_SPACE(sizeof(struct xfrm_user_tmpl) * xp->xfrm_nr);
+	len += NLMSG_SPACE(sizeof(struct xfrm_userpolicy_info));
+
+	skb = alloc_skb(len, GFP_ATOMIC);
+	if (skb == NULL)
+		return -ENOMEM;
+	b = skb->tail;
+
+	if (c->event == XFRM_SAP_ADDED)
+		nlt = XFRM_MSG_NEWPOLICY;
+	else if (c->event == XFRM_SAP_UPDATED)
+		nlt = XFRM_MSG_UPDPOLICY;
+	else if (c->event == XFRM_SAP_DELETED)
+		nlt = XFRM_MSG_DELPOLICY;
+	else
+		goto nlmsg_failure;
+
+	nlh = NLMSG_PUT(skb, c->pid, c->seq, nlt, sizeof(*p));
+
+	p = NLMSG_DATA(nlh);
+
+	nlh->nlmsg_flags = 0;
+
+	copy_to_user_policy(xp, p, dir);
+	if (copy_to_user_tmpl(xp, skb) < 0)
+		goto nlmsg_failure;
+
+	nlh->nlmsg_len = skb->tail - b;
+
+	return netlink_broadcast(xfrm_nl, skb, 0, XFRMGRP_POLICY, GFP_ATOMIC);
+
+nlmsg_failure:
+	kfree_skb(skb);
+	return -1;
+}
+
+static int xfrm_notify_policy_flush(struct km_event *c)
+{
+	struct nlmsghdr *nlh;
+	struct sk_buff *skb;
+	unsigned char *b;
+	int len = NLMSG_LENGTH(0);
+
+	skb = alloc_skb(len, GFP_ATOMIC);
+	if (skb == NULL)
+		return -ENOMEM;
+	b = skb->tail;
+
+
+	nlh = NLMSG_PUT(skb, c->pid, c->seq, XFRM_MSG_FLUSHPOLICY, 0);
+
+	nlh->nlmsg_len = skb->tail - b;
+
+	return netlink_broadcast(xfrm_nl, skb, 0, XFRMGRP_POLICY, GFP_ATOMIC);
+
+nlmsg_failure:
+	kfree_skb(skb);
+	return -1;
+}
+
+static int xfrm_send_policy_notify(struct xfrm_policy *xp, int dir, struct km_event *c)
+{
+
+	switch (c->event) {
+	case XFRM_SAP_ADDED:
+	case XFRM_SAP_UPDATED:
+	case XFRM_SAP_DELETED:
+		return xfrm_notify_policy(xp, dir, c);
+	case XFRM_SAP_FLUSHED:
+		return xfrm_notify_policy_flush(c);
+	case XFRM_SAP_EXPIRED:
+		return xfrm_exp_policy_notify(xp, dir, c);
+	default:
+		printk("xfrm_user: Unknown Policy event %d\n", c->event);
+	}
+
+	return 0;
+
+}
+
 static struct xfrm_mgr netlink_mgr = {
 	.id		= "netlink",
 	.notify		= xfrm_send_state_notify,

Re: [1/7] [IPSEC] Add complete xfrm event notification

[Herbert Xu]

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

On Sat, May 07, 2005 at 05:18:24PM +1000, herbert wrote:
> 
> I've made some white space clean-up's.  I'll include an incremental
> patch showing the changes in a reply to this message.

Here goes.
-- 
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

[-- Attachment #2: xfrm-event-white-space --]
[-- Type: text/plain, Size: 7631 bytes --]

diff -urN old/include/net/xfrm.h new/include/net/xfrm.h
--- old/include/net/xfrm.h	2005-05-07 15:54:50.000000000 +1000
+++ new/include/net/xfrm.h	2005-05-07 15:38:09.000000000 +1000
@@ -173,13 +173,12 @@
 /* callback structure passed from either netlink or pfkey */
 struct km_event
 {
-	u32	data; 
-	u32	seq; 
-	u32	pid; 
-	u32	event; 
+	u32	data;
+	u32	seq;
+	u32	pid;
+	u32	event;
 };
 
-
 struct xfrm_type;
 struct xfrm_dst;
 struct xfrm_policy_afinfo {
@@ -204,7 +203,6 @@
 extern void km_policy_notify(struct xfrm_policy *xp, int dir, struct km_event *c);
 extern void km_state_notify(struct xfrm_state *x, struct km_event *c);
 
-
 #define XFRM_ACQ_EXPIRES	30
 
 struct xfrm_tmpl;
@@ -309,8 +307,8 @@
 	struct xfrm_tmpl       	xfrm_vec[XFRM_MAX_DEPTH];
 };
 
-
 #define XFRM_KM_TIMEOUT		30
+
 struct xfrm_mgr
 {
 	struct list_head	list;
diff -urN old/net/key/af_key.c new/net/key/af_key.c
--- old/net/key/af_key.c	2005-05-07 15:54:50.000000000 +1000
+++ new/net/key/af_key.c	2005-05-07 15:51:42.000000000 +1000
@@ -1240,7 +1240,7 @@
 	return 0;
 }
 
-static inline int event2poltype (int event)
+static inline int event2poltype(int event)
 {
 	switch (event) {
 	case XFRM_SAP_DELETED:
@@ -1252,14 +1252,14 @@
 	case XFRM_SAP_EXPIRED:
 	//	return SADB_X_SPDEXPIRE;
 	default:
-		printk("pfkey: Unknown policy event %d\n",event);
+		printk("pfkey: Unknown policy event %d\n", event);
 		break;
 	}
 
 	return 0;
 }
 
-static inline int event2keytype (int event)
+static inline int event2keytype(int event)
 {
 	switch (event) {
 	case XFRM_SAP_DELETED:
@@ -1271,7 +1271,7 @@
 	case XFRM_SAP_EXPIRED:
 		return SADB_EXPIRE;
 	default:
-		printk("pfkey: Unknown SA event %d\n",event);
+		printk("pfkey: Unknown SA event %d\n", event);
 		break;
 	}
 
@@ -1298,7 +1298,7 @@
 	skb = pfkey_xfrm_state2msg(x, 0, hsc);
 
 	if (IS_ERR(skb))
-		return  PTR_ERR(skb);
+		return PTR_ERR(skb);
 
 	hdr = (struct sadb_msg *) skb->data;
 	hdr->sadb_msg_version = PF_KEY_V2;
@@ -1948,7 +1948,7 @@
 	hdr->sadb_msg_reserved = atomic_read(&xp->refcnt);
 }
 
-static int key_notify_policy( struct xfrm_policy *xp, int dir, struct km_event *c)
+static int key_notify_policy(struct xfrm_policy *xp, int dir, struct km_event *c)
 {
 	struct sk_buff *out_skb;
 	struct sadb_msg *out_hdr;
@@ -1956,13 +1956,13 @@
 
 	out_skb = pfkey_xfrm_policy2msg_prep(xp);
 	if (IS_ERR(out_skb)) {
-		err =  PTR_ERR(out_skb);
+		err = PTR_ERR(out_skb);
 		goto out;
 	}
 	pfkey_xfrm_policy2msg(out_skb, xp, dir);
 
 	out_hdr = (struct sadb_msg *) out_skb->data;
-	out_hdr->sadb_msg_version =  PF_KEY_V2;
+	out_hdr->sadb_msg_version = PF_KEY_V2;
 
 	if (c->data && c->event == XFRM_SAP_DELETED)
 		out_hdr->sadb_msg_type = SADB_X_SPDDELETE2;
@@ -2052,18 +2052,16 @@
 	    (err = parse_ipsecrequests(xp, pol)) < 0)
 		goto out;
 
-
 	err = xfrm_policy_insert(pol->sadb_x_policy_dir-1, xp,
 				 hdr->sadb_msg_type != SADB_X_SPDUPDATE);
-
 	if (err) {
 		kfree(xp);
 		return err;
 	}
 
-	if (hdr->sadb_msg_type == SADB_X_SPDUPDATE) 
+	if (hdr->sadb_msg_type == SADB_X_SPDUPDATE)
 		c.event = XFRM_SAP_UPDATED;
-	else 
+	else
 		c.event = XFRM_SAP_ADDED;
 
 	c.seq = hdr->sadb_msg_seq;
@@ -2129,7 +2127,6 @@
 	return err;
 }
 
-
 static int key_pol_get_resp(struct sock *sk, struct xfrm_policy *xp, struct sadb_msg *hdr, int dir)
 {
 	int err;
@@ -2223,6 +2220,7 @@
 {
 	struct sk_buff *skb_out;
 	struct sadb_msg *hdr;
+
 	skb_out = alloc_skb(sizeof(struct sadb_msg) + 16, GFP_ATOMIC);
 	if (!skb_out)
 		return -ENOBUFS;
@@ -2445,7 +2443,6 @@
 	}
 }
 
-/* XXX: Noisy for now */
 static int key_notify_policy_expire(struct xfrm_policy *xp, struct km_event *c)
 {
 	return 0;
@@ -2493,7 +2490,7 @@
 	case XFRM_SAP_FLUSHED:
 		return key_notify_sa_flush(c);
 	default:
-		printk("pfkey: Unknown SA event %d\n",c->event);
+		printk("pfkey: Unknown SA event %d\n", c->event);
 		break;
 	}
 
@@ -2512,12 +2509,13 @@
 	case XFRM_SAP_FLUSHED:
 		return key_notify_policy_flush(c);
 	default:
-		printk("pfkey: Unknown policy event %d\n",c->event);
+		printk("pfkey: Unknown policy event %d\n", c->event);
 		break;
 	}
 
 	return 0;
 }
+
 static u32 get_acqseq(void)
 {
 	u32 res;
diff -urN old/net/xfrm/xfrm_state.c new/net/xfrm/xfrm_state.c
--- old/net/xfrm/xfrm_state.c	2005-05-07 15:54:50.000000000 +1000
+++ new/net/xfrm/xfrm_state.c	2005-05-07 15:44:40.000000000 +1000
@@ -440,7 +440,6 @@
 
 static struct xfrm_state *__xfrm_find_acq_byseq(u32 seq);
 
-
 int xfrm_state_add(struct xfrm_state *x)
 {
 	struct xfrm_state_afinfo *afinfo;
diff -urN old/net/xfrm/xfrm_user.c new/net/xfrm/xfrm_user.c
--- old/net/xfrm/xfrm_user.c	2005-05-07 15:54:50.000000000 +1000
+++ new/net/xfrm/xfrm_user.c	2005-05-07 15:53:09.000000000 +1000
@@ -359,7 +359,6 @@
 	int this_idx;
 };
 
-
 static int dump_one_state(struct xfrm_state *x, int count, void *ptr)
 {
 	struct xfrm_dump_info *sp = ptr;
@@ -709,7 +708,7 @@
 	if (!xp)
 		return err;
 
-	/* shouldnt excl be based on nlh flags?? 
+	/* shouldnt excl be based on nlh flags??
 	 * Aha! this is anti-netlink really i.e  more pfkey derived
 	 * in netlink excl is a flag and you wouldnt need
 	 * a type XFRM_MSG_UPDPOLICY - JHS */
@@ -720,7 +719,6 @@
 		return err;
 	}
 
-
 	if (!excl)
 		c.event = XFRM_SAP_UPDATED;
 	else
@@ -850,7 +848,6 @@
 	struct km_event c;
 	int delete;
 
-
 	p = NLMSG_DATA(nlh);
 	delete = nlh->nlmsg_type == XFRM_MSG_DELPOLICY;
 
@@ -1124,6 +1121,7 @@
 {
 	struct sk_buff *skb;
 	int hard = c ->data;
+
 	/* fix to do alloc using NLM macros */
 	skb = alloc_skb(sizeof(struct xfrm_user_expire) + 16, GFP_ATOMIC);
 	if (skb == NULL)
@@ -1150,7 +1148,7 @@
 		return -ENOMEM;
 	b = skb->tail;
 
-	nlh = NLMSG_PUT(skb, c->pid,  c->seq,
+	nlh = NLMSG_PUT(skb, c->pid, c->seq,
 			XFRM_MSG_FLUSHSA, sizeof(*p));
 	nlh->nlmsg_flags = 0;
 
@@ -1170,13 +1168,13 @@
 {
 	int l = NLMSG_LENGTH(sizeof(struct xfrm_usersa_info));
 	if (x->aalg)
-		l+= RTA_SPACE(sizeof(*(x->aalg))+(x->aalg->alg_key_len+7)/8);
+		l += RTA_SPACE(sizeof(*x->aalg) + (x->aalg->alg_key_len+7)/8);
 	if (x->ealg)
-		l+= RTA_SPACE(sizeof(*(x->ealg))+(x->ealg->alg_key_len+7)/8);
+		l += RTA_SPACE(sizeof(*x->ealg) + (x->ealg->alg_key_len+7)/8);
 	if (x->calg)
-		l+= RTA_SPACE(sizeof(*(x->calg)));
+		l += RTA_SPACE(sizeof(*x->calg));
 	if (x->encap)
-		l+= RTA_SPACE(sizeof(*x->encap));
+		l += RTA_SPACE(sizeof(*x->encap));
 
 	return l;
 }
@@ -1215,7 +1213,7 @@
 			sizeof(*(x->aalg))+(x->aalg->alg_key_len+7)/8, x->aalg);
 	if (x->ealg)
 		RTA_PUT(skb, XFRMA_ALG_CRYPT,
-			sizeof(*(x->ealg))+(x->ealg->alg_key_len+7)/8, x->ealg);        
+			sizeof(*(x->ealg))+(x->ealg->alg_key_len+7)/8, x->ealg);
 	if (x->calg)
 		RTA_PUT(skb, XFRMA_ALG_COMP, sizeof(*(x->calg)), x->calg);
 
@@ -1245,7 +1243,7 @@
 	case XFRM_SAP_FLUSHED:
 		return xfrm_notify_sa_flush(c);
 	default:
-		 printk("xfrm_user: Unknown SA event %d\n",c->event);
+		 printk("xfrm_user: Unknown SA event %d\n", c->event);
 		 break;
 	}
 
@@ -1386,7 +1384,6 @@
 	return -1;
 }
 
-
 static int xfrm_exp_policy_notify(struct xfrm_policy *xp, int dir, struct km_event *c)
 {
 	struct sk_buff *skb;
@@ -1406,7 +1403,7 @@
 	return netlink_broadcast(xfrm_nl, skb, 0, XFRMGRP_EXPIRE, GFP_ATOMIC);
 }
 
-static int xfrm_notify_policy( struct xfrm_policy *xp, int dir, struct km_event *c)
+static int xfrm_notify_policy(struct xfrm_policy *xp, int dir, struct km_event *c)
 {
 	struct xfrm_userpolicy_info *p;
 	struct nlmsghdr *nlh;
@@ -1486,7 +1483,7 @@
 	case XFRM_SAP_EXPIRED:
 		return xfrm_exp_policy_notify(xp, dir, c);
 	default:
-		printk("xfrm_user: Unknown Policy event %d\n",c->event);
+		printk("xfrm_user: Unknown Policy event %d\n", c->event);
 	}
 
 	return 0;

[2/7] [IPSEC] Fix xfrm to pfkey SA state conversion

[Herbert Xu]

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

Hi:

This patch adjusts the SA state conversion in af_key such that
XFRM_STATE_ERROR/XFRM_STATE_DEAD will be converted to SADB_STATE_DEAD
instead of SADB_STATE_DYING.

According to RFC 2367, SADB_STATE_DYING SAs can be turned into
mature ones through updating their lifetime settings.  Since SAs
which are in the states XFRM_STATE_ERROR/XFRM_STATE_DEAD cannot
be resurrected, this value is unsuitable.

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

[-- Attachment #2: xfrm-event-2 --]
[-- Type: text/plain, Size: 855 bytes --]

--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -656,13 +656,18 @@ static struct sk_buff * pfkey_xfrm_state
 	sa->sadb_sa_exttype = SADB_EXT_SA;
 	sa->sadb_sa_spi = x->id.spi;
 	sa->sadb_sa_replay = x->props.replay_window;
-	sa->sadb_sa_state = SADB_SASTATE_DYING;
-	if (x->km.state == XFRM_STATE_VALID && !x->km.dying)
-		sa->sadb_sa_state = SADB_SASTATE_MATURE;
-	else if (x->km.state == XFRM_STATE_ACQ)
+	switch (x->km.state) {
+	case XFRM_STATE_VALID:
+		sa->sadb_sa_state = x->km.dying ?
+			SADB_SASTATE_DYING : SADB_SASTATE_MATURE;
+		break;
+	case XFRM_STATE_ACQ:
 		sa->sadb_sa_state = SADB_SASTATE_LARVAL;
-	else if (x->km.state == XFRM_STATE_EXPIRED)
+		break;
+	default:
 		sa->sadb_sa_state = SADB_SASTATE_DEAD;
+		break;
+	}
 	sa->sadb_sa_auth = 0;
 	if (x->aalg) {
 		struct xfrm_algo_desc *a = xfrm_aalg_get_byname(x->aalg->alg_name, 0);

[3/7] [IPSEC] Kill spurious hard expire messages

[Herbert Xu]

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

Hi:

This patch ensures that the hard state/policy expire notifications are
only sent when the state/policy is successfully removed from their
respective tables.

As it is, it's possible for a state/policy to both expire through
reaching a hard limit, as well as being deleted by the user.

Note that this behaviour isn't actually forbidden by RFC 2367.
However, it is a quality of implementation issue.

As an added bonus, the restructuring in this patch will help
eventually in moving the expire notifications from softirq
context into process context, thus improving their reliability.

One important side-effect from this change is that SAs reaching
their hard byte/packet limits are now deleted immediately, just
like SAs that have reached their hard time limits.

Previously they were announced immediately but only deleted after
30 seconds.

This is bad because it prevents the system from issuing an ACQUIRE
command until the existing state was deleted by the user or expires
after the time is up.

In the scenario where the expire notification was lost this introduces
a 30 second delay into the system for no good reason.
 
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

[-- Attachment #2: xfrm-event-3 --]
[-- Type: text/plain, Size: 2489 bytes --]

--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -677,7 +677,7 @@ static inline int xfrm_sk_clone_policy(s
 	return 0;
 }
 
-extern void xfrm_policy_delete(struct xfrm_policy *pol, int dir);
+extern int xfrm_policy_delete(struct xfrm_policy *pol, int dir);
 
 static inline void xfrm_sk_free_policy(struct sock *sk)
 {
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -216,8 +216,8 @@ out:
 
 expired:
 	read_unlock(&xp->lock);
-	km_policy_expired(xp, dir, 1);
-	xfrm_policy_delete(xp, dir);
+	if (!xfrm_policy_delete(xp, dir))
+		km_policy_expired(xp, dir, 1);
 	xfrm_pol_put(xp);
 }
 
@@ -555,7 +555,7 @@ static struct xfrm_policy *__xfrm_policy
 	return NULL;
 }
 
-void xfrm_policy_delete(struct xfrm_policy *pol, int dir)
+int xfrm_policy_delete(struct xfrm_policy *pol, int dir)
 {
 	write_lock_bh(&xfrm_policy_lock);
 	pol = __xfrm_policy_unlink(pol, dir);
@@ -564,7 +564,9 @@ void xfrm_policy_delete(struct xfrm_poli
 		if (dir < XFRM_POLICY_MAX)
 			atomic_inc(&flow_cache_genid);
 		xfrm_policy_kill(pol);
+		return 0;
 	}
+	return -ENOENT;
 }
 
 int xfrm_sk_policy_insert(struct sock *sk, int dir, struct xfrm_policy *pol)
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -154,6 +154,7 @@ static void xfrm_timer_handler(unsigned 
 			next = tmo;
 	}
 
+	x->km.dying = warn;
 	if (warn)
 		km_state_expired(x, 0);
 resched:
@@ -169,9 +170,8 @@ expired:
 		next = 2;
 		goto resched;
 	}
-	if (x->id.spi != 0)
+	if (!__xfrm_state_delete(x) && x->id.spi)
 		km_state_expired(x, 1);
-	__xfrm_state_delete(x);
 
 out:
 	spin_unlock(&x->lock);
@@ -566,16 +566,18 @@ int xfrm_state_check_expire(struct xfrm_
 
 	if (x->curlft.bytes >= x->lft.hard_byte_limit ||
 	    x->curlft.packets >= x->lft.hard_packet_limit) {
-		km_state_expired(x, 1);
-		if (!mod_timer(&x->timer, jiffies + XFRM_ACQ_EXPIRES*HZ))
+		x->km.state = XFRM_STATE_EXPIRED;
+		if (!mod_timer(&x->timer, jiffies))
 			xfrm_state_hold(x);
 		return -EINVAL;
 	}
 
 	if (!x->km.dying &&
 	    (x->curlft.bytes >= x->lft.soft_byte_limit ||
-	     x->curlft.packets >= x->lft.soft_packet_limit))
+	     x->curlft.packets >= x->lft.soft_packet_limit)) {
+		x->km.dying = 1;
 		km_state_expired(x, 0);
+	}
 	return 0;
 }
 EXPORT_SYMBOL(xfrm_state_check_expire);
@@ -833,10 +835,6 @@ static void km_state_expired(struct xfrm
 {
 	struct km_event c;
 
-	if (hard)
-		x->km.state = XFRM_STATE_EXPIRED;
-	else
-		x->km.dying = 1;
 	c.data = hard;
 	c.event = XFRM_SAP_EXPIRED;
 	km_state_notify(x, &c);

[4/7] [IPSEC] Turn km_event.data into a union

[Herbert Xu]

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

Hi:

This patch turns km_event.data into a union.  This makes code that
uses it clearer.
  
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

[-- Attachment #2: xfrm-event-4 --]
[-- Type: text/plain, Size: 3761 bytes --]

--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -173,7 +173,12 @@ enum {
 /* callback structure passed from either netlink or pfkey */
 struct km_event
 {
-	u32	data;
+	union {
+		u32 hard;
+		u32 proto;
+		u32 byid;
+	} data;
+
 	u32	seq;
 	u32	pid;
 	u32	event;
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -1293,13 +1293,6 @@ static int key_notify_sa(struct xfrm_sta
 	if (c->event == XFRM_SAP_DELETED)
 		hsc = 0;
 
-	if (c->event == XFRM_SAP_EXPIRED) {
-		if (c->data)
-			hsc = 2;
-		else
-			hsc = 1;
-	}
-
 	skb = pfkey_xfrm_state2msg(x, 0, hsc);
 
 	if (IS_ERR(skb))
@@ -1534,7 +1527,7 @@ static int key_notify_sa_flush(struct km
 	if (!skb)
 		return -ENOBUFS;
 	hdr = (struct sadb_msg *) skb_put(skb, sizeof(struct sadb_msg));
-	hdr->sadb_msg_satype = pfkey_proto2satype(c->data);
+	hdr->sadb_msg_satype = pfkey_proto2satype(c->data.proto);
 	hdr->sadb_msg_seq = c->seq;
 	hdr->sadb_msg_pid = c->pid;
 	hdr->sadb_msg_version = PF_KEY_V2;
@@ -1556,7 +1549,7 @@ static int pfkey_flush(struct sock *sk, 
 		return -EINVAL;
 
 	xfrm_state_flush(proto);
-	c.data = proto;
+	c.data.proto = proto;
 	c.seq = hdr->sadb_msg_seq;
 	c.pid = hdr->sadb_msg_pid;
 	c.event = XFRM_SAP_FLUSHED;
@@ -1969,7 +1962,7 @@ static int key_notify_policy(struct xfrm
 	out_hdr = (struct sadb_msg *) out_skb->data;
 	out_hdr->sadb_msg_version = PF_KEY_V2;
 
-	if (c->data && c->event == XFRM_SAP_DELETED)
+	if (c->data.byid && c->event == XFRM_SAP_DELETED)
 		out_hdr->sadb_msg_type = SADB_X_SPDDELETE2;
 	else
 		out_hdr->sadb_msg_type = event2poltype(c->event);
@@ -2180,7 +2173,7 @@ static int pfkey_spdget(struct sock *sk,
 	c.seq = hdr->sadb_msg_seq;
 	c.pid = hdr->sadb_msg_pid;
 	if (hdr->sadb_msg_type == SADB_X_SPDDELETE2) {
-		c.data = 1; // to signal pfkey of SADB_X_SPDDELETE2
+		c.data.byid = 1;
 		c.event = XFRM_SAP_DELETED;
 		km_policy_notify(xp, pol->sadb_x_policy_dir-1, &c);
 	} else {
@@ -2460,7 +2453,7 @@ static int key_notify_sa_expire(struct x
 	int hard;
 	int hsc;
 
-	hard = c->data;
+	hard = c->data.hard;
 	if (hard)
 		hsc = 2;
 	else
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -835,7 +835,7 @@ static void km_state_expired(struct xfrm
 {
 	struct km_event c;
 
-	c.data = hard;
+	c.data.hard = hard;
 	c.event = XFRM_SAP_EXPIRED;
 	km_state_notify(x, &c);
 
@@ -883,8 +883,7 @@ void km_policy_expired(struct xfrm_polic
 {
 	struct km_event c;
 
-	c.data = hard;
-	c.data = hard;
+	c.data.hard = hard;
 	c.event = XFRM_SAP_EXPIRED;
 	km_policy_notify(pol, dir, &c);
 
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -891,7 +891,7 @@ static int xfrm_flush_sa(struct sk_buff 
 	struct xfrm_usersa_flush *p = NLMSG_DATA(nlh);
 
 	xfrm_state_flush(p->proto);
-	c.data = p->proto;
+	c.data.proto = p->proto;
 	c.event = XFRM_SAP_FLUSHED;
 	c.seq = nlh->nlmsg_seq;
 	c.pid = nlh->nlmsg_pid;
@@ -1120,14 +1120,13 @@ nlmsg_failure:
 static int xfrm_exp_state_notify(struct xfrm_state *x, struct km_event *c)
 {
 	struct sk_buff *skb;
-	int hard = c ->data;
 
 	/* fix to do alloc using NLM macros */
 	skb = alloc_skb(sizeof(struct xfrm_user_expire) + 16, GFP_ATOMIC);
 	if (skb == NULL)
 		return -ENOMEM;
 
-	if (build_expire(skb, x, hard) < 0)
+	if (build_expire(skb, x, c->data.hard) < 0)
 		BUG();
 
 	NETLINK_CB(skb).dst_groups = XFRMGRP_EXPIRE;
@@ -1153,7 +1152,7 @@ static int xfrm_notify_sa_flush(struct k
 	nlh->nlmsg_flags = 0;
 
 	p = NLMSG_DATA(nlh);
-	p->proto = c->data;
+	p->proto = c->data.proto;
 
 	nlh->nlmsg_len = skb->tail - b;
 
@@ -1395,7 +1394,7 @@ static int xfrm_exp_policy_notify(struct
 	if (skb == NULL)
 		return -ENOMEM;
 
-	if (build_polexpire(skb, xp, dir, c->data) < 0)
+	if (build_polexpire(skb, xp, dir, c->data.hard) < 0)
 		BUG();
 
 	NETLINK_CB(skb).dst_groups = XFRMGRP_EXPIRE;

Re: [7/7] [IPSEC] Add XFRMA_SA/XFRMA_POLICY for delete notification

[jamal]

On Sat, 2005-07-05 at 17:23 +1000, Herbert Xu wrote:
> Hi:
> 
> This patch changes the format of the XFRM_MSG_DELSA and
> XFRM_MSG_DELPOLICY notification so that the main message
> sent is of the same format as that received by the kernel
> if the original message was via netlink.  This also means
> that we won't lose the byid information carried in km_event.
> 

This is incosistent in two ways:
1) Typical netlink behavior is to return the object being deleted.
Every other netlink user behaves that way - the only exception is sone
filters in tc and this is because they cant retrieve that detail
(something that needs resolution at some point). There is nothing that
xfrm_usersa_id provides that can be found in xfrm_usersa_info.
Same for the policy.

2) You cant have one behavior when something is deleted by pfkey and a
different one when it is deleted by netlink.

So i would recommend pulling this one out.

cheers,
jamal

Re: [7/7] [IPSEC] Add XFRMA_SA/XFRMA_POLICY for delete notification

[Herbert Xu]

On Sat, May 07, 2005 at 08:04:16AM -0400, jamal wrote:
> 
> This is incosistent in two ways:
> 1) Typical netlink behavior is to return the object being deleted.
> Every other netlink user behaves that way - the only exception is sone
> filters in tc and this is because they cant retrieve that detail
> (something that needs resolution at some point). There is nothing that
> xfrm_usersa_id provides that can be found in xfrm_usersa_info.
> Same for the policy.

This analogy is flawed since unlike other rtnetlink delete operations
the xfrm delete operations do not carry the same payload type as their
add/update cousins.

> 2) You cant have one behavior when something is deleted by pfkey and a
> different one when it is deleted by netlink.

As far as I can see the behaviour is identical.

> So i would recommend pulling this one out.

Well I don't really have time to get into another debate so I'll
chuck this one out.
-- 
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: [7/7] [IPSEC] Add XFRMA_SA/XFRMA_POLICY for delete notification

[jamal]

On Sat, 2005-07-05 at 22:25 +1000, Herbert Xu wrote:
> On Sat, May 07, 2005 at 08:04:16AM -0400, jamal wrote:
> > 
> > This is incosistent in two ways:
> > 1) Typical netlink behavior is to return the object being deleted.
> > Every other netlink user behaves that way - the only exception is sone
> > filters in tc and this is because they cant retrieve that detail
> > (something that needs resolution at some point). There is nothing that
> > xfrm_usersa_id provides that can be found in xfrm_usersa_info.
> > Same for the policy.
> 
> This analogy is flawed since unlike other rtnetlink delete operations
> the xfrm delete operations do not carry the same payload type as their
> add/update cousins.
> 

No, this is not true. Study the tc code.
It is nice to be able to return exactly the same detail - user space can
then infer what exactly happened. It is nicer to be able to return more
detail because user space doesnt have to infer anything.

> > 2) You cant have one behavior when something is deleted by pfkey and a
> > different one when it is deleted by netlink.
> 
> As far as I can see the behaviour is identical.
> 

If this is true, then #1 is forgivable.  This was my main concern.

You describe the patch this way

---
This patch changes the format of the XFRM_MSG_DELSA and
XFRM_MSG_DELPOLICY notification so that the main message
sent is of the same format as that received by the kernel
if the original message was via netlink.
----

That it only happens when you delete via netlink. Is this not so?

cheers,
jamal

Re: [1/7] [IPSEC] Add complete xfrm event notification

[Patrick McHardy]

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

Herbert Xu wrote:
> @@ -1254,6 +1326,7 @@ static int pfkey_add(struct sock *sk, st
>  	if (IS_ERR(x))
>  		return PTR_ERR(x);
>  
> +	xfrm_state_hold(x);

This introduces a leak when xfrm_state_add()/xfrm_state_update()
fail. We hold two references (one from xfrm_state_alloc(), one
from xfrm_state_hold()), but only drop one. We need to take the
reference because the reference from xfrm_state_alloc() can
be dropped by __xfrm_state_delete(), so the fix is to drop both
references on error. Same problem in xfrm_user.c.


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

[XFRM]: Fix xfrm_state leaks in error path

Signed-off-by: Patrick McHardy <kaber@trash.net>

---
commit a4222e4b4f4fe6a28204e7960972ef833ac0c4ce
tree c24f26cfe03081d10a3a3f66d5d3e503395090b4
parent 16efae13731912e8cd028a85257fb33726318770
author Patrick McHardy <kaber@trash.net> 1115477180 +0200
committer Patrick McHardy <kaber@trash.net> 1115477180 +0200

Index: net/key/af_key.c
===================================================================
--- 6c0df7e8f613031668cf54aec5735e8b9f76aaa9/net/key/af_key.c  (mode:100644 sha1:577f0bb5bb31816bb1ecf94848ae2758d9c2cbcf)
+++ c24f26cfe03081d10a3a3f66d5d3e503395090b4/net/key/af_key.c  (mode:100644 sha1:98b72f2024ffd84564530e5973861b908fd8f541)
@@ -1333,7 +1333,7 @@
 	if (err < 0) {
 		x->km.state = XFRM_STATE_DEAD;
 		xfrm_state_put(x);
-		return err;
+		goto out;
 	}
 
 	if (hdr->sadb_msg_type == SADB_ADD)
@@ -1343,8 +1343,8 @@
 	c.seq = hdr->sadb_msg_seq;
 	c.pid = hdr->sadb_msg_pid;
 	km_state_notify(x, &c);
+out:
 	xfrm_state_put(x);
-
 	return err;
 }
 
Index: net/xfrm/xfrm_user.c
===================================================================
--- 6c0df7e8f613031668cf54aec5735e8b9f76aaa9/net/xfrm/xfrm_user.c  (mode:100644 sha1:6c8c6d6924939fe30264caab9f6fca943cf70e3b)
+++ c24f26cfe03081d10a3a3f66d5d3e503395090b4/net/xfrm/xfrm_user.c  (mode:100644 sha1:4f37b4f2ea8a238b8ae5f97496b727df7489d5fb)
@@ -287,7 +287,7 @@
 	if (err < 0) {
 		x->km.state = XFRM_STATE_DEAD;
 		xfrm_state_put(x);
-		return err;
+		goto out;
 	}
 
 	c.seq = nlh->nlmsg_seq;
@@ -295,8 +295,8 @@
 	c.event = nlh->nlmsg_type;
 
 	km_state_notify(x, &c);
+out:
 	xfrm_state_put(x);
-
 	return err;
 }
 

Re: [7/7] [IPSEC] Add XFRMA_SA/XFRMA_POLICY for delete notification

[Herbert Xu]

On Sat, May 07, 2005 at 08:46:44AM -0400, jamal wrote:
> 
> No, this is not true. Study the tc code.
> It is nice to be able to return exactly the same detail - user space can
> then infer what exactly happened. It is nicer to be able to return more
> detail because user space doesnt have to infer anything.

This patch is making it return more detail, not less! The full
description of the deleted policy/state is still being returned,
albeit as RTA payloads.

Prior to the change, netlink users do not know whether the original
policy delete request was by selector or by id.  Now that information
is also returned.

> You describe the patch this way
> 
> ---
> This patch changes the format of the XFRM_MSG_DELSA and
> XFRM_MSG_DELPOLICY notification so that the main message
> sent is of the same format as that received by the kernel
> if the original message was via netlink.
> ----
> 
> That it only happens when you delete via netlink. Is this not so?

The same change applies even if you sent the delete via pfkey.  What
the change does is to make netlink always send a delete message that
is valid in the sense that if you sent it back to netlink then it
would delete that policy/state.

As it is the netlink delete messages sent by notification are invalid
by its own standard.

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: [1/7] [IPSEC] Add complete xfrm event notification

[Herbert Xu]

On Sat, May 07, 2005 at 04:51:33PM +0200, Patrick McHardy wrote:
> 
> This introduces a leak when xfrm_state_add()/xfrm_state_update()
> fail. We hold two references (one from xfrm_state_alloc(), one
> from xfrm_state_hold()), but only drop one. We need to take the
> reference because the reference from xfrm_state_alloc() can
> be dropped by __xfrm_state_delete(), so the fix is to drop both
> references on error. Same problem in xfrm_user.c.

Patch applied.  Thanks for plugging the leaks :)
-- 
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: [7/7] [IPSEC] Add XFRMA_SA/XFRMA_POLICY for delete notification

[jamal]

On Sun, 2005-08-05 at 05:35 +1000, Herbert Xu wrote:
> On Sat, May 07, 2005 at 08:46:44AM -0400, jamal wrote:
> > 
> > No, this is not true. Study the tc code.
> > It is nice to be able to return exactly the same detail - user space can
> > then infer what exactly happened. It is nicer to be able to return more
> > detail because user space doesnt have to infer anything.
> 
> This patch is making it return more detail, not less! The full
> description of the deleted policy/state is still being returned,
> albeit as RTA payloads.

It's the other extreme of what i thought.
The only thing user space needs to know is what object was deleted.
If you just passed the id, it could be deduced if user space maintained
state; if you pass the object, no such deduction is needed.

> Prior to the change, netlink users do not know whether the original
> policy delete request was by selector or by id.  Now that information
> is also returned.
> 

Why would someone need to deduce whether it has been deleted by index or
selector?
If you gave me the object - i can reproduce the action of deleting it.
Thats the _only_ important_ thing.

> > You describe the patch this way
> > 
> > ---
> > This patch changes the format of the XFRM_MSG_DELSA and
> > XFRM_MSG_DELPOLICY notification so that the main message
> > sent is of the same format as that received by the kernel
> > if the original message was via netlink.
> > ----
> > 
> > That it only happens when you delete via netlink. Is this not so?
> 
> The same change applies even if you sent the delete via pfkey.  What
> the change does is to make netlink always send a delete message that
> is valid in the sense that if you sent it back to netlink then it
> would delete that policy/state.
> 

Does pfkey have ability to delete by index and selector?
If yes, how do you distinguish the two cases when you are sending the
netlink event?

> As it is the netlink delete messages sent by notification are invalid
> by its own standard.
> 

It doesnt seem to me what you provided in the patch produces exactly the
same thing that was sent by user space back in the event.
You introduce some other new TLVs to reflect it back.
But even that is besides the point:
None of those xfrm objects obey any of the standard rules 
netlink uses to begin with - you have more than one way of deleting an
object. What you need to do is the detail of what object was deleted.

Heres what i will say so we can put this to rest:
The patch is unneeded (i hate to use strong words like bogus - but it is
getting close to that), but if you feel strongly about it just lets have
it well documented and provide the iproute2 patch as well.

cheers,
jamal

Re: [7/7] [IPSEC] Add XFRMA_SA/XFRMA_POLICY for delete notification

[Herbert Xu]

On Sun, May 08, 2005 at 09:56:33AM -0400, jamal wrote:
> 
> Why would someone need to deduce whether it has been deleted by index or
> selector?

It isn't just about deducing the message.  It's about sending a delete
message in the same format as we would receive them.  As it is the
delete message sent would be not be accepted if you sent it back to the

> Does pfkey have ability to delete by index and selector?

Yes.

> If yes, how do you distinguish the two cases when you are sending the
> netlink event?

Using the byid attribute that *you* introduced :)

> > As it is the netlink delete messages sent by notification are invalid
> > by its own standard.
> 
> It doesnt seem to me what you provided in the patch produces exactly the
> same thing that was sent by user space back in the event.

That's not the point.  The point is if you send exactly the same
message to the kernel, even with the RTAs attached, the kernel
would accept it and perform the deletion if there is a matching
policy.
 
> Heres what i will say so we can put this to rest:
> The patch is unneeded (i hate to use strong words like bogus - but it is
> getting close to that), but if you feel strongly about it just lets have
> it well documented and provide the iproute2 patch as well.

I'll leave the decision up to Dave.

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: [7/7] [IPSEC] Add XFRMA_SA/XFRMA_POLICY for delete notification

[jamal]

On Mon, 2005-09-05 at 07:40 +1000, Herbert Xu wrote:
> On Sun, May 08, 2005 at 09:56:33AM -0400, jamal wrote:
> > 
> > Why would someone need to deduce whether it has been deleted by index or
> > selector?
> 
> It isn't just about deducing the message.  It's about sending a delete
> message in the same format as we would receive them.  As it is the
> delete message sent would be not be accepted if you sent it back to the
> 

If you enumerate all netlink messages, you will see this is not always
the case. It is a nice but not a necessary condition; infact, not even
what you generate with that patch is _the same_ message that was sent
(you add new TLVs in the response that didnt exist in user->kernel).

What is necessary is that if i look at the event i know exactly what was
deleted. If i have such detail, i can build the message that was sent
from user->kernel to delete the object (because i know exactly what was
deleted). 
As an example:
I can derive the xfrm_usersa_id that was sent to the kernel if the event
sent me xfrm_usersa_info and therefore build the same a message that
will delete _exactly_ the same object. 

It does get worse on occasion (I can point at tc filters) - where you
really cant derive the deleted object.


> > If yes, how do you distinguish the two cases when you are sending the
> > netlink event?
> 
> Using the byid attribute that *you* introduced :)
> 

Ok, theres no inconsistency then.

> > It doesnt seem to me what you provided in the patch produces exactly the
> > same thing that was sent by user space back in the event.
> 
> That's not the point.  The point is if you send exactly the same
> message to the kernel, even with the RTAs attached, the kernel
> would accept it and perform the deletion if there is a matching
> policy.

So you are depending on the kernel not checking for the extra TLVs you
send?;->
Refer to what i said above.

>  
> > Heres what i will say so we can put this to rest:
> > The patch is unneeded (i hate to use strong words like bogus - but it is
> > getting close to that), but if you feel strongly about it just lets have
> > it well documented and provide the iproute2 patch as well.
> 
> I'll leave the decision up to Dave.

Like i said: I think its extraneous stuff that is unneeded(what is in
there at the moment is sufficient detail) - but because theres no
inconsistency, i will not squirm in pain if it is included. I am
agreeing to disagree essentially ;->

cheers,
jamal