[PATCH net-next 0/6] neigh cleanups and fixes

[PATCH net-next 0/6] neigh cleanups and fixes

[Ying Xue]

The series aims to clean up several mistakes especially to correct
the usage of neigh timer. But before doing them, we have to resolve
a crash issue discussed in the thread[1].

[1] http://www.spinics.net/lists/netdev/msg323883.html

Ying Xue (6):
  net: fix a double free issue for neighbour entry
  neigh: fix a possible leak issue of neigh entry
  neigh: don't delete neighbour time in neigh_destroy
  neigh: align the usage of neigh timer with one of sock timer
  neigh: neigh dead and timer variables should be protected under its
    lock
  neigh: use standard interface to modify timer

 net/core/neighbour.c |   39 +++++++++++++++++++++++----------------
 net/ipv4/ip_output.c |    2 +-
 2 files changed, 24 insertions(+), 17 deletions(-)

-- 
1.7.9.5

[PATCH net-next 1/6] net: fix a double free issue for neighbour entry

[Ying Xue]

Calling __ipv4_neigh_lookup_noref() inside rcu_read_lock_bh() can
guarantee that its searched neighbour entry is not freed before RCU
grace period, but it cannot ensure that its obtained neighbour will
be freed shortly. Exactly saying, it cannot prevent neigh_destroy()
from being executed on another context at the same time. For example,
if ip_finish_output2() continues to deliver a SKB with a neighbour
entry whose refcount is zero, neigh_add_timer() may be called in
neigh_resolve_output() subsequently. As a result, neigh_add_timer()
takes refcount on the neighbour that already had a refcount of zero.
When the neighbour refcount is put before the timer's handler is
exited, neigh_destroy() is called again, meaning crash happens at the
moment.

To prevent the issue from occurring, we must check whether the refcount
of a neighbour searched by __ipv4_neigh_lookup_noref() is decremented
to zero or not. If it's zero, we should create a new one.

Reported-by: Joern Engel <joern@logfs.org>
Signed-off-by: Ying Xue <ying.xue@windriver.com>
---
 net/ipv4/ip_output.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index 2acc5dc..580dd4d 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -200,7 +200,7 @@ static inline int ip_finish_output2(struct sock *sk, struct sk_buff *skb)
 	rcu_read_lock_bh();
 	nexthop = (__force u32) rt_nexthop(rt, ip_hdr(skb)->daddr);
 	neigh = __ipv4_neigh_lookup_noref(dev, nexthop);
-	if (unlikely(!neigh))
+	if (unlikely(!neigh || !atomic_read(&neigh->refcnt)))
 		neigh = __neigh_create(&arp_tbl, &nexthop, dev, false);
 	if (!IS_ERR(neigh)) {
 		int res = dst_neigh_output(dst, neigh, skb);
-- 
1.7.9.5

[PATCH net-next 2/6] neigh: fix a possible leak issue of neigh entry

[Ying Xue]

Once modifying a pending timer of a neighbour, it's insufficient to
post a warning message. Instead we should not take the neighbour's
reference count at the same time, otherwise, it causes an issue that
the neighbour cannot be freed forever.

Signed-off-by: Ying Xue <ying.xue@windriver.com>
---
 net/core/neighbour.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 3de6542..5595db3 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -164,10 +164,11 @@ static int neigh_forced_gc(struct neigh_table *tbl)
 
 static void neigh_add_timer(struct neighbour *n, unsigned long when)
 {
-	neigh_hold(n);
-	if (unlikely(mod_timer(&n->timer, when))) {
-		printk("NEIGH: BUG, double timer add, state is %x\n",
-		       n->nud_state);
+	if (likely(!mod_timer(&n->timer, when))) {
+		neigh_hold(n);
+	} else {
+		pr_warn("NEIGH: BUG, double timer add, state is %x\n",
+			n->nud_state);
 		dump_stack();
 	}
 }
-- 
1.7.9.5

Re: [PATCH net-next 2/6] neigh: fix a possible leak issue of neigh entry

[Eric Dumazet]

On Fri, 2015-05-15 at 14:55 +0800, Ying Xue wrote:
> Once modifying a pending timer of a neighbour, it's insufficient to
> post a warning message. Instead we should not take the neighbour's
> reference count at the same time, otherwise, it causes an issue that
> the neighbour cannot be freed forever.
> 
> Signed-off-by: Ying Xue <ying.xue@windriver.com>
> ---
>  net/core/neighbour.c |    9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 3de6542..5595db3 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -164,10 +164,11 @@ static int neigh_forced_gc(struct neigh_table *tbl)
>  
>  static void neigh_add_timer(struct neighbour *n, unsigned long when)
>  {
> -	neigh_hold(n);
> -	if (unlikely(mod_timer(&n->timer, when))) {
> -		printk("NEIGH: BUG, double timer add, state is %x\n",
> -		       n->nud_state);
> +	if (likely(!mod_timer(&n->timer, when))) {
> +		neigh_hold(n);
> +	} else {
> +		pr_warn("NEIGH: BUG, double timer add, state is %x\n",
> +			n->nud_state);
>  		dump_stack();
>  	}
>  }


NACK

Re: [PATCH net-next 2/6] neigh: fix a possible leak issue of neigh entry

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 15 May 2015 05:12:42 -0700

> On Fri, 2015-05-15 at 14:55 +0800, Ying Xue wrote:
>> Once modifying a pending timer of a neighbour, it's insufficient to
>> post a warning message. Instead we should not take the neighbour's
>> reference count at the same time, otherwise, it causes an issue that
>> the neighbour cannot be freed forever.
>> 
>> Signed-off-by: Ying Xue <ying.xue@windriver.com>
>> ---
>>  net/core/neighbour.c |    9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>> 
>> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
>> index 3de6542..5595db3 100644
>> --- a/net/core/neighbour.c
>> +++ b/net/core/neighbour.c
>> @@ -164,10 +164,11 @@ static int neigh_forced_gc(struct neigh_table *tbl)
>>  
>>  static void neigh_add_timer(struct neighbour *n, unsigned long when)
>>  {
>> -	neigh_hold(n);
>> -	if (unlikely(mod_timer(&n->timer, when))) {
>> -		printk("NEIGH: BUG, double timer add, state is %x\n",
>> -		       n->nud_state);
>> +	if (likely(!mod_timer(&n->timer, when))) {
>> +		neigh_hold(n);
>> +	} else {
>> +		pr_warn("NEIGH: BUG, double timer add, state is %x\n",
>> +			n->nud_state);
>>  		dump_stack();
>>  	}
>>  }
> 
> 
> NACK

Indeed, major NACK.  And you've been told this change is unacceptable
multiple times already, and you've been told exactly why as well.

Re: [PATCH net-next 2/6] neigh: fix a possible leak issue of neigh entry

[Ying Xue]

On 05/15/2015 11:39 PM, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Fri, 15 May 2015 05:12:42 -0700
> 
>> On Fri, 2015-05-15 at 14:55 +0800, Ying Xue wrote:
>>> Once modifying a pending timer of a neighbour, it's insufficient to
>>> post a warning message. Instead we should not take the neighbour's
>>> reference count at the same time, otherwise, it causes an issue that
>>> the neighbour cannot be freed forever.
>>>
>>> Signed-off-by: Ying Xue <ying.xue@windriver.com>
>>> ---
>>>  net/core/neighbour.c |    9 +++++----
>>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
>>> index 3de6542..5595db3 100644
>>> --- a/net/core/neighbour.c
>>> +++ b/net/core/neighbour.c
>>> @@ -164,10 +164,11 @@ static int neigh_forced_gc(struct neigh_table *tbl)
>>>  
>>>  static void neigh_add_timer(struct neighbour *n, unsigned long when)
>>>  {
>>> -	neigh_hold(n);
>>> -	if (unlikely(mod_timer(&n->timer, when))) {
>>> -		printk("NEIGH: BUG, double timer add, state is %x\n",
>>> -		       n->nud_state);
>>> +	if (likely(!mod_timer(&n->timer, when))) {
>>> +		neigh_hold(n);
>>> +	} else {
>>> +		pr_warn("NEIGH: BUG, double timer add, state is %x\n",
>>> +			n->nud_state);
>>>  		dump_stack();
>>>  	}
>>>  }
>>
>>
>> NACK
> 
> Indeed, major NACK.  And you've been told this change is unacceptable
> multiple times already, and you've been told exactly why as well.
> 

Eric, according to your below comments for the single patch,

http://www.spinics.net/lists/netdev/msg328828.html

I supposed the reason why you rejected it was because the issue of "neigh
use-after-free" was not resolved yet. More importantly, you did not explicitly
explain why the patch was wrong although I stated the problem the patch tries to
fix is different with the one discussed in the thread:

http://www.spinics.net/lists/netdev/msg323883.html

So, regarding my understanding for your comment, the patch can be acceptable if
the issue of "neigh use-after-free" is solved.

Since then I started to investigate its root cause and created the first patch
attempting to fix it. This is why I involve the patch into the series again.

If the issue of "neigh use-after-free" is fixed by the first patch although you
said the atomic_read() was not safe for us, is the patch still wrong? If it's
really wrong, can you please give more detailed explanation to help me
understand why the change is wrong and but why a similar timer usage in
sk_reset_timer() is not wrong?

Thanks,
Ying

Re: [PATCH net-next 2/6] neigh: fix a possible leak issue of neigh entry

[Eric Dumazet]

On Mon, 2015-05-18 at 11:24 +0800, Ying Xue wrote:

> If the issue of "neigh use-after-free" is fixed by the first patch although you
> said the atomic_read() was not safe for us, is the patch still wrong? If it's
> really wrong, can you please give more detailed explanation to help me
> understand why the change is wrong and but why a similar timer usage in
> sk_reset_timer() is not wrong?

Why do you believe sk_reset_timer() would be wrong ?

Difference between neigh_add_timer() and sk_reset_timer() is very
simple :

neigh_add_timer() must be called while the timer is not yet armed.

sk_reset_timer() can be called while timer is already armed.

You are changing neigh_add_timer() for no good reason, just because you
want it to be 'like sk_reset_timer()' ?

Re: [PATCH net-next 2/6] neigh: fix a possible leak issue of neigh entry

[Ying Xue]

On 05/18/2015 12:58 PM, Eric Dumazet wrote:
> On Mon, 2015-05-18 at 11:24 +0800, Ying Xue wrote:
> 
>> If the issue of "neigh use-after-free" is fixed by the first patch although you
>> said the atomic_read() was not safe for us, is the patch still wrong? If it's
>> really wrong, can you please give more detailed explanation to help me
>> understand why the change is wrong and but why a similar timer usage in
>> sk_reset_timer() is not wrong?
> 
> Why do you believe sk_reset_timer() would be wrong ?
> 

Sorry, I don't think sk_reset_timer() is wrong, instead I suppose
neigh_add_timer() is wrong :)

> Difference between neigh_add_timer() and sk_reset_timer() is very
> simple :
> 
> neigh_add_timer() must be called while the timer is not yet armed.
> 

In case that the caller of neigh_add_timer() attempts to modify an active timer
due to a bug or something wrong else, why not prevent neigh_add_timer() from
taking neigh refcnt beside posting a warning message?

So, exactly speaking, we cannot say neigh_add_timer() is completely wrong
regarding your mentioned above assumption that neigh timer is absolutely not
armed when neigh_add_timer() is called. But we can say its behaviour is not
designed very well. From this point, the patch seems still a bit valuable for us.

Regards,
Ying

> sk_reset_timer() can be called while timer is already armed.
> 
> You are changing neigh_add_timer() for no good reason, just because you
> want it to be 'like sk_reset_timer()' ?
> 
> 
> 
> 
> 

Re: [PATCH net-next 2/6] neigh: fix a possible leak issue of neigh entry

[Eric Dumazet]

On Mon, 2015-05-18 at 13:55 +0800, Ying Xue wrote:

> So, exactly speaking, we cannot say neigh_add_timer() is completely wrong
> regarding your mentioned above assumption that neigh timer is absolutely not
> armed when neigh_add_timer() is called. But we can say its behaviour is not
> designed very well. From this point, the patch seems still a bit valuable for us.

Its design is correct.

We have to fix the caller that does not respect the proper convention.

So please try to understand the current logic, instead of changing
everything, and adding another bunch of bugs.

[PATCH net-next 3/6] neigh: don't delete neighbour time in neigh_destroy

[Ying Xue]

The timer of neighbour should be stopped before neigh_destroy(),
otherwise, this is definitely a bug. Even if we do the deletion of
the timer in neigh_destroy(), it's meaningless as its neighbour
would be freed shortly. So it's better to add a warning message
when detectting timer is pending in neigh_destroy().

Signed-off-by: Ying Xue <ying.xue@windriver.com>
---
 net/core/neighbour.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 5595db3..433b617 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -689,8 +689,10 @@ void neigh_destroy(struct neighbour *neigh)
 		return;
 	}
 
-	if (neigh_del_timer(neigh))
-		pr_warn("Impossible event\n");
+	if (timer_pending(&neigh->timer)) {
+		pr_warn("Timer is pending, impossible event %p\n", neigh);
+		dump_stack();
+	}
 
 	write_lock_bh(&neigh->lock);
 	__skb_queue_purge(&neigh->arp_queue);
-- 
1.7.9.5

[PATCH net-next 4/6] neigh: align the usage of neigh timer with one of sock timer

[Ying Xue]

Keep the usage of neigh timer aligned with one of sock timer, making
the usage of neigh timer more understandable. To reach the goal, a new
function - __neigh_release() is introduced in neigh_del_timer(), and
it just decreases neigh refcount but doesn't call neigh_destroy().
This makes sense because neigh recount has been taken when its timer
is added, and it cannot hit zero when the timer is deleted.

Signed-off-by: Ying Xue <ying.xue@windriver.com>
---
 net/core/neighbour.c |   18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 433b617..8a438fc 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -173,14 +173,18 @@ static void neigh_add_timer(struct neighbour *n, unsigned long when)
 	}
 }
 
-static int neigh_del_timer(struct neighbour *n)
+/* Ungrab neigh in the context, which assumes that neigh refcnt
+ * cannot hit zero.
+ */
+static void __neigh_release(struct neighbour *n)
 {
-	if ((n->nud_state & NUD_IN_TIMER) &&
-	    del_timer(&n->timer)) {
-		neigh_release(n);
-		return 1;
-	}
-	return 0;
+	atomic_dec(&n->refcnt);
+}
+
+static void neigh_del_timer(struct neighbour *n)
+{
+	if ((n->nud_state & NUD_IN_TIMER) && del_timer(&n->timer))
+		__neigh_release(n);
 }
 
 static void pneigh_queue_purge(struct sk_buff_head *list)
-- 
1.7.9.5

[PATCH net-next 5/6] neigh: neigh dead and timer variables should be protected under its lock

[Ying Xue]

As neigh lock is not taken before its variables like dead and timer
are used in neigh_destroy(), this is unsafe for us. The commit allows
neigh lock to cover all area of using them.

Signed-off-by: Ying Xue <ying.xue@windriver.com>
---
 net/core/neighbour.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 8a438fc..67a0e42 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -687,9 +687,11 @@ void neigh_destroy(struct neighbour *neigh)
 
 	NEIGH_CACHE_STAT_INC(neigh->tbl, destroys);
 
+	write_lock_bh(&neigh->lock);
 	if (!neigh->dead) {
 		pr_warn("Destroying alive neighbour %p\n", neigh);
 		dump_stack();
+		write_unlock_bh(&neigh->lock);
 		return;
 	}
 
@@ -698,7 +700,6 @@ void neigh_destroy(struct neighbour *neigh)
 		dump_stack();
 	}
 
-	write_lock_bh(&neigh->lock);
 	__skb_queue_purge(&neigh->arp_queue);
 	write_unlock_bh(&neigh->lock);
 	neigh->arp_queue_len_bytes = 0;
-- 
1.7.9.5

[PATCH net-next 6/6] neigh: use standard interface to modify timer

[Ying Xue]

Should use standard interface - neigh_add_timer() to modify neigh
timer.

Signed-off-by: Ying Xue <ying.xue@windriver.com>
---
 net/core/neighbour.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 67a0e42..d7c2d36 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -939,8 +939,7 @@ static void neigh_timer_handler(unsigned long arg)
 	if (neigh->nud_state & NUD_IN_TIMER) {
 		if (time_before(next, jiffies + HZ/2))
 			next = jiffies + HZ/2;
-		if (!mod_timer(&neigh->timer, next))
-			neigh_hold(neigh);
+		neigh_add_timer(neigh, next);
 	}
 	if (neigh->nud_state & (NUD_INCOMPLETE | NUD_PROBE)) {
 		neigh_probe(neigh);
-- 
1.7.9.5

Re: [PATCH net-next 0/6] neigh cleanups and fixes

[Eric Dumazet]

On Fri, 2015-05-15 at 14:55 +0800, Ying Xue wrote:
> The series aims to clean up several mistakes especially to correct
> the usage of neigh timer. But before doing them, we have to resolve
> a crash issue discussed in the thread[1].
> 
> [1] http://www.spinics.net/lists/netdev/msg323883.html
> 
> Ying Xue (6):
>   net: fix a double free issue for neighbour entry
>   neigh: fix a possible leak issue of neigh entry
>   neigh: don't delete neighbour time in neigh_destroy
>   neigh: align the usage of neigh timer with one of sock timer
>   neigh: neigh dead and timer variables should be protected under its
>     lock
>   neigh: use standard interface to modify timer
> 
>  net/core/neighbour.c |   39 +++++++++++++++++++++++----------------
>  net/ipv4/ip_output.c |    2 +-
>  2 files changed, 24 insertions(+), 17 deletions(-)
> 

NACK for the whole series.

Your fixes make no sense, adding race conditions of all sorts.

atomic_read() by definition are unsafe.

Make a single and logical change, do not throw random patches like that,
ignoring feedback you already got.

Thanks.

Re: [PATCH net-next 0/6] neigh cleanups and fixes

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 15 May 2015 05:14:42 -0700

> Make a single and logical change, do not throw random patches like that,
> ignoring feedback you already got.

+1

Re: [PATCH net-next 0/6] neigh cleanups and fixes

[Ying Xue]

On 05/15/2015 08:14 PM, Eric Dumazet wrote:
> atomic_read() by definition are unsafe.

Can you please recommend which atomic interface can be safely used to check
whether neigh refcnt is zero in the first patch?

Thanks,
Ying