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
|