data race in virtio net

Go can detect race conditions by giving the -race option. According to the results, there is apparently a data race for virtio net in cdd468358729 ("Merge pull request #135 from bobuhiro11/add_travis_retry").

We may need to implement some locking mechanism (e.g. sync.Mutex) for virtio data.

ubuntu2004thinkpad:~/gokvm$ sudo env "PATH=$PATH" go test ./machine/... -race

...

==================
WARNING: DATA RACE
Read at 0x00c00038e048 by goroutine 14:
  github.com/bobuhiro11/gokvm/virtio.(*Net).IOInHandler()
      <autogenerated>:1 +0x85
  github.com/bobuhiro11/gokvm/pci.Device.IOInHandler-fm()
      <autogenerated>:1 +0x81
  github.com/bobuhiro11/gokvm/machine.(*Machine).RunOnce()
      /home/bobuhiro11/gokvm/machine/machine.go:683 +0x9a2
  github.com/bobuhiro11/gokvm/machine.(*Machine).RunInfiniteLoop()
      /home/bobuhiro11/gokvm/machine/machine.go:648 +0xaa
  github.com/bobuhiro11/gokvm/machine_test.testNewAndLoadLinux.func1()
      /home/bobuhiro11/gokvm/machine/machine_test.go:63 +0x44

Previous write at 0x00c00038e048 by goroutine 12:
  github.com/bobuhiro11/gokvm/virtio.(*Net).Rx()
      /home/bobuhiro11/gokvm/virtio/net.go:169 +0x335
  github.com/bobuhiro11/gokvm/virtio.(*Net).RxThreadEntry()
      /home/bobuhiro11/gokvm/virtio/net.go:101 +0x84
  github.com/bobuhiro11/gokvm/machine.(*Machine).AddTapIf.func2()
      /home/bobuhiro11/gokvm/machine/machine.go:196 +0x39

Goroutine 14 (running) created at:
  github.com/bobuhiro11/gokvm/machine_test.testNewAndLoadLinux()
      /home/bobuhiro11/gokvm/machine/machine_test.go:62 +0x989
  github.com/bobuhiro11/gokvm/machine_test.TestNewAndLoadLinuxWithBzImage()
      /home/bobuhiro11/gokvm/machine/machine_test.go:99 +0x71
  testing.tRunner()
      /home/bobuhiro11/.goenv/versions/1.19.0/src/testing/testing.go:1446 +0x216
  testing.(*T).Run.func1()
      /home/bobuhiro11/.goenv/versions/1.19.0/src/testing/testing.go:1493 +0x47

Goroutine 12 (running) created at:
  github.com/bobuhiro11/gokvm/machine.(*Machine).AddTapIf()
      /home/bobuhiro11/gokvm/machine/machine.go:196 +0x1b3
  github.com/bobuhiro11/gokvm/machine_test.testNewAndLoadLinux()
      /home/bobuhiro11/gokvm/machine/machine_test.go:28 +0x255
  github.com/bobuhiro11/gokvm/machine_test.TestNewAndLoadLinuxWithBzImage()
      /home/bobuhiro11/gokvm/machine/machine_test.go:99 +0x71
  testing.tRunner()
      /home/bobuhiro11/.goenv/versions/1.19.0/src/testing/testing.go:1446 +0x216
  testing.(*T).Run.func1()
      /home/bobuhiro11/.goenv/versions/1.19.0/src/testing/testing.go:1493 +0x47
==================

A simple lock/unlock as shown below resolves the data race but fails the unit tests. I wonder why.

Click to expand

```diff $ git --no-pager diff diff --git a/virtio/blk.go b/virtio/blk.go index 4dfeaf10f3f9..4f57874598e4 100644 --- a/virtio/blk.go +++ b/virtio/blk.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/binary" "os" + "sync" "unsafe" "github.com/bobuhiro11/gokvm/pci" @@ -28,6 +29,7 @@ type Blk struct { irq uint8 IRQInjector IRQInjector + mux sync.Mutex } type blkHdr struct { @@ -49,7 +51,10 @@ type blkHeader struct { capacity uint64 } -func (v Blk) GetDeviceHeader() pci.DeviceHeader { +func (v *Blk) GetDeviceHeader() pci.DeviceHeader { + v.mux.Lock() + defer v.mux.Unlock() + return pci.DeviceHeader{ DeviceID: 0x1001, VendorID: 0x1AF4, @@ -66,7 +71,10 @@ func (v Blk) GetDeviceHeader() pci.DeviceHeader { } } -func (v Blk) IOInHandler(port uint64, bytes []byte) error { +func (v *Blk) IOInHandler(port uint64, bytes []byte) error { + v.mux.Lock() + defer v.mux.Unlock() + offset := int(port - BlkIOPortStart) b, err := v.Hdr.Bytes() @@ -147,10 +155,16 @@ func (v *Blk) IO() error { } usedRing.Idx++ + + v.mux.Lock() v.LastAvailIdx[sel]++ + v.mux.Unlock() } + v.mux.Lock() v.Hdr.commonHeader.isr = 0x1 + v.mux.Unlock() + if err := v.IRQInjector.InjectVirtioBlkIRQ(); err != nil { return err } @@ -159,6 +173,9 @@ func (v *Blk) IO() error { } func (v *Blk) IOOutHandler(port uint64, bytes []byte) error { + v.mux.Lock() + defer v.mux.Unlock() + offset := int(port - BlkIOPortStart) switch offset { diff --git a/virtio/net.go b/virtio/net.go index 7c2c18a2e130..01cec8286e40 100644 --- a/virtio/net.go +++ b/virtio/net.go @@ -8,6 +8,7 @@ import ( "io" "os" "os/signal" + "sync" "syscall" "unsafe" @@ -47,6 +48,7 @@ type Net struct { irq uint8 IRQInjector IRQInjector + mux sync.Mutex } func (h netHdr) Bytes() ([]byte, error) { @@ -65,7 +67,10 @@ type netHeader struct { _ uint16 // maxVirtQueuePairs } -func (v Net) GetDeviceHeader() pci.DeviceHeader { +func (v *Net) GetDeviceHeader() pci.DeviceHeader { + v.mux.Lock() + defer v.mux.Unlock() + return pci.DeviceHeader{ DeviceID: 0x1000, VendorID: 0x1AF4, @@ -82,15 +87,20 @@ func (v Net) GetDeviceHeader() pci.DeviceHeader { } } -func (v Net) IOInHandler(port uint64, bytes []byte) error { +func (v *Net) IOInHandler(port uint64, bytes []byte) error { + v.mux.Lock() + defer v.mux.Unlock() + offset := int(port - NetIOPortStart) b, err := v.Hdr.Bytes() + if err != nil { return err } l := len(bytes) + copy(bytes[:l], b[offset:offset+l]) return nil @@ -119,7 +129,10 @@ func (v *Net) Rx() error { sel := 0 - if v.VirtQueue[sel] == nil { + v.mux.Lock() + q := v.VirtQueue[sel] + v.mux.Unlock() + if q == nil { return ErrVQNotInit } @@ -171,7 +184,9 @@ func (v *Net) Rx() error { usedRing.Idx++ + v.mux.Lock() v.Hdr.commonHeader.isr = 0x1 + v.mux.Unlock() return v.IRQInjector.InjectVirtioNetIRQ() } @@ -229,15 +244,23 @@ func (v *Net) Tx() error { return err } usedRing.Idx++ + + v.mux.Lock() v.LastAvailIdx[sel]++ + v.mux.Unlock() } + v.mux.Lock() v.Hdr.commonHeader.isr = 0x1 + v.mux.Unlock() return v.IRQInjector.InjectVirtioNetIRQ() } func (v *Net) IOOutHandler(port uint64, bytes []byte) error { + v.mux.Lock() + defer v.mux.Unlock() + offset := int(port - NetIOPortStart) switch offset { ```

bobuhiro11 / gokvm

data race in virtio net #137