ipvs: IPv6 fragment handling for IPVS

PATCH Superseded Jesper Dangaard Brouer 4 participants 12 messages Aug 20, 2012 – Sep 4, 2012

Subsystems (5)

Networking Network drivers netfilter IPv4/IPv6 +1 more subsystems

lore

Share & tools

lore

Jesper Dangaard Brouer

author

2012-08-20 13:08 UTC #0 lore raw

cover ipvs: IPv6 fragment handling for IPVS

The following patchset implement IPv6 fragment handling for IPVS.

This work is based upon patches from Hans Schillstrom. I have taken over the patchset, in close agreement with Hans, because he don't have (gotten allocated) time to complete his work.

I have cleaned up the patchset, changed the API a bit, fixed a refcnt bug, and rebased on top of Julians recent changes. (All with Hans'es knowledge)

Patch01: is just unrelated trivial fixes.

Patch02: Fix faulty IPv6 extension header handling in IPVS

Patch03: Complete IPv6 fragment handling for IPVS

This patchset is based upon:
Homes ipvs-next tree:
git://git.kernel.org/pub/scm/linux/kernel/git/horms/ipvs-next.git

On top of commit 3654e61137db891f5312e6dd813b961484b5fdf3:
ipvs: add pmtu_disc option to disable IP DF for TUN packets

---

Jesper Dangaard Brouer (3):
ipvs: Complete IPv6 fragment handling for IPVS
ipvs: Fix faulty IPv6 extension header handling in IPVS
ipvs: Trivial changes, use compressed IPv6 address in output

 include/net/ip_vs.h                     |  191 +++++++++++----
 net/netfilter/ipvs/Kconfig              |    7 -
 net/netfilter/ipvs/ip_vs_conn.c         |   15 -
 net/netfilter/ipvs/ip_vs_core.c         |  384 +++++++++++++++++--------------
 net/netfilter/ipvs/ip_vs_dh.c           |    2 
 net/netfilter/ipvs/ip_vs_lblc.c         |    2 
 net/netfilter/ipvs/ip_vs_lblcr.c        |    2 
 net/netfilter/ipvs/ip_vs_pe_sip.c       |   27 ++
 net/netfilter/ipvs/ip_vs_proto.c        |    6 
 net/netfilter/ipvs/ip_vs_proto_ah_esp.c |    9 -
 net/netfilter/ipvs/ip_vs_proto_sctp.c   |   42 +--
 net/netfilter/ipvs/ip_vs_proto_tcp.c    |   40 +--
 net/netfilter/ipvs/ip_vs_proto_udp.c    |   41 +--
 net/netfilter/ipvs/ip_vs_sched.c        |    2 
 net/netfilter/ipvs/ip_vs_sh.c           |    2 
 net/netfilter/ipvs/ip_vs_xmit.c         |   75 +++---
 net/netfilter/xt_ipvs.c                 |    4 
 17 files changed, 489 insertions(+), 362 deletions(-)

--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Sr. Network Kernel Developer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer

Jesper Dangaard Brouer

author

2012-08-20 13:08 UTC #1 lore raw

PATCH 1/3 ipvs: Trivial changes, use compressed IPv6 address in output

Have not converted the proc file output to compressed IPv6 addresses.

Move trivial fixes to this first patch.

Signed-off-by Jesper Dangaard Brouer <[email protected]>

---

 include/net/ip_vs.h              |    2 +-
 net/netfilter/ipvs/ip_vs_proto.c |    6 +++---
 net/netfilter/ipvs/ip_vs_sched.c |    2 +-
 net/netfilter/ipvs/ip_vs_xmit.c  |   10 +++++-----
 4 files changed, 10 insertions(+), 10 deletions(-)

include/net/ip_vs.h +1 -1

index ee75ccd..aba0bb2 100644
@@ -165,7 +165,7 @@ static inline const char *ip_vs_dbg_addr(int af, char *buf, size_t buf_len,
 	int len;
 #ifdef CONFIG_IP_VS_IPV6
 	if (af == AF_INET6)
-		len = snprintf(&buf[*idx], buf_len - *idx, "[%pI6]",
+		len = snprintf(&buf[*idx], buf_len - *idx, "[%pI6c]",
 			       &addr->in6) + 1;
 	else
 #endif

net/netfilter/ipvs/ip_vs_proto.c +3 -3

index 50d8218..939f7fb 100644
@@ -280,17 +280,17 @@ ip_vs_tcpudp_debug_packet_v6(struct ip_vs_protocol *pp,
 	if (ih == NULL)
 		sprintf(buf, "TRUNCATED");
 	else if (ih->nexthdr == IPPROTO_FRAGMENT)
-		sprintf(buf, "%pI6->%pI6 frag",	&ih->saddr, &ih->daddr);
+		sprintf(buf, "%pI6c->%pI6c frag", &ih->saddr, &ih->daddr);
 	else {
 		__be16 _ports[2], *pptr;
 
 		pptr = skb_header_pointer(skb, offset + sizeof(struct ipv6hdr),
 					  sizeof(_ports), _ports);
 		if (pptr == NULL)
-			sprintf(buf, "TRUNCATED %pI6->%pI6",
+			sprintf(buf, "TRUNCATED %pI6c->%pI6c",
 				&ih->saddr, &ih->daddr);
 		else
-			sprintf(buf, "%pI6:%u->%pI6:%u",
+			sprintf(buf, "%pI6c:%u->%pI6c:%u",
 				&ih->saddr, ntohs(pptr[0]),
 				&ih->daddr, ntohs(pptr[1]));
 	}

net/netfilter/ipvs/ip_vs_sched.c +1 -1

index 08dbdd5..d6bf20d 100644
@@ -159,7 +159,7 @@ void ip_vs_scheduler_err(struct ip_vs_service *svc, const char *msg)
 			     svc->fwmark, msg);
 #ifdef CONFIG_IP_VS_IPV6
 	} else if (svc->af == AF_INET6) {
-		IP_VS_ERR_RL("%s: %s [%pI6]:%d - %s\n",
+		IP_VS_ERR_RL("%s: %s [%pI6c]:%d - %s\n",
 			     svc->scheduler->name,
 			     ip_vs_proto_name(svc->protocol),
 			     &svc->addr.in6, ntohs(svc->port), msg);

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

index 543a554..eef3432 100644
@@ -319,7 +319,7 @@ __ip_vs_get_out_rt_v6(struct sk_buff *skb, struct ip_vs_dest *dest,
 	local = __ip_vs_is_local_route6(rt);
 	if (!((local ? IP_VS_RT_MODE_LOCAL : IP_VS_RT_MODE_NON_LOCAL) &
 	      rt_mode)) {
-		IP_VS_DBG_RL("Stopping traffic to %s address, dest: %pI6\n",
+		IP_VS_DBG_RL("Stopping traffic to %s address, dest: %pI6c\n",
 			     local ? "local":"non-local", daddr);
 		dst_release(&rt->dst);
 		return NULL;

@@ -327,8 +327,8 @@ __ip_vs_get_out_rt_v6(struct sk_buff *skb, struct ip_vs_dest *dest,
 	if (local && !(rt_mode & IP_VS_RT_MODE_RDR) &&
 	    !((ort = (struct rt6_info *) skb_dst(skb)) &&
 	      __ip_vs_is_local_route6(ort))) {
-		IP_VS_DBG_RL("Redirect from non-local address %pI6 to local "
-			     "requires NAT method, dest: %pI6\n",
+		IP_VS_DBG_RL("Redirect from non-local address %pI6c to local "
+			     "requires NAT method, dest: %pI6c\n",
 			     &ipv6_hdr(skb)->daddr, daddr);
 		dst_release(&rt->dst);
 		return NULL;

@@ -336,8 +336,8 @@ __ip_vs_get_out_rt_v6(struct sk_buff *skb, struct ip_vs_dest *dest,
 	if (unlikely(!local && (!skb->dev || skb->dev->flags & IFF_LOOPBACK) &&
 		     ipv6_addr_type(&ipv6_hdr(skb)->saddr) &
 				    IPV6_ADDR_LOOPBACK)) {
-		IP_VS_DBG_RL("Stopping traffic from loopback address %pI6 "
-			     "to non-local address, dest: %pI6\n",
+		IP_VS_DBG_RL("Stopping traffic from loopback address %pI6c "
+			     "to non-local address, dest: %pI6c\n",
 			     &ipv6_hdr(skb)->saddr, daddr);
 		dst_release(&rt->dst);
 		return NULL;

Jesper Dangaard Brouer

author

2012-08-20 13:08 UTC #2 lore raw

PATCH 2/3 ipvs: Fix faulty IPv6 extension header handling in IPVS

In reply to Jesper Dangaard Brouer

Based on patch from: Hans Schillstrom

IPv6 headers must be processed in order of appearance, neither can it be assumed that Upper layer headers is first. If anything else than L4 is the first header IPVS will throw it.

IPVS will write SNAT & DNAT modifications at a fixed pos which will corrupt the message. Proper header position must be found before writing modifying packet.

This patch contains a lot of API changes. This is done, to avoid the costly scan of finding the IPv6 headers, via ipv6_find_hdr(). Finding the IPv6 headers is done as early as possible, and passed on as a pointer "struct ip_vs_iphdr *" to the affected functions.

Notice, I have choosen, not to change the API of function pointer "(*schedule)" (in struct ip_vs_scheduler) as it can be used by external schedulers, via {un,}register_ip_vs_scheduler. Only 4 out of 10 schedulers use info from ip_vs_iphdr*, and when they do, they are only interested in iph->{s,d}addr.

This patch depends on commit 84018f55a:
"netfilter: ip6_tables: add flags parameter to ipv6_find_hdr()"

This also adds a dependency to ip6_tables.

Hans left some questions in ip_vs_pe_sip.c, which I'm uncertain about.

Signed-off-by Jesper Dangaard Brouer <[email protected]>

Signed-off-by Hans Schillstrom <[email protected]>

---

 include/net/ip_vs.h                     |  173 +++++++++++++++------
 net/netfilter/ipvs/ip_vs_conn.c         |   15 +-
 net/netfilter/ipvs/ip_vs_core.c         |  253 +++++++++++++------------------
 net/netfilter/ipvs/ip_vs_dh.c           |    2 
 net/netfilter/ipvs/ip_vs_lblc.c         |    2 
 net/netfilter/ipvs/ip_vs_lblcr.c        |    2 
 net/netfilter/ipvs/ip_vs_pe_sip.c       |   27 ++-
 net/netfilter/ipvs/ip_vs_proto_ah_esp.c |    9 -
 net/netfilter/ipvs/ip_vs_proto_sctp.c   |   42 ++---
 net/netfilter/ipvs/ip_vs_proto_tcp.c    |   40 ++---
 net/netfilter/ipvs/ip_vs_proto_udp.c    |   41 ++---
 net/netfilter/ipvs/ip_vs_sh.c           |    2 
 net/netfilter/ipvs/ip_vs_xmit.c         |   41 +++--
 net/netfilter/xt_ipvs.c                 |    4 
 14 files changed, 340 insertions(+), 313 deletions(-)

include/net/ip_vs.h +123 ~~-50~~

index aba0bb2..8d5920f 100644
@@ -22,6 +22,9 @@
 #include <linux/ip.h>
 #include <linux/ipv6.h>			/* for struct ipv6hdr */
 #include <net/ipv6.h>
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
+#include <linux/netfilter_ipv6/ip6_tables.h>
+#endif
 #if defined(CONFIG_NF_CONNTRACK) || defined(CONFIG_NF_CONNTRACK_MODULE)
 #include <net/netfilter/nf_conntrack.h>
 #endif

@@ -103,30 +106,99 @@ static inline struct net *seq_file_single_net(struct seq_file *seq)
 /* Connections' size value needed by ip_vs_ctl.c */
 extern int ip_vs_conn_tab_size;
 
-
 struct ip_vs_iphdr {
-	int len;
-	__u8 protocol;
+	__u32 len;	/* IPv4 simply where L4 starts
+			   IPv6 where to find next header */
+	__u32 offs;	/* IPv6 frags: header offset in nfct_reasm skb */
+	__u16 fragoffs; /* IPv6 fragment offset, 0 if first frag (or not frag)*/
+	__s16 protocol;
+	__s32 flags;
 	union nf_inet_addr saddr;
 	union nf_inet_addr daddr;
 };
 
+/* Dependency to module: nf_defrag_ipv6 */
+#if defined(CONFIG_NF_DEFRAG_IPV6) || defined(CONFIG_NF_DEFRAG_IPV6_MODULE)
+static inline struct sk_buff *skb_nfct_reasm(const struct sk_buff *skb)
+{
+	return skb->nfct_reasm;
+}
+#else
+static inline struct sk_buff *skb_nfct_reasm(const struct sk_buff *skb)
+{
+	return NULL;
+}
+#endif
+
+static inline void
+ip_vs_fill_ip4hdr(const void *nh, struct ip_vs_iphdr *iphdr)
+{
+	const struct iphdr *iph = nh;
+
+	iphdr->len	= iph->ihl * 4;
+	iphdr->fragoffs	= 0;
+	iphdr->protocol	= iph->protocol;
+	iphdr->saddr.ip	= iph->saddr;
+	iphdr->daddr.ip	= iph->daddr;
+}
+
+/* This function handles filling *ip_vs_iphdr, both for IPv4 and IPv6.
+ * IPv6 requires some extra work, as finding proper header position,
+ * depend on the IPv6 extension headers.
+ */
 static inline void
-ip_vs_fill_iphdr(int af, const void *nh, struct ip_vs_iphdr *iphdr)
+ip_vs_fill_iph_skb(int af, const struct sk_buff *skb, struct ip_vs_iphdr *iphdr)
 {
 #ifdef CONFIG_IP_VS_IPV6
 	if (af == AF_INET6) {
-		const struct ipv6hdr *iph = nh;
-		iphdr->len = sizeof(struct ipv6hdr);
-		iphdr->protocol = iph->nexthdr;
+		const struct ipv6hdr *iph =
+			(struct ipv6hdr *)skb_network_header(skb);
 		iphdr->saddr.in6 = iph->saddr;
 		iphdr->daddr.in6 = iph->daddr;
+		/* ipv6_find_hdr() updates len, flags, offs */
+		iphdr->len	 = 0;
+		iphdr->flags	 = 0;
+		iphdr->offs	 = 0;
+		iphdr->protocol  = ipv6_find_hdr(skb, &iphdr->len, -1,
+						 &iphdr->fragoffs,
+						 &iphdr->flags);
+		/* get proto from re-assembled packet and it's offset */
+		if (skb_nfct_reasm(skb))
+			iphdr->protocol = ipv6_find_hdr(skb_nfct_reasm(skb),
+							&iphdr->offs, -1, NULL,
+							NULL);
 	} else
 #endif
 	{
-		const struct iphdr *iph = nh;
-		iphdr->len = iph->ihl * 4;
-		iphdr->protocol = iph->protocol;
+		const struct iphdr *iph =
+			(struct iphdr *)skb_network_header(skb);
+		iphdr->len	= iph->ihl * 4;
+		iphdr->fragoffs	= 0;
+		iphdr->protocol	= iph->protocol;
+		iphdr->saddr.ip	= iph->saddr;
+		iphdr->daddr.ip	= iph->daddr;
+	}
+}
+
+/* This function is a faster version of ip_vs_fill_iph_skb().
+ * Where we only populate {s,d}addr (and avoid calling ipv6_find_hdr()).
+ * This is used by the some of the ip_vs_*_schedule() functions.
+ * (Mostly done to avoid ABI breakage of external schedulers)
+ */
+static inline void
+ip_vs_fill_iph_addr_only(int af, const struct sk_buff *skb,
+			 struct ip_vs_iphdr *iphdr)
+{
+#ifdef CONFIG_IP_VS_IPV6
+	if (af == AF_INET6) {
+		const struct ipv6hdr *iph =
+			(struct ipv6hdr *)skb_network_header(skb);
+		iphdr->saddr.in6 = iph->saddr;
+		iphdr->daddr.in6 = iph->daddr;
+	} else {
+#endif
+		const struct iphdr *iph =
+			(struct iphdr *)skb_network_header(skb);
 		iphdr->saddr.ip = iph->saddr;
 		iphdr->daddr.ip = iph->daddr;
 	}

@@ -398,27 +470,26 @@ struct ip_vs_protocol {
 
 	int (*conn_schedule)(int af, struct sk_buff *skb,
 			     struct ip_vs_proto_data *pd,
-			     int *verdict, struct ip_vs_conn **cpp);
+			     int *verdict, struct ip_vs_conn **cpp,
+			     struct ip_vs_iphdr *iph);
 
 	struct ip_vs_conn *
 	(*conn_in_get)(int af,
 		       const struct sk_buff *skb,
 		       const struct ip_vs_iphdr *iph,
-		       unsigned int proto_off,
 		       int inverse);
 
 	struct ip_vs_conn *
 	(*conn_out_get)(int af,
 			const struct sk_buff *skb,
 			const struct ip_vs_iphdr *iph,
-			unsigned int proto_off,
 			int inverse);
 
-	int (*snat_handler)(struct sk_buff *skb,
-			    struct ip_vs_protocol *pp, struct ip_vs_conn *cp);
+	int (*snat_handler)(struct sk_buff *skb, struct ip_vs_protocol *pp,
+			    struct ip_vs_conn *cp, struct ip_vs_iphdr *iph);
 
-	int (*dnat_handler)(struct sk_buff *skb,
-			    struct ip_vs_protocol *pp, struct ip_vs_conn *cp);
+	int (*dnat_handler)(struct sk_buff *skb, struct ip_vs_protocol *pp,
+			    struct ip_vs_conn *cp, struct ip_vs_iphdr *iph);
 
 	int (*csum_check)(int af, struct sk_buff *skb,
 			  struct ip_vs_protocol *pp);

@@ -518,7 +589,7 @@ struct ip_vs_conn {
 	   NF_ACCEPT can be returned when destination is local.
 	 */
 	int (*packet_xmit)(struct sk_buff *skb, struct ip_vs_conn *cp,
-			   struct ip_vs_protocol *pp);
+			   struct ip_vs_protocol *pp, struct ip_vs_iphdr *iph);
 
 	/* Note: we can group the following members into a structure,
 	   in order to save more space, and the following members are

@@ -769,13 +840,11 @@ struct ip_vs_app {
 
 	struct ip_vs_conn *
 	(*conn_in_get)(const struct sk_buff *skb, struct ip_vs_app *app,
-		       const struct iphdr *iph, unsigned int proto_off,
-		       int inverse);
+		       const struct iphdr *iph, int inverse);
 
 	struct ip_vs_conn *
 	(*conn_out_get)(const struct sk_buff *skb, struct ip_vs_app *app,
-			const struct iphdr *iph, unsigned int proto_off,
-			int inverse);
+			const struct iphdr *iph, int inverse);
 
 	int (*state_transition)(struct ip_vs_conn *cp, int direction,
 				const struct sk_buff *skb,

@@ -1074,14 +1143,12 @@ struct ip_vs_conn *ip_vs_ct_in_get(const struct ip_vs_conn_param *p);
 
 struct ip_vs_conn * ip_vs_conn_in_get_proto(int af, const struct sk_buff *skb,
 					    const struct ip_vs_iphdr *iph,
-					    unsigned int proto_off,
 					    int inverse);
 
 struct ip_vs_conn *ip_vs_conn_out_get(const struct ip_vs_conn_param *p);
 
 struct ip_vs_conn * ip_vs_conn_out_get_proto(int af, const struct sk_buff *skb,
 					     const struct ip_vs_iphdr *iph,
-					     unsigned int proto_off,
 					     int inverse);
 
 /* put back the conn without restarting its timer */

@@ -1254,9 +1321,10 @@ extern struct ip_vs_scheduler *ip_vs_scheduler_get(const char *sched_name);
 extern void ip_vs_scheduler_put(struct ip_vs_scheduler *scheduler);
 extern struct ip_vs_conn *
 ip_vs_schedule(struct ip_vs_service *svc, struct sk_buff *skb,
-	       struct ip_vs_proto_data *pd, int *ignored);
+	       struct ip_vs_proto_data *pd, int *ignored,
+	       struct ip_vs_iphdr *iph);
 extern int ip_vs_leave(struct ip_vs_service *svc, struct sk_buff *skb,
-			struct ip_vs_proto_data *pd);
+			struct ip_vs_proto_data *pd, struct ip_vs_iphdr *iph);
 
 extern void ip_vs_scheduler_err(struct ip_vs_service *svc, const char *msg);

@@ -1315,33 +1383,38 @@ extern void ip_vs_read_estimator(struct ip_vs_stats_user *dst,
 /*
  *	Various IPVS packet transmitters (from ip_vs_xmit.c)
  */
-extern int ip_vs_null_xmit
-(struct sk_buff *skb, struct ip_vs_conn *cp, struct ip_vs_protocol *pp);
-extern int ip_vs_bypass_xmit
-(struct sk_buff *skb, struct ip_vs_conn *cp, struct ip_vs_protocol *pp);
-extern int ip_vs_nat_xmit
-(struct sk_buff *skb, struct ip_vs_conn *cp, struct ip_vs_protocol *pp);
-extern int ip_vs_tunnel_xmit
-(struct sk_buff *skb, struct ip_vs_conn *cp, struct ip_vs_protocol *pp);
-extern int ip_vs_dr_xmit
-(struct sk_buff *skb, struct ip_vs_conn *cp, struct ip_vs_protocol *pp);
-extern int ip_vs_icmp_xmit
-(struct sk_buff *skb, struct ip_vs_conn *cp, struct ip_vs_protocol *pp,
- int offset, unsigned int hooknum);
+extern int ip_vs_null_xmit(struct sk_buff *skb, struct ip_vs_conn *cp,
+			   struct ip_vs_protocol *pp, struct ip_vs_iphdr *iph);
+extern int ip_vs_bypass_xmit(struct sk_buff *skb, struct ip_vs_conn *cp,
+			     struct ip_vs_protocol *pp,
+			     struct ip_vs_iphdr *iph);
+extern int ip_vs_nat_xmit(struct sk_buff *skb, struct ip_vs_conn *cp,
+			  struct ip_vs_protocol *pp, struct ip_vs_iphdr *iph);
+extern int ip_vs_tunnel_xmit(struct sk_buff *skb, struct ip_vs_conn *cp,
+			     struct ip_vs_protocol *pp,
+			     struct ip_vs_iphdr *iph);
+extern int ip_vs_dr_xmit(struct sk_buff *skb, struct ip_vs_conn *cp,
+			 struct ip_vs_protocol *pp, struct ip_vs_iphdr *iph);
+extern int ip_vs_icmp_xmit(struct sk_buff *skb, struct ip_vs_conn *cp,
+			   struct ip_vs_protocol *pp, int offset,
+			   unsigned int hooknum, struct ip_vs_iphdr *iph);
 extern void ip_vs_dst_reset(struct ip_vs_dest *dest);
 
 #ifdef CONFIG_IP_VS_IPV6
-extern int ip_vs_bypass_xmit_v6
-(struct sk_buff *skb, struct ip_vs_conn *cp, struct ip_vs_protocol *pp);
-extern int ip_vs_nat_xmit_v6
-(struct sk_buff *skb, struct ip_vs_conn *cp, struct ip_vs_protocol *pp);
-extern int ip_vs_tunnel_xmit_v6
-(struct sk_buff *skb, struct ip_vs_conn *cp, struct ip_vs_protocol *pp);
-extern int ip_vs_dr_xmit_v6
-(struct sk_buff *skb, struct ip_vs_conn *cp, struct ip_vs_protocol *pp);
-extern int ip_vs_icmp_xmit_v6
-(struct sk_buff *skb, struct ip_vs_conn *cp, struct ip_vs_protocol *pp,
- int offset, unsigned int hooknum);
+extern int ip_vs_bypass_xmit_v6(struct sk_buff *skb, struct ip_vs_conn *cp,
+				struct ip_vs_protocol *pp,
+				struct ip_vs_iphdr *iph);
+extern int ip_vs_nat_xmit_v6(struct sk_buff *skb, struct ip_vs_conn *cp,
+			     struct ip_vs_protocol *pp,
+			     struct ip_vs_iphdr *iph);
+extern int ip_vs_tunnel_xmit_v6(struct sk_buff *skb, struct ip_vs_conn *cp,
+				struct ip_vs_protocol *pp,
+				struct ip_vs_iphdr *iph);
+extern int ip_vs_dr_xmit_v6(struct sk_buff *skb, struct ip_vs_conn *cp,
+			    struct ip_vs_protocol *pp, struct ip_vs_iphdr *iph);
+extern int ip_vs_icmp_xmit_v6(struct sk_buff *skb, struct ip_vs_conn *cp,
+			      struct ip_vs_protocol *pp, int offset,
+			      unsigned int hooknum, struct ip_vs_iphdr *iph);
 #endif
 
 #ifdef CONFIG_SYSCTL

net/netfilter/ipvs/ip_vs_conn.c +6 -9

index 1548df9..a00db99 100644
@@ -308,13 +308,12 @@ struct ip_vs_conn *ip_vs_conn_in_get(const struct ip_vs_conn_param *p)
 static int
 ip_vs_conn_fill_param_proto(int af, const struct sk_buff *skb,
 			    const struct ip_vs_iphdr *iph,
-			    unsigned int proto_off, int inverse,
-			    struct ip_vs_conn_param *p)
+			    int inverse, struct ip_vs_conn_param *p)
 {
 	__be16 _ports[2], *pptr;
 	struct net *net = skb_net(skb);
 
-	pptr = skb_header_pointer(skb, proto_off, sizeof(_ports), _ports);
+	pptr = skb_header_pointer(skb, iph->len, sizeof(_ports), _ports);
 	if (pptr == NULL)
 		return 1;

@@ -329,12 +328,11 @@ ip_vs_conn_fill_param_proto(int af, const struct sk_buff *skb,
 
 struct ip_vs_conn *
 ip_vs_conn_in_get_proto(int af, const struct sk_buff *skb,
-			const struct ip_vs_iphdr *iph,
-			unsigned int proto_off, int inverse)
+			const struct ip_vs_iphdr *iph, int inverse)
 {
 	struct ip_vs_conn_param p;
 
-	if (ip_vs_conn_fill_param_proto(af, skb, iph, proto_off, inverse, &p))
+	if (ip_vs_conn_fill_param_proto(af, skb, iph, inverse, &p))
 		return NULL;
 
 	return ip_vs_conn_in_get(&p);

@@ -432,12 +430,11 @@ struct ip_vs_conn *ip_vs_conn_out_get(const struct ip_vs_conn_param *p)
 
 struct ip_vs_conn *
 ip_vs_conn_out_get_proto(int af, const struct sk_buff *skb,
-			 const struct ip_vs_iphdr *iph,
-			 unsigned int proto_off, int inverse)
+			 const struct ip_vs_iphdr *iph, int inverse)
 {
 	struct ip_vs_conn_param p;
 
-	if (ip_vs_conn_fill_param_proto(af, skb, iph, proto_off, inverse, &p))
+	if (ip_vs_conn_fill_param_proto(af, skb, iph, inverse, &p))
 		return NULL;
 
 	return ip_vs_conn_out_get(&p);

net/netfilter/ipvs/ip_vs_core.c +108 ~~-145~~

576 lines not on this page · raw message

net/netfilter/ipvs/ip_vs_dh.c +1 -1

index 8b7dca9..7f3b0cc 100644
@@ -215,7 +215,7 @@ ip_vs_dh_schedule(struct ip_vs_service *svc, const struct sk_buff *skb)
 	struct ip_vs_dh_bucket *tbl;
 	struct ip_vs_iphdr iph;
 
-	ip_vs_fill_iphdr(svc->af, skb_network_header(skb), &iph);
+	ip_vs_fill_iph_addr_only(svc->af, skb, &iph);
 
 	IP_VS_DBG(6, "%s(): Scheduling...\n", __func__);

net/netfilter/ipvs/ip_vs_lblc.c +1 -1

index df646cc..cbd3748 100644
@@ -479,7 +479,7 @@ ip_vs_lblc_schedule(struct ip_vs_service *svc, const struct sk_buff *skb)
 	struct ip_vs_dest *dest = NULL;
 	struct ip_vs_lblc_entry *en;
 
-	ip_vs_fill_iphdr(svc->af, skb_network_header(skb), &iph);
+	ip_vs_fill_iph_addr_only(svc->af, skb, &iph);
 
 	IP_VS_DBG(6, "%s(): Scheduling...\n", __func__);

net/netfilter/ipvs/ip_vs_lblcr.c +1 -1

index 570e31e..161b679 100644
@@ -649,7 +649,7 @@ ip_vs_lblcr_schedule(struct ip_vs_service *svc, const struct sk_buff *skb)
 	struct ip_vs_dest *dest = NULL;
 	struct ip_vs_lblcr_entry *en;
 
-	ip_vs_fill_iphdr(svc->af, skb_network_header(skb), &iph);
+	ip_vs_fill_iph_addr_only(svc->af, skb, &iph);
 
 	IP_VS_DBG(6, "%s(): Scheduling...\n", __func__);

net/netfilter/ipvs/ip_vs_pe_sip.c +19 -8

index 1aa5cac..bb28b4f 100644
@@ -68,26 +68,37 @@ static int get_callid(const char *dptr, unsigned int dataoff,
 static int
 ip_vs_sip_fill_param(struct ip_vs_conn_param *p, struct sk_buff *skb)
 {
+	struct sk_buff *reasm = skb_nfct_reasm(skb);
 	struct ip_vs_iphdr iph;
 	unsigned int dataoff, datalen, matchoff, matchlen;
 	const char *dptr;
 	int retc;
 
-	ip_vs_fill_iphdr(p->af, skb_network_header(skb), &iph);
+	ip_vs_fill_iph_skb(p->af, skb, &iph);
 
 	/* Only useful with UDP */
 	if (iph.protocol != IPPROTO_UDP)
 		return -EINVAL;
+	/*
+	 * todo: IPv6 fragments:
+	 *       I think this only should be done for the first fragment. /HS
+	 */
+	if (!reasm) {
+		reasm = skb;
+		dataoff = iph.len + sizeof(struct udphdr);
+	} else
+		dataoff = iph.offs + sizeof(struct udphdr);
 
-	/* No Data ? */
-	dataoff = iph.len + sizeof(struct udphdr);
-	if (dataoff >= skb->len)
+	if (dataoff >= reasm->len)
 		return -EINVAL;
-
-	if ((retc=skb_linearize(skb)) < 0)
+	/*
+	 * todo: Check if this will mess-up the reasm skb !!! /HS
+	 */
+	retc = skb_linearize(reasm);
+	if (retc < 0)
 		return retc;
-	dptr = skb->data + dataoff;
-	datalen = skb->len - dataoff;
+	dptr = reasm->data + dataoff;
+	datalen = reasm->len - dataoff;
 
 	if (get_callid(dptr, dataoff, datalen, &matchoff, &matchlen))
 		return -EINVAL;

net/netfilter/ipvs/ip_vs_proto_ah_esp.c +4 -5

index 5b8eb8b..5de3dd3 100644
@@ -57,7 +57,7 @@ ah_esp_conn_fill_param_proto(struct net *net, int af,
 
 static struct ip_vs_conn *
 ah_esp_conn_in_get(int af, const struct sk_buff *skb,
-		   const struct ip_vs_iphdr *iph, unsigned int proto_off,
+		   const struct ip_vs_iphdr *iph,
 		   int inverse)
 {
 	struct ip_vs_conn *cp;

@@ -85,9 +85,7 @@ ah_esp_conn_in_get(int af, const struct sk_buff *skb,
 
 static struct ip_vs_conn *
 ah_esp_conn_out_get(int af, const struct sk_buff *skb,
-		    const struct ip_vs_iphdr *iph,
-		    unsigned int proto_off,
-		    int inverse)
+		    const struct ip_vs_iphdr *iph, int inverse)
 {
 	struct ip_vs_conn *cp;
 	struct ip_vs_conn_param p;

@@ -110,7 +108,8 @@ ah_esp_conn_out_get(int af, const struct sk_buff *skb,
 
 static int
 ah_esp_conn_schedule(int af, struct sk_buff *skb, struct ip_vs_proto_data *pd,
-		     int *verdict, struct ip_vs_conn **cpp)
+		     int *verdict, struct ip_vs_conn **cpp,
+		     struct ip_vs_iphdr *iph)
 {
 	/*
 	 * AH/ESP is only related traffic. Pass the packet to IP stack.

net/netfilter/ipvs/ip_vs_proto_sctp.c +18 ~~-24~~

96 lines not on this page · raw message

net/netfilter/ipvs/ip_vs_proto_tcp.c +17 ~~-23~~

index cd609cc..9af653a 100644
@@ -33,16 +33,14 @@
 
 static int
 tcp_conn_schedule(int af, struct sk_buff *skb, struct ip_vs_proto_data *pd,
-		  int *verdict, struct ip_vs_conn **cpp)
+		  int *verdict, struct ip_vs_conn **cpp,
+		  struct ip_vs_iphdr *iph)
 {
 	struct net *net;
 	struct ip_vs_service *svc;
 	struct tcphdr _tcph, *th;
-	struct ip_vs_iphdr iph;
 
-	ip_vs_fill_iphdr(af, skb_network_header(skb), &iph);
-
-	th = skb_header_pointer(skb, iph.len, sizeof(_tcph), &_tcph);
+	th = skb_header_pointer(skb, iph->len, sizeof(_tcph), &_tcph);
 	if (th == NULL) {
 		*verdict = NF_DROP;
 		return 0;

@@ -50,8 +48,8 @@ tcp_conn_schedule(int af, struct sk_buff *skb, struct ip_vs_proto_data *pd,
 	net = skb_net(skb);
 	/* No !th->ack check to allow scheduling on SYN+ACK for Active FTP */
 	if (th->syn &&
-	    (svc = ip_vs_service_get(net, af, skb->mark, iph.protocol,
-				     &iph.daddr, th->dest))) {
+	    (svc = ip_vs_service_get(net, af, skb->mark, iph->protocol,
+				     &iph->daddr, th->dest))) {
 		int ignored;
 
 		if (ip_vs_todrop(net_ipvs(net))) {

@@ -68,10 +66,10 @@ tcp_conn_schedule(int af, struct sk_buff *skb, struct ip_vs_proto_data *pd,
 		 * Let the virtual server select a real server for the
 		 * incoming connection, and create a connection entry.
 		 */
-		*cpp = ip_vs_schedule(svc, skb, pd, &ignored);
+		*cpp = ip_vs_schedule(svc, skb, pd, &ignored, iph);
 		if (!*cpp && ignored <= 0) {
 			if (!ignored)
-				*verdict = ip_vs_leave(svc, skb, pd);
+				*verdict = ip_vs_leave(svc, skb, pd, iph);
 			else {
 				ip_vs_service_put(svc);
 				*verdict = NF_DROP;

@@ -128,20 +126,18 @@ tcp_partial_csum_update(int af, struct tcphdr *tcph,
 
 
 static int
-tcp_snat_handler(struct sk_buff *skb,
-		 struct ip_vs_protocol *pp, struct ip_vs_conn *cp)
+tcp_snat_handler(struct sk_buff *skb, struct ip_vs_protocol *pp,
+		 struct ip_vs_conn *cp, struct ip_vs_iphdr *iph)
 {
 	struct tcphdr *tcph;
-	unsigned int tcphoff;
+	unsigned int tcphoff = iph->len;
 	int oldlen;
 	int payload_csum = 0;
 
 #ifdef CONFIG_IP_VS_IPV6
-	if (cp->af == AF_INET6)
-		tcphoff = sizeof(struct ipv6hdr);
-	else
+	if (cp->af == AF_INET6 && iph->fragoffs)
+		return 1;
 #endif
-		tcphoff = ip_hdrlen(skb);
 	oldlen = skb->len - tcphoff;
 
 	/* csum_check requires unshared skb */

@@ -208,20 +204,18 @@ tcp_snat_handler(struct sk_buff *skb,
 
 
 static int
-tcp_dnat_handler(struct sk_buff *skb,
-		 struct ip_vs_protocol *pp, struct ip_vs_conn *cp)
+tcp_dnat_handler(struct sk_buff *skb, struct ip_vs_protocol *pp,
+		 struct ip_vs_conn *cp, struct ip_vs_iphdr *iph)
 {
 	struct tcphdr *tcph;
-	unsigned int tcphoff;
+	unsigned int tcphoff = iph->len;
 	int oldlen;
 	int payload_csum = 0;
 
 #ifdef CONFIG_IP_VS_IPV6
-	if (cp->af == AF_INET6)
-		tcphoff = sizeof(struct ipv6hdr);
-	else
+	if (cp->af == AF_INET6 && iph->fragoffs)
+		return 1;
 #endif
-		tcphoff = ip_hdrlen(skb);
 	oldlen = skb->len - tcphoff;
 
 	/* csum_check requires unshared skb */

net/netfilter/ipvs/ip_vs_proto_udp.c +18 ~~-23~~

91 lines not on this page · raw message

net/netfilter/ipvs/ip_vs_sh.c +1 -1

index 0512652..e331269 100644
@@ -228,7 +228,7 @@ ip_vs_sh_schedule(struct ip_vs_service *svc, const struct sk_buff *skb)
 	struct ip_vs_sh_bucket *tbl;
 	struct ip_vs_iphdr iph;
 
-	ip_vs_fill_iphdr(svc->af, skb_network_header(skb), &iph);
+	ip_vs_fill_iph_addr_only(svc->af, skb, &iph);
 
 	IP_VS_DBG(6, "ip_vs_sh_schedule(): Scheduling...\n");

net/netfilter/ipvs/ip_vs_xmit.c +21 ~~-20~~

143 lines not on this page · raw message

net/netfilter/xt_ipvs.c +2 -2

index bb10b07..8d47c37 100644
@@ -67,7 +67,7 @@ ipvs_mt(const struct sk_buff *skb, struct xt_action_param *par)
 		goto out;
 	}
 
-	ip_vs_fill_iphdr(family, skb_network_header(skb), &iph);
+	ip_vs_fill_iph_skb(family, skb, &iph);
 
 	if (data->bitmask & XT_IPVS_PROTO)
 		if ((iph.protocol == data->l4proto) ^

@@ -85,7 +85,7 @@ ipvs_mt(const struct sk_buff *skb, struct xt_action_param *par)
 	/*
 	 * Check if the packet belongs to an existing entry
 	 */
-	cp = pp->conn_out_get(family, skb, &iph, iph.len, 1 /* inverse */);
+	cp = pp->conn_out_get(family, skb, &iph, 1 /* inverse */);
 	if (unlikely(cp == NULL)) {
 		match = false;
 		goto out;

Jesper Dangaard Brouer

author

2012-08-20 13:08 UTC #8 lore raw

PATCH 3/3 ipvs: Complete IPv6 fragment handling for IPVS

In reply to Jesper Dangaard Brouer

IPVS now supports fragmented packets, with support from nf_conntrack_reasm.c

Based on patch from: Hans Schillstrom.

IPVS do like conntrack i.e. use the skb->nfct_reasm (i.e. when all fragments is collected, nf_ct_frag6_output() starts a "re-play" of all fragments into the interrupted PREROUTING chain at prio -399 (NF_IP6_PRI_CONNTRACK_DEFRAG+1) with nfct_reasm pointing to the assembled packet.)

Notice, module nf_defrag_ipv6 must be loaded for this to work.

IPVS adds a new hook into prerouting chain at prio -99 (NF_IP6_PRI_NAT_DST+1) to catch fragments, and copy fw-mark info from the first packet with an upper layer header.

Also, for IPv6, handle all ICMPv6 NONE Informational Messages (via ICMPV6_INFOMSG_MASK). This actually only extend our handling to type ICMPV6_PARAMPROB (Parameter Problem), and future types.

- Fixed refcnt bug since last.

Signed-off-by Jesper Dangaard Brouer <[email protected]>

Signed-off-by Hans Schillstrom <[email protected]>

---

 include/net/ip_vs.h             |   16 ++++
 net/netfilter/ipvs/Kconfig      |    7 +-
 net/netfilter/ipvs/ip_vs_conn.c |    2 
 net/netfilter/ipvs/ip_vs_core.c |  173 ++++++++++++++++++++++++++++-----------
 net/netfilter/ipvs/ip_vs_xmit.c |   24 ++++-
 5 files changed, 161 insertions(+), 61 deletions(-)

include/net/ip_vs.h +16 -0

index 8d5920f..50f377e 100644
@@ -123,11 +123,27 @@ static inline struct sk_buff *skb_nfct_reasm(const struct sk_buff *skb)
 {
 	return skb->nfct_reasm;
 }
+static inline void *frag_safe_skb_hp(const struct sk_buff *skb, int offset,
+				      int len, void *buffer,
+				      const struct ip_vs_iphdr *ipvsh)
+{
+	if (unlikely(ipvsh->fragoffs && skb_nfct_reasm(skb)))
+		return skb_header_pointer(skb_nfct_reasm(skb), ipvsh->offs,
+					  len, buffer);
+
+	return skb_header_pointer(skb, offset, len, buffer);
+}
 #else
 static inline struct sk_buff *skb_nfct_reasm(const struct sk_buff *skb)
 {
 	return NULL;
 }
+static inline void *frag_safe_skb_hp(const struct sk_buff *skb, int offset,
+				      int len, void *buffer,
+				      const struct ip_vs_iphdr *ipvsh)
+{
+	return skb_header_pointer(skb, offset, len, buffer);
+}
 #endif
 
 static inline void

net/netfilter/ipvs/Kconfig +3 -4

index 8b2cffd..0c3b167 100644
@@ -28,12 +28,11 @@ if IP_VS
 config	IP_VS_IPV6
 	bool "IPv6 support for IPVS"
 	depends on IPV6 = y || IP_VS = IPV6
+	select IP6_NF_IPTABLES
 	---help---
-	  Add IPv6 support to IPVS. This is incomplete and might be dangerous.
+	  Add IPv6 support to IPVS.
 
-	  See http://www.mindbasket.com/ipvs for more information.
-
-	  Say N if unsure.
+	  Say Y if unsure.
 
 config	IP_VS_DEBUG
 	bool "IP virtual server debugging"

net/netfilter/ipvs/ip_vs_conn.c +1 -1

index a00db99..30e764a 100644
@@ -313,7 +313,7 @@ ip_vs_conn_fill_param_proto(int af, const struct sk_buff *skb,
 	__be16 _ports[2], *pptr;
 	struct net *net = skb_net(skb);
 
-	pptr = skb_header_pointer(skb, iph->len, sizeof(_ports), _ports);
+	pptr = frag_safe_skb_hp(skb, iph->len, sizeof(_ports), _ports, iph);
 	if (pptr == NULL)
 		return 1;

net/netfilter/ipvs/ip_vs_core.c +124 ~~-49~~

325 lines not on this page · raw message

net/netfilter/ipvs/ip_vs_xmit.c +17 -7

index 925cca2..422b92f 100644
@@ -497,7 +497,9 @@ ip_vs_bypass_xmit_v6(struct sk_buff *skb, struct ip_vs_conn *cp,
 
 			skb->dev = net->loopback_dev;
 		}
-		icmpv6_send(skb, ICMPV6_PKT_TOOBIG, 0, mtu);
+		/* only send ICMP too big on first fragment */
+		if (!iph->fragoffs)
+			icmpv6_send(skb, ICMPV6_PKT_TOOBIG, 0, mtu);
 		dst_release(&rt->dst);
 		IP_VS_DBG_RL("%s(): frag needed\n", __func__);
 		goto tx_error;

@@ -667,7 +669,7 @@ ip_vs_nat_xmit_v6(struct sk_buff *skb, struct ip_vs_conn *cp,
 	EnterFunction(10);
 
 	/* check if it is a connection of no-client-port */
-	if (unlikely(cp->flags & IP_VS_CONN_F_NO_CPORT)) {
+	if (unlikely(cp->flags & IP_VS_CONN_F_NO_CPORT && !iph->fragoffs)) {
 		__be16 _pt, *p;
 		p = skb_header_pointer(skb, iph->len, sizeof(_pt), &_pt);
 		if (p == NULL)

@@ -693,7 +695,7 @@ ip_vs_nat_xmit_v6(struct sk_buff *skb, struct ip_vs_conn *cp,
 
 		if (ct && !nf_ct_is_untracked(ct)) {
 			IP_VS_DBG_RL_PKT(10, AF_INET6, pp, skb, 0,
-					 "ip_vs_nat_xmit_v6(): "
+					 "ip_vs_nat_xmit_v6(): "\
 					 "stopping DNAT to local address");
 			goto tx_error_put;
 		}

@@ -717,7 +719,9 @@ ip_vs_nat_xmit_v6(struct sk_buff *skb, struct ip_vs_conn *cp,
 
 			skb->dev = net->loopback_dev;
 		}
-		icmpv6_send(skb, ICMPV6_PKT_TOOBIG, 0, mtu);
+		/* only send ICMP too big on first fragment */
+		if (!iph->fragoffs)
+			icmpv6_send(skb, ICMPV6_PKT_TOOBIG, 0, mtu);
 		IP_VS_DBG_RL_PKT(0, AF_INET6, pp, skb, 0,
 				 "ip_vs_nat_xmit_v6(): frag needed for");
 		goto tx_error_put;

@@ -952,7 +956,9 @@ ip_vs_tunnel_xmit_v6(struct sk_buff *skb, struct ip_vs_conn *cp,
 
 			skb->dev = net->loopback_dev;
 		}
-		icmpv6_send(skb, ICMPV6_PKT_TOOBIG, 0, mtu);
+		/* only send ICMP too big on first fragment */
+		if (!ipvsh->fragoffs)
+			icmpv6_send(skb, ICMPV6_PKT_TOOBIG, 0, mtu);
 		IP_VS_DBG_RL("%s(): frag needed\n", __func__);
 		goto tx_error_put;
 	}

@@ -1118,7 +1124,9 @@ ip_vs_dr_xmit_v6(struct sk_buff *skb, struct ip_vs_conn *cp,
 
 			skb->dev = net->loopback_dev;
 		}
-		icmpv6_send(skb, ICMPV6_PKT_TOOBIG, 0, mtu);
+		/* only send ICMP too big on first fragment */
+		if (!iph->fragoffs)
+			icmpv6_send(skb, ICMPV6_PKT_TOOBIG, 0, mtu);
 		dst_release(&rt->dst);
 		IP_VS_DBG_RL("%s(): frag needed\n", __func__);
 		goto tx_error;

@@ -1356,7 +1364,9 @@ ip_vs_icmp_xmit_v6(struct sk_buff *skb, struct ip_vs_conn *cp,
 
 			skb->dev = net->loopback_dev;
 		}
-		icmpv6_send(skb, ICMPV6_PKT_TOOBIG, 0, mtu);
+		/* only send ICMP too big on first fragment */
+		if (!iph->fragoffs)
+			icmpv6_send(skb, ICMPV6_PKT_TOOBIG, 0, mtu);
 		IP_VS_DBG_RL("%s(): frag needed\n", __func__);
 		goto tx_error_put;
 	}

Simon Horman

maintainer

2012-08-21 05:24 UTC #9 lore raw

In reply to Jesper Dangaard Brouer

Quoted 14 lines · Jesper Dangaard Brouer

The following patchset implement IPv6 fragment handling for IPVS.
This work is based upon patches from Hans Schillstrom. I have taken over the patchset, in close agreement with Hans, because he don't have (gotten allocated) time to complete his work.
I have cleaned up the patchset, changed the API a bit, fixed a refcnt bug, and rebased on top of Julians recent changes. (All with Hans'es knowledge)
Patch01: is just unrelated trivial fixes.
Patch02: Fix faulty IPv6 extension header handling in IPVS

Quoted 8 lines

Patch03: Complete IPv6 fragment handling for IPVS
This patchset is based upon:
Homes ipvs-next tree:
git://git.kernel.org/pub/scm/linux/kernel/git/horms/ipvs-next.git
On top of commit 3654e61137db891f5312e6dd813b961484b5fdf3:
ipvs: add pmtu_disc option to disable IP DF for TUN packets

I have no objection to these changes, but I would be more comfortable applying them after a review from Hans, Julian or Pablo.

Quoted 36 lines

---

Jesper Dangaard Brouer (3):
ipvs: Complete IPv6 fragment handling for IPVS
ipvs: Fix faulty IPv6 extension header handling in IPVS
ipvs: Trivial changes, use compressed IPv6 address in output

 include/net/ip_vs.h                     |  191 +++++++++++----
 net/netfilter/ipvs/Kconfig              |    7 -
 net/netfilter/ipvs/ip_vs_conn.c         |   15 -
 net/netfilter/ipvs/ip_vs_core.c         |  384 +++++++++++++++++--------------
 net/netfilter/ipvs/ip_vs_dh.c           |    2 
 net/netfilter/ipvs/ip_vs_lblc.c         |    2 
 net/netfilter/ipvs/ip_vs_lblcr.c        |    2 
 net/netfilter/ipvs/ip_vs_pe_sip.c       |   27 ++
 net/netfilter/ipvs/ip_vs_proto.c        |    6 
 net/netfilter/ipvs/ip_vs_proto_ah_esp.c |    9 -
 net/netfilter/ipvs/ip_vs_proto_sctp.c   |   42 +--
 net/netfilter/ipvs/ip_vs_proto_tcp.c    |   40 +--
 net/netfilter/ipvs/ip_vs_proto_udp.c    |   41 +--
 net/netfilter/ipvs/ip_vs_sched.c        |    2 
 net/netfilter/ipvs/ip_vs_sh.c           |    2 
 net/netfilter/ipvs/ip_vs_xmit.c         |   75 +++---
 net/netfilter/xt_ipvs.c                 |    4 
 17 files changed, 489 insertions(+), 362 deletions(-)

--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Sr. Network Kernel Developer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer

Jesper Dangaard Brouer

author

2012-08-21 07:51 UTC #10 lore raw

Quoted 3 lines · Simon Horman

I have no objection to these changes, but I would be more comfortable applying them after a review from Hans, Julian or Pablo.

I would appreciate a review, especially from Julian.

I'm going to do some more extensive testing on the patchset, in this week. So, no hurry on applying these, we have time for a good review process.

Julian Anastasov

maintainer

2012-08-21 14:14 UTC #3 lore raw

Re: [PATCH 2/3] ipvs: Fix faulty IPv6 extension header handling in IPVS

In reply to Jesper Dangaard Brouer

Hello,

Quoted 24 lines · Jesper Dangaard Brouer

Based on patch from: Hans Schillstrom
IPv6 headers must be processed in order of appearance, neither can it be assumed that Upper layer headers is first. If anything else than L4 is the first header IPVS will throw it.
IPVS will write SNAT & DNAT modifications at a fixed pos which will corrupt the message. Proper header position must be found before writing modifying packet.
This patch contains a lot of API changes. This is done, to avoid the costly scan of finding the IPv6 headers, via ipv6_find_hdr(). Finding the IPv6 headers is done as early as possible, and passed on as a pointer "struct ip_vs_iphdr *" to the affected functions.
Notice, I have choosen, not to change the API of function pointer "(*schedule)" (in struct ip_vs_scheduler) as it can be used by external schedulers, via {un,}register_ip_vs_scheduler. Only 4 out of 10 schedulers use info from ip_vs_iphdr*, and when they do, they are only interested in iph->{s,d}addr.
This patch depends on commit 84018f55a:
"netfilter: ip6_tables: add flags parameter to ipv6_find_hdr()"

Quoted 6 lines

This also adds a dependency to ip6_tables.
Hans left some questions in ip_vs_pe_sip.c, which I'm uncertain about.
Signed-off-by Jesper Dangaard Brouer <[email protected]>
Signed-off-by Hans Schillstrom <[email protected]>

Patch 1 looks ok, following are some small comments
for patch 2 and 3.

include/net/ip_vs.h line 27

Quoted 3 lines

include/net/ip_vs.h +3 -0

+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
+#include <linux/netfilter_ipv6/ip6_tables.h>
+#endif

There is already #if IS_ENABLED(CONFIG_IPV6) that
can replace #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)

It seems we need IS_ENABLED for many places:
CONFIG_NF_CONNTRACK, CONFIG_NF_DEFRAG_IPV6

net/netfilter/ipvs/ip_vs_core.c static int ip_vs_out_icmp_v6(struct sk_buff *skb, int *related, · line 947

Quoted 10 lines

net/netfilter/ipvs/ip_vs_core.c +2 -4

@@ -958,34 +943,26 @@ static int ip_vs_out_icmp_v6(struct sk_buff *skb, int *related,
 	}
 
 	/* Now find the contained IP header */
-	offset += sizeof(_icmph);
-	cih = skb_header_pointer(skb, offset, sizeof(_ciph), &_ciph);
-	if (cih == NULL)
-		return NF_ACCEPT; /* The packet looks wrong, ignore */
+	ipvsh->len += sizeof(_icmph);
+	ip6 = skb_header_pointer(skb, ipvsh->len, sizeof(_ip6), &_ip6);

ip6 is not checked here for NULL or we rely on
ipv6_find_hdr checks?

Quoted 5 lines

net/netfilter/ipvs/ip_vs_core.c +0 -1

@@ -1506,39 +1464,43 @@ ip_vs_in_icmp_v6(struct sk_buff *skb, int *related, unsigned int hooknum)
 	}
 
 	/* Now find the contained IP header */
-	offset += sizeof(_icmph);

net/netfilter/ipvs/ip_vs_core.c ip_vs_in_icmp_v6(struct sk_buff *skb, int *related, unsigned int hooknum) · line 1473

Quoted 10 lines

net/netfilter/ipvs/ip_vs_core.c +7 -3

-	cih = skb_header_pointer(skb, offset, sizeof(_ciph), &_ciph);
-	if (cih == NULL)
-		return NF_ACCEPT; /* The packet looks wrong, ignore */
+	ciph.len = iph->len + sizeof(_icmph);
+	ciph.flags = 0;
+	ciph.fragoffs = 0;
+	ciph.protocol = ipv6_find_hdr(skb, &ciph.len, -1, &ciph.fragoffs,
+				      &ciph.flags);
+	ciph.saddr = iph->saddr;	/* con_in_get() handles reverse order */
+	ciph.daddr = iph->daddr;

The ciph initialization looks dangerous if one day
we add new field into the header.

Can we use ciph = (struct ip_vs_iphdr) { .XXX = val, ... },
in such case we have to call ipv6_find_hdr out of (after) this initialization? Of course, we will write twice to small fields such as protocol, len, fragoffs, flags

Also ipv6_find_hdr looks a bit noisy for missing header,
can it be a problem for the inner IPv6 header in ICMP messages?

In patch 3 ip_vs_in_icmp_v6 initializes ciph in the
same way. It will be difficult to audit the code later considering the large number of places where iph is used.

Quoted 29 lines

net/netfilter/ipvs/ip_vs_core.c +8 -8

 	net = skb_net(skb);
-	pd = ip_vs_proto_data_get(net, cih->nexthdr);
+	pd = ip_vs_proto_data_get(net, ciph.protocol);
 	if (!pd)
 		return NF_ACCEPT;
 	pp = pd->pp;
 
-	/* Is the embedded protocol header present? */
-	/* TODO: we don't support fragmentation at the moment anyways */
-	if (unlikely(cih->nexthdr == IPPROTO_FRAGMENT && pp->dont_defrag))
+	/* Is the embedded protocol header present?
+	 * If it's the second or later fragment we don't know what it is
+	 * i.e. just let it through.
+	 */
+	if (ciph.fragoffs)
 		return NF_ACCEPT;
 
+	offset = ciph.len;
 	IP_VS_DBG_PKT(11, AF_INET6, pp, skb, offset,
 		      "Checking incoming ICMPv6 for");
 
-	offset += sizeof(struct ipv6hdr);
-
-	ip_vs_fill_iphdr(AF_INET6, cih, &ciph);
 	/* The embedded headers contain source and dest in reverse order */
-	cp = pp->conn_in_get(AF_INET6, skb, &ciph, offset, 1);
+	cp = pp->conn_in_get(AF_INET6, skb, &ciph, 1);
 	if (!cp)
 		return NF_ACCEPT;

net/netfilter/ipvs/ip_vs_core.c ip_vs_in_icmp_v6(struct sk_buff *skb, int *related, unsigned int hooknum) · line 1501

Quoted 10 lines

net/netfilter/ipvs/ip_vs_core.c +3 -4

 
 	/* do the statistics and put it back */
 	ip_vs_in_stats(cp, skb);
-	if (IPPROTO_TCP == cih->nexthdr || IPPROTO_UDP == cih->nexthdr ||
-	    IPPROTO_SCTP == cih->nexthdr)
-		offset += 2 * sizeof(__u16);
-	verdict = ip_vs_icmp_xmit_v6(skb, cp, pp, offset, hooknum);
+	if (IPPROTO_TCP == ciph.protocol || IPPROTO_UDP == ciph.protocol ||
+	    IPPROTO_SCTP == ciph.protocol)
+		offset = ciph.len + (2 * sizeof(__u16));

Still in the same func, above code is correct but
patch 3 changes it back to wrong state (offs_ciph = ciph.len).

Quoted 11 lines

net/netfilter/ipvs/ip_vs_core.c +2 -0

+
+	verdict = ip_vs_icmp_xmit_v6(skb, cp, pp, offset, hooknum, &ciph);
 
 	__ip_vs_conn_put(cp);

net/netfilter/ipvs/ip_vs_pe_sip.c +0 -0

index 1aa5cac..bb28b4f 100644
@@ -68,26 +68,37 @@ static int get_callid(const char *dptr, unsigned int dataoff,
 static int

net/netfilter/ipvs/ip_vs_pe_sip.c static int get_callid(const char *dptr, unsigned int dataoff, · line 77

Quoted 10 lines

net/netfilter/ipvs/ip_vs_pe_sip.c +2 -1

 ip_vs_sip_fill_param(struct ip_vs_conn_param *p, struct sk_buff *skb)
 {
+	struct sk_buff *reasm = skb_nfct_reasm(skb);
 	struct ip_vs_iphdr iph;
 	unsigned int dataoff, datalen, matchoff, matchlen;
 	const char *dptr;
 	int retc;
 
-	ip_vs_fill_iphdr(p->af, skb_network_header(skb), &iph);
+	ip_vs_fill_iph_skb(p->af, skb, &iph);

May be skb_linearize is bad for IPv6? IIRC,
ip_vs_pe_sip.c needs access just to read the Call-ID. For IPv4 it was simple to use skb_linearize, may be the logic should be improved to read the values even from non-linear data. May be there is already some example code for this. For IPv6 I'm not sure what kind are the problems here, may be it depends if we try to call skb_linearize for reasm packet?

Quoted 5 lines

net/netfilter/ipvs/ip_vs_pe_sip.c +3 -2

-
-	if ((retc=skb_linearize(skb)) < 0)
+	/*
+	 * todo: Check if this will mess-up the reasm skb !!! /HS
+	 */

net/netfilter/ipvs/ip_vs_pe_sip.c static int get_callid(const char *dptr, unsigned int dataoff, · line 104

Quoted 10 lines

net/netfilter/ipvs/ip_vs_pe_sip.c +4 -2

+	retc = skb_linearize(reasm);
+	if (retc < 0)
 		return retc;
-	dptr = skb->data + dataoff;
-	datalen = skb->len - dataoff;
+	dptr = reasm->data + dataoff;
+	datalen = reasm->len - dataoff;
 
 	if (get_callid(dptr, dataoff, datalen, &matchoff, &matchlen))
 		return -EINVAL;

There are recents changes for IPv6 from Patrick McHardy:

http://marc.info/?l=netfilter-devel&m=134543406303402&w=2
http://marc.info/?l=netfilter-devel&m=134543407803412&w=2

In this context, may be soon we will modify ip_vs_ftp to
support IPv6. It is possible to work with present FTP helper in netfilter. Does it mean that we will see only reassembled packets when conntrack is running? No original fragments. Are we prepared to work in both ways (originals+reasm and just reasm) ?

- For patch 3 in Kconfig do we need 'select NF_DEFRAG_IPV6' ?

Basicly, I'm concerned what will happen when we
start to mangle the protocol payloads (FTP). For now we are safe by touching only addresses and ports. May be we have to synchronize these changes with the work from Patrick.

Regards

--
Julian Anastasov <[email protected]>

Simon Horman

maintainer

2012-08-22 06:42 UTC #11 lore raw

In reply to Jesper Dangaard Brouer

Quoted 9 lines · Jesper Dangaard Brouer

Quoted 3 lines · Simon Horman
I have no objection to these changes, but I would be more comfortable applying them after a review from Hans, Julian or Pablo.
I would appreciate a review, especially from Julian.
I'm going to do some more extensive testing on the patchset, in this week. So, no hurry on applying these, we have time for a good review process.

Thanks Jesper, and Julian and Hans who also responded. Let me know when you feel the series is ready to merge. I am also in no hurry.

Jesper Dangaard Brouer

author

2012-08-23 12:50 UTC #4 lore raw

Re: [PATCH 2/3] ipvs: Fix faulty IPv6 extension header handling in IPVS

In reply to Julian Anastasov

Quoted 36 lines · Julian Anastasov

Quoted 24 lines · Jesper Dangaard Brouer
Based on patch from: Hans Schillstrom
IPv6 headers must be processed in order of appearance, neither can it be assumed that Upper layer headers is first. If anything else than L4 is the first header IPVS will throw it.
IPVS will write SNAT & DNAT modifications at a fixed pos which will corrupt the message. Proper header position must be found before writing modifying packet.
This patch contains a lot of API changes. This is done, to avoid the costly scan of finding the IPv6 headers, via ipv6_find_hdr(). Finding the IPv6 headers is done as early as possible, and passed on as a pointer "struct ip_vs_iphdr *" to the affected functions.
Notice, I have choosen, not to change the API of function pointer "(*schedule)" (in struct ip_vs_scheduler) as it can be used by external schedulers, via {un,}register_ip_vs_scheduler. Only 4 out of 10 schedulers use info from ip_vs_iphdr*, and when they do, they are only interested in iph->{s,d}addr.
This patch depends on commit 84018f55a:
"netfilter: ip6_tables: add flags parameter to ipv6_find_hdr()"
Quoted 6 lines
This also adds a dependency to ip6_tables.
Hans left some questions in ip_vs_pe_sip.c, which I'm uncertain about.
Signed-off-by Jesper Dangaard Brouer <[email protected]>
Signed-off-by Hans Schillstrom <[email protected]>
Patch 1 looks ok, following are some small comments
for patch 2 and 3.

Quoted 6 lines

include/net/ip_vs.h line 27
Quoted 3 lines
include/net/ip_vs.h +3 -0
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
+#include <linux/netfilter_ipv6/ip6_tables.h>
+#endif
There is already #if IS_ENABLED(CONFIG_IPV6) that
can replace #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)

Okay.

Quoted 2 lines

It seems we need IS_ENABLED for many places:
CONFIG_NF_CONNTRACK, CONFIG_NF_DEFRAG_IPV6

Wondering if we should keep these cleanup changes to a separate patch.

Quoted 13 lines

net/netfilter/ipvs/ip_vs_core.c static int ip_vs_out_icmp_v6(struct sk_buff *skb, int *related, · line 947
Quoted 10 lines
net/netfilter/ipvs/ip_vs_core.c +2 -4
@@ -958,34 +943,26 @@ static int ip_vs_out_icmp_v6(struct sk_buff *skb, int *related,
 	}
 
 	/* Now find the contained IP header */
-	offset += sizeof(_icmph);
-	cih = skb_header_pointer(skb, offset, sizeof(_ciph), &_ciph);
-	if (cih == NULL)
-		return NF_ACCEPT; /* The packet looks wrong, ignore */
+	ipvsh->len += sizeof(_icmph);
+	ip6 = skb_header_pointer(skb, ipvsh->len, sizeof(_ip6), &_ip6);
ip6 is not checked here for NULL or we rely on
ipv6_find_hdr checks?

Good catch, I'll re-add the NULL pointer check.

Quoted 13 lines

net/netfilter/ipvs/ip_vs_core.c ip_vs_in_icmp_v6(struct sk_buff *skb, int *related, unsigned int hooknum) · line 1471

Quoted 13 lines

net/netfilter/ipvs/ip_vs_core.c +5 -4

@@ -1506,39 +1464,43 @@ ip_vs_in_icmp_v6(struct sk_buff *skb, int *related, unsigned int hooknum)
 	}
 
 	/* Now find the contained IP header */
-	offset += sizeof(_icmph);
-	cih = skb_header_pointer(skb, offset, sizeof(_ciph), &_ciph);
-	if (cih == NULL)
-		return NF_ACCEPT; /* The packet looks wrong, ignore */
+	ciph.len = iph->len + sizeof(_icmph);
+	ciph.flags = 0;
+	ciph.fragoffs = 0;
+	ciph.protocol = ipv6_find_hdr(skb, &ciph.len, -1, &ciph.fragoffs,
+				      &ciph.flags);

(notice that &ciph.len can get updated by ipv6_find_hdr())

Quoted 10 lines

net/netfilter/ipvs/ip_vs_core.c ip_vs_in_icmp_v6(struct sk_buff *skb, int *related, unsigned int hooknum) · line 1473
Quoted 2 lines
net/netfilter/ipvs/ip_vs_core.c +2 -0
+	ciph.saddr = iph->saddr;	/* con_in_get() handles reverse order */
+	ciph.daddr = iph->daddr;
The ciph initialization looks dangerous if one day
we add new field into the header.
Can we use ciph = (struct ip_vs_iphdr) { .XXX = val, ... },
in such case we have to call ipv6_find_hdr out of (after) this initialization? Of course, we will write twice to small fields such as protocol, len, fragoffs, flags

I'm not sure I follow/understand.

Quoted 2 lines

Also ipv6_find_hdr looks a bit noisy for missing header,
can it be a problem for the inner IPv6 header in ICMP messages?

I can see, that I don't handle the error cases of missing headers, from a call to ipv6_find_hdr() ... I guess I need to check if the return value "protocol" is negative. But I'm not sure if it matters in this case (Hans?)

Quoted 3 lines

In patch 3 ip_vs_in_icmp_v6 initializes ciph in the
same way. It will be difficult to audit the code later considering the large number of places where iph is used.

I'm not sure what you want me to do?

Quoted 30 lines

net/netfilter/ipvs/ip_vs_core.c +8 -8

 	net = skb_net(skb);
-	pd = ip_vs_proto_data_get(net, cih->nexthdr);
+	pd = ip_vs_proto_data_get(net, ciph.protocol);
 	if (!pd)
 		return NF_ACCEPT;
 	pp = pd->pp;
 
-	/* Is the embedded protocol header present? */
-	/* TODO: we don't support fragmentation at the moment anyways */
-	if (unlikely(cih->nexthdr == IPPROTO_FRAGMENT && pp->dont_defrag))
+	/* Is the embedded protocol header present?
+	 * If it's the second or later fragment we don't know what it is
+	 * i.e. just let it through.
+	 */
+	if (ciph.fragoffs)
 		return NF_ACCEPT;
 
+	offset = ciph.len;
 	IP_VS_DBG_PKT(11, AF_INET6, pp, skb, offset,
 		      "Checking incoming ICMPv6 for");
 
-	offset += sizeof(struct ipv6hdr);
-
-	ip_vs_fill_iphdr(AF_INET6, cih, &ciph);
 	/* The embedded headers contain source and dest in reverse order */
-	cp = pp->conn_in_get(AF_INET6, skb, &ciph, offset, 1);
+	cp = pp->conn_in_get(AF_INET6, skb, &ciph, 1);
 	if (!cp)
 		return NF_ACCEPT;

Quoted 12 lines

net/netfilter/ipvs/ip_vs_core.c ip_vs_in_icmp_v6(struct sk_buff *skb, int *related, unsigned int hooknum) · line 1501
Quoted 9 lines
net/netfilter/ipvs/ip_vs_core.c +3 -4
 	/* do the statistics and put it back */
 	ip_vs_in_stats(cp, skb);
-	if (IPPROTO_TCP == cih->nexthdr || IPPROTO_UDP == cih->nexthdr ||
-	    IPPROTO_SCTP == cih->nexthdr)
-		offset += 2 * sizeof(__u16);
-	verdict = ip_vs_icmp_xmit_v6(skb, cp, pp, offset, hooknum);
+	if (IPPROTO_TCP == ciph.protocol || IPPROTO_UDP == ciph.protocol ||
+	    IPPROTO_SCTP == ciph.protocol)
+		offset = ciph.len + (2 * sizeof(__u16));
Still in the same func, above code is correct but
patch 3 changes it back to wrong state (offs_ciph = ciph.len).

Don't think its a "bug" in patch3, it might be a "bug" in this patch. Because &ciph.len gets updated earlier by ipv6_find_hdr()... but I'm starting to get confused.
Perhaps we should move these changes to ip_vs_in_icmp_v6() into the same patch?

Quoted 19 lines

net/netfilter/ipvs/ip_vs_core.c +2 -0

+
+	verdict = ip_vs_icmp_xmit_v6(skb, cp, pp, offset, hooknum, &ciph);
 
 	__ip_vs_conn_put(cp);

net/netfilter/ipvs/ip_vs_pe_sip.c +1 -0

index 1aa5cac..bb28b4f 100644
@@ -68,26 +68,37 @@ static int get_callid(const char *dptr, unsigned int dataoff,
 static int
 ip_vs_sip_fill_param(struct ip_vs_conn_param *p, struct sk_buff *skb)
 {
+	struct sk_buff *reasm = skb_nfct_reasm(skb);
 	struct ip_vs_iphdr iph;
 	unsigned int dataoff, datalen, matchoff, matchlen;
 	const char *dptr;
 	int retc;

Quoted 11 lines

net/netfilter/ipvs/ip_vs_pe_sip.c static int get_callid(const char *dptr, unsigned int dataoff, · line 77
Quoted 2 lines
net/netfilter/ipvs/ip_vs_pe_sip.c +1 -1
-	ip_vs_fill_iphdr(p->af, skb_network_header(skb), &iph);
+	ip_vs_fill_iph_skb(p->af, skb, &iph);
May be skb_linearize is bad for IPv6? IIRC,
ip_vs_pe_sip.c needs access just to read the Call-ID. For IPv4 it was simple to use skb_linearize, may be the logic should be improved to read the values even from non-linear data. May be there is already some example code for this. For IPv6 I'm not sure what kind are the problems here, may be it depends if we try to call skb_linearize for reasm packet?

I'm not able to answer these questions, my self...

Quoted 18 lines

Quoted 5 lines
net/netfilter/ipvs/ip_vs_pe_sip.c +3 -2
-
-	if ((retc=skb_linearize(skb)) < 0)
+	/*
+	 * todo: Check if this will mess-up the reasm skb !!! /HS
+	 */
net/netfilter/ipvs/ip_vs_pe_sip.c static int get_callid(const char *dptr, unsigned int dataoff, · line 104
Quoted 10 lines
net/netfilter/ipvs/ip_vs_pe_sip.c +4 -2
+	retc = skb_linearize(reasm);
+	if (retc < 0)
 		return retc;
-	dptr = skb->data + dataoff;
-	datalen = skb->len - dataoff;
+	dptr = reasm->data + dataoff;
+	datalen = reasm->len - dataoff;
 
 	if (get_callid(dptr, dataoff, datalen, &matchoff, &matchlen))
 		return -EINVAL;
There are recents changes for IPv6 from Patrick McHardy:

Quoted 7 lines

http://marc.info/?l=netfilter-devel&m=134543406303402&w=2
http://marc.info/?l=netfilter-devel&m=134543407803412&w=2
In this context, may be soon we will modify ip_vs_ftp to
support IPv6. It is possible to work with present FTP helper in netfilter. Does it mean that we will see only reassembled packets when conntrack is running? No original fragments.

Yes, it seems that we will *only* see the reassembled packet (no original fragments) after Patricks patches. BUT *only* when loading the module nf_conntrack_ipv6.

Quoted 2 lines

Are we prepared to work in both ways (originals+reasm and just reasm) ?

As mentioned in another thread, no. But only because the reassembled packet will be dropped due to the MTU check. After this is fixed, the ipvs code seems to work. (Notice, this is both without and with my/these patches)

Quoted 1 line

- For patch 3 in Kconfig do we need 'select NF_DEFRAG_IPV6' ?

Yes, but it is still possible not to load the module nf_defrag_ipv6.

When not loading, nf_defrag_ipv6, these patches have no effect, and no fragments are passed through.

Quoted 5 lines

Basicly, I'm concerned what will happen when we
start to mangle the protocol payloads (FTP). For now we are safe by touching only addresses and ports. May be we have to synchronize these changes with the work from Patrick.

Yes, I think we should take Patrick's work into account.

My biggest concern is, that depending on which modules (nf_defrag_ipv6 only, or also nf_conntrack_ipv6) are loaded, different code paths are used (to support IPv6 fragments for IPVS).
This will be hard to understand, from a user perspective. And also difficult for us, when users report bugs...

Is loading nf_conntrack_ipv6 considered a big performance problem/issue for IPVS?
(Can we tell people, to enable conntrack for frag support?)

Signature · 5 lines

Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Sr. Network Kernel Developer at Red Hat
  Author of http://www.iptv-analyzer.org
  LinkedIn: http://www.linkedin.com/in/brouer

Julian Anastasov

maintainer

2012-08-23 16:06 UTC #5 lore raw

Re: [PATCH 2/3] ipvs: Fix faulty IPv6 extension header handling in IPVS

Hello,

Quoted 7 lines · Jesper Dangaard Brouer

Quoted 6 lines
include/net/ip_vs.h line 27
Quoted 3 lines
include/net/ip_vs.h +3 -0
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
+#include <linux/netfilter_ipv6/ip6_tables.h>
+#endif
There is already #if IS_ENABLED(CONFIG_IPV6) that
can replace #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)

Quoted 6 lines

Okay.
Quoted 2 lines
It seems we need IS_ENABLED for many places:
CONFIG_NF_CONNTRACK, CONFIG_NF_DEFRAG_IPV6
Wondering if we should keep these cleanup changes to a separate patch.

The way you prefer. We can fix it after this
patchset.

Quoted 19 lines

Quoted 10 lines
net/netfilter/ipvs/ip_vs_core.c ip_vs_in_icmp_v6(struct sk_buff *skb, int *related, unsigned int hooknum) · line 1471
Quoted 10 lines
net/netfilter/ipvs/ip_vs_core.c +5 -4
 	/* Now find the contained IP header */
-	offset += sizeof(_icmph);
-	cih = skb_header_pointer(skb, offset, sizeof(_ciph), &_ciph);
-	if (cih == NULL)
-		return NF_ACCEPT; /* The packet looks wrong, ignore */
+	ciph.len = iph->len + sizeof(_icmph);
+	ciph.flags = 0;
+	ciph.fragoffs = 0;
+	ciph.protocol = ipv6_find_hdr(skb, &ciph.len, -1, &ciph.fragoffs,
+				      &ciph.flags);
(notice that &ciph.len can get updated by ipv6_find_hdr())
Quoted 6 lines
net/netfilter/ipvs/ip_vs_core.c ip_vs_in_icmp_v6(struct sk_buff *skb, int *related, unsigned int hooknum) · line 1473
Quoted 2 lines
net/netfilter/ipvs/ip_vs_core.c +2 -0
+	ciph.saddr = iph->saddr;	/* con_in_get() handles reverse order */
+	ciph.daddr = iph->daddr;
The ciph initialization looks dangerous if one day
we add new field into the header.

Quoted 6 lines

Quoted 4 lines
Can we use ciph = (struct ip_vs_iphdr) { .XXX = val, ... },
in such case we have to call ipv6_find_hdr out of (after) this initialization? Of course, we will write twice to small fields such as protocol, len, fragoffs, flags
I'm not sure I follow/understand.

	For example:

	ciph = (struct ip_vs_iphdr) {
		.len = iph->len + sizeof(_icmph),
		.saddr = iph->saddr,
		.daddr = iph->daddr,
	};
	ciph.protocol = ipv6_find_hdr(skb, &ciph.len, -1, &ciph.fragoffs,
				      &ciph.flags);

	but I'm not sure if the saddr/daddr part compiles.

Quoted 7 lines

Quoted 2 lines
Also ipv6_find_hdr looks a bit noisy for missing header,
can it be a problem for the inner IPv6 header in ICMP messages?
I can see, that I don't handle the error cases of missing headers, from a call to ipv6_find_hdr() ... I guess I need to check if the return value "protocol" is negative. But I'm not sure if it matters in this case (Hans?)

ok. But I was referring to this message:

printk(KERN_ERR "IPv6 header not found\n");

Quoted 5 lines

Quoted 3 lines
In patch 3 ip_vs_in_icmp_v6 initializes ciph in the
same way. It will be difficult to audit the code later considering the large number of places where iph is used.
I'm not sure what you want me to do?

May be it is only in ip_vs_in_icmp_v6, my above
example is for ip_vs_in_icmp_v6, let me know if it compiles.

Quoted 8 lines

Quoted 2 lines
Still in the same func, above code is correct but
patch 3 changes it back to wrong state (offs_ciph = ciph.len).
Don't think its a "bug" in patch3, it might be a "bug" in this patch. Because &ciph.len gets updated earlier by ipv6_find_hdr()... but I'm starting to get confused.
Perhaps we should move these changes to ip_vs_in_icmp_v6() into the same patch?

ip_vs_icmp_xmit_v6 expects to see length that
will be made writeable, not exactly offset of header. May be you can rename the ip_vs_icmp_xmit_v6 argument from offset to wrtlen, it covers the ports because we are going to mangle them. Patch 3 should not change anymore there.

Quoted 8 lines

Quoted 4 lines
In this context, may be soon we will modify ip_vs_ftp to
support IPv6. It is possible to work with present FTP helper in netfilter. Does it mean that we will see only reassembled packets when conntrack is running? No original fragments.
Yes, it seems that we will *only* see the reassembled packet (no original fragments) after Patricks patches. BUT *only* when loading the module nf_conntrack_ipv6.

Yes, ok

Quoted 7 lines

Quoted 2 lines
Are we prepared to work in both ways (originals+reasm and just reasm) ?
As mentioned in another thread, no. But only because the reassembled packet will be dropped due to the MTU check. After this is fixed, the ipvs code seems to work. (Notice, this is both without and with my/these patches)

Very good

Quoted 3 lines

Quoted 1 line
- For patch 3 in Kconfig do we need 'select NF_DEFRAG_IPV6' ?
Yes, but it is still possible not to load the module nf_defrag_ipv6.

nf_defrag_ipv6 becomes mandatory for IPVS-IPv6.
Conntrack should be optional.

Quoted 2 lines

When not loading, nf_defrag_ipv6, these patches have no effect, and no fragments are passed through.

What happens with fragments if nf_defrag_ipv6 is
not loaded? IPVS will see original fragments without reasm ptr? I assume IPVS can see them but can not do much except to mangle addresses and ports.

Quoted 11 lines

Quoted 5 lines
Basicly, I'm concerned what will happen when we
start to mangle the protocol payloads (FTP). For now we are safe by touching only addresses and ports. May be we have to synchronize these changes with the work from Patrick.
Yes, I think we should take Patrick's work into account.
My biggest concern is, that depending on which modules (nf_defrag_ipv6 only, or also nf_conntrack_ipv6) are loaded, different code paths are used (to support IPv6 fragments for IPVS).

May be just like for IPv4, I think, nf_defrag_ipv6
should be loaded (added as dep) but nf_conntrack_ipv6 should be required only for FTP as we use some code from there.

Quoted 6 lines

This will be hard to understand, from a user perspective. And also difficult for us, when users report bugs...
Is loading nf_conntrack_ipv6 considered a big performance problem/issue for IPVS?
(Can we tell people, to enable conntrack for frag support?)

We should avoid it if possible. There can be small
routers that do not want conntrack. It would be better if IPVS uses some symbol that will cause nf_defrag_ipv6 to load, now we can add it as dep but what if user does not load it? nf_defrag_ipv6_hooks.c is very small, I'm not sure if we have to duplicate it just to cause defrag at prerouting or even input to work for us in the case without conntrack. Without this support we can not mangle FTP payload.

Regards

--
Julian Anastasov <[email protected]>

Patrick McHardy

2012-08-26 21:13 UTC #6 lore raw

Re: [PATCH 2/3] ipvs: Fix faulty IPv6 extension header handling in IPVS

In reply to Jesper Dangaard Brouer

Quoted 6 lines · Jesper Dangaard Brouer

Based on patch from: Hans Schillstrom
IPv6 headers must be processed in order of appearance, neither can it be assumed that Upper layer headers is first. If anything else than L4 is the first header IPVS will throw it.

Quoted 8 lines

IPVS will write SNAT & DNAT modifications at a fixed pos which will corrupt the message. Proper header position must be found before writing modifying packet.
This patch contains a lot of API changes. This is done, to avoid the costly scan of finding the IPv6 headers, via ipv6_find_hdr(). Finding the IPv6 headers is done as early as possible, and passed on as a pointer "struct ip_vs_iphdr *" to the affected functions.

How about we change netfilter to set up the skb's transport header at an early time so we can avoid all (most of) these header scans in netfilter?

Jesper Dangaard Brouer

author

2012-09-04 21:25 UTC #7 lore raw

Re: [PATCH 2/3] ipvs: Fix faulty IPv6 extension header handling in IPVS

On Mon, 20 Aug 2012, Jesper Dangaard Brouer wrote:

[cut]

Quoted 2 lines

This patch contains a lot of API changes. This is done, to avoid the costly scan of finding the IPv6 headers, via ipv6_find_hdr().

(small correction ipv6_find_hdr() is not that costly for the general case of no exthdrs)

Quoted 2 lines

Finding the IPv6 headers is done as early as possible, and passed on as a pointer "struct ip_vs_iphdr *" to the affected functions.

This passing the "struct ip_vs_iphdr" actually makes sense. It reminds me of the way netfilter/iptables passes the xt_actions_param to each rule. Which contains the same information as ip_vs_iphdr. (note ipvs register at hooks at a lower level and don't get passed the xt_actions_param).

Thus, perhaps we should keep these API changes. Even if we decide to optimize ipv6_find_hdr(). (as proposed by my RFC patch)

Perhaps we should consider adding a "family" to ip_vs_iphdr, as is done in xt_actions_param. This could help us, with collapsing IPv4 and IPv6 code, but i can see that other structs in IPVS carry this info already, so not sure its relevant.