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
|