On Tue, 2022-12-13 at 16:34 -0800, Andrii Nakryiko wrote:
> 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
> ;).
Makes sense, I'll work on it.
>
> Anyways, great work, thanks!
Thank you.
>
> > [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
> >
