cmd/link: lock down future uses of linkname #67401

rsc · 2024-05-15T21:07:00Z

Overuse of //go:linkname to reach into Go standard library internals (especially runtime internals) means that when we do change the standard library internals in ways that should not matter, we can end up breaking packages that are depended on by a large swath of the Go ecosystem. For example, https://go.dev/cl/583756 broke github.com/goccy/go-json because it turns out that package copied most of the runtime's internal type API. Now we can't change anything in that list, despite that being an ostensibly internal package, without breaking goccy/go-json. And goccy is used by many packages, including Kubernetes.

This situation is unsustainable. Internals are internal for a reason. We can't keep Go programs working when they create explicit dependencies on details that we have kept internal. But we also care a lot about compatibility: we don't want to break Go programs either. The obvious conclusion is that we have to stop Go programs from being able to create these dependencies on internal details in the first place.

This issue tracks work to prevent new //go:linkname-based dependencies and contain existing ones.

Right now, if package A has a symbol and package B wants to refer to it with //go:linkname, there are three patterns:

(Push) Package A uses a //go:linkname to rename one of its own symbols to B.foo, and then B declares func foo() without a body. In this form, A clearly intends for B to use foo, although the compiler cannot quite tell what's going on in B and warns about foo not having a body unless you create an empty dummy.s file.
(Pull) Package A defines foo without any annotation, and package B uses //go:linkname to access A.foo. In this form, A may not intend for B to use foo at all. That's a serious problem: when A renames foo and/or changes its type signature, B breaks, and A may never even have heard of B.
(Handshake) Package A defines foo with a //go:linkname and package B defines foo also with a //go:linkname, and the two agree on the name (either A.foo or B.foo). This is the ideal form, and it avoids the dummy.s workaround that is needed in the Push case.

The ideal goal state is a world where all //go:linkname usage must be in the Handshake form: both sides must agree to use linkname for a given symbol in order for it to succeed. This will mean that arbitrary packages cannot create new dependencies on runtime internals. At the same time, we realize that the current world is not this ideal world, and we don't want to break all existing uses.

Our plan is as follows.

Introduce a new -checklinkname=1 flag to cmd/link that requires the Handshake form for symbols in the standard library. That flag is already landed in at tip, but it is not the default.
Survey all existing open-source Go packages to find standard library symbols that are being //go:linkname'd (behind our backs!) using the Pull pattern. Add the necessary //go:linkname annotations to the standard library to keep those working, documenting why each exists. The explicit //go:linkname lines and documentation will help avoid accidental breakage in future refactoring. We have done a preliminary survey, but we haven't yet added all the necessary //go:linkname lines.
Make -checklinkname=1 the default for Go 1.23. If this breaks anything, users can use -ldflags=-checklinkname=0 to get unbroken, and we hope they will also file reports letting us know what we missed.
As we get reports of additional breakage we missed, add more //go:linkname annotations to the standard library.

At the completion of that plan, we won't be in the ideal world, but we will have accomplished two important things:

We won't have broken anything.
We will have stopped new damage from accumulating: there will be no more new references to runtime internals introduced. In particular, new internals we added during the Go 1.23 cycle, like coro and weak pointers, cannot be linknamed, now or ever. And anything that wasn't linknamed yet won't grow new linknames in the future.

Note that anyone who wants to experiment can always build with -ldflags=-checklinkname=0 and linkname whatever they like. That's fine. We like experimenting too. But the fact that the code won't build without special flags should help prevent code that digs into internal details from becoming a core dependency in the Go ecosystem that we end up having to maintain forever.

Note also that for now, //go:linkname can still be used in Pull mode to get at internals of non-standard library packages. We'd like to change that eventually too, insisting on Handshakes everywhere. For now, we are starting with the standard library. If all goes well, we'll circle back and try to devise a plan for the rest of the ecosystem.

The text was updated successfully, but these errors were encountered:

gopherbot · 2024-05-15T21:45:34Z

Change https://go.dev/cl/585820 mentions this issue: runtime/coverage: remove uses of //go:linkname

Jorropo · 2024-05-15T21:46:59Z

I think github.com/goccy/go-json pulled internal runtime APIs in a reckless manner and there are safe ways to pull internal packages from the std.
For example quic-go did an equally dangerous thing with crypto/tls by accessing private fields using unsafe.Pointer and maintaining forks of crypto/tls:

However key differences:

it was guarded by build tags on the go version
it had a future proof fallback which created a compile time error linking to documentation explaining the situation

As a downstream of quic-go I clearly understood the situation, the worst was the need to wait a couple of days for quic-go to release a new release compatible with the new go release before updating my go toolchain and the inability to run on tip or RCs.

In the case of github.com/goccy/go-json the situation could have been even better than quic-go's as it claims to be compatible with encoder/json:

Fast JSON encoder/decoder compatible with encoding/json for Go

instead of creating a compile time error they could have stubbed their API by forwarding calls to encoding/json when the release of go were to be unknown. (I don't know the details, maybe due to some edge cases or behaviors only go-json implements this wouldn't have been possible)

What I actually propose:

Continue to allow the Pull kind of linkname from std packages when the file is locked with build tags:

//go:build go1.23.4 && !go1.23.5 && !go1.24

To know if a file is properly locked down, the toolchain can evaluate the build tags with the next future release and it MUST fail to be allowed. That means if a file pass the current version but not the next one, then Pull linknames from the std would be allowed.

There is a downside to this approach which is that if there is no change required between two releases you still need a different file per version with the same implementation, to satisfy this constraint. Maybe parsing the build tags would be better.

I am not sure if goX.Y or goX.Y.Z should be used. goX.Y might be more dangerous in case a fix require breaking some internal API, quic-go used that and it was fine and created this couple of days period where you can't use latest go only every 6 months (note that goX.Y.Z build tags didn't existed back then).

rsc · 2024-05-15T22:51:28Z

I think ... there are safe ways to pull internal packages from the std.

I completely disagree. Quic-go's use of linkname caused all manner of problems for us release after release too, because anyone using quic-go couldn't update to a new Go version until quic-go did.

gopherbot · 2024-05-15T23:07:25Z

Change https://go.dev/cl/585916 mentions this issue: internal/coverage/cfile: remove //go:linkname into testing

gopherbot · 2024-05-15T23:07:26Z

Change https://go.dev/cl/585915 mentions this issue: internal/coverage/cfile: remove more //go:linkname usage

randall77 · 2024-05-16T01:16:42Z

We can't fix what we don't know about.

As we get reports of additional breakage we missed, add more //go:linkname annotations to the standard library.

We're open to any reports from closed-source packages. It would be particularly useful to hear about these when the release candidate comes out so they can make the .0 release.

ruyi789 · 2024-05-16T03:56:21Z

Misuse is undesirable, disabling is even worse, you want to change you maintain that package (go-json), that doesn't solve it?

bjorndm · 2024-05-16T11:22:06Z

I would like to say that //go:linkname is very useful for certain types of low level programming such as Ebitengine , etc. While I agree the situation with goccy/go-json is bad, we should look at how it is being used in detail and think of alternatives for the legitimate uses.

DmitriyMV · 2024-05-16T12:08:09Z

@bjorndm should't -ldflags=-checklinkname=0 which disables this check be enough for low level programming? And if you can always raise a proposal if something is missing and truly needed without ld flags.

As we get reports of additional breakage we missed, add more //go:linkname annotations to the standard library.

cherrymui · 2024-05-16T14:26:41Z

In C, static symbols are not accessible outside of the compilation unit, full stop. There is no way to pull a static symbol from a C library. A number of other languages have similar strict visibility rules. They are very successful languages and are widely used. This suggests that a lot programs can be written and things can go very well without a mechanism to break into a library's internal details.

I don't think Go is fundamentally different. Ideally we could also have strict visibility rules. I would think Go unexported symbols are meant to be similar to C static symbols. In fact, that is what gccgo does. Unfortunately for the gc toolchain it is not the case today. But we can get closer to it. And as we care a lot about compatibility, we'll keep the existing code continue to build in Go 1.23 (Step 2 in the plan). And we have a linker flag to disable the restriction (e.g. for experiments; as far as I know, the C linker doesn't seem to have such an option).

Also, I think the authors of the code should have a way to decide which symbols are visible externally and which are not.

TotallyGamerJet · 2024-05-16T16:29:49Z

Purego which is a dependency of Oto, Beep, and Ebitengine as well as others doesn't just pull symbols it also pushes since it reimplements runtime/cgo package when CGO_ENABLED=0 entirely in Go. Is there any way the symbols defined in the runtime that hook into that package also get comments to avoid breaking us?

Another potential solution for us is to prebuild runtime/cgo into a cgo_GOOS_GOARCH_GOVERSION.syso that ships with purego. That would save us from having to keep up with any changes that package has and allows the Go team the freedom to change it as they like. Is this actually possible? I tried with setting different -buildmode but none of them would link.

Of course, if the Go team wanted to port runtime/cgo to Go that would be optimal.

cherrymui · 2024-05-16T19:56:53Z

Is there any way the symbols defined in the runtime that hook into that package also get comments to avoid breaking us?

Push linknames are still allowed. If they are currently pushed from runtime/cgo, I believe you can still push them from Purego. Does Purego push symbols more than runtime/cgo?

Of course, if the Go team wanted to port runtime/cgo to Go that would be optimal.

This might be a possible option, but I think we need to understand the rationales better. The runtime/cgo package is intended to work with cgo, that is, interacting with C code. I'm not sure I understand the use case of runtime/cgo with CGO_ENABLED=0. This is probably better to be a separate discussion. Thanks.

TotallyGamerJet · 2024-05-16T21:18:25Z

Push linknames are still allowed. If they are currently pushed from runtime/cgo, I believe you can still push them from Purego. Does Purego push symbols more than runtime/cgo?

No, Purego pushes the same symbols that runtime/cgo does which means the "Handshake" is already satisfied for those. Step 2 of the suggested plan only mentions surveying for Pull linknames and marking them with comments to avoid future breakage. I'm wondering if there is any plans for Pushes as changes to those would break Purego?

This might be a possible option, but I think we need to understand the rationales better. The runtime/cgo package is intended to work with cgo, that is, interacting with C code. I'm not sure I understand the use case of runtime/cgo with CGO_ENABLED=0. This is probably better to be a separate discussion. Thanks.

Indeed, runtime/cgo is required to allow Go code and C code to play well with each other. Purego provides an entirely Go version of it so that you can call C code using purego.Dlopen and purego.Dlsym without the need of a C compiler so cross-compiling is again possible. We can discuss this further elsewhere.

cherrymui · 2024-05-16T21:23:06Z

Thanks.

I'm wondering if there is any plans for Pushes as changes to those would break Purego?

I don't think there is any plan to break the use case of Purego's pushes. If we do anything to restrict push-only ones, they will be equally applied to the ones runtime/cgo pushing to runtime. So we'll need to fix those first (I think many of them are already in handshake form, but it is possible we missed some). And that should make Purego work as well.

gopherbot · 2024-05-17T16:14:01Z

Change https://go.dev/cl/586259 mentions this issue: runtime: move exit hooks into internal/runtime/exithook

gopherbot · 2024-05-17T16:29:41Z

Change https://go.dev/cl/586137 mentions this issue: all: add push linknames to allow legacy pull linknames

Recent changes [1] in go liker breaks `anet` build with go 1.23.0. An information about how to deal with it should be helpful. [1] golang/go#67401

The Go runtime has started to [lock down future uses of linkname][1] since go1.23. In the go source code, containerd project has been marked in the comment, [hall of shame][2]. Well, the go:linkname is used to fork no-op subprocess efficiently. However, since that comment, I would like to use ptrace and remove go:linkname in the whole repository. With go1.22 `go:linkname`: ```bash $ go test -bench=. -benchmem ./ -exec sudo goos: linux goarch: amd64 pkg: github.com/containerd/containerd/v2/core/mount cpu: AMD Ryzen 7 5800H with Radeon Graphics BenchmarkBatchRunGetUsernsFD_Concurrent1-16 2440 533320 ns/op 1145 B/op 43 allocs/op BenchmarkBatchRunGetUsernsFD_Concurrent10-16 342 3661616 ns/op 11562 B/op 421 allocs/op PASS ok github.com/containerd/containerd/v2/core/mount 2.983s ``` With go1.22 `ptrace`: ```bash $ go test -bench=. -benchmem ./ -exec sudo goos: linux goarch: amd64 pkg: github.com/containerd/containerd/v2/core/mount cpu: AMD Ryzen 7 5800H with Radeon Graphics BenchmarkBatchRunGetUsernsFD_Concurrent1-16 1785 739557 ns/op 3948 B/op 68 allocs/op BenchmarkBatchRunGetUsernsFD_Concurrent10-16 328 4024300 ns/op 39601 B/op 671 allocs/op PASS ok github.com/containerd/containerd/v2/core/mount 3.104s ``` With go1.23 `ptrace`: ```bash $ go test -bench=. -benchmem ./ -exec sudo goos: linux goarch: amd64 pkg: github.com/containerd/containerd/v2/core/mount cpu: AMD Ryzen 7 5800H with Radeon Graphics BenchmarkBatchRunGetUsernsFD_Concurrent1-16 1815 723252 ns/op 4220 B/op 69 allocs/op BenchmarkBatchRunGetUsernsFD_Concurrent10-16 319 3957157 ns/op 42351 B/op 682 allocs/op PASS ok github.com/containerd/containerd/v2/core/mount 3.051s ``` Diff: The `ptrace` is slower than `go:linkname` mode. However, it's accepctable. ``` goos: linux goarch: amd64 pkg: github.com/containerd/containerd/v2/core/mount cpu: AMD Ryzen 7 5800H with Radeon Graphics │ go122-golinkname │ go122-ptrace │ go123-ptrace │ │ sec/op │ sec/op vs base │ sec/op vs base │ BatchRunGetUsernsFD_Concurrent1-16 533.3µ ± ∞ ¹ 739.6µ ± ∞ ¹ ~ (p=1.000 n=1) ² 723.3µ ± ∞ ¹ ~ (p=1.000 n=1) ² BatchRunGetUsernsFD_Concurrent10-16 3.662m ± ∞ ¹ 4.024m ± ∞ ¹ ~ (p=1.000 n=1) ² 3.957m ± ∞ ¹ ~ (p=1.000 n=1) ² geomean 1.397m 1.725m +23.45% 1.692m +21.06% ¹ need >= 6 samples for confidence interval at level 0.95 ² need >= 4 samples to detect a difference at alpha level 0.05 │ go122-golinkname │ go122-ptrace │ go123-ptrace │ │ B/op │ B/op vs base │ B/op vs base │ BatchRunGetUsernsFD_Concurrent1-16 1.118Ki ± ∞ ¹ 3.855Ki ± ∞ ¹ ~ (p=1.000 n=1) ² 4.121Ki ± ∞ ¹ ~ (p=1.000 n=1) ² BatchRunGetUsernsFD_Concurrent10-16 11.29Ki ± ∞ ¹ 38.67Ki ± ∞ ¹ ~ (p=1.000 n=1) ² 41.36Ki ± ∞ ¹ ~ (p=1.000 n=1) ² geomean 3.553Ki 12.21Ki +243.65% 13.06Ki +267.43% ¹ need >= 6 samples for confidence interval at level 0.95 ² need >= 4 samples to detect a difference at alpha level 0.05 │ go122-golinkname │ go122-ptrace │ go123-ptrace │ │ allocs/op │ allocs/op vs base │ allocs/op vs base │ BatchRunGetUsernsFD_Concurrent1-16 43.00 ± ∞ ¹ 68.00 ± ∞ ¹ ~ (p=1.000 n=1) ² 69.00 ± ∞ ¹ ~ (p=1.000 n=1) ² BatchRunGetUsernsFD_Concurrent10-16 421.0 ± ∞ ¹ 671.0 ± ∞ ¹ ~ (p=1.000 n=1) ² 682.0 ± ∞ ¹ ~ (p=1.000 n=1) ² geomean 134.5 213.6 +58.76% 216.9 +61.23% ¹ need >= 6 samples for confidence interval at level 0.95 ² need >= 4 samples to detect a difference at alpha level 0.05 ``` [1]: <golang/go#67401> [2]: <https:/golang/go/blob/release-branch.go1.23/src/runtime/proc.go#L4820> Signed-off-by: Wei Fu <[email protected]>

CoiaPrant233 · 2024-09-08T08:35:33Z

In fact, if linknames require handshaking, then why don't we export these symbols? This seems to defeat the purpose of linknames themselves.

In addition, when we use ldflags=-checklinkname=0 add a new tag like //go:build linkname, just like //go:build go1.23.
Or define ldflags=-checklinkname=0 in go.mod?

Before this commit, we used the private variable safeSet available in encoding/json through linkname. But, since golang 1.23, linkname is not authorized by default [1]. So, this commit introduces the safeset package which defines the SafeSet map. This map has the exact same value as encoding/json.safeSet [2]. It is also safe to add it in our sources, as this variable was not modified upstream since its introduction in 2016 [3]. This permits the whole project to be compiled using golang 1.23. Signed-off-by: Francis Laniel <[email protected]> [1]: golang/go#67401 [2]: https://cs.opensource.google/go/go/+/release-branch.go1.23:src/encoding/json/tables.go;l=15-112 [3]: https://cs.opensource.google/go/go/+/ed8f207940c8787d344664a43071b235e2ce5c68

MGLSUDE · 2024-09-22T12:26:37Z

Why not learn Android and limit reflection like that. Or don't include the go: linkname feature from the start.

rsc added the release-blocker label May 15, 2024

rsc added this to the Go1.23 milestone May 15, 2024

gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label May 15, 2024

This comment has been minimized.

Sign in to view

This comment was marked as outdated.

Sign in to view

hajimehoshi mentioned this issue May 16, 2024

check //go:linkname usages hajimehoshi/hitsumabushi#29

Open

eihigh mentioned this issue May 16, 2024

Check //go:linkname usage tenntenn/testtime#14

Closed

TotallyGamerJet mentioned this issue May 16, 2024

Check //go:linkname usage ebitengine/purego#247

Closed

6 tasks

hajimehoshi mentioned this issue May 16, 2024

confirm all the //go:linkname usages to be 'handshake' or 'push' ebitengine/purego#248

Closed

6 tasks

cuonglm mentioned this issue May 16, 2024

Check //go:linkname usage orijtech/structslop#64

Open

pkujhd mentioned this issue May 16, 2024

//go:linkname lock down future use pkujhd/goloader#98

Closed

dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 16, 2024

fuweid mentioned this issue Aug 19, 2024

core/mount: use ptrace instead of go:linkname containerd/containerd#10611

Merged

bobrofon added a commit to bobrofon/anet that referenced this issue Aug 22, 2024

Add section about go 1.23.0 to the readme

27971d0

Recent changes [1] in go liker breaks `anet` build with go 1.23.0. An information about how to deal with it should be helpful. [1] golang/go#67401

bobrofon added a commit to bobrofon/anet that referenced this issue Aug 22, 2024

Update README to include Go 1.23.0 section

ac7b7fd

Recent changes [1] in go liker breaks `anet` build with go 1.23.0. An information about how to deal with it should be helpful. [1] golang/go#67401

bobrofon mentioned this issue Aug 22, 2024

Update README to include Go 1.23.0 section wlynxg/anet#5

Merged

loafman-kangjun mentioned this issue Aug 26, 2024

关于 invalid reference to internal/poll.execIO 的解决方案 SagerNet/sing-box#2064

Closed

This comment was marked as off-topic.

Sign in to view

iDigitalFlame mentioned this issue Aug 29, 2024

Go! iDigitalFlame/XMT#1

Open

xiaost mentioned this issue Sep 3, 2024

runtime: remove github.com/cloudwego/frugal from linkname comment #69222

Closed

TorchedSammy mentioned this issue Sep 4, 2024

Doesn't Build in Go 1.23 Rosettea/Hilbish#326

Open

maypok86 mentioned this issue Sep 9, 2024

Stop importing nanotime from runtime via linkname jackc/puddle#35

Merged

eiffel-fl mentioned this issue Sep 11, 2024

pkg: Add safeset package. inspektor-gadget/inspektor-gadget#3472

Merged

royalcat mentioned this issue Sep 25, 2024

Fix linkname for go 1.23 Iceber/iouring-go#31

Open

xieyuschen mentioned this issue Oct 9, 2024

x/tools/go/packages: support for fake GOROOTs #69824

Open

multippt mentioned this issue Oct 10, 2024

Fix build issue on go1.23 lrita/numa#6

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/link: lock down future uses of linkname #67401

cmd/link: lock down future uses of linkname #67401

rsc commented May 15, 2024 •

edited

Loading

gopherbot commented May 15, 2024

Jorropo commented May 15, 2024 •

edited

Loading

rsc commented May 15, 2024

gopherbot commented May 15, 2024

gopherbot commented May 15, 2024

This comment has been minimized.

randall77 commented May 16, 2024

This comment has been minimized.

This comment was marked as outdated.

This comment was marked as outdated.

ruyi789 commented May 16, 2024

bjorndm commented May 16, 2024

DmitriyMV commented May 16, 2024

cherrymui commented May 16, 2024

TotallyGamerJet commented May 16, 2024 •

edited

Loading

cherrymui commented May 16, 2024

TotallyGamerJet commented May 16, 2024 •

edited

Loading

cherrymui commented May 16, 2024 •

edited

Loading

gopherbot commented May 17, 2024

gopherbot commented May 17, 2024

This comment was marked as off-topic.

This comment was marked as off-topic.

CoiaPrant233 commented Sep 8, 2024 •

edited

Loading

MGLSUDE commented Sep 22, 2024

cmd/link: lock down future uses of linkname #67401

cmd/link: lock down future uses of linkname #67401

Comments

rsc commented May 15, 2024 • edited Loading

gopherbot commented May 15, 2024

Jorropo commented May 15, 2024 • edited Loading

What I actually propose:

rsc commented May 15, 2024

gopherbot commented May 15, 2024

gopherbot commented May 15, 2024

This comment has been minimized.

randall77 commented May 16, 2024

This comment has been minimized.

This comment was marked as outdated.

This comment was marked as outdated.

ruyi789 commented May 16, 2024

bjorndm commented May 16, 2024

DmitriyMV commented May 16, 2024

cherrymui commented May 16, 2024

TotallyGamerJet commented May 16, 2024 • edited Loading

cherrymui commented May 16, 2024

TotallyGamerJet commented May 16, 2024 • edited Loading

cherrymui commented May 16, 2024 • edited Loading

gopherbot commented May 17, 2024

gopherbot commented May 17, 2024

This comment was marked as off-topic.

This comment was marked as off-topic.

CoiaPrant233 commented Sep 8, 2024 • edited Loading

MGLSUDE commented Sep 22, 2024

rsc commented May 15, 2024 •

edited

Loading

Jorropo commented May 15, 2024 •

edited

Loading

TotallyGamerJet commented May 16, 2024 •

edited

Loading

TotallyGamerJet commented May 16, 2024 •

edited

Loading

cherrymui commented May 16, 2024 •

edited

Loading

CoiaPrant233 commented Sep 8, 2024 •

edited

Loading