[Top] [All Lists]

Re: take 2-2 WAS(Re: PATCH: IPSEC xfrm events

To: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
Subject: Re: take 2-2 WAS(Re: PATCH: IPSEC xfrm events
From: jamal <hadi@xxxxxxxxxx>
Date: 04 Apr 2005 08:51:37 -0400
Cc: Patrick McHardy <kaber@xxxxxxxxx>, Masahide NAKAMURA <nakam@xxxxxxxxxxxxxx>, "David S. Miller" <davem@xxxxxxxxxxxxx>, netdev <netdev@xxxxxxxxxxx>
In-reply-to: <20050404121641.GA12103@xxxxxxxxxxxxxxxxxxx>
Organization: jamalopolous
References: <1112358278.1096.160.camel@xxxxxxxxxxxxxxxx> <20050401123554.GA3468@xxxxxxxxxxxxxxxxxxx> <1112403845.1088.14.camel@xxxxxxxxxxxxxxxx> <20050402012813.GA24575@xxxxxxxxxxxxxxxxxxx> <1112406164.1088.54.camel@xxxxxxxxxxxxxxxx> <20050402014619.GB24861@xxxxxxxxxxxxxxxxxxx> <1112469601.1088.173.camel@xxxxxxxxxxxxxxxx> <1112538718.1096.394.camel@xxxxxxxxxxxxxxxx> <20050404005805.GA16543@xxxxxxxxxxxxxxxxxxx> <1112614706.1096.439.camel@xxxxxxxxxxxxxxxx> <20050404121641.GA12103@xxxxxxxxxxxxxxxxxxx>
Reply-to: hadi@xxxxxxxxxx
Sender: netdev-bounce@xxxxxxxxxxx

On Mon, 2005-04-04 at 08:16, Herbert Xu wrote:
> Hi Jamal:
> On Mon, Apr 04, 2005 at 07:38:26AM -0400, jamal wrote:
> > Ok, heres an update.

> > +void km_policy_notify(struct xfrm_policy *xp, int dir, struct km_event *c);
> > +void km_state_notify(struct xfrm_state *x, struct km_event *c);
> No need for these prototypes since they're already in xfrm.h.

Good catch.

> > @@ -764,37 +778,60 @@
> >  }
> >  EXPORT_SYMBOL(xfrm_replay_advance);
> >  
> > -static struct list_head xfrm_km_list = LIST_HEAD_INIT(xfrm_km_list);
> > -static DEFINE_RWLOCK(xfrm_km_lock);
> How about letting these guys stay where they are? The move was
> necessary before because the km_*_notify functions had to be called
> in this file but that's no longer the case.

- dont see what the harm was as they were in that patch though.

> Sorry, you need to hold x before the call to
> xfrm_state_add/xfrm_state_update as otherwise
> they can cause x to be freed.
> In general hold/put is only useful if
> 1) When you call hold you already have a reference to the object.
> 2) In between the hold/put you may free the object.
> Same problem as xfrm_user.  We need the hold to occur before the
> add/update for it to be effective.  In fact the original code was
> buggy too since it didn't hold a reference at all.
> Of course this is very unlikely to crash since it requires a
> small life time and some bad luck in getting preempted.

;-> Yes, indeed. I think its time for you to throw in the towel ;->
There was really not even a need for that hold given the likelihood of
anything like this happening. Anyways ive made this fix and heres the
updated patch.


Attachment: ipsec-event-take2-3
Description: Text document

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