From 554c405e87285622e2d4019baf2c6a19b47f6007 Mon Sep 17 00:00:00 2001
From: Ghanan Gowripalan <ghanan@google.com>
Date: Sat, 6 Feb 2021 16:56:33 -0800
Subject: Synchronously send packets over pipe link endpoint

Before this change, packets were delivered asynchronously to the remote
end of a pipe. This was to avoid a deadlock during link resolution where
the stack would attempt to double-lock a mutex (see removed comments in
the parent commit for details).

As of https://github.com/google/gvisor/commit/4943347137, we do not hold
locks while sending link resolution probes so the deadlock will no
longer occur.

PiperOrigin-RevId: 356066224
---
 pkg/tcpip/link/pipe/pipe.go | 44 +++++++++++++++-----------------------------
 1 file changed, 15 insertions(+), 29 deletions(-)

(limited to 'pkg/tcpip/link/pipe')

diff --git a/pkg/tcpip/link/pipe/pipe.go b/pkg/tcpip/link/pipe/pipe.go
index bbe84f220..21fb87757 100644
--- a/pkg/tcpip/link/pipe/pipe.go
+++ b/pkg/tcpip/link/pipe/pipe.go
@@ -46,6 +46,10 @@ type Endpoint struct {
 }
 
 func (e *Endpoint) deliverPackets(r stack.RouteInfo, proto tcpip.NetworkProtocolNumber, pkts stack.PacketBufferList) {
+	if !e.linked.IsAttached() {
+		return
+	}
+
 	// Note that the local address from the perspective of this endpoint is the
 	// remote address from the perspective of the other end of the pipe
 	// (e.linked). Similarly, the remote address from the perspective of this
@@ -54,44 +58,26 @@ func (e *Endpoint) deliverPackets(r stack.RouteInfo, proto tcpip.NetworkProtocol
 	// Deliver the packet in a new goroutine to escape this goroutine's stack and
 	// avoid a deadlock when a packet triggers a response which leads the stack to
 	// try and take a lock it already holds.
-	//
-	// As of writing, a deadlock may occur when performing link resolution as the
-	// neighbor table will send a solicitation while holding a lock and the
-	// response advertisement will be sent in the same stack that sent the
-	// solictation. When the response is received, the stack attempts to take the
-	// same lock it already took before sending the solicitation, leading to a
-	// deadlock. Basically, we attempt to lock the same lock twice in the same
-	// call stack.
-	//
-	// TODO(gvisor.dev/issue/5289): don't use a new goroutine once we support send
-	// and receive queues.
-	go func() {
-		for pkt := pkts.Front(); pkt != nil; pkt = pkt.Next() {
-			e.linked.dispatcher.DeliverNetworkPacket(r.LocalLinkAddress /* remote */, r.RemoteLinkAddress /* local */, proto, stack.NewPacketBuffer(stack.PacketBufferOptions{
-				Data: buffer.NewVectorisedView(pkt.Size(), pkt.Views()),
-			}))
-		}
-	}()
+	for pkt := pkts.Front(); pkt != nil; pkt = pkt.Next() {
+		e.linked.dispatcher.DeliverNetworkPacket(r.LocalLinkAddress /* remote */, r.RemoteLinkAddress /* local */, proto, stack.NewPacketBuffer(stack.PacketBufferOptions{
+			Data: buffer.NewVectorisedView(pkt.Size(), pkt.Views()),
+		}))
+	}
 }
 
 // WritePacket implements stack.LinkEndpoint.
 func (e *Endpoint) WritePacket(r stack.RouteInfo, _ *stack.GSO, proto tcpip.NetworkProtocolNumber, pkt *stack.PacketBuffer) tcpip.Error {
-	if e.linked.IsAttached() {
-		var pkts stack.PacketBufferList
-		pkts.PushBack(pkt)
-		e.deliverPackets(r, proto, pkts)
-	}
-
+	var pkts stack.PacketBufferList
+	pkts.PushBack(pkt)
+	e.deliverPackets(r, proto, pkts)
 	return nil
 }
 
 // WritePackets implements stack.LinkEndpoint.
 func (e *Endpoint) WritePackets(r stack.RouteInfo, _ *stack.GSO, pkts stack.PacketBufferList, proto tcpip.NetworkProtocolNumber) (int, tcpip.Error) {
-	if e.linked.IsAttached() {
-		e.deliverPackets(r, proto, pkts)
-	}
-
-	return pkts.Len(), nil
+	n := pkts.Len()
+	e.deliverPackets(r, proto, pkts)
+	return n, nil
 }
 
 // Attach implements stack.LinkEndpoint.
-- 
cgit v1.2.3