inet6: minor cleanups

PATCH In review Gerrit Renker 2 participants 7 messages Jul 19, 2009 – Jul 22, 2009 1 message not in the archive

Subsystems (7)

Networking Network drivers netfilter IPv4/IPv6 +3 more subsystems

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): inet6_minor_cleanups

Gerrit Renker

author

2009-07-19 18:35 UTC #1 lore raw

cover inet6: minor cleanups

This is a small set of minor-cleanup patches, found while studying the tclass code.

Patch #1: Unifies return convention of datagram_send_ctl(). Patch #2: Consolidates cmsg code for Traffic Class / Hop Limit. Patch #3: Simplifies assignments from u8 to int (no conversion). Patch #4: Finds that RT_TOS() redefines IPTOS_TOS().

Gerrit Renker

author

2009-07-19 18:38 UTC #2 lore raw

PATCH 1/4 inet6: Return convention in datagram_send_ctl

The current code has both return conventions for errors
* set err = 'errval' and then goto exit_f,
* return 'errval' directly.
This patch reduces the number of alternatives to one.

Signed-off-by Gerrit Renker <[email protected]>

---
 net/ipv6/datagram.c |  156 ++++++++++++++++++---------------------------------
 1 files changed, 55 insertions(+), 101 deletions(-)

net/ipv6/datagram.c +55 ~~-101~~

@@ -505,15 +505,12 @@ int datagram_send_ctl(struct net *net,
 	struct ipv6_rt_hdr *rthdr;
 	struct ipv6_opt_hdr *hdr;
 	int len;
-	int err = 0;
 
 	for (cmsg = CMSG_FIRSTHDR(msg); cmsg; cmsg = CMSG_NXTHDR(msg, cmsg)) {
 		int addr_type;
 
-		if (!CMSG_OK(msg, cmsg)) {
-			err = -EINVAL;
-			goto exit_f;
-		}
+		if (!CMSG_OK(msg, cmsg))
+			return -EINVAL;
 
 		if (cmsg->cmsg_level != SOL_IPV6)
 			continue;

@@ -524,10 +521,8 @@ int datagram_send_ctl(struct net *net,
 		    {
 			struct net_device *dev = NULL;
 
-			if (cmsg->cmsg_len < CMSG_LEN(sizeof(struct in6_pktinfo))) {
-				err = -EINVAL;
-				goto exit_f;
-			}
+			if (cmsg->cmsg_len < CMSG_LEN(sizeof(struct in6_pktinfo)))
+				return -EINVAL;
 
 			src_info = (struct in6_pktinfo *)CMSG_DATA(cmsg);

@@ -548,99 +543,76 @@ int datagram_send_ctl(struct net *net,
 
 			if (addr_type != IPV6_ADDR_ANY) {
 				int strict = __ipv6_addr_src_scope(addr_type) <= IPV6_ADDR_SCOPE_LINKLOCAL;
-				if (!ipv6_chk_addr(net, &src_info->ipi6_addr,
-						   strict ? dev : NULL, 0))
-					err = -EINVAL;
-				else
-					ipv6_addr_copy(&fl->fl6_src, &src_info->ipi6_addr);
+				if (!ipv6_chk_addr(net, &src_info->ipi6_addr, strict ? dev : NULL, 0))
+					return -EINVAL;
+				ipv6_addr_copy(&fl->fl6_src, &src_info->ipi6_addr);
 			}
 
 			if (dev)
 				dev_put(dev);
 
-			if (err)
-				goto exit_f;
-
 			break;
 		    }
 
 		case IPV6_FLOWINFO:
-			if (cmsg->cmsg_len < CMSG_LEN(4)) {
-				err = -EINVAL;
-				goto exit_f;
-			}
+			if (cmsg->cmsg_len < CMSG_LEN(4))
+				return -EINVAL;
 
 			if (fl->fl6_flowlabel&IPV6_FLOWINFO_MASK) {
-				if ((fl->fl6_flowlabel^*(__be32 *)CMSG_DATA(cmsg))&~IPV6_FLOWINFO_MASK) {
-					err = -EINVAL;
-					goto exit_f;
-				}
+				if ((fl->fl6_flowlabel^*(__be32 *)CMSG_DATA(cmsg))&~IPV6_FLOWINFO_MASK)
+					return -EINVAL;
 			}
 			fl->fl6_flowlabel = IPV6_FLOWINFO_MASK & *(__be32 *)CMSG_DATA(cmsg);
 			break;
 
 		case IPV6_2292HOPOPTS:
 		case IPV6_HOPOPTS:
-			if (opt->hopopt || cmsg->cmsg_len < CMSG_LEN(sizeof(struct ipv6_opt_hdr))) {
-				err = -EINVAL;
-				goto exit_f;
-			}
+			if (opt->hopopt || cmsg->cmsg_len < CMSG_LEN(sizeof(struct ipv6_opt_hdr)))
+				return -EINVAL;
 
 			hdr = (struct ipv6_opt_hdr *)CMSG_DATA(cmsg);
 			len = ((hdr->hdrlen + 1) << 3);
-			if (cmsg->cmsg_len < CMSG_LEN(len)) {
-				err = -EINVAL;
-				goto exit_f;
-			}
-			if (!capable(CAP_NET_RAW)) {
-				err = -EPERM;
-				goto exit_f;
-			}
+
+			if (cmsg->cmsg_len < CMSG_LEN(len))
+				return -EINVAL;
+			if (!capable(CAP_NET_RAW))
+				return -EPERM;
+
 			opt->opt_nflen += len;
 			opt->hopopt = hdr;
 			break;
 
 		case IPV6_2292DSTOPTS:
-			if (cmsg->cmsg_len < CMSG_LEN(sizeof(struct ipv6_opt_hdr))) {
-				err = -EINVAL;
-				goto exit_f;
-			}
+			if (cmsg->cmsg_len < CMSG_LEN(sizeof(struct ipv6_opt_hdr)))
+				return -EINVAL;
 
 			hdr = (struct ipv6_opt_hdr *)CMSG_DATA(cmsg);
 			len = ((hdr->hdrlen + 1) << 3);
-			if (cmsg->cmsg_len < CMSG_LEN(len)) {
-				err = -EINVAL;
-				goto exit_f;
-			}
-			if (!capable(CAP_NET_RAW)) {
-				err = -EPERM;
-				goto exit_f;
-			}
-			if (opt->dst1opt) {
-				err = -EINVAL;
-				goto exit_f;
-			}
+
+			if (cmsg->cmsg_len < CMSG_LEN(len))
+				return -EINVAL;
+			if (!capable(CAP_NET_RAW))
+				return -EPERM;
+			if (opt->dst1opt)
+				return -EINVAL;
+
 			opt->opt_flen += len;
 			opt->dst1opt = hdr;
 			break;
 
 		case IPV6_DSTOPTS:
 		case IPV6_RTHDRDSTOPTS:
-			if (cmsg->cmsg_len < CMSG_LEN(sizeof(struct ipv6_opt_hdr))) {
-				err = -EINVAL;
-				goto exit_f;
-			}
+			if (cmsg->cmsg_len < CMSG_LEN(sizeof(struct ipv6_opt_hdr)))
+				return -EINVAL;
 
 			hdr = (struct ipv6_opt_hdr *)CMSG_DATA(cmsg);
 			len = ((hdr->hdrlen + 1) << 3);
-			if (cmsg->cmsg_len < CMSG_LEN(len)) {
-				err = -EINVAL;
-				goto exit_f;
-			}
-			if (!capable(CAP_NET_RAW)) {
-				err = -EPERM;
-				goto exit_f;
-			}
+			if (cmsg->cmsg_len < CMSG_LEN(len))
+				return -EINVAL;
+
+			if (!capable(CAP_NET_RAW))
+				return -EPERM;
+
 			if (cmsg->cmsg_type == IPV6_DSTOPTS) {
 				opt->opt_flen += len;
 				opt->dst1opt = hdr;

@@ -652,10 +624,8 @@ int datagram_send_ctl(struct net *net,
 
 		case IPV6_2292RTHDR:
 		case IPV6_RTHDR:
-			if (cmsg->cmsg_len < CMSG_LEN(sizeof(struct ipv6_rt_hdr))) {
-				err = -EINVAL;
-				goto exit_f;
-			}
+			if (cmsg->cmsg_len < CMSG_LEN(sizeof(struct ipv6_rt_hdr)))
+				return -EINVAL;
 
 			rthdr = (struct ipv6_rt_hdr *)CMSG_DATA(cmsg);

@@ -663,29 +633,22 @@ int datagram_send_ctl(struct net *net,
 #if defined(CONFIG_IPV6_MIP6) || defined(CONFIG_IPV6_MIP6_MODULE)
 			case IPV6_SRCRT_TYPE_2:
 				if (rthdr->hdrlen != 2 ||
-				    rthdr->segments_left != 1) {
-					err = -EINVAL;
-					goto exit_f;
-				}
+				    rthdr->segments_left != 1)
+					return -EINVAL;
 				break;
 #endif
 			default:
-				err = -EINVAL;
-				goto exit_f;
+				return -EINVAL;
 			}
 
 			len = ((rthdr->hdrlen + 1) << 3);
 
-			if (cmsg->cmsg_len < CMSG_LEN(len)) {
-				err = -EINVAL;
-				goto exit_f;
-			}
+			if (cmsg->cmsg_len < CMSG_LEN(len))
+				return -EINVAL;
 
 			/* segments left must also match */
-			if ((rthdr->hdrlen >> 1) != rthdr->segments_left) {
-				err = -EINVAL;
-				goto exit_f;
-			}
+			if ((rthdr->hdrlen >> 1) != rthdr->segments_left)
+				return -EINVAL;
 
 			opt->opt_nflen += len;
 			opt->srcrt = rthdr;

@@ -703,16 +666,12 @@ int datagram_send_ctl(struct net *net,
 
 		case IPV6_2292HOPLIMIT:
 		case IPV6_HOPLIMIT:
-			if (cmsg->cmsg_len != CMSG_LEN(sizeof(int))) {
-				err = -EINVAL;
-				goto exit_f;
-			}
+			if (cmsg->cmsg_len != CMSG_LEN(sizeof(int)))
+				return -EINVAL;
 
 			*hlimit = *(int *)CMSG_DATA(cmsg);
-			if (*hlimit < -1 || *hlimit > 0xff) {
-				err = -EINVAL;
-				goto exit_f;
-			}
+			if (*hlimit < -1 || *hlimit > 0xff)
+				return -EINVAL;
 
 			break;

@@ -720,16 +679,13 @@ int datagram_send_ctl(struct net *net,
 		    {
 			int tc;
 
-			err = -EINVAL;
-			if (cmsg->cmsg_len != CMSG_LEN(sizeof(int))) {
-				goto exit_f;
-			}
+			if (cmsg->cmsg_len != CMSG_LEN(sizeof(int)))
+				return -EINVAL;
 
 			tc = *(int *)CMSG_DATA(cmsg);
 			if (tc < -1 || tc > 0xff)
-				goto exit_f;
+				return -EINVAL;
 
-			err = 0;
 			*tclass = tc;
 
 			break;

@@ -737,11 +693,9 @@ int datagram_send_ctl(struct net *net,
 		default:
 			LIMIT_NETDEBUG(KERN_DEBUG "invalid cmsg type: %d\n",
 				       cmsg->cmsg_type);
-			err = -EINVAL;
-			goto exit_f;
+			return -EINVAL;
 		}
 	}
 
-exit_f:
-	return err;
+	return 0;
 }

Gerrit Renker

author

2009-07-19 18:36 UTC #3 lore raw

PATCH 2/4 inet6: Consolidate common code for IPv6 Hop Limit / Traffic Class

The RFC 3542 definitions for Hop Limit (6.3) and Traffic Class (6.5) cmsg values differ only in the names of the cmsg type. So does the code. The patch combines these commonalities.

Further changes:
----------------
Replaced other use of temporary 'int' variable with 'val' variable.

Signed-off-by Gerrit Renker <[email protected]>

---
 net/ipv6/datagram.c |   34 ++++++++++++----------------------
 1 files changed, 12 insertions(+), 22 deletions(-)

net/ipv6/datagram.c +12 ~~-22~~

@@ -504,10 +504,10 @@ int datagram_send_ctl(struct net *net,
 	struct cmsghdr *cmsg;
 	struct ipv6_rt_hdr *rthdr;
 	struct ipv6_opt_hdr *hdr;
+	int val;
 	int len;
 
 	for (cmsg = CMSG_FIRSTHDR(msg); cmsg; cmsg = CMSG_NXTHDR(msg, cmsg)) {
-		int addr_type;
 
 		if (!CMSG_OK(msg, cmsg))
 			return -EINVAL;

@@ -532,17 +532,17 @@ int datagram_send_ctl(struct net *net,
 				fl->oif = src_info->ipi6_ifindex;
 			}
 
-			addr_type = __ipv6_addr_type(&src_info->ipi6_addr);
+			val = __ipv6_addr_type(&src_info->ipi6_addr);
 
 			if (fl->oif) {
 				dev = dev_get_by_index(net, fl->oif);
 				if (!dev)
 					return -ENODEV;
-			} else if (addr_type & IPV6_ADDR_LINKLOCAL)
+			} else if (val & IPV6_ADDR_LINKLOCAL)
 				return -EINVAL;
 
-			if (addr_type != IPV6_ADDR_ANY) {
-				int strict = __ipv6_addr_src_scope(addr_type) <= IPV6_ADDR_SCOPE_LINKLOCAL;
+			if (val != IPV6_ADDR_ANY) {
+				int strict = __ipv6_addr_src_scope(val) <= IPV6_ADDR_SCOPE_LINKLOCAL;
 				if (!ipv6_chk_addr(net, &src_info->ipi6_addr, strict ? dev : NULL, 0))
 					return -EINVAL;
 				ipv6_addr_copy(&fl->fl6_src, &src_info->ipi6_addr);

@@ -666,30 +666,20 @@ int datagram_send_ctl(struct net *net,
 
 		case IPV6_2292HOPLIMIT:
 		case IPV6_HOPLIMIT:
-			if (cmsg->cmsg_len != CMSG_LEN(sizeof(int)))
-				return -EINVAL;
-
-			*hlimit = *(int *)CMSG_DATA(cmsg);
-			if (*hlimit < -1 || *hlimit > 0xff)
-				return -EINVAL;
-
-			break;
-
 		case IPV6_TCLASS:
-		    {
-			int tc;
-
 			if (cmsg->cmsg_len != CMSG_LEN(sizeof(int)))
 				return -EINVAL;
 
-			tc = *(int *)CMSG_DATA(cmsg);
-			if (tc < -1 || tc > 0xff)
+			val = *(int *)CMSG_DATA(cmsg);
+			if (val < -1 || val > 0xff)
 				return -EINVAL;
 
-			*tclass = tc;
-
+			if (cmsg->cmsg_type == IPV6_TCLASS)
+				*tclass = val;
+			else
+				*hlimit = val;
 			break;
-		    }
+
 		default:
 			LIMIT_NETDEBUG(KERN_DEBUG "invalid cmsg type: %d\n",
 				       cmsg->cmsg_type);

Gerrit Renker

author

2009-07-19 18:37 UTC #4 lore raw

PATCH 3/4 inet6: Conversion from u8 to int

This replaces assignments of the type "int on LHS" = "u8 on RHS" with simpler code. The LHS can express all of the unsigned right hand side values, hence the assigned value can not be negative.

Signed-off-by Gerrit Renker <[email protected]>

---
 net/ipv6/icmp.c          |   17 ++++-------------
 net/ipv6/ip6_output.c    |   15 +++++----------
 net/ipv6/ipv6_sockglue.c |    2 --
 net/ipv6/raw.c           |    5 +----
 net/ipv6/udp.c           |    5 +----
 5 files changed, 11 insertions(+), 33 deletions(-)

net/ipv6/icmp.c +4 ~~-13~~

@@ -323,7 +323,7 @@ void icmpv6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info,
 	int iif = 0;
 	int addr_type = 0;
 	int len;
-	int hlimit, tclass;
+	int hlimit;
 	int err = 0;
 
 	if ((u8 *)hdr < skb->head ||

@@ -469,10 +469,6 @@ route_done:
 	if (hlimit < 0)
 		hlimit = ip6_dst_hoplimit(dst);
 
-	tclass = np->tclass;
-	if (tclass < 0)
-		tclass = 0;
-
 	msg.skb = skb;
 	msg.offset = skb_network_offset(skb);
 	msg.type = type;

@@ -488,8 +484,8 @@ route_done:
 
 	err = ip6_append_data(sk, icmpv6_getfrag, &msg,
 			      len + sizeof(struct icmp6hdr),
-			      sizeof(struct icmp6hdr),
-			      hlimit, tclass, NULL, &fl, (struct rt6_info*)dst,
+			      sizeof(struct icmp6hdr), hlimit,
+			      np->tclass, NULL, &fl, (struct rt6_info*)dst,
 			      MSG_DONTWAIT);
 	if (err) {
 		ip6_flush_pending_frames(sk);

@@ -522,7 +518,6 @@ static void icmpv6_echo_reply(struct sk_buff *skb)
 	struct dst_entry *dst;
 	int err = 0;
 	int hlimit;
-	int tclass;
 
 	saddr = &ipv6_hdr(skb)->daddr;

@@ -562,10 +557,6 @@ static void icmpv6_echo_reply(struct sk_buff *skb)
 	if (hlimit < 0)
 		hlimit = ip6_dst_hoplimit(dst);
 
-	tclass = np->tclass;
-	if (tclass < 0)
-		tclass = 0;
-
 	idev = in6_dev_get(skb->dev);
 
 	msg.skb = skb;

@@ -573,7 +564,7 @@ static void icmpv6_echo_reply(struct sk_buff *skb)
 	msg.type = ICMPV6_ECHO_REPLY;
 
 	err = ip6_append_data(sk, icmpv6_getfrag, &msg, skb->len + sizeof(struct icmp6hdr),
-				sizeof(struct icmp6hdr), hlimit, tclass, NULL, &fl,
+				sizeof(struct icmp6hdr), hlimit, np->tclass, NULL, &fl,
 				(struct rt6_info*)dst, MSG_DONTWAIT);
 
 	if (err) {

net/ipv6/ip6_output.c +5 ~~-10~~

@@ -206,7 +206,8 @@ int ip6_xmit(struct sock *sk, struct sk_buff *skb, struct flowi *fl,
 	struct ipv6hdr *hdr;
 	u8  proto = fl->proto;
 	int seg_len = skb->len;
-	int hlimit, tclass;
+	int hlimit = -1;
+	int tclass = 0;
 	u32 mtu;
 
 	if (opt) {

@@ -249,19 +250,13 @@ int ip6_xmit(struct sock *sk, struct sk_buff *skb, struct flowi *fl,
 	/*
 	 *	Fill in the IPv6 header
 	 */
-
-	hlimit = -1;
-	if (np)
+	if (np) {
+		tclass = np->tclass;
 		hlimit = np->hop_limit;
+	}
 	if (hlimit < 0)
 		hlimit = ip6_dst_hoplimit(dst);
 
-	tclass = -1;
-	if (np)
-		tclass = np->tclass;
-	if (tclass < 0)
-		tclass = 0;
-
 	*(__be32 *)hdr = htonl(0x60000000 | (tclass << 20)) | fl->fl6_flowlabel;
 
 	hdr->payload_len = htons(seg_len);

net/ipv6/ipv6_sockglue.c +0 -2

@@ -1037,8 +1037,6 @@ static int do_ipv6_getsockopt(struct sock *sk, int level, int optname,
 
 	case IPV6_TCLASS:
 		val = np->tclass;
-		if (val < 0)
-			val = 0;
 		break;
 
 	case IPV6_RECVTCLASS:

net/ipv6/raw.c +1 -4

@@ -877,11 +877,8 @@ static int rawv6_sendmsg(struct kiocb *iocb, struct sock *sk,
 			hlimit = ip6_dst_hoplimit(dst);
 	}
 
-	if (tclass < 0) {
+	if (tclass < 0)
 		tclass = np->tclass;
-		if (tclass < 0)
-			tclass = 0;
-	}
 
 	if (msg->msg_flags&MSG_CONFIRM)
 		goto do_confirm;

net/ipv6/udp.c +1 -4

@@ -900,11 +900,8 @@ do_udp_sendmsg:
 			hlimit = ip6_dst_hoplimit(dst);
 	}
 
-	if (tclass < 0) {
+	if (tclass < 0)
 		tclass = np->tclass;
-		if (tclass < 0)
-			tclass = 0;
-	}
 
 	if (msg->msg_flags&MSG_CONFIRM)
 		goto do_confirm;

Gerrit Renker

author

2009-07-19 18:39 UTC #5 lore raw

PATCH 4/4 inet: in_route.h redefined macro

linux/in_route.h declares RT_TOS(), which is the same as IPTOS_TOS() from linux/ip.h. The files that use RT_TOS() all also include linux/ip.h.

The patch removes duplication, all it does is s/RT_TOS/IPTOS_TOS/g.

Signed-off-by Gerrit Renker <[email protected]>

---
 include/linux/in_route.h        |    2 --
 include/net/ip_vs.h             |    2 +-
 include/net/route.h             |    2 +-
 net/bridge/br_netfilter.c       |    2 +-
 net/ipv4/icmp.c                 |    6 +++---
 net/ipv4/ip_gre.c               |    6 +++---
 net/ipv4/ip_output.c            |    2 +-
 net/ipv4/ipip.c                 |    4 ++--
 net/ipv4/ipmr.c                 |    4 ++--
 net/ipv4/netfilter.c            |    4 ++--
 net/ipv4/udp.c                  |    2 +-
 net/ipv6/ip6_tunnel.c           |    2 +-
 net/ipv6/sit.c                  |    4 ++--
 net/netfilter/ipvs/ip_vs_xmit.c |   10 +++++-----
 14 files changed, 25 insertions(+), 27 deletions(-)

include/linux/in_route.h +0 -2

@@ -27,6 +27,4 @@
 
 #define RTCF_NAT	(RTCF_DNAT|RTCF_SNAT)
 
-#define RT_TOS(tos)	((tos)&IPTOS_TOS_MASK)
-
 #endif /* _LINUX_IN_ROUTE_H */

include/net/route.h +1 -1

@@ -45,7 +45,7 @@
 /* RTO_CONN is not used (being alias for 0), but preserved not to break
  * some modules referring to it. */
 
-#define RT_CONN_FLAGS(sk)   (RT_TOS(inet_sk(sk)->tos) | sock_flag(sk, SOCK_LOCALROUTE))
+#define RT_CONN_FLAGS(sk)   (IPTOS_TOS(inet_sk(sk)->tos) | sock_flag(sk, SOCK_LOCALROUTE))
 
 struct fib_nh;
 struct inet_peer;

include/net/ip_vs.h +1 -1

@@ -484,7 +484,7 @@ struct ip_vs_dest {
 	/* for destination cache */
 	spinlock_t		dst_lock;	/* lock of dst_cache */
 	struct dst_entry	*dst_cache;	/* destination cache entry */
-	u32			dst_rtos;	/* RT_TOS(tos) for dst */
+	u32			dst_rtos;	/* IPTOS_TOS(tos) for dst */
 
 	/* for virtual service */
 	struct ip_vs_service	*svc;		/* service it belongs to */

net/bridge/br_netfilter.c +1 -1

@@ -355,7 +355,7 @@ static int br_nf_pre_routing_finish(struct sk_buff *skb)
 					.ip4_u = {
 						 .daddr = iph->daddr,
 						 .saddr = 0,
-						 .tos = RT_TOS(iph->tos) },
+						 .tos = IPTOS_TOS(iph->tos) },
 				},
 				.proto = 0,
 			};

net/ipv4/icmp.c +3 -3

@@ -385,7 +385,7 @@ static void icmp_reply(struct icmp_bxm *icmp_param, struct sk_buff *skb)
 		struct flowi fl = { .nl_u = { .ip4_u =
 					      { .daddr = daddr,
 						.saddr = rt->rt_spec_dst,
-						.tos = RT_TOS(ip_hdr(skb)->tos) } },
+						.tos = IPTOS_TOS(ip_hdr(skb)->tos) } },
 				    .proto = IPPROTO_ICMP };
 		security_skb_classify_flow(skb, &fl);
 		if (ip_route_output_key(net, &rt, &fl))

@@ -543,7 +543,7 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
 						icmp_param.replyopts.faddr :
 						iph->saddr,
 					.saddr = saddr,
-					.tos = RT_TOS(tos)
+					.tos = IPTOS_TOS(tos)
 				}
 			},
 			.proto = IPPROTO_ICMP,

@@ -593,7 +593,7 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
 			/* Ugh! */
 			odst = skb_dst(skb_in);
 			err = ip_route_input(skb_in, fl.fl4_dst, fl.fl4_src,
-					     RT_TOS(tos), rt2->u.dst.dev);
+					     IPTOS_TOS(tos), rt2->u.dst.dev);
 
 			dst_release(&rt2->u.dst);
 			rt2 = skb_rtable(skb_in);

net/ipv4/ip_gre.c +3 -3

@@ -746,7 +746,7 @@ static int ipgre_tunnel_xmit(struct sk_buff *skb, struct net_device *dev)
 				    .nl_u = { .ip4_u =
 					      { .daddr = dst,
 						.saddr = tiph->saddr,
-						.tos = RT_TOS(tos) } },
+						.tos = IPTOS_TOS(tos) } },
 				    .proto = IPPROTO_GRE };
 		if (ip_route_output_key(dev_net(dev), &rt, &fl)) {
 			stats->tx_carrier_errors++;

@@ -920,7 +920,7 @@ static int ipgre_tunnel_bind_dev(struct net_device *dev)
 				    .nl_u = { .ip4_u =
 					      { .daddr = iph->daddr,
 						.saddr = iph->saddr,
-						.tos = RT_TOS(iph->tos) } },
+						.tos = IPTOS_TOS(iph->tos) } },
 				    .proto = IPPROTO_GRE };
 		struct rtable *rt;
 		if (!ip_route_output_key(dev_net(dev), &rt, &fl)) {

@@ -1181,7 +1181,7 @@ static int ipgre_open(struct net_device *dev)
 				    .nl_u = { .ip4_u =
 					      { .daddr = t->parms.iph.daddr,
 						.saddr = t->parms.iph.saddr,
-						.tos = RT_TOS(t->parms.iph.tos) } },
+						.tos = IPTOS_TOS(t->parms.iph.tos) } },
 				    .proto = IPPROTO_GRE };
 		struct rtable *rt;
 		if (ip_route_output_key(dev_net(dev), &rt, &fl))

net/ipv4/ip_output.c +1 -1

@@ -1381,7 +1381,7 @@ void ip_send_reply(struct sock *sk, struct sk_buff *skb, struct ip_reply_arg *ar
 				    .nl_u = { .ip4_u =
 					      { .daddr = daddr,
 						.saddr = rt->rt_spec_dst,
-						.tos = RT_TOS(ip_hdr(skb)->tos) } },
+						.tos = IPTOS_TOS(ip_hdr(skb)->tos) } },
 				    /* Not quite clean, but right. */
 				    .uli_u = { .ports =
 					       { .sport = tcp_hdr(skb)->dest,

net/ipv4/ipip.c +2 -2

@@ -428,7 +428,7 @@ static int ipip_tunnel_xmit(struct sk_buff *skb, struct net_device *dev)
 				    .nl_u = { .ip4_u =
 					      { .daddr = dst,
 						.saddr = tiph->saddr,
-						.tos = RT_TOS(tos) } },
+						.tos = IPTOS_TOS(tos) } },
 				    .proto = IPPROTO_IPIP };
 		if (ip_route_output_key(dev_net(dev), &rt, &fl)) {
 			stats->tx_carrier_errors++;

@@ -549,7 +549,7 @@ static void ipip_tunnel_bind_dev(struct net_device *dev)
 				    .nl_u = { .ip4_u =
 					      { .daddr = iph->daddr,
 						.saddr = iph->saddr,
-						.tos = RT_TOS(iph->tos) } },
+						.tos = IPTOS_TOS(iph->tos) } },
 				    .proto = IPPROTO_IPIP };
 		struct rtable *rt;
 		if (!ip_route_output_key(dev_net(dev), &rt, &fl)) {

net/ipv4/ipmr.c +2 -2

@@ -1245,7 +1245,7 @@ static void ipmr_queue_xmit(struct sk_buff *skb, struct mfc_cache *c, int vifi)
 				    .nl_u = { .ip4_u =
 					      { .daddr = vif->remote,
 						.saddr = vif->local,
-						.tos = RT_TOS(iph->tos) } },
+						.tos = IPTOS_TOS(iph->tos) } },
 				    .proto = IPPROTO_IPIP };
 		if (ip_route_output_key(net, &rt, &fl))
 			goto out_free;

@@ -1254,7 +1254,7 @@ static void ipmr_queue_xmit(struct sk_buff *skb, struct mfc_cache *c, int vifi)
 		struct flowi fl = { .oif = vif->link,
 				    .nl_u = { .ip4_u =
 					      { .daddr = iph->daddr,
-						.tos = RT_TOS(iph->tos) } },
+						.tos = IPTOS_TOS(iph->tos) } },
 				    .proto = IPPROTO_IPIP };
 		if (ip_route_output_key(net, &rt, &fl))
 			goto out_free;

net/ipv4/netfilter.c +2 -2

@@ -33,7 +33,7 @@ int ip_route_me_harder(struct sk_buff *skb, unsigned addr_type)
 		fl.nl_u.ip4_u.daddr = iph->daddr;
 		if (type == RTN_LOCAL)
 			fl.nl_u.ip4_u.saddr = iph->saddr;
-		fl.nl_u.ip4_u.tos = RT_TOS(iph->tos);
+		fl.nl_u.ip4_u.tos = IPTOS_TOS(iph->tos);
 		fl.oif = skb->sk ? skb->sk->sk_bound_dev_if : 0;
 		fl.mark = skb->mark;
 		fl.flags = skb->sk ? inet_sk_flowi_flags(skb->sk) : 0;

@@ -52,7 +52,7 @@ int ip_route_me_harder(struct sk_buff *skb, unsigned addr_type)
 
 		odst = skb_dst(skb);
 		if (ip_route_input(skb, iph->daddr, iph->saddr,
-				   RT_TOS(iph->tos), rt->u.dst.dev) != 0) {
+				   IPTOS_TOS(iph->tos), rt->u.dst.dev) != 0) {
 			dst_release(&rt->u.dst);
 			return -1;
 		}

net/ipv4/udp.c +1 -1

@@ -667,7 +667,7 @@ int udp_sendmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
 		faddr = ipc.opt->faddr;
 		connected = 0;
 	}
-	tos = RT_TOS(inet->tos);
+	tos = IPTOS_TOS(inet->tos);
 	if (sock_flag(sk, SOCK_LOCALROUTE) ||
 	    (msg->msg_flags & MSG_DONTROUTE) ||
 	    (ipc.opt && ipc.opt->is_strictroute)) {

net/ipv6/ip6_tunnel.c +1 -1

@@ -541,7 +541,7 @@ ip4ip6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 	/* Try to guess incoming interface */
 	memset(&fl, 0, sizeof(fl));
 	fl.fl4_dst = eiph->saddr;
-	fl.fl4_tos = RT_TOS(eiph->tos);
+	fl.fl4_tos = IPTOS_TOS(eiph->tos);
 	fl.proto = IPPROTO_IPIP;
 	if (ip_route_output_key(dev_net(skb->dev), &rt, &fl))
 		goto out;

net/ipv6/sit.c +2 -2

@@ -689,7 +689,7 @@ static int ipip6_tunnel_xmit(struct sk_buff *skb, struct net_device *dev)
 		struct flowi fl = { .nl_u = { .ip4_u =
 					      { .daddr = dst,
 						.saddr = tiph->saddr,
-						.tos = RT_TOS(tos) } },
+						.tos = IPTOS_TOS(tos) } },
 				    .oif = tunnel->parms.link,
 				    .proto = IPPROTO_IPV6 };
 		if (ip_route_output_key(dev_net(dev), &rt, &fl)) {

@@ -818,7 +818,7 @@ static void ipip6_tunnel_bind_dev(struct net_device *dev)
 		struct flowi fl = { .nl_u = { .ip4_u =
 					      { .daddr = iph->daddr,
 						.saddr = iph->saddr,
-						.tos = RT_TOS(iph->tos) } },
+						.tos = IPTOS_TOS(iph->tos) } },
 				    .oif = tunnel->parms.link,
 				    .proto = IPPROTO_IPV6 };
 		struct rtable *rt;

net/netfilter/ipvs/ip_vs_xmit.c +5 -5

@@ -229,7 +229,7 @@ ip_vs_bypass_xmit(struct sk_buff *skb, struct ip_vs_conn *cp,
 			.ip4_u = {
 				.daddr = iph->daddr,
 				.saddr = 0,
-				.tos = RT_TOS(tos), } },
+				.tos = IPTOS_TOS(tos), } },
 	};
 
 	EnterFunction(10);

@@ -368,7 +368,7 @@ ip_vs_nat_xmit(struct sk_buff *skb, struct ip_vs_conn *cp,
 		IP_VS_DBG(10, "filled cport=%d\n", ntohs(*p));
 	}
 
-	if (!(rt = __ip_vs_get_out_rt(cp, RT_TOS(iph->tos))))
+	if (!(rt = __ip_vs_get_out_rt(cp, IPTOS_TOS(iph->tos))))
 		goto tx_error_icmp;
 
 	/* MTU checking */

@@ -542,7 +542,7 @@ ip_vs_tunnel_xmit(struct sk_buff *skb, struct ip_vs_conn *cp,
 		goto tx_error;
 	}
 
-	if (!(rt = __ip_vs_get_out_rt(cp, RT_TOS(tos))))
+	if (!(rt = __ip_vs_get_out_rt(cp, IPTOS_TOS(tos))))
 		goto tx_error_icmp;
 
 	tdev = rt->u.dst.dev;

@@ -752,7 +752,7 @@ ip_vs_dr_xmit(struct sk_buff *skb, struct ip_vs_conn *cp,
 
 	EnterFunction(10);
 
-	if (!(rt = __ip_vs_get_out_rt(cp, RT_TOS(iph->tos))))
+	if (!(rt = __ip_vs_get_out_rt(cp, IPTOS_TOS(iph->tos))))
 		goto tx_error_icmp;
 
 	/* MTU checking */

@@ -880,7 +880,7 @@ ip_vs_icmp_xmit(struct sk_buff *skb, struct ip_vs_conn *cp,
 	 * mangle and send the packet here (only for VS/NAT)
 	 */
 
-	if (!(rt = __ip_vs_get_out_rt(cp, RT_TOS(ip_hdr(skb)->tos))))
+	if (!(rt = __ip_vs_get_out_rt(cp, IPTOS_TOS(ip_hdr(skb)->tos))))
 		goto tx_error_icmp;
 
 	/* MTU checking */

David Miller

maintainer

2009-07-21 19:57 UTC #6 lore raw

In reply to Gerrit Renker

From: Gerrit Renker <[email protected]>
Date: Sun, 19 Jul 2009 20:23:35 +0200

Quoted 4 lines

This is a small set of minor-cleanup patches, found while studying the tclass code.
Patch #1: Unifies return convention of datagram_send_ctl().

OK.

Quoted 1 line

Patch #2: Consolidates cmsg code for Traffic Class / Hop Limit.

You delete the 'addr_type' variable but that's rediculious.

The name of the variable describes what is held in it, now that value gets stored in 'val' which makes the code harder to read and understand.

I'm not applying this, it's noise, and on top of that it's bad noise and makes the code worse.

Quoted 1 line

Patch #3: Simplifies assignments from u8 to int (no conversion).

OK.

Quoted 1 line

Patch #4: Finds that RT_TOS() redefines IPTOS_TOS().

This macro probably exists so that if the ipv4 route cache entry TOS representation changes compared to IPTOS_TOS(), we would simply have to change the definition of this macro.

Just because they happen to be the same now is no reason to kill off this macro I think.

I'm not applying this either.

Gerrit Renker

author

2009-07-22 20:17 UTC #7 lore raw

Question: AF-Independence of ECN (was [PATCH 0/4] inet6: minor cleanups)

Thank you for reviewing the patches.

| > Patch #4: Finds that RT_TOS() redefines IPTOS_TOS().
| 
| This macro probably exists so that if the ipv4 route cache entry TOS
| representation changes compared to IPTOS_TOS(), we would simply have
| to change the definition of this macro.
| 
| Just because they happen to be the same now is no reason to kill off
| this macro I think.
| 
| I'm not applying this either.
|

These patches are a byproduct of studying the code. I think it is better to post the question I am trying to resolve, before discussing any more patches. I hope that there will be some input and/or discussion.

The problem is that
* IPv4 TOS and IPv6 Traffic Class have identical semantics
- the DiffServ bits 0..5 due to RFC 2474,
- the 'CU'/ECN bits 6,7 due to RFC 3168.
* in the code this is currently handled AF-dependent:
- as inet_sk's "tos" field,
- as ipv6_pinfo's "tclass" field.

There are already three existing Linux transport protocols affected by the AF-dependence of ECN:
* TCPv4 supports ECN, but TCPv6 currently does not;
* SCTP achieves ECN support for v4 and v6 only with difficulties:
- it uses a function pointer 'ecn_capable', which calls wrappers around
- INET_ECN_xmit (sctp_v4_ecn_capable)
- modification of inet6_sk(sk)->tclass (sctp_v6_ecn_capable)
* DCCP ECN is not supported yet, but a working patch set exists.

So the whole point is: to achieve AF-independent ECN/DiffServ support, I was using an ugly hack to work around the existing AF-dependence:

  #define IP6_ECN_flow_xmit(sk, label) do {                               \
  -       if (INET_ECN_is_capable(inet6_sk(sk)->tclass))                  \
  -               (label) |= htonl(INET_ECN_ECT_0 << 20);                 \
  +       IP6_ECN_flow_init(label);                                       \
  +       (label) |= htonl((inet_sk(sk)->tos & INET_ECN_MASK) << 20);     \
       } while (0)

Realising that
* this is so ugly that I don't want to submit it (it has been the reason for several patches sitting idly for a couple of months), and
* finding that other transport protocols are facing a similar situation,

I am interested in solving the problem so that
(a) the DCCP code becomes usable and
(b) other transport protocols might benefit from a more generic solution.

My question:
* is there support for an AF-independent use of ECN/DiffServ field?
* what needs to be considered other than a good understanding of the code:
- are there possible breakages,
- legacy code or,
- anything else that would prevent this?

Gerrit