RFC: IPSEC patch 0 for netlink events

RFC: IPSEC patch 0 for netlink events

[jamal]

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


Folks,

first patch that changes internal api slightly to allow for next
patch which will add other events. It includes the other acquire patch i
posted earlier - please ignore that - Or if you like this patch use it
instead.
This patch also adds a few new groups.

I am dependent on this before going to next steps so please comment
if you can - the patch is very small

cheers,
jamal

[-- Attachment #2: ipsec_p0 --]
[-- Type: text/plain, Size: 3276 bytes --]

--- a/include/linux/xfrm.h	2005/03/26 18:30:26	1.1
+++ b/include/linux/xfrm.h	2005/03/26 18:47:48
@@ -254,5 +254,8 @@
 
 #define XFRMGRP_ACQUIRE		1
 #define XFRMGRP_EXPIRE		2
+#define XFRMGRP_POLEXPIRE	4
+#define XFRMGRP_SA		8
+#define XFRMGRP_POLICY		0x10
 
 #endif /* _LINUX_XFRM_H */
--- a/include/net/xfrm.h	2005/03/26 17:04:48	1.1
+++ b/include/net/xfrm.h	2005/03/26 18:28:47
@@ -157,6 +157,18 @@
 	XFRM_STATE_DEAD
 };
 
+/* events that could be sent by kernel */
+enum {
+	XFRM_SA_INVALID,
+	XFRM_SA_EXPIRED,
+	XFRM_SA_ADDED,
+	XFRM_SA_UPDATED,
+	XFRM_SA_DELETED,
+	XFRM_SA_FLUSHED,
+	__XFRM_SA_MAX
+};
+
+#define XFRM_SA_MAX (__XFRM_SA_MAX - 1)
 struct xfrm_type;
 struct xfrm_dst;
 struct xfrm_policy_afinfo {
@@ -289,7 +301,7 @@
 {
 	struct list_head	list;
 	char			*id;
-	int			(*notify)(struct xfrm_state *x, int event);
+	int			(*notify)(struct xfrm_state *x, int event, int cb_id);
 	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);
--- a/net/xfrm/xfrm_state.c	2005-03-19 01:35:00.000000000 -0500
+++ b/net/xfrm/xfrm_state.c	2005-03-26 13:22:34.000000000 -0500
@@ -778,23 +778,27 @@
 
 	read_lock(&xfrm_km_lock);
 	list_for_each_entry(km, &xfrm_km_list, list)
-		km->notify(x, hard);
+		km->notify(x, XFRM_SA_EXPIRED, hard);
 	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 = -EINVAL;
 	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;
--- a/net/xfrm/xfrm_user.c	2005/03/26 17:02:39	1.1
+++ b/net/xfrm/xfrm_user.c	2005/03/26 17:22:06
@@ -1053,7 +1053,7 @@
 	return -1;
 }
 
-static int xfrm_send_state_notify(struct xfrm_state *x, int hard)
+static int xfrm_send_state_notify(struct xfrm_state *x, int event, int cb_id)
 {
 	struct sk_buff *skb;
 
@@ -1061,7 +1061,10 @@
 	if (skb == NULL)
 		return -ENOMEM;
 
-	if (build_expire(skb, x, hard) < 0)
+	if (event != XFRM_SA_EXPIRED)
+		return 0;
+
+	if (build_expire(skb, x, cb_id) < 0)
 		BUG();
 
 	NETLINK_CB(skb).dst_groups = XFRMGRP_EXPIRE;
--- a/net/key/af_key.c	2005-03-19 01:35:06.000000000 -0500
+++ b/net/key/af_key.c	2005-03-26 12:20:19.000000000 -0500
@@ -2317,12 +2317,20 @@
 	}
 }
 
-static int pfkey_send_notify(struct xfrm_state *x, int hard)
+static int pfkey_send_notify(struct xfrm_state *x, int event, int cb_id)
 {
 	struct sk_buff *out_skb;
 	struct sadb_msg *out_hdr;
+	int hard = cb_id;
 	int hsc = (hard ? 2 : 1);
 
+	/* 
+	 * migrate pf_key later - for now only support is for
+	 * expire events 
+	*/
+	if (event != XFRM_SA_EXPIRED)
+		return 0; 
+
 	out_skb = pfkey_xfrm_state2msg(x, 0, hsc);
 	if (IS_ERR(out_skb))
 		return PTR_ERR(out_skb);

Re: RFC: IPSEC patch 0 for netlink events

[Herbert Xu]

On Sat, Mar 26, 2005 at 02:22:51PM -0500, jamal wrote:
>
> --- a/include/linux/xfrm.h	2005/03/26 18:30:26	1.1
> +++ b/include/linux/xfrm.h	2005/03/26 18:47:48
> @@ -254,5 +254,8 @@
>  
>  #define XFRMGRP_ACQUIRE		1
>  #define XFRMGRP_EXPIRE		2
> +#define XFRMGRP_POLEXPIRE	4

Is the intention that you'll be sending policy expire messages to
POLEXPIRE only?

If so please add a new group for SA expire messages only so that
XFRMGRP_EXPIRE continues to get all expire messages for compatibility.

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: RFC: IPSEC patch 0 for netlink events

[jamal]

On Sat, 2005-03-26 at 14:40, Herbert Xu wrote:
> On Sat, Mar 26, 2005 at 02:22:51PM -0500, jamal wrote:
> >
> > --- a/include/linux/xfrm.h	2005/03/26 18:30:26	1.1
> > +++ b/include/linux/xfrm.h	2005/03/26 18:47:48
> > @@ -254,5 +254,8 @@
> >  
> >  #define XFRMGRP_ACQUIRE		1
> >  #define XFRMGRP_EXPIRE		2
> > +#define XFRMGRP_POLEXPIRE	4
> 
> Is the intention that you'll be sending policy expire messages to
> POLEXPIRE only?

Right. Unless you think this will break existing apps?


> If so please add a new group for SA expire messages only so that
> XFRMGRP_EXPIRE continues to get all expire messages for compatibility.
> 

Ok, i should have read ahead ;->
Maybe i should leave EXPIRE alone?

cheers,
jamal

Re: RFC: IPSEC patch 0 for netlink events

[Herbert Xu]

On Sat, Mar 26, 2005 at 03:05:05PM -0500, jamal wrote:
>
> > Is the intention that you'll be sending policy expire messages to
> > POLEXPIRE only?
> 
> Right. Unless you think this will break existing apps?

Yes openswan listens on XFRMGRP_EXPIRE for poilcy expire messages
to detect unused pass policies that were automatically generated.

> Maybe i should leave EXPIRE alone?

I think so.

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: RFC: IPSEC patch 0 for netlink events

[Herbert Xu]

On Sat, Mar 26, 2005 at 02:22:51PM -0500, jamal wrote:
>
> --- a/include/linux/xfrm.h	2005/03/26 18:30:26	1.1
> +++ b/include/linux/xfrm.h	2005/03/26 18:47:48
> @@ -254,5 +254,8 @@
>  
>  #define XFRMGRP_ACQUIRE		1
>  #define XFRMGRP_EXPIRE		2
> +#define XFRMGRP_POLEXPIRE	4
> +#define XFRMGRP_SA		8
> +#define XFRMGRP_POLICY		0x10

Since we're adding new multicast groups, what about adding one for
the passive event monitor? That way we can return ESRCH in km_query
if there are no registered ACQUIRE listeners but still send messages
to the monitor.

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: RFC: IPSEC patch 0 for netlink events

[jamal]

On Sat, 2005-03-26 at 14:47, Herbert Xu wrote:

> Since we're adding new multicast groups, what about adding one for
> the passive event monitor? That way we can return ESRCH in km_query
> if there are no registered ACQUIRE listeners but still send messages
> to the monitor.
> 

Not sure how to do it for both PF_KEY and netlink. It does sound like a
reasonable thing to do. Thoughts?

cheers,
jamal

Re: RFC: IPSEC patch 0 for netlink events

[Herbert Xu]

On Sat, Mar 26, 2005 at 03:11:15PM -0500, jamal wrote:
> On Sat, 2005-03-26 at 14:47, Herbert Xu wrote:
> 
> > Since we're adding new multicast groups, what about adding one for
> > the passive event monitor? That way we can return ESRCH in km_query
> > if there are no registered ACQUIRE listeners but still send messages
> > to the monitor.
> 
> Not sure how to do it for both PF_KEY and netlink. It does sound like a
> reasonable thing to do. Thoughts?

For non-standard extensions like this I wouldn't worry about PF_KEY.
After all, if you're going to make sense of all the messages from
the kernel you'll have to use netlink anyway.

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: RFC: IPSEC patch 0 for netlink events

[jamal]

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

On Sun, 2005-03-27 at 03:18, Herbert Xu wrote:

> For non-standard extensions like this I wouldn't worry about PF_KEY.
> After all, if you're going to make sense of all the messages from
> the kernel you'll have to use netlink anyway.
> 

Just for consistency (since both call the same xfrm_state core code)
I made some minor changes to pf_key internal-to-kernel API (not exposed
to user space). Sample patch, still under construction, attached.
pfkey already does adverts on its own after a response from the generic
code. 
In the future this could be modified to do events about the same time
netlink does them i.e invocation from core xfrm_state code.
At the moment pfkey listeners are slightly delayed relative to netlink.

cheers,
jamal

[-- Attachment #2: ipsec_p1-2 --]
[-- Type: text/plain, Size: 8561 bytes --]

--- 26115/include/net/xfrm.h	2005-03-19 01:35:02.000000000 -0500
+++ 26115-mod/include/net/xfrm.h	2005-03-27 09:00:03.000000000 -0500
@@ -157,6 +157,36 @@
 	XFRM_STATE_DEAD
 };
 
+/* events that could be sent by kernel */
+enum {
+	XFRM_SA_INVALID,
+	XFRM_SA_EXPIRED,
+	XFRM_SA_ADDED,
+	XFRM_SA_UPDATED,
+	XFRM_SA_DELETED,
+	XFRM_SA_FLUSHED,
+	__XFRM_SA_MAX
+};
+#define XFRM_SA_MAX (__XFRM_SA_MAX - 1)
+
+/* callback structure passed from either netlink or pfkey */
+struct xfrm_sa_cb
+{
+	u32	type; /* the type of caller netlink/pfkey/other */
+	u32	data; /* callee to caller */
+	void *hdr;
+	struct sk_buff *skb;
+};
+
+/* the types used in sa_cb */
+enum {
+	KM_SA_INVALID,
+	KM_SA_NETLINK,
+	KM_SA_PFKEY,
+	__KM_SA_MAX
+};
+#define KM_SA_MAX (__KM_SA_MAX - 1)
+
 struct xfrm_type;
 struct xfrm_dst;
 struct xfrm_policy_afinfo {
@@ -289,7 +319,7 @@
 {
 	struct list_head	list;
 	char			*id;
-	int			(*notify)(struct xfrm_state *x, int event);
+	int			(*notify)(struct xfrm_state *x, int event, struct xfrm_sa_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);
@@ -798,8 +828,8 @@
 					  unsigned short family);
 extern int xfrm_state_check_expire(struct xfrm_state *x);
 extern void xfrm_state_insert(struct xfrm_state *x);
-extern int xfrm_state_add(struct xfrm_state *x);
-extern int xfrm_state_update(struct xfrm_state *x);
+extern int xfrm_state_add(struct xfrm_state *x, struct xfrm_sa_cb *c);
+extern int xfrm_state_update(struct xfrm_state *x, struct xfrm_sa_cb *c);
 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);
--- 26115/include/linux/xfrm.h	2005-03-19 01:35:06.000000000 -0500
+++ 26115-mod/include/linux/xfrm.h	2005-03-27 10:19:49.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 */
--- 26115/net/xfrm/xfrm_state.c	2005-03-19 01:35:00.000000000 -0500
+++ 26115-mod/net/xfrm/xfrm_state.c	2005-03-27 09:14:09.000000000 -0500
@@ -402,7 +402,19 @@
 
 static struct xfrm_state *__xfrm_find_acq_byseq(u32 seq);
 
-int xfrm_state_add(struct xfrm_state *x)
+static struct list_head xfrm_km_list = LIST_HEAD_INIT(xfrm_km_list);
+static DEFINE_RWLOCK(xfrm_km_lock);
+
+void xfrm_sa_notify(struct xfrm_state *x, struct xfrm_sa_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);
+}
+
+int xfrm_state_add(struct xfrm_state *x, struct xfrm_sa_cb *c)
 {
 	struct xfrm_state_afinfo *afinfo;
 	struct xfrm_state *x1;
@@ -438,6 +450,7 @@
 			&x->id.daddr, &x->props.saddr, 0);
 
 	__xfrm_state_insert(x);
+	xfrm_sa_notify(x, c, XFRM_SA_ADDED);
 	err = 0;
 
 out:
@@ -453,7 +466,7 @@
 }
 EXPORT_SYMBOL(xfrm_state_add);
 
-int xfrm_state_update(struct xfrm_state *x)
+int xfrm_state_update(struct xfrm_state *x, struct xfrm_sa_cb *c)
 {
 	struct xfrm_state_afinfo *afinfo;
 	struct xfrm_state *x1;
@@ -478,6 +491,9 @@
 
 	if (x1->km.state == XFRM_STATE_ACQ) {
 		__xfrm_state_insert(x);
+		/* XXXX: We already have xfrm_state_lock
+		 * do we need to grab x->lock as well? */
+		xfrm_sa_notify(x, c, XFRM_SA_ADDED);
 		x = NULL;
 	}
 	err = 0;
@@ -509,6 +525,7 @@
 			xfrm_state_check_expire(x1);
 
 		err = 0;
+		xfrm_sa_notify(x, c, XFRM_SA_UPDATED);
 	}
 	spin_unlock_bh(&x1->lock);
 
@@ -764,37 +781,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 xfrm_sa_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_SA_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;
--- 26115/net/xfrm/xfrm_user.c	2005-03-19 01:34:58.000000000 -0500
+++ 26115-mod/net/xfrm/xfrm_user.c	2005-03-27 09:06:49.000000000 -0500
@@ -267,6 +267,7 @@
 {
 	struct xfrm_usersa_info *p = NLMSG_DATA(nlh);
 	struct xfrm_state *x;
+	struct xfrm_sa_cb c;
 	int err;
 
 	err = verify_newsa_info(p, (struct rtattr **) xfrma);
@@ -277,10 +278,14 @@
 	if (!x)
 		return err;
 
+	c.type = KM_SA_NETLINK;
+	c.skb = skb;
+	c.hdr = nlh;
+
 	if (nlh->nlmsg_type == XFRM_MSG_NEWSA)
-		err = xfrm_state_add(x);
+		err = xfrm_state_add(x, &c);
 	else
-		err = xfrm_state_update(x);
+		err = xfrm_state_update(x, &c);
 
 	if (err < 0) {
 		x->km.state = XFRM_STATE_DEAD;
@@ -1053,10 +1058,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 +1074,63 @@
 	return netlink_broadcast(xfrm_nl, skb, 0, XFRMGRP_EXPIRE, GFP_ATOMIC);
 }
 
+static int notify_add_upd( struct xfrm_state *x, int event, struct xfrm_sa_cb *c)
+{
+	struct xfrm_usersa_info *p;
+	struct nlmsghdr *nlh;
+	struct sk_buff *skb;
+	u32 nlt;
+	unsigned char *b;
+	u32 ppid = 0;
+	struct nlmsghdr *in_nlh = c->hdr;
+	struct sk_buff *in_skb = c->skb;
+	int len = NLMSG_LENGTH(sizeof(struct xfrm_usersa_info));
+
+	skb = alloc_skb(len, GFP_ATOMIC);
+	if (skb == NULL)
+		return -ENOMEM;
+	b = skb->tail;
+
+	if (c->type == KM_SA_NETLINK)
+		ppid = NETLINK_CB(in_skb).pid;
+
+	if (event == XFRM_SA_ADDED)
+		nlt = XFRM_MSG_NEWSA;
+	else if (event == XFRM_SA_UPDATED)
+		nlt = XFRM_MSG_UPDSA;
+	else
+		goto nlmsg_failure;
+
+	nlh = NLMSG_PUT(skb, ppid,
+			in_nlh->nlmsg_seq,
+			nlt, sizeof(*p));
+	nlh->nlmsg_flags = in_nlh->nlmsg_flags;
+
+	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 xfrm_sa_cb *c)
+{
+
+	if ((event == XFRM_SA_ADDED) || (event == XFRM_SA_UPDATED))
+		return notify_add_upd(x, event, c);
+
+	if (event != XFRM_SA_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)
--- 26115/net/key/af_key.c	2005-03-19 01:35:06.000000000 -0500
+++ 26115-mod/net/key/af_key.c	2005-03-27 09:01:01.000000000 -0500
@@ -1246,6 +1246,7 @@
 	struct sk_buff *out_skb;
 	struct sadb_msg *out_hdr;
 	struct xfrm_state *x;
+	struct xfrm_sa_cb c;
 	int err;
 
 	xfrm_probe_algs();
@@ -1254,10 +1255,14 @@
 	if (IS_ERR(x))
 		return PTR_ERR(x);
 
+	c.type = KM_SA_PFKEY;
+	c.hdr =  hdr;
+	c.skb =  skb;
+
 	if (hdr->sadb_msg_type == SADB_ADD)
-		err = xfrm_state_add(x);
+		err = xfrm_state_add(x, &c);
 	else
-		err = xfrm_state_update(x);
+		err = xfrm_state_update(x, &c);
 
 	if (err < 0) {
 		x->km.state = XFRM_STATE_DEAD;
@@ -2317,12 +2322,20 @@
 	}
 }
 
-static int pfkey_send_notify(struct xfrm_state *x, int hard)
+static int pfkey_send_notify(struct xfrm_state *x, int event, struct xfrm_sa_cb *c)
 {
 	struct sk_buff *out_skb;
 	struct sadb_msg *out_hdr;
+	int hard = c->data;
 	int hsc = (hard ? 2 : 1);
 
+	/* 
+	 * migrate pf_key later - for now only support is for
+	 * expire events 
+	*/
+	if (event != XFRM_SA_EXPIRED)
+		return 0; 
+
 	out_skb = pfkey_xfrm_state2msg(x, 0, hsc);
 	if (IS_ERR(out_skb))
 		return PTR_ERR(out_skb);

Re: RFC: IPSEC patch 0 for netlink events

[Patrick McHardy]

jamal wrote:

> @@ -478,6 +491,9 @@
>  
>  	if (x1->km.state == XFRM_STATE_ACQ) {
>  		__xfrm_state_insert(x);
> +		/* XXXX: We already have xfrm_state_lock
> +		 * do we need to grab x->lock as well? */
> +		xfrm_sa_notify(x, c, XFRM_SA_ADDED);

To answer this question: no. xfrm_state_lock can be nested in
x->lock, but not the other way around. If you want to avoid that
the state changes below you, you could notify before insertion.

Regards
Patrick

Re: RFC: IPSEC patch 0 for netlink events

[Herbert Xu]

On Mon, Mar 28, 2005 at 11:23:19PM +0200, Patrick McHardy wrote:
> 
> To answer this question: no. xfrm_state_lock can be nested in
> x->lock, but not the other way around. If you want to avoid that

That's true for now but it's something that I'd like to change for
2.6.13.  We should use the refcnt as much as possible and modify
the locking so that the state lock is only used to guard the state
of the state :)

> the state changes below you, you could notify before insertion.

Agreed.

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: RFC: IPSEC patch 0 for netlink events

[Herbert Xu]

Hi Jamal:

On Sun, Mar 27, 2005 at 02:07:29PM -0500, jamal wrote:
>
> to user space). Sample patch, still under construction, attached.
> pfkey already does adverts on its own after a response from the generic
> code. 

This looks good over all.  Just a few minor things.

> --- 26115/include/net/xfrm.h	2005-03-19 01:35:02.000000000 -0500
> +++ 26115-mod/include/net/xfrm.h	2005-03-27 09:00:03.000000000 -0500
> @@ -157,6 +157,36 @@
>  	XFRM_STATE_DEAD
>  };
>  
> +/* events that could be sent by kernel */
> +enum {
> +	XFRM_SA_INVALID,
> +	XFRM_SA_EXPIRED,
> +	XFRM_SA_ADDED,
> +	XFRM_SA_UPDATED,
> +	XFRM_SA_DELETED,
> +	XFRM_SA_FLUSHED,

What's the difference between DELETED and FLUSHED and why do we
need that distinction?

> +/* callback structure passed from either netlink or pfkey */
> +struct xfrm_sa_cb
> +{
> +	u32	type; /* the type of caller netlink/pfkey/other */
> +	u32	data; /* callee to caller */
> +	void *hdr;
> +	struct sk_buff *skb;

I don't think we need to carry the original hdr and skb around.
I see below that you're using it to fill in the pid/seq when
sending netlink messages.  Since these are multicast messages
resent by the kernel they should not inherit those values from
the original skb.

> +/* the types used in sa_cb */
> +enum {
> +	KM_SA_INVALID,
> +	KM_SA_NETLINK,
> +	KM_SA_PFKEY,
> +	__KM_SA_MAX
> +};
> +#define KM_SA_MAX (__KM_SA_MAX - 1)

If we got rid if hdr/skb above then we don't need this either.

This would reduce xfrm_sa_cb to just one u32.

> --- 26115/net/xfrm/xfrm_state.c	2005-03-19 01:35:00.000000000 -0500
> +++ 26115-mod/net/xfrm/xfrm_state.c	2005-03-27 09:14:09.000000000 -0500
> @@ -402,7 +402,19 @@
>  
>  static struct xfrm_state *__xfrm_find_acq_byseq(u32 seq);
>  
> -int xfrm_state_add(struct xfrm_state *x)
> +static struct list_head xfrm_km_list = LIST_HEAD_INIT(xfrm_km_list);
> +static DEFINE_RWLOCK(xfrm_km_lock);
> +
> +void xfrm_sa_notify(struct xfrm_state *x, struct xfrm_sa_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);
> +}

It would be more consistent to call this km_state_notify and put it
next to the other km functions below.

> +int xfrm_state_add(struct xfrm_state *x, struct xfrm_sa_cb *c)

We wouldn't need the cb argument here once you get rid of
everything there apart from data.
  
> -int xfrm_state_update(struct xfrm_state *x)
> +int xfrm_state_update(struct xfrm_state *x, struct xfrm_sa_cb *c)

Ditto.

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: RFC: IPSEC patch 0 for netlink events

[jamal]

On Tue, 2005-03-29 at 19:49, Herbert Xu wrote:

> >  
> > +/* events that could be sent by kernel */
> > +enum {
> > +	XFRM_SA_INVALID,
> > +	XFRM_SA_EXPIRED,
> > +	XFRM_SA_ADDED,
> > +	XFRM_SA_UPDATED,
> > +	XFRM_SA_DELETED,
> > +	XFRM_SA_FLUSHED,
> 
> What's the difference between DELETED and FLUSHED and why do we
> need that distinction?
> 

In the case you flush you dont wanna send multiple DELETEs. Just one 
FLUSH at the end when flushing is done. At least this is behavior
of PFKEY (at least on the RFC). I think its a good thing.

> > +/* callback structure passed from either netlink or pfkey */
> > +struct xfrm_sa_cb
> > +{
> > +	u32	type; /* the type of caller netlink/pfkey/other */
> > +	u32	data; /* callee to caller */
> > +	void *hdr;
> > +	struct sk_buff *skb;
> 
> I don't think we need to carry the original hdr and skb around.
> I see below that you're using it to fill in the pid/seq when
> sending netlink messages.  Since these are multicast messages
> resent by the kernel they should not inherit those values from
> the original skb.
> 

Thinking ... thinking .. Good point.

I defineteley need at least:
a) case original message sent via netlink: pid of the original process
so when i multicast in netlink i dont echo back to it.
b) case original message sent via pfkey: 
When netlink side manager is invoked i ignore the pid and multicast to
every listener of that mcast group.

I may have gotten lazy while coding and shotgunned with both
the skb and the header thinking as i keep coding i may need more off
the originators message.

> > +/* the types used in sa_cb */
> > +enum {
> > +	KM_SA_INVALID,
> > +	KM_SA_NETLINK,
> > +	KM_SA_PFKEY,
> > +	__KM_SA_MAX
> > +};
> > +#define KM_SA_MAX (__KM_SA_MAX - 1)
> 
> If we got rid if hdr/skb above then we don't need this either.
> 

refer to b) above; i use these to figure the originator km.

> This would reduce xfrm_sa_cb to just one u32.
> 
> > --- 26115/net/xfrm/xfrm_state.c	2005-03-19 01:35:00.000000000 -0500
> > +++ 26115-mod/net/xfrm/xfrm_state.c	2005-03-27 09:14:09.000000000 -0500
> > @@ -402,7 +402,19 @@
> >  
> >  static struct xfrm_state *__xfrm_find_acq_byseq(u32 seq);
> >  
> > -int xfrm_state_add(struct xfrm_state *x)
> > +static struct list_head xfrm_km_list = LIST_HEAD_INIT(xfrm_km_list);
> > +static DEFINE_RWLOCK(xfrm_km_lock);
> > +
> > +void xfrm_sa_notify(struct xfrm_state *x, struct xfrm_sa_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);
> > +}
> 
> It would be more consistent to call this km_state_notify and put it
> next to the other km functions below.
> 

I will make the change.

> > +int xfrm_state_add(struct xfrm_state *x, struct xfrm_sa_cb *c)
> 
> We wouldn't need the cb argument here once you get rid of
> everything there apart from data.

We would need at least those two variables. 

>   
> > -int xfrm_state_update(struct xfrm_state *x)
> > +int xfrm_state_update(struct xfrm_state *x, struct xfrm_sa_cb *c)
> 
> Ditto.
> 

Ditto.
It is safer to maintain the cb structure incase we need to add more
variables in the future. Let me complete this - If it turns out that
i only need one 32 bit variable then its not worth it. Note: I plan to
use the same structure for the SPD side; so i will change its name to
xfrm_sap_cb

cheers,
jamal 

Re: RFC: IPSEC patch 0 for netlink events

[Herbert Xu]

Hi Jamal:

On Wed, Mar 30, 2005 at 07:09:32AM -0500, jamal wrote:
> 
> In the case you flush you dont wanna send multiple DELETEs. Just one 
> FLUSH at the end when flushing is done. At least this is behavior
> of PFKEY (at least on the RFC). I think its a good thing.

You're right.

> > > +/* callback structure passed from either netlink or pfkey */
> > > +struct xfrm_sa_cb
> > > +{
> > > +	u32	type; /* the type of caller netlink/pfkey/other */
> > > +	u32	data; /* callee to caller */
> > > +	void *hdr;
> > > +	struct sk_buff *skb;
> > 
> > I don't think we need to carry the original hdr and skb around.
> > I see below that you're using it to fill in the pid/seq when
> > sending netlink messages.  Since these are multicast messages
> > resent by the kernel they should not inherit those values from
> > the original skb.
> > 
> 
> Thinking ... thinking .. Good point.
> 
> I defineteley need at least:
> a) case original message sent via netlink: pid of the original process
> so when i multicast in netlink i dont echo back to it.

I don't think you need to do that.  RFC 2367 says that the kernel should
send that message to all listening processes.  It doesn't put an exception
on the sending process.

This makes sense also because the echoed message is really a new packet
sent by the kernel.  It's not necessarily equal to the original packet.

In fact checking the socket pid is not enough to avoid sending the
message back to the originator.  For instance, if the originator
used a different socket for receiving multicast messages then this
wouldn't work anyway.

For netlink it is important to use a different socket for receiving
multicast messages since otherwise you run the risk of losing unicast
messages when it overruns.

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: RFC: IPSEC patch 0 for netlink events

[Herbert Xu]

On Wed, Mar 30, 2005 at 10:23:21PM +1000, herbert wrote:
> 
> For netlink it is important to use a different socket for receiving
> multicast messages since otherwise you run the risk of losing unicast
> messages when it overruns.

This would work for PFKEY too since if we did this, we can simply
remove the BROADCAST_ONE messages from the pfkey_* functions.
-- 
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: RFC: IPSEC patch 0 for netlink events

[jamal]

Herbert,

On Wed, 2005-03-30 at 07:23, Herbert Xu wrote:

> > Thinking ... thinking .. Good point.
> > 
> > I defineteley need at least:
> > a) case original message sent via netlink: pid of the original process
> > so when i multicast in netlink i dont echo back to it.
> 
> I don't think you need to do that.  RFC 2367 says that the kernel should
> send that message to all listening processes.  It doesn't put an exception
> on the sending process.
> 
> This makes sense also because the echoed message is really a new packet
> sent by the kernel.  It's not necessarily equal to the original packet.
> 

I may be influenced by some of the behaviors of rtnetlink users then.
Typically (I may need to go back and double check), they dont echo back.
The final response of success is unicast though if a ACK flag was set in
the original message. Actually an echo may happen in addition to the
success indicator if such an echo is requested for by NLM_F_ECHO.
I will go back and scan, but example:

---
     netlink_broadcast(rtnl, skb, pid, RTMGRP_IPV4_ROUTE, GFP_KERNEL);
        if (n->nlmsg_flags&NLM_F_ECHO)
                netlink_unicast(rtnl, skb, pid, MSG_DONTWAIT);
----

> In fact checking the socket pid is not enough to avoid sending the
> message back to the originator.  For instance, if the originator
> used a different socket for receiving multicast messages then this
> wouldn't work anyway.
> 

True.

> For netlink it is important to use a different socket for receiving
> multicast messages since otherwise you run the risk of losing unicast
> messages when it overruns.
> 

This may be the reason actually now that i think of it. Its not the user
you are avoiding to echo back to, rather the overloading the user. What
i could do is double check for the NLM_F_ECHO as well so we give the
user a choice to receive the message as well if they indicate they need
it. i.e something along:

if (n->nlmsg_flags&NLM_F_ECHO)
  broadcast to all
else
  just send to all sans the originator

sounds reasonable?

I am actually indifferent - if you think strongly about it then i could
be swayed (pending testing)

cheers,
jamal  

Re: RFC: IPSEC patch 0 for netlink events

[Herbert Xu]

Hi Jamal:

On Wed, Mar 30, 2005 at 07:53:30AM -0500, jamal wrote:
>
> I may be influenced by some of the behaviors of rtnetlink users then.
> Typically (I may need to go back and double check), they dont echo back.

That's right.  However, this isn't really an echo.  It's a notification
from the kernel to a particular multicast group.  For example, in the
case of a delete request, the multicast message will presumably contain
full details about the state being deleted.

This is something that may be of interest to the sending process.
 
> > For netlink it is important to use a different socket for receiving
> > multicast messages since otherwise you run the risk of losing unicast
> > messages when it overruns.
> 
> This may be the reason actually now that i think of it. Its not the user
> you are avoiding to echo back to, rather the overloading the user. What

Well the message itself shouldn't overload the user since its size can
be calculated before hand.  If anything is going to overload the netlink
socket it's going to be multicast messages caused by other processes.

Really, if you're going to listen to multicast messages via netlink,
you should do it in a socket where you don't expect to get unicast
responses.
 
> if (n->nlmsg_flags&NLM_F_ECHO)
>   broadcast to all
> else
>   just send to all sans the originator
> 
> sounds reasonable?

Well since I don't think this is an echo request, no :)

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: RFC: IPSEC patch 0 for netlink events

[jamal]

Herbert,

Since you feel strongly about this - I will make the change and
we can always blame the RFC;->
I need to do some testing later tonight to make sure nothing goes
berserk.

cheers,
jamal

On Wed, 2005-03-30 at 08:14, Herbert Xu wrote:
> Hi Jamal:
> 
> On Wed, Mar 30, 2005 at 07:53:30AM -0500, jamal wrote:
> >
> > I may be influenced by some of the behaviors of rtnetlink users then.
> > Typically (I may need to go back and double check), they dont echo back.
> 
> That's right.  However, this isn't really an echo.  It's a notification
> from the kernel to a particular multicast group.  For example, in the
> case of a delete request, the multicast message will presumably contain
> full details about the state being deleted.
> 
> This is something that may be of interest to the sending process.
>  
> > > For netlink it is important to use a different socket for receiving
> > > multicast messages since otherwise you run the risk of losing unicast
> > > messages when it overruns.
> > 
> > This may be the reason actually now that i think of it. Its not the user
> > you are avoiding to echo back to, rather the overloading the user. What
> 
> Well the message itself shouldn't overload the user since its size can
> be calculated before hand.  If anything is going to overload the netlink
> socket it's going to be multicast messages caused by other processes.
> 
> Really, if you're going to listen to multicast messages via netlink,
> you should do it in a socket where you don't expect to get unicast
> responses.
>  
> > if (n->nlmsg_flags&NLM_F_ECHO)
> >   broadcast to all
> > else
> >   just send to all sans the originator
> > 
> > sounds reasonable?
> 
> Well since I don't think this is an echo request, no :)
> 
> Cheers,

Re: RFC: IPSEC patch 0 for netlink events

[jamal]

Herbert,

On Tue, 2005-03-29 at 19:49, Herbert Xu wrote:
> Hi Jamal:
> 
> On Sun, Mar 27, 2005 at 02:07:29PM -0500, jamal wrote:
> >

> > +/* callback structure passed from either netlink or pfkey */
> > +struct xfrm_sa_cb
> > +{
> > +	u32	type; /* the type of caller netlink/pfkey/other */
> > +	u32	data; /* callee to caller */
> > +	void *hdr;
> > +	struct sk_buff *skb;
> 
> I don't think we need to carry the original hdr and skb around.

Any objection to still calling it (new name) struct xfrm_sa_cb but
with only one 32 bit value in it - namely data?
This way i could add any other thing in the future in it without
changing the function call paramaters.

cheers,
jamal

Re: RFC: IPSEC patch 0 for netlink events

[Herbert Xu]

On Wed, Mar 30, 2005 at 09:55:59PM -0500, jamal wrote:
> 
> Any objection to still calling it (new name) struct xfrm_sa_cb but
> with only one 32 bit value in it - namely data?
> This way i could add any other thing in the future in it without
> changing the function call paramaters.

That's fine by me.

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: RFC: IPSEC patch 0 for netlink events

[Herbert Xu]

On Thu, Mar 31, 2005 at 01:00:36PM +1000, herbert wrote:
> On Wed, Mar 30, 2005 at 09:55:59PM -0500, jamal wrote:
> > 
> > Any objection to still calling it (new name) struct xfrm_sa_cb but
> > with only one 32 bit value in it - namely data?
> > This way i could add any other thing in the future in it without
> > changing the function call paramaters.
> 
> That's fine by me.

Actually, could you rename it to km_state_cb? This information is
only used for communication with the the km and it should be marked
as such just like the other km_* identifiers.
-- 
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: RFC: IPSEC patch 0 for netlink events

[jamal]

On Wed, 2005-03-30 at 22:01, Herbert Xu wrote:

> Actually, could you rename it to km_state_cb? This information is
> only used for communication with the the km and it should be marked
> as such just like the other km_* identifiers.

no problemo. 
[Should be able to post patch for both SP+SA with all 
these changes maybe sometime tommorow. Both Masahide and myself are
doing some testing].

cheers,
jamal

Re: RFC: IPSEC patch 0 for netlink events

[Herbert Xu]

Hi Jamal:

On Sun, Mar 27, 2005 at 02:07:29PM -0500, jamal wrote:
>
> @@ -478,6 +491,9 @@
>  
>  	if (x1->km.state == XFRM_STATE_ACQ) {
>  		__xfrm_state_insert(x);
> +		/* XXXX: We already have xfrm_state_lock
> +		 * do we need to grab x->lock as well? */
> +		xfrm_sa_notify(x, c, XFRM_SA_ADDED);

Actually, this shouldn't be here at all.

Calls to xfrm_sa_notify (better to call it km_state_notify for
consistency) should be made from the places where we currently
call pfkey_broadcast and the corresponding locations 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: RFC: IPSEC patch 0 for netlink events

[jamal]

On Wed, 2005-03-30 at 07:55, Herbert Xu wrote:
> Hi Jamal:
> 
> On Sun, Mar 27, 2005 at 02:07:29PM -0500, jamal wrote:
> >
> > @@ -478,6 +491,9 @@
> >  
> >  	if (x1->km.state == XFRM_STATE_ACQ) {
> >  		__xfrm_state_insert(x);
> > +		/* XXXX: We already have xfrm_state_lock
> > +		 * do we need to grab x->lock as well? */
> > +		xfrm_sa_notify(x, c, XFRM_SA_ADDED);
> 
> Actually, this shouldn't be here at all.
> 
> Calls to xfrm_sa_notify (better to call it km_state_notify for
> consistency) should be made from the places where we currently
> call pfkey_broadcast and the corresponding locations in xfrm_user.
> 

we let the km make the call; pfkey already had these calls and i believe
they are the ones that should go in order to be generic. Actually more
like split into two. But thats defered for later (if it ever gets done).

cheers,
jamal

Re: RFC: IPSEC patch 0 for netlink events

[Herbert Xu]

On Wed, Mar 30, 2005 at 08:18:25AM -0500, jamal wrote:
> 
> we let the km make the call; pfkey already had these calls and i believe
> they are the ones that should go in order to be generic. Actually more
> like split into two. But thats defered for later (if it ever gets done).

Yes you're right.  I thought you were putting it into xfrm_state_insert
but that's not the case.

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