Skip to content

Conversation

@a-nogikh
Copy link
Collaborator

Fixes #5395.

It's not very useful to just have the number of the broken seeds. Let's
also show their names and the reasons why they are broken.
@a-nogikh a-nogikh requested a review from dvyukov October 14, 2024 10:28
@dvyukov
Copy link
Collaborator

dvyukov commented Oct 14, 2024

We have a test somewhere that checks parsing of all seeds. How did it happen that it wasn't triggered?
Can we fix the test instead to catch this earlier? There is really no point in committing broken seeds in the first place, so none should reach syz-manager (that's why it did not have proper diagnostics for this case).

@a-nogikh
Copy link
Collaborator Author

I think printing the failed tests is useful anyway -- it e.g. gives more some human-friendly feedback during development and local syz-manager runs, and this can often happen well before one gets to run all presubmit tests.

There is really no point in committing broken seeds in the first place

I agree.

We do check that we can parse the seed programs (and AFAIK in multiple places), but here the seed was violating the expectations of pkg/manager on the maximum number of calls in it.

One of the tests (the most relevant one?) is here:

func TestParsing(t *testing.T) {
t.Parallel()
// Test only one target in race mode (we have gazillion of auto-generated Linux test).
raceTarget := targets.Get(targets.TestOS, targets.TestArch64)
for OS, arches := range targets.List {
if testutil.RaceEnabled && OS != raceTarget.OS {
continue
}
dir := filepath.Join("..", "..", "sys", OS, "test")

But it relies on the checks done in pkg/runtest itself:

p, err := target.Deserialize(data, prog.Strict)
if err != nil {
return nil, nil, nil, fmt.Errorf("failed to deserialize %v: %w", filename, err)
}

We can try to reuse manager.LoadProg() here to rely on the same logic that will later be used by syz-manager.

@dvyukov
Copy link
Collaborator

dvyukov commented Oct 14, 2024

Extending syz-manager reporting is fine (I guess it may be useful for some downstream users with custom seeds).
But the most important here is to fix the test to do what prod does. I guess we need to move the loading function from syz-manager to somewhere else and reuse it in the test.

The test has become too big (>40 calls).
Split off dev_iommu_vfio and remove the overlap with dev_iommu_hwpt.
It will help us catch broken seeds right in TestParse().
@a-nogikh a-nogikh force-pushed the features/list-broken-seeds branch from 552a08d to f4a36c0 Compare October 14, 2024 13:27
@a-nogikh
Copy link
Collaborator Author

Added a commit that reuses the same seed validation logic. PTAL.

@a-nogikh a-nogikh added this pull request to the merge queue Oct 14, 2024
Merged via the queue into google:master with commit 56381a3 Oct 14, 2024
@peiwithhao
Copy link

So how to fix it?I found the fuzz program has been stopped because of the failed
image

@a-nogikh
Copy link
Collaborator Author

a-nogikh commented Dec 2, 2024

The fuzzing cannot have been stopped because of these
a) it's just seeds (syzkaller can run without them)
b) it's 13 out of 100s of seed programs.

@ramosian-glider It looks like we need to prevent syz-manager from trying to parse arm64 seeds on non-arm64 machines.

There's an annotation for that

# requires: arch=amd64

But I'm not sure it's supported by syz-manager now, probably so far only by pkg/runtest.

@ramosian-glider
Copy link
Member

We also have annotations like this: # {Threaded:true Repeat:true RepeatTimes:0 Procs:1 Slowdown:10 Sandbox: SandboxArg:0 Leak:false NetInjection:false NetDevices:false NetReset:false Cgroups:false BinfmtMisc:false CloseFDs:false KCSAN:false DevlinkPCI:false NicVF:false USB:false VhciInjection:false Wifi:false IEEE802154:false Sysctl:false Swap:false UseTmpDir:false HandleSegv:false Trace:false LegacyOptions:{Collide:false Fault:false FaultCall:0 FaultNth:0}} in reproducers generated by syzkaller, perhaps these should be unified?

@a-nogikh
Copy link
Collaborator Author

a-nogikh commented Dec 2, 2024

perhaps these should be unified?

Unified with what exactly?

@ramosian-glider
Copy link
Member

So how to fix it?I found the fuzz program has been stopped because of the failed image

This should be fixed in #5560

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.

broken programs in the corpus: 0, broken seeds: 1

4 participants