Replace unsafe.Slice with memory copying to avoid potential fault memory issue #6664

XinShuYang · 2024-09-12T03:58:37Z

Refactored ListIPForwardRows to deep copy IP forwarding table rows.
Removed unsafe.Slice and replaced with manual pointer dereferencing and copying.

This change addresses a potential fault memory issue when iterating through the IP forwarding table,
caused by the use of slices after corresponding memory has been freed, leading to access failure.

XinShuYang · 2024-09-12T04:24:42Z

/test-windows-all

XinShuYang · 2024-09-12T07:16:19Z

/test-windows-containerd-e2e

cmd/antrea-agent/agent.go

antoninbas

Please break down the PR into multiple ones as it is doing essentially 3 unrelated changes. You can then backport specifically what's needed using the normal backporting process, instead of opening an independent / "duplicate" PR (#6665).

cmd/antrea-agent/agent.go

antoninbas · 2024-09-12T16:45:42Z

pkg/agent/util/syscall/syscall_windows.go

-	return unsafe.Slice(&table.Table[0], table.NumEntries), nil
+	rows := make([]MibIPForwardRow, table.NumEntries, table.NumEntries)
+
+	pFirstRow := uintptr(unsafe.Pointer(&table.Table[0]))
+	rowSize := unsafe.Sizeof(table.Table[0])
+
+	for i := uint32(0); i < table.NumEntries; i++ {
+		row := *(*MibIPForwardRow)(unsafe.Pointer(pFirstRow + rowSize*uintptr(i)))
+		rows[i] = row
+	}
+	return rows, nil


does this fix a specific issue?

We received a report indicating that when host memory is less than 8GB, using unsafe.Slice can lead to memory faults. It’s possible that this function didn’t perform a deep copy, so after calling freeMibTable, we may encounter invalid memory addresses when access rows again. We have already verified that this deep copy method resolves this issue in the same environment.

XinShuYang · 2024-09-13T02:26:08Z

Please break down the PR into multiple ones as it is doing essentially 3 unrelated changes. You can then backport specifically what's needed using the normal backporting process, instead of opening an independent / "duplicate" PR (#6665).

@antoninbas Can I merge the agent crash fix and windows CI fix commits into one PR? Otherwise, the Windows CI pipeline will still fail due to rate limit issue.

antoninbas · 2024-09-13T16:50:28Z

Please break down the PR into multiple ones as it is doing essentially 3 unrelated changes. You can then backport specifically what's needed using the normal backporting process, instead of opening an independent / "duplicate" PR (#6665).

@antoninbas Can I merge the agent crash fix and windows CI fix commits into one PR? Otherwise, the Windows CI pipeline will still fail due to rate limit issue.

Yes, that's ok given that SNATFullyRandomPorts was introduced after the last minor release and the change doesn't need to be backported.

antoninbas · 2024-09-13T17:14:26Z

pkg/agent/util/syscall/syscall_windows.go

@@ -368,7 +399,16 @@ func (n *netIO) ListIPForwardRows(family uint16) ([]MibIPForwardRow, error) {
 	if err != nil {
 		return nil, os.NewSyscallError("iphlpapi.GetIpForwardTable", err)
 	}
-	return unsafe.Slice(&table.Table[0], table.NumEntries), nil


As far as I know the only issue with the previous code is that we were not making a copy of the slice prior to returning, while at the same time freeing the underlying memory by calling n.freeMibTable(unsafe.Pointer(table)).

The Wireguard Windows code has a simpler implementation of this, which still uses unsafe.Slice. It achieves correctness by making a proper copy of the slice: https://github.com/WireGuard/wireguard-windows/blob/e70799b1440690e7d4140bffc7c73baf903c7b54/tunnel/winipcfg/winipcfg.go#L151

That being said, if this new version has already been tested thoroughly, there is probably no string reason to change it.

@antoninbas , we used to think about the solution as what Wiregurad is taking, a concern is some fields in MibIPForwardRow are using type []byte, which still exists the risk with a shadow copy if the memory resource is limited (e.g., Windows OS re-allocate the memory to other objects after the MibIPForwardTable is free). So we finally choose the solution to deep copy the objects before free the memory. The latest code is verified on the problematic cluster.

Do you mean [N]byte types like the [26]byte field in RawSockAddrInet ([]byte would not be possible here AFAIK)? These should be no different than integer types, they should be inlined in the struct with no indirection.

The code before this change was wrong for obvious reasons. I don't think the Wireguard code is wrong. But if you feel like manual copy is the best approach and this has been well-tested, as I wrote before we can merge it as is.

The fields in MibIPForwardRow is array not slice. It will be deepcopied automatically.
And the current code is inconsistent: it makes a manual deepcopy for RawSockAddrInet but not AddressPrefix which also has a RawSockAddrInet.

An example: https://play.golang.com/p/z6Gc0YhEQSV

wenyingd

LGTM

XinShuYang · 2024-09-14T02:54:08Z

/test-windows-all

luolanzone · 2024-09-14T03:15:05Z

Hi @antoninbas @tnqn could you help to double check if we can more forward? this issue is a blocker for downstream customers. Thanks.

XinShuYang · 2024-09-14T03:38:58Z

/test-windows-containerd-e2e

antoninbas · 2024-09-14T03:37:51Z

pkg/agent/util/syscall/syscall_windows.go

@@ -368,7 +399,16 @@ func (n *netIO) ListIPForwardRows(family uint16) ([]MibIPForwardRow, error) {
 	if err != nil {
 		return nil, os.NewSyscallError("iphlpapi.GetIpForwardTable", err)
 	}
-	return unsafe.Slice(&table.Table[0], table.NumEntries), nil


Do you mean [N]byte types like the [26]byte field in RawSockAddrInet ([]byte would not be possible here AFAIK)? These should be no different than integer types, they should be inlined in the struct with no indirection.

The code before this change was wrong for obvious reasons. I don't think the Wireguard code is wrong. But if you feel like manual copy is the best approach and this has been well-tested, as I wrote before we can merge it as is.

antoninbas · 2024-09-14T03:41:29Z

pkg/agent/util/syscall/syscall_windows.go

+	sockData := [26]byte{}
+	copy(a.data[:], sockData[:])
+	return RawSockAddrInet{Family: a.Family, data: sockData}


return *a would make a copy given that data is an array, not a slice
am I missing something @tnqn ?

You are right. The whole function is what struct copy does exactly.

…ory issue * Refactored ListIPForwardRows to copy IP forwarding table rows. * Removed unsafe.Slice and replaced with manual pointer dereferencing and copying. This change addresses a potential fault memory issue when iterating through the IP forwarding table, caused by the use of slices after corresponding memory has been freed, leading to access failure. Signed-off-by: Shuyang Xin <[email protected]> Signed-off-by: Wenying Dong <[email protected]>

XinShuYang · 2024-09-14T04:09:04Z

Thanks for all comments @antoninbas @tnqn , I have updated the code based on the current Windows network management API implementation(https://github.com/tailscale/winipcfg-go/blob/84d0a1a3b210638dfe24d6169581a7af315c13f0/wt_mib_ipforward_row2.go#L51C1-L78C2).

tnqn

LGTM

tnqn · 2024-09-14T04:24:45Z

Please let me know once the verification is done

XinShuYang · 2024-09-14T04:30:22Z

/test-windows-all

tnqn · 2024-09-14T04:33:34Z

Thanks for all comments @antoninbas @tnqn , I have updated the code based on the current Windows network management API implementation(https://github.com/tailscale/winipcfg-go/blob/84d0a1a3b210638dfe24d6169581a7af315c13f0/wt_mib_ipforward_row2.go#L51C1-L78C2).

Both are correct, The code in wireguard windows is more concise.

XinShuYang · 2024-09-14T05:43:28Z

Please let me know once the verification is done

@tnqn Windows CI passed.

tnqn · 2024-09-14T06:00:31Z

/skip-all

tnqn · 2024-09-14T06:01:12Z

/skip-all

…ory issue (antrea-io#6664) * Refactored ListIPForwardRows to copy IP forwarding table rows. * Removed unsafe.Slice and replaced with manual pointer dereferencing and copying. This change addresses a potential fault memory issue when iterating through the IP forwarding table, caused by the use of slices after corresponding memory has been freed, leading to access failure. Signed-off-by: Shuyang Xin <[email protected]> Signed-off-by: Wenying Dong <[email protected]>

XinShuYang marked this pull request as draft September 12, 2024 04:02

XinShuYang force-pushed the fixwinsyscall branch 3 times, most recently from 602534b to eab0b6c Compare September 12, 2024 04:09

XinShuYang marked this pull request as ready for review September 12, 2024 04:09

XinShuYang force-pushed the fixwinsyscall branch from eab0b6c to cdbe6e0 Compare September 12, 2024 04:17

XinShuYang requested review from wenyingd, luolanzone, tnqn and antoninbas September 12, 2024 05:20

XinShuYang force-pushed the fixwinsyscall branch from cdbe6e0 to c531e9c Compare September 12, 2024 07:12

XinShuYang commented Sep 12, 2024

View reviewed changes

cmd/antrea-agent/agent.go Outdated Show resolved Hide resolved

antoninbas reviewed Sep 12, 2024

View reviewed changes

XinShuYang force-pushed the fixwinsyscall branch from c531e9c to 10d9351 Compare September 13, 2024 10:43

XinShuYang requested a review from antoninbas September 13, 2024 10:57

XinShuYang force-pushed the fixwinsyscall branch from 10d9351 to f003d82 Compare September 13, 2024 15:26

XinShuYang mentioned this pull request Sep 13, 2024

Resolve Windows agent crash issue caused by egressSNATRandomFully #6668

Merged

antoninbas reviewed Sep 13, 2024

View reviewed changes

wenyingd previously approved these changes Sep 14, 2024

View reviewed changes

XinShuYang force-pushed the fixwinsyscall branch from f003d82 to 6721cc6 Compare September 14, 2024 02:19

XinShuYang requested a review from antoninbas September 14, 2024 02:35

antoninbas previously approved these changes Sep 14, 2024

View reviewed changes

XinShuYang dismissed stale reviews from antoninbas and wenyingd via 10ba34b September 14, 2024 04:06

XinShuYang force-pushed the fixwinsyscall branch from 6721cc6 to 10ba34b Compare September 14, 2024 04:06

XinShuYang requested a review from antoninbas September 14, 2024 04:10

tnqn approved these changes Sep 14, 2024

View reviewed changes

tnqn merged commit b96d22f into antrea-io:main Sep 14, 2024
54 of 60 checks passed

XinShuYang mentioned this pull request Sep 14, 2024

Automated cherry pick of #6664: Replace unsafe.Slice with memory copying to avoid potential #6670

Open

XinShuYang mentioned this pull request Sep 14, 2024

Automated cherry pick of #6664: Replace unsafe.Slice with memory copying to avoid potential #6671

Open

XinShuYang mentioned this pull request Sep 14, 2024

Automated cherry pick of #6664: Replace unsafe.Slice with memory copying to avoid potential #6672

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace unsafe.Slice with memory copying to avoid potential fault memory issue #6664

Replace unsafe.Slice with memory copying to avoid potential fault memory issue #6664

XinShuYang commented Sep 12, 2024 •

edited

Loading

XinShuYang commented Sep 12, 2024

XinShuYang commented Sep 12, 2024

antoninbas left a comment

antoninbas Sep 12, 2024

XinShuYang Sep 13, 2024

XinShuYang commented Sep 13, 2024

antoninbas commented Sep 13, 2024

antoninbas Sep 13, 2024

wenyingd Sep 14, 2024

antoninbas Sep 14, 2024

tnqn Sep 14, 2024

tnqn Sep 14, 2024

wenyingd left a comment

XinShuYang commented Sep 14, 2024

luolanzone commented Sep 14, 2024

XinShuYang commented Sep 14, 2024

antoninbas Sep 14, 2024

antoninbas Sep 14, 2024

tnqn Sep 14, 2024

XinShuYang commented Sep 14, 2024 •

edited

Loading

tnqn left a comment

tnqn commented Sep 14, 2024

XinShuYang commented Sep 14, 2024

tnqn commented Sep 14, 2024

XinShuYang commented Sep 14, 2024

tnqn commented Sep 14, 2024

tnqn commented Sep 14, 2024

Replace unsafe.Slice with memory copying to avoid potential fault memory issue #6664

Replace unsafe.Slice with memory copying to avoid potential fault memory issue #6664

Conversation

XinShuYang commented Sep 12, 2024 • edited Loading

XinShuYang commented Sep 12, 2024

XinShuYang commented Sep 12, 2024

antoninbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XinShuYang commented Sep 13, 2024

antoninbas commented Sep 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenyingd left a comment

Choose a reason for hiding this comment

XinShuYang commented Sep 14, 2024

luolanzone commented Sep 14, 2024

XinShuYang commented Sep 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XinShuYang commented Sep 14, 2024 • edited Loading

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Sep 14, 2024

XinShuYang commented Sep 14, 2024

tnqn commented Sep 14, 2024

XinShuYang commented Sep 14, 2024

tnqn commented Sep 14, 2024

tnqn commented Sep 14, 2024

XinShuYang commented Sep 12, 2024 •

edited

Loading

XinShuYang commented Sep 14, 2024 •

edited

Loading