netdev
[Top] [All Lists]

Re: PATCH: IPSEC xfrm events

To: jamal <hadi@xxxxxxxxxx>
Subject: Re: PATCH: IPSEC xfrm events
From: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
Date: Fri, 1 Apr 2005 21:42:58 +1000
Cc: Patrick McHardy <kaber@xxxxxxxxx>, Masahide NAKAMURA <nakam@xxxxxxxxxxxxxx>, "David S. Miller" <davem@xxxxxxxxxxxxx>, netdev <netdev@xxxxxxxxxxx>
In-reply-to: <1112353398.1096.116.camel@jzny.localdomain>
References: <1112319441.1089.83.camel@jzny.localdomain> <20050401042106.GA27762@gondor.apana.org.au> <1112353398.1096.116.camel@jzny.localdomain>
Sender: netdev-bounce@xxxxxxxxxxx
User-agent: Mutt/1.5.6+20040907i
On Fri, Apr 01, 2005 at 06:03:18AM -0500, jamal wrote:
>
> > 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

I understand.  However, that's not determined by where you put the
km_notify call itself.  Even when you call km_notify from af_key
or xfrm_user it will notify every km in the system.

It's the fact that we're calling km_notify instead of pfkey_broadcast
or netlink_broadcast that's important, not the location.

Having the km_notify call made in af_key/xfrm_user is convenient though
for the reason I outlined above.
 
> 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 ... 

That's what the ref counter is for.

> > 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?

Imagine you've got a KM that's trying to delete a state via af_key that's
about to expire.  If pfkey_delete looks up the state successfully, and
then the timer triggers before the actual xfrm_state_delete, you will
get one event generated by the timer and another by pfkey_delete.

> Again like i said: I need to tell every km user about the event, not
> just the originator.

I'm suggesting that you add the km_notify calls to af_key and xfrm_user.
That will take care of notifying everyone.
 
> 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?

RFC 2367 says that:

     The messaging behavior for SADB_FLUSH is:

           Send an SADB_FLUSH message from a user process to the kernel.

           <base>

           The kernel will return an SADB_FLUSH message to all listening
           sockets.

           <base>

As you can see, there is no exception for the case of an empty database.
So my interpretation would be that a broadcast is needed.

> Refer to my comments above on being able to tell multiple managers about
> the events originated by one. 

May I also refer you to my comment above about this being achieved
by calling km_notify, even if you do it from within af_key or
xfrm_user :)

> 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.

Try this as an exercise.  List all the xfrm_policy_kills that need
notifications and all those that don't, you will find that the former
all originate from delete/flush commands in af_key/xfrm_user, while
the latter originate from other callers.

In other words, by placing the call in af_key/xfrm_user you simplify
the logic and make it more maintainable.

> > 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.

Well when the policy expires you will get one expire notification from
the current timer code and a new one from your patch since the timer
calls xfrm_policy_delete.

See my point? By putting the call in xfrm_policy.c you have to be
really careful in dividing the internal users which shouldn't
generate notifications and the external users which should.  By doing
it in af_key/xfrm_user you can avoid all this work.

> 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.

Well I must repeat that if you were calling km_notify from
af_key/xfrm_user you will be sending these events to all km's
no matter what their affiliation is :)

> > 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?

Using jiffies means that you can have two successive messages that
share the same sequence number.  It's not a big deal of course.  But
if we're going to indicate ordering, we might as well go the full
length.

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

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