google / gvisor

Application Kernel for Containers
https://gvisor.dev
Apache License 2.0
15.85k stars 1.3k forks source link

tcpip package compilation fails on 32-bit systems. #1446

Closed abarisani closed 3 years ago

abarisani commented 4 years ago

The sndSsthresh variable, defined as int, overflows with assignment math.MaxInt64 on 32-bit systems.

This currently prevents compilation on arm/32-bit systems, which has to be worked around by defining the variable as int32 or assigning a non overflowing 32-bit value (which is probably the wrong thing to do).

Error:

pkg/mod/gvisor.dev/gvisor@v0.0.0-20191220014331-99cfc711c699/pkg/tcpip/transport/tcp/snd.go:276:16: constant 9223372036854775807 overflows int

comzyh commented 4 years ago

814 May help.

abarisani commented 4 years ago

Pull request https://github.com/google/gvisor/pull/814 solves this issue, though on 32-bit ones another one remains, please see our (crude) workaround https://github.com/f-secure-foundry/gvisor/commit/98827aa916070b2c8ae68a1fb509e82c98030e61

mikma commented 3 years ago

Why is this issue in the arm64 Support milestone? The tcpip package seems to work on arm64, at least the go branch which I have tested.

kevinGC commented 3 years ago

This should be fixed by #5665. I can build on the go branch via:

GO111MODULE=on GOARCH=386 CGO_ENABLED=0 go build gvisor.dev/gvisor/pkg/tcpip/...
bradfitz commented 3 years ago

@kevinGC, thanks! I can confirm it works. I just built our application with GOOS=386 and it worked in netstack mode.

abarisani commented 3 years ago

I can confirm that building now works, however we still have to work around sync/atomic bugs, see:

https://github.com/f-secure-foundry/gvisor/commit/c18d73317e0f8d228bbb91ee790b18aaaf52ae3e https://github.com/f-secure-foundry/gvisor/commit/949d1f7623ed059511021fc7b93b121b50d0da1a

amscanne commented 3 years ago

The (language spec is not helpful in this area.

However, I strongly suspect a struct like this:

type Foo struct { value int64 }

would normally have an 8-byte alignment if allocated on its own (just because of the heap pool).

We must be hitting this issue just because we have these structs embedded in other structs that are causing alignment issues. The easy solution is to change the code to use *Foo for these kinds of values that must be aligned (and document that requirement on the struct), and not pack in 15-bytes. Another decent option is to perhaps allow the checkatomic analyzer (not yet in) to support alignment checks and pad the structures ourselves -- I will think if this is possible.

prattmic commented 3 years ago

https://go101.org/article/memory-layout.html discusses this issue, plus possible workarounds near the end (including heap allocation or large slices). There are a few Go proposals to fix this, but nothing accepted.