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

[Dash Four <mr.dash.four@googlemail.com>]

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.