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

pkg/fuzzer: use layered queues #4762

Merged
merged 13 commits into from May 16, 2024

Conversation

a-nogikh
Copy link
Collaborator

@a-nogikh a-nogikh commented May 3, 2024

Instead of relying on a fuzzer-internal priority queue, utilize stackable layers of request-generating steps.

Move the functionality to a separate pkg/fuzzer/queue package.

The pkg/fuzzer/queue package can be reused to add extra processing layers on top of the fuzzing and to combine machine checking and fuzzing execution pipelines.

@a-nogikh a-nogikh force-pushed the features/common-exec-queue branch 2 times, most recently from 7a1b19c to 20a7018 Compare May 3, 2024 16:50
@a-nogikh a-nogikh changed the title WIP: pkg/fuzzer: use queue layers WIP: pkg/fuzzer: use layered queues May 3, 2024
@dvyukov
Copy link
Collaborator

dvyukov commented May 6, 2024

While working on moving code from the target to the host, I see lots of utilities that duplicate code and pain to update.
4 more use cases that would benefit from moving into syz-manager:

  • syz-crush: repeatedly execute fixed set of programs on all VMs
  • syz-runtest: execute a fixed set of programs, print results, exit
  • reproducer testing in syz-ci: run syz/C repro for fixed amount of time and exit
  • image testing in syz-ci: simply exit after check

All of these are similar: do something else (rather than pkg/fuzzer) after the VM checking procedure.
It would nice if the new architecture would support these use-cases natively.

Copy link
Collaborator

@dvyukov dvyukov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this allow to create the fuzzer lazily after the VM checking procedure?

pkg/fuzzer/queue/queue.go Outdated Show resolved Hide resolved
pkg/fuzzer/queue/queue.go Outdated Show resolved Hide resolved
pkg/fuzzer/queue/queue.go Outdated Show resolved Hide resolved
@a-nogikh
Copy link
Collaborator Author

a-nogikh commented May 6, 2024

Will this allow to create the fuzzer lazily after the VM checking procedure?

For that we need some dynamically configurable layer that would return nil in Next() before we create the fuzzer object and explicitly plug it in.


The somewhat problematic consequence of the PR is that all running jobs of the same type get to execute something in a round robin manner. It doesn't matter for hints/smash/candidate, but it's somewhat problematic for triage jobs, because now it takes ages to see corpus/coverage increases after we have begun fuzzing. It looks like we should still let the triage jobs that we started earlier finish first.

@a-nogikh a-nogikh force-pushed the features/common-exec-queue branch 3 times, most recently from 4f1689f to 7fd743e Compare May 6, 2024 17:42
@a-nogikh
Copy link
Collaborator Author

a-nogikh commented May 6, 2024

I've had to return priority queues to add order to the triage job executions. Now the question is whether these changes make pkg/fuzzer code better... The PR makes pkg/fuzzer smaller at the expense of pkg/fuzzer/queue, which might already be a good thing though.

While working on moving code from the target to the host, I see lots of utilities that duplicate code and pain to update. 4 more use cases that would benefit from moving into syz-manager:

  • syz-crush: repeatedly execute fixed set of programs on all VMs
  • syz-runtest: execute a fixed set of programs, print results, exit
  • reproducer testing in syz-ci: run syz/C repro for fixed amount of time and exit
  • image testing in syz-ci: simply exit after check

All of these are similar: do something else (rather than pkg/fuzzer) after the VM checking procedure. It would nice if the new architecture would support these use-cases natively.

It feels like it's not just the execution queues that are common here, but rather taking requests from the queue and executing them on a pool of VMs -- that's what rpc.go and manager.go do.

Repro and image testing are somewhat simpler of these 4 (if any VM crashed at any stage in the process, we don't retry, but just report the result), but syz-crush and runtest are indeed quite similar to what syz-manager does. There indeed seems to be some common functionality like "Here's a VM pool object and here are the VM indices, please serve requests from this queue using a pool of those VMs".

Syz-manager makes things a bit more complicated by dynamically resizing this VM pool to reproduce bugs.

I'm not sure whether merging all these tools into syz-manager is a good idea, but taking this functionality to e.g. a separate package and using the package in all of those cases may be a good first step.

@a-nogikh a-nogikh force-pushed the features/common-exec-queue branch from 7fd743e to 1c1eb66 Compare May 6, 2024 18:05
@dvyukov
Copy link
Collaborator

dvyukov commented May 7, 2024

For that we need some dynamically configurable layer that would return nil in Next() before we create the fuzzer object and explicitly plug it in.

Potentially we could always create all layers statically at start, but then some layers (fuzzer) won't be ready to work until they receive additional bits of information, which will need to be passed to them later somehow.
Not sure if it's better, but just listing options.

@dvyukov
Copy link
Collaborator

dvyukov commented May 7, 2024

Repro and image testing are somewhat simpler of these 4 (if any VM crashed at any stage in the process, we don't retry, but just report the result), but syz-crush and runtest are indeed quite similar to what syz-manager does. There indeed seems to be some common functionality like "Here's a VM pool object and here are the VM indices, please serve requests from this queue using a pool of those VMs".

Significant part of common functionality even with image testing and repro is all of the VM booting and RPC communication (initial handshake, VM checking + normal exchanges).

@dvyukov
Copy link
Collaborator

dvyukov commented May 7, 2024

Syz-manager makes things a bit more complicated by dynamically resizing this VM pool to reproduce bugs.

Potentially we could merge repro into the normal manager operation, repro will just supply more special requests for execution. Is special-ness is that they need to run on freshly booted VMs only. We needed that for syz-verifier (for final states of bug triage we also wanted to run tests of clean VMs).

@dvyukov
Copy link
Collaborator

dvyukov commented May 7, 2024

I'm not sure whether merging all these tools into syz-manager is a good idea, but taking this functionality to e.g. a separate package and using the package in all of those cases may be a good first step.

Agree that would be cleaner. However, it's even more work and refactoring.

@a-nogikh
Copy link
Collaborator Author

a-nogikh commented May 7, 2024

I've pushed the changes that move pkg/vminfo to the new execution interface, but it has broken tools/syz-runtest. Looks like I'll need to port pkg/runtest as well.

@dvyukov
Copy link
Collaborator

dvyukov commented May 7, 2024

I've pushed the changes that move pkg/vminfo to the new execution interface, but it has broken tools/syz-runtest. Looks like I'll need to port pkg/runtest as well.

Now you feel part of my pain with all these duplicate tools :)

@a-nogikh
Copy link
Collaborator Author

a-nogikh commented May 7, 2024

pkg/runtest is using the new interface now, the tests pass. Now I need to refactor tools/syz-runtest.

@a-nogikh a-nogikh force-pushed the features/common-exec-queue branch from ea47191 to 0a86ab0 Compare May 8, 2024 08:54
pkg/fuzzer/queue/queue.go Outdated Show resolved Hide resolved
pkg/fuzzer/fuzzer.go Outdated Show resolved Hide resolved
pkg/fuzzer/queue/queue.go Outdated Show resolved Hide resolved
@a-nogikh a-nogikh force-pushed the features/common-exec-queue branch from 0a86ab0 to a244f3b Compare May 8, 2024 11:58
pkg/fuzzer/queue/queue.go Outdated Show resolved Hide resolved
pkg/fuzzer/queue/queue.go Show resolved Hide resolved
pkg/fuzzer/queue/queue.go Outdated Show resolved Hide resolved
pkg/fuzzer/queue/queue.go Outdated Show resolved Hide resolved
pkg/fuzzer/queue/queue.go Outdated Show resolved Hide resolved
pkg/fuzzer/queue/queue.go Outdated Show resolved Hide resolved
pkg/fuzzer/queue/queue.go Outdated Show resolved Hide resolved
pkg/fuzzer/queue/queue.go Outdated Show resolved Hide resolved
pkg/runtest/run.go Show resolved Hide resolved
pkg/fuzzer/queue/retry.go Outdated Show resolved Hide resolved
pkg/fuzzer/queue/retry.go Outdated Show resolved Hide resolved
pkg/fuzzer/queue/queue.go Show resolved Hide resolved
pkg/vminfo/syscalls.go Outdated Show resolved Hide resolved
pkg/vminfo/syscalls.go Show resolved Hide resolved
syz-manager/rpc.go Outdated Show resolved Hide resolved
syz-manager/rpc.go Outdated Show resolved Hide resolved
syz-manager/rpc.go Show resolved Hide resolved
syz-manager/rpc.go Outdated Show resolved Hide resolved
pkg/fuzzer/queue/queue.go Outdated Show resolved Hide resolved
pkg/fuzzer/queue/queue.go Outdated Show resolved Hide resolved
pkg/fuzzer/fuzzer.go Outdated Show resolved Hide resolved
pkg/fuzzer/fuzzer.go Outdated Show resolved Hide resolved
pkg/fuzzer/queue/queue.go Outdated Show resolved Hide resolved
@a-nogikh a-nogikh force-pushed the features/common-exec-queue branch from a244f3b to 23358bb Compare May 10, 2024 12:03
syz-manager/rpc.go Outdated Show resolved Hide resolved
@a-nogikh a-nogikh force-pushed the features/common-exec-queue branch from 4563b65 to b4a10c5 Compare May 16, 2024 13:26
pkg/fuzzer/queue/queue.go Outdated Show resolved Hide resolved
pkg/fuzzer/queue/queue.go Outdated Show resolved Hide resolved
syz-manager/rpc.go Outdated Show resolved Hide resolved
syz-manager/rpc.go Outdated Show resolved Hide resolved
pkg/fuzzer/queue/queue.go Outdated Show resolved Hide resolved
pkg/fuzzer/queue/queue.go Outdated Show resolved Hide resolved
dvyukov
dvyukov previously approved these changes May 16, 2024
Instead of relying on a fuzzer-internal priority queue, utilize
stackable layers of request-generating steps.

Move the functionality to a separate pkg/fuzzer/queue package.

The pkg/fuzzer/queue package can be reused to add extra processing
layers on top of the fuzzing and to combine machine checking and fuzzing
execution pipelines.
Make Result statuses more elaborate.
Instead of retrying inputs directly in rpc.go, extract this logic to a
separate entity in pkg/fuzzer/queue.
Use the same interfaces as the fuzzer.
Now syz-manager no longer needs to treat machine check executions
differently.
There's no need to duplicate the execution mechanisms.
There's no need to wait until all results have been completed to print
them.
Mark some requests as Important. The Retry() layer will give them one
more chance even if they were not executed due to a VM crash.

For now, the only important requests are related to triage, candidates
and pkg/vminfo tests.

Add tests for retry.go.
If syz-runtest wants several runs, it will pass it as an option for C
code generation.
There's no need in duplicating the signal, coverage, hints flags.
For now, only ProgTypes is enough.
Return a new queue.Source from the machine check callback.
We don't support them in syz-manager.
Use a simpler implementation.
Don't assume the nested Source may be nil.
@dvyukov dvyukov enabled auto-merge May 16, 2024 15:38
@dvyukov dvyukov added this pull request to the merge queue May 16, 2024
Merged via the queue into google:master with commit ad5321c May 16, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants