all: configure proc restart frequency #6260
Conversation
a-nogikh
force-pushed
the
features/control-proc-restarts
branch
4 times, most recently
from
9fd12a4 to
be6e28b
Compare
a-nogikh
force-pushed
the
features/control-proc-restarts
branch
from
be6e28b to
a05f705
Compare
a-nogikh
changed the title
a-nogikh
force-pushed
the
features/control-proc-restarts
branch
2 times, most recently
from
9a9b616 to
318c0b9
Compare
|
I still can't pinpoint the actual reason, but it looks as if sometimes syzkaller doesn't respect the modified proc restart frequency value. In those cases, the real To some extent, it may be explained by the syzkaller/executor/executor_runner.h Line 127 in 523f460 Yet, corpus triage only took a small share of time and (judging by |
tarasmadan
force-pushed
the
features/control-proc-restarts
branch
from
318c0b9 to
a157327
Compare
|
@gemini-cli /review |
|
There is a problem with the Gemini CLI PR review. Please check the action logs for details. |
|
@gemini-cli /review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
📋 Review Summary
This PR introduces a configurable proc restart frequency, which is a great feature for improving bug reproduction. The implementation is mostly solid, but there are a few issues regarding the handling of a zero value for the restart frequency, which can lead to crashes or incorrect behavior in syz-execprog and during bug reproduction.
🔍 General Feedback
- The introduction of
ProcOptsis a good refactoring that improves code readability inexecutor_runner.h. - The new configuration option is consistently integrated across different packages.
- A few edge cases with the new
proc_restart_freqparameter need to be addressed to prevent crashes and ensure robust behavior.
| program_timeout_ms_(program_timeout_ms), | ||
| opts_(opts), | ||
| req_shmem_(kMaxInput), | ||
| resp_shmem_(kMaxOutput), |
There was a problem hiding this comment.
medium: The logic for proc restarts does not handle proc_restart_freq = 0 gracefully. If proc_restart_freq is 0, it can lead to a restart loop. A value of 0 should disable frequency-based restarts.
| resp_shmem_(kMaxOutput), | |
| if (state_ == State::Idle && ((corpus_triaged_ && restarting_ == 0 && opts_.proc_restart_freq > 0 && freshness_ >= opts_.proc_restart_freq) || |
| proc_opts_.is_kernel_64_bit = is_kernel_64_bit = conn_reply.kernel_64_bit; | ||
| proc_opts_.slowdown = conn_reply.slowdown; |
There was a problem hiding this comment.
high: This check is too strict and will cause syz-execprog to fail with its default settings. A proc_restart_freq of 0 should be a valid value to disable frequency-based restarts. This check should be removed, and the restart logic should handle 0 gracefully (as suggested in another comment).
| proc_opts_.is_kernel_64_bit = is_kernel_64_bit = conn_reply.kernel_64_bit; | |
| proc_opts_.slowdown = conn_reply.slowdown; |
| func(opts *csource.Options) bool { | ||
| if opts.ProcRestartFreq == 0 { | ||
| return false | ||
| } | ||
| opts.ProcRestartFreq = 0 | ||
| return true |
There was a problem hiding this comment.
high: This simplification step sets ProcRestartFreq to 0. This can prevent reproduction of bugs that only trigger with a specific proc restart frequency. Furthermore, with the current implementation, a value of 0 can cause the executor to fail or enter a restart loop. This simplification step should be removed.
| func(opts *csource.Options) bool { | |
| if opts.ProcRestartFreq == 0 { | |
| return false | |
| } | |
| opts.ProcRestartFreq = 0 | |
| return true |
| } | ||
| } | ||
|
|
||
| // Do more frequent proc restarts to facilitate a higher reproducation rate. |
There was a problem hiding this comment.
low: Typo in the comment.
| // Do more frequent proc restarts to facilitate a higher reproducation rate. | |
| // Do more frequent proc restarts to facilitate a higher reproduction rate. |
| flagEnable = flag.String("enable", "none", "enable only listed additional features") | ||
| flagDisable = flag.String("disable", "none", "enable all additional features except listed") | ||
| flagExecutor = flag.String("executor", "./syz-executor", "path to executor binary") | ||
| flagThreaded = flag.Bool("threaded", true, "use threaded mode in executor") |
There was a problem hiding this comment.
medium: The default value of 0 for restart_freq will cause the executor to fail due to a check that rejects a value of 0. While the executor should be made more robust, it's good practice to have consistent default values. The default should be aligned with the one in rpcserver (600).
| flagThreaded = flag.Bool("threaded", true, "use threaded mode in executor") | |
| flagRestartFreq = flag.Int("restart_freq", 600, "restart procs every X executions") |
Instead of always using the default value of 600, make it possible to set it via the manager config. For now in the experimental list of features.
This should increase the bug reproduction rate.
If we use a non-default restart frequence during fuzzing, we should keep doing so during bug reproduction.
a-nogikh
force-pushed
the
features/control-proc-restarts
branch
from
a157327 to
ac7cfc7
Compare
A small refactoring of
executor_runner.h+ a new (experimental) syz-manager config that makes the proc restart rate configurable.