On Tue, May 25, 2010 at 6:36 PM, Jan Engelhardt <jengelh@xxxxxxxxxx> 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@xxxxxxxxx)
--
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
