On Fri, Dec 9, 2022 at 5:58 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> This patch-set consists of a series of bug fixes for register ID
> tracking in verifier.c:states_equal()/regsafe() functions:
> - for registers of type PTR_TO_MAP_{KEY,VALUE}, PTR_TO_PACKET[_META]
> the regsafe() should call check_ids() even if registers are
> byte-to-byte equal;
> - states_equal() must maintain idmap that covers all function frames
> in the state because functions like mark_ptr_or_null_regs() operate
> on all registers in the state;
> - regsafe() must compare spin lock ids for PTR_TO_MAP_VALUE registers.
>
> The last point covers issue reported by Kumar Kartikeya Dwivedi in [1],
> I borrowed the test commit from there.
> Note, that there is also an issue with register id tracking for
> scalars described here [2], it would be addressed separately.
>
Awesome set of patches, thanks for working on this! I left a few
comments and suggestions, please take a look, and if they do make
sense, consider sending follow up patches.
Let's really try to use asm() next time for selftests, though.
It would be awesome to somehow automatically move test_verifier's
tests to this test_progs-based embedded assembly way, but that
probably takes some Python hackery (awesome project for some curious
soul, for sure).
Anyways, back to the point I wanted to make. Given you've clearly
thought about all the ID checks a lot, consider checking refsafe()
(not regsafe()!) as well. I think we should do check_ids() there as
well. And you did all the preliminary work with making idmap
persistent across all frames. Just something to improve (and looks
straightforward, unlike many other things you've dealt with recently
;).
Anyways, great work, thanks!
> [1] https://lore.kernel.org/bpf/20221111202719.982118-1-memxor@xxxxxxxxx/
> [2] https://lore.kernel.org/bpf/20221128163442.280187-2-eddyz87@xxxxxxxxx/
>
> Eduard Zingerman (6):
> bpf: regsafe() must not skip check_ids()
> selftests/bpf: test cases for regsafe() bug skipping check_id()
> bpf: states_equal() must build idmap for all function frames
> selftests/bpf: verify states_equal() maintains idmap across all frames
> bpf: use check_ids() for active_lock comparison
> selftests/bpf: test case for relaxed prunning of active_lock.id
>
> Kumar Kartikeya Dwivedi (1):
> selftests/bpf: Add pruning test case for bpf_spin_lock
>
> include/linux/bpf_verifier.h | 4 +-
> kernel/bpf/verifier.c | 48 ++++----
> tools/testing/selftests/bpf/verifier/calls.c | 82 +++++++++++++
> .../bpf/verifier/direct_packet_access.c | 54 +++++++++
> .../selftests/bpf/verifier/spin_lock.c | 114 ++++++++++++++++++
> .../selftests/bpf/verifier/value_or_null.c | 49 ++++++++
> 6 files changed, 324 insertions(+), 27 deletions(-)
>
> --
> 2.34.1
>
