[PATCH v4] netlink: Fix nl_route_dup by 3-step processing

[PATCH v4] netlink: Fix nl_route_dup by 3-step processing

[Martin Schitter]

This is a rewrite of my patches to fix issue #221.

[PATCH v4] netlink: Fix nl_route_dup by 3-step processing

[Martin Schitter]

Fixes: #221

Eliminate the route entries capacity limitations of nl_route_dup
by copping them to the destination namespace in three successive
steps to bypass dependency issues. This approach is taken from
the handling used in the restore subcommand of iproute2.

Signed-off-by: Martin Schitter <ms+git@mur.at>
---
 netlink.c | 207 ++++++++++++++++++++++++++----------------------------
 1 file changed, 99 insertions(+), 108 deletions(-)

diff --git a/netlink.c b/netlink.c
index 225b666..803cb20 100644
--- a/netlink.c
+++ b/netlink.c
@@ -525,6 +525,20 @@ int nl_route_set_def(int s, unsigned int ifi, sa_family_t af, const void *gw)
 	return nl_do(s, &req, RTM_NEWROUTE, NLM_F_CREATE | NLM_F_EXCL, len);
 }
 
+/**
+ * nl_rtattr_cmp() -- compare two routing attributes
+ * @rta1:	Routing attribute
+ * @rta2:	Routing attribute
+ *
+ * Return: 0 if equal
+  */
+static int nl_rtattr_cmp(const struct rtattr *rta1, const struct rtattr *rta2)
+{
+	if (!rta1 || !rta2 || rta1->rta_len != rta2->rta_len)
+		return 1;
+	return memcmp(RTA_DATA(rta1), RTA_DATA(rta2), RTA_PAYLOAD(rta1));
+}
+
 /**
  * nl_route_dup() - Copy routes for given interface and address family
  * @s_src:	Netlink socket in source namespace
@@ -535,56 +549,73 @@ int nl_route_set_def(int s, unsigned int ifi, sa_family_t af, const void *gw)
  *
  * Return: 0 on success, negative error code on failure
  */
-int nl_route_dup(int s_src, unsigned int ifi_src,
-		 int s_dst, unsigned int ifi_dst, sa_family_t af)
+int nl_route_dup(int s_src, unsigned int ifi_src, int s_dst,
+		 unsigned int ifi_dst, sa_family_t af)
 {
-	struct req_t {
-		struct nlmsghdr nlh;
-		struct rtmsg rtm;
-		struct rtattr rta;
-		unsigned int ifi;
-	} req = {
-		.rtm.rtm_family	  = af,
-		.rtm.rtm_table	  = RT_TABLE_MAIN,
-		.rtm.rtm_scope	  = RT_SCOPE_UNIVERSE,
-		.rtm.rtm_type	  = RTN_UNICAST,
+	/* Duplicate routes in correct order using 3 succesive stages:
+	 * 0. ones for local addresses,
+	 * 1. ones for local networks,
+	 * 2. others (remote networks/hosts).
+	 */
+	for (int prio = 0; prio < 3; prio++) {
+		struct req_t {
+			struct nlmsghdr nlh;
+			struct rtmsg rtm;
+			struct rtattr rta;
+			unsigned int ifi;
+		} req = {
+			.rtm.rtm_family = af,
+			.rtm.rtm_table = RT_TABLE_MAIN,
+			.rtm.rtm_scope = RT_SCOPE_UNIVERSE,
+			.rtm.rtm_type = RTN_UNICAST,
+
+			.rta.rta_type = RTA_OIF,
+			.rta.rta_len = RTA_LENGTH(sizeof(unsigned int)),
+			.ifi = ifi_src,
+		};
+		ssize_t status;
+		struct nlmsghdr *nh;
+		char buf[NLBUFSIZ];
+		uint32_t seq;
+
+		seq = nl_send(s_src, &req, RTM_GETROUTE, NLM_F_DUMP,
+			      sizeof(req));
+
+		nl_foreach(nh, status, s_src, buf, seq) {
+			struct rtmsg *rtm = (struct rtmsg *)NLMSG_DATA(nh);
+			struct rtattr *rta;
+			size_t na;
+			uint16_t flags = nh->nlmsg_flags;
+			int rc;
 
-		.rta.rta_type	  = RTA_OIF,
-		.rta.rta_len	  = RTA_LENGTH(sizeof(unsigned int)),
-		.ifi		  = ifi_src,
-	};
-	ssize_t nlmsgs_size, left, status;
-	unsigned dup_routes = 0;
-	struct nlmsghdr *nh;
-	char buf[NLBUFSIZ];
-	uint32_t seq;
-	unsigned i;
 
-	seq = nl_send(s_src, &req, RTM_GETROUTE, NLM_F_DUMP, sizeof(req));
+			if (nh->nlmsg_type != RTM_NEWROUTE)
+				continue;
 
-	/* nl_foreach() will step through multiple response datagrams,
-	 * which we don't want here because we need to have all the
-	 * routes in the buffer at once.
-	 */
-	nh = nl_next(s_src, buf, NULL, &nlmsgs_size);
-	for (left = nlmsgs_size;
-	     NLMSG_OK(nh, left) && (status = nl_status(nh, left, seq)) > 0;
-	     nh = NLMSG_NEXT(nh, left)) {
-		struct rtmsg *rtm = (struct rtmsg *)NLMSG_DATA(nh);
-		bool discard = false;
-		struct rtattr *rta;
-		size_t na;
+			struct rtattr *tb[RTA_MAX+1] = { 0 };
 
-		if (nh->nlmsg_type != RTM_NEWROUTE)
-			continue;
+			for (rta = RTM_RTA(rtm), na = RTM_PAYLOAD(nh);
+			     RTA_OK(rta, na); rta = RTA_NEXT(rta, na)) {
+				tb[rta->rta_type] = rta;
+			}
 
-		/* nexthop state flags don't apply to freshly created routes,
-		 * and the kernel will refuse our route if they are set.
-		 */
-		rtm->rtm_flags &= ~RTNH_COMPARE_MASK;
+			/* Process only matching routes of current priority
+			 * (variant of an approach used in iproute2s restore_handler)
+			 */
+			if (!(prio == 2 && tb[RTA_GATEWAY]) &&
+			    !(prio == 1 && !tb[RTA_GATEWAY] &&
+			      tb[RTA_PREFSRC] &&
+			      nl_rtattr_cmp(tb[RTA_PREFSRC], tb[RTA_DST])) &&
+			    !(prio == 0 && !tb[RTA_GATEWAY] &&
+			      (!tb[RTA_PREFSRC] ||
+			       !nl_rtattr_cmp(tb[RTA_PREFSRC], tb[RTA_DST]))))
+				continue;
+
+			/* nexthop state flags don't apply to freshly created routes,
+			 * and the kernel will refuse our route if they are set.
+			 */
+			rtm->rtm_flags &= ~RTNH_COMPARE_MASK;
 
-		for (rta = RTM_RTA(rtm), na = RTM_PAYLOAD(nh); RTA_OK(rta, na);
-		     rta = RTA_NEXT(rta, na)) {
 			/* RTA_OIF and RTA_MULTIPATH attributes carry the
 			 * identifier of a host interface. If they match the
 			 * host interface we're copying from, change them to
@@ -595,103 +626,63 @@ int nl_route_dup(int s_src, unsigned int ifi_src,
 			 * available), or if any interface index in nexthop
 			 * objects differ from the host interface, discard the
 			 * route altogether.
-			*/
-			if (rta->rta_type == RTA_OIF) {
-				if (*(unsigned int *)RTA_DATA(rta) != ifi_src) {
-					discard = true;
-					break;
+			 */
+			if (tb[RTA_OIF]) {
+				if (*(unsigned int *)RTA_DATA(tb[RTA_OIF]) !=
+				    ifi_src) {
+					continue;
 				}
-
-				*(unsigned int *)RTA_DATA(rta) = ifi_dst;
-			} else if (rta->rta_type == RTA_MULTIPATH) {
-				int nh_len = RTA_PAYLOAD(rta);
+				*(unsigned int *)RTA_DATA(tb[RTA_OIF]) =
+					ifi_dst;
+			} else if (tb[RTA_MULTIPATH]) {
+				int nh_len = RTA_PAYLOAD(tb[RTA_MULTIPATH]);
 				struct rtnexthop *rtnh;
-
-				for (rtnh = (struct rtnexthop *)RTA_DATA(rta);
+				bool discard = false;
+				for (rtnh = (struct rtnexthop *)
+					     RTA_DATA(tb[RTA_MULTIPATH]);
 				     RTNH_OK(rtnh, nh_len);
-				     rtnh = RTNH_NEXT_AND_DEC(rtnh, nh_len)) {
+				     rtnh = RTNH_NEXT_AND_DEC(rtnh,
+							      nh_len)) {
 					int src = (int)ifi_src;
-
 					if (rtnh->rtnh_ifindex != src) {
 						discard = true;
 						break;
 					}
-
 					rtnh->rtnh_ifindex = ifi_dst;
 				}
-
 				if (discard)
-					break;
-			} else if (rta->rta_type == RTA_PREFSRC ||
-				   rta->rta_type == RTA_NH_ID) {
+					continue;
+			} else if (tb[RTA_PREFSRC]) {
 				/* Strip RTA_PREFSRC attributes: host routes
 				 * might include a preferred source address,
 				 * which must be one of the host's addresses.
 				 * However, with -a, pasta will use a different
 				 * namespace address, making such a route
 				 * invalid in the namespace.
-				 *
-				 * Strip RTA_NH_ID attributes: host routes set
+				 */
+				tb[RTA_PREFSRC]->rta_type = RTA_UNSPEC;
+			} else if (tb[RTA_NH_ID]) {
+				/* Strip RTA_NH_ID attributes: host routes set
 				 * up via routing protocols (e.g. OSPF) might
 				 * contain a nexthop ID (and not nexthop
 				 * objects, which are taken care of in the
 				 * RTA_MULTIPATH case above) that's not valid
 				 * in the target namespace.
 				 */
-				rta->rta_type = RTA_UNSPEC;
+				tb[RTA_NH_ID]->rta_type = RTA_UNSPEC;
 			}
-		}
-
-		if (discard)
-			nh->nlmsg_type = NLMSG_NOOP;
-		else
-			dup_routes++;
-	}
-
-	if (!NLMSG_OK(nh, left)) {
-		/* Process any remaining datagrams in a different
-		 * buffer so we don't overwrite the first one.
-		 */
-		char tail[NLBUFSIZ];
-		unsigned extra = 0;
-
-		nl_foreach_oftype(nh, status, s_src, tail, seq, RTM_NEWROUTE)
-			extra++;
-
-		if (extra) {
-			err("netlink: Too many routes to duplicate");
-			return -E2BIG;
-		}
-	}
-	if (status < 0)
-		return status;
-
-	/* Routes might have dependencies between each other, and the kernel
-	 * processes RTM_NEWROUTE messages sequentially. For n routes, we might
-	 * need to send the requests up to n times to get all of them inserted.
-	 * Routes that have been already inserted will return -EEXIST, but we
-	 * can safely ignore that and repeat the requests. This avoids the need
-	 * to calculate dependencies: let the kernel do that.
-	 */
-	for (i = 0; i < dup_routes; i++) {
-		for (nh = (struct nlmsghdr *)buf, left = nlmsgs_size;
-		     NLMSG_OK(nh, left);
-		     nh = NLMSG_NEXT(nh, left)) {
-			uint16_t flags = nh->nlmsg_flags;
-			int rc;
-
-			if (nh->nlmsg_type != RTM_NEWROUTE)
-				continue;
 
 			rc = nl_do(s_dst, nh, RTM_NEWROUTE,
-				   (flags & ~NLM_F_DUMP_FILTERED) | NLM_F_CREATE,
+				   (flags & ~NLM_F_DUMP_FILTERED) |
+					   NLM_F_CREATE,
 				   nh->nlmsg_len);
 			if (rc < 0 && rc != -EEXIST &&
 			    rc != -ENETUNREACH && rc != -EHOSTUNREACH)
 				return rc;
 		}
+		if (status < 0)
+			return status;
 	}
-
 	return 0;
 }
 
-- 
2.53.0