| Message ID | 1347033467-3757-2-git-send-email-nicolas.dichtel@6wind.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On 09/07/2012 11:57 AM, Nicolas Dichtel wrote: > When a xfrm policy is inserted or deleted, we must invalidate > all dst and recalculate the route. > One suggestion may be to add an inline to get flow_cache_genid and then you can get rid of #ifdefs in the code Otherwise, this extends what IPv6 is doing to all protocols. Seem like the right thing to do, but I'll let DaveM and others make the final determination. -vlad > Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> > --- > include/net/dst.h | 7 +++++++ > net/core/dst.c | 1 + > net/core/sock.c | 4 ++-- > net/ipv6/ip6_tunnel.c | 3 +-- > 4 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/include/net/dst.h b/include/net/dst.h > index 9a78810..478e55a 100644 > --- a/include/net/dst.h > +++ b/include/net/dst.h > @@ -101,6 +101,9 @@ struct dst_entry { > atomic_t __refcnt; /* client references */ > int __use; > unsigned long lastuse; > +#ifdef CONFIG_XFRM > + u32 flow_cache_genid; > +#endif > union { > struct dst_entry *next; > struct rtable __rcu *rt_next; > @@ -457,6 +460,10 @@ static inline int dst_input(struct sk_buff *skb) > > static inline struct dst_entry *dst_check(struct dst_entry *dst, u32 cookie) > { > +#ifdef CONFIG_XFRM > + if (dst->flow_cache_genid != atomic_read(&flow_cache_genid)) > + return NULL; > +#endif > if (dst->obsolete) > dst = dst->ops->check(dst, cookie); > return dst; > diff --git a/net/core/dst.c b/net/core/dst.c > index f6593d2..19899d3 100644 > --- a/net/core/dst.c > +++ b/net/core/dst.c > @@ -181,6 +181,7 @@ void *dst_alloc(struct dst_ops *ops, struct net_device *dev, > dst->path = dst; > #ifdef CONFIG_XFRM > dst->xfrm = NULL; > + dst->flow_cache_genid = atomic_read(&flow_cache_genid); > #endif > dst->input = dst_discard; > dst->output = dst_discard; > diff --git a/net/core/sock.c b/net/core/sock.c > index d765156..1d06d4e 100644 > --- a/net/core/sock.c > +++ b/net/core/sock.c > @@ -491,7 +491,7 @@ struct dst_entry *__sk_dst_check(struct sock *sk, u32 cookie) > { > struct dst_entry *dst = __sk_dst_get(sk); > > - if (dst && dst->obsolete && dst->ops->check(dst, cookie) == NULL) { > + if (dst && dst_check(dst, cookie) == NULL) { > sk_tx_queue_clear(sk); > RCU_INIT_POINTER(sk->sk_dst_cache, NULL); > dst_release(dst); > @@ -506,7 +506,7 @@ struct dst_entry *sk_dst_check(struct sock *sk, u32 cookie) > { > struct dst_entry *dst = sk_dst_get(sk); > > - if (dst && dst->obsolete && dst->ops->check(dst, cookie) == NULL) { > + if (dst && dst_check(dst, cookie) == NULL) { > sk_dst_reset(sk); > dst_release(dst); > return NULL; > diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c > index cb7e2de..e96431a 100644 > --- a/net/ipv6/ip6_tunnel.c > +++ b/net/ipv6/ip6_tunnel.c > @@ -130,8 +130,7 @@ struct dst_entry *ip6_tnl_dst_check(struct ip6_tnl *t) > { > struct dst_entry *dst = t->dst_cache; > > - if (dst && dst->obsolete && > - dst->ops->check(dst, t->dst_cookie) == NULL) { > + if (dst && dst_check(dst, t->dst_cookie) == NULL) { > t->dst_cache = NULL; > dst_release(dst); > return NULL; > -- 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
On Fri, 2012-09-07 at 17:57 +0200, Nicolas Dichtel wrote: > When a xfrm policy is inserted or deleted, we must invalidate > all dst and recalculate the route. > > Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> > --- > include/net/dst.h | 7 +++++++ > net/core/dst.c | 1 + > net/core/sock.c | 4 ++-- > net/ipv6/ip6_tunnel.c | 3 +-- > 4 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/include/net/dst.h b/include/net/dst.h > index 9a78810..478e55a 100644 > --- a/include/net/dst.h > +++ b/include/net/dst.h > @@ -101,6 +101,9 @@ struct dst_entry { > atomic_t __refcnt; /* client references */ > int __use; > unsigned long lastuse; > +#ifdef CONFIG_XFRM > + u32 flow_cache_genid; > +#endif > union { > struct dst_entry *next; > struct rtable __rcu *rt_next; > @@ -457,6 +460,10 @@ static inline int dst_input(struct sk_buff *skb) > > static inline struct dst_entry *dst_check(struct dst_entry *dst, u32 cookie) > { > +#ifdef CONFIG_XFRM > + if (dst->flow_cache_genid != atomic_read(&flow_cache_genid)) > + return NULL; > +#endif Hmm... cant we reuse rt_genid ? (When changing flow_cache_genid, change &net->ipv4.rt_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
Le 07/09/2012 16:35, Eric Dumazet a écrit : > On Fri, 2012-09-07 at 17:57 +0200, Nicolas Dichtel wrote: >> When a xfrm policy is inserted or deleted, we must invalidate >> all dst and recalculate the route. >> >> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> >> --- >> include/net/dst.h | 7 +++++++ >> net/core/dst.c | 1 + >> net/core/sock.c | 4 ++-- >> net/ipv6/ip6_tunnel.c | 3 +-- >> 4 files changed, 11 insertions(+), 4 deletions(-) >> >> diff --git a/include/net/dst.h b/include/net/dst.h >> index 9a78810..478e55a 100644 >> --- a/include/net/dst.h >> +++ b/include/net/dst.h >> @@ -101,6 +101,9 @@ struct dst_entry { >> atomic_t __refcnt; /* client references */ >> int __use; >> unsigned long lastuse; >> +#ifdef CONFIG_XFRM >> + u32 flow_cache_genid; >> +#endif >> union { >> struct dst_entry *next; >> struct rtable __rcu *rt_next; >> @@ -457,6 +460,10 @@ static inline int dst_input(struct sk_buff *skb) >> >> static inline struct dst_entry *dst_check(struct dst_entry *dst, u32 cookie) >> { >> +#ifdef CONFIG_XFRM >> + if (dst->flow_cache_genid != atomic_read(&flow_cache_genid)) >> + return NULL; >> +#endif > > Hmm... cant we reuse rt_genid ? > > (When changing flow_cache_genid, change &net->ipv4.rt_genid) And so adding a new field in net->ipv6? Regards, Nicolas -- 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
On 09/07/2012 10:35 AM, Eric Dumazet wrote: > On Fri, 2012-09-07 at 17:57 +0200, Nicolas Dichtel wrote: >> When a xfrm policy is inserted or deleted, we must invalidate >> all dst and recalculate the route. >> >> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> >> --- >> include/net/dst.h | 7 +++++++ >> net/core/dst.c | 1 + >> net/core/sock.c | 4 ++-- >> net/ipv6/ip6_tunnel.c | 3 +-- >> 4 files changed, 11 insertions(+), 4 deletions(-) >> >> diff --git a/include/net/dst.h b/include/net/dst.h >> index 9a78810..478e55a 100644 >> --- a/include/net/dst.h >> +++ b/include/net/dst.h >> @@ -101,6 +101,9 @@ struct dst_entry { >> atomic_t __refcnt; /* client references */ >> int __use; >> unsigned long lastuse; >> +#ifdef CONFIG_XFRM >> + u32 flow_cache_genid; >> +#endif >> union { >> struct dst_entry *next; >> struct rtable __rcu *rt_next; >> @@ -457,6 +460,10 @@ static inline int dst_input(struct sk_buff *skb) >> >> static inline struct dst_entry *dst_check(struct dst_entry *dst, u32 cookie) >> { >> +#ifdef CONFIG_XFRM >> + if (dst->flow_cache_genid != atomic_read(&flow_cache_genid)) >> + return NULL; >> +#endif > > Hmm... cant we reuse rt_genid ? > > (When changing flow_cache_genid, change &net->ipv4.rt_genid) > I thought of that too, but that requires hacking xfrm to know the namespace at the time it changes the flow_cache_genid. This one seems simpler to do. -vlad -- 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
On Fri, 2012-09-07 at 10:51 -0400, Vlad Yasevich wrote: > I thought of that too, but that requires hacking xfrm to know the > namespace at the time it changes the flow_cache_genid. This one seems > simpler to do. Simpler maybe, but its yet another test done in fast path. -- 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
On Fri, 2012-09-07 at 16:47 +0200, Nicolas Dichtel wrote: > Le 07/09/2012 16:35, Eric Dumazet a écrit : > > > > Hmm... cant we reuse rt_genid ? > > > > (When changing flow_cache_genid, change &net->ipv4.rt_genid) > > And so adding a new field in net->ipv6? or move net->ipv4.rt_genid to net->rt_genid Having separate field for IPv4/IPv6 is of little interest IMHO -- 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
Le 07/09/2012 17:09, Eric Dumazet a écrit : > On Fri, 2012-09-07 at 16:47 +0200, Nicolas Dichtel wrote: >> Le 07/09/2012 16:35, Eric Dumazet a écrit : >>> >>> Hmm... cant we reuse rt_genid ? >>> >>> (When changing flow_cache_genid, change &net->ipv4.rt_genid) >> >> And so adding a new field in net->ipv6? > > or move net->ipv4.rt_genid to net->rt_genid > > Having separate field for IPv4/IPv6 is of little interest IMHO > Ok, I will wait feedback from other people and repost a patch after. -- 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
From: Nicolas Dichtel <nicolas.dichtel@6wind.com> Date: Fri, 07 Sep 2012 17:13:35 +0200 > Le 07/09/2012 17:09, Eric Dumazet a écrit : >> On Fri, 2012-09-07 at 16:47 +0200, Nicolas Dichtel wrote: >>> Le 07/09/2012 16:35, Eric Dumazet a écrit : >>>> >>>> Hmm... cant we reuse rt_genid ? >>>> >>>> (When changing flow_cache_genid, change &net->ipv4.rt_genid) >>> >>> And so adding a new field in net->ipv6? >> >> or move net->ipv4.rt_genid to net->rt_genid >> >> Having separate field for IPv4/IPv6 is of little interest IMHO >> > Ok, I will wait feedback from other people and repost a patch after. A global net->rt_genid is definitely the way to go. -- 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
Le 07/09/2012 20:48, David Miller a écrit : > From: Nicolas Dichtel <nicolas.dichtel@6wind.com> > Date: Fri, 07 Sep 2012 17:13:35 +0200 > >> Le 07/09/2012 17:09, Eric Dumazet a écrit : >>> On Fri, 2012-09-07 at 16:47 +0200, Nicolas Dichtel wrote: >>>> Le 07/09/2012 16:35, Eric Dumazet a écrit : >>>>> >>>>> Hmm... cant we reuse rt_genid ? >>>>> >>>>> (When changing flow_cache_genid, change &net->ipv4.rt_genid) >>>> >>>> And so adding a new field in net->ipv6? >>> >>> or move net->ipv4.rt_genid to net->rt_genid >>> >>> Having separate field for IPv4/IPv6 is of little interest IMHO >>> >> Ok, I will wait feedback from other people and repost a patch after. > > A global net->rt_genid is definitely the way to go. > So it means that IPv6 dst entries will be invalidated by IPv4 route management. For example, calling rt_cache_flush() will flush IPv6 dst too. Is this acceptable? I will send a new version. -- 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
On Mon, 2012-09-10 at 14:47 +0200, Nicolas Dichtel wrote: > So it means that IPv6 dst entries will be invalidated by IPv4 route management. > For example, calling rt_cache_flush() will flush IPv6 dst too. Is this acceptable? If it can save another test in fast path, this is acceptable, yes. -- 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
The goal of these patches is to fix the following problem: a session is
established (TCP, SCTP) and after a new policy is inserted. The current
code does not recalculate the route, thus the traffic is not encrypted.
The patch propose to check flow_cache_genid value when checking a dst
entry, which is incremented each time a policy is inserted or deleted.
v2: use net->ipv4.rt_genid instead of flow_cache_genid (and thus save a test
in fast path). Also move it to net->rt_genid, to be able to use it for IPv6
too. Note that IPv6 will have one more test in fast path.
Patches are tested with TCP and SCTP, IPv4 and IPv6.
Comments are welcome.
Regards,
Nicolas
--
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
On 09/10/2012 09:22 AM, Nicolas Dichtel wrote: > The goal of these patches is to fix the following problem: a session is > established (TCP, SCTP) and after a new policy is inserted. The current > code does not recalculate the route, thus the traffic is not encrypted. > > The patch propose to check flow_cache_genid value when checking a dst > entry, which is incremented each time a policy is inserted or deleted. > > v2: use net->ipv4.rt_genid instead of flow_cache_genid (and thus save a test > in fast path). Also move it to net->rt_genid, to be able to use it for IPv6 > too. Note that IPv6 will have one more test in fast path. > > Patches are tested with TCP and SCTP, IPv4 and IPv6. > > Comments are welcome. > > Regards, > Nicolas > I am not sure this is right... This has a side-effect that when an rt_cache_flush() is called, it invalidates IPv6 routes a well.... Its all fine and good do this when a new policy is added, but not when IPv4 routing table changes. -vlad -- 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
Le 10/09/2012 16:35, Vlad Yasevich a écrit : > On 09/10/2012 09:22 AM, Nicolas Dichtel wrote: >> The goal of these patches is to fix the following problem: a session is >> established (TCP, SCTP) and after a new policy is inserted. The current >> code does not recalculate the route, thus the traffic is not encrypted. >> >> The patch propose to check flow_cache_genid value when checking a dst >> entry, which is incremented each time a policy is inserted or deleted. >> >> v2: use net->ipv4.rt_genid instead of flow_cache_genid (and thus save a test >> in fast path). Also move it to net->rt_genid, to be able to use it for IPv6 >> too. Note that IPv6 will have one more test in fast path. >> >> Patches are tested with TCP and SCTP, IPv4 and IPv6. >> >> Comments are welcome. >> >> Regards, >> Nicolas >> > > I am not sure this is right... This has a side-effect that when an > rt_cache_flush() is called, it invalidates IPv6 routes a well.... > > Its all fine and good do this when a new policy is added, but not when IPv4 > routing table changes. I already ask for this side effect, Eric answers me: http://marc.info/?l=linux-netdev&m=134728265000776&w=2 -- 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
From: Vlad Yasevich <vyasevich@gmail.com> Date: Mon, 10 Sep 2012 10:35:03 -0400 > I am not sure this is right... This has a side-effect that when an > rt_cache_flush() is called, it invalidates IPv6 routes a well.... > > Its all fine and good do this when a new policy is added, but not when > IPv4 routing table changes. I disagree. -- 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
On 09/10/2012 01:18 PM, David Miller wrote: > From: Vlad Yasevich <vyasevich@gmail.com> > Date: Mon, 10 Sep 2012 10:35:03 -0400 > >> I am not sure this is right... This has a side-effect that when an >> rt_cache_flush() is called, it invalidates IPv6 routes a well.... >> >> Its all fine and good do this when a new policy is added, but not when >> IPv4 routing table changes. > > I disagree. > So you are perfectly ok with invalidating IPv6 cache when IPv4 table changes, but not invalidating IPv4 cache if IPv6 table changes? -vlad -- 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
From: Vlad Yasevich <vyasevich@gmail.com> Date: Mon, 10 Sep 2012 13:59:22 -0400 > On 09/10/2012 01:18 PM, David Miller wrote: >> From: Vlad Yasevich <vyasevich@gmail.com> >> Date: Mon, 10 Sep 2012 10:35:03 -0400 >> >>> I am not sure this is right... This has a side-effect that when an >>> rt_cache_flush() is called, it invalidates IPv6 routes a well.... >>> >>> Its all fine and good do this when a new policy is added, but not when >>> IPv4 routing table changes. >> >> I disagree. >> > > So you are perfectly ok with invalidating IPv6 cache when IPv4 table > changes, but not invalidating IPv4 cache if IPv6 table changes? Due to tunneling I can't see how this is avoidable? We do ipv6 over ipv4, but not vice-versa. -- 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
On Monday 2012-09-10 20:01, David Miller wrote: >> >> So you are perfectly ok with invalidating IPv6 cache when IPv4 table >> changes, but not invalidating IPv4 cache if IPv6 table changes? > >Due to tunneling I can't see how this is avoidable? > >We do ipv6 over ipv4, but not vice-versa. I have a setup here where 6 machines are connected with one another (most of them) to form 9 IPsec sessions, all of which are ESP6 links - since native IPv6 is provided - that also handle the site-to-site IPv4 traffic. Does that count? -- 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/include/net/dst.h b/include/net/dst.h index 9a78810..478e55a 100644 --- a/include/net/dst.h +++ b/include/net/dst.h @@ -101,6 +101,9 @@ struct dst_entry { atomic_t __refcnt; /* client references */ int __use; unsigned long lastuse; +#ifdef CONFIG_XFRM + u32 flow_cache_genid; +#endif union { struct dst_entry *next; struct rtable __rcu *rt_next; @@ -457,6 +460,10 @@ static inline int dst_input(struct sk_buff *skb) static inline struct dst_entry *dst_check(struct dst_entry *dst, u32 cookie) { +#ifdef CONFIG_XFRM + if (dst->flow_cache_genid != atomic_read(&flow_cache_genid)) + return NULL; +#endif if (dst->obsolete) dst = dst->ops->check(dst, cookie); return dst; diff --git a/net/core/dst.c b/net/core/dst.c index f6593d2..19899d3 100644 --- a/net/core/dst.c +++ b/net/core/dst.c @@ -181,6 +181,7 @@ void *dst_alloc(struct dst_ops *ops, struct net_device *dev, dst->path = dst; #ifdef CONFIG_XFRM dst->xfrm = NULL; + dst->flow_cache_genid = atomic_read(&flow_cache_genid); #endif dst->input = dst_discard; dst->output = dst_discard; diff --git a/net/core/sock.c b/net/core/sock.c index d765156..1d06d4e 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -491,7 +491,7 @@ struct dst_entry *__sk_dst_check(struct sock *sk, u32 cookie) { struct dst_entry *dst = __sk_dst_get(sk); - if (dst && dst->obsolete && dst->ops->check(dst, cookie) == NULL) { + if (dst && dst_check(dst, cookie) == NULL) { sk_tx_queue_clear(sk); RCU_INIT_POINTER(sk->sk_dst_cache, NULL); dst_release(dst); @@ -506,7 +506,7 @@ struct dst_entry *sk_dst_check(struct sock *sk, u32 cookie) { struct dst_entry *dst = sk_dst_get(sk); - if (dst && dst->obsolete && dst->ops->check(dst, cookie) == NULL) { + if (dst && dst_check(dst, cookie) == NULL) { sk_dst_reset(sk); dst_release(dst); return NULL; diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c index cb7e2de..e96431a 100644 --- a/net/ipv6/ip6_tunnel.c +++ b/net/ipv6/ip6_tunnel.c @@ -130,8 +130,7 @@ struct dst_entry *ip6_tnl_dst_check(struct ip6_tnl *t) { struct dst_entry *dst = t->dst_cache; - if (dst && dst->obsolete && - dst->ops->check(dst, t->dst_cookie) == NULL) { + if (dst && dst_check(dst, t->dst_cookie) == NULL) { t->dst_cache = NULL; dst_release(dst); return NULL;
When a xfrm policy is inserted or deleted, we must invalidate all dst and recalculate the route. Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> --- include/net/dst.h | 7 +++++++ net/core/dst.c | 1 + net/core/sock.c | 4 ++-- net/ipv6/ip6_tunnel.c | 3 +-- 4 files changed, 11 insertions(+), 4 deletions(-)