Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BPF control flow graph and precision backtrack fixes #647

Closed
wants to merge 3 commits into from

Conversation

danielocfb
Copy link
Owner

Pull request for series with
subject: BPF control flow graph and precision backtrack fixes
version: 1
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=799796

@danielocfb
Copy link
Owner Author

Upstream branch: b4e59c1
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=799796
version: 1

@danielocfb
Copy link
Owner Author

Upstream branch: b4e59c1
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=799796
version: 1

@danielocfb danielocfb force-pushed the series/799796=>bpf-next branch from 36d1d6f to b7ad91e Compare November 9, 2023 17:58
@danielocfb danielocfb force-pushed the bpf-next_base branch 2 times, most recently from 902178a to bc1b833 Compare November 9, 2023 18:41
@danielocfb
Copy link
Owner Author

Upstream branch: 3815f89
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=799796
version: 1

@danielocfb danielocfb force-pushed the series/799796=>bpf-next branch from b7ad91e to 0afcd99 Compare November 9, 2023 18:42
@danielocfb danielocfb force-pushed the bpf-next_base branch 2 times, most recently from 0fbe9f3 to b0d20d2 Compare November 9, 2023 19:02
@danielocfb
Copy link
Owner Author

Upstream branch: 6f101db
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=799796
version: 1

@danielocfb danielocfb force-pushed the series/799796=>bpf-next branch from 0afcd99 to 0cf98d5 Compare November 9, 2023 19:03
@danielocfb danielocfb force-pushed the bpf-next_base branch 2 times, most recently from 1e02037 to a98cbca Compare November 9, 2023 19:19
@danielocfb
Copy link
Owner Author

Upstream branch: e80742d
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=799796
version: 1

@danielocfb
Copy link
Owner Author

Upstream branch: e80742d
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=799796
version: 1

@danielocfb danielocfb force-pushed the series/799796=>bpf-next branch from 64221bf to 9a4beb0 Compare November 9, 2023 20:39
@danielocfb
Copy link
Owner Author

Upstream branch: e80742d
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=799796
version: 1

@danielocfb
Copy link
Owner Author

Upstream branch: e80742d
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=799796
version: 1

ldimm64 instructions are 16-byte long, and so have to be handled
appropriately in check_cfg(), just like the rest of BPF verifier does.

This has implications in three places:
  - when determining next instruction for non-jump instructions;
  - when determining next instruction for callback address ldimm64
    instructions (in visit_func_call_insn());
  - when checking for unreachable instructions, where second half of
    ldimm64 is expected to be unreachable;

We take this also as an opportunity to report jump into the middle of
ldimm64. And adjust few test_verifier tests accordingly.

Acked-by: Eduard Zingerman <[email protected]>
Reported-by: Hao Sun <[email protected]>
Fixes: 475fb78 ("bpf: verifier (add branch/goto checks)")
Signed-off-by: Andrii Nakryiko <[email protected]>
Fix an edge case in __mark_chain_precision() which prematurely stops
backtracking instructions in a state if it happens that state's first
and last instruction indexes are the same. This situations doesn't
necessarily mean that there were no instructions simulated in a state,
but rather that we starting from the instruction, jumped around a bit,
and then ended up at the same instruction before checkpointing or
marking precision.

To distinguish between these two possible situations, we need to consult
jump history. If it's empty or contain a single record "bridging" parent
state and first instruction of processed state, then we indeed
backtracked all instructions in this state. But if history is not empty,
we are definitely not done yet.

Move this logic inside get_prev_insn_idx() to contain it more nicely.
Use -ENOENT return code to denote "we are out of instructions"
situation.

This bug was exposed by verifier_loop1.c's bounded_recursion subtest, once
the next fix in this patch set is applied.

Acked-by: Eduard Zingerman <[email protected]>
Fixes: b5dc016 ("bpf: precise scalar_value tracking")
Signed-off-by: Andrii Nakryiko <[email protected]>
Add a dedicated selftests to try to set up conditions to have a state
with same first and last instruction index, but it actually is a loop
3->4->1->2->3. This confuses mark_chain_precision() if verifier doesn't
take into account jump history.

Signed-off-by: Andrii Nakryiko <[email protected]>
@danielocfb
Copy link
Owner Author

Upstream branch: 155addf
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=800079
version: 2

@danielocfb
Copy link
Owner Author

At least one diff in series https://patchwork.kernel.org/project/netdevbpf/list/?series=800079 irrelevant now. Closing PR.

@danielocfb danielocfb added accepted and removed new labels Nov 10, 2023
@danielocfb danielocfb closed this Nov 10, 2023
@danielocfb danielocfb deleted the series/799796=>bpf-next branch November 10, 2023 05:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants