Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

2015-09-02 Thread Eric Dumazet
On Wed, 2015-09-02 at 16:10 -0700, Martin KaFai Lau wrote: > On Wed, Sep 02, 2015 at 03:48:57PM -0700, Eric Dumazet wrote: > > On Wed, 2015-09-02 at 14:52 -0700, Martin KaFai Lau wrote: > > > On Wed, Sep 02, 2015 at 02:30:45PM -0700, Eric Dumazet wrote: > > > > Object cannot be freed until all cpus

Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

2015-09-02 Thread David Miller
From: Martin KaFai Lau Date: Wed, 2 Sep 2015 16:10:31 -0700 > On Wed, Sep 02, 2015 at 03:48:57PM -0700, Eric Dumazet wrote: >> dst_free() is called after RCU grace period, in the case you are >> interested in. >> >> Look at dst_rcu_free() and rt_free() > Yes for IPv4 FIB > > Not for IPv6 FIB. F.

Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

2015-09-02 Thread Martin KaFai Lau
On Wed, Sep 02, 2015 at 03:48:57PM -0700, Eric Dumazet wrote: > On Wed, 2015-09-02 at 14:52 -0700, Martin KaFai Lau wrote: > > On Wed, Sep 02, 2015 at 02:30:45PM -0700, Eric Dumazet wrote: > > > Object cannot be freed until all cpus have exited their RCU sections. > > You meant the dst_destroy() he

Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

2015-09-02 Thread David Miller
From: Eric Dumazet Date: Wed, 02 Sep 2015 15:48:57 -0700 > On Wed, 2015-09-02 at 14:52 -0700, Martin KaFai Lau wrote: >> On Wed, Sep 02, 2015 at 02:30:45PM -0700, Eric Dumazet wrote: >> > Object cannot be freed until all cpus have exited their RCU sections. >> You meant the dst_destroy() here wil

Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

2015-09-02 Thread Eric Dumazet
On Wed, 2015-09-02 at 14:52 -0700, Martin KaFai Lau wrote: > On Wed, Sep 02, 2015 at 02:30:45PM -0700, Eric Dumazet wrote: > > Object cannot be freed until all cpus have exited their RCU sections. > You meant the dst_destroy() here will wait for all cpus exited their RCU > sections? > > static in

Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

2015-09-02 Thread Martin KaFai Lau
On Wed, Sep 02, 2015 at 02:30:45PM -0700, Eric Dumazet wrote: > Object cannot be freed until all cpus have exited their RCU sections. You meant the dst_destroy() here will wait for all cpus exited their RCU sections? static inline void dst_free(struct dst_entry *dst) { if (dst->obsolete >

Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

2015-09-02 Thread Eric Dumazet
On Wed, 2015-09-02 at 13:58 -0700, Martin KaFai Lau wrote: > On Tue, Sep 01, 2015 at 01:14:20PM -0700, Eric Dumazet wrote: > > > 2. Use a spinlock to protect the dst_cache operations > > > > Well, a seqlock would be better : No need for an atomic operation in > > fast path. > > > seqlock can ensure

Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

2015-09-02 Thread Martin KaFai Lau
On Tue, Sep 01, 2015 at 01:14:20PM -0700, Eric Dumazet wrote: > > 2. Use a spinlock to protect the dst_cache operations > > Well, a seqlock would be better : No need for an atomic operation in > fast path. > seqlock can ensure consistency between idst->dst and idst->cookie. However, IPv6 dst destru

Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

2015-09-01 Thread Martin KaFai Lau
On Tue, Sep 01, 2015 at 01:14:20PM -0700, Eric Dumazet wrote: > On Tue, 2015-09-01 at 11:55 -0700, Martin KaFai Lau wrote: > > Problems in the current dst_entry cache in the ip6_tunnel: > > > > 1. ip6_tnl_dst_set is racy. There is no lock to protect it: > >- One major problem is that the dst r

Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

2015-09-01 Thread Martin KaFai Lau
On Tue, Sep 01, 2015 at 05:42:00PM -0700, Martin KaFai Lau wrote: > I look a closer look at dst_rcu_free() and your commit pointers. I can see > your point > for DST_NOCACHE. > > However, dst_free() for not DST_NOCACHE is still an issue, I think. oops. Ignore this email and continue the discussio

Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

2015-09-01 Thread Martin KaFai Lau
On Tue, Sep 01, 2015 at 05:31:44PM -0700, Martin KaFai Lau wrote: > On Tue, Sep 01, 2015 at 03:38:36PM -0700, Eric Dumazet wrote: > > On Tue, 2015-09-01 at 15:25 -0700, Martin KaFai Lau wrote: > > > On Tue, Sep 01, 2015 at 02:26:58PM -0700, Eric Dumazet wrote: > > > > On Tue, 2015-09-01 at 13:55 -0

Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

2015-09-01 Thread Martin KaFai Lau
On Tue, Sep 01, 2015 at 03:38:36PM -0700, Eric Dumazet wrote: > On Tue, 2015-09-01 at 15:25 -0700, Martin KaFai Lau wrote: > > On Tue, Sep 01, 2015 at 02:26:58PM -0700, Eric Dumazet wrote: > > > On Tue, 2015-09-01 at 13:55 -0700, Martin KaFai Lau wrote: > > > > On Tue, Sep 01, 2015 at 01:14:20PM -0

Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

2015-09-01 Thread Eric Dumazet
On Tue, 2015-09-01 at 15:25 -0700, Martin KaFai Lau wrote: > On Tue, Sep 01, 2015 at 02:26:58PM -0700, Eric Dumazet wrote: > > On Tue, 2015-09-01 at 13:55 -0700, Martin KaFai Lau wrote: > > > On Tue, Sep 01, 2015 at 01:14:20PM -0700, Eric Dumazet wrote: > > > > It should not be a problem. refcnt is

Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

2015-09-01 Thread Martin KaFai Lau
On Tue, Sep 01, 2015 at 02:26:58PM -0700, Eric Dumazet wrote: > On Tue, 2015-09-01 at 13:55 -0700, Martin KaFai Lau wrote: > > On Tue, Sep 01, 2015 at 01:14:20PM -0700, Eric Dumazet wrote: > > > It should not be a problem. refcnt is taken when/if necessary (skb > > > queued on a qdisc for example)

Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

2015-09-01 Thread Eric Dumazet
On Tue, 2015-09-01 at 13:55 -0700, Martin KaFai Lau wrote: > On Tue, Sep 01, 2015 at 01:14:20PM -0700, Eric Dumazet wrote: > > It should not be a problem. refcnt is taken when/if necessary (skb > > queued on a qdisc for example) > > > > We have other uses of skb_dst_set_noref() > > > > Please descr

Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

2015-09-01 Thread Martin KaFai Lau
On Tue, Sep 01, 2015 at 01:14:20PM -0700, Eric Dumazet wrote: > It should not be a problem. refcnt is taken when/if necessary (skb > queued on a qdisc for example) > > We have other uses of skb_dst_set_noref() > > Please describe the problem ? The current ip6_tnl_dst_get() does not take the dst ref

Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

2015-09-01 Thread Eric Dumazet
On Tue, 2015-09-01 at 11:55 -0700, Martin KaFai Lau wrote: > Problems in the current dst_entry cache in the ip6_tunnel: > > 1. ip6_tnl_dst_set is racy. There is no lock to protect it: >- One major problem is that the dst refcnt gets messed up. F.e. > the same dst_cache can be released mu

[PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

2015-09-01 Thread Martin KaFai Lau
Problems in the current dst_entry cache in the ip6_tunnel: 1. ip6_tnl_dst_set is racy. There is no lock to protect it: - One major problem is that the dst refcnt gets messed up. F.e. the same dst_cache can be released multiple times and then triggering the infamous dst refcnt < 0 war