netstack: remove only the first matching route on RTM_DELROUTE

pkg/sentry/socket/netstack/BUILD

@@ -82,7 +82,16 @@ go_library(
 go_test(
     name = "netstack_test",
     srcs = ["stack_test.go"],
-    deps = [":netstack"],
+    deps = [
+        ":netstack",
+        "//pkg/abi/linux",
+        "//pkg/context",
+        "//pkg/marshal/primitive",
+        "//pkg/sentry/socket/netlink/nlmsg",
+        "//pkg/syserr",
+        "//pkg/tcpip",
+        "//pkg/tcpip/stack",
+    ],
 )
 
 proto_library(

pkg/sentry/socket/netstack/stack.go

@@ -1087,7 +1087,12 @@ func (s *Stack) RemoveRoute(ctx context.Context, msg *nlmsg.Message) *syserr.Err
 	if err != nil {
 		return err
 	}
+	found := false
 	if removed := s.Stack.RemoveRoutes(func(rt tcpip.Route) bool {
+		// Like Linux, remove only the first matching route.
+		if found {
+			return false
+		}
 		// Both gateway and NIC are compared with existing routes
 		// only when they are present in the netlink message.
 		if localRoute.Gateway.Len() > 0 && !localRoute.Gateway.Equal(rt.Gateway) {
@@ -1096,7 +1101,8 @@ func (s *Stack) RemoveRoute(ctx context.Context, msg *nlmsg.Message) *syserr.Err
 		if localRoute.NIC > 0 && localRoute.NIC != rt.NIC {
 			return false
 		}
-		return rt.Destination.Equal(localRoute.Destination)
+		found = rt.Destination.Equal(localRoute.Destination)
+		return found
 	}); removed == 0 {
 		return syserr.ErrNoProcess
 	}

pkg/sentry/socket/netstack/stack_test.go

@@ -17,7 +17,14 @@ package netstack_test
 import (
 	"testing"
 
+	"gvisor.dev/gvisor/pkg/abi/linux"
+	"gvisor.dev/gvisor/pkg/context"
+	"gvisor.dev/gvisor/pkg/marshal/primitive"
+	"gvisor.dev/gvisor/pkg/sentry/socket/netlink/nlmsg"
 	"gvisor.dev/gvisor/pkg/sentry/socket/netstack"
+	"gvisor.dev/gvisor/pkg/syserr"
+	"gvisor.dev/gvisor/pkg/tcpip"
+	"gvisor.dev/gvisor/pkg/tcpip/stack"
 )
 
 func TestDestroyNilStack(t *testing.T) {
@@ -27,3 +34,43 @@ func TestDestroyNilStack(t *testing.T) {
 	// This should not panic.
 	s.Destroy()
 }
+
+func TestRemoveRouteRemovesFirstMatchOnly(t *testing.T) {
+	s := &netstack.Stack{
+		Stack: stack.New(stack.Options{}),
+	}
+	dst := tcpip.AddressWithPrefix{
+		Address:   tcpip.AddrFrom4([4]byte{10, 0, 0, 0}),
+		PrefixLen: 24,
+	}.Subnet()
+	s.Stack.AddRoute(tcpip.Route{Destination: dst, NIC: 1})
+	s.Stack.AddRoute(tcpip.Route{Destination: dst, NIC: 2})
+
+	delRoute := func() *syserr.Error {
+		msg := nlmsg.NewMessage(linux.NetlinkMessageHeader{
+			Type: linux.RTM_DELROUTE,
+		})
+		msg.Put(&linux.RouteMessage{
+			Family: linux.AF_INET,
+			DstLen: 24,
+		})
+		msg.PutAttr(linux.RTA_DST, primitive.AsByteSlice([]byte{10, 0, 0, 0}))
+		return s.RemoveRoute(context.Background(), msg)
+	}
+
+	if err := delRoute(); err != nil {
+		t.Fatalf("first RemoveRoute failed: %v", err)
+	}
+	if got := s.Stack.GetRouteTable(); len(got) != 1 || got[0].NIC != 2 {
+		t.Fatalf("route table after first RemoveRoute = %v, want only the NIC 2 route", got)
+	}
+	if err := delRoute(); err != nil {
+		t.Fatalf("second RemoveRoute failed: %v", err)
+	}
+	if got := s.Stack.GetRouteTable(); len(got) != 0 {
+		t.Fatalf("route table after second RemoveRoute = %v, want empty", got)
+	}
+	if err := delRoute(); err != syserr.ErrNoProcess {
+		t.Fatalf("third RemoveRoute = %v, want %v", err, syserr.ErrNoProcess)
+	}
+}

test/rtnetlink/linux/route_test.sh

@@ -37,6 +37,30 @@ ip netns exec test ip r list | grep "default via 192.168.11.2 dev eth0"
 # Remove all routes that are add/modified above.
 ip netns exec test ip r del default via 192.168.11.2 dev eth0
 ip netns exec test ip r del 192.168.146.48/28
+
+# A route delete removes only the first matching route.
+count_routes() {
+  ip netns exec test ip r list | grep -c "^10.0.0.0/24 " || true
+}
+ip link add name veth2 type veth peer name eth1 netns test
+ip netns exec test ip link set up dev eth1
+ip netns exec test ip r add 10.0.0.0/24 dev eth0 metric 100
+ip netns exec test ip r add 10.0.0.0/24 dev eth1 metric 200
+if [[ "$(count_routes)" != 2 ]]; then
+  fail "expected two 10.0.0.0/24 routes"
+fi
+ip netns exec test ip r del 10.0.0.0/24
+if [[ "$(count_routes)" != 1 ]]; then
+  fail "expected one 10.0.0.0/24 route after the first delete"
+fi
+ip netns exec test ip r del 10.0.0.0/24
+if [[ "$(count_routes)" != 0 ]]; then
+  fail "expected no 10.0.0.0/24 routes after the second delete"
+fi
+if ip netns exec test ip r del 10.0.0.0/24; then
+  fail "deleting a missing route succeeded"
+fi
+
 CURRENT_ROUTES=$(ip netns exec test ip r)
 
 if [[ "$ORIGINAL_ROUTES" != "$CURRENT_ROUTES" ]]; then

test/syscalls/linux/socket_netlink_route.cc

@@ -2493,6 +2493,101 @@ TEST(NetlinkRouteTest, VethAdd) {
   EXPECT_NO_ERRNO(NetlinkRequestAckOrError(fd, kSeq, &req, req.hdr.nlmsg_len));
 }
 
+TEST(NetlinkRouteTest, DeleteRouteRemovesFirstMatchOnly) {
+  SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveCapability(CAP_NET_ADMIN)));
+  SKIP_IF(IsRunningWithHostinet());
+  // Routes are not savable.
+  DisableSave ds;
+
+  const FileDescriptor curr_nsfd =
+      ASSERT_NO_ERRNO_AND_VALUE(Open("/proc/thread-self/ns/net", O_RDONLY));
+  Cleanup restore_netns = Cleanup([&] {
+    ASSERT_THAT(setns(curr_nsfd.get(), CLONE_NEWNET),
+                SyscallSucceedsWithValue(0));
+  });
+  ASSERT_THAT(unshare(CLONE_NEWNET), SyscallSucceedsWithValue(0));
+
+  FileDescriptor fd =
+      ASSERT_NO_ERRNO_AND_VALUE(NetlinkBoundSocket(NETLINK_ROUTE));
+  VethRequest veth_req = GetVethRequest(kSeq, "veth1", "veth2");
+  ASSERT_NO_ERRNO(
+      NetlinkRequestAckOrError(fd, kSeq, &veth_req, veth_req.hdr.nlmsg_len));
+  const int veth1_idx = if_nametoindex("veth1");
+  const int veth2_idx = if_nametoindex("veth2");
+  ASSERT_NE(veth1_idx, 0);
+  ASSERT_NE(veth2_idx, 0);
+  ASSERT_NO_ERRNO(LinkChangeFlags(veth1_idx, IFF_UP, IFF_UP));
+  ASSERT_NO_ERRNO(LinkChangeFlags(veth2_idx, IFF_UP, IFF_UP));
+
+  struct in_addr dst;
+  ASSERT_EQ(inet_pton(AF_INET, "10.0.0.0", &dst), 1);
+  ASSERT_NO_ERRNO(AddUnicastRoute(veth1_idx, AF_INET, 24, &dst, sizeof(dst)));
+  ASSERT_NO_ERRNO(AddUnicastRoute(veth2_idx, AF_INET, 24, &dst, sizeof(dst)));
+
+  // Deletes 10.0.0.0/24 without naming an output interface, so that both
+  // routes match the request.
+  auto del_route = [&]() -> PosixError {
+    struct request {
+      struct nlmsghdr hdr;
+      struct rtmsg rtm;
+      char buf[64];
+    };
+    struct request req = {};
+    req.hdr.nlmsg_len = NLMSG_LENGTH(sizeof(req.rtm));
+    req.hdr.nlmsg_type = RTM_DELROUTE;
+    req.hdr.nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK;
+    req.hdr.nlmsg_seq = kSeq;
+    req.rtm.rtm_family = AF_INET;
+    req.rtm.rtm_dst_len = 24;
+    addattr(&req.hdr, sizeof(req), RTA_DST, &dst, sizeof(dst));
+    return NetlinkRequestAckOrError(fd, kSeq, &req, req.hdr.nlmsg_len);
+  };
+
+  auto count_routes = [&]() -> int {
+    struct request {
+      struct nlmsghdr hdr;
+      struct rtmsg rtm;
+    };
+    struct request req = {};
+    req.hdr.nlmsg_len = sizeof(req);
+    req.hdr.nlmsg_type = RTM_GETROUTE;
+    req.hdr.nlmsg_flags = NLM_F_REQUEST | NLM_F_DUMP;
+    req.hdr.nlmsg_seq = kSeq;
+    req.rtm.rtm_family = AF_INET;
+
+    int count = 0;
+    EXPECT_NO_ERRNO(NetlinkRequestResponse(
+        fd, &req, sizeof(req),
+        [&](const struct nlmsghdr* hdr) {
+          if (hdr->nlmsg_type != RTM_NEWROUTE) {
+            return;
+          }
+          const struct rtmsg* msg =
+              reinterpret_cast<const struct rtmsg*>(NLMSG_DATA(hdr));
+          if (msg->rtm_dst_len != 24) {
+            return;
+          }
+          int len = RTM_PAYLOAD(hdr);
+          for (struct rtattr* attr = RTM_RTA(msg); RTA_OK(attr, len);
+               attr = RTA_NEXT(attr, len)) {
+            if (attr->rta_type == RTA_DST &&
+                memcmp(RTA_DATA(attr), &dst, sizeof(dst)) == 0) {
+              count++;
+            }
+          }
+        },
+        false));
+    return count;
+  };
+
+  ASSERT_EQ(count_routes(), 2);
+  ASSERT_NO_ERRNO(del_route());
+  EXPECT_EQ(count_routes(), 1);
+  ASSERT_NO_ERRNO(del_route());
+  EXPECT_EQ(count_routes(), 0);
+  EXPECT_THAT(del_route(), PosixErrorIs(ESRCH, _));
+}
+
 TEST(NetlinkRouteTest, VethAddShortPeerIfInfoMsg) {
   SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveCapability(CAP_NET_ADMIN)));
   SKIP_IF(IsRunningWithHostinet());