Re: [PATCH v2 net-next] neighbor: Improve garbage collection

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

Eric Dumazet wrote:
> 
> 
> On 12/07/2018 04:03 PM, David Miller wrote:
> > From: David Ahern <dsahern@kernel.org>
> > Date: Fri,  7 Dec 2018 12:24:57 -0800
> > 
> >> From: David Ahern <dsahern@gmail.com>
> >>
> >> The existing garbage collection algorithm has a number of problems:
> >  ...
> >> This patch addresses these problems as follows:
> >>
> >> 1. Use of a separate list_head to track entries that can be garbage
> >>    collected along with a separate counter. PERMANENT entries are not
> >>    added to this list.
> >>
> >>    The gc_thresh parameters are only compared to the new counter, not the
> >>    total entries in the table. The forced_gc function is updated to only
> >>    walk this new gc_list looking for entries to evict.
> >>
> >> 2. Entries are added to the list head at the tail and removed from the
> >>    front.
> >>
> >> 3. Entries are only evicted if they were last updated more than 5 seconds
> >>    ago, adhering to the original intent of gc_thresh2.
> >>
> >> 4. Forced gc is stopped once the number of gc_entries drops below
> >>    gc_thresh2.
> >>
> >> 5. Since gc checks do not apply to PERMANENT entries, gc levels are skipped
> >>    when allocating a new neighbor for a PERMANENT entry. By extension this
> >>    means there are no explicit limits on the number of PERMANENT entries
> >>    that can be created, but this is no different than FIB entries or FDB
> >>    entries.
> >>
> >> Signed-off-by: David Ahern <dsahern@gmail.com>
> >> ---
> >> v2
> >> - remove on_gc_list boolean in favor of !list_empty
> >> - fix neigh_alloc to add new entry to tail of list_head
> > 
> > Again, looks great, applied.
> > 
> 
> What protects gc_list linkage ?
> 
> We can not use list_del_init(&n->gc_list);                    or 
>                list_add_tail(&n->gc_list, &n->tbl->gc_list);
> 
> if tbl->lock is not held.
> 
> It seems to me this patch needs more care.
> 

This seems to be problematic in neigh_create() which uses neigh_alloc()
before grabbing the lock (which uses list_add_tail()), as seen in the
syzbot traces:
-> KASAN: use-after-free Read in ___neigh_create
   syzbot+e4d42eb35f6a27b0a628@syzkaller.appspotmail.com
-> KASAN: slab-out-of-bounds Read in ___neigh_create
   syzbot+3ddead5619658537909b@syzkaller.appspotmail.com
-> BUG: corrupted list in ___neigh_create
   syzbot+b354d1fb59091ea73c37@syzkaller.appspotmail.com