| 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
| ||
|
Message-ID: <7e8e835d-ef94-80e6-e98e-f8ed4a8fc78c@huaweicloud.com>
Date: Tue, 5 Nov 2024 10:49:44 +0800
From: Hou Tao <houtao@...weicloud.com>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
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
Hi,
On 11/5/2024 12:28 AM, Sebastian Andrzej Siewior wrote:
> 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?
I think moving free_htab_elem() after htab_unlock_bucket() is OK. But
the fix below is not enough, and there is some corn cases for
pre-allocated element . I had written a patch for the problem a few day
ago because the problem can be easily reproduced by running test_maps. I
am also writing a selftest patch for it. I could post the patch and the
selftest patch if you are OK with it.
>
> 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:
>
> .
Powered by blists - more mailing lists