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@xxxxxxxxxxxxxxxxxxx>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
|