Re: [PATCH] netfilter: don't track ICMPv6 negotiation message.

[Christoph Paasch <christoph.paasch@gmail.com>]

Hi,

thanks for your reply.
On Sun February 8 2009, Eric Leblond wrote:
> Hi,
>
> Christoph Paasch a écrit :
> > Why do you set skb->nfctinfo = IP_CT_NEW?
> > Because in xt_state.c, at state_mt(...) :
> > 	if it is in front of an untracked packet (using nf_ct_is_untracked(skb))
> > it automatically sets the statebit to UNTRACKED and so the IP_CT_NEW
> > isn't used.
>
> Not much to say on that point. I wanted to be homogeneous with what is
> done in xt_NOTRACK.c.
OK.

But then I'm asking me, why does the UNTRACKED state isn't documented in the 
iptables manpage, even if this one exists for the iptables state module? As 
those ICMPv6 messages will be recognized as UNTRACKED by xt_state.c, and so 
the firewall administrator needs to allow those kind of packets.

But well, this is more an issue of the iptables manpage, ...

>
> > Why do you return NF_ACCEPT and not -NF_ACCEPT?
> > By returning a positiv value, the packet will continue it's way through
> > the connection tracker.
>
> If I understand well, icmpv6_error will be called in nf_conntrack_core.c
> as l4proto->error :
>
> 	if (l4proto->error != NULL) {
> 		ret = l4proto->error(net, skb, dataoff, &ctinfo, pf, hooknum);
> 		if (ret <= 0) {
> 			NF_CT_STAT_INC_ATOMIC(net, error);
> 			NF_CT_STAT_INC_ATOMIC(net, invalid);
> 			return -ret;
> 		}
> 	}
>
> It will thus increment error counters if return is -NF_ACCEPT. As the
> packets we deal with are not error I don't think it is correct to return
> -NF_ACCEPT.
>
> But I agree with the fact, that returning NF_ACCEPT leads to some
> useless work inside the kernel.

The packet will then go up to the call to icmpv6_new (as the connection 
tracker will still look for a connection matching the tuple):

	if (type < 0 || type >= sizeof(valid_new) || !valid_new[type]) {
		/* Can't create a new ICMPv6 `conn' with this. */
		pr_debug("icmpv6: can't create new conn with type %u\n",
			 type + 128);
		nf_ct_dump_tuple_ipv6(&ct->tuplehash[0].tuple);
		return false;
	}

And then the invalid counter will get incremented in nf_conntrack_in(...):

	ct = resolve_normal_ct(net, skb, dataoff, pf, protonum,
			       l3proto, l4proto, &set_reply, &ctinfo);
	if (!ct) {
		/* Not valid part of a connection */
		NF_CT_STAT_INC_ATOMIC(net, invalid);
		return NF_ACCEPT;
	}

So, maybe the handling of the ICMPv6 negotiation messages should get moved to 
icmpv6_new, with a more descriptive pr_debug message than the one who is 
present.

--
Christoph Paasch

www.rollerbulls.be
--
--
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