Re: take 2 WAS(Re: PATCH: IPSEC xfrm events

[jamal <hadi@cyberus.ca>]

Herbert,
Now that you are picking on whitespaces i think we are almost there ;->
Comments below

On Sun, 2005-04-03 at 20:58, Herbert Xu wrote:

> On Sun, Apr 03, 2005 at 10:31:58AM -0400, jamal wrote:
[.. ..]

> > +
> > +EXPORT_SYMBOL(km_policy_notify);
> > +EXPORT_SYMBOL(km_state_notify);
> 
> Can we perhaps move these lines next to the other km functions
> further down? They look rather lonely here.
> 

Sure. 

> > +	/* XXX: Do we wanna do this right at the top??
> > +	 * if the state is dead we dont want to announce 
> > +	 * the expire - a delete may already have announced
> > +	 * it 
> > +	*/
> 
> Please code this check differently so that it isn't racy.
> 
> One way to do it is to change xfrm_timer_handler to do:
> 
> 	if (__xfrm_state_delete(x) && x->id.spi)
> 		km_state_expired(x, 1);
> 
> > +	/* XXX: Do we still wanna wakeup km_waitq?
> > +	 * if the policy is dead we dont want to announce 
> > +	 * the expire - a delete may already have announced
> > +	 * it 
> > +	*/
> 
> Ditto.
> 

I think i am gonna take out any attempts to address this race above.
It's a bug thats there already - a separate patch after this will be
better.

> > --- a/net/xfrm/xfrm_policy.c	2005-03-25 22:28:21.000000000 -0500
> > +++ b/net/xfrm/xfrm_policy.c	2005-04-02 12:16:30.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)
> 
> What's this for?
>   

Good catch - gunk from previous patch.

> > +	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);
> 
> You need to hold onto x here.  So do a hold before you call xfrm_state_*
> and then drop the reference after km_state_notify.

Good point.

>   
> >  static int xfrm_del_sa(struct sk_buff *skb, struct nlmsghdr *nlh, void **xfrma)
>   
> > -	xfrm_state_delete(x);
> > +	err = xfrm_state_delete(x);
> > +	if (err < 0) {
> > +		x->km.state = XFRM_STATE_DEAD;
> > +		xfrm_state_put(x);
> > +		return err;
> 
> If the xfrm_state_delete fails then it's already dead.  So kill
> the line that modifies its state.
> 

Good point.

> > +static int xfrm_notify_sa( struct xfrm_state *x, struct km_event *c)
> 
> Extra space after the paren.
> 
> > +	int len = NLMSG_LENGTH(sizeof(struct xfrm_usersa_info));
> 
> Please add the additional payloads for NAT-T and the keys.
> 

I dont think we should broadcast out keys. 
NAT-T - where do i look at to see what to send?

> > +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 = NLMSG_LENGTH(sizeof(struct xfrm_userpolicy_info));
> 
> Please attach the templates.
> 

What is not being attached right now?

> > @@ -1256,7 +1328,7 @@
> >  
> >  	if (hdr->sadb_msg_type == SADB_ADD)
> >  		err = xfrm_state_add(x);
> > -	else
> > +	else 
> 
> A better editor that doesn't leave trailing spaces is needed here :)

you insulting vi? ;->

>   	
> > -	xfrm_state_delete(x);
> > -	xfrm_state_put(x);
> > +	err = xfrm_state_delete(x);
> > +	if (err < 0) {
> > +		x->km.state = XFRM_STATE_DEAD;
> 
> Please remove this line as it's already dead if the delete fails.
>   
> > +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));
> > +	// XXX:do we have to pass proto as well?
> 
> I think so.  A flush of all IPCOMP states is certainly quite different
> from a flush of all states.  It's just a matter of calling satype2proto.
> 

Looks doable.


> > -	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;
> 
> However, you do need to keep this code for the real GET case.
> 

Get seems to a separate entry point - pfkey_get() which i didnt touch.

cheers,
jamal