Skip to contentSkip to conversation

ipset matches either,both

PATCH In review Amin Azez 2 participants 4 messages Aug 3, 2006 – Aug 7, 2006 1 message not in the archive

Subsystems (2)
lore
Share & tools
lore

Amin Azez

author
#0 lore raw

PATCH 1/1 ipset matches either,both

These patches to ipset allow "either" and "both" to be specified as well as "src" and "dst".

I considered doing this as a hack in ip_set.c, by calling the match twice, faking flags of SRC and DST, but it was a hack and wouldn't easily cover the porthash which consumes two flags.

Instead I've altered the test of all of the set types, to accept BOTH and EITHER as well.

The encoding of BOTH and EITHER was troublesome as flags is a mix of an enum and bitmap-set; what I did isn't clean, but it will do, and doesn't break user-space compatability.

I've tested the behaviour of src dst either and both for iphash. I'm not sure about ipportmap because the man pages aren't clear how to write rules for this.

I'm not sure of the effect on this with bindings; I suspect that with either,both we may want a preferred test order, and the one that matches first is used to follow bindings.

I guess that
res = set->type->testip_kernel(set, skb, &ip, flags, i++);

sets ip for use by bindings following, and I haven't fiddled with this, so src will be set if that matched for either, otherwise "dst" will be set for "either" or "both"

Comments welcome.

include/linux/netfilter_ipv4/ip_set.h +9 -0
Index: linux-2.6.17.1/include/linux/netfilter_ipv4/ip_set.h
===================================================================
@@ -5,6 +5,7 @@
  *                         Patrick Schaaf <[email protected]>
  *                         Martin Josefsson <[email protected]>
  * Copyright (C) 2003-2004 Jozsef Kadlecsik <[email protected]>
+ * Copyright (C) 2006 UFO Mechanic          <[email protected]>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -79,6 +80,14 @@ typedef uint16_t ip_set_id_t;
 #define IPSET_SRC 		0x01	/* Source match/add */
 #define IPSET_DST		0x02	/* Destination match/add */
 #define IPSET_MATCH_INV		0x04	/* Inverse matching */
+#define IPSET_BOTH		0x08	/* Source and dest match/add */
+#define IPSET_EITHER		0x10	/* Source or dest match, add both*/
+/* Debug helper */
+#define IP_SET_OPERATION(index)	((index & IPSET_EITHER)?"EITHER":\
+				(index & IPSET_BOTH)?"BOTH":\
+				(index & IPSET_SRC)?"SRC":\
+				(index & IPSET_DST)?"DST":"UNKNOWN")
+
 
 /*
  * Set features
net/ipv4/netfilter/ip_set_iphash.c +19 -5
Index: linux-2.6.17.1/net/ipv4/netfilter/ip_set_iphash.c
===================================================================
@@ -1,4 +1,5 @@
 /* Copyright (C) 2003-2004 Jozsef Kadlecsik <[email protected]>
+ * Copyright (C) 2006 UFO Mechanic          <[email protected]>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -86,11 +87,24 @@ testip_kernel(struct ip_set *set, 
 	      const u_int32_t *flags,
 	      unsigned char index)
 {
-	return __testip(set,
-			ntohl(flags[index] & IPSET_SRC 
-				? skb->nh.iph->saddr 
-				: skb->nh.iph->daddr),
-			hash_ip);
+	int src;
+	int dst;
+
+	src = ( (flags[index] & (IPSET_SRC | IPSET_BOTH | IPSET_EITHER)) && \
+			__testip(set, ntohl(skb->nh.iph->saddr), hash_ip) );
+
+	/* don't need to check dst if we have src matched and want SRC or EITHER */
+	if (src && (flags[index] & (IPSET_SRC | IPSET_EITHER)) )
+		return src;
+
+	dst = ( (flags[index] & (IPSET_DST | IPSET_BOTH | IPSET_EITHER)) && \
+		__testip(set, ntohl(skb->nh.iph->daddr), hash_ip) );
+
+	/* Now work out the answer */
+	if (flags[index] & IPSET_BOTH)
+		return src && dst;
+	else
+		return src || dst;
 }
 
 static inline int
net/ipv4/netfilter/ip_set_ipmap.c +19 -8
Index: linux-2.6.17.1/net/ipv4/netfilter/ip_set_ipmap.c
===================================================================
@@ -1,6 +1,7 @@
 /* Copyright (C) 2000-2002 Joakim Axelsson <[email protected]>
  *                         Patrick Schaaf <[email protected]>
  * Copyright (C) 2003-2004 Jozsef Kadlecsik <[email protected]>
+ * Copyright (C) 2006 UFO Mechanic          <[email protected]>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -64,19 +65,29 @@ testip_kernel(struct ip_set *set, 
 	      const u_int32_t *flags,
 	      unsigned char index)
 {
-	int res;
+	int src;
+	int dst;
 	
 	DP("flag: %s src: %u.%u.%u.%u dst: %u.%u.%u.%u",
-	   flags[index] & IPSET_SRC ? "SRC" : "DST",
+	   IP_SET_OPERATION(flags[index]),
 	   NIPQUAD(skb->nh.iph->saddr),
 	   NIPQUAD(skb->nh.iph->daddr));
 
-	res =  __testip(set,
-			ntohl(flags[index] & IPSET_SRC 
-				? skb->nh.iph->saddr 
-				: skb->nh.iph->daddr),
-			hash_ip);
-	return (res < 0 ? 0 : res);
+	src = ( (flags[index] & (IPSET_SRC | IPSET_BOTH | IPSET_EITHER)) && \
+			__testip(set, ntohl(skb->nh.iph->saddr), hash_ip) > 0 );
+
+	/* don't need to check dst if we have src matched and want SRC or EITHER */
+	if (src && (flags[index] & (IPSET_SRC | IPSET_EITHER)) )
+		return src;
+
+	dst = ( (flags[index] & (IPSET_DST | IPSET_BOTH | IPSET_EITHER)) && \
+		__testip(set, ntohl(skb->nh.iph->daddr), hash_ip) > 0 );
+
+	/* Now work out the answer */
+	if (flags[index] & IPSET_BOTH)
+		return src && dst;
+	else
+		return src || dst;
 }
 
 static inline int
net/ipv4/netfilter/ip_set_ipporthash.c +61 -17

98 lines not on this page · raw message

net/ipv4/netfilter/ip_set_iptree.c +19 -8
Index: linux-2.6.17.1/net/ipv4/netfilter/ip_set_iptree.c
===================================================================
@@ -1,4 +1,5 @@
 /* Copyright (C) 2005 Jozsef Kadlecsik <[email protected]>
+ * Copyright (C) 2006 UFO Mechanic          <[email protected]>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -97,19 +98,29 @@ testip_kernel(struct ip_set *set, 
 	      const u_int32_t *flags,
 	      unsigned char index)
 {
-	int res;
+	int src;
+	int dst;
 	
 	DP("flag: %s src: %u.%u.%u.%u dst: %u.%u.%u.%u",
-	   flags[index] & IPSET_SRC ? "SRC" : "DST",
+	   IP_SET_OPERATION(flags[index]),
 	   NIPQUAD(skb->nh.iph->saddr),
 	   NIPQUAD(skb->nh.iph->daddr));
 
-	res =  __testip(set,
-			ntohl(flags[index] & IPSET_SRC 
-				? skb->nh.iph->saddr 
-				: skb->nh.iph->daddr),
-			hash_ip);
-	return (res < 0 ? 0 : res);
+	src = ( (flags[index] & (IPSET_SRC | IPSET_BOTH | IPSET_EITHER)) && \
+			__testip(set, ntohl(skb->nh.iph->saddr), hash_ip) > 0);
+
+	/* don't need to check dst if we have src matched and want SRC or EITHER */
+	if (src && (flags[index] & (IPSET_SRC | IPSET_EITHER)) )
+		return src;
+
+	dst = ( (flags[index] & (IPSET_DST | IPSET_BOTH | IPSET_EITHER)) && \
+		__testip(set, ntohl(skb->nh.iph->daddr), hash_ip) > 0);
+
+	/* Now work out the answer */
+	if (flags[index] & IPSET_BOTH)
+		return src && dst;
+	else
+		return src || dst;
 }
 
 #define ADDIP_WALK(map, elem, branch, type, cachep, flags) do {	\
net/ipv4/netfilter/ip_set_macipmap.c +37 -13

75 lines not on this page · raw message

net/ipv4/netfilter/ip_set_nethash.c +19 -5
Index: linux-2.6.17.1/net/ipv4/netfilter/ip_set_nethash.c
===================================================================
@@ -1,4 +1,5 @@
 /* Copyright (C) 2003-2004 Jozsef Kadlecsik <[email protected]>
+ * Copyright (C) 2006 UFO Mechanic          <[email protected]>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -109,11 +110,24 @@ testip_kernel(struct ip_set *set, 
 	      const u_int32_t *flags,
 	      unsigned char index)
 {
-	return __testip(set,
-			ntohl(flags[index] & IPSET_SRC 
-				? skb->nh.iph->saddr 
-				: skb->nh.iph->daddr),
-			hash_ip);
+	int src;
+	int dst;
+
+	src = ( (flags[index] & (IPSET_SRC | IPSET_BOTH | IPSET_EITHER)) && \
+			__testip(set, ntohl(skb->nh.iph->saddr), hash_ip) );
+
+	/* don't need to check dst if we have src matched and want SRC or EITHER */
+	if (src && (flags[index] & (IPSET_SRC | IPSET_EITHER)) )
+		return src;
+
+	dst = ( (flags[index] & (IPSET_DST | IPSET_BOTH | IPSET_EITHER)) && \
+		__testip(set, ntohl(skb->nh.iph->daddr), hash_ip) );
+
+	/* Now work out the answer */
+	if (flags[index] & IPSET_BOTH)
+		return src && dst;
+	else
+		return src || dst;
 }
 
 static inline int
net/ipv4/netfilter/ip_set_portmap.c +23 -8
Index: linux-2.6.17.1/net/ipv4/netfilter/ip_set_portmap.c
===================================================================
@@ -1,4 +1,5 @@
 /* Copyright (C) 2003-2004 Jozsef Kadlecsik <[email protected]>
+ * Copyright (C) 2006 UFO Mechanic          <[email protected]>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -99,16 +100,30 @@ testport_kernel(struct ip_set *set, 
 	        const u_int32_t *flags,
 	        unsigned char index)
 {
-	int res;
-	ip_set_ip_t port = get_port(skb, flags[index]);
+	int src=0;
+	int dst=0;
+	ip_set_ip_t port;
+
+	if ( (flags[index] & (IPSET_SRC | IPSET_BOTH | IPSET_EITHER))) {
+		DP("flag %s port %u", IP_SET_OPERATION(flags[index]), port);
+		port = get_port(skb, IPSET_SRC);
+		src = (port != INVALID_PORT) && (__testport(set, port, hash_port) > 0);
+	}
 
-	DP("flag %s port %u", flags[index] & IPSET_SRC ? "SRC" : "DST", port);	
-	if (port == INVALID_PORT)
-		return 0;	
+	if (src && (flags[index] & (IPSET_SRC | IPSET_EITHER)) )
+		return src;
 
-	res =  __testport(set, port, hash_port);
-	
-	return (res < 0 ? 0 : res);
+	if ( (flags[index] & (IPSET_DST | IPSET_BOTH | IPSET_EITHER))) {
+		DP("flag %s port %u", IP_SET_OPERATION(flags[index]), port);
+		port = get_port(skb, IPSET_DST);
+		dst = (port != INVALID_PORT) && (__testport(set, port, hash_port) > 0);
+	}
+
+	/* Now work out the answer */
+	if (flags[index] & IPSET_BOTH)
+		return src && dst;
+	else
+		return src || dst;
 }
 
 static inline int
libipt_set.c renamed from libipt_set.c.both +1 -1
@@ -120,7 +125,7 @@
 			break;		
 		printf("%s%s",
 		       i == 0 ? " " : ",",
-		       info->flags[i] & IPSET_SRC ? "src" : "dst");
+		       IP_SET_ACTION(info->flags[i]));
 	}
 	printf(" ");
 }
libipt_set.h renamed from libipt_set.h.both +10 -1
@@ -11,6 +11,11 @@
 #define DEBUGP(x, args...) 
 #endif
 
+#define IP_SET_ACTION(action) ((action & IPSET_EITHER)?"either":\
+				  (action & IPSET_BOTH)?"both":\
+				  (action & IPSET_SRC)?"src":\
+				  (action & IPSET_DST)?"dst":"unknown")
+
 static void
 parse_bindings(const char *optarg, struct ipt_set_info *info)
 {
@@ -24,9 +29,13 @@
 			info->flags[i++] |= IPSET_SRC;
 		else if (strncmp(ptr, "dst", 3) == 0)
 			info->flags[i++] |= IPSET_DST;
+		else if (strncmp(ptr, "either", 6) == 0)
+			info->flags[i++] |= IPSET_EITHER;
+		else if (strncmp(ptr, "both", 4) == 0)
+			info->flags[i++] |= IPSET_BOTH;
 		else
 			exit_error(PARAMETER_PROBLEM,
-				   "You must spefify (the comma separated list of) 'src' or 'dst'.");
+				   "You must spefify (the comma separated list of) 'src', 'dst', 'either', or 'both'."); sprintf(stderr,"set as %x\n",info->flags[0]);
 	}
 
 	if (tmp)
#1 lore raw

Hi,

Quoted 2 lines · Amin Azez

These patches to ipset allow "either" and "both" to be specified as well as "src" and "dst".

I'm a bit surprised because "either" can naturally be expressed by two iptables rules and "both" is equivalent with two set matches:

iptables ... -m set --set foo src -m set --set foo dst

Why do you need "both" and "either" encoded internally in ipset?

Quoted 6 lines

I considered doing this as a hack in ip_set.c, by calling the match twice, faking flags of SRC and DST, but it was a hack and wouldn't easily cover the porthash which consumes two flags.

Instead I've altered the test of all of the set types, to accept BOTH and EITHER as well.

But thus all the extensions (set types) have to know what "both" and "either" means, which'd belong to the ipset core as I believe, if introduced.

Quoted 3 lines

The encoding of BOTH and EITHER was troublesome as flags is a mix of an enum and bitmap-set; what I did isn't clean, but it will do, and doesn't break user-space compatability.

That is nice! I tend to break backward-compatibility too lightheartedly :-(.

Quoted 3 lines

I've tested the behaviour of src dst either and both for iphash. I'm not sure about ipportmap because the man pages aren't clear how to write rules for this.

In order to match both dst IP and dst port (for example all publicly available servers behind a firewall) one can use an ipportmap type of set and a single rule like this:

iptables ... -m set --set foo dst,dst

Quoted 3 lines

I'm not sure of the effect on this with bindings; I suspect that with either,both we may want a preferred test order, and the one that matches first is used to follow bindings.

(Unless someone convinces me that bindings are really great and should deserve to outlast) I think bindings are a dead-end: internally not efficient enough and hard to understand by the users.

Pablo and I started to rewrite ipset using netlink as kernel/userspace communication interface in the spring and thrown out bindings as unnecessary baggage. Unfortunately I have been overburdened since then and will only be able to pick up and continue the work in October :-(. But that gives a wide window for new ideas in ipset(/nfset) :-).

Best regards,
Jozsef
-
E-mail : [email protected], [email protected] PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics

          H-1525 Budapest 114, POB. 49, Hungary

A message that is not in the archive (sent off-list or not yet received): [email protected]

#3 lore raw
Quoted 12 lines · Amin Azez

* Jozsef Kadlecsik wrote, On 07/08/06 08:41:

Quoted 11 lines

Hi,

Quoted 2 lines · Amin Azez

These patches to ipset allow "either" and "both" to be specified as well as "src" and "dst".

I'm a bit surprised because "either" can naturally be expressed by two iptables rules and "both" is equivalent with two set matches:

Quoted 8 lines
Quoted 4 lines

iptables ... -m set --set foo src -m set --set foo dst

Why do you need "both" and "either" encoded internally in ipset?

Since when did iptables allow the same module to be used twice in a rule like that?

From the svn changelog:

iptables.c, rev 6474, modified Fri Mar 3 09:36:50 2006 UTC (5 months ago):

Multiple matches of the same type can be specified on the commandline.

If two or more matches of the same type are detected then the options are assumed to be grouped in order to tell which option belongs to which match:

... -m foo ... <options0> ... -m foo ... <options1> ...

Otherwise the commandline parsing is unmodified.

Quoted 4 lines

Although "either" can be expressed as two iptables rules, it gets hard if you want to use ! for neither, and your action is drop or return or something drastic. You have to start fiddling with subchains and it all gets very awkward.

You can express "neither A or B" as "not A and not B".

Quoted 7 lines
Quoted 7 lines
Quoted 6 lines

I considered doing this as a hack in ip_set.c, by calling the match twice, faking flags of SRC and DST, but it was a hack and wouldn't easily cover the porthash which consumes two flags.

Instead I've altered the test of all of the set types, to accept BOTH and EITHER as well.

Quoted 6 lines
Quoted 4 lines

But thus all the extensions (set types) have to know what "both" and "either" means, which'd belong to the ipset core as I believe, if introduced.

You are probably right, but the current design where ipporthash consumes two flags doesn't permit this.

Sorry, I don't see why.

Quoted 4 lines
Quoted 4 lines
Quoted 3 lines

I've tested the behaviour of src dst either and both for iphash. I'm not sure about ipportmap because the man pages aren't clear how to write rules for this.

Quoted 8 lines
Quoted 6 lines

In order to match both dst IP and dst port (for example all publicly available servers behind a firewall) one can use an ipportmap type of set and a single rule like this:

iptables ... -m set --set foo dst,dst

with bindings, yeah; we we going to do this, it would allow "both" if a set was bound to itself, but wouldn't allow "either."

No, without bindings. It *is* confusing :-(. In an ipporthash type of set (sorry for the typo above, ipportmap does not exist) you store an IP address and a port together. And therefore you can match any of the four possibilities (src,src,...., dst,dst) in one go, but the most useful is the example I cited:

# our public servers:
ipset -N foo ipporthash --network 192.168.0.0/16
ipset -A foo 192.168.0.1%22
ipset -A foo 192.168.0.2%80
ipset -A foo 192.168.0.3%25
....

# Allow access to our public servers
iptables -A FORWARD -p tcp -m set --set foo dst,dst -j ACCEPT

Quoted 8 lines
Quoted 6 lines

Pablo and I started to rewrite ipset using netlink as kernel/userspace communication interface in the spring and thrown out bindings as unnecessary baggage. Unfortunately I have been overburdened since then and will only be able to pick up and continue the work in October :-(. But that gives a wide window for new ideas in ipset(/nfset) :-).

Are you happy to "bless" this patch as embodying an ipset feature that will be retained in future revisions?

I'm open to feature requests and suggestions for the next revisions but still not convinced about "both" and "either", sorry:

"both" is equal to -m set --set foo src -m set --set foo dst
"neither" is equal to -m set ! --set foo src -m ! set --set foo dst "either" can be expressed by two iptables rules.

So what can we gain?

Best regards,
Jozsef
-
E-mail : [email protected], [email protected] PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics

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

* Jozsef Kadlecsik wrote, On 07/08/06 11:49:

Quoted 6 lines
Quoted 1 line

From the svn changelog:

iptables.c, rev 6474, modified Fri Mar 3 09:36:50 2006 UTC (5 months ago):

Multiple matches of the same type can be specified on the commandline.

Quoted 8 lines

If two or more matches of the same type are detected then the options are assumed to be grouped in order to tell which option belongs to which match:

... -m foo ... <options0> ... -m foo ... <options1> ...

Otherwise the commandline parsing is unmodified.

dang, I missed that.
...

Quoted 9 lines

I'm open to feature requests and suggestions for the next revisions but still not convinced about "both" and "either", sorry:

"both" is equal to -m set --set foo src -m set --set foo dst
"neither" is equal to -m set ! --set foo src -m ! set --set foo dst "either" can be expressed by two iptables rules.

So what can we gain?

indeed. *sob*

Sam