On Fri, 2005-04-01 at 06:42, Herbert Xu wrote:
> 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 think either scheme is fine really;-> I will definetely go back and
consider the approach you are suggesting and see if it results into
more maintanable code - then fair. Otherwise you realize its more work
for me ;->
> > > 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.
>
I havent checked the state machine closely, but the following seems to
make sense:
The first thing that happens to delete the state/policy should win if
the state/policy is transitioned to dead.
> 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.
>
Does it really make sense, Herbert? ;->
What is it that you just flushed that results in the event?
The RFC is ambigous in my opinion. Look at what it says about deleting
(same ambiguity).
----
3.1.4 SADB_DELETE
The SADB_DELETE message causes the kernel to delete a Security
Association from the key table. The delete message consists of the
base header followed by the association, and the source and
destination sockaddrs in the address extension. The kernel deletes
the security association matching the type, spi, source address, and
destination address in the message.
The message behavior for SADB_DELETE is as follows:
Send an SADB_DELETE message from a user process to the kernel.
<base, SA(*), address(SD)>
The kernel returns the SADB_DELETE message to all listening
processes.
<base, SA(*), address(SD)>
------
So why would you generate an event in the case when you didnt delete anything?
> > 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.
>
I will go over the code and review.
You may be absolutely right - thats the better approach to take.
> 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.
>
Thats a bug really which is being exposed now. So it has nothing to do
with the approach taken ;->
No expire should be sent if the policy has transitioned to dead. The bug
is trivial to fix - and actually should be fixed regardless of this
patch.
> > 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.
>
Good point. I will stay lazy and just set a 0 ;->
cheers,
jamal
|