From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wolfgang Bumiller Subject: Re: [PATCH v2 net-next] neighbor: Improve garbage collection Date: Mon, 10 Dec 2018 09:55:44 +0100 Message-ID: <5c0e2a107caee_16112ae4fe0d10d42b@olga.notmuch> References: <20181207202457.24574-1-dsahern@kernel.org> <20181207.160347.151827133614133030.davem@davemloft.net> <54aa78ec-9a5b-b798-d93d-7daf2e6b1a96@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, roopa@cumulusnetworks.com, dsahern@gmail.com To: Eric Dumazet , David Miller , dsahern@kernel.org Return-path: Received: from proxmox-new.maurer-it.com ([212.186.127.180]:44866 "EHLO proxmox-new.maurer-it.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726667AbeLJJFN (ORCPT ); Mon, 10 Dec 2018 04:05:13 -0500 In-Reply-To: <54aa78ec-9a5b-b798-d93d-7daf2e6b1a96@gmail.com> Sender: netdev-owner@vger.kernel.org List-ID: Eric Dumazet wrote: > > > On 12/07/2018 04:03 PM, David Miller wrote: > > From: David Ahern > > Date: Fri, 7 Dec 2018 12:24:57 -0800 > > > >> From: David Ahern > >> > >> 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 > >> --- > >> 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