Support plugin network stack

[amysaq2023]

This commit supports a third-party network stack as a plugin stack for
gVisor.

The overall plugin package structure is the following:

• pkg/sentry/socket/plugin:
  Interfaces for initializing plugin network stack. It will be used
  in network setting up during sandbox creating.

• pkg/sentry/socket/plugin/stack:
  Glue layer for plugin stack's socket and stack ops with sentry. It
  will also register plugin stack operations if imported.

• pkg/sentry/socket/plugin/cgo:
  Interfaces defined in C for plugin network stack to support.

To build target runsc-plugin-stack, which imports
pkg/sentry/socket/plugin/stack package and enables CGO:

bazel build --config=cgo-enable runsc:runsc-plugin-stack

By using runsc-plugin-stack binary and setting "--network=plugin" in
runtimeArgs, user can use third-party network stack instead of
netstack embedded in gVisor to get better network performance.

Redis benchmark with following setups:

1. KVM platform
2. 4 physical cores for target pod
3. target pod as redis server

Runc:
$redis-benchmark -h [target ip] -n 100000 -t get,set -q
SET: 115207.38 requests per second, p50=0.215 msec
GET: 92336.11 requests per second, p50=0.279 msec

$redis-benchmark -h [target ip] -n 100000 -t get,set -q
SET: 113895.21 requests per second, p50=0.247 msec
GET: 96899.23 requests per second, p50=0.271 msec

$redis-benchmark -h [target ip] -n 100000 -t get,set -q
SET: 126582.27 requests per second, p50=0.199 msec
GET: 95969.28 requests per second, p50=0.271 msec

Runsc with plugin stack:
$redis-benchmark -h [target ip] -n 100000 -t get,set -q
SET: 123915.74 requests per second, p50=0.343 msec
GET: 115473.45 requests per second, p50=0.335 msec

$redis-benchmark -h [target ip] -n 100000 -t get,set -q
SET: 120918.98 requests per second, p50=0.351 msec
GET: 117647.05 requests per second, p50=0.351 msec

$redis-benchmark -h [target ip] -n 100000 -t get,set -q
SET: 119904.08 requests per second, p50=0.367 msec
GET: 112739.57 requests per second, p50=0.375 msec

Runsc with netstack:
$redis-benchmark -h [target ip] -n 100000 -t get,set -q
SET: 59952.04 requests per second, p50=0.759 msec
GET: 61162.08 requests per second, p50=0.631 msec

$redis-benchmark -h [target ip] -n 100000 -t get,set -q
SET: 52219.32 requests per second, p50=0.719 msec
GET: 58719.91 requests per second, p50=0.663 msec

$redis-benchmark -h [target ip] -n 100000 -t get,set -q
SET: 59952.04 requests per second, p50=0.751 msec
GET: 60827.25 requests per second, p50=0.751 msec

Updates #9266

Co-developed-by: Tianyu Zhou wentong.zty@antgroup.com
Signed-off-by: Anqi Shen amy.saq@antgroup.com

[kevinGC]

Great start, looking forward to it being filled out more in subsequent PRs.

General notes so far:

• Happy to see init-time registration.
• All the public functions and types will need comments (useful, plus our static checks yell at us about it).

[kevinGC]

Sorry for my lateness, looking now.

[kevinGC]

This is a partial review -- I haven't finished pkg/sentry/socket yet. But I don't want to hold you up more than I already have, so here's what I have so far.

[kevinGC]

Same as above: I don't think the plugin should affect the gofer filters.

[amysaq2023]

@kevinGC Sorry, I just learned that you're out sick at the moment. There is no rush on reviewing this PR and please ignore the PING. Wish you all the best and get well soon.

[kevinGC]

More partial review.

[kevinGC]

Another partial review (32/43 files done so far).

[kevinGC]

I don't think you need this variable. Can this just be:

	if e == linuxerr.ErrWouldBlock || e == linuxerr.EAGAIN {
		e = linuxerr.ErrWouldBlock
	}

	return syserr.FromError(e)

[kevinGC]

We can actually delete this whole function and instead use syserr.FromError(e) directly.

linuxerr.ErrWouldBlock and linuxerr.EAGAIN are the same, so this does nothing. Plus converting linuxerr.ErrWouldBlock to linuxerr.ErrWouldBlock wasn't very helpful :)

[kevinGC]

Why does the fsgofer need extra permissions?

[kevinGC]

Bumping this -- I don't understand why the fsgofer would need more permissions.

[amysaq2023]

Hi Kevin, sorry for confusion. It seems that I reply this reason to some other comments. Here is the link for why we currently add more permissions to fsgofer: #9551 (comment)
In brief, as we import cgo package for plugin stack, goruntime will set the global variable iscgo to true and all newm1() will call cgo_thread_start, which needs more permissions here.

[kevinGC]

Thanks, many fewer comments this time 👍

[kevinGC]

We can actually delete this whole function and instead use syserr.FromError(e) directly.

linuxerr.ErrWouldBlock and linuxerr.EAGAIN are the same, so this does nothing. Plus converting linuxerr.ErrWouldBlock to linuxerr.ErrWouldBlock wasn't very helpful :)

[kevinGC]

Hey, still waiting for an answer about the fsgofer syscall filters. Plus one comment about external dependencies.

I know this has been a slow process, but we're almost done.

[kevinGC]

We've talked about this internally: I think we would prefer that this chunk be maintained by you as a patch. It doesn't really fit with our other OSS dependencies, and getting it to work with the internal build tooling may be challenging.

[amysaq2023]

Hi Kevin, we are a little bit confused here about what is expected to do to "maintaining the chunk as a patch".
Could you please explain a little more on how this external dependency is expected to be maintained and/or probably could we find some similar cases? Thank you so much.

[kevinGC]

There is no similar case. gVisor tries to limit our external dependencies, and those that we utilize tend to be small or from widely used open source projects. This repo, on the other hand, is a substantial chunk of code that seems to be maintained and used exclusively by you.

Keeping this dependency in the repo would:

• Force open source users to pull in a large chunk of code that's not widely used or audited.
• Force the team members at Google to pull this repo into our internal codebase. This can be difficult, especially for a large repo that (again) isn't widely used.

I believe all you'd have to do is maintain this as a ~20 line patch to be applied when syncing with this repo.

[amysaq2023]

Hi Kevin, just to make it more clear, as I understand, what we want to do here is to avoid keeping cloning the whole tldk repo into local codebase without checking whether we actually will use plugin stack or not.
So we probably want to keep the compile process of plugin stack lib in plugin stack repo itself and only export a C library which will be imported as a dependency in cgo. Please correct me if I misunderstand this. Thanks a lot.

[kevinGC]

Bumping this -- I don't understand why the fsgofer would need more permissions.

[amysaq2023]

Hi @kevinGC , just coming back to checkout whether there is any comment on how we currently handle the plugin stack dependency. Again, thanks for any advice :)

[kevinGC]

I think we'll live with the dependency as-is.

[EtiennePerot]

An update here: this is difficult to merge internally due to this PR being the first to use the cdeps, clinkopts, and copts arguments of the go_library rule, which works slightly differently than Bazel's rules_go version. This requires internal work to support these.

There's no action needed from your end, just wanted to comment on why this is taking a while.

[EtiennePerot]

Hi and thanks so much for putting up with the long wait!

@avagin has been working on making this PR fit within the internal build systems and it's, well, complicated. We have a path forward now but it will require editing the files in this PR. I've left some comments about the specific changes that need to happen. This is probably not the last set of changes needed, but they will already makes the build systems happier.

Thanks in advance!

[EtiennePerot]

Please add --define=network_plugins=true at the end of this line.

[EtiennePerot]

Please add a config setting here like so:

config_setting(
    name = "network_plugins",
    values = {"define": "network_plugins=true"},
)

[EtiennePerot]

After #10938 is submitted, please rename the arguments as follows:

• cdeps -> cgo_cdeps
• copts -> cgo_copts
• clinkopts -> cgo_clinkopts

Then also add a new argument called bazel_cgo, set depending on the config_setting like so:

    bazel_cgo = select({
        ":network_plugins": True,
        "//conditions:default": False,
    }),

[EtiennePerot]

Please add the following two comments above package cgo, making sure to leave a blank line before and after:

 // See the License for the specific language governing permissions and
 // limitations under the License.
+
+//go:build network_plugins
+// +build network_plugins
+
 package cgo

[EtiennePerot]

Please add the same two comments above the package docstring like so, making sure to leave a blank line before and after:

 // [...]
 // See the License for the specific language governing permissions and
 // limitations under the License.
+
+//go:build network_plugins
+// +build network_plugins
+
 // Package cgo provides interfaces definition to interact with third-party
 // [...]
 package cgo

[EtiennePerot]

Please create a new file named nocgo_stub_unsafe.go in this directory. It should have the following header:

// Copyright 2024 The gVisor Authors.
// [...]
// See the License for the specific language governing permissions and
// limitations under the License.

//go:build !network_plugins
// +build !network_plugins

package cgo

For every exported function (i.e. function starting with an uppercase letter) in socket_unsafe.go / stack_unsafe.go / util_unsafe.go, please create a stub function in this new nocgo_stub_unsafe.go that panics like so:

// PreInitStack is only implemented when network plugins are enabled.
func PreInitStack(pid int) (string, []int, error) {
	panic("unimplemented")
}

// ... Repeat for every exported function.

In other words, this new nocgo_stub_unsafe.go file should allow the cgo package to be built and have the same exported symbols as would be exported by socket_unsafe.go + stack_unsafe.go + util_unsafe.go.

// GetPtr is only implemented when network plugins are enabled.
func GetPtr(bs []byte) unsafe.Pointer {
	panic("unimplemented")
}

// EpollCreate is only implemented when network plugins are enabled.
func EpollCreate() int {
	panic("unimplemented")
}

// EpollCtl is only implemented when network plugins are enabled.
func EpollCtl(epfd int32, op int, handle, events uint32) {
	panic("unimplemented")
}

// EpollWait is only implemented when network plugins are enabled.
func EpollWait(epfd int32, events []syscall.EpollEvent, n int, us int) int {
	panic("unimplemented")
}

// Socket is only implemented when network plugins are enabled.
func Socket(domain, skType, protocol int) int64 {
	panic("unimplemented")
}

// Bind is only implemented when network plugins are enabled.
func Bind(handle uint32, sa []byte) int64 {
	panic("unimplemented")
}

// Listen is only implemented when network plugins are enabled.
func Listen(handle uint32, backlog int) int64 {
	panic("unimplemented")
}

// Accept is only implemented when network plugins are enabled.
func Accept(handle uint32, addrPtr *byte, lenPtr *uint32) int64 {
	panic("unimplemented")
}

// Ioctl is only implemented when network plugins are enabled.
func Ioctl(handle uint32, cmd uint32, buf []byte) int64 {
	panic("unimplemented")
}

// Connect is only implemented when network plugins are enabled.
func Connect(handle uint32, addr []byte) int64 {
	panic("unimplemented")
}

// Getsockopt is only implemented when network plugins are enabled.
func Getsockopt(handle uint32, l int, n int, val []byte, s int) (int64, int) {
	panic("unimplemented")
}

// Setsockopt is only implemented when network plugins are enabled.
func Setsockopt(handle uint32, l int, n int, val []byte) int64 {
	panic("unimplemented")
}

// Shutdown is only implemented when network plugins are enabled.
func Shutdown(handle uint32, how int) int64 {
	panic("unimplemented")
}

// Close is only implemented when network plugins are enabled.
func Close(handle uint32) {
	panic("unimplemented")
}

// Getsockname is only implemented when network plugins are enabled.
func Getsockname(handle uint32, addr []byte, addrlen *uint32) int64 {
	panic("unimplemented")
}

// GetPeername is only implemented when network plugins are enabled.
func GetPeername(handle uint32, addr []byte, addrlen *uint32) int64 {
	panic("unimplemented")
}

// Readiness is only implemented when network plugins are enabled.
func Readiness(handle uint32, mask uint64) int64 {
	panic("unimplemented")
}

// Read is only implemented when network plugins are enabled.
func Read(handle uint32, buf uintptr, count int) int64 {
	panic("unimplemented")
}

// Readv is only implemented when network plugins are enabled.
func Readv(handle uint32, iovs []syscall.Iovec) int64 {
	panic("unimplemented")
}

// Recvfrom is only implemented when network plugins are enabled.
func Recvfrom(handle uint32, buf, addr []byte, flags int) (int64, int) {
	panic("unimplemented")
}

// Recvmsg is only implemented when network plugins are enabled.
func Recvmsg(handle uint32, iovs []syscall.Iovec, addr, control []byte, flags int) (int64, int, int, int) {
	panic("unimplemented")
}

// Write is only implemented when network plugins are enabled.
func Write(handle uint32, buf uintptr, count int) int64 {
	panic("unimplemented")
}

// Writev is only implemented when network plugins are enabled.
func Writev(handle uint32, iovs []syscall.Iovec) int64 {
	panic("unimplemented")
}

// Sendto is only implemented when network plugins are enabled.
func Sendto(handle uint32, buf uintptr, count int, flags int, addr []byte) int64 {
	panic("unimplemented")
}

// Sendmsg is only implemented when network plugins are enabled.
func Sendmsg(handle uint32, iovs []syscall.Iovec, addr []byte, flags int) int64 {
	panic("unimplemented")
}

// InitStack is only implemented when network plugins are enabled.
func InitStack(initStr string, fds []int) error {
	panic("unimplemented")
}

// PreInitStack is only implemented when network plugins are enabled.
func PreInitStack(pid int) (string, []int, error) {
	panic("unimplemented")
}

[EtiennePerot]

Once you have created nocgo_stub_unsafe.go, add it to the list of srcs.

[EtiennePerot]

Please edit Makefile in this directory, and change the unit-tests target to the following:

unit-tests: ## Local package unit tests in pkg/..., tools/.., etc.
	@$(call test,--test_tag_filters=-nogo$(COMMA)-requires-kvm --build_tag_filters=-network_plugins -- //:all pkg/... tools/... runsc/... vdso/... test/trace/... -//pkg/metric:metric_test -//pkg/coretag:coretag_test -//runsc/config:config_test -//tools/tracereplay:tracereplay_test -//test/trace:trace_test)
.PHONY: unit-tests

(The diff is the addition of --build_tag_filters=-network_plugins.)

[EtiennePerot]

Please edit .buildkite/pipeline.yaml and update the "build runsc" steps to exclude the tag like this:

  # Build runsc and pkg (presubmits only).
  - <<: *common
    <<: *source_test_presubmit
    label: ":world_map: Build runsc and pkg (AMD64)"
    commands:
      - "make build TARGETS=//pkg/..."
      - "make build TARGETS='--build_tag_filters=-network_plugins //runsc/...'"
    agents:
      arch: "amd64"

  - <<: *common
    <<: *source_test_presubmit
    label: ":world_map: Build runsc and pkg (ARM64)"
    commands:
      - "make build TARGETS=//pkg/..."
      - "make build TARGETS='--build_tag_filters=-network_plugins //runsc/...'"
    agents:
      arch: "arm64"

  # Build everything (continuous only).
  - <<: *common
    <<: *source_test_continuous
    label: ":world_map: Build everything"
    commands:
      - "make build TARGETS='--build_tag_filters=-network_plugins //...'"

(The diff is the addition of --build_tag_filters=-network_plugins.)

[EtiennePerot]

In the repo's top-level BUILD file, please add this to the go_path:

 go_path(
     name = "gopath",
     mode = "archive",
     deps = [
         # [...]
         "//third_party/gvisor/tools/checklocks",
 
         # Packages that are not dependencies of the above.
+        "//third_party/gvisor/pkg/sentry/socket/plugin/stack",
         "//third_party/gvisor/pkg/sentry/kernel/memevent",
         "//third_party/gvisor/pkg/tcpip/adapters/gonet",
         "//third_party/gvisor/pkg/tcpip/faketime",
         "//third_party/gvisor/pkg/tcpip/link/channel",
         "//third_party/gvisor/pkg/tcpip/link/ethernet",

[EtiennePerot]

Replace with:

package(
    default_applicable_licenses = ["//:license"],
    licenses = ["notice"],
)

[EtiennePerot]

Replace with:

package(
    default_applicable_licenses = ["//:license"],
    licenses = ["notice"],
)

[EtiennePerot]

Replace with:

package(
    default_applicable_licenses = ["//:license"],
    licenses = ["notice"],
)

[EtiennePerot]

@avagin applied the above changes and merged the modified PR, so no need to do the above changes anymore :)

But the important thing is... it's merged! Yay 🎉

Thanks a lot for all the work on this PR and sorry about the long wait!

[avagin]

Unfortunately, we couldn't merge this PR as-is due to some necessary internal changes. However, we've preserved the original author information, so all credit still goes to you. You can view the commit in the master branch here: 56f2530

I probably could break something with my changes. I am sorry if it happened. Let's fix all findings on top of the master branch. Plus, we need to introduce a buildkite workflow to test the network plugin.

[amysaq2023]

@EtiennePerot @avagin @kevinGC Thanks a lot for reviewing this patch and all the work to make it work with the internal build system.