Date:	Tue, 25 May 2010 19:26:50 +0800
From:	Changli Gao <xiaosuo@...il.com>
To:	Jan Engelhardt <jengelh@...ozas.de>
Cc:	Patrick McHardy <kaber@...sh.net>,
	"David S. Miller" <davem@...emloft.net>,
	Alexey Kuznetsov <kuznet@....inr.ac.ru>,
	James Morris <jmorris@...ei.org>,
	netfilter-devel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH RFC] netfilter: iptables target SYNPROXY

On Tue, May 25, 2010 at 6:36 PM, Jan Engelhardt <jengelh@...ozas.de> wrote:
>
> On Tuesday 2010-05-25 09:06, Changli Gao wrote:
>> net/ipv4/netfilter/Kconfig                  |   12
>> net/ipv4/netfilter/Makefile                 |    1
>> net/ipv4/netfilter/ipt_SYNPROXY.c           |  658 ++++++++++++++++++++++++++++
>> net/ipv4/syncookies.c                       |   21
>> net/ipv4/tcp_ipv4.c                         |    5
>> net/netfilter/nf_conntrack_core.c           |   44 +
>
> Please make this an Xtables extension.
> There is excellent documentation ("Writing Netfilter Modules" to google)
> on the details if needed.

Hmm. I don't know IPv6 well. So I leave it as an iptables extension,
and hope sb. comes on with IPv6 support after it gets merged.

>
>>--- a/include/net/netfilter/nf_conntrack_core.h
>>+++ b/include/net/netfilter/nf_conntrack_core.h
>>@@ -63,8 +63,21 @@ static inline int nf_conntrack_confirm(struct sk_buff *skb)
>>       if (ct && ct != &nf_conntrack_untracked) {
>>               if (!nf_ct_is_confirmed(ct) && !nf_ct_is_dying(ct))
>>                       ret = __nf_conntrack_confirm(skb);
>>-              if (likely(ret == NF_ACCEPT))
>>+              if (likely(ret == NF_ACCEPT)) {
>>+#if defined(CONFIG_IP_NF_TARGET_SYNPROXY) || \
>>+    defined(CONFIG_IP_NF_TARGET_SYNPROXY_MODULE)
>>+                      int (*syn_proxy)(struct sk_buff *, struct nf_conn *,
>>+                                       enum ip_conntrack_info);
>>+#endif
>>+
>>                       nf_ct_deliver_cached_events(ct);
>>+#if defined(CONFIG_IP_NF_TARGET_SYNPROXY) || \
>>+    defined(CONFIG_IP_NF_TARGET_SYNPROXY_MODULE)
>>+                      syn_proxy = rcu_dereference(syn_proxy_post_hook);
>>+                      if (syn_proxy)
>>+                              ret = syn_proxy(skb, ct, skb->nfctinfo);
>>+#endif
>>+              }
>>       }
>>       return ret;
>> }
>
> I'm sure this is possible with lesser macro intrusion - use a separate
> function. Same in nf_conntrack_core.c

I'm thinking about adding two helper functions into structure
nf_conntrack_l4proto. Then the l4 protocol sepcific code won't be in
nf_conntrack_core.c but in nf_conntrack_proto_tcp.c.

>
>>--- a/include/net/tcp.h
>>+++ b/include/net/tcp.h
>>@@ -460,8 +460,18 @@ extern int                        tcp_disconnect(struct sock *sk, int flags);
>> extern __u32 syncookie_secret[2][16-4+SHA_DIGEST_WORDS];
>> extern struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb,
>>                                   struct ip_options *opt);
>>-extern __u32 cookie_v4_init_sequence(struct sock *sk, struct sk_buff *skb,
>>-                                   __u16 *mss);
>>+extern __u32 __cookie_v4_init_sequence(__be32 saddr, __be32 daddr,
>>+                                     __be16 sport, __be16 dport, __u32 seq,
>>+                                     __u16 *mssp);
>>+static inline __u32 cookie_v4_init_sequence(const struct iphdr *iph,
>>+                                          const struct tcphdr *th,
>>+                                          __u16 *mssp)
>>+{
>>+      return __cookie_v4_init_sequence(iph->saddr, iph->daddr, th->source,
>>+                                       th->dest, ntohl(th->seq), mssp);
>>+}
>
> Since there is only one cookie_v4_init_sequence user AFAICS,
> moving the function there seems to make best sense.

OK. I wanted to keep cookie_v4_init_sequence() and
cookie_v4_check_sequence() symmetric.

>
>>+static int get_mss(u8 *data, int len)
>
> unsigned

I am afraid that I can't do that. As get_mss() may return a negative
value, if there is some invalid TCP option.

>
>>+      /* only support IPv4 now */
>
> Please fix :)

The same reasion as above.

>
>>+      th = skb_header_pointer(skb, iph->ihl * 4, sizeof(_th), &_th);
>>+      BUG_ON(th == NULL);
>
> I wouldn't call that a bug. I think that can happen on an evil TCP tinygram
> (with proper IPV4 header).

I copied the code from file nf_conntrack_proto_tcp.c. In fact, BUG_ON
isn't useful at all, as tcp_error is called before this function
is called.

>
>>+static int syn_proxy_mangle_pkt(struct sk_buff *skb, struct iphdr *iph,
>>+                              struct tcphdr *th, u32 seq_diff)
>>+{
>>+      __be32 new;
>>+      int olen;
>>+
>>+      if (skb->len < (iph->ihl + th->doff) * 4)
>>+              return NF_DROP;
>
> Verdicts are unsigned too. (Also in other functions.)

OK. Thanks.

>
>>+static struct xt_target synproxy_tg_reg __read_mostly = {
>>+      .name           = "SYNPROXY",
>>+      .family         = NFPROTO_IPV4,
>>+      .target         = synproxy_tg,
>>+      .table          = "raw",
>>+      .hooks          = (1 << NF_INET_PRE_ROUTING),
>
> No need for (),

Yea. Thanks.

>
>>+      .proto          = IPPROTO_TCP,
>>+      .me             = THIS_MODULE,
>>+};
>>+
>>+static int __init synproxy_tg_init(void)
>>+{
>>+      int err, cpu;
>>+
>>+      for_each_possible_cpu(cpu)
>>+              per_cpu(syn_proxy_state, cpu).seq_inited = 0;
>
> Static data starts out zeroed. That also applies to percpu data, doesn't it?
>

Yea, Eric has mentioned that.

-- 
Regards，
Changli Gao(xiaosuo@...il.com)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists