netdev
[Top] [All Lists]

Re: PATCH: IPSEC xfrm events

To: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
Subject: Re: PATCH: IPSEC xfrm events
From: jamal <hadi@xxxxxxxxxx>
Date: 01 Apr 2005 06:03:18 -0500
Cc: Patrick McHardy <kaber@xxxxxxxxx>, Masahide NAKAMURA <nakam@xxxxxxxxxxxxxx>, "David S. Miller" <davem@xxxxxxxxxxxxx>, netdev <netdev@xxxxxxxxxxx>
In-reply-to: <20050401042106.GA27762@gondor.apana.org.au>
Organization: jamalopolous
References: <1112319441.1089.83.camel@jzny.localdomain> <20050401042106.GA27762@gondor.apana.org.au>
Reply-to: hadi@xxxxxxxxxx
Sender: netdev-bounce@xxxxxxxxxxx
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


<Prev in Thread] Current Thread [Next in Thread>