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

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

[Dash Four]

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 <mr.dash.four@googlemail.com>
---
  extensions/libxt_set.h |    8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/extensions/libxt_set.h b/extensions/libxt_set.h
index 47c3f5b..a352ac3 100644
--- a/extensions/libxt_set.h
+++ b/extensions/libxt_set.h
@@ -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'.");
  	}

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

[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:

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 <mr.dash.four@googlemail.com>
---
  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

diff --git a/kernel/include/linux/netfilter/ipset/ip_set.h b/kernel/include/linux/netfilter/ipset/ip_set.h
index 8499e25..5b6ef72 100644
--- a/kernel/include/linux/netfilter/ipset/ip_set.h
+++ b/kernel/include/linux/netfilter/ipset/ip_set.h
@@ -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
diff --git a/kernel/include/linux/netfilter/ipset/ip_set_getport.h b/kernel/include/linux/netfilter/ipset/ip_set_getport.h
index 90d0930..8cdc177 100644
--- a/kernel/include/linux/netfilter/ipset/ip_set_getport.h
+++ b/kernel/include/linux/netfilter/ipset/ip_set_getport.h
@@ -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);

diff --git a/kernel/include/linux/netfilter/ipset/ip_set_icmp.h b/kernel/include/linux/netfilter/ipset/ip_set_icmp.h
new file mode 100644
index 0000000..e116d28
--- /dev/null
+++ b/kernel/include/linux/netfilter/ipset/ip_set_icmp.h
@@ -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
+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*/
diff --git a/kernel/include/uapi/linux/netfilter/ipset/ip_set.h b/kernel/include/uapi/linux/netfilter/ipset/ip_set.h
index 8024cdf..e9e6586 100644
--- a/kernel/include/uapi/linux/netfilter/ipset/ip_set.h
+++ b/kernel/include/uapi/linux/netfilter/ipset/ip_set.h
@@ -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,
diff --git a/kernel/net/netfilter/ipset/ip_set_getport.c b/kernel/net/netfilter/ipset/ip_set_getport.c
index a0d96eb..73ccfb3 100644
--- a/kernel/net/netfilter/ipset/ip_set_getport.c
+++ b/kernel/net/netfilter/ipset/ip_set_getport.c
@@ -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 ||
+		    !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;
+}
+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);
+}
  EXPORT_SYMBOL_GPL(ip_set_get_ip_port);

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

[Jozsef Kadlecsik]

On Mon, 17 Jun 2013, Dash Four wrote:

> 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:

The patchset is a good step forward.

I have no objections against the other patches, however I have to comment 
this one.
 
> 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.
 
> Signed-off-by: Dash Four <mr.dash.four@googlemail.com>
> ---
>  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
> 
> diff --git a/kernel/include/linux/netfilter/ipset/ip_set.h
> b/kernel/include/linux/netfilter/ipset/ip_set.h
> index 8499e25..5b6ef72 100644
> --- a/kernel/include/linux/netfilter/ipset/ip_set.h
> +++ b/kernel/include/linux/netfilter/ipset/ip_set.h
> @@ -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.

>  #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 ||

Why do you have to check the IP header?

> +	    (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.

> +	*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;

The same comment as above.

> +	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;
> +	}

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

> +	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
> diff --git a/kernel/include/linux/netfilter/ipset/ip_set_getport.h
> b/kernel/include/linux/netfilter/ipset/ip_set_getport.h
> index 90d0930..8cdc177 100644
> --- a/kernel/include/linux/netfilter/ipset/ip_set_getport.h
> +++ b/kernel/include/linux/netfilter/ipset/ip_set_getport.h
> @@ -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);
> 
> diff --git a/kernel/include/linux/netfilter/ipset/ip_set_icmp.h
> b/kernel/include/linux/netfilter/ipset/ip_set_icmp.h
> new file mode 100644
> index 0000000..e116d28
> --- /dev/null
> +++ b/kernel/include/linux/netfilter/ipset/ip_set_icmp.h
> @@ -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
> +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.

> diff --git a/kernel/include/uapi/linux/netfilter/ipset/ip_set.h
> b/kernel/include/uapi/linux/netfilter/ipset/ip_set.h
> index 8024cdf..e9e6586 100644
> --- a/kernel/include/uapi/linux/netfilter/ipset/ip_set.h
> +++ b/kernel/include/uapi/linux/netfilter/ipset/ip_set.h
> @@ -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,
> diff --git a/kernel/net/netfilter/ipset/ip_set_getport.c
> b/kernel/net/netfilter/ipset/ip_set_getport.c
> index a0d96eb..73ccfb3 100644
> --- a/kernel/net/netfilter/ipset/ip_set_getport.c
> +++ b/kernel/net/netfilter/ipset/ip_set_getport.c
> @@ -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 ||
> +		    !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.

> +}
> +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);
> +}
>  EXPORT_SYMBOL_GPL(ip_set_get_ip_port);
> 

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
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

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

[Dash Four]

Jozsef Kadlecsik wrote:
> 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.

>> +#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.

>>  #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 ||
>>     
>
> 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.

>> +	    (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.

>  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.

>> +	*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;
>>     
>
> 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.

>> +	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;
>> +	}
>>     
>
> 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.

>> +#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.

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

[Jeff Haran]

> -----Original Message-----
> From: netfilter-devel-owner@vger.kernel.org [mailto:netfilter-devel-owner@vger.kernel.org] 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
> 
...
> 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

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

[Dash Four]

Jeff Haran wrote:
> 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.

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

[Jozsef Kadlecsik]

On Thu, 27 Jun 2013, Jeff Haran wrote:

> ...
> > 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.

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  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
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

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

[Jeff Haran]

> -----Original Message-----
> From: Jozsef Kadlecsik [mailto:kadlec@blackhole.kfki.hu]
> 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
> 
> On Thu, 27 Jun 2013, Jeff Haran wrote:
> 
> > ...
> > > 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.
> 
> 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

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

[Jozsef Kadlecsik]

On Thu, 27 Jun 2013, Dash Four wrote:

> Jozsef Kadlecsik wrote:
> > 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.
 
> > >  #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 ||
> > >     
> > 
> > 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.

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.
 
> > > +	    (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.
> 
> 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  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
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

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

[Dash Four]

Jozsef Kadlecsik wrote:
>> 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.

> 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.

>> 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 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"?

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

[Jozsef Kadlecsik]

On Sat, 29 Jun 2013, Dash Four wrote:

> > > 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  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
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

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

[Dash Four]

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 <mr.dash.four@googlemail.com>
---
  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(-)

diff --git a/kernel/net/netfilter/ipset/ip_set_bitmap_ip.c b/kernel/net/netfilter/ipset/ip_set_bitmap_ip.c
index ce99d26..20c5ade 100644
--- a/kernel/net/netfilter/ipset/ip_set_bitmap_ip.c
+++ b/kernel/net/netfilter/ipset/ip_set_bitmap_ip.c
@@ -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;

diff --git a/kernel/net/netfilter/ipset/ip_set_bitmap_ipmac.c b/kernel/net/netfilter/ipset/ip_set_bitmap_ipmac.c
index 6d5bad9..c7d490c 100644
--- a/kernel/net/netfilter/ipset/ip_set_bitmap_ipmac.c
+++ b/kernel/net/netfilter/ipset/ip_set_bitmap_ipmac.c
@@ -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;

diff --git a/kernel/net/netfilter/ipset/ip_set_bitmap_port.c b/kernel/net/netfilter/ipset/ip_set_bitmap_port.c
index b220489..1200e07 100644
--- a/kernel/net/netfilter/ipset/ip_set_bitmap_port.c
+++ b/kernel/net/netfilter/ipset/ip_set_bitmap_port.c
@@ -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);

diff --git a/kernel/net/netfilter/ipset/ip_set_hash_ip.c b/kernel/net/netfilter/ipset/ip_set_hash_ip.c
index 260c9a8..924a497 100644
--- a/kernel/net/netfilter/ipset/ip_set_hash_ip.c
+++ b/kernel/net/netfilter/ipset/ip_set_hash_ip.c
@@ -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;
diff --git a/kernel/net/netfilter/ipset/ip_set_hash_ipport.c b/kernel/net/netfilter/ipset/ip_set_hash_ipport.c
index 64caad3..dd52323 100644
--- a/kernel/net/netfilter/ipset/ip_set_hash_ipport.c
+++ b/kernel/net/netfilter/ipset/ip_set_hash_ipport.c
@@ -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);
  }

diff --git a/kernel/net/netfilter/ipset/ip_set_hash_ipportip.c b/kernel/net/netfilter/ipset/ip_set_hash_ipportip.c
index 2873bbc..e826a09 100644
--- a/kernel/net/netfilter/ipset/ip_set_hash_ipportip.c
+++ b/kernel/net/netfilter/ipset/ip_set_hash_ipportip.c
@@ -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);
  }

diff --git a/kernel/net/netfilter/ipset/ip_set_hash_ipportnet.c b/kernel/net/netfilter/ipset/ip_set_hash_ipportnet.c
index db0e761..378344a 100644
--- a/kernel/net/netfilter/ipset/ip_set_hash_ipportnet.c
+++ b/kernel/net/netfilter/ipset/ip_set_hash_ipportnet.c
@@ -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);
diff --git a/kernel/net/netfilter/ipset/ip_set_hash_net.c b/kernel/net/netfilter/ipset/ip_set_hash_net.c
index 846ec80..41acc51 100644
--- a/kernel/net/netfilter/ipset/ip_set_hash_net.c
+++ b/kernel/net/netfilter/ipset/ip_set_hash_net.c
@@ -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);
diff --git a/kernel/net/netfilter/ipset/ip_set_hash_netiface.c b/kernel/net/netfilter/ipset/ip_set_hash_netiface.c
index 8f0e496..129b8d2 100644
--- a/kernel/net/netfilter/ipset/ip_set_hash_netiface.c
+++ b/kernel/net/netfilter/ipset/ip_set_hash_netiface.c
@@ -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) {
diff --git a/kernel/net/netfilter/ipset/ip_set_hash_netport.c b/kernel/net/netfilter/ipset/ip_set_hash_netport.c
index 021d716..4befafd 100644
--- a/kernel/net/netfilter/ipset/ip_set_hash_netport.c
+++ b/kernel/net/netfilter/ipset/ip_set_hash_netport.c
@@ -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);

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

[Dash Four]

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

Revision history:

v1 * initial revision

Signed-off-by: Dash Four <mr.dash.four@googlemail.com>
---
  kernel/net/netfilter/xt_set.c |   21 +++++++++++++++++++++
  1 file changed, 21 insertions(+)

diff --git a/kernel/net/netfilter/xt_set.c b/kernel/net/netfilter/xt_set.c
index 00b9c58..430bcff 100644
--- a/kernel/net/netfilter/xt_set.c
+++ b/kernel/net/netfilter/xt_set.c
@@ -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 = {

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

[Dash Four]

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

Revision history:

v1 * initial revision

Signed-off-by: Dash Four <mr.dash.four@googlemail.com>
---
  extensions/libxt_set.c                 |  199 ++++++++++++++++++++++++++++++++
  extensions/libxt_set.man               |   10 ++
  include/linux/netfilter/ipset/ip_set.h |    2 +
  3 files changed, 211 insertions(+)

diff --git a/extensions/libxt_set.c b/extensions/libxt_set.c
index 2cb9e78..3b20c94 100644
--- a/extensions/libxt_set.c
+++ b/extensions/libxt_set.c
@@ -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)
diff --git a/extensions/libxt_set.man b/extensions/libxt_set.man
index 7012ef2..21e443c 100644
--- a/extensions/libxt_set.man
+++ b/extensions/libxt_set.man
@@ -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
diff --git a/include/linux/netfilter/ipset/ip_set.h b/include/linux/netfilter/ipset/ip_set.h
index eb9123e..1b3d9cb 100644
--- a/include/linux/netfilter/ipset/ip_set.h
+++ b/include/linux/netfilter/ipset/ip_set.h
@@ -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,