| Message ID | 1270030626-16687-3-git-send-email-timo.teras@iki.fi |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, Mar 31, 2010 at 01:17:03PM +0300, Timo Teras wrote: > __xfrm_state_bump_genids() is used to update the genid of all > matching xfrm_state's, so any bundle using the state would get > refreshed with the newly inserted state. > > However, since __xfrm_state_bump_genids() is called before the > __xfrm_state_insert() which actually bumps the genid counter, > it is possible that the genid was not updated at all (if there > was no state inserts previously). > > This is fixed by moving the genid incrementation to > __xfrm_state_bump_genids() so the older states are guaranteed > to get different genid. > > Signed-off-by: Timo Teras <timo.teras@iki.fi> It would appear that not all xfrm_state_insert calls are preceded by xfrm_state_bump_genids so this patch isn't correct. Cheers,
Herbert Xu wrote: > On Wed, Mar 31, 2010 at 01:17:03PM +0300, Timo Teras wrote: >> __xfrm_state_bump_genids() is used to update the genid of all >> matching xfrm_state's, so any bundle using the state would get >> refreshed with the newly inserted state. >> >> However, since __xfrm_state_bump_genids() is called before the >> __xfrm_state_insert() which actually bumps the genid counter, >> it is possible that the genid was not updated at all (if there >> was no state inserts previously). >> >> This is fixed by moving the genid incrementation to >> __xfrm_state_bump_genids() so the older states are guaranteed >> to get different genid. >> >> Signed-off-by: Timo Teras <timo.teras@iki.fi> > > It would appear that not all xfrm_state_insert calls are preceded > by xfrm_state_bump_genids so this patch isn't correct. Yes, I noticed that there's one. But __xfrm_state_insert() is called to replace an acquire in that case. Acquires are never used in bundle, so this is good. But maybe it'd be more explicit if the genid increment is done before each __xfrm_state_insert()? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Timo Teräs wrote: > Herbert Xu wrote: >> On Wed, Mar 31, 2010 at 01:17:03PM +0300, Timo Teras wrote: >>> __xfrm_state_bump_genids() is used to update the genid of all >>> matching xfrm_state's, so any bundle using the state would get >>> refreshed with the newly inserted state. >>> >>> However, since __xfrm_state_bump_genids() is called before the >>> __xfrm_state_insert() which actually bumps the genid counter, >>> it is possible that the genid was not updated at all (if there >>> was no state inserts previously). >>> >>> This is fixed by moving the genid incrementation to >>> __xfrm_state_bump_genids() so the older states are guaranteed >>> to get different genid. >>> >>> Signed-off-by: Timo Teras <timo.teras@iki.fi> >> >> It would appear that not all xfrm_state_insert calls are preceded >> by xfrm_state_bump_genids so this patch isn't correct. > > Yes, I noticed that there's one. But __xfrm_state_insert() is > called to replace an acquire in that case. Acquires are never > used in bundle, so this is good. > > But maybe it'd be more explicit if the genid increment is done > before each __xfrm_state_insert()? Actually. Search for xfrm_state_genid in xfrm_state.c. It's only used in two places: __xfrm_state_bumb_genid() and _insert(). The only case when it needs to get incremented is in the bump function. If we are adding a state for the first time, there's no need to bump the genid as it's per-matching state. The actual work to invalidate states is done in the bump function, so it's the only place that needs to increment it. If any other xfrm_state_insert place needs to invalidate old states it needs an additional bumping call. So the bumping function is the right place to increment the genid. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c index 17d5b96..b4efc28 100644 --- a/net/xfrm/xfrm_state.c +++ b/net/xfrm/xfrm_state.c @@ -923,7 +923,7 @@ static void __xfrm_state_insert(struct xfrm_state *x) struct net *net = xs_net(x); unsigned int h; - x->genid = ++xfrm_state_genid; + x->genid = xfrm_state_genid; list_add(&x->km.all, &net->xfrm.state_all); @@ -963,6 +963,7 @@ static void __xfrm_state_bump_genids(struct xfrm_state *xnew) unsigned int h; u32 mark = xnew->mark.v & xnew->mark.m; + xfrm_state_genid++; h = xfrm_dst_hash(net, &xnew->id.daddr, &xnew->props.saddr, reqid, family); hlist_for_each_entry(x, entry, net->xfrm.state_bydst+h, bydst) { if (x->props.family == family &&
__xfrm_state_bump_genids() is used to update the genid of all matching xfrm_state's, so any bundle using the state would get refreshed with the newly inserted state. However, since __xfrm_state_bump_genids() is called before the __xfrm_state_insert() which actually bumps the genid counter, it is possible that the genid was not updated at all (if there was no state inserts previously). This is fixed by moving the genid incrementation to __xfrm_state_bump_genids() so the older states are guaranteed to get different genid. Signed-off-by: Timo Teras <timo.teras@iki.fi> --- net/xfrm/xfrm_state.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)