vm: pass Report to Diagnose

The way to diagnose generally depends on the issue.
E.g. do we need register dump to debug this issue?
Do we need host dmesg dump? Some diagnosis may be
directly specific to a particular problem (e.g. dumping
a particular debugfs/procfs file).
Pass Report to Diagnose to make this possible.

13 files changed, 60 insertions, 43 deletions

diff --git a/pkg/report/report.go b/pkg/report/report.go
index bd648e8ec..fc923764f 100644
--- a/pkg/report/report.go
+++ b/pkg/report/report.go
@@ -123,8 +123,6 @@ func NewReporter(cfg *mgrconfig.Config) (Reporter, error) {
 }
 
 const (
-	VMDiagnosisStart = "\nVM DIAGNOSIS:\n"
-
 	unexpectedKernelReboot = "unexpected kernel reboot"
 	memoryLeakPrefix       = "memory leak in "
 	dataRacePrefix         = "KCSAN: data-race"
@@ -184,9 +182,6 @@ func (wrap *reporterWrapper) Parse(output []byte) *Report {
 	if match := reportFrameRe.FindStringSubmatch(rep.Title); match != nil {
 		rep.Frame = match[1]
 	}
-	if pos := bytes.Index(rep.Report, []byte(VMDiagnosisStart)); pos != -1 {
-		rep.Report = rep.Report[:pos]
-	}
 	rep.SkipPos = len(output)
 	if pos := bytes.IndexByte(output[rep.StartPos:], '\n'); pos != -1 {
 		rep.SkipPos = rep.StartPos + pos
diff --git a/vm/adb/adb.go b/vm/adb/adb.go
index 75e4a1f7c..63031c822 100644
--- a/vm/adb/adb.go
+++ b/vm/adb/adb.go
@@ -20,6 +20,7 @@ import (
 	"github.com/google/syzkaller/pkg/config"
 	"github.com/google/syzkaller/pkg/log"
 	"github.com/google/syzkaller/pkg/osutil"
+	"github.com/google/syzkaller/pkg/report"
 	"github.com/google/syzkaller/vm/vmimpl"
 )
 
@@ -404,6 +405,6 @@ func (inst *instance) Run(timeout time.Duration, stop <-chan bool, command strin
 	return vmimpl.Multiplex(adb, merger, tty, timeout, stop, inst.closed, inst.debug)
 }
 
-func (inst *instance) Diagnose() ([]byte, bool) {
+func (inst *instance) Diagnose(rep *report.Report) ([]byte, bool) {
 	return nil, false
 }
diff --git a/vm/bhyve/bhyve.go b/vm/bhyve/bhyve.go
index b4004aef2..ec0eae81a 100644
--- a/vm/bhyve/bhyve.go
+++ b/vm/bhyve/bhyve.go
@@ -16,6 +16,7 @@ import (
 	"github.com/google/syzkaller/pkg/config"
 	"github.com/google/syzkaller/pkg/log"
 	"github.com/google/syzkaller/pkg/osutil"
+	"github.com/google/syzkaller/pkg/report"
 	"github.com/google/syzkaller/vm/vmimpl"
 )
 
@@ -347,7 +348,7 @@ func (inst *instance) Run(timeout time.Duration, stop <-chan bool, command strin
 	return inst.merger.Output, errc, nil
 }
 
-func (inst *instance) Diagnose() ([]byte, bool) {
+func (inst *instance) Diagnose(rep *report.Report) ([]byte, bool) {
 	return vmimpl.DiagnoseFreeBSD(inst.consolew)
 }
 
diff --git a/vm/gce/gce.go b/vm/gce/gce.go
index e3f23ccee..b5565aff0 100644
--- a/vm/gce/gce.go
+++ b/vm/gce/gce.go
@@ -28,6 +28,7 @@ import (
 	"github.com/google/syzkaller/pkg/kd"
 	"github.com/google/syzkaller/pkg/log"
 	"github.com/google/syzkaller/pkg/osutil"
+	"github.com/google/syzkaller/pkg/report"
 	"github.com/google/syzkaller/sys/targets"
 	"github.com/google/syzkaller/vm/vmimpl"
 )
@@ -369,7 +370,7 @@ func waitForConsoleConnect(merger *vmimpl.OutputMerger) error {
 	}
 }
 
-func (inst *instance) Diagnose() ([]byte, bool) {
+func (inst *instance) Diagnose(rep *report.Report) ([]byte, bool) {
 	if inst.env.OS == targets.FreeBSD {
 		return vmimpl.DiagnoseFreeBSD(inst.consolew)
 	}
diff --git a/vm/gvisor/gvisor.go b/vm/gvisor/gvisor.go
index 8d8454e25..4365c22f9 100644
--- a/vm/gvisor/gvisor.go
+++ b/vm/gvisor/gvisor.go
@@ -20,6 +20,7 @@ import (
 	"github.com/google/syzkaller/pkg/config"
 	"github.com/google/syzkaller/pkg/log"
 	"github.com/google/syzkaller/pkg/osutil"
+	"github.com/google/syzkaller/pkg/report"
 	"github.com/google/syzkaller/vm/vmimpl"
 )
 
@@ -352,7 +353,8 @@ func (inst *instance) guestProxy() (*os.File, error) {
 	return guestSock, nil
 }
 
-func (inst *instance) Diagnose() ([]byte, bool) {
+func (inst *instance) Diagnose(rep *report.Report) ([]byte, bool) {
+	// TODO: stacks and dmesg are mostly useful for hangs/stalls, so we could do this only sometimes based on rep.
 	b, err := osutil.Run(time.Minute, inst.runscCmd("debug", "-stacks", "--ps", inst.name))
 	if err != nil {
 		b = append(b, fmt.Sprintf("\n\nError collecting stacks: %v", err)...)
diff --git a/vm/isolated/isolated.go b/vm/isolated/isolated.go
index 6f703280f..e62548e54 100755
--- a/vm/isolated/isolated.go
+++ b/vm/isolated/isolated.go
@@ -17,6 +17,7 @@ import (
 	"github.com/google/syzkaller/pkg/config"
 	"github.com/google/syzkaller/pkg/log"
 	"github.com/google/syzkaller/pkg/osutil"
+	"github.com/google/syzkaller/pkg/report"
 	"github.com/google/syzkaller/vm/vmimpl"
 )
 
@@ -420,10 +421,12 @@ func (inst *instance) readPstoreContents() ([]byte, error) {
 	return stdout.Bytes(), nil
 }
 
-func (inst *instance) Diagnose() ([]byte, bool) {
+func (inst *instance) Diagnose(rep *report.Report) ([]byte, bool) {
 	if !inst.cfg.Pstore {
 		return nil, false
 	}
+	// TODO: kernel may not reboot after some errors.
+	// E.g. if panic_on_warn is not set, or some errors don't trigger reboot at all (e.g. LOCKDEP overflows).
 	log.Logf(2, "waiting for crashed DUT to come back up")
 	if err := inst.waitRebootAndSSH(5*60, 30*time.Minute); err != nil {
 		return []byte(fmt.Sprintf("unable to SSH into DUT after reboot: %v", err)), false
diff --git a/vm/kvm/kvm.go b/vm/kvm/kvm.go
index 88983971c..e63e128e9 100644
--- a/vm/kvm/kvm.go
+++ b/vm/kvm/kvm.go
@@ -18,6 +18,7 @@ import (
 	"github.com/google/syzkaller/pkg/config"
 	"github.com/google/syzkaller/pkg/log"
 	"github.com/google/syzkaller/pkg/osutil"
+	"github.com/google/syzkaller/pkg/report"
 	"github.com/google/syzkaller/vm/vmimpl"
 )
 
@@ -287,7 +288,7 @@ func (inst *instance) Run(timeout time.Duration, stop <-chan bool, command strin
 	return outputC, errorC, nil
 }
 
-func (inst *instance) Diagnose() ([]byte, bool) {
+func (inst *instance) Diagnose(rep *report.Report) ([]byte, bool) {
 	return nil, false
 }
 
diff --git a/vm/qemu/qemu.go b/vm/qemu/qemu.go
index 7b2061c20..d0689fa1d 100644
--- a/vm/qemu/qemu.go
+++ b/vm/qemu/qemu.go
@@ -18,6 +18,7 @@ import (
 	"github.com/google/syzkaller/pkg/config"
 	"github.com/google/syzkaller/pkg/log"
 	"github.com/google/syzkaller/pkg/osutil"
+	"github.com/google/syzkaller/pkg/report"
 	"github.com/google/syzkaller/sys/targets"
 	"github.com/google/syzkaller/vm/vmimpl"
 )
@@ -619,7 +620,9 @@ func (inst *instance) Run(timeout time.Duration, stop <-chan bool, command strin
 	return inst.merger.Output, errc, nil
 }
 
-func (inst *instance) Diagnose() ([]byte, bool) {
+func (inst *instance) Diagnose(rep *report.Report) ([]byte, bool) {
+	// TODO: we don't need registers on all reports. Probably only relevant for "crashes"
+	// (NULL derefs, paging faults, etc), but is not useful for WARNING/BUG/HANG (?).
 	ret := []byte(fmt.Sprintf("%s Registers:\n", time.Now().Format("15:04:05 ")))
 	for cpu := 0; cpu < inst.cfg.CPU; cpu++ {
 		regs, err := inst.hmp("info registers", cpu)
diff --git a/vm/vm.go b/vm/vm.go
index ab6bf6a21..d8f3c73f9 100644
--- a/vm/vm.go
+++ b/vm/vm.go
@@ -137,8 +137,11 @@ func (inst *Instance) Run(timeout time.Duration, stop <-chan bool, command strin
 	return inst.impl.Run(timeout, stop, command)
 }
 
-func (inst *Instance) Diagnose() ([]byte, bool) {
-	return inst.impl.Diagnose()
+func (inst *Instance) diagnose(rep *report.Report) ([]byte, bool) {
+	if rep == nil {
+		panic("rep is nil")
+	}
+	return inst.impl.Diagnose(rep)
 }
 
 func (inst *Instance) Close() {
@@ -267,14 +270,8 @@ type monitor struct {
 
 func (mon *monitor) extractError(defaultError string) *report.Report {
 	diagOutput, diagWait := []byte{}, false
-	appendDiagOutput := func() {
-		if len(diagOutput) > 0 {
-			mon.output = append(mon.output, report.VMDiagnosisStart...)
-			mon.output = append(mon.output, diagOutput...)
-		}
-	}
 	if defaultError != "" {
-		diagOutput, diagWait = mon.inst.Diagnose()
+		diagOutput, diagWait = mon.inst.diagnose(mon.createReport(defaultError))
 	}
 	// Give it some time to finish writing the error message.
 	// But don't wait for "no output", we already waited enough.
@@ -284,28 +281,36 @@ func (mon *monitor) extractError(defaultError string) *report.Report {
 	if bytes.Contains(mon.output, []byte(fuzzerPreemptedStr)) {
 		return nil
 	}
-	if !mon.reporter.ContainsCrash(mon.output[mon.matchPos:]) {
+	if defaultError == "" && mon.reporter.ContainsCrash(mon.output[mon.matchPos:]) {
+		// We did not call Diagnose above because we thought there is no error, so call it now.
+		diagOutput, diagWait = mon.inst.diagnose(mon.createReport(defaultError))
+		if diagWait {
+			mon.waitForOutput()
+		}
+	}
+	rep := mon.createReport(defaultError)
+	if rep == nil {
+		return nil
+	}
+	if len(diagOutput) > 0 {
+		rep.Output = append(rep.Output, vmDiagnosisStart...)
+		rep.Output = append(rep.Output, diagOutput...)
+	}
+	return rep
+}
+
+func (mon *monitor) createReport(defaultError string) *report.Report {
+	rep := mon.reporter.Parse(mon.output[mon.matchPos:])
+	if rep == nil {
 		if defaultError == "" {
 			return nil
 		}
-		appendDiagOutput()
 		return &report.Report{
 			Title:      defaultError,
 			Output:     mon.output,
 			Suppressed: report.IsSuppressed(mon.reporter, mon.output),
 		}
 	}
-	if defaultError == "" {
-		diagOutput, diagWait = mon.inst.Diagnose()
-		if diagWait {
-			mon.waitForOutput()
-		}
-	}
-	appendDiagOutput()
-	rep := mon.reporter.Parse(mon.output[mon.matchPos:])
-	if rep == nil {
-		panic(fmt.Sprintf("reporter.ContainsCrash/Parse disagree:\n%s", mon.output[mon.matchPos:]))
-	}
 	start := mon.matchPos + rep.StartPos - beforeContext
 	if start < 0 {
 		start = 0
@@ -347,6 +352,7 @@ const (
 	executingProgramStr1 = "executing program"  // syz-fuzzer output
 	executingProgramStr2 = "executed programs:" // syz-execprog output
 	fuzzerPreemptedStr   = "SYZ-FUZZER: PREEMPTED"
+	vmDiagnosisStart     = "\nVM DIAGNOSIS:\n"
 )
 
 var (
diff --git a/vm/vm_test.go b/vm/vm_test.go
index 31dd65045..3c3a14aa3 100644
--- a/vm/vm_test.go
+++ b/vm/vm_test.go
@@ -51,7 +51,7 @@ func (inst *testInstance) Run(timeout time.Duration, stop <-chan bool, command s
 	return inst.outc, inst.errc, nil
 }
 
-func (inst *testInstance) Diagnose() ([]byte, bool) {
+func (inst *testInstance) Diagnose(rep *report.Report) ([]byte, bool) {
 	var diag []byte
 	if inst.diagnoseBug {
 		diag = []byte("BUG: DIAGNOSE\n")
diff --git a/vm/vmimpl/vmimpl.go b/vm/vmimpl/vmimpl.go
index f83d39d57..0ef8636ee 100644
--- a/vm/vmimpl/vmimpl.go
+++ b/vm/vmimpl/vmimpl.go
@@ -18,6 +18,7 @@ import (
 
 	"github.com/google/syzkaller/pkg/log"
 	"github.com/google/syzkaller/pkg/osutil"
+	"github.com/google/syzkaller/pkg/report"
 )
 
 // Pool represents a set of test machines (VMs, physical devices, etc) of particular type.
@@ -44,13 +45,14 @@ type Instance interface {
 	// Command is terminated after timeout. Send on the stop chan can be used to terminate it earlier.
 	Run(timeout time.Duration, stop <-chan bool, command string) (outc <-chan []byte, errc <-chan error, err error)
 
-	// Diagnose retrieves additional debugging info from the VM (e.g. by
-	// sending some sys-rq's or SIGABORT'ing a Go program).
+	// Diagnose retrieves additional debugging info from the VM
+	// (e.g. by sending some sys-rq's or SIGABORT'ing a Go program).
 	//
-	// Optionally returns (some or all) of the info directly. If wait ==
-	// true, the caller must wait for the VM to output info directly to its
-	// log.
-	Diagnose() (diagnosis []byte, wait bool)
+	// Optionally returns (some or all) of the info directly. If wait == true,
+	// the caller must wait for the VM to output info directly to its log.
+	//
+	// rep describes the reason why Diagnose was called.
+	Diagnose(rep *report.Report) (diagnosis []byte, wait bool)
 
 	// Close stops and destroys the VM.
 	Close()
diff --git a/vm/vmm/vmm.go b/vm/vmm/vmm.go
index 62908de1f..9872e4d50 100644
--- a/vm/vmm/vmm.go
+++ b/vm/vmm/vmm.go
@@ -17,6 +17,7 @@ import (
 	"github.com/google/syzkaller/pkg/config"
 	"github.com/google/syzkaller/pkg/log"
 	"github.com/google/syzkaller/pkg/osutil"
+	"github.com/google/syzkaller/pkg/report"
 	"github.com/google/syzkaller/vm/vmimpl"
 )
 
@@ -310,7 +311,7 @@ func (inst *instance) Run(timeout time.Duration, stop <-chan bool, command strin
 	return inst.merger.Output, errc, nil
 }
 
-func (inst *instance) Diagnose() ([]byte, bool) {
+func (inst *instance) Diagnose(rep *report.Report) ([]byte, bool) {
 	return vmimpl.DiagnoseOpenBSD(inst.consolew)
 }
 
diff --git a/vm/vmware/vmware.go b/vm/vmware/vmware.go
index 25f4eb4ec..ebfb995bb 100644
--- a/vm/vmware/vmware.go
+++ b/vm/vmware/vmware.go
@@ -17,6 +17,7 @@ import (
 	"github.com/google/syzkaller/pkg/config"
 	"github.com/google/syzkaller/pkg/log"
 	"github.com/google/syzkaller/pkg/osutil"
+	"github.com/google/syzkaller/pkg/report"
 	"github.com/google/syzkaller/vm/vmimpl"
 )
 
@@ -217,6 +218,6 @@ func (inst *instance) Run(timeout time.Duration, stop <-chan bool, command strin
 	return vmimpl.Multiplex(cmd, merger, dmesg, timeout, stop, inst.closed, inst.debug)
 }
 
-func (inst *instance) Diagnose() ([]byte, bool) {
+func (inst *instance) Diagnose(rep *report.Report) ([]byte, bool) {
 	return nil, false
 }