make bisection more robust in case of config minimization failures

[bulwahn]

When using the bisect script, we noticed that some bisection just failed as the config minimization internally failed and the bisect script then just gave up. However, we can actually continue bisection with the original config and still find reasonable bisection results. The needed change for that behavior is small and simple.

[google-cla[bot]]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 22.22222% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 63.0%. Comparing base (6ee49f2) to head (5a3e2cb).

Additional details and impacted files

Files	Coverage Δ
pkg/bisect/bisect.go	72.5% <40.0%> (-0.3%)	:arrow_down:
pkg/vcs/linux.go	26.2% <0.0%> (-0.2%)	:arrow_down:

... and 1 file with indirect coverage changes

[a-nogikh]

How exactly did the config minimization fail? Maybe that's something we can improve? Normally it shouldn't be failing with an error.

By just ignoring such errors (e.g. on syzbot we don't have capacity to manually monitor all bisection logs) and by continuing the process as if nothing happened we just risk never noticing any other breakages in that code. Or noticing them too late.

[Krzpaw]

Hello Aleksandr, the config minimization failed because of the unknown symbol found in one of the Kconfigs - someone used boolean instead of bool:

deerl1pr131 starts bisection 2023-09-10 18:03:09.846342702 +0200 CEST m=+2.252515632
bisecting fixing commit since cfb41ef9deb1e6572ac218ddfcec9567e5d1c101
ensuring issue is reproducible on original commit cfb41ef9deb1e6572ac218ddfcec9567e5d1c101

testing commit cfb41ef9deb1e6572ac218ddfcec9567e5d1c101 gcc
compiler: gcc version 13.2.1 20230728 (Red Hat 13.2.1-1) (GCC)
kernel signature: d0871d857e470c98bd6f37cdbee7753aca5219255ca724cf3b2531fb183611b4
all runs: crashed: KASAN: null-ptr-deref Read in cgroup_file_write
representative crash: KASAN: null-ptr-deref Read in cgroup_file_write, types: [KASAN]
revisions tested: 1, total time: 2m24.869122692s (build: 46.309111133s, test: 1m4.711605112s)
error: config minimization failed: failed to parse Kconfig: /home/ebfuzz/workspace/krzpaw/linux/drivers/staging/rtlwifi/Kconfig:20:8: unknown line
	boolean
bisection failed: config minimization failed: failed to parse Kconfig: /home/ebfuzz/workspace/krzpaw/linux/drivers/staging/rtlwifi/Kconfig:20:8: unknown line
	boolean
exit status 1

So actually the error is caused by the broken kernel sources and not by the Syzkaller itself; but I'm not sure how to handle this on the Syzkaller side other than just to ignore and proceed. Perhaps we should implement a mechanism that would distinguish between errors thrown during the minimization and decide whether we should proceed with the original config or fail.

[a-nogikh]

I see, thanks for the clarification!

It was not a big problem for Kconfig parser in the Linux kernel, right?

Perhaps we should implement a mechanism that would distinguish between errors thrown during the minimization and decide whether we should proceed with the original config or fail.

That sounds like a better option to me. We can indeed expect kernel code and configuration files to have problems and should not be failing hard because of that, but in all other cases aborting the bisection process looks reasonable.

We can use Go error wrapping mechanism here:

var ErrBadKconfig = errors.New("failed to parse Kconfig")

And then:

return fmt.Errorf("%w: %v", ErrBadKconfig, err)

And then:

if errors.Is(err, vcs.ErrBadKconfig) {
  // print a warning and go on
} else {
 // abort 
}

[Krzpaw]

Ok, got it, thank you for your hint!

Let me improve the implementation and I'll be back with an updated patch after the Christmas break.

[bulwahn]

@a-nogikh Sorry for the delay. Due to some unexpected larger company disturbances (and not due to local personal failures), we lost @Krzpaw to continue contributing to syzkaller. Fortunately, @simone-weiss could take over this work after a bit of delay. So, she reworked this pull request as discussed above. I hope it is now ready to be merged.

@Krzpaw if you would have a second, please review and ack. @a-nogikh If @Krzpaw does not have time for a review, it should not be blocker for merging, though.