Re: [PATCH] DHCPv6 connection tracker helper

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Mon, Feb 13, 2012 at 10:55:58AM +0100, Jan Engelhardt wrote:
> On Friday 2012-02-10 03:30, Darren Willis wrote:
> 
> >+MODULE_ALIAS("ipv6_conntrack_dhcpv6");
> 
> I see no use for this one.
> 
> >+static int dhcpv6_help(struct sk_buff *skb,
> >+		       unsigned int protoff,
> >+		       struct nf_conn *ct,
> >+		       enum ip_conntrack_info ctinfo)
> >+{
> >+	struct nf_conntrack_expect *exp;
> >+	struct iphdr *iph = ip_hdr(skb);
> >+	if (iph->version == 6) {
> >+		struct ipv6hdr *ip6h = ipv6_hdr(skb);
> >+		if (skb->sk == NULL)
> >+			goto out;
> >+		if (ipv6_addr_is_multicast(&ip6h->daddr)) {
> >+			exp = nf_ct_expect_alloc(ct);
> >+			if (exp == NULL)
> >+				goto out;
> >+			exp->tuple = ct->tuplehash[IP_CT_DIR_REPLY].tuple;
> 
> This reeks of http://stackoverflow.com/a/57611/1091587 :)
> And an ip(v4)hdr is not very fruitful in the realm of IPv6.
> How about getting rid of the goto as well, as in
> 
> 	struct nf_conntrack_expect *exp;
> 	const struct ipv6hdr *iph = ipv6_hdr(skb);
> 	if (iph->version == 6 || skb->sk == NULL ||
> 	    !ipv6_addr_is_multicast(&iph->daddr))
> 		return NF_ACCEPT;
> 	exp = nf_ct_expect_alloc(ct);
> 	if (exp == NULL)
> 		return NF_ACCEPT;
> 	exp->tuple = ...
> 	...
> 	return NF_ACCEPT;
> 
> 
> 
> Something for further separate patches on top:
> 
> You probably want to add a test for the skb actually being a
> DHCP message type that desires a reply (DHCPOFFER, etc.)
> before creating the exp.
> 
> >+	nf_ct_refresh(ct, skb, timeout * HZ);
> >	return NF_ACCEPT;
> >+}
> 
> In all cases, NF_ACCEPT is returned - makes some eyebrows raise.
> Upon looking for it, nf_conntrack_ftp.c returns NF_DROP on exp==NULL, so 
> should you.
> It is probably a sane thing to also check that the DHCP packet is 
> looking valid (packet size, flag combinations, etc.)
> 
> Thirdly, your module does not seem to make any attempt (yet) to handle 
> the DHCP v6 RECONFIGURE message.
> 
> The timeout value for the exp could depend on the message type; RFC 
> 3315 defines different timeouts for different message types, up to 600 
> seconds for RENEW/REBOUND. (Is 600 seconds a wise thing?)

I agree with Jan's review.