While digging into related code for spin lock correctness, the pruning related checks for PTR_TO_MAP_VALUE in regsafe looked suspect. The reg->id is never compared (which is preserved only in case the map value has a bpf_spin_lock) between rold and rcur. Turns out this allows unlocking a bpf_spin_unlock for a reg with a different reg->id, i.e. not pairing spin lock calls correctly. A regression test is included in patch 2. However, looking more closely, it seems to me that the logic of check_ids is broken as well. Edward, given you introduced the idmap, can you provide a little more historical context on what the idea behind check_ids was, since it seems to be doing the wrong thing as far as I understood things. I think we need to compare the ids directly everywhere. For instance, when we have a case like below: r0 = bpf_map_lookup_elem(&map, ...); // id=1 r6 = r0; r0 = bpf_map_lookup_elem(&map, ...); // id=2 r7 = r0; bpf_spin_lock(r1=r6); if (cond) r6 = r7; p: bpf_spin_unlock(r1=r6); Only r6 differs between old and cur at pruning point p. If we did the check in patch 1 using check_ids, it would end up seeing that no mapping exists for old id so it will set up mapping of 1 to 2, and then return true. I think similar problems exist elsewhere where after establishing the first mapping, if there are no more lookups into idmap, it will just pass the states_equal check. We are already inconsistent in other places, since if it made sense states_equal should have been using check_ids logic for active_spin_lock checks (but it's not a bug in that case, just more conservative). If we agree it needs fixing, I will send a separate fix removing its use from regsafe. For now this patch should address the bpf_spin_lock issue. Kumar Kartikeya Dwivedi (2): bpf: Fix state pruning check for PTR_TO_MAP_VALUE selftests/bpf: Add pruning test case for bpf_spin_lock kernel/bpf/verifier.c | 33 +++++++++++++--- .../selftests/bpf/verifier/spin_lock.c | 39 +++++++++++++++++++ 2 files changed, 66 insertions(+), 6 deletions(-) base-commit: 5704bc7e8991164b14efb748b5afa0715c25fac3 -- 2.38.1
