Re: xt_connlimit 20070707 kernel

[Patrick McHardy <kaber@trash.net>]

Jan Engelhardt wrote:
> On Jul 6 2007 15:05, Patrick McHardy wrote:
> 
>>>Ok so what should I put in for nhoff, dataoff, what_l3 and what_l4?
>>
>>
>>Check out nf_conntrack_in() and the IPv4/IPv6 prepare functions.
>>
> 
> 
> Does this look ok?


Yes, just two small things left:

> +	read_unlock_bh(&nf_conntrack_lock);
> +
> +	if (addit) {
> +		/* save the new connection in our list */
> +		conn = kzalloc(sizeof(*conn), GFP_ATOMIC);
> +		if (conn == NULL)
> +			return -ENOMEM;
> +
> +		INIT_LIST_HEAD(&conn->list);

No need to initialize member list_heads.

> +		conn->tuple = *tuple;
> +		list_add(&conn->list, hash);
> +		++matches;
> +	}
> +
> +	return matches;
> +}
> +
> +static bool connlimit_match(const struct sk_buff *skb,
> +			    const struct net_device *in,
> +			    const struct net_device *out,
> +			    const struct xt_match *match,
> +			    const void *matchinfo, int offset,
> +			    unsigned int protoff, bool *hotdrop)
> +{
> +	const struct xt_connlimit_info *info = matchinfo;
> +	const struct nf_conntrack_tuple *tuple_ptr;
> +	union nf_conntrack_address addr, mask;
> +	struct nf_conntrack_tuple tuple;
> +	enum ip_conntrack_info ctinfo;
> +	const struct nf_conn *ct;
> +	int connections;
> +
> +	ct = nf_ct_get(skb, &ctinfo);
> +	if (ct != NULL) {
> +		tuple_ptr = &ct->tuplehash[0].tuple;
> +	} else {
> +		struct nf_conntrack_l3proto *l3proto;
> +		struct nf_conntrack_l4proto *l4proto;
> +
> +		l3proto = nf_ct_l3proto_find_get(match->family);
> +		if (l3proto == NULL) {
> +			*hotdrop = true;
> +			return false;
> +		}
> +		l4proto = nf_ct_l4proto_find_get(match->family, match->proto);


The module reference taking functions should not be used in the
packet processing path. Please use __nf_ct_l3proto_find and
__nf_ct_l4proto_find. Since the l3proto is static for one
instance of the match you could also store it info->data and
only do the lookup once (for then you need to take the module
reference of course).

> +
> +	if (match->family == AF_INET6) {
> +		const struct ipv6hdr *iph = ipv6_hdr(skb);
> +		memcpy(&addr.ip6, &iph->saddr, sizeof(iph->saddr));
> +		memcpy(&mask.ip6, info->v6_mask, sizeof(info->v6_mask));
> +	} else {
> +		const struct iphdr *iph = ip_hdr(skb);
> +		addr.ip = iph->saddr;
> +		mask.ip = info->v4_mask;
> +	}
> +
> +	spin_lock_bh(&info->data->lock);
> +	connections = count_them(info->data, tuple_ptr, &addr, &mask, match);
> +	spin_unlock_bh(&info->data->lock);
> +
> +	if (connections < 0) {
> +		/* kmalloc failed, drop it entirely */
> +		*hotdrop = 1;


This should be "= true" I guess ..