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

[PATCH net-next] 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] neigh: fix a possible leak issue of neigh entry

[Eric Dumazet]

On Wed, 2015-05-13 at 13:43 +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();
>  	}
>  }

Have you hit this condition ?

If yes, there is a bug elsewhere and we need to fix it, not trying to
recover.

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

[Ying Xue]

On 05/13/2015 02:20 PM, Eric Dumazet wrote:
> Have you hit this condition ?
> 

No, I just found the issue when I was reading the code.

> If yes, there is a bug elsewhere and we need to fix it, not trying to
> recover.
> 

Sorry, I don't know its relevant story, so can you please give some hints?

Thanks,
Ying

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

[Ying Xue]

On 05/13/2015 03:25 PM, Ying Xue wrote:
> On 05/13/2015 02:20 PM, Eric Dumazet wrote:
>> Have you hit this condition ?
>>
> 
> No, I just found the issue when I was reading the code.
> 
>> If yes, there is a bug elsewhere and we need to fix it, not trying to
>> recover.
>>

After a search, it's found your mentioned bug probably should be:

https://patchwork.ozlabs.org/patch/458112/

But I think the bug should be a different issue that the patch is to be
resolved. When the bug happened, neigh_add_timer() would take a reference on an
object that already had a refcount of zero, which means somebody already
decreased the refcount to zero before neigh_add_timer() was called. So, the bug
should be unrelated to neigh_add_timer() although the bug really needs to be
fixed. But the issue the patch tries to fix is wrong because it obviously
violates the generic usage of how to take an object refcount in the object's
timer handler as well as when its timer is started or deleted. Specially when a
neighbour's timer to be modified is in pending state, we should not take its
refcount, otherwise, the neighbour is leaked forever.

Therefore, the patch is needed for us even if your mentioned bug is not fixed.

Regards,
Ying

> 
> Sorry, I don't know its relevant story, so can you please give some hints?
> 
> Thanks,
> Ying
> 
> --
> 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
> 

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

[Eric Dumazet]

On Wed, 2015-05-13 at 18:32 +0800, Ying Xue wrote:
> On 05/13/2015 03:25 PM, Ying Xue wrote:
> > On 05/13/2015 02:20 PM, Eric Dumazet wrote:
> >> Have you hit this condition ?
> >>
> > 
> > No, I just found the issue when I was reading the code.
> > 
> >> If yes, there is a bug elsewhere and we need to fix it, not trying to
> >> recover.
> >>
> 
> After a search, it's found your mentioned bug probably should be:
> 
> https://patchwork.ozlabs.org/patch/458112/
> 
> But I think the bug should be a different issue that the patch is to be
> resolved. When the bug happened, neigh_add_timer() would take a reference on an
> object that already had a refcount of zero, which means somebody already
> decreased the refcount to zero before neigh_add_timer() was called. So, the bug
> should be unrelated to neigh_add_timer() although the bug really needs to be
> fixed. But the issue the patch tries to fix is wrong because it obviously
> violates the generic usage of how to take an object refcount in the object's
> timer handler as well as when its timer is started or deleted. Specially when a
> neighbour's timer to be modified is in pending state, we should not take its
> refcount, otherwise, the neighbour is leaked forever.
> 
> Therefore, the patch is needed for us even if your mentioned bug is not fixed.

Sorry, your patch is wrong.

As I said, if the issue is there, a different fix is needed.

Caller of the function must owns a refcount by definition.