1 files changed, 148 insertions, 104 deletions

diff --git a/test/packetimpact/tests/udp_icmp_error_propagation_test.go b/test/packetimpact/tests/udp_icmp_error_propagation_test.go
index 9e4810842..66f64eaa2 100644
--- a/test/packetimpact/tests/udp_icmp_error_propagation_test.go
+++ b/test/packetimpact/tests/udp_icmp_error_propagation_test.go
@@ -27,9 +27,9 @@ import (
 	tb "gvisor.dev/gvisor/test/packetimpact/testbench"
 )
 
-type connected bool
+type connectionMode bool
 
-func (c connected) String() string {
+func (c connectionMode) String() string {
 	if c {
 		return "Connected"
 	}
@@ -63,147 +63,191 @@ func (e icmpError) ToICMPv4() *tb.ICMPv4 {
 	return nil
 }
 
-type errorDetectionFunc func(context.Context, *tb.DUT, *tb.UDPIPv4, int32, syscall.Errno) error
+type errorDetection struct {
+	name         string
+	useValidConn bool
+	f            func(context.Context, testData) error
+}
+
+type testData struct {
+	dut        *tb.DUT
+	conn       *tb.UDPIPv4
+	remoteFD   int32
+	remotePort uint16
+	cleanFD    int32
+	cleanPort  uint16
+	wantErrno  syscall.Errno
+}
 
-// testRecv tests observing the ICMP error through the recv syscall.
-// A packet is sent to the DUT, and if wantErrno is non-zero, then the first
-// recv should fail and the second should succeed. Otherwise if wantErrno is
-// zero then the first recv should succeed immediately.
-func testRecv(ctx context.Context, dut *tb.DUT, conn *tb.UDPIPv4, remoteFD int32, wantErrno syscall.Errno) error {
-	conn.Send(tb.UDP{})
+// testRecv tests observing the ICMP error through the recv syscall. A packet
+// is sent to the DUT, and if wantErrno is non-zero, then the first recv should
+// fail and the second should succeed. Otherwise if wantErrno is zero then the
+// first recv should succeed immediately.
+func testRecv(ctx context.Context, d testData) error {
+	// Check that receiving on the clean socket works.
+	d.conn.Send(tb.UDP{DstPort: &d.cleanPort})
+	d.dut.Recv(d.cleanFD, 100, 0)
 
-	if wantErrno != syscall.Errno(0) {
+	d.conn.Send(tb.UDP{})
+
+	if d.wantErrno != syscall.Errno(0) {
 		ctx, cancel := context.WithTimeout(ctx, time.Second)
 		defer cancel()
-		ret, _, err := dut.RecvWithErrno(ctx, remoteFD, 100, 0)
+		ret, _, err := d.dut.RecvWithErrno(ctx, d.remoteFD, 100, 0)
 		if ret != -1 {
-			return fmt.Errorf("recv after ICMP error succeeded unexpectedly, expected (%[1]d) %[1]v", wantErrno)
+			return fmt.Errorf("recv after ICMP error succeeded unexpectedly, expected (%[1]d) %[1]v", d.wantErrno)
 		}
-		if err != wantErrno {
-			return fmt.Errorf("recv after ICMP error resulted in error (%[1]d) %[1]v, expected (%[2]d) %[2]v", err, wantErrno)
+		if err != d.wantErrno {
+			return fmt.Errorf("recv after ICMP error resulted in error (%[1]d) %[1]v, expected (%[2]d) %[2]v", err, d.wantErrno)
 		}
 	}
 
-	dut.Recv(remoteFD, 100, 0)
+	d.dut.Recv(d.remoteFD, 100, 0)
 	return nil
 }
 
-// testSendTo tests observing the ICMP error through the send syscall.
-// If wantErrno is non-zero, the first send should fail and a subsequent send
+// testSendTo tests observing the ICMP error through the send syscall. If
+// wantErrno is non-zero, the first send should fail and a subsequent send
 // should suceed; while if wantErrno is zero then the first send should just
 // succeed.
-func testSendTo(ctx context.Context, dut *tb.DUT, conn *tb.UDPIPv4, remoteFD int32, wantErrno syscall.Errno) error {
-	if wantErrno != syscall.Errno(0) {
+func testSendTo(ctx context.Context, d testData) error {
+	// Check that sending on the clean socket works.
+	d.dut.SendTo(d.cleanFD, nil, 0, d.conn.LocalAddr())
+	if _, err := d.conn.Expect(tb.UDP{SrcPort: &d.cleanPort}, time.Second); err != nil {
+		return fmt.Errorf("did not receive UDP packet from clean socket on DUT: %s", err)
+	}
+
+	if d.wantErrno != syscall.Errno(0) {
 		ctx, cancel := context.WithTimeout(ctx, time.Second)
 		defer cancel()
-		ret, err := dut.SendToWithErrno(ctx, remoteFD, nil, 0, conn.LocalAddr())
+		ret, err := d.dut.SendToWithErrno(ctx, d.remoteFD, nil, 0, d.conn.LocalAddr())
 
 		if ret != -1 {
-			return fmt.Errorf("sendto after ICMP error succeeded unexpectedly, expected (%[1]d) %[1]v", wantErrno)
+			return fmt.Errorf("sendto after ICMP error succeeded unexpectedly, expected (%[1]d) %[1]v", d.wantErrno)
 		}
-		if err != wantErrno {
-			return fmt.Errorf("sendto after ICMP error resulted in error (%[1]d) %[1]v, expected (%[2]d) %[2]v", err, wantErrno)
+		if err != d.wantErrno {
+			return fmt.Errorf("sendto after ICMP error resulted in error (%[1]d) %[1]v, expected (%[2]d) %[2]v", err, d.wantErrno)
 		}
 	}
 
-	dut.SendTo(remoteFD, nil, 0, conn.LocalAddr())
-	if _, err := conn.Expect(tb.UDP{}, time.Second); err != nil {
+	d.dut.SendTo(d.remoteFD, nil, 0, d.conn.LocalAddr())
+	if _, err := d.conn.Expect(tb.UDP{}, time.Second); err != nil {
 		return fmt.Errorf("did not receive UDP packet as expected: %s", err)
 	}
 	return nil
 }
 
-func testSockOpt(_ context.Context, dut *tb.DUT, conn *tb.UDPIPv4, remoteFD int32, wantErrno syscall.Errno) error {
-	errno := syscall.Errno(dut.GetSockOptInt(remoteFD, unix.SOL_SOCKET, unix.SO_ERROR))
-	if errno != wantErrno {
-		return fmt.Errorf("SO_ERROR sockopt after ICMP error is (%[1]d) %[1]v, expected (%[2]d) %[2]v", errno, wantErrno)
+func testSockOpt(_ context.Context, d testData) error {
+	// Check that there's no pending error on the clean socket.
+	if errno := syscall.Errno(d.dut.GetSockOptInt(d.cleanFD, unix.SOL_SOCKET, unix.SO_ERROR)); errno != syscall.Errno(0) {
+		return fmt.Errorf("unexpected error (%[1]d) %[1]v on clean socket", errno)
+	}
+
+	if errno := syscall.Errno(d.dut.GetSockOptInt(d.remoteFD, unix.SOL_SOCKET, unix.SO_ERROR)); errno != d.wantErrno {
+		return fmt.Errorf("SO_ERROR sockopt after ICMP error is (%[1]d) %[1]v, expected (%[2]d) %[2]v", errno, d.wantErrno)
 	}
 
 	// Check that after clearing socket error, sending doesn't fail.
-	dut.SendTo(remoteFD, nil, 0, conn.LocalAddr())
-	if _, err := conn.Expect(tb.UDP{}, time.Second); err != nil {
+	d.dut.SendTo(d.remoteFD, nil, 0, d.conn.LocalAddr())
+	if _, err := d.conn.Expect(tb.UDP{}, time.Second); err != nil {
 		return fmt.Errorf("did not receive UDP packet as expected: %s", err)
 	}
 	return nil
 }
 
-type testParameters struct {
-	connected connected
-	icmpErr   icmpError
-	wantErrno syscall.Errno
-	f         errorDetectionFunc
-	fName     string
-}
-
-// TestUDPICMPErrorPropagation tests that ICMP PortUnreachable error messages
-// destined for a "connected" UDP socket are observable on said socket by:
-// 1. causing the next send to fail with ECONNREFUSED,
-// 2. causing the next recv to fail with ECONNREFUSED, or
-// 3. returning ECONNREFUSED through the SO_ERROR socket option.
+// TestUDPICMPErrorPropagation tests that ICMP error messages in response to
+// UDP datagrams are processed correctly. RFC 1122 section 4.1.3.3 states that:
+// "UDP MUST pass to the application layer all ICMP error messages that it
+// receives from the IP layer."
+//
+// The test cases are parametrized in 3 dimensions: 1. the UDP socket is either
+// put into connection mode or left connectionless, 2. the ICMP message type
+// and code, and 3. the method by which the ICMP error is observed on the
+// socket: sendto, recv, or getsockopt(SO_ERROR).
+//
+// Linux's udp(7) man page states: "All fatal errors will be passed to the user
+// as an error return even when the socket is not connected. This includes
+// asynchronous errors received from the network." In practice, the only
+// combination of parameters to the test that causes an error to be observable
+// on the UDP socket is receiving a port unreachable message on a connected
+// socket.
 func TestUDPICMPErrorPropagation(t *testing.T) {
-	var testCases []testParameters
-	for _, c := range []connected{true, false} {
-		for _, i := range []icmpError{portUnreachable, timeToLiveExceeded} {
-			e := syscall.Errno(0)
-			if c && i == portUnreachable {
-				e = unix.ECONNREFUSED
+	for _, connect := range []connectionMode{true, false} {
+		for _, icmpErr := range []icmpError{portUnreachable, timeToLiveExceeded} {
+			wantErrno := syscall.Errno(0)
+			if connect && icmpErr == portUnreachable {
+				wantErrno = unix.ECONNREFUSED
 			}
-			for _, f := range []struct {
-				name string
-				f    errorDetectionFunc
-			}{
-				{"SendTo", testSendTo},
-				{"Recv", testRecv},
-				{"SockOpt", testSockOpt},
+			for _, errDetect := range []errorDetection{
+				errorDetection{"SendTo", false, testSendTo},
+				// Send to an address that's different from the one that caused an ICMP
+				// error to be returned.
+				errorDetection{"SendToValid", true, testSendTo},
+				errorDetection{"Recv", false, testRecv},
+				errorDetection{"SockOpt", false, testSockOpt},
 			} {
-				testCases = append(testCases, testParameters{c, i, e, f.f, f.name})
+				t.Run(fmt.Sprintf("%s/%s/%s", connect, icmpErr, errDetect.name), func(t *testing.T) {
+					dut := tb.NewDUT(t)
+					defer dut.TearDown()
+
+					remoteFD, remotePort := dut.CreateBoundSocket(unix.SOCK_DGRAM, unix.IPPROTO_UDP, net.ParseIP("0.0.0.0"))
+					defer dut.Close(remoteFD)
+
+					// Create a second, clean socket on the DUT to ensure that the ICMP
+					// error messages only affect the sockets they are intended for.
+					cleanFD, cleanPort := dut.CreateBoundSocket(unix.SOCK_DGRAM, unix.IPPROTO_UDP, net.ParseIP("0.0.0.0"))
+					defer dut.Close(cleanFD)
+
+					conn := tb.NewUDPIPv4(t, tb.UDP{DstPort: &remotePort}, tb.UDP{SrcPort: &remotePort})
+					defer conn.Close()
+
+					if connect {
+						dut.Connect(remoteFD, conn.LocalAddr())
+						dut.Connect(cleanFD, conn.LocalAddr())
+					}
+
+					dut.SendTo(remoteFD, nil, 0, conn.LocalAddr())
+					udp, err := conn.Expect(tb.UDP{}, time.Second)
+					if err != nil {
+						t.Fatalf("did not receive message from DUT: %s", err)
+					}
+
+					if icmpErr == timeToLiveExceeded {
+						ip, ok := udp.Prev().(*tb.IPv4)
+						if !ok {
+							t.Fatalf("expected %s to be IPv4", udp.Prev())
+						}
+						*ip.TTL = 1
+						// Let serialization recalculate the checksum since we set the TTL
+						// to 1.
+						ip.Checksum = nil
+
+						// Note that the ICMP payload is valid in this case because the UDP
+						// payload is empty. If the UDP payload were not empty, the packet
+						// length during serialization may not be calculated correctly,
+						// resulting in a mal-formed packet.
+						conn.SendIP(icmpErr.ToICMPv4(), ip, udp)
+					} else {
+						conn.SendIP(icmpErr.ToICMPv4(), udp.Prev(), udp)
+					}
+
+					errDetectConn := &conn
+					if errDetect.useValidConn {
+						// connClean is a UDP socket on the test runner that was not
+						// involved in the generation of the ICMP error. As such,
+						// interactions between it and the the DUT should be independent of
+						// the ICMP error at least at the port level.
+						connClean := tb.NewUDPIPv4(t, tb.UDP{DstPort: &remotePort}, tb.UDP{SrcPort: &remotePort})
+						defer connClean.Close()
+
+						errDetectConn = &connClean
+					}
+
+					if err := errDetect.f(context.Background(), testData{&dut, errDetectConn, remoteFD, remotePort, cleanFD, cleanPort, wantErrno}); err != nil {
+						t.Fatal(err)
+					}
+				})
 			}
 		}
 	}
-
-	for _, tt := range testCases {
-		t.Run(fmt.Sprintf("%s/%s/%s", tt.connected, tt.icmpErr, tt.fName), func(t *testing.T) {
-			dut := tb.NewDUT(t)
-			defer dut.TearDown()
-
-			remoteFD, remotePort := dut.CreateBoundSocket(unix.SOCK_DGRAM, unix.IPPROTO_UDP, net.ParseIP("0.0.0.0"))
-			defer dut.Close(remoteFD)
-
-			conn := tb.NewUDPIPv4(t, tb.UDP{DstPort: &remotePort}, tb.UDP{SrcPort: &remotePort})
-			defer conn.Close()
-
-			if tt.connected {
-				dut.Connect(remoteFD, conn.LocalAddr())
-			}
-
-			dut.SendTo(remoteFD, nil, 0, conn.LocalAddr())
-			udp, err := conn.Expect(tb.UDP{}, time.Second)
-			if err != nil {
-				t.Fatalf("did not receive message from DUT: %s", err)
-			}
-
-			if tt.icmpErr == timeToLiveExceeded {
-				ip, ok := udp.Prev().(*tb.IPv4)
-				if !ok {
-					t.Fatalf("expected %s to be IPv4", udp.Prev())
-				}
-				*ip.TTL = 1
-				// Let serialization recalculate the checksum since we set the
-				// TTL to 1.
-				ip.Checksum = nil
-
-				// Note that the ICMP payload is valid in this case because the UDP
-				// payload is empty. If the UDP payload were not empty, the packet
-				// length during serialization may not be calculated correctly,
-				// resulting in a mal-formed packet.
-				conn.SendIP(tt.icmpErr.ToICMPv4(), ip, udp)
-			} else {
-				conn.SendIP(tt.icmpErr.ToICMPv4(), udp.Prev(), udp)
-			}
-
-			if err := tt.f(context.Background(), &dut, &conn, remoteFD, tt.wantErrno); err != nil {
-				t.Fatal(err)
-			}
-		})
-	}
 }