linux-kernel - Re: [syzbot] [bpf?] WARNING: locking bug in bpf_map

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241104162832.OQvrGDiP@linutronix.de>
Date: Mon, 4 Nov 2024 17:28:32 +0100
From: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To: syzbot <syzbot+d2adb332fe371b0595e3@...kaller.appspotmail.com>
Cc: andrii@...nel.org, ast@...nel.org, boqun.feng@...il.com,
	bpf@...r.kernel.org, daniel@...earbox.net, eadavis@...com,
	eddyz87@...il.com, haoluo@...gle.com, john.fastabend@...il.com,
	jolsa@...nel.org, kpsingh@...nel.org, linux-kernel@...r.kernel.org,
	longman@...hat.com, martin.lau@...ux.dev, sdf@...ichev.me,
	song@...nel.org, syzkaller-bugs@...glegroups.com,
	yonghong.song@...ux.dev, tglx@...utronix.de
Subject: Re: [syzbot] [bpf?] WARNING: locking bug in bpf_map_put

On 2024-11-03 18:29:04 [-0800], syzbot wrote:
> syzbot has bisected this issue to:
> 
> commit 560af5dc839eef08a273908f390cfefefb82aa04
> Author: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> Date:   Wed Oct 9 15:45:03 2024 +0000
> 
>     lockdep: Enable PROVE_RAW_LOCK_NESTING with PROVE_LOCKING.
> 
> bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=122a4740580000
> start commit:   f9f24ca362a4 Add linux-next specific files for 20241031
> git tree:       linux-next
> final oops:     https://syzkaller.appspot.com/x/report.txt?x=112a4740580000
> console output: https://syzkaller.appspot.com/x/log.txt?x=162a4740580000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=328572ed4d152be9
> dashboard link: https://syzkaller.appspot.com/bug?extid=d2adb332fe371b0595e3
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=174432a7980000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=14ffe55f980000
> 
> Reported-by: syzbot+d2adb332fe371b0595e3@...kaller.appspotmail.com
> Fixes: 560af5dc839e ("lockdep: Enable PROVE_RAW_LOCK_NESTING with PROVE_LOCKING.")
> 
> For information about bisection process see: https://goo.gl/tpsmEJ#bisection

This is due to raw_spinlock_t in bucket::lock and the acquired
spinlock_t underneath. Would it would to move free part outside of the
locked section?

diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
index b14b87463ee04..1d8d09fdd2da5 100644
--- a/kernel/bpf/hashtab.c
+++ b/kernel/bpf/hashtab.c
@@ -824,13 +824,14 @@ static bool htab_lru_map_delete_node(void *arg, struct bpf_lru_node *node)
 	hlist_nulls_for_each_entry_rcu(l, n, head, hash_node)
 		if (l == tgt_l) {
 			hlist_nulls_del_rcu(&l->hash_node);
-			check_and_free_fields(htab, l);
 			bpf_map_dec_elem_count(&htab->map);
 			break;
 		}
 
 	htab_unlock_bucket(htab, b, tgt_l->hash, flags);
 
+	if (l == tgt_l)
+		check_and_free_fields(htab, l);
 	return l == tgt_l;
 }
 
@@ -1181,14 +1182,18 @@ static long htab_map_update_elem(struct bpf_map *map, void *key, void *value,
 	 * concurrent search will find it before old elem
 	 */
 	hlist_nulls_add_head_rcu(&l_new->hash_node, head);
-	if (l_old) {
+	if (l_old)
 		hlist_nulls_del_rcu(&l_old->hash_node);
+	htab_unlock_bucket(htab, b, hash, flags);
+
+	if (l_old) {
 		if (!htab_is_prealloc(htab))
 			free_htab_elem(htab, l_old);
 		else
 			check_and_free_fields(htab, l_old);
 	}
-	ret = 0;
+	return 0;
+
 err:
 	htab_unlock_bucket(htab, b, hash, flags);
 	return ret;
@@ -1433,14 +1438,15 @@ static long htab_map_delete_elem(struct bpf_map *map, void *key)
 
 	l = lookup_elem_raw(head, hash, key, key_size);
 
-	if (l) {
+	if (l)
 		hlist_nulls_del_rcu(&l->hash_node);
-		free_htab_elem(htab, l);
-	} else {
+	else
 		ret = -ENOENT;
-	}
 
 	htab_unlock_bucket(htab, b, hash, flags);
+
+	if (l)
+		free_htab_elem(htab, l);
 	return ret;
 }
 
@@ -1647,14 +1653,16 @@ static int __htab_map_lookup_and_delete_elem(struct bpf_map *map, void *key,
 		}
 
 		hlist_nulls_del_rcu(&l->hash_node);
-		if (!is_lru_map)
-			free_htab_elem(htab, l);
 	}
 
 	htab_unlock_bucket(htab, b, hash, bflags);
 
-	if (is_lru_map && l)
-		htab_lru_push_free(htab, l);
+	if (l) {
+		if (is_lru_map)
+			htab_lru_push_free(htab, l);
+		else
+			free_htab_elem(htab, l);
+	}
 
 	return ret;
 }
@@ -1851,15 +1859,12 @@ __htab_map_lookup_and_delete_batch(struct bpf_map *map,
 
 			/* bpf_lru_push_free() will acquire lru_lock, which
 			 * may cause deadlock. See comments in function
-			 * prealloc_lru_pop(). Let us do bpf_lru_push_free()
-			 * after releasing the bucket lock.
+			 * prealloc_lru_pop(). htab_lru_push_free() may allocate
+			 * sleeping locks. Let us do bpf_lru_push_free() after
+			 * releasing the bucket lock.
 			 */
-			if (is_lru_map) {
-				l->batch_flink = node_to_free;
-				node_to_free = l;
-			} else {
-				free_htab_elem(htab, l);
-			}
+			l->batch_flink = node_to_free;
+			node_to_free = l;
 		}
 		dst_key += key_size;
 		dst_val += value_size;
@@ -1871,7 +1876,10 @@ __htab_map_lookup_and_delete_batch(struct bpf_map *map,
 	while (node_to_free) {
 		l = node_to_free;
 		node_to_free = node_to_free->batch_flink;
-		htab_lru_push_free(htab, l);
+		if (is_lru_map)
+			htab_lru_push_free(htab, l);
+		else
+			free_htab_elem(htab, l);
 	}
 
 next_batch: