[PATCH net] inet: frags: rework rhashtable dismantle

syszbot found an interesting use-after-free [1] happening
while IPv4 fragment rhashtable was destroyed at netns dismantle.

While no insertions can possibly happen at the time a dismantling
netns is destroying this rhashtable, timers can still fire and
attempt to remove elements from this rhashtable.

This is forbidden, since rhashtable_free_and_destroy() has
no synchronization against concurrent inserts and deletes.

It seems we need to clean the rhashtable before destroying it.

[1]
BUG: KASAN: use-after-free in __read_once_size include/linux/compiler.h:188 [inline]
BUG: KASAN: use-after-free in rhashtable_last_table+0x216/0x240 lib/rhashtable.c:217
Read of size 8 at addr ffff88019a4c8840 by task kworker/0:4/8279

CPU: 0 PID: 8279 Comm: kworker/0:4 Not tainted 4.19.0-rc5+ #61
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Workqueue: events rht_deferred_worker
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x1c4/0x2b4 lib/dump_stack.c:113
 print_address_description.cold.8+0x9/0x1ff mm/kasan/report.c:256
 kasan_report_error mm/kasan/report.c:354 [inline]
 kasan_report.cold.9+0x242/0x309 mm/kasan/report.c:412
 __asan_report_load8_noabort+0x14/0x20 mm/kasan/report.c:433
 __read_once_size include/linux/compiler.h:188 [inline]
 rhashtable_last_table+0x216/0x240 lib/rhashtable.c:217
 rht_deferred_worker+0x157/0x1de0 lib/rhashtable.c:410
 process_one_work+0xc90/0x1b90 kernel/workqueue.c:2153
 worker_thread+0x17f/0x1390 kernel/workqueue.c:2296
 kthread+0x35a/0x420 kernel/kthread.c:246
 ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:413

Allocated by task 5:
 save_stack+0x43/0xd0 mm/kasan/kasan.c:448
 set_track mm/kasan/kasan.c:460 [inline]
 kasan_kmalloc+0xc7/0xe0 mm/kasan/kasan.c:553
 __do_kmalloc_node mm/slab.c:3682 [inline]
 __kmalloc_node+0x47/0x70 mm/slab.c:3689
 kmalloc_node include/linux/slab.h:555 [inline]
 kvmalloc_node+0xb9/0xf0 mm/util.c:423
 kvmalloc include/linux/mm.h:577 [inline]
 kvzalloc include/linux/mm.h:585 [inline]
 bucket_table_alloc+0x9a/0x4e0 lib/rhashtable.c:176
 rhashtable_rehash_alloc+0x73/0x100 lib/rhashtable.c:353
 rht_deferred_worker+0x278/0x1de0 lib/rhashtable.c:413
 process_one_work+0xc90/0x1b90 kernel/workqueue.c:2153
 worker_thread+0x17f/0x1390 kernel/workqueue.c:2296
 kthread+0x35a/0x420 kernel/kthread.c:246
 ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:413

Freed by task 8283:
 save_stack+0x43/0xd0 mm/kasan/kasan.c:448
 set_track mm/kasan/kasan.c:460 [inline]
 __kasan_slab_free+0x102/0x150 mm/kasan/kasan.c:521
 kasan_slab_free+0xe/0x10 mm/kasan/kasan.c:528
 __cache_free mm/slab.c:3498 [inline]
 kfree+0xcf/0x230 mm/slab.c:3813
 kvfree+0x61/0x70 mm/util.c:452
 bucket_table_free+0xda/0x250 lib/rhashtable.c:108
 rhashtable_free_and_destroy+0x152/0x900 lib/rhashtable.c:1163
 inet_frags_exit_net+0x3d/0x50 net/ipv4/inet_fragment.c:96
 ipv4_frags_exit_net+0x73/0x90 net/ipv4/ip_fragment.c:914
 ops_exit_list.isra.7+0xb0/0x160 net/core/net_namespace.c:153
 cleanup_net+0x555/0xb10 net/core/net_namespace.c:551
 process_one_work+0xc90/0x1b90 kernel/workqueue.c:2153
 worker_thread+0x17f/0x1390 kernel/workqueue.c:2296
 kthread+0x35a/0x420 kernel/kthread.c:246
 ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:413

The buggy address belongs to the object at ffff88019a4c8800
 which belongs to the cache kmalloc-16384 of size 16384
The buggy address is located 64 bytes inside of
 16384-byte region [ffff88019a4c8800, ffff88019a4cc800)
The buggy address belongs to the page:
page:ffffea0006693200 count:1 mapcount:0 mapping:ffff8801da802200 index:0x0 compound_mapcount: 0
flags: 0x2fffc0000008100(slab|head)
raw: 02fffc0000008100 ffffea0006685608 ffffea0006617c08 ffff8801da802200
raw: 0000000000000000 ffff88019a4c8800 0000000100000001 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
 ffff88019a4c8700: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 ffff88019a4c8780: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc

ffff88019a4c8800: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb

                                           ^
 ffff88019a4c8880: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 ffff88019a4c8900: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb

Fixes: 648700f76b03 ("inet: frags: use rhashtables for reassembly units")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: syzbot <redacted>
Cc: Thomas Graf <tgraf@suug.ch>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
---
 net/ipv4/inet_fragment.c | 55 ++++++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 22 deletions(-)

diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
index bcb11f3a27c0c34115af05034a5a20f57842eb0a..50d74a191ff14078bcb87c86640fe7dd342f9956 100644
--- a/net/ipv4/inet_fragment.c
+++ b/net/ipv4/inet_fragment.c

@@ -68,32 +68,43 @@ void inet_frags_fini(struct inet_frags *f)
 }
 EXPORT_SYMBOL(inet_frags_fini);
 
-static void inet_frags_free_cb(void *ptr, void *arg)
-{
-	struct inet_frag_queue *fq = ptr;
-
-	/* If we can not cancel the timer, it means this frag_queue
-	 * is already disappearing, we have nothing to do.
-	 * Otherwise, we own a refcount until the end of this function.
-	 */
-	if (!del_timer(&fq->timer))
-		return;
-
-	spin_lock_bh(&fq->lock);
-	if (!(fq->flags & INET_FRAG_COMPLETE)) {
-		fq->flags |= INET_FRAG_COMPLETE;
-		refcount_dec(&fq->refcnt);
-	}
-	spin_unlock_bh(&fq->lock);
-
-	inet_frag_put(fq);
-}
-
 void inet_frags_exit_net(struct netns_frags *nf)
 {
+	struct rhashtable_iter hti;
+	struct inet_frag_queue *fq;
+
+	/* Since we want to cleanup the hashtable, make sure that
+	 * we wont trigger an automatic shrinking while in our
+	 * rhashtable_walk_next() loop.
+	 * Also make sure that no resize is in progress.
+	 */
 	nf->high_thresh = 0; /* prevent creation of new frags */
+	nf->rhashtable.p.automatic_shrinking = false;
+	cancel_work_sync(&nf->rhashtable.run_work);
 
-	rhashtable_free_and_destroy(&nf->rhashtable, inet_frags_free_cb, NULL);
+	rhashtable_walk_enter(&nf->rhashtable, &hti);
+	rhashtable_walk_start(&hti);
+	while ((fq = rhashtable_walk_next(&hti)) != NULL) {
+		if (IS_ERR(fq)) /* should not happen */
+			break;
+		if (!del_timer_sync(&fq->timer))
+			continue;
+
+		spin_lock_bh(&fq->lock);
+		inet_frag_kill(fq);
+		spin_unlock_bh(&fq->lock);
+
+		inet_frag_put(fq);
+		if (need_resched()) {
+			rhashtable_walk_stop(&hti);
+			cond_resched();
+			rhashtable_walk_start(&hti);
+		}
+	}
+	rhashtable_walk_stop(&hti);
+	rhashtable_walk_exit(&hti);
+
+	rhashtable_destroy(&nf->rhashtable);
 }
 EXPORT_SYMBOL(inet_frags_exit_net);
 

-- 
2.19.0.605.g01d371f741-goog

On Mon, Oct 01, 2018 at 10:58:21AM -0700, Eric Dumazet wrote:

 void inet_frags_exit_net(struct netns_frags *nf)
 {
+	struct rhashtable_iter hti;
+	struct inet_frag_queue *fq;
+
+	/* Since we want to cleanup the hashtable, make sure that
+	 * we wont trigger an automatic shrinking while in our
+	 * rhashtable_walk_next() loop.
+	 * Also make sure that no resize is in progress.
+	 */
 	nf->high_thresh = 0; /* prevent creation of new frags */
+	nf->rhashtable.p.automatic_shrinking = false;
+	cancel_work_sync(&nf->rhashtable.run_work);
 
-	rhashtable_free_and_destroy(&nf->rhashtable, inet_frags_free_cb, NULL);
+	rhashtable_walk_enter(&nf->rhashtable, &hti);
+	rhashtable_walk_start(&hti);
+	while ((fq = rhashtable_walk_next(&hti)) != NULL) {
+		if (IS_ERR(fq)) /* should not happen */
+			break;
+		if (!del_timer_sync(&fq->timer))
+			continue;
+
+		spin_lock_bh(&fq->lock);
+		inet_frag_kill(fq);
+		spin_unlock_bh(&fq->lock);
+
+		inet_frag_put(fq);
+		if (need_resched()) {
+			rhashtable_walk_stop(&hti);
+			cond_resched();
+			rhashtable_walk_start(&hti);
+		}
+	}

The walk interface was designed to handle read-only iteration
through the hash table.  While this probably works since the
actual freeing is delayed by RCU, it seems to be rather fragile.

How about using the dead flag but instead of putting it in the
rhashtable put it in netns_frags and have the timers check on that
before calling rhashtable_remove?

Cheers,
-- 
Email: Herbert Xu [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

On Mon, Oct 1, 2018 at 5:58 PM Herbert Xu [off-list ref] wrote:

The walk interface was designed to handle read-only iteration
through the hash table.  While this probably works since the
actual freeing is delayed by RCU, it seems to be rather fragile.

How about using the dead flag but instead of putting it in the
rhashtable put it in netns_frags and have the timers check on that
before calling rhashtable_remove?

Sure, I will send a new version, thanks.