RE: neigh use-after-free

[Alexei Potashnik <alexei@purestorage.com>]

> -----Original Message-----
> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
>
> On Fri, 2015-04-03 at 13:52 -0700, Alexei Potashnik wrote:
> > Would this be an appropriate solution:
> >
> > diff --git a/net/core/neighbour.c b/net/core/neighbour.c index
> > b49e8ba..38265f2 100644
> > --- a/net/core/neighbour.c
> > +++ b/net/core/neighbour.c
> > @@ -168,7 +168,8 @@ static int neigh_forced_gc(struct neigh_table
> > *tbl)
> >
> >  static void neigh_add_timer(struct neighbour *n, unsigned long when)
> > {
> > -       neigh_hold(n);
> > +       if (!atomic_inc_not_zero(&n->refcnt))
> > +               return;
> >         if (unlikely(mod_timer(&n->timer, when))) {
> >                 printk("NEIGH: BUG, double timer add, state is %x\n",
> >                        n->nud_state);
>
> This is a very ugly hack. Please find root cause.
>
> The correct way to implement refcount on a timer is :
>
> if (!mod_timer(&n->timer, when))
> 	neigh_hold(&n->refcnt);
>
> And current code seems to do that (It dumps a printk() and stack trace
> otherwise)

The printk() and stack trace don't get printed because this is a different
case --
it’s not double timer add, but rather add of the timer after neighbor object
is deleted.

> If refcnt is 0 at this point, it means there is another bug, and we should
> fix it
> instead of trying to work around it.

I agree. And it seems to be clear what code does it:

ip_finish_output2
+-> __ipv4_neigh_lookup_noref

Lookup name is clearly indicating that noref is intentional. What's not
clear is why.
The change seems to have originated in

commit a263b3093641fb1ec377582c90986a7fd0625184
Author: David S. Miller <davem@davemloft.net>
Date:   Mon Jul 2 02:02:15 2012

    ipv4: Make neigh lookups directly in output packet path.

    Do not use the dst cached neigh, we'll be getting rid of that.

    Signed-off-by: David S. Miller <davem@davemloft.net>

Before that neigh object came from dst_get_neighbour_noref(), which used to
return dst->_neighbour. Now, if dst->_neighbour used own a reference, than
the
above commit has introduced the bug. Otherwise, the issue has always been
there
and, perhaps, David Miller can explain why it was deemed safe to operate on
neigh
object here without extra ref.

Alexei