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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
