Skip to contentSkip to conversation

iptables: bugfix: prevent wrong syntax being accepted by the set match

PATCH In review Dash Four 3 participants 14 messages Jun 16, 2013 – Jul 1, 2013 1 message not in the archive netfilter

lore
Share & tools
lore

Dash Four

author
#1 lore raw

PATCH v2 1/5 iptables: bugfix: prevent wrong syntax being accepted by the set match

In reply to a message not in the archive

This patch corrects a bug, which was allowing wrong src and dst syntax to be specified and accepted by the set match.

Signed-off-by Dash Four <[email protected]>
---
 extensions/libxt_set.h |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
extensions/libxt_set.h +4 -4
index 47c3f5b..a352ac3 100644
@@ -109,9 +109,9 @@ parse_dirs_v0(const char *opt_arg, struct xt_set_info_v0 *info)
 	
 	while (i < (IPSET_DIM_MAX - 1) && tmp != NULL) {
 		ptr = strsep(&tmp, ",");
-		if (strncmp(ptr, "src", 3) == 0)
+		if (strncmp(ptr, "src", 4) == 0)
 			info->u.flags[i++] |= IPSET_SRC;
-		else if (strncmp(ptr, "dst", 3) == 0)
+		else if (strncmp(ptr, "dst", 4) == 0)
 			info->u.flags[i++] |= IPSET_DST;
 		else
 			xtables_error(PARAMETER_PROBLEM,

@@ -135,9 +135,9 @@ parse_dirs(const char *opt_arg, struct xt_set_info *info)
 	while (info->dim < IPSET_DIM_MAX && tmp != NULL) {
 		info->dim++;
 		ptr = strsep(&tmp, ",");
-		if (strncmp(ptr, "src", 3) == 0)
+		if (strncmp(ptr, "src", 4) == 0)
 			info->flags |= (1 << info->dim);
-		else if (strncmp(ptr, "dst", 3) != 0)
+		else if (strncmp(ptr, "dst", 4) != 0)
 			xtables_error(PARAMETER_PROBLEM,
 				"You must spefify (the comma separated list of) 'src' or 'dst'.");
 	}
author
#2 lore raw

PATCH v2 2/5 ipset: add "inner" flag implementation

In reply to a message not in the archive

This patch implements "inner" flag option in set iptables match, allowing matching based on the properties (source/destination IP address, protocol, port and so on) of the original (inner) connection in the event of the following ICMP[v4,v6] messages:

ICMPv4 destination-unreachable (code 3);
ICMPv4 source-quench (code 4);
ICMPv4 time-exceeded (code 11);
ICMPv6 destination-unreachable (code 1);
ICMPv6 packet-too-big (code 2);
ICMPv6 time-exceeded (code 3);

Revision history:

v1 * initial revision
v2 * redundant code removed;
* added a new header file (ip_set_icmp.h) with 2 inline functions,

     allowing access to the internal icmp header properties;

* removed ip[46]inneraddr[ptr]functions as they are no longer needed
* added new ipv[46]addr[ptr] and ip_set_get*port functions, the old functions are still preserved for backwards compatibility

Signed-off-by Dash Four <[email protected]>
---
 kernel/include/linux/netfilter/ipset/ip_set.h      |   66 ++++++++++++++
 .../include/linux/netfilter/ipset/ip_set_getport.h |   16 ++++
 kernel/include/linux/netfilter/ipset/ip_set_icmp.h |   91 ++++++++++++++++++++
 kernel/include/uapi/linux/netfilter/ipset/ip_set.h |    2 +
 kernel/net/netfilter/ipset/ip_set_getport.c        |   78 +++++++++++++----
 5 files changed, 238 insertions(+), 15 deletions(-)
 create mode 100644 kernel/include/linux/netfilter/ipset/ip_set_icmp.h
kernel/include/linux/netfilter/ipset/ip_set.h +66 -0
index 8499e25..5b6ef72 100644
@@ -17,9 +17,13 @@
 #include <linux/netfilter/x_tables.h>
 #include <linux/stringify.h>
 #include <linux/vmalloc.h>
+#include <net/ip.h>
+#include <net/ipv6.h>
+#include <net/icmp.h>
 #include <net/netlink.h>
 #include <uapi/linux/netfilter/ipset/ip_set.h>
 #include <linux/netfilter/ipset/ip_set_compat.h>
+#include <linux/netfilter/ipset/ip_set_icmp.h>
 
 #define _IP_SET_MODULE_DESC(a, b, c)		\
 	MODULE_DESCRIPTION(a " type of IP sets, revisions " b "-" c)

@@ -361,6 +365,25 @@ static inline int nla_put_ipaddr6(struct sk_buff *skb, int type,
 }
 
 /* Get address from skbuff */
+static inline bool
+ipv4addrptr(const struct sk_buff *skb, bool inner, bool src, __be32 *addr)
+{
+	struct iphdr *ih = ip_hdr(skb);
+	unsigned int protooff = ip_hdrlen(skb);
+
+	if (ih == NULL ||
+	    (inner &&
+	     (ih->protocol != IPPROTO_ICMP ||
+	      !ip_set_get_icmpv4_inner_hdr(skb, &protooff, &ih))))
+			goto err;
+
+	*addr = src ? ih->saddr : ih->daddr;
+	return true;
+
+err:
+	return false;
+}
+
 static inline __be32
 ip4addr(const struct sk_buff *skb, bool src)
 {

@@ -373,12 +396,55 @@ ip4addrptr(const struct sk_buff *skb, bool src, __be32 *addr)
 	*addr = src ? ip_hdr(skb)->saddr : ip_hdr(skb)->daddr;
 }
 
+#if defined(CONFIG_IP6_NF_IPTABLES) || defined(CONFIG_IP6_NF_IPTABLES_MODULE)
+static inline bool
+ipv6addrptr(const struct sk_buff *skb, bool inner, bool src,
+	    struct in6_addr *addr)
+{
+	struct ipv6hdr *ih = ipv6_hdr(skb);
+
+	if (ih == NULL)
+		goto err;
+
+	if (inner) {
+		unsigned int protooff;
+		u8 nexthdr = ih->nexthdr;
+		__be16 frag_off;
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(3, 3, 0)
+		protooff = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr),
+					    &nexthdr, &frag_off);
+#else
+		protooff = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr),
+					    &nexthdr);
+#endif
+		if (protooff < 0 || nexthdr != IPPROTO_ICMPV6 ||
+		    !ip_set_get_icmpv6_inner_hdr(skb, &protooff, &ih))
+			goto err;
+	}
+	memcpy(addr, src ? &ih->saddr : &ih->daddr, sizeof(*addr));
+	return true;
+
+err:
+	return false;
+}
+
 static inline void
 ip6addrptr(const struct sk_buff *skb, bool src, struct in6_addr *addr)
 {
 	memcpy(addr, src ? &ipv6_hdr(skb)->saddr : &ipv6_hdr(skb)->daddr,
 	       sizeof(*addr));
 }
+#else
+static inline bool
+ipv6addrptr(const struct sk_buff *skb, bool inner, bool src,
+	    struct in6_addr *addr)
+{
+	return false;
+}
+
+static inline void
+ip6addrptr(const struct sk_buff *skb, bool src, struct in6_addr *addr) { ; }
+#endif
 
 /* Calculate the bytes required to store the inclusive range of a-b */
 static inline int
kernel/include/linux/netfilter/ipset/ip_set_getport.h +16 -0
index 90d0930..8cdc177 100644
@@ -1,13 +1,26 @@
 #ifndef _IP_SET_GETPORT_H
 #define _IP_SET_GETPORT_H
 
+extern bool ip_set_get_ipv4_port(const struct sk_buff *skb, bool inner,
+				 bool src, __be16 *port, u8 *proto);
+
 extern bool ip_set_get_ip4_port(const struct sk_buff *skb, bool src,
 				__be16 *port, u8 *proto);
 
 #if defined(CONFIG_IP6_NF_IPTABLES) || defined(CONFIG_IP6_NF_IPTABLES_MODULE)
+extern bool ip_set_get_ipv6_port(const struct sk_buff *skb, bool inner,
+				 bool src, __be16 *port, u8 *proto);
+
 extern bool ip_set_get_ip6_port(const struct sk_buff *skb, bool src,
 				__be16 *port, u8 *proto);
 #else
+static inline bool ip_set_get_ipv6_port(const struct sk_buff *skb,
+					bool inner, bool src,
+					__be16 *port, u8 *proto)
+{
+	return false;
+}
+
 static inline bool ip_set_get_ip6_port(const struct sk_buff *skb, bool src,
 				       __be16 *port, u8 *proto)
 {

@@ -15,6 +28,9 @@ static inline bool ip_set_get_ip6_port(const struct sk_buff *skb, bool src,
 }
 #endif
 
+extern bool ip_set_get_ipv_port(const struct sk_buff *skb, u8 pf, bool inner,
+				bool src, __be16 *port);
+
 extern bool ip_set_get_ip_port(const struct sk_buff *skb, u8 pf, bool src,
 				__be16 *port);
kernel/include/linux/netfilter/ipset/ip_set_icmp.h new file +91 -0

91 lines not on this page · raw message

kernel/include/uapi/linux/netfilter/ipset/ip_set.h +2 -0
index 8024cdf..e9e6586 100644
@@ -161,6 +161,8 @@ enum ipset_cmd_flags {
 		(1 << IPSET_FLAG_BIT_SKIP_SUBCOUNTER_UPDATE),
 	IPSET_FLAG_BIT_MATCH_COUNTERS = 5,
 	IPSET_FLAG_MATCH_COUNTERS = (1 << IPSET_FLAG_BIT_MATCH_COUNTERS),
+	IPSET_FLAG_BIT_INNER = 6,
+	IPSET_FLAG_INNER = (1 << IPSET_FLAG_BIT_INNER),
 	IPSET_FLAG_BIT_RETURN_NOMATCH = 7,
 	IPSET_FLAG_RETURN_NOMATCH = (1 << IPSET_FLAG_BIT_RETURN_NOMATCH),
 	IPSET_FLAG_CMD_MAX = 15,
kernel/net/netfilter/ipset/ip_set_getport.c +63 -15

138 lines not on this page · raw message

author
#12 lore raw

PATCH v2 3/5 ipset: add set match "inner" flag support

In reply to a message not in the archive

This patch implements "inner" flag support to all registered ipset types.

Revision history:

v1 * initial revision
v2 * redundant code removed;
* use the new ipv[46]addr[ptr] and ip_set_get*port functions;

Signed-off-by Dash Four <[email protected]>
---
 kernel/net/netfilter/ipset/ip_set_bitmap_ip.c      |    7 ++++++-
 kernel/net/netfilter/ipset/ip_set_bitmap_ipmac.c   |    7 ++++++-
 kernel/net/netfilter/ipset/ip_set_bitmap_port.c    |    7 ++++---
 kernel/net/netfilter/ipset/ip_set_hash_ip.c        |   10 +++++++--
 kernel/net/netfilter/ipset/ip_set_hash_ipport.c    |   18 +++++++++-------
 kernel/net/netfilter/ipset/ip_set_hash_ipportip.c  |   22 +++++++++++++-------
 kernel/net/netfilter/ipset/ip_set_hash_ipportnet.c |   22 +++++++++++++-------
 kernel/net/netfilter/ipset/ip_set_hash_net.c       |    8 +++++--
 kernel/net/netfilter/ipset/ip_set_hash_netiface.c  |    8 +++++--
 kernel/net/netfilter/ipset/ip_set_hash_netport.c   |   17 ++++++++-------
 10 files changed, 85 insertions(+), 41 deletions(-)
kernel/net/netfilter/ipset/ip_set_bitmap_ip.c +6 -1
index ce99d26..20c5ade 100644
@@ -115,8 +115,13 @@ bitmap_ip_kadt(struct ip_set *set, const struct sk_buff *skb,
 	struct bitmap_ip_adt_elem e = { };
 	struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, map);
 	u32 ip;
+	__be32 _ip;
 
-	ip = ntohl(ip4addr(skb, opt->flags & IPSET_DIM_ONE_SRC));
+	if (!ipv4addrptr(skb, opt->cmdflags & IPSET_FLAG_INNER,
+			opt->flags & IPSET_DIM_ONE_SRC, &_ip))
+			return -EINVAL;
+
+	ip = ntohl(_ip);
 	if (ip < map->first_ip || ip > map->last_ip)
 		return -IPSET_ERR_BITMAP_RANGE;
kernel/net/netfilter/ipset/ip_set_bitmap_ipmac.c +6 -1
index 6d5bad9..c7d490c 100644
@@ -218,12 +218,17 @@ bitmap_ipmac_kadt(struct ip_set *set, const struct sk_buff *skb,
 	struct bitmap_ipmac_adt_elem e = {};
 	struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, map);
 	u32 ip;
+	__be32 _ip;
 
 	/* MAC can be src only */
 	if (!(opt->flags & IPSET_DIM_TWO_SRC))
 		return 0;
 
-	ip = ntohl(ip4addr(skb, opt->flags & IPSET_DIM_ONE_SRC));
+	if (!ipv4addrptr(skb, opt->cmdflags & IPSET_FLAG_INNER,
+			opt->flags & IPSET_DIM_ONE_SRC, &_ip))
+			return -EINVAL;
+
+	ip = ntohl(_ip);
 	if (ip < map->first_ip || ip > map->last_ip)
 		return -IPSET_ERR_BITMAP_RANGE;
kernel/net/netfilter/ipset/ip_set_bitmap_port.c +4 -3
index b220489..1200e07 100644
@@ -110,9 +110,10 @@ bitmap_port_kadt(struct ip_set *set, const struct sk_buff *skb,
 	__be16 __port;
 	u16 port = 0;
 
-	if (!ip_set_get_ip_port(skb, opt->family,
-				opt->flags & IPSET_DIM_ONE_SRC, &__port))
-		return -EINVAL;
+	if (!ip_set_get_ipv_port(skb, opt->family,
+				 opt->cmdflags & IPSET_FLAG_INNER,
+				 opt->flags & IPSET_DIM_ONE_SRC, &__port))
+			return -EINVAL;
 
 	port = ntohs(__port);
kernel/net/netfilter/ipset/ip_set_hash_ip.c +8 -2
index 260c9a8..924a497 100644
@@ -102,7 +102,10 @@ hash_ip4_kadt(struct ip_set *set, const struct sk_buff *skb,
 	struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, h);
 	__be32 ip;
 
-	ip4addrptr(skb, opt->flags & IPSET_DIM_ONE_SRC, &ip);
+	if (!ipv4addrptr(skb, opt->cmdflags & IPSET_FLAG_INNER,
+			 opt->flags & IPSET_DIM_ONE_SRC, &ip))
+			return -EINVAL;
+
 	ip &= ip_set_netmask(h->netmask);
 	if (ip == 0)
 		return -EINVAL;

@@ -255,7 +258,10 @@ hash_ip6_kadt(struct ip_set *set, const struct sk_buff *skb,
 	struct hash_ip6_elem e = {};
 	struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, h);
 
-	ip6addrptr(skb, opt->flags & IPSET_DIM_ONE_SRC, &e.ip.in6);
+	if (!ipv6addrptr(skb, opt->cmdflags & IPSET_FLAG_INNER,
+			 opt->flags & IPSET_DIM_ONE_SRC, &e.ip.in6))
+		return -EINVAL;
+
 	hash_ip6_netmask(&e.ip, h->netmask);
 	if (ipv6_addr_any(&e.ip.in6))
 		return -EINVAL;
kernel/net/netfilter/ipset/ip_set_hash_ipport.c +11 -7
index 64caad3..dd52323 100644
@@ -121,11 +121,13 @@ hash_ipport4_kadt(struct ip_set *set, const struct sk_buff *skb,
 	struct hash_ipport4_elem e = { };
 	struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, h);
 
-	if (!ip_set_get_ip4_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
-				 &e.port, &e.proto))
-		return -EINVAL;
+	if (!ip_set_get_ipv4_port(skb, opt->cmdflags & IPSET_FLAG_INNER,
+				  opt->flags & IPSET_DIM_TWO_SRC, &e.port,
+				  &e.proto) ||
+	    !ipv4addrptr(skb, opt->cmdflags & IPSET_FLAG_INNER,
+			 opt->flags & IPSET_DIM_ONE_SRC, &e.ip))
+			return -EINVAL;
 
-	ip4addrptr(skb, opt->flags & IPSET_DIM_ONE_SRC, &e.ip);
 	return adtfn(set, &e, &ext, &opt->ext, opt->cmdflags);
 }
 

@@ -311,11 +313,13 @@ hash_ipport6_kadt(struct ip_set *set, const struct sk_buff *skb,
 	struct hash_ipport6_elem e = { };
 	struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, h);
 
-	if (!ip_set_get_ip6_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
-				 &e.port, &e.proto))
+	if (!ip_set_get_ipv6_port(skb, opt->cmdflags & IPSET_FLAG_INNER,
+				  opt->flags & IPSET_DIM_TWO_SRC, &e.port,
+				  &e.proto) ||
+	    !ipv6addrptr(skb, opt->cmdflags & IPSET_FLAG_INNER,
+			 opt->flags & IPSET_DIM_ONE_SRC, &e.ip.in6))
 		return -EINVAL;
 
-	ip6addrptr(skb, opt->flags & IPSET_DIM_ONE_SRC, &e.ip.in6);
 	return adtfn(set, &e, &ext, &opt->ext, opt->cmdflags);
 }
kernel/net/netfilter/ipset/ip_set_hash_ipportip.c +14 -8
index 2873bbc..e826a09 100644
@@ -125,12 +125,15 @@ hash_ipportip4_kadt(struct ip_set *set, const struct sk_buff *skb,
 	struct hash_ipportip4_elem e = { };
 	struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, h);
 
-	if (!ip_set_get_ip4_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
-				 &e.port, &e.proto))
+	if (!ip_set_get_ipv4_port(skb, opt->cmdflags & IPSET_FLAG_INNER,
+				  opt->flags & IPSET_DIM_TWO_SRC, &e.port,
+				  &e.proto) ||
+	    !ipv4addrptr(skb, opt->cmdflags & IPSET_FLAG_INNER,
+			 opt->flags & IPSET_DIM_ONE_SRC, &e.ip) ||
+	    !ipv4addrptr(skb, opt->cmdflags & IPSET_FLAG_INNER,
+			 opt->flags & IPSET_DIM_THREE_SRC, &e.ip2))
 		return -EINVAL;
 
-	ip4addrptr(skb, opt->flags & IPSET_DIM_ONE_SRC, &e.ip);
-	ip4addrptr(skb, opt->flags & IPSET_DIM_THREE_SRC, &e.ip2);
 	return adtfn(set, &e, &ext, &opt->ext, opt->cmdflags);
 }
 

@@ -324,12 +327,15 @@ hash_ipportip6_kadt(struct ip_set *set, const struct sk_buff *skb,
 	struct hash_ipportip6_elem e = { };
 	struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, h);
 
-	if (!ip_set_get_ip6_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
-				 &e.port, &e.proto))
+	if (!ip_set_get_ipv6_port(skb, opt->cmdflags & IPSET_FLAG_INNER,
+				  opt->flags & IPSET_DIM_TWO_SRC, &e.port,
+				  &e.proto) ||
+	    !ipv6addrptr(skb, opt->cmdflags & IPSET_FLAG_INNER,
+			 opt->flags & IPSET_DIM_ONE_SRC, &e.ip.in6) ||
+	    !ipv6addrptr(skb, opt->cmdflags & IPSET_FLAG_INNER,
+			 opt->flags & IPSET_DIM_THREE_SRC, &e.ip2.in6))
 		return -EINVAL;
 
-	ip6addrptr(skb, opt->flags & IPSET_DIM_ONE_SRC, &e.ip.in6);
-	ip6addrptr(skb, opt->flags & IPSET_DIM_THREE_SRC, &e.ip2.in6);
 	return adtfn(set, &e, &ext, &opt->ext, opt->cmdflags);
 }
kernel/net/netfilter/ipset/ip_set_hash_ipportnet.c +14 -8
index db0e761..378344a 100644
@@ -177,12 +177,15 @@ hash_ipportnet4_kadt(struct ip_set *set, const struct sk_buff *skb,
 	if (adt == IPSET_TEST)
 		e.cidr = HOST_MASK - 1;
 
-	if (!ip_set_get_ip4_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
-				 &e.port, &e.proto))
+	if (!ip_set_get_ipv4_port(skb, opt->cmdflags & IPSET_FLAG_INNER,
+				  opt->flags & IPSET_DIM_TWO_SRC, &e.port,
+				  &e.proto) ||
+	    !ipv4addrptr(skb, opt->cmdflags & IPSET_FLAG_INNER,
+			 opt->flags & IPSET_DIM_ONE_SRC, &e.ip) ||
+	    !ipv4addrptr(skb, opt->cmdflags & IPSET_FLAG_INNER,
+			 opt->flags & IPSET_DIM_THREE_SRC, &e.ip2))
 		return -EINVAL;
 
-	ip4addrptr(skb, opt->flags & IPSET_DIM_ONE_SRC, &e.ip);
-	ip4addrptr(skb, opt->flags & IPSET_DIM_THREE_SRC, &e.ip2);
 	e.ip2 &= ip_set_netmask(e.cidr + 1);
 
 	return adtfn(set, &e, &ext, &opt->ext, opt->cmdflags);

@@ -461,12 +464,15 @@ hash_ipportnet6_kadt(struct ip_set *set, const struct sk_buff *skb,
 	if (adt == IPSET_TEST)
 		e.cidr = HOST_MASK - 1;
 
-	if (!ip_set_get_ip6_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
-				 &e.port, &e.proto))
+	if (!ip_set_get_ipv6_port(skb, opt->cmdflags & IPSET_FLAG_INNER,
+				  opt->flags & IPSET_DIM_TWO_SRC, &e.port,
+				  &e.proto) ||
+	    !ipv6addrptr(skb, opt->cmdflags & IPSET_FLAG_INNER,
+			 opt->flags & IPSET_DIM_ONE_SRC, &e.ip.in6) ||
+	    !ipv6addrptr(skb, opt->cmdflags & IPSET_FLAG_INNER,
+			 opt->flags & IPSET_DIM_THREE_SRC, &e.ip2.in6))
 		return -EINVAL;
 
-	ip6addrptr(skb, opt->flags & IPSET_DIM_ONE_SRC, &e.ip.in6);
-	ip6addrptr(skb, opt->flags & IPSET_DIM_THREE_SRC, &e.ip2.in6);
 	ip6_netmask(&e.ip2, e.cidr + 1);
 
 	return adtfn(set, &e, &ext, &opt->ext, opt->cmdflags);
kernel/net/netfilter/ipset/ip_set_hash_net.c +6 -2
index 846ec80..41acc51 100644
@@ -151,8 +151,10 @@ hash_net4_kadt(struct ip_set *set, const struct sk_buff *skb,
 		return -EINVAL;
 	if (adt == IPSET_TEST)
 		e.cidr = HOST_MASK;
+	if (!ipv4addrptr(skb, opt->cmdflags & IPSET_FLAG_INNER,
+			 opt->flags & IPSET_DIM_ONE_SRC, &e.ip))
+			return -EINVAL;
 
-	ip4addrptr(skb, opt->flags & IPSET_DIM_ONE_SRC, &e.ip);
 	e.ip &= ip_set_netmask(e.cidr);
 
 	return adtfn(set, &e, &ext, &opt->ext, opt->cmdflags);

@@ -346,8 +348,10 @@ hash_net6_kadt(struct ip_set *set, const struct sk_buff *skb,
 		return -EINVAL;
 	if (adt == IPSET_TEST)
 		e.cidr = HOST_MASK;
+	if (!ipv6addrptr(skb, opt->cmdflags & IPSET_FLAG_INNER,
+			 opt->flags & IPSET_DIM_ONE_SRC, &e.ip.in6))
+		return -EINVAL;
 
-	ip6addrptr(skb, opt->flags & IPSET_DIM_ONE_SRC, &e.ip.in6);
 	ip6_netmask(&e.ip, e.cidr);
 
 	return adtfn(set, &e, &ext, &opt->ext, opt->cmdflags);
kernel/net/netfilter/ipset/ip_set_hash_netiface.c +6 -2
index 8f0e496..129b8d2 100644
@@ -275,8 +275,10 @@ hash_netiface4_kadt(struct ip_set *set, const struct sk_buff *skb,
 		return -EINVAL;
 	if (adt == IPSET_TEST)
 		e.cidr = HOST_MASK;
+	if (!ipv4addrptr(skb, opt->cmdflags & IPSET_FLAG_INNER,
+			 opt->flags & IPSET_DIM_ONE_SRC, &e.ip))
+			return -EINVAL;
 
-	ip4addrptr(skb, opt->flags & IPSET_DIM_ONE_SRC, &e.ip);
 	e.ip &= ip_set_netmask(e.cidr);
 
 #define IFACE(dir)	(par->dir ? par->dir->name : NULL)

@@ -544,8 +546,10 @@ hash_netiface6_kadt(struct ip_set *set, const struct sk_buff *skb,
 		return -EINVAL;
 	if (adt == IPSET_TEST)
 		e.cidr = HOST_MASK;
+	if (!ipv6addrptr(skb, opt->cmdflags & IPSET_FLAG_INNER,
+			 opt->flags & IPSET_DIM_ONE_SRC, &e.ip.in6))
+		return -EINVAL;
 
-	ip6addrptr(skb, opt->flags & IPSET_DIM_ONE_SRC, &e.ip.in6);
 	ip6_netmask(&e.ip, e.cidr);
 
 	if (opt->cmdflags & IPSET_FLAG_PHYSDEV) {
kernel/net/netfilter/ipset/ip_set_hash_netport.c +10 -7
index 021d716..4befafd 100644
@@ -169,11 +169,13 @@ hash_netport4_kadt(struct ip_set *set, const struct sk_buff *skb,
 	if (adt == IPSET_TEST)
 		e.cidr = HOST_MASK - 1;
 
-	if (!ip_set_get_ip4_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
-				 &e.port, &e.proto))
+	if (!ip_set_get_ipv4_port(skb, opt->cmdflags & IPSET_FLAG_INNER,
+				  opt->flags & IPSET_DIM_TWO_SRC, &e.port,
+				  &e.proto) ||
+	    !ipv4addrptr(skb, opt->cmdflags & IPSET_FLAG_INNER,
+			 opt->flags & IPSET_DIM_ONE_SRC, &e.ip))
 		return -EINVAL;
 
-	ip4addrptr(skb, opt->flags & IPSET_DIM_ONE_SRC, &e.ip);
 	e.ip &= ip_set_netmask(e.cidr + 1);
 
 	return adtfn(set, &e, &ext, &opt->ext, opt->cmdflags);

@@ -413,12 +415,13 @@ hash_netport6_kadt(struct ip_set *set, const struct sk_buff *skb,
 
 	if (adt == IPSET_TEST)
 		e.cidr = HOST_MASK - 1;
-
-	if (!ip_set_get_ip6_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
-				 &e.port, &e.proto))
+	if (!ip_set_get_ipv6_port(skb, opt->cmdflags & IPSET_FLAG_INNER,
+				  opt->flags & IPSET_DIM_TWO_SRC, &e.port,
+				  &e.proto) ||
+	    !ipv6addrptr(skb, opt->cmdflags & IPSET_FLAG_INNER,
+			 opt->flags & IPSET_DIM_ONE_SRC, &e.ip.in6))
 		return -EINVAL;
 
-	ip6addrptr(skb, opt->flags & IPSET_DIM_ONE_SRC, &e.ip.in6);
 	ip6_netmask(&e.ip, e.cidr + 1);
 
 	return adtfn(set, &e, &ext, &opt->ext, opt->cmdflags);
author
#13 lore raw

PATCH v2 4/5 iptables: add set match "inner" flag support

In reply to a message not in the archive

This patch adds "inner" flag support for the set match.

Revision history:

v1 * initial revision

Signed-off-by Dash Four <[email protected]>
---
 kernel/net/netfilter/xt_set.c |   21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
kernel/net/netfilter/xt_set.c +21 -0
index 00b9c58..430bcff 100644
@@ -464,6 +464,27 @@ static struct xt_match set_matches[] __read_mostly = {
 		.destroy	= set_match_v3_destroy,
 		.me		= THIS_MODULE
 	},
+	/* --inner flag support */
+	{
+		.name		= "set",
+		.family		= NFPROTO_IPV4,
+		.revision	= 4,
+		.match		= set_match_v3,
+		.matchsize	= sizeof(struct xt_set_info_match_v3),
+		.checkentry	= set_match_v3_checkentry,
+		.destroy	= set_match_v3_destroy,
+		.me		= THIS_MODULE
+	},
+	{
+		.name		= "set",
+		.family		= NFPROTO_IPV6,
+		.revision	= 4,
+		.match		= set_match_v3,
+		.matchsize	= sizeof(struct xt_set_info_match_v3),
+		.checkentry	= set_match_v3_checkentry,
+		.destroy	= set_match_v3_destroy,
+		.me		= THIS_MODULE
+	},
 };
 
 static struct xt_target set_targets[] __read_mostly = {
author
#14 lore raw

PATCH v2 5/5 iptables (userspace): add set match "inner" flag support

In reply to a message not in the archive

This patch adds "inner" flag support for the set match in iptables.

Revision history:

v1 * initial revision

Signed-off-by Dash Four <[email protected]>
---
 extensions/libxt_set.c                 |  199 ++++++++++++++++++++++++++++++++
 extensions/libxt_set.man               |   10 ++
 include/linux/netfilter/ipset/ip_set.h |    2 +
 3 files changed, 211 insertions(+)
extensions/libxt_set.c +199 -0
index 2cb9e78..3b20c94 100644
@@ -497,6 +497,191 @@ set_save_v3(const void *ip, const struct xt_entry_match *match)
 	set_print_v3_matchinfo(info, "--match-set", "--");
 }
 
+/* Revision 4 - add --inner flag */
+static void
+set_help_v4(void)
+{
+	printf("set match options:\n"
+	       " [!] --match-set name flags [--return-nomatch] [--inner]\n"
+	       "   [! --update-counters] [! --update-subcounters]\n"
+	       "   [[!] --packets-eq value | --packets-lt value | --packets-gt value\n"
+	       "   [[!] --bytes-eq value | --bytes-lt value | --bytes-gt value\n"
+	       "		 'name' is the set name from to match,\n"
+	       "		 'flags' are the comma separated list of\n"
+	       "		 'src' and 'dst' specifications.\n");
+}
+
+static const struct option set_opts_v4[] = {
+	{.name = "match-set",		.has_arg = true,	.val = '1'},
+	{.name = "set",			.has_arg = true,	.val = '2'},
+	{.name = "return-nomatch",	.has_arg = false,	.val = '3'},
+	{.name = "update-counters",	.has_arg = false,	.val = '4'},
+	{.name = "packets-eq",		.has_arg = true,	.val = '5'},
+	{.name = "packets-lt",		.has_arg = true,	.val = '6'},
+	{.name = "packets-gt",		.has_arg = true,	.val = '7'},
+	{.name = "bytes-eq",		.has_arg = true,	.val = '8'},
+	{.name = "bytes-lt",		.has_arg = true,	.val = '9'},
+	{.name = "bytes-gt",		.has_arg = true,	.val = '0'},
+	{.name = "update-subcounters",	.has_arg = false,	.val = 'a'},
+	{.name = "inner",		.has_arg = false,	.val = 'b'},
+	XT_GETOPT_TABLEEND,
+};
+
+static int
+set_parse_v4(int c, char **argv, int invert, unsigned int *flags,
+	     const void *entry, struct xt_entry_match **match)
+{
+	struct xt_set_info_match_v3 *info =
+		(struct xt_set_info_match_v3 *) (*match)->data;
+
+	switch (c) {
+	case 'b':
+		if (invert)
+			xtables_error(PARAMETER_PROBLEM,
+				      "--inner flag cannot be inverted\n");
+		info->flags |= IPSET_FLAG_INNER;
+		break;
+	case 'a':
+		if (invert)
+			info->flags |= IPSET_FLAG_SKIP_SUBCOUNTER_UPDATE;
+		break;
+	case '0':
+		if (info->bytes.op != IPSET_COUNTER_NONE)
+			xtables_error(PARAMETER_PROBLEM,
+				      "only one of the --bytes-[eq|lt|gt]"
+				      " is allowed\n");
+		if (invert)
+			xtables_error(PARAMETER_PROBLEM,
+				      "--bytes-gt option cannot be inverted\n");
+		info->bytes.op = IPSET_COUNTER_GT;
+		info->bytes.value = parse_counter(optarg);
+		break;
+	case '9':
+		if (info->bytes.op != IPSET_COUNTER_NONE)
+			xtables_error(PARAMETER_PROBLEM,
+				      "only one of the --bytes-[eq|lt|gt]"
+				      " is allowed\n");
+		if (invert)
+			xtables_error(PARAMETER_PROBLEM,
+				      "--bytes-lt option cannot be inverted\n");
+		info->bytes.op = IPSET_COUNTER_LT;
+		info->bytes.value = parse_counter(optarg);
+		break;
+	case '8':
+		if (info->bytes.op != IPSET_COUNTER_NONE)
+			xtables_error(PARAMETER_PROBLEM,
+				      "only one of the --bytes-[eq|lt|gt]"
+				      " is allowed\n");
+		info->bytes.op = invert ? IPSET_COUNTER_NE : IPSET_COUNTER_EQ;
+		info->bytes.value = parse_counter(optarg);
+		break;
+	case '7':
+		if (info->packets.op != IPSET_COUNTER_NONE)
+			xtables_error(PARAMETER_PROBLEM,
+				      "only one of the --packets-[eq|lt|gt]"
+				      " is allowed\n");
+		if (invert)
+			xtables_error(PARAMETER_PROBLEM,
+				      "--packets-gt option cannot be inverted\n");
+		info->packets.op = IPSET_COUNTER_GT;
+		info->packets.value = parse_counter(optarg);
+		break;
+	case '6':
+		if (info->packets.op != IPSET_COUNTER_NONE)
+			xtables_error(PARAMETER_PROBLEM,
+				      "only one of the --packets-[eq|lt|gt]"
+				      " is allowed\n");
+		if (invert)
+			xtables_error(PARAMETER_PROBLEM,
+				      "--packets-lt option cannot be inverted\n");
+		info->packets.op = IPSET_COUNTER_LT;
+		info->packets.value = parse_counter(optarg);
+		break;
+	case '5':
+		if (info->packets.op != IPSET_COUNTER_NONE)
+			xtables_error(PARAMETER_PROBLEM,
+				      "only one of the --packets-[eq|lt|gt]"
+				      " is allowed\n");
+		info->packets.op = invert ? IPSET_COUNTER_NE : IPSET_COUNTER_EQ;
+		info->packets.value = parse_counter(optarg);
+		break;
+	case '4':
+		if (invert)
+			info->flags |= IPSET_FLAG_SKIP_COUNTER_UPDATE;
+		break;
+	case '3':
+		if (invert)
+			xtables_error(PARAMETER_PROBLEM,
+				      "--return-nomatch flag cannot be inverted\n");
+		info->flags |= IPSET_FLAG_RETURN_NOMATCH;
+		break;
+	case '2':
+		fprintf(stderr,
+			"--set option deprecated, please use --match-set\n");
+	case '1':		/* --match-set <set> <flag>[,<flag> */
+		if (info->match_set.dim)
+			xtables_error(PARAMETER_PROBLEM,
+				      "--match-set can be specified only once");
+		if (invert)
+			info->match_set.flags |= IPSET_INV_MATCH;
+
+		if (!argv[optind]
+		    || argv[optind][0] == '-'
+		    || argv[optind][0] == '!')
+			xtables_error(PARAMETER_PROBLEM,
+				      "--match-set requires two args.");
+
+		if (strlen(optarg) > IPSET_MAXNAMELEN - 1)
+			xtables_error(PARAMETER_PROBLEM,
+				      "setname `%s' too long, max %d characters.",
+				      optarg, IPSET_MAXNAMELEN - 1);
+
+		get_set_byname(optarg, &info->match_set);
+		parse_dirs(argv[optind], &info->match_set);
+		DEBUGP("parse: set index %u\n", info->match_set.index);
+		optind++;
+		
+		*flags = 1;
+		break;
+	}
+
+	return 1;
+}
+
+static void
+set_print_v4_matchinfo(const struct xt_set_info_match_v3 *info,
+		       const char *opt, const char *sep)
+{
+	print_match(opt, &info->match_set);
+	if (info->flags & IPSET_FLAG_RETURN_NOMATCH)
+		printf(" %sreturn-nomatch", sep);
+	if (info->flags & IPSET_FLAG_INNER)
+		printf(" %sinner", sep);
+	if ((info->flags & IPSET_FLAG_SKIP_COUNTER_UPDATE))
+		printf(" ! %supdate-counters", sep);
+	if ((info->flags & IPSET_FLAG_SKIP_SUBCOUNTER_UPDATE))
+		printf(" ! %supdate-subcounters", sep);
+	set_printv3_counter(&info->packets, "packets", sep);
+	set_printv3_counter(&info->bytes, "bytes", sep);
+}
+
+/* Prints out the matchinfo. */
+static void
+set_print_v4(const void *ip, const struct xt_entry_match *match, int numeric)
+{
+	const struct xt_set_info_match_v3 *info = (const void *)match->data;
+
+	set_print_v4_matchinfo(info, "match-set", "");
+}
+
+static void
+set_save_v4(const void *ip, const struct xt_entry_match *match)
+{
+	const struct xt_set_info_match_v3 *info = (const void *)match->data;
+
+	set_print_v4_matchinfo(info, "--match-set", "--");
+}
+
 static struct xtables_match set_mt_reg[] = {
 	{
 		.name		= "set",

@@ -554,6 +739,20 @@ static struct xtables_match set_mt_reg[] = {
 		.save		= set_save_v3,
 		.extra_opts	= set_opts_v3,
 	},
+	{
+		.name		= "set",
+		.revision	= 4,
+		.version	= XTABLES_VERSION,
+		.family		= NFPROTO_UNSPEC,
+		.size		= XT_ALIGN(sizeof(struct xt_set_info_match_v3)),
+		.userspacesize	= XT_ALIGN(sizeof(struct xt_set_info_match_v3)),
+		.help		= set_help_v4,
+		.parse		= set_parse_v4,
+		.final_check	= set_check_v0,
+		.print		= set_print_v4,
+		.save		= set_save_v4,
+		.extra_opts	= set_opts_v4,
+	},
 };
 
 void _init(void)
extensions/libxt_set.man +10 -0
index 7012ef2..21e443c 100644
@@ -21,6 +21,16 @@ supports the \fBnomatch\fP flag, then the matching is reversed: a match
 with an element flagged with \fBnomatch\fP returns \fBtrue\fP, while a
 match with a plain element returns \fBfalse\fP.
 .TP
+\fB\-\-inner\fP
+Please note that this option will only produce matches in the event of
+the following ICMPv4 and ICMPv6 messages: \fBdestination-unreachable\fP
+(ICMPv4 code 3 and ICMPv6 code 1), \fBsource-quench\fP (ICMPv4 code 4),
+\fBtime-exceeded\fP (ICMPv4 code 11 and ICMPv6 code 3) and
+\fBpacket-too-big\fP (ICMPv6 code 2). If the \fB\-\-inner\fP option is
+specified, then element matching is based on the properties
+(source/destination IP address, protocol, port and so on) of the original
+(inner) connection, which produced the above ICMP[v6] type messages.
+.TP
 \fB!\fP \fB\-\-update\-counters\fP
 If the \fB\-\-update\-counters\fP flag is negated, then the packet and
 byte counters of the matching element in the set won't be updated. Default
include/linux/netfilter/ipset/ip_set.h +2 -0
index eb9123e..1b3d9cb 100644
@@ -161,6 +161,8 @@ enum ipset_cmd_flags {
 		(1 << IPSET_FLAG_BIT_SKIP_SUBCOUNTER_UPDATE),
 	IPSET_FLAG_BIT_MATCH_COUNTERS = 5,
 	IPSET_FLAG_MATCH_COUNTERS = (1 << IPSET_FLAG_BIT_MATCH_COUNTERS),
+	IPSET_FLAG_BIT_INNER = 6,
+	IPSET_FLAG_INNER = (1 << IPSET_FLAG_BIT_INNER),
 	IPSET_FLAG_BIT_RETURN_NOMATCH = 7,
 	IPSET_FLAG_RETURN_NOMATCH = (1 << IPSET_FLAG_BIT_RETURN_NOMATCH),
 	IPSET_FLAG_CMD_MAX = 15,
#3 lore raw

Re: [PATCH v2 2/5] ipset: add "inner" flag implementation

In reply to Dash Four
Quoted 5 lines · Dash Four

This patch implements "inner" flag option in set iptables match, allowing matching based on the properties (source/destination IP address, protocol, port and so on) of the original (inner) connection in the event of the following ICMP[v4,v6] messages:

Quoted 8 lines

ICMPv4 destination-unreachable (code 3);
ICMPv4 source-quench (code 4);
ICMPv4 time-exceeded (code 11);
ICMPv6 destination-unreachable (code 1);
ICMPv6 packet-too-big (code 2);
ICMPv6 time-exceeded (code 3);

Revision history:

The patchset is a good step forward.

I have no objections against the other patches, however I have to comment this one.

Quoted 7 lines

v1 * initial revision
v2 * redundant code removed;
* added a new header file (ip_set_icmp.h) with 2 inline functions,

     allowing access to the internal icmp header properties;

* removed ip[46]inneraddr[ptr]functions as they are no longer needed
* added new ipv[46]addr[ptr] and ip_set_get*port functions, the old functions are still preserved for backwards compatibility

Get rid of the compatibility functions. Or keep the original function names with the extended arguments: there's no stable API.

Quoted 17 lines
Signed-off-by Dash Four <[email protected]>
---
 kernel/include/linux/netfilter/ipset/ip_set.h      |   66 ++++++++++++++
 .../include/linux/netfilter/ipset/ip_set_getport.h |   16 ++++
 kernel/include/linux/netfilter/ipset/ip_set_icmp.h |   91
++++++++++++++++++++
 kernel/include/uapi/linux/netfilter/ipset/ip_set.h |    2 +
 kernel/net/netfilter/ipset/ip_set_getport.c        |   78 +++++++++++++----
 5 files changed, 238 insertions(+), 15 deletions(-)
 create mode 100644 kernel/include/linux/netfilter/ipset/ip_set_icmp.h
kernel/include/linux/netfilter/ipset/ip_set.h +0 -0
index 8499e25..5b6ef72 100644

kernel/include/linux/netfilter/ipset/ip_set.h line 26

Quoted 10 lines
kernel/include/linux/netfilter/ipset/ip_set.h +4 -0
@@ -17,9 +17,13 @@
 #include <linux/netfilter/x_tables.h>
 #include <linux/stringify.h>
 #include <linux/vmalloc.h>
+#include <net/ip.h>
+#include <net/ipv6.h>
+#include <net/icmp.h>
 #include <net/netlink.h>
 #include <uapi/linux/netfilter/ipset/ip_set.h>
 #include <linux/netfilter/ipset/ip_set_compat.h>
+#include <linux/netfilter/ipset/ip_set_icmp.h>

I don't really like the new header file. ip[v]4|6addrptr are generic functions (from ipset point of view) and getting the inner header is too, so those deserve to be in the ipset core.

Quoted 4 lines
kernel/include/linux/netfilter/ipset/ip_set.h +0 -0
 #define _IP_SET_MODULE_DESC(a, b, c)		\
 	MODULE_DESCRIPTION(a " type of IP sets, revisions " b "-" c)

@@ -361,6 +365,25 @@ static inline int nla_put_ipaddr6(struct sk_buff *skb,
 int type,

kernel/include/linux/netfilter/ipset/ip_set.h static inline int nla_put_ipaddr6(struct sk_buff *skb, · line 375

Quoted 10 lines
kernel/include/linux/netfilter/ipset/ip_set.h +7 -0
 }
 
 /* Get address from skbuff */
+static inline bool
+ipv4addrptr(const struct sk_buff *skb, bool inner, bool src, __be32 *addr)
+{
+	struct iphdr *ih = ip_hdr(skb);
+	unsigned int protooff = ip_hdrlen(skb);
+
+	if (ih == NULL ||

Why do you have to check the IP header?

kernel/include/linux/netfilter/ipset/ip_set.h static inline int nla_put_ipaddr6(struct sk_buff *skb, int type, · line 378

Quoted 4 lines
kernel/include/linux/netfilter/ipset/ip_set.h +4 -0
+	    (inner &&
+	     (ih->protocol != IPPROTO_ICMP ||
+	      !ip_set_get_icmpv4_inner_hdr(skb, &protooff, &ih))))
+			goto err;

Move the protocol checking into ip_set_get_icmpv4_inner_hdr. Also, I suggested the name ip_set_get[_ip4]_inner_hdr because I want to extend it to support IPIP and GRE too. By moving the protocol checking into the function and using a not so specific name, those make possible the extension straightforward. Also, there's no need for the goto here:
there's no other error path.

Quoted 14 lines
kernel/include/linux/netfilter/ipset/ip_set.h +7 -0
+	*addr = src ? ih->saddr : ih->daddr;
+	return true;
+
+err:
+	return false;
+}
+
 static inline __be32
 ip4addr(const struct sk_buff *skb, bool src)
 {

@@ -373,12 +396,55 @@ ip4addrptr(const struct sk_buff *skb, bool src, __be32
 *addr)
 	*addr = src ? ip_hdr(skb)->saddr : ip_hdr(skb)->daddr;
 }

kernel/include/linux/netfilter/ipset/ip_set.h ip4addrptr(const struct sk_buff *skb, bool src, __be32 · line 408

Quoted 10 lines
kernel/include/linux/netfilter/ipset/ip_set.h +9 -0
 
+#if defined(CONFIG_IP6_NF_IPTABLES) || defined(CONFIG_IP6_NF_IPTABLES_MODULE)
+static inline bool
+ipv6addrptr(const struct sk_buff *skb, bool inner, bool src,
+	    struct in6_addr *addr)
+{
+	struct ipv6hdr *ih = ipv6_hdr(skb);
+
+	if (ih == NULL)
+		goto err;

The same comment as above.

Quoted 5 lines
kernel/include/linux/netfilter/ipset/ip_set.h +5 -0
+	if (inner) {
+		unsigned int protooff;
+		u8 nexthdr = ih->nexthdr;
+		__be16 frag_off;
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(3, 3, 0)

kernel/include/linux/netfilter/ipset/ip_set.h ip4addrptr(const struct sk_buff *skb, bool src, __be32 *addr) · line 423

Quoted 10 lines
kernel/include/linux/netfilter/ipset/ip_set.h +10 -0
+		protooff = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr),
+					    &nexthdr, &frag_off);
+#else
+		protooff = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr),
+					    &nexthdr);
+#endif
+		if (protooff < 0 || nexthdr != IPPROTO_ICMPV6 ||
+		    !ip_set_get_icmpv6_inner_hdr(skb, &protooff, &ih))
+			goto err;
+	}

Move the whole code in the braces into ip_set_get_ipv6_inner_hdr, for the same reason as above.

Quoted 158 lines
kernel/include/linux/netfilter/ipset/ip_set.h +18 -0
+	memcpy(addr, src ? &ih->saddr : &ih->daddr, sizeof(*addr));
+	return true;
+
+err:
+	return false;
+}
+
 static inline void
 ip6addrptr(const struct sk_buff *skb, bool src, struct in6_addr *addr)
 {
 	memcpy(addr, src ? &ipv6_hdr(skb)->saddr : &ipv6_hdr(skb)->daddr,
 	       sizeof(*addr));
 }
+#else
+static inline bool
+ipv6addrptr(const struct sk_buff *skb, bool inner, bool src,
+	    struct in6_addr *addr)
+{
+	return false;
+}
+
+static inline void
+ip6addrptr(const struct sk_buff *skb, bool src, struct in6_addr *addr) { ; }
+#endif
 
 /* Calculate the bytes required to store the inclusive range of a-b */
 static inline int
kernel/include/linux/netfilter/ipset/ip_set_getport.h +16 -0
index 90d0930..8cdc177 100644
@@ -1,13 +1,26 @@
 #ifndef _IP_SET_GETPORT_H
 #define _IP_SET_GETPORT_H
 
+extern bool ip_set_get_ipv4_port(const struct sk_buff *skb, bool inner,
+				 bool src, __be16 *port, u8 *proto);
+
 extern bool ip_set_get_ip4_port(const struct sk_buff *skb, bool src,
 				__be16 *port, u8 *proto);
 
 #if defined(CONFIG_IP6_NF_IPTABLES) || defined(CONFIG_IP6_NF_IPTABLES_MODULE)
+extern bool ip_set_get_ipv6_port(const struct sk_buff *skb, bool inner,
+				 bool src, __be16 *port, u8 *proto);
+
 extern bool ip_set_get_ip6_port(const struct sk_buff *skb, bool src,
 				__be16 *port, u8 *proto);
 #else
+static inline bool ip_set_get_ipv6_port(const struct sk_buff *skb,
+					bool inner, bool src,
+					__be16 *port, u8 *proto)
+{
+	return false;
+}
+
 static inline bool ip_set_get_ip6_port(const struct sk_buff *skb, bool src,
 				       __be16 *port, u8 *proto)
 {

@@ -15,6 +28,9 @@ static inline bool ip_set_get_ip6_port(const struct sk_buff
 *skb, bool src,
 }
 #endif
 
+extern bool ip_set_get_ipv_port(const struct sk_buff *skb, u8 pf, bool inner,
+				bool src, __be16 *port);
+
 extern bool ip_set_get_ip_port(const struct sk_buff *skb, u8 pf, bool src,
 				__be16 *port);
kernel/include/linux/netfilter/ipset/ip_set_icmp.h new file +81 -0
new file mode 100644
index 0000000..e116d28
@@ -0,0 +1,91 @@
+#ifndef _IP_SET_ICMP_H
+#define _IP_SET_ICMP_H
+
+#include <linux/ip.h>
+#include <linux/ipv6.h>
+#include <linux/icmp.h>
+
+static inline
+bool ip_set_get_icmpv4_inner_hdr(const struct sk_buff *skb,
+				 unsigned int *offset,
+				 struct iphdr **ih)
+{
+	u8 type;
+	struct iphdr _iph;
+	struct icmphdr _icmph;
+	struct iphdr *iph;
+	const struct icmphdr *ich;
+	/* RFC 1122: 3.2.2: req'd len: IP header + 8 bytes of inner header */
+	static const size_t req_len = sizeof(struct iphdr) + 8;
+
+	if (offset == NULL || ih == NULL)
+		goto err;
+
+	ich = skb_header_pointer(skb, *offset, sizeof(_icmph), &_icmph);
+	if (ich == NULL ||
+	    (ich->type <= NR_ICMP_TYPES && skb->len - *offset < req_len))
+		goto err;
+
+	type = ich->type;
+	if (type == ICMP_DEST_UNREACH ||
+	    type == ICMP_SOURCE_QUENCH ||
+	    type == ICMP_TIME_EXCEEDED) {
+		*offset += sizeof(_icmph);
+		iph = skb_header_pointer(skb, *offset, sizeof(_iph), &_iph);
+		if (iph == NULL || ntohs(iph->frag_off) & IP_OFFSET)
+			goto err;
+
+		*ih = iph;
+		return true;
+	}
+
+err:
+	return false;
+}
+
+#if defined(CONFIG_IP6_NF_IPTABLES) || defined(CONFIG_IP6_NF_IPTABLES_MODULE)
+static inline
+bool ip_set_get_icmpv6_inner_hdr(const struct sk_buff *skb,
+				 unsigned int *offset,
+				 struct ipv6hdr **ih)
+{
+	u8 type;
+	const struct icmp6hdr *ic;
+	struct icmp6hdr _icmp6h;
+	struct ipv6hdr _ip6h;
+	struct ipv6hdr *iph;
+
+	if (offset == NULL || ih == NULL)
+		goto err;
+
+	ic = skb_header_pointer(skb, *offset, sizeof(_icmp6h), &_icmp6h);
+	if (ic == NULL)
+		goto err;
+
+	type = ic->icmp6_type;
+	if (type == ICMPV6_DEST_UNREACH ||
+	    type == ICMPV6_PKT_TOOBIG ||
+	    type == ICMPV6_TIME_EXCEED) {
+		*offset += sizeof(_icmp6h);
+		iph = skb_header_pointer(skb, *offset, sizeof(_ip6h), &_ip6h);
+		if (iph == NULL)
+			goto err;
+
+		*ih = iph;
+		return true;
+	}
+
+err:
+	return false;
+}
+#else

kernel/include/linux/netfilter/ipset/ip_set_icmp.h line 91

Quoted 10 lines
kernel/include/linux/netfilter/ipset/ip_set_icmp.h new file +10 -0
+static inline
+bool ip_set_get_icmpv6_inner_hdr(const struct sk_buff *skb,
+				 unsigned int offset,
+				 struct ipv6hdr **ih)
+{
+	return false;
+}
+#endif
+
+#endif /*_IP_SET_ICMP_H*/

Move the functions into ip_set_core.c as non-inline ones and export them.

Quoted 58 lines
kernel/include/uapi/linux/netfilter/ipset/ip_set.h +2 -0
index 8024cdf..e9e6586 100644
@@ -161,6 +161,8 @@ enum ipset_cmd_flags {
 		(1 << IPSET_FLAG_BIT_SKIP_SUBCOUNTER_UPDATE),
 	IPSET_FLAG_BIT_MATCH_COUNTERS = 5,
 	IPSET_FLAG_MATCH_COUNTERS = (1 << IPSET_FLAG_BIT_MATCH_COUNTERS),
+	IPSET_FLAG_BIT_INNER = 6,
+	IPSET_FLAG_INNER = (1 << IPSET_FLAG_BIT_INNER),
 	IPSET_FLAG_BIT_RETURN_NOMATCH = 7,
 	IPSET_FLAG_RETURN_NOMATCH = (1 << IPSET_FLAG_BIT_RETURN_NOMATCH),
 	IPSET_FLAG_CMD_MAX = 15,
kernel/net/netfilter/ipset/ip_set_getport.c +8 -4
index a0d96eb..73ccfb3 100644
@@ -19,7 +19,9 @@
 #include <linux/netfilter_ipv6/ip6_tables.h>
 #include <net/ip.h>
 #include <net/ipv6.h>
+#include <net/icmp.h>
 
+#include <linux/netfilter/ipset/ip_set_icmp.h>
 #include <linux/netfilter/ipset/ip_set_getport.h>
 
 /* We must handle non-linear skbs */

@@ -97,10 +99,10 @@ get_port(const struct sk_buff *skb, int protocol, unsigned
 int protooff,
 }
 
 bool
-ip_set_get_ip4_port(const struct sk_buff *skb, bool src,
-		    __be16 *port, u8 *proto)
+ip_set_get_ipv4_port(const struct sk_buff *skb, bool inner, bool src,
+		     __be16 *port, u8 *proto)
 {
-	const struct iphdr *iph = ip_hdr(skb);
+	struct iphdr *iph = ip_hdr(skb);
 	unsigned int protooff = ip_hdrlen(skb);
 	int protocol = iph->protocol;

@@ -116,54 +118,93 @@ ip_set_get_ip4_port(const struct sk_buff *skb, bool src,
 		case IPPROTO_UDPLITE:
 		case IPPROTO_ICMP:
 			/* Port info not available for fragment offset > 0 */
-			return false;
+			goto err;
 		default:
 			/* Other protocols doesn't have ports,
 			   so we can match fragments */
 			*proto = protocol;
 			return true;
 		}
+	if (inner) {
+		if (protocol != IPPROTO_ICMP ||

kernel/net/netfilter/ipset/ip_set_getport.c ip_set_get_ip4_port(const struct sk_buff *skb, bool src, · line 139

Quoted 10 lines
kernel/net/netfilter/ipset/ip_set_getport.c +8 -0
+		    !ip_set_get_icmpv4_inner_hdr(skb, &protooff, &iph))
+			goto err;
 
+		protocol = iph->protocol;
+		protooff += iph->ihl*4;
+	}
 	return get_port(skb, protocol, protooff, src, port, proto);
+
+err:
+	return false;

If there's nothing else in the error path just a return, then the goto is unnecessary.

Quoted 94 lines
kernel/net/netfilter/ipset/ip_set_getport.c +47 -11
+}
+EXPORT_SYMBOL_GPL(ip_set_get_ipv4_port);
+
+bool
+ip_set_get_ip4_port(const struct sk_buff *skb, bool src,
+		    __be16 *port, u8 *proto)
+{
+	return ip_set_get_ipv4_port(skb, false, src, port, proto);
 }
 EXPORT_SYMBOL_GPL(ip_set_get_ip4_port);
 
 #if defined(CONFIG_IP6_NF_IPTABLES) || defined(CONFIG_IP6_NF_IPTABLES_MODULE)
 bool
-ip_set_get_ip6_port(const struct sk_buff *skb, bool src,
-		    __be16 *port, u8 *proto)
+ip_set_get_ipv6_port(const struct sk_buff *skb, bool inner, bool src,
+		     __be16 *port, u8 *proto)
 {
-	int protoff;
+	unsigned int protooff;
 	u8 nexthdr;
 	__be16 frag_off = 0;
 
 	nexthdr = ipv6_hdr(skb)->nexthdr;
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(3, 3, 0)
-	protoff = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr), &nexthdr,
+	protooff = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr), &nexthdr,
 				   &frag_off);
 #else
-	protoff = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr), &nexthdr);
+	protooff = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr), &nexthdr);
 #endif
-	if (protoff < 0 || (frag_off & htons(~0x7)) != 0)
-		return false;
+	if (protooff < 0 || (frag_off & htons(~0x7)) != 0)
+		goto err;
+
+	if (inner) {
+		struct ipv6hdr *ih;
+		if (nexthdr != IPPROTO_ICMPV6 ||
+		    !ip_set_get_icmpv6_inner_hdr(skb, &protooff, &ih))
+			goto err;
 
-	return get_port(skb, nexthdr, protoff, src, port, proto);
+		nexthdr = ih->nexthdr;
+		protooff += sizeof(struct ipv6hdr);
+	}
+	return get_port(skb, nexthdr, protooff, src, port, proto);
+
+err:
+	return false;
+}
+EXPORT_SYMBOL_GPL(ip_set_get_ipv6_port);
+
+bool
+ip_set_get_ip6_port(const struct sk_buff *skb, bool src,
+		    __be16 *port, u8 *proto)
+{
+	return ip_set_get_ipv6_port(skb, false, src, port, proto);
 }
 EXPORT_SYMBOL_GPL(ip_set_get_ip6_port);
 #endif
 
 bool
-ip_set_get_ip_port(const struct sk_buff *skb, u8 pf, bool src, __be16 *port)
+ip_set_get_ipv_port(const struct sk_buff *skb, u8 pf, bool inner, bool src,
+		    __be16 *port)
 {
 	bool ret;
 	u8 proto;
 
 	switch (pf) {
 	case NFPROTO_IPV4:
-		ret = ip_set_get_ip4_port(skb, src, port, &proto);
+		ret = ip_set_get_ipv4_port(skb, inner, src, port, &proto);
 		break;
 	case NFPROTO_IPV6:
-		ret = ip_set_get_ip6_port(skb, src, port, &proto);
+		ret = ip_set_get_ipv6_port(skb, inner, src, port, &proto);
 		break;
 	default:
 		return false;

@@ -178,4 +219,11 @@ ip_set_get_ip_port(const struct sk_buff *skb, u8 pf, bool
 src, __be16 *port)
 		return false;
 	}
 }
+EXPORT_SYMBOL_GPL(ip_set_get_ipv_port);
+
+bool
+ip_set_get_ip_port(const struct sk_buff *skb, u8 pf, bool src, __be16 *port)
+{
+	return ip_set_get_ipv_port(skb, pf, false, src, port);
+}
Quoted 2 lines

EXPORT_SYMBOL_GPL(ip_set_get_ip_port);

Best regards,
Jozsef
-
E-mail : [email protected], [email protected] PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences

          H-1525 Budapest 114, POB. 49, Hungary
author
#4 lore raw

Re: [PATCH v2 2/5] ipset: add "inner" flag implementation

Quoted 2 lines · Jozsef Kadlecsik

Get rid of the compatibility functions. Or keep the original function names with the extended arguments: there's no stable API.

As you've probably guessed, I kept the old functions and their parameters in order to preserve the existing interface API, since any changes I make to these will break existing code using them. If there is no objection and there is no such requirement, I'll get rid of them in the next release of the patches - just let me know.

Quoted 5 lines
Quoted 2 lines

+#include <linux/netfilter/ipset/ip_set_icmp.h>

I don't really like the new header file. ip[v]4|6addrptr are generic functions (from ipset point of view) and getting the inner header is too, so those deserve to be in the ipset core.

That's a fair comment - I'll move them in ip_set_core.c as you suggest below.

Quoted 6 lines
Quoted 6 lines

#define _IP_SET_MODULE_DESC(a, b, c) \

kernel/include/linux/netfilter/ipset/ip_set.h +0 -0
 	MODULE_DESCRIPTION(a " type of IP sets, revisions " b "-" c)

@@ -361,6 +365,25 @@ static inline int nla_put_ipaddr6(struct sk_buff *skb,
 int type,
 }
Quoted 12 lines

kernel/include/linux/netfilter/ipset/ip_set.h static inline int nla_put_ipaddr6(struct sk_buff *skb, int type,

Quoted 9 lines
kernel/include/linux/netfilter/ipset/ip_set.h +7 -0
 /* Get address from skbuff */
+static inline bool
+ipv4addrptr(const struct sk_buff *skb, bool inner, bool src, __be32 *addr)
+{
+	struct iphdr *ih = ip_hdr(skb);
+	unsigned int protooff = ip_hdrlen(skb);
+
+	if (ih == NULL ||

Why do you have to check the IP header?

Simply playing it safe. I am referencing it later on in the code and if that pointer is NULL for whatever reason, it will trigger a null-pointer dereference, hence the check above.

Quoted 7 lines

kernel/include/linux/netfilter/ipset/ip_set.h static inline int nla_put_ipaddr6(struct sk_buff *skb, int type,

Quoted 5 lines
kernel/include/linux/netfilter/ipset/ip_set.h +4 -0
+	    (inner &&
+	     (ih->protocol != IPPROTO_ICMP ||
+	      !ip_set_get_icmpv4_inner_hdr(skb, &protooff, &ih))))
+			goto err;

Move the protocol checking into ip_set_get_icmpv4_inner_hdr. Also, I suggested the name ip_set_get[_ip4]_inner_hdr because I want to extend it to support IPIP and GRE too. By moving the protocol checking into the function and using a not so specific name, those make possible the extension straightforward.

Noted - will do that in the next version of the patches.

Quoted 3 lines

Also, there's no need for the goto here:
there's no other error path.

I disagree. By having "return false" (or "return 0", "return -1" and so on) instead of "goto err" it is not immediately apparent to someone who studies/reviews/uses the code that this is an error condition/path. I have been in that situation many times when I have to go back and look at a particular function call to see what "return false" or "return 0" actually means.

By including "goto err" instead of multiple "return false" statement, that makes it instantly obvious what the purpose of that statement is without having to look elsewhere.

Quoted 15 lines
Quoted 15 lines
kernel/include/linux/netfilter/ipset/ip_set.h +7 -0
+	*addr = src ? ih->saddr : ih->daddr;
+	return true;
+
+err:
+	return false;
+}
+
 static inline __be32
 ip4addr(const struct sk_buff *skb, bool src)
 {

@@ -373,12 +396,55 @@ ip4addrptr(const struct sk_buff *skb, bool src, __be32
 *addr)
 	*addr = src ? ip_hdr(skb)->saddr : ip_hdr(skb)->daddr;
 }
Quoted 13 lines

kernel/include/linux/netfilter/ipset/ip_set.h ip4addrptr(const struct sk_buff *skb, bool src, __be32 *addr) · line 448

Quoted 10 lines
kernel/include/linux/netfilter/ipset/ip_set.h +9 -0
+#if defined(CONFIG_IP6_NF_IPTABLES) || defined(CONFIG_IP6_NF_IPTABLES_MODULE)
+static inline bool
+ipv6addrptr(const struct sk_buff *skb, bool inner, bool src,
+	    struct in6_addr *addr)
+{
+	struct ipv6hdr *ih = ipv6_hdr(skb);
+
+	if (ih == NULL)
+		goto err;

The same comment as above.

Same answer - it is simply used for code clarity and makes the code easier to understand. The presence of "goto" instead of multiple "return false" (or "return 0") statements is also negligible in terms of performance impact.

Quoted 10 lines
Quoted 10 lines
kernel/include/linux/netfilter/ipset/ip_set.h +10 -0
+	if (inner) {
+		unsigned int protooff;
+		u8 nexthdr = ih->nexthdr;
+		__be16 frag_off;
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(3, 3, 0)
+		protooff = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr),
+					    &nexthdr, &frag_off);
+#else
+		protooff = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr),
+					    &nexthdr);
Quoted 9 lines

kernel/include/linux/netfilter/ipset/ip_set.h ip4addrptr(const struct sk_buff *skb, bool src, __be32 *addr) · line 448

Quoted 6 lines
kernel/include/linux/netfilter/ipset/ip_set.h +5 -0
+#endif
+		if (protooff < 0 || nexthdr != IPPROTO_ICMPV6 ||
+		    !ip_set_get_icmpv6_inner_hdr(skb, &protooff, &ih))
+			goto err;
+	}

Move the whole code in the braces into ip_set_get_ipv6_inner_hdr, for the same reason as above.

Noted, will include this in the next version of the patches.

Quoted 5 lines

Review ip4addrptr(const struct sk_buff *skb, bool src, __be32 · line 427

Quoted 2 lines
(quoted diff) +1 -0
+#endif /*_IP_SET_ICMP_H*/

Move the functions into ip_set_core.c as non-inline ones and export them.

Yep, will do that in the next submission.

#5 lore raw

RE: [PATCH v2 2/5] ipset: add "inner" flag implementation

Quoted 7 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Dash Four
Sent: Thursday, June 27, 2013 3:37 PM
To: Jozsef Kadlecsik
Cc: Pablo Neira Ayuso; Netfilter Core Team
Subject: Re: [PATCH v2 2/5] ipset: add "inner" flag implementation

...

Quoted 10 lines

I disagree. By having "return false" (or "return 0", "return -1" and so on) instead of "goto err" it is not immediately apparent to someone who studies/reviews/uses the code that this is an error condition/path. I have been in that situation many times when I have to go back and look at a particular function call to see what "return false" or "return 0" actually means.

By including "goto err" instead of multiple "return false" statement, that makes it instantly obvious what the purpose of that statement is without having to look elsewhere.

I suppose as an alternative you could go way against the usual practice and put some text in a function header comment block indicating what the return code means. I know it doesn't get used much but C has had this /* comment */ thing for a long time. I've never understood why more people don't use it.

Jeff Haran

author
#6 lore raw

Re: [PATCH v2 2/5] ipset: add "inner" flag implementation

Quoted 2 lines · Jeff Haran

I suppose as an alternative you could go way against the usual practice and put some text in a function header comment block indicating what the return code means. I know it doesn't get used much but C has had this /* comment */ thing for a long time. I've never understood why more people don't use it.

All true. I guess I could go the full hog and document the entire function, then use "return false" as Jozsef suggested, but this will stick out as a sore thumb as it will be the only function documented in the entire module.

Jozsef, I don't mind doing either of the suggested alternatives: keep "goto err" or document the entire function and use "return false" - let me know what your preference is.

#9 lore raw

Re: [PATCH v2 2/5] ipset: add "inner" flag implementation

In reply to Dash Four
Quoted 9 lines · Dash Four
Quoted 3 lines · Jozsef Kadlecsik

Get rid of the compatibility functions. Or keep the original function names with the extended arguments: there's no stable API.

As you've probably guessed, I kept the old functions and their parameters in order to preserve the existing interface API, since any changes I make to these will break existing code using them. If there is no objection and there is no such requirement, I'll get rid of them in the next release of the patches - just let me know.

Yes, there's no such requirement and remove the backward compatibility part.

Quoted 20 lines
Quoted 7 lines
Quoted 7 lines

#define _IP_SET_MODULE_DESC(a, b, c) \

kernel/include/linux/netfilter/ipset/ip_set.h +0 -0
 	MODULE_DESCRIPTION(a " type of IP sets, revisions " b "-" c)

@@ -361,6 +365,25 @@ static inline int nla_put_ipaddr6(struct sk_buff
 *skb,
 int type,
 }
Quoted 13 lines

kernel/include/linux/netfilter/ipset/ip_set.h static inline int nla_put_ipaddr6(struct sk_buff *skb, int type, · line 374

Quoted 10 lines
kernel/include/linux/netfilter/ipset/ip_set.h +7 -0
 /* Get address from skbuff */
+static inline bool
+ipv4addrptr(const struct sk_buff *skb, bool inner, bool src, __be32
 *addr)
+{
+	struct iphdr *ih = ip_hdr(skb);
+	unsigned int protooff = ip_hdrlen(skb);
+
+	if (ih == NULL ||

Why do you have to check the IP header?

Quoted 3 lines

Simply playing it safe. I am referencing it later on in the code and if that pointer is NULL for whatever reason, it will trigger a null-pointer dereference, hence the check above.

The return value of ip[6]_hdr was never checked in ipset code yet, because it doesn't return NULL at the point where these code parts are called. It's an unnecessary checking.

Quoted 15 lines
Quoted 8 lines

kernel/include/linux/netfilter/ipset/ip_set.h static inline int nla_put_ipaddr6(struct sk_buff *skb, int type, · line 378

Quoted 5 lines
kernel/include/linux/netfilter/ipset/ip_set.h +4 -0
+	    (inner &&
+	     (ih->protocol != IPPROTO_ICMP ||
+	      !ip_set_get_icmpv4_inner_hdr(skb, &protooff, &ih))))
+			goto err;

Also, there's no need for the goto here: there's no other error path.

I disagree. By having "return false" (or "return 0", "return -1" and so on) instead of "goto err" it is not immediately apparent to someone who studies/reviews/uses the code that this is an error condition/path. I have been in that situation many times when I have to go back and look at a particular function call to see what "return false" or "return 0" actually means.

Quoted 3 lines

By including "goto err" instead of multiple "return false" statement, that makes it instantly obvious what the purpose of that statement is without having to look elsewhere.

I see the point, to self-document the code, with the price of more lines. It's a little bit overdoing in my opinion: pretty apparent which is the error path and which is not. But this is highly a personal taste, I won't press it.

Best regards,
Jozsef
-
E-mail : [email protected], [email protected] PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences

          H-1525 Budapest 114, POB. 49, Hungary
#7 lore raw

RE: [PATCH v2 2/5] ipset: add "inner" flag implementation

In reply to Jeff Haran
Quoted 12 lines · Jeff Haran

...

Quoted 10 lines

I disagree. By having "return false" (or "return 0", "return -1" and so on) instead of "goto err" it is not immediately apparent to someone who studies/reviews/uses the code that this is an error condition/path. I have been in that situation many times when I have to go back and look at a particular function call to see what "return false" or "return 0" actually means.

By including "goto err" instead of multiple "return false" statement, that makes it instantly obvious what the purpose of that statement is without having to look elsewhere.

Quoted 5 lines

I suppose as an alternative you could go way against the usual practice and put some text in a function header comment block indicating what the return code means. I know it doesn't get used much but C has had this /* comment */ thing for a long time. I've never understood why more people don't use it.

In general I'd agree with you, but this is a boolean function. So why should the the meaning of the return value "true" and "false" be explained? (This is also why I regard the goto unnecessary.)

Best regards,
Jozsef
-
E-mail : [email protected], [email protected] PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences

          H-1525 Budapest 114, POB. 49, Hungary
author
#10 lore raw

Re: [PATCH v2 2/5] ipset: add "inner" flag implementation

In reply to Jozsef Kadlecsik
Quoted 9 lines · Jozsef Kadlecsik
Quoted 6 lines

As you've probably guessed, I kept the old functions and their parameters in order to preserve the existing interface API, since any changes I make to these will break existing code using them. If there is no objection and there is no such requirement, I'll get rid of them in the next release of the patches - just let me know.

Yes, there's no such requirement and remove the backward compatibility part.

OK, I'll remove it then.

Quoted 2 lines

The return value of ip[6]_hdr was never checked in ipset code yet, because it doesn't return NULL at the point where these code parts are called. It's an unnecessary checking.

Fair enough - I'll remove that check too.

Quoted 7 lines
Quoted 7 lines

I disagree. By having "return false" (or "return 0", "return -1" and so on) instead of "goto err" it is not immediately apparent to someone who studies/reviews/uses the code that this is an error condition/path. I have been in that situation many times when I have to go back and look at a particular function call to see what "return false" or "return 0" actually means.

Quoted 7 lines
Quoted 4 lines

By including "goto err" instead of multiple "return false" statement, that makes it instantly obvious what the purpose of that statement is without having to look elsewhere.

I see the point, to self-document the code, with the price of more lines. It's a little bit overdoing in my opinion: pretty apparent which is the error path and which is not. But this is highly a personal taste, I won't press it.

In other words, you'll be happy for me to leave things as they are - with the "goto"?

#11 lore raw

Re: [PATCH v2 2/5] ipset: add "inner" flag implementation

Quoted 11 lines · Dash Four
Quoted 9 lines
Quoted 3 lines

By including "goto err" instead of multiple "return false" statement, that makes it instantly obvious what the purpose of that statement is without having to look elsewhere.

I see the point, to self-document the code, with the price of more lines. It's a little bit overdoing in my opinion: pretty apparent which is the error path and which is not. But this is highly a personal taste, I won't press it.

In other words, you'll be happy for me to leave things as they are - with the "goto"?

I won't be happy but I can stomach it. So you can leave the gotos there as you are conviced those add a readability value.

Best regards,
Jozsef
-
E-mail : [email protected], [email protected] PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences

          H-1525 Budapest 114, POB. 49, Hungary
#8 lore raw

RE: [PATCH v2 2/5] ipset: add "inner" flag implementation

In reply to Jozsef Kadlecsik
Quoted 27 lines

-----Original Message-----
From: Jozsef Kadlecsik [mailto:[email protected]]
Sent: Saturday, June 29, 2013 4:10 AM
To: Jeff Haran
Cc: Dash Four; Pablo Neira Ayuso; Netfilter Core Team
Subject: RE: [PATCH v2 2/5] ipset: add "inner" flag implementation

Quoted 12 lines · Jeff Haran

...

Quoted 10 lines

I disagree. By having "return false" (or "return 0", "return -1" and so on) instead of "goto err" it is not immediately apparent to someone who studies/reviews/uses the code that this is an error condition/path. I have been in that situation many times when I have to go back and look at a particular function call to see what "return false" or "return 0" actually means.

By including "goto err" instead of multiple "return false" statement, that makes it instantly obvious what the purpose of that statement is without having to look elsewhere.

Quoted 5 lines

I suppose as an alternative you could go way against the usual practice and put some text in a function header comment block indicating what the return code means. I know it doesn't get used much but C has had this /* comment */ thing for a long time. I've never understood why more people don't use it.

Quoted 4 lines

In general I'd agree with you, but this is a boolean function. So why should the the meaning of the return value "true" and "false" be explained? (This is also why I regard the goto unnecessary.)

I was just pointing out that if there was some concern by the author of this code that the intent of the function would not be clear to other readers, instead of "commenting" it via a coding standard (using the goto) it would be easier and clearer to simply write a function header comment describing it. In this particular case where the return code is a bool and the function is setting something, then it's pretty clear that if it returns true it worked and if it returns false it didn't, but the author described other cases where he had to research "what 'return false' or 'return 0' actually means in other kernel sources and he didn't want to create the same issue for others reading his code. I personally can sympathize with that, I've often been in the same boat trying to understand what t
he author of some piece of code had intended by a return of 0. Comments are easy to write and they remove ambiguity so long as they are kept up to date. They also serve to document what an author intended the code to do, which is not always what it actually does.

Jeff Haran