diff mbox

[v2,1/3] iptables: change 'iface' part in hash:net,iface set

Message ID 09f86300bb1913aa4c6063618fbbcd84fac2e608.1341871200.git.mr.dash.four@googlemail.com
State Not Applicable
Headers show

Commit Message

Mr Dash Four July 9, 2012, 10:23 p.m. UTC
Userspace changes to iptables, allowing 'in' and 'out' values to be
specified for the 'iface' part of hash:net,iface type sets only.

Man pages updated accordingly. This patch also makes some minor
corrections to the syntax of some console messages produced by
the set match and SET target.

Signed-off-by: Mr Dash Four <mr.dash.four@googlemail.com>
---
 extensions/libxt_SET.c                 |    9 +++--
 extensions/libxt_SET.man               |   23 +++++++------
 extensions/libxt_set.c                 |   15 ++++++---
 extensions/libxt_set.h                 |   58 +++++++++++++++++++++++++++++---
 extensions/libxt_set.man               |   20 +++++------
 include/linux/netfilter/ipset/ip_set.h |   32 ++++++++++++++++++
 6 files changed, 123 insertions(+), 34 deletions(-)

Comments

Jozsef Kadlecsik July 10, 2012, 3:54 p.m. UTC | #1
On Mon, 9 Jul 2012, Mr Dash Four wrote:

> Userspace changes to iptables, allowing 'in' and 'out' values to be
> specified for the 'iface' part of hash:net,iface type sets only.
> 
> Man pages updated accordingly. This patch also makes some minor
> corrections to the syntax of some console messages produced by
> the set match and SET target.
> 
> Signed-off-by: Mr Dash Four <mr.dash.four@googlemail.com>
> ---
>  extensions/libxt_SET.c                 |    9 +++--
>  extensions/libxt_SET.man               |   23 +++++++------
>  extensions/libxt_set.c                 |   15 ++++++---
>  extensions/libxt_set.h                 |   58 +++++++++++++++++++++++++++++---
>  extensions/libxt_set.man               |   20 +++++------
>  include/linux/netfilter/ipset/ip_set.h |   32 ++++++++++++++++++
>  6 files changed, 123 insertions(+), 34 deletions(-)
> 
> diff --git a/extensions/libxt_SET.c b/extensions/libxt_SET.c
> index a11db39..967d905 100644
> --- a/extensions/libxt_SET.c
> +++ b/extensions/libxt_SET.c
> @@ -176,7 +176,7 @@ parse_target(char **argv, int invert, struct xt_set_info *info,
>  			      "setname `%s' too long, max %d characters.",
>  			      optarg, IPSET_MAXNAMELEN - 1);
>  
> -	get_set_byname(optarg, info);
> +	get_set_byname_with_features(optarg, info);
>  	parse_dirs(argv[optind], info);
>  	optind++;
>  }
> @@ -206,15 +206,20 @@ print_target(const char *prefix, const struct xt_set_info *info)
>  {
>  	int i;
>  	char setname[IPSET_MAXNAMELEN];
> +	char *ptr;
>  
>  	if (info->index == IPSET_INVALID_ID)
>  		return;
>  	get_set_byid(setname, info->index);
>  	printf(" %s %s", prefix, setname);
>  	for (i = 1; i <= info->dim; i++) {
> +		if ((info->flags & IPSET_DIM_IFACE_INOUT) && i == IPSET_DIM_TWO)
> +			ptr = (info->flags & (1 << i) ? "in" : "out");
> +		else
> +			ptr = (info->flags & (1 << i) ? "src" : "dst");
>  		printf("%s%s",
>  		       i == 1 ? " " : ",",
> -		       info->flags & (1 << i) ? "src" : "dst");
> +			ptr);
>  	}
>  }

Introduce a new, revision 3 target with its own parse, print functions.
Don't forget to add it to the kernel part.
  
> diff --git a/extensions/libxt_SET.man b/extensions/libxt_SET.man
> index 63eb383..747235f 100644
> --- a/extensions/libxt_SET.man
> +++ b/extensions/libxt_SET.man
> @@ -1,25 +1,26 @@
> -This modules adds and/or deletes entries from IP sets which can be defined 
> +This module adds and/or deletes entries from IP sets which can be defined 
>  by ipset(8).
>  .TP
>  \fB\-\-add\-set\fP \fIsetname\fP \fIflag\fP[\fB,\fP\fIflag\fP...]
> -add the address(es)/port(s) of the packet to the sets
> +add the address(es)/port(s) of the packet to the set
>  .TP
>  \fB\-\-del\-set\fP \fIsetname\fP \fIflag\fP[\fB,\fP\fIflag\fP...]
> -delete the address(es)/port(s) of the packet from the sets
> +delete the address(es)/port(s) of the packet from the set
>  .IP
> -where flags are
> +where 'flag' above is comma separated list of
>  .BR "src"
>  and/or
>  .BR "dst"
> -specifications and there can be no more than six of them.
> +with the exception of hash:ip,iface where in addition to the above flags, the following is also allowed for the 'iface' part of that set:

Too long line, please break these up.

> +.BR "in"
> +or
> +.BR "out" 
> +corresponding to the incoming or outgoing network interface. The above flags cannot exceed six in total for a given set.
>  .TP
>  \fB\-\-timeout\fP \fIvalue\fP
> -when adding entry, the timeout value to use instead of the default
> -one from the set definition
> +when adding an entry, the timeout value to use instead of the default one from the set definition.
>  .TP
>  \fB\-\-exist\fP
> -when adding entry if it already exists, reset the timeout value
> -to the specified one or to the default from the set definition
> +when adding an entry, if such entry already exists, reset the timeout value to the specified one or to the default from the set definition.
>  .PP
> -Use of -j SET requires that ipset kernel support is provided, which, for
> -standard kernels, is the case since Linux 2.6.39.
> +Use of -j SET requires that ipset kernel support is provided, which, for standard kernels, is the case since Linux 2.6.39.
> diff --git a/extensions/libxt_set.c b/extensions/libxt_set.c
> index 77e3f07..dfc311a 100644
> --- a/extensions/libxt_set.c
> +++ b/extensions/libxt_set.c
> @@ -60,7 +60,7 @@ set_parse_v0(int c, char **argv, int invert, unsigned int *flags,
>  	case '2':
>  		fprintf(stderr,
>  			"--set option deprecated, please use --match-set\n");
> -	case '1':		/* --match-set <set> <flag>[,<flag> */
> +	case '1':		/* --match-set <set> <flag>[,<flag>] */
>  		if (info->u.flags[0])
>  			xtables_error(PARAMETER_PROBLEM,
>  				      "--match-set can be specified only once");
> @@ -140,7 +140,7 @@ set_parse_v1(int c, char **argv, int invert, unsigned int *flags,
>  	case '2':
>  		fprintf(stderr,
>  			"--set option deprecated, please use --match-set\n");
> -	case '1':		/* --match-set <set> <flag>[,<flag> */
> +	case '1':		/* --match-set <set> <flag>[,<flag>] */
>  		if (info->dim)
>  			xtables_error(PARAMETER_PROBLEM,
>  				      "--match-set can be specified only once");
> @@ -158,9 +158,9 @@ set_parse_v1(int c, char **argv, int invert, unsigned int *flags,
>  				      "setname `%s' too long, max %d characters.",
>  				      optarg, IPSET_MAXNAMELEN - 1);
>  
> -		get_set_byname(optarg, info);
> +		get_set_byname_with_features(optarg, info);
>  		parse_dirs(argv[optind], info);
> -		DEBUGP("parse: set index %u\n", info->index);
> +		DEBUGP("parse: set index %u, set flags %u\n", info->index, info->flags);
>  		optind++;
>  		
>  		*flags = 1;
> @@ -175,6 +175,7 @@ print_match(const char *prefix, const struct xt_set_info *info)
>  {
>  	int i;
>  	char setname[IPSET_MAXNAMELEN];
> +	char *ptr;
>  
>  	get_set_byid(setname, info->index);
>  	printf("%s %s %s",
> @@ -182,9 +183,13 @@ print_match(const char *prefix, const struct xt_set_info *info)
>  	       prefix,
>  	       setname); 
>  	for (i = 1; i <= info->dim; i++) {		
> +		if ((info->flags & IPSET_DIM_IFACE_INOUT) && i == IPSET_DIM_TWO)
> +			ptr = (info->flags & (1 << i) ? "in" : "out");
> +		else
> +			ptr = (info->flags & (1 << i) ? "src" : "dst");
>  		printf("%s%s",
>  		       i == 1 ? " " : ",",
> -		       info->flags & (1 << i) ? "src" : "dst");
> +		       ptr);
>  	}
>  }

Introduce a new, revision 2 match with its own parse, print functions and 
add it to the kernel part as well.
  
> diff --git a/extensions/libxt_set.h b/extensions/libxt_set.h
> index 47c3f5b..d47e263 100644
> --- a/extensions/libxt_set.h
> +++ b/extensions/libxt_set.h
> @@ -101,6 +101,39 @@ get_set_byname(const char *setname, struct xt_set_info *info)
>  }
>  
>  static void
> +get_set_byname_with_features(const char *setname, struct xt_set_info *info)
> +{
> +	struct ip_set_req_get_features req;
> +	socklen_t size = sizeof(struct ip_set_req_get_features);
> +	int res, sockfd;
> +
> +	sockfd = get_version(&req.version);
> +	req.op = IP_SET_OP_GET_FEATURES;
> +	strncpy(req.set.name, setname, IPSET_MAXNAMELEN);
> +	req.set.name[IPSET_MAXNAMELEN - 1] = '\0';
> +	res = getsockopt(sockfd, SOL_IP, SO_IP_SET, &req, &size);
> +	close(sockfd);
> +	if (res != 0 && errno== EBADMSG) { /* IP_SET_OP_GET_FEATURES not supported, revert back to IP_SET_OP_GET_BYNAME */
> +		get_set_byname(setname, info);

With the new target/match revisions, there's no need to call the old 
interface.

> +	} else {
> +		if (res != 0)
> +			xtables_error(OTHER_PROBLEM,
> +				"Problem when communicating with ipset, errno=%d.\n",
> +				errno);
> +		if (size != sizeof(struct ip_set_req_get_features))
> +			xtables_error(OTHER_PROBLEM,
> +				"Incorrect return size from kernel during ipset lookup, "
> +				"(want %zu, got %zu)\n",
> +				sizeof(struct ip_set_req_get_features), (size_t)size);
> +		if (req.set.index == IPSET_INVALID_ID)
> +			xtables_error(PARAMETER_PROBLEM,
> +				      "Set %s doesn't exist.\n", setname);
> +		info->index = req.set.index;
> +		info->flags |= req.features;

Better do not mix the received features with the info flags. Make 
get_set_byname_with_features non-void and just return the value to the 
caller.

> +	}
> +}
> +
> +static void
>  parse_dirs_v0(const char *opt_arg, struct xt_set_info_v0 *info)
>  {
>  	char *saved = strdup(opt_arg);
> @@ -115,7 +148,7 @@ parse_dirs_v0(const char *opt_arg, struct xt_set_info_v0 *info)
>  			info->u.flags[i++] |= IPSET_DST;
>  		else
>  			xtables_error(PARAMETER_PROBLEM,
> -				"You must spefify (the comma separated list of) 'src' or 'dst'.");
> +				"You must specify (comma separated list of) 'src' or 'dst'.");
>  	}
>  
>  	if (tmp)
> @@ -130,16 +163,31 @@ static void
>  parse_dirs(const char *opt_arg, struct xt_set_info *info)
>  {
>  	char *saved = strdup(opt_arg);
> +	__u8 saved_flags = info->flags;
>  	char *ptr, *tmp = saved;
> -	
> +
> +	/* clear everything else, but IPSET_INV_MATCH */
> +	info->flags &= IPSET_INV_MATCH;
> +
>  	while (info->dim < IPSET_DIM_MAX && tmp != NULL) {
>  		info->dim++;
>  		ptr = strsep(&tmp, ",");
> -		if (strncmp(ptr, "src", 3) == 0)
> -			info->flags |= (1 << info->dim);
> +		if (strncmp(ptr, "in", 2) == 0 && 	/* 'in' */
> +			info->dim == IPSET_DIM_TWO && 	/* x,'in' */
> +			saved_flags & IPSET_TYPE_IFACE)	/* hash:net,iface type set */
> +				info->flags |= (1 << info->dim | IPSET_DIM_IFACE_INOUT);
> +
> +		else if (strncmp(ptr, "out", 3) == 0 && /* 'out' */
> +			info->dim == IPSET_DIM_TWO && 	/* x,'out' */
> +			saved_flags & IPSET_TYPE_IFACE)	/* hash:net,iface type set */
> +				info->flags |= IPSET_DIM_IFACE_INOUT;
> +
> +		else if (strncmp(ptr, "src", 3) == 0)
> +				info->flags |= (1 << info->dim);
> +
>  		else if (strncmp(ptr, "dst", 3) != 0)
>  			xtables_error(PARAMETER_PROBLEM,
> -				"You must spefify (the comma separated list of) 'src' or 'dst'.");
> +				"You must specify (comma separated list of) 'src'|'dst' or 'in'|'out' just for the interface part of hash:net,iface set, if used.");
>  	}

Keep the old parse_dir (rename to parse_dirs_v1) and introduce a new one. 
Isn't there a comma missing from the error message, i.e:

You must specify (comma separated list of) src'|'dst',
or 'in'|'out' just for the interface part of hash:net,iface set, if used.

And maybe it'd be good to print it in two lines, like here.
  
>  	if (tmp)
> diff --git a/extensions/libxt_set.man b/extensions/libxt_set.man
> index 1ad9085..31be0eb 100644
> --- a/extensions/libxt_set.man
> +++ b/extensions/libxt_set.man
> @@ -1,22 +1,20 @@
>  This module matches IP sets which can be defined by ipset(8).
>  .TP
>  [\fB!\fP] \fB\-\-match\-set\fP \fIsetname\fP \fIflag\fP[\fB,\fP\fIflag\fP]...
> -where flags are the comma separated list of
> +where 'flag' above is comma separated list of
>  .BR "src"
>  and/or
>  .BR "dst" 
> -specifications and there can be no more than six of them. Hence the command
> +with the exception of hash:ip,iface where, in addition to these two options, the following is also allowed for the 'iface' part:
> +.BR "in" 
> +or
> +.BR "out" 
> +corresponding to the incoming or outgoing network interface. The above options cannot exceed six in total for a given set. The command
>  .IP
>   iptables \-A FORWARD \-m set \-\-match\-set test src,dst
>  .IP
> -will match packets, for which (if the set type is ipportmap) the source
> -address and destination port pair can be found in the specified set. If
> -the set type of the specified set is single dimension (for example ipmap),
> -then the command will match packets for which the source address can be
> -found in the specified set. 
> +will match packets for which, if the set is of type hash:ip,port for example, the source IP address and destination port pair can be found and matched successfully. If the specified set is one dimensional (i.e. bitmap:ip), then the command will match packets for which the source address can be found in the set specified.
>  .PP
> -The option \fB\-\-match\-set\fP can be replaced by \fB\-\-set\fP if that does 
> -not clash with an option of other extensions.
> +The option \fB\-\-match\-set\fP can be replaced by \fB\-\-set\fP if that does not clash with an option from other extensions.
>  .PP
> -Use of -m set requires that ipset kernel support is provided, which, for
> -standard kernels, is the case since Linux 2.6.39.
> +Use of -m set requires that ipset kernel support is provided, which, for standard kernels, is the case since Linux 2.6.39.
> diff --git a/include/linux/netfilter/ipset/ip_set.h b/include/linux/netfilter/ipset/ip_set.h
> index 79cb077..f2a038c 100644
> --- a/include/linux/netfilter/ipset/ip_set.h
> +++ b/include/linux/netfilter/ipset/ip_set.h
> @@ -186,6 +186,10 @@ enum ip_set_dim {
>  	 * If changed, new revision of iptables match/target is required.
>  	 */
>  	IPSET_DIM_MAX = 6,
> +	/* 
> +	 * Indicates whether the new 'iface' format (in/out) has been used.
> +	 */
> +	IPSET_DIM_IFACE = 7, 
>  };

Rename here and below.
  
>  /* Option flags for kernel operations */
> @@ -194,8 +198,28 @@ enum ip_set_kopt {
>  	IPSET_DIM_ONE_SRC = (1 << IPSET_DIM_ONE),
>  	IPSET_DIM_TWO_SRC = (1 << IPSET_DIM_TWO),
>  	IPSET_DIM_THREE_SRC = (1 << IPSET_DIM_THREE),
> +	IPSET_DIM_IFACE_INOUT = (1 << IPSET_DIM_IFACE),
>  };
>  
> +/* Set features */
> +enum ip_set_feature {
> +	IPSET_TYPE_IP_FLAG = 0,
> +	IPSET_TYPE_IP = (1 << IPSET_TYPE_IP_FLAG),
> +	IPSET_TYPE_PORT_FLAG = 1,
> +	IPSET_TYPE_PORT = (1 << IPSET_TYPE_PORT_FLAG),
> +	IPSET_TYPE_MAC_FLAG = 2,
> +	IPSET_TYPE_MAC = (1 << IPSET_TYPE_MAC_FLAG),
> +	IPSET_TYPE_IP2_FLAG = 3,
> +	IPSET_TYPE_IP2 = (1 << IPSET_TYPE_IP2_FLAG),
> +	IPSET_TYPE_NAME_FLAG = 4,
> +	IPSET_TYPE_NAME = (1 << IPSET_TYPE_NAME_FLAG),
> +	IPSET_TYPE_IFACE_FLAG = 5,
> +	IPSET_TYPE_IFACE = (1 << IPSET_TYPE_IFACE_FLAG),
> +	/* Strictly speaking not a feature, but a flag for dumping:
> +	 * this settype must be dumped last */
> +	IPSET_DUMP_LAST_FLAG = 7,
> +	IPSET_DUMP_LAST = (1 << IPSET_DUMP_LAST_FLAG),
> +};

The header files in include/linux/netfilter are the same as the kernel 
ones with the parts protected by "#ifdef __KERNEL__" ripped out. So please
move this enum definition in the kernel include file outside of the kernel 
specific part, according to the placement here. 
  
>  /* Interface to iptables/ip6tables */
>  
> @@ -216,6 +240,14 @@ struct ip_set_req_get_set {
>  #define IP_SET_OP_GET_BYINDEX	0x00000007	/* Get set name by index */
>  /* Uses ip_set_req_get_set */
>  
> +#define IP_SET_OP_GET_FEATURES	0x00000008	/* Get set features by name */
> +struct ip_set_req_get_features {
> +	unsigned int op;
> +	unsigned int version;
> +	__u8 features;
> +	union ip_set_name_index set;
> +};

Let features be unsigned int here too.

>  #define IP_SET_OP_VERSION	0x00000100	/* Ask kernel version */
>  struct ip_set_req_version {
>  	unsigned op;
> -- 
> 1.7.10.4

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
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mr Dash Four July 10, 2012, 11:41 p.m. UTC | #2
>> @@ -206,15 +206,20 @@ print_target(const char *prefix, const struct xt_set_info *info)
>>  {
>>  	int i;
>>  	char setname[IPSET_MAXNAMELEN];
>> +	char *ptr;
>>  
>>  	if (info->index == IPSET_INVALID_ID)
>>  		return;
>>  	get_set_byid(setname, info->index);
>>  	printf(" %s %s", prefix, setname);
>>  	for (i = 1; i <= info->dim; i++) {
>> +		if ((info->flags & IPSET_DIM_IFACE_INOUT) && i == IPSET_DIM_TWO)
>> +			ptr = (info->flags & (1 << i) ? "in" : "out");
>> +		else
>> +			ptr = (info->flags & (1 << i) ? "src" : "dst");
>>  		printf("%s%s",
>>  		       i == 1 ? " " : ",",
>> -		       info->flags & (1 << i) ? "src" : "dst");
>> +			ptr);
>>  	}
>>  }
> 
> Introduce a new, revision 3 target with its own parse, print functions.
> Don't forget to add it to the kernel part.
I am not completely clear I understand you. 

If I introduce a new version and register it with the kernel, how is the "new iptables - old kernel" combination going to function? Similarly, if I rename the new functions to something else, won't that cause compatibility issues where other programs are going to look for these functions (from what I remember these functions are defined in the C header files, so, potentially, after this change they are bound to break something!). Could you elaborate a bit more please?


>> -specifications and there can be no more than six of them.
>> +with the exception of hash:ip,iface where in addition to the above flags, the following is also allowed for the 'iface' part of that set:
> 
> Too long line, please break these up.
I tried to do that initially, but the man page, when displayed, gives me long spaces in the places where I have inserted a new lines, which I find rather annoying. Is this a "feature" of the man (the program), or is there a way to avoid all this? That was the sole reason to collapse everything and use single lines (I have done this across all man pages).

>> @@ -182,9 +183,13 @@ print_match(const char *prefix, const struct xt_set_info *info)
>>  	       prefix,
>>  	       setname); 
>>  	for (i = 1; i <= info->dim; i++) {		
>> +		if ((info->flags & IPSET_DIM_IFACE_INOUT) && i == IPSET_DIM_TWO)
>> +			ptr = (info->flags & (1 << i) ? "in" : "out");
>> +		else
>> +			ptr = (info->flags & (1 << i) ? "src" : "dst");
>>  		printf("%s%s",
>>  		       i == 1 ? " " : ",",
>> -		       info->flags & (1 << i) ? "src" : "dst");
>> +		       ptr);
>>  	}
>>  }
> 
> Introduce a new, revision 2 match with its own parse, print functions and 
> add it to the kernel part as well.
See above - I need to understand how this works and how the compatibility is protected.

>> +	if (res != 0 && errno== EBADMSG) { /* IP_SET_OP_GET_FEATURES not supported, revert back to IP_SET_OP_GET_BYNAME */
>> +		get_set_byname(setname, info);
> 
> With the new target/match revisions, there's no need to call the old 
> interface.
Agreed. It was done solely for compatibility reasons just in case "new iptables - old kernel" combination is used, since I am not clear how the new "versioning" is going to work.

>> +		info->index = req.set.index;
>> +		info->flags |= req.features;
> 
> Better do not mix the received features with the info flags. Make 
> get_set_byname_with_features non-void and just return the value to the 
> caller.
Again, this was done out of necessity as: 1) I only used a single bit from "features" and didn't need anything else; 2) The "features" itself was __u8, so that variable was adequate to hold whatever information I wanted from that function; and 3) this was done for the sake of compatibility as I didn't know if I introduce changes to this function whether anything else is going to brake (I am sure Mr Engelhardt would have been on my case in an instant if that was so). 

Provided I could use a new version and know the implications of that, of course, I'll change it and use the new version.

> Keep the old parse_dir (rename to parse_dirs_v1) and introduce a new one. 
> Isn't there a comma missing from the error message, i.e:
> 
> You must specify (comma separated list of) src'|'dst',
> or 'in'|'out' just for the interface part of hash:net,iface set, if used.
> 
> And maybe it'd be good to print it in two lines, like here.
No, I have something different in mind, which is much better and will be introduced with the next set of patches.

>> +	IPSET_DIM_IFACE = 7, 
>>  };
> 
> Rename here and below.
Noted, will do.

>> +/* Set features */
>> +enum ip_set_feature {
>> +	IPSET_TYPE_IP_FLAG = 0,
>> +	IPSET_TYPE_IP = (1 << IPSET_TYPE_IP_FLAG),
>> +	IPSET_TYPE_PORT_FLAG = 1,
>> +	IPSET_TYPE_PORT = (1 << IPSET_TYPE_PORT_FLAG),
>> +	IPSET_TYPE_MAC_FLAG = 2,
>> +	IPSET_TYPE_MAC = (1 << IPSET_TYPE_MAC_FLAG),
>> +	IPSET_TYPE_IP2_FLAG = 3,
>> +	IPSET_TYPE_IP2 = (1 << IPSET_TYPE_IP2_FLAG),
>> +	IPSET_TYPE_NAME_FLAG = 4,
>> +	IPSET_TYPE_NAME = (1 << IPSET_TYPE_NAME_FLAG),
>> +	IPSET_TYPE_IFACE_FLAG = 5,
>> +	IPSET_TYPE_IFACE = (1 << IPSET_TYPE_IFACE_FLAG),
>> +	/* Strictly speaking not a feature, but a flag for dumping:
>> +	 * this settype must be dumped last */
>> +	IPSET_DUMP_LAST_FLAG = 7,
>> +	IPSET_DUMP_LAST = (1 << IPSET_DUMP_LAST_FLAG),
>> +};
> 
> The header files in include/linux/netfilter are the same as the kernel 
> ones with the parts protected by "#ifdef __KERNEL__" ripped out. So please
> move this enum definition in the kernel include file outside of the kernel 
> specific part, according to the placement here. 
I am not sure I understand this either! This is iptables' ip_set.h (userspace) and is more-or-less a carbon copy of the "essential" data structures from the "real" ip_set.h from ipset (kernel).  Could you clarify please?

>> +#define IP_SET_OP_GET_FEATURES	0x00000008	/* Get set features by name */
>> +struct ip_set_req_get_features {
>> +	unsigned int op;
>> +	unsigned int version;
>> +	__u8 features;
>> +	union ip_set_name_index set;
>> +};
> 
> Let features be unsigned int here too.
As I already explained, the original "features" field from the ip_set struct, I think, has u8 (a byte), not unsigned int (a word). If I had introduced it, I would have broken half the code in the parse/print functions. I still don't see any sense in encapsulating something which is u8 (features in ip_set struct) as unsigned int here. Why would you want to do this Jozsef?

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jozsef Kadlecsik July 12, 2012, 7:11 a.m. UTC | #3
On Wed, 11 Jul 2012, Mr Dash Four wrote:

> >> @@ -206,15 +206,20 @@ print_target(const char *prefix, const struct xt_set_info *info)
> >>  {
> >>  	int i;
> >>  	char setname[IPSET_MAXNAMELEN];
> >> +	char *ptr;
> >>  
> >>  	if (info->index == IPSET_INVALID_ID)
> >>  		return;
> >>  	get_set_byid(setname, info->index);
> >>  	printf(" %s %s", prefix, setname);
> >>  	for (i = 1; i <= info->dim; i++) {
> >> +		if ((info->flags & IPSET_DIM_IFACE_INOUT) && i == IPSET_DIM_TWO)
> >> +			ptr = (info->flags & (1 << i) ? "in" : "out");
> >> +		else
> >> +			ptr = (info->flags & (1 << i) ? "src" : "dst");
> >>  		printf("%s%s",
> >>  		       i == 1 ? " " : ",",
> >> -		       info->flags & (1 << i) ? "src" : "dst");
> >> +			ptr);
> >>  	}
> >>  }
> > 
> > Introduce a new, revision 3 target with its own parse, print functions.
> > Don't forget to add it to the kernel part.
> I am not completely clear I understand you. 
> 
> If I introduce a new version and register it with the kernel, how is the 
> "new iptables - old kernel" combination going to function? 

A userspace match/target "works" with the corresponding kernel 
match/target only when their revision numbers match. The new revisions are 
our standard way to introduce new features in matches/targets so that it 
won't break anything and work fine in any old-new kernel-iptables 
combinations: the system uses (falls back to) the highest revision which 
is avaliable at both sides.

The new match/target revision in userspace just need the new 
parse/print/save functions, the matching new match/target revision in 
kernel space differ from the current one just in revision number.

> Similarly, if I rename the new functions to something else, won't that 
> cause compatibility issues where other programs are going to look for 
> these functions (from what I remember these functions are defined in the 
> C header files, so, potentially, after this change they are bound to 
> break something!). Could you elaborate a bit more please?

These functions are static. Nothing else uses them.

> >> -specifications and there can be no more than six of them.
> >> +with the exception of hash:ip,iface where in addition to the above flags, the following is also allowed for the 'iface' part of that set:
> > 
> > Too long line, please break these up.
> I tried to do that initially, but the man page, when displayed, gives me 
> long spaces in the places where I have inserted a new lines, which I 
> find rather annoying. Is this a "feature" of the man (the program), or 
> is there a way to avoid all this? That was the sole reason to collapse 
> everything and use single lines (I have done this across all man pages).

Let there be long spaces, I'll fix those. But with so long lines, it's 
hard to see the changes.

> >> @@ -182,9 +183,13 @@ print_match(const char *prefix, const struct xt_set_info *info)
> >>  	       prefix,
> >>  	       setname); 
> >>  	for (i = 1; i <= info->dim; i++) {		
> >> +		if ((info->flags & IPSET_DIM_IFACE_INOUT) && i == IPSET_DIM_TWO)
> >> +			ptr = (info->flags & (1 << i) ? "in" : "out");
> >> +		else
> >> +			ptr = (info->flags & (1 << i) ? "src" : "dst");
> >>  		printf("%s%s",
> >>  		       i == 1 ? " " : ",",
> >> -		       info->flags & (1 << i) ? "src" : "dst");
> >> +		       ptr);
> >>  	}
> >>  }
> > 
> > Introduce a new, revision 2 match with its own parse, print functions and 
> > add it to the kernel part as well.
> See above - I need to understand how this works and how the 
> compatibility is protected.
> 
> >> +	if (res != 0 && errno== EBADMSG) { /* IP_SET_OP_GET_FEATURES not supported, revert back to IP_SET_OP_GET_BYNAME */
> >> +		get_set_byname(setname, info);
> > 
> > With the new target/match revisions, there's no need to call the old 
> > interface.

> Agreed. It was done solely for compatibility reasons just in case "new 
> iptables - old kernel" combination is used, since I am not clear how the 
> new "versioning" is going to work.
> 
> >> +		info->index = req.set.index;
> >> +		info->flags |= req.features;
> > 
> > Better do not mix the received features with the info flags. Make 
> > get_set_byname_with_features non-void and just return the value to the 
> > caller.

> Again, this was done out of necessity as: 1) I only used a single bit 
> from "features" and didn't need anything else; 2) The "features" itself 
> was __u8, so that variable was adequate to hold whatever information I 
> wanted from that function; and 3) this was done for the sake of 
> compatibility as I didn't know if I introduce changes to this function 
> whether anything else is going to brake (I am sure Mr Engelhardt would 
> have been on my case in an instant if that was so).
> 
> Provided I could use a new version and know the implications of that, of 
> course, I'll change it and use the new version.

Do that. 
 
> > Keep the old parse_dir (rename to parse_dirs_v1) and introduce a new one. 
> > Isn't there a comma missing from the error message, i.e:
> > 
> > You must specify (comma separated list of) src'|'dst',
> > or 'in'|'out' just for the interface part of hash:net,iface set, if used.
> > 
> > And maybe it'd be good to print it in two lines, like here.
> No, I have something different in mind, which is much better and will be 
> introduced with the next set of patches.

The two lines were just an idea to separate the cases better and help the 
reader.
 
> >> +enum ip_set_feature {
> >> +	IPSET_TYPE_IP_FLAG = 0,
> >> +	IPSET_TYPE_IP = (1 << IPSET_TYPE_IP_FLAG),
> >> +	IPSET_TYPE_PORT_FLAG = 1,
> >> +	IPSET_TYPE_PORT = (1 << IPSET_TYPE_PORT_FLAG),
> >> +	IPSET_TYPE_MAC_FLAG = 2,
> >> +	IPSET_TYPE_MAC = (1 << IPSET_TYPE_MAC_FLAG),
> >> +	IPSET_TYPE_IP2_FLAG = 3,
> >> +	IPSET_TYPE_IP2 = (1 << IPSET_TYPE_IP2_FLAG),
> >> +	IPSET_TYPE_NAME_FLAG = 4,
> >> +	IPSET_TYPE_NAME = (1 << IPSET_TYPE_NAME_FLAG),
> >> +	IPSET_TYPE_IFACE_FLAG = 5,
> >> +	IPSET_TYPE_IFACE = (1 << IPSET_TYPE_IFACE_FLAG),
> >> +	/* Strictly speaking not a feature, but a flag for dumping:
> >> +	 * this settype must be dumped last */
> >> +	IPSET_DUMP_LAST_FLAG = 7,
> >> +	IPSET_DUMP_LAST = (1 << IPSET_DUMP_LAST_FLAG),
> >> +};
> > 
> > The header files in include/linux/netfilter are the same as the kernel 
> > ones with the parts protected by "#ifdef __KERNEL__" ripped out. So please
> > move this enum definition in the kernel include file outside of the kernel 
> > specific part, according to the placement here. 
> I am not sure I understand this either! This is iptables' ip_set.h 
> (userspace) and is more-or-less a carbon copy of the "essential" data 
> structures from the "real" ip_set.h from ipset (kernel).  Could you 
> clarify please?

The header files in include/linux/netfilter in the iptables source are 
usually not maintained manually. They are generated from the kernel header 
files by filtering out the kernel specific parts protected by the ifdefs.

At the moment, the enum ip_set_feature definition is kernel specific in 
the kernel header file. Next time Pablo regenerates the header files for 
iptables from the kernel ones, your modification above will be lost. 
Therefore the enum definition must be moved out from the "#ifdef 
__KERNEL__" region in the kernel header file.

> >> +#define IP_SET_OP_GET_FEATURES	0x00000008	/* Get set features by name */
> >> +struct ip_set_req_get_features {
> >> +	unsigned int op;
> >> +	unsigned int version;
> >> +	__u8 features;
> >> +	union ip_set_name_index set;
> >> +};
> > 
> > Let features be unsigned int here too.

> As I already explained, the original "features" field from the ip_set 
> struct, I think, has u8 (a byte), not unsigned int (a word). If I had 
> introduced it, I would have broken half the code in the parse/print 
> functions. I still don't see any sense in encapsulating something which 
> is u8 (features in ip_set struct) as unsigned int here. Why would you 
> want to do this Jozsef?

Read my other mail to PATCH v2 3/3.

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
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mr Dash Four July 13, 2012, 12:41 a.m. UTC | #4
> A userspace match/target "works" with the corresponding kernel 
> match/target only when their revision numbers match. The new revisions are 
> our standard way to introduce new features in matches/targets so that it 
> won't break anything and work fine in any old-new kernel-iptables 
> combinations: the system uses (falls back to) the highest revision which 
> is avaliable at both sides.
>
> The new match/target revision in userspace just need the new 
> parse/print/save functions, the matching new match/target revision in 
> kernel space differ from the current one just in revision number.
>   
All noted and tested - that is exactly how it works. Thanks.

>> Similarly, if I rename the new functions to something else, won't that 
>> cause compatibility issues where other programs are going to look for 
>> these functions (from what I remember these functions are defined in the 
>> C header files, so, potentially, after this change they are bound to 
>> break something!). Could you elaborate a bit more please?
>>     
>
> These functions are static. Nothing else uses them.
>   
OK, that's good because I intend to change the parse_dirs function in 
the new "version" and introduce additional parameter called "features" 
so that these are used directly and not rely on the info->flags to store 
these (as was the case up until now). That would also allow for 
additional features to be added in the future, if needed (u8 is almost 
exhausted - you have one spare bit left there!).

> Let there be long spaces, I'll fix those. But with so long lines, it's 
> hard to see the changes.
>   
Noted, will do.

> The header files in include/linux/netfilter in the iptables source are 
> usually not maintained manually. They are generated from the kernel header 
> files by filtering out the kernel specific parts protected by the ifdefs.
>
> At the moment, the enum ip_set_feature definition is kernel specific in 
> the kernel header file. Next time Pablo regenerates the header files for 
> iptables from the kernel ones, your modification above will be lost. 
> Therefore the enum definition must be moved out from the "#ifdef 
> __KERNEL__" region in the kernel header file.
>   
Yep, just saw that too, so I'll just move the ip_set_feature enum just 
above the #ifdef __KERNEL__ part and I assume it would be picked up 
"automatically", is that right?

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jozsef Kadlecsik July 13, 2012, 8:11 a.m. UTC | #5
On Fri, 13 Jul 2012, Mr Dash Four wrote:

> > These functions are static. Nothing else uses them.
> >   
> OK, that's good because I intend to change the parse_dirs function in 
> the new "version" and introduce additional parameter called "features" 
> so that these are used directly and not rely on the info->flags to store 
> these (as was the case up until now). That would also allow for 
> additional features to be added in the future, if needed (u8 is almost 
> exhausted - you have one spare bit left there!).

The almost exhausted u8 is an additional reason why it's better to use 
unsigned int in the *protocol* between the kernel-userspace 
(struct ip_set_req_get_features).
 
> > At the moment, the enum ip_set_feature definition is kernel specific 
> > in the kernel header file. Next time Pablo regenerates the header 
> > files for iptables from the kernel ones, your modification above will 
> > be lost. Therefore the enum definition must be moved out from the 
> > "#ifdef __KERNEL__" region in the kernel header file.
> >   
> Yep, just saw that too, so I'll just move the ip_set_feature enum just 
> above the #ifdef __KERNEL__ part and I assume it would be picked up 
> "automatically", is that right?

No, let the patchset be complete in itself, i.e. include the patches both 
for the kernel and iptables header files.

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
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mr Dash Four July 13, 2012, 1:56 p.m. UTC | #6
>> OK, that's good because I intend to change the parse_dirs function in 
>> the new "version" and introduce additional parameter called "features" 
>> so that these are used directly and not rely on the info->flags to store 
>> these (as was the case up until now). That would also allow for 
>> additional features to be added in the future, if needed (u8 is almost 
>> exhausted - you have one spare bit left there!).
>>     
>
> The almost exhausted u8 is an additional reason why it's better to use 
> unsigned int in the *protocol* between the kernel-userspace 
> (struct ip_set_req_get_features).
>   
Yeah, I got it Jozsef - I'll alter ip_set_req_get_features to use 
unsigned in instead of u8. Jesus!

Are you happy for the extra parameter called "features" to be included 
in the print function (iptables - userspace) as this is where I will 
store the features, not by using info->flags anymore?

>> Yep, just saw that too, so I'll just move the ip_set_feature enum just 
>> above the #ifdef __KERNEL__ part and I assume it would be picked up 
>> "automatically", is that right?
>>     
>
> No, let the patchset be complete in itself, i.e. include the patches both 
> for the kernel and iptables header files.
>   
Fair enough.

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/extensions/libxt_SET.c b/extensions/libxt_SET.c
index a11db39..967d905 100644
--- a/extensions/libxt_SET.c
+++ b/extensions/libxt_SET.c
@@ -176,7 +176,7 @@  parse_target(char **argv, int invert, struct xt_set_info *info,
 			      "setname `%s' too long, max %d characters.",
 			      optarg, IPSET_MAXNAMELEN - 1);
 
-	get_set_byname(optarg, info);
+	get_set_byname_with_features(optarg, info);
 	parse_dirs(argv[optind], info);
 	optind++;
 }
@@ -206,15 +206,20 @@  print_target(const char *prefix, const struct xt_set_info *info)
 {
 	int i;
 	char setname[IPSET_MAXNAMELEN];
+	char *ptr;
 
 	if (info->index == IPSET_INVALID_ID)
 		return;
 	get_set_byid(setname, info->index);
 	printf(" %s %s", prefix, setname);
 	for (i = 1; i <= info->dim; i++) {
+		if ((info->flags & IPSET_DIM_IFACE_INOUT) && i == IPSET_DIM_TWO)
+			ptr = (info->flags & (1 << i) ? "in" : "out");
+		else
+			ptr = (info->flags & (1 << i) ? "src" : "dst");
 		printf("%s%s",
 		       i == 1 ? " " : ",",
-		       info->flags & (1 << i) ? "src" : "dst");
+			ptr);
 	}
 }
 
diff --git a/extensions/libxt_SET.man b/extensions/libxt_SET.man
index 63eb383..747235f 100644
--- a/extensions/libxt_SET.man
+++ b/extensions/libxt_SET.man
@@ -1,25 +1,26 @@ 
-This modules adds and/or deletes entries from IP sets which can be defined 
+This module adds and/or deletes entries from IP sets which can be defined 
 by ipset(8).
 .TP
 \fB\-\-add\-set\fP \fIsetname\fP \fIflag\fP[\fB,\fP\fIflag\fP...]
-add the address(es)/port(s) of the packet to the sets
+add the address(es)/port(s) of the packet to the set
 .TP
 \fB\-\-del\-set\fP \fIsetname\fP \fIflag\fP[\fB,\fP\fIflag\fP...]
-delete the address(es)/port(s) of the packet from the sets
+delete the address(es)/port(s) of the packet from the set
 .IP
-where flags are
+where 'flag' above is comma separated list of
 .BR "src"
 and/or
 .BR "dst"
-specifications and there can be no more than six of them.
+with the exception of hash:ip,iface where in addition to the above flags, the following is also allowed for the 'iface' part of that set:
+.BR "in"
+or
+.BR "out" 
+corresponding to the incoming or outgoing network interface. The above flags cannot exceed six in total for a given set.
 .TP
 \fB\-\-timeout\fP \fIvalue\fP
-when adding entry, the timeout value to use instead of the default
-one from the set definition
+when adding an entry, the timeout value to use instead of the default one from the set definition.
 .TP
 \fB\-\-exist\fP
-when adding entry if it already exists, reset the timeout value
-to the specified one or to the default from the set definition
+when adding an entry, if such entry already exists, reset the timeout value to the specified one or to the default from the set definition.
 .PP
-Use of -j SET requires that ipset kernel support is provided, which, for
-standard kernels, is the case since Linux 2.6.39.
+Use of -j SET requires that ipset kernel support is provided, which, for standard kernels, is the case since Linux 2.6.39.
diff --git a/extensions/libxt_set.c b/extensions/libxt_set.c
index 77e3f07..dfc311a 100644
--- a/extensions/libxt_set.c
+++ b/extensions/libxt_set.c
@@ -60,7 +60,7 @@  set_parse_v0(int c, char **argv, int invert, unsigned int *flags,
 	case '2':
 		fprintf(stderr,
 			"--set option deprecated, please use --match-set\n");
-	case '1':		/* --match-set <set> <flag>[,<flag> */
+	case '1':		/* --match-set <set> <flag>[,<flag>] */
 		if (info->u.flags[0])
 			xtables_error(PARAMETER_PROBLEM,
 				      "--match-set can be specified only once");
@@ -140,7 +140,7 @@  set_parse_v1(int c, char **argv, int invert, unsigned int *flags,
 	case '2':
 		fprintf(stderr,
 			"--set option deprecated, please use --match-set\n");
-	case '1':		/* --match-set <set> <flag>[,<flag> */
+	case '1':		/* --match-set <set> <flag>[,<flag>] */
 		if (info->dim)
 			xtables_error(PARAMETER_PROBLEM,
 				      "--match-set can be specified only once");
@@ -158,9 +158,9 @@  set_parse_v1(int c, char **argv, int invert, unsigned int *flags,
 				      "setname `%s' too long, max %d characters.",
 				      optarg, IPSET_MAXNAMELEN - 1);
 
-		get_set_byname(optarg, info);
+		get_set_byname_with_features(optarg, info);
 		parse_dirs(argv[optind], info);
-		DEBUGP("parse: set index %u\n", info->index);
+		DEBUGP("parse: set index %u, set flags %u\n", info->index, info->flags);
 		optind++;
 		
 		*flags = 1;
@@ -175,6 +175,7 @@  print_match(const char *prefix, const struct xt_set_info *info)
 {
 	int i;
 	char setname[IPSET_MAXNAMELEN];
+	char *ptr;
 
 	get_set_byid(setname, info->index);
 	printf("%s %s %s",
@@ -182,9 +183,13 @@  print_match(const char *prefix, const struct xt_set_info *info)
 	       prefix,
 	       setname); 
 	for (i = 1; i <= info->dim; i++) {		
+		if ((info->flags & IPSET_DIM_IFACE_INOUT) && i == IPSET_DIM_TWO)
+			ptr = (info->flags & (1 << i) ? "in" : "out");
+		else
+			ptr = (info->flags & (1 << i) ? "src" : "dst");
 		printf("%s%s",
 		       i == 1 ? " " : ",",
-		       info->flags & (1 << i) ? "src" : "dst");
+		       ptr);
 	}
 }
 
diff --git a/extensions/libxt_set.h b/extensions/libxt_set.h
index 47c3f5b..d47e263 100644
--- a/extensions/libxt_set.h
+++ b/extensions/libxt_set.h
@@ -101,6 +101,39 @@  get_set_byname(const char *setname, struct xt_set_info *info)
 }
 
 static void
+get_set_byname_with_features(const char *setname, struct xt_set_info *info)
+{
+	struct ip_set_req_get_features req;
+	socklen_t size = sizeof(struct ip_set_req_get_features);
+	int res, sockfd;
+
+	sockfd = get_version(&req.version);
+	req.op = IP_SET_OP_GET_FEATURES;
+	strncpy(req.set.name, setname, IPSET_MAXNAMELEN);
+	req.set.name[IPSET_MAXNAMELEN - 1] = '\0';
+	res = getsockopt(sockfd, SOL_IP, SO_IP_SET, &req, &size);
+	close(sockfd);
+	if (res != 0 && errno== EBADMSG) { /* IP_SET_OP_GET_FEATURES not supported, revert back to IP_SET_OP_GET_BYNAME */
+		get_set_byname(setname, info);
+	} else {
+		if (res != 0)
+			xtables_error(OTHER_PROBLEM,
+				"Problem when communicating with ipset, errno=%d.\n",
+				errno);
+		if (size != sizeof(struct ip_set_req_get_features))
+			xtables_error(OTHER_PROBLEM,
+				"Incorrect return size from kernel during ipset lookup, "
+				"(want %zu, got %zu)\n",
+				sizeof(struct ip_set_req_get_features), (size_t)size);
+		if (req.set.index == IPSET_INVALID_ID)
+			xtables_error(PARAMETER_PROBLEM,
+				      "Set %s doesn't exist.\n", setname);
+		info->index = req.set.index;
+		info->flags |= req.features;
+	}
+}
+
+static void
 parse_dirs_v0(const char *opt_arg, struct xt_set_info_v0 *info)
 {
 	char *saved = strdup(opt_arg);
@@ -115,7 +148,7 @@  parse_dirs_v0(const char *opt_arg, struct xt_set_info_v0 *info)
 			info->u.flags[i++] |= IPSET_DST;
 		else
 			xtables_error(PARAMETER_PROBLEM,
-				"You must spefify (the comma separated list of) 'src' or 'dst'.");
+				"You must specify (comma separated list of) 'src' or 'dst'.");
 	}
 
 	if (tmp)
@@ -130,16 +163,31 @@  static void
 parse_dirs(const char *opt_arg, struct xt_set_info *info)
 {
 	char *saved = strdup(opt_arg);
+	__u8 saved_flags = info->flags;
 	char *ptr, *tmp = saved;
-	
+
+	/* clear everything else, but IPSET_INV_MATCH */
+	info->flags &= IPSET_INV_MATCH;
+
 	while (info->dim < IPSET_DIM_MAX && tmp != NULL) {
 		info->dim++;
 		ptr = strsep(&tmp, ",");
-		if (strncmp(ptr, "src", 3) == 0)
-			info->flags |= (1 << info->dim);
+		if (strncmp(ptr, "in", 2) == 0 && 	/* 'in' */
+			info->dim == IPSET_DIM_TWO && 	/* x,'in' */
+			saved_flags & IPSET_TYPE_IFACE)	/* hash:net,iface type set */
+				info->flags |= (1 << info->dim | IPSET_DIM_IFACE_INOUT);
+
+		else if (strncmp(ptr, "out", 3) == 0 && /* 'out' */
+			info->dim == IPSET_DIM_TWO && 	/* x,'out' */
+			saved_flags & IPSET_TYPE_IFACE)	/* hash:net,iface type set */
+				info->flags |= IPSET_DIM_IFACE_INOUT;
+
+		else if (strncmp(ptr, "src", 3) == 0)
+				info->flags |= (1 << info->dim);
+
 		else if (strncmp(ptr, "dst", 3) != 0)
 			xtables_error(PARAMETER_PROBLEM,
-				"You must spefify (the comma separated list of) 'src' or 'dst'.");
+				"You must specify (comma separated list of) 'src'|'dst' or 'in'|'out' just for the interface part of hash:net,iface set, if used.");
 	}
 
 	if (tmp)
diff --git a/extensions/libxt_set.man b/extensions/libxt_set.man
index 1ad9085..31be0eb 100644
--- a/extensions/libxt_set.man
+++ b/extensions/libxt_set.man
@@ -1,22 +1,20 @@ 
 This module matches IP sets which can be defined by ipset(8).
 .TP
 [\fB!\fP] \fB\-\-match\-set\fP \fIsetname\fP \fIflag\fP[\fB,\fP\fIflag\fP]...
-where flags are the comma separated list of
+where 'flag' above is comma separated list of
 .BR "src"
 and/or
 .BR "dst" 
-specifications and there can be no more than six of them. Hence the command
+with the exception of hash:ip,iface where, in addition to these two options, the following is also allowed for the 'iface' part:
+.BR "in" 
+or
+.BR "out" 
+corresponding to the incoming or outgoing network interface. The above options cannot exceed six in total for a given set. The command
 .IP
  iptables \-A FORWARD \-m set \-\-match\-set test src,dst
 .IP
-will match packets, for which (if the set type is ipportmap) the source
-address and destination port pair can be found in the specified set. If
-the set type of the specified set is single dimension (for example ipmap),
-then the command will match packets for which the source address can be
-found in the specified set. 
+will match packets for which, if the set is of type hash:ip,port for example, the source IP address and destination port pair can be found and matched successfully. If the specified set is one dimensional (i.e. bitmap:ip), then the command will match packets for which the source address can be found in the set specified.
 .PP
-The option \fB\-\-match\-set\fP can be replaced by \fB\-\-set\fP if that does 
-not clash with an option of other extensions.
+The option \fB\-\-match\-set\fP can be replaced by \fB\-\-set\fP if that does not clash with an option from other extensions.
 .PP
-Use of -m set requires that ipset kernel support is provided, which, for
-standard kernels, is the case since Linux 2.6.39.
+Use of -m set requires that ipset kernel support is provided, which, for standard kernels, is the case since Linux 2.6.39.
diff --git a/include/linux/netfilter/ipset/ip_set.h b/include/linux/netfilter/ipset/ip_set.h
index 79cb077..f2a038c 100644
--- a/include/linux/netfilter/ipset/ip_set.h
+++ b/include/linux/netfilter/ipset/ip_set.h
@@ -186,6 +186,10 @@  enum ip_set_dim {
 	 * If changed, new revision of iptables match/target is required.
 	 */
 	IPSET_DIM_MAX = 6,
+	/* 
+	 * Indicates whether the new 'iface' format (in/out) has been used.
+	 */
+	IPSET_DIM_IFACE = 7, 
 };
 
 /* Option flags for kernel operations */
@@ -194,8 +198,28 @@  enum ip_set_kopt {
 	IPSET_DIM_ONE_SRC = (1 << IPSET_DIM_ONE),
 	IPSET_DIM_TWO_SRC = (1 << IPSET_DIM_TWO),
 	IPSET_DIM_THREE_SRC = (1 << IPSET_DIM_THREE),
+	IPSET_DIM_IFACE_INOUT = (1 << IPSET_DIM_IFACE),
 };
 
+/* Set features */
+enum ip_set_feature {
+	IPSET_TYPE_IP_FLAG = 0,
+	IPSET_TYPE_IP = (1 << IPSET_TYPE_IP_FLAG),
+	IPSET_TYPE_PORT_FLAG = 1,
+	IPSET_TYPE_PORT = (1 << IPSET_TYPE_PORT_FLAG),
+	IPSET_TYPE_MAC_FLAG = 2,
+	IPSET_TYPE_MAC = (1 << IPSET_TYPE_MAC_FLAG),
+	IPSET_TYPE_IP2_FLAG = 3,
+	IPSET_TYPE_IP2 = (1 << IPSET_TYPE_IP2_FLAG),
+	IPSET_TYPE_NAME_FLAG = 4,
+	IPSET_TYPE_NAME = (1 << IPSET_TYPE_NAME_FLAG),
+	IPSET_TYPE_IFACE_FLAG = 5,
+	IPSET_TYPE_IFACE = (1 << IPSET_TYPE_IFACE_FLAG),
+	/* Strictly speaking not a feature, but a flag for dumping:
+	 * this settype must be dumped last */
+	IPSET_DUMP_LAST_FLAG = 7,
+	IPSET_DUMP_LAST = (1 << IPSET_DUMP_LAST_FLAG),
+};
 
 /* Interface to iptables/ip6tables */
 
@@ -216,6 +240,14 @@  struct ip_set_req_get_set {
 #define IP_SET_OP_GET_BYINDEX	0x00000007	/* Get set name by index */
 /* Uses ip_set_req_get_set */
 
+#define IP_SET_OP_GET_FEATURES	0x00000008	/* Get set features by name */
+struct ip_set_req_get_features {
+	unsigned int op;
+	unsigned int version;
+	__u8 features;
+	union ip_set_name_index set;
+};
+
 #define IP_SET_OP_VERSION	0x00000100	/* Ask kernel version */
 struct ip_set_req_version {
 	unsigned op;