[PATCH v4 0/2] ipset: add "inner" flag version support

[PATCH v4 0/2] ipset: add "inner" flag version support

[Dash Four]

This series of 2 patches supplements the previous version (v3) and adds
"inner" flag version support to all registered ipset types
(kernel + userspace).

Dash Four (2):
   ipset: add set match "inner" flag support
   ipset (userspace): add "inner" flag version support

  kernel/net/netfilter/ipset/ip_set_bitmap_ip.c      | 10 ++-
  kernel/net/netfilter/ipset/ip_set_bitmap_ipmac.c   | 10 ++-
  kernel/net/netfilter/ipset/ip_set_bitmap_port.c    |  4 +-
  kernel/net/netfilter/ipset/ip_set_hash_ip.c        | 13 +++-
  kernel/net/netfilter/ipset/ip_set_hash_ipport.c    | 19 +++--
  kernel/net/netfilter/ipset/ip_set_hash_ipportip.c  | 25 ++++---
  kernel/net/netfilter/ipset/ip_set_hash_ipportnet.c | 29 +++++---
  kernel/net/netfilter/ipset/ip_set_hash_net.c       | 13 +++-
  kernel/net/netfilter/ipset/ip_set_hash_netiface.c  | 13 +++-
  kernel/net/netfilter/ipset/ip_set_hash_netport.c   | 24 +++---
  lib/ipset_bitmap_ip.c                              | 45 ++++++++++++
  lib/ipset_bitmap_ipmac.c                           | 51 +++++++++++++
  lib/ipset_bitmap_port.c                            | 44 +++++++++++
  lib/ipset_hash_ip.c                                | 44 +++++++++++
  lib/ipset_hash_ipport.c                            | 63 ++++++++++++++++
  lib/ipset_hash_ipportip.c                          | 74 +++++++++++++++++++
  lib/ipset_hash_ipportnet.c                         | 84 +++++++++++++++++++++
  lib/ipset_hash_net.c                               | 49 +++++++++++++
  lib/ipset_hash_netiface.c                          | 64 ++++++++++++++++
  lib/ipset_hash_netport.c                           | 69 ++++++++++++++++++
  20 files changed, 694 insertions(+), 53 deletions(-)

Re: [PATCH v4 0/2] ipset: add "inner" flag version support

[Jozsef Kadlecsik]

On Fri, 5 Jul 2013, Dash Four wrote:

> This series of 2 patches supplements the previous version (v3) and adds
> "inner" flag version support to all registered ipset types
> (kernel + userspace).
> 
> Dash Four (2):
>   ipset: add set match "inner" flag support
>   ipset (userspace): add "inner" flag version support

All of your patches (including the previous series) are mangled and I'm 
unable to apply them. It seems there's an extra space at the beginning of 
every line starting with a whitespace character or something like that. 
Please check your patches and resubmit the fixed ones.

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 v4 0/2] ipset: add "inner" flag version support

[Jozsef Kadlecsik]

On Mon, 8 Jul 2013, Jozsef Kadlecsik wrote:

> On Fri, 5 Jul 2013, Dash Four wrote:
> 
> > This series of 2 patches supplements the previous version (v3) and adds
> > "inner" flag version support to all registered ipset types
> > (kernel + userspace).
> > 
> > Dash Four (2):
> >   ipset: add set match "inner" flag support
> >   ipset (userspace): add "inner" flag version support
> 
> All of your patches (including the previous series) are mangled and I'm 
> unable to apply them. It seems there's an extra space at the beginning of 
> every line starting with a whitespace character or something like that. 
> Please check your patches and resubmit the fixed ones.

Meanwhile, while looking at your code on how to extend it, I have noticed 
two issues:

- From ip_set_get_ip4_inner_hdr and ip_set_get_ip6_inner_hdr you pass back
  the pointer to a local buffer, which is not good. Please move the local
  buffer definitions into all of the functions which call *_inner_hdr.

- The second is an optimization issue: for two or more dimensional
  sets the *_inner_hdr functions are called multiple times instead of 
  once. *_inner_hdr functions should pass back the pointer to the IP
  header/buffer (and use simple ip[46]addrptr inline functions on that) 
  and set protooff for *get_port.

  So it should be something like this:

hash_ipportip4_kadt(...
	const struct hash_ipportip *h = set->data;
	ipset_adtfn adtfn = set->variant->adt[adt];
	struct hash_ipportip4_elem e = { };
	struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, h);
	struct iphdr _iph, *iph = &_iph;
	unsigned int protooff = ip_hdrlen(skb);

	if (!ip_set_get_ip4_inner_hdr(skb, &protooff, iph,
				opt->cmdflags & IPSET_FLAG_INNER) ||
	    !ip_set_get_ip4_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
				 &e.port, &e.proto, protooff))
		return -EINVAL

	ip4addrptr(iph, opt->flags & IPSET_DIM_ONE_SRC, &e.ip);
	ip4addrptr(iph, opt->flags & IPSET_DIM_THREE_SRC, &e.ip2);
	...

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 v4 0/2] ipset: add "inner" flag version support

[Dash Four]

Jozsef Kadlecsik wrote:
> On Mon, 8 Jul 2013, Jozsef Kadlecsik wrote:
>
>   
>> On Fri, 5 Jul 2013, Dash Four wrote:
>>
>>     
>>> This series of 2 patches supplements the previous version (v3) and adds
>>> "inner" flag version support to all registered ipset types
>>> (kernel + userspace).
>>>
>>> Dash Four (2):
>>>   ipset: add set match "inner" flag support
>>>   ipset (userspace): add "inner" flag version support
>>>       
>> All of your patches (including the previous series) are mangled and I'm 
>> unable to apply them. It seems there's an extra space at the beginning of 
>> every line starting with a whitespace character or something like that. 
>> Please check your patches and resubmit the fixed ones.
>>     
>
> Meanwhile, while looking at your code on how to extend it, I have noticed 
> two issues:
>
> - From ip_set_get_ip4_inner_hdr and ip_set_get_ip6_inner_hdr you pass back
>   the pointer to a local buffer, which is not good.
Why? By "local buffer" I take it you mean the ip header pointer, correct?

>  Please move the local
>   buffer definitions into all of the functions which call *_inner_hdr.
>
> - The second is an optimization issue: for two or more dimensional
>   sets the *_inner_hdr functions are called multiple times instead of 
>   once. *_inner_hdr functions should pass back the pointer to the IP
>   header/buffer (and use simple ip[46]addrptr inline functions on that) 
>   and set protooff for *get_port.
>
>   So it should be something like this:
>
> hash_ipportip4_kadt(...
> 	const struct hash_ipportip *h = set->data;
> 	ipset_adtfn adtfn = set->variant->adt[adt];
> 	struct hash_ipportip4_elem e = { };
> 	struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, h);
> 	struct iphdr _iph, *iph = &_iph;
> 	unsigned int protooff = ip_hdrlen(skb);
>
> 	if (!ip_set_get_ip4_inner_hdr(skb, &protooff, iph,
> 				opt->cmdflags & IPSET_FLAG_INNER) ||
> 	    !ip_set_get_ip4_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
> 				 &e.port, &e.proto, protooff))
> 		return -EINVAL
>
> 	ip4addrptr(iph, opt->flags & IPSET_DIM_ONE_SRC, &e.ip);
> 	ip4addrptr(iph, opt->flags & IPSET_DIM_THREE_SRC, &e.ip2);
> 	...
>   
I'll look into this in the next few days - just in case you find 
something else you are unhappy with as I have time constraints and 
cannot afford to be churning out patches on a daily basis.