nf_conntrack_netlink port take#2
Subsystems (2)
RFC PATCH 1/1 nf_conntrack_netlink port take#2
Hi,
Sorry for the bad timing. Here attached the take#2 of the nf_conntrack_netlink port. It includes a layer-3 abstraction as suggested. Attached two patches:
- required modifications for nf_conntrack (modifs-take2.patch)
- a diff against ip_conntrack_netlink to make easy the review (x).
The whole nf_conntrack_netlink file is available at:
http://people.netfilter.org/pablo/nf_conntrack_netlink.c
Cheers.
Signature · 1 line
Pablo
include/net/netfilter/nf_conntrack.h +31 -0
@@ -94,6 +94,9 @@ struct nf_conn/* Current number of expected connections */ unsigned int expecting; +/* Unique ID that identifies this conntrack*/ +unsigned int id; +/* Helper. if any */ struct nf_conntrack_helper *helper;
@@ -140,6 +143,9 @@ struct nf_conntrack_expect/* Usage count. */ atomic_t use; +/* Unique ID */ +unsigned int id; +/* Flags */ unsigned int flags;
@@ -190,6 +196,31 @@ static inline void nf_ct_put(struct nf_cnf_conntrack_put (&ct->ct_general); } +extern struct nf_conntrack_tuple_hash * +__nf_conntrack_find (const struct nf_conntrack_tuple *tuple, +const struct nf_conn *ignored_conntrack); + +extern void nf_conntrack_hash_insert (struct nf_conn *ct); + +extern struct nf_conntrack_expect * +__nf_conntrack_expect_find (const struct nf_conntrack_tuple *tuple); + +extern struct nf_conntrack_expect * +nf_conntrack_expect_find (const struct nf_conntrack_tuple *tuple); + +extern void nf_ct_unlink_expect (struct nf_conntrack_expect *exp); + +extern void nf_ct_remove_expectations (struct nf_conn *ct); + +extern void nf_conntrack_flush (void ); + +extern struct nf_conntrack_helper * +nf_ct_helper_find_get (const struct nf_conntrack_tuple *tuple); +extern void nf_ct_helper_put (struct nf_conntrack_helper *helper); + +extern struct nf_conntrack_helper * +__nf_conntrack_helper_find_byname (const char *name); +/* call to create an explicit dependency on nf_conntrack. */ extern void need_nf_conntrack (void );
include/net/netfilter/nf_conntrack_helper.h +2 -0
@@ -33,6 +33,8 @@ struct nf_conntrack_helperunsigned int protoff, struct nf_conn *ct, enum ip_conntrack_info conntrackinfo); + +int (*to_nfattr)(struct sk_buff *skb,const struct nf_conn *ct); };extern int nf_conntrack_helper_register (struct nf_conntrack_helper *);
include/net/netfilter/nf_conntrack_l3proto.h +14 -1
@@ -14,6 +14,8 @@#include <linux/seq_file.h> #include <net/netfilter/nf_conntrack.h> +struct nfattr ; +struct nf_conntrack_l3proto {/* Next pointer. */
@@ -70,6 +72,12 @@ struct nf_conntrack_l3protou_int32_t (*get_features)(const struct nf_conntrack_tuple *tuple); +int (*tuple_to_nfattr)(struct sk_buff *skb, +const struct nf_conntrack_tuple *t); + +int (*nfattr_to_tuple)(struct nfattr *tb[], +struct nf_conntrack_tuple *t); +/* Module (if any) which this is connected to. */ struct module *me; };
@@ -81,11 +89,16 @@ extern int nf_conntrack_l3proto_registerextern void nf_conntrack_l3proto_unregister (struct nf_conntrack_l3proto *proto);static inline struct nf_conntrack_l3proto * -nf_ct_find_l3proto (u_int16_t l3proto) +__nf_ct_l3proto_find (u_int16_t l3proto) {return nf_ct_l3protos[l3proto]; } +extern struct nf_conntrack_l3proto * +nf_ct_l3proto_find_get (u_int16_t l3proto); + +extern void nf_ct_l3proto_put (struct nf_conntrack_l3proto *p); +/* Existing built-in protocols */ extern struct nf_conntrack_l3proto nf_conntrack_l3proto_ipv4;extern struct nf_conntrack_l3proto nf_conntrack_l3proto_ipv6;
include/net/netfilter/nf_conntrack_protocol.h +25 -1
@@ -12,6 +12,7 @@#include <net/netfilter/nf_conntrack.h> struct seq_file ; +struct nfattr ;struct nf_conntrack_protocol {
@@ -66,6 +67,18 @@ struct nf_conntrack_protocolenum ip_conntrack_info *ctinfo, int pf, unsigned int hooknum); +/* convert protoinfo to nfnetink attributes */ +int (*to_nfattr)(struct sk_buff *skb,struct nfattr *nfa, +const struct nf_conn *ct); + +/* convert nfnetlink attributes to protoinfo */ +int (*from_nfattr)(struct nfattr *tb[],struct nf_conn *ct); + +int (*tuple_to_nfattr)(struct sk_buff *skb, +const struct nf_conntrack_tuple *t); +int (*nfattr_to_tuple)(struct nfattr *tb[], +struct nf_conntrack_tuple *t); +/* Module (if any) which this is connected to. */ struct module *me; };
@@ -80,12 +93,23 @@ extern struct nf_conntrack_protocol nf_cextern struct nf_conntrack_protocol **nf_ct_protos[PF_MAX ];extern struct nf_conntrack_protocol * -nf_ct_find_proto (u_int16_t l3proto,u_int8_t protocol); +__nf_ct_proto_find (u_int16_t l3proto,u_int8_t protocol); + +extern struct nf_conntrack_protocol * +nf_ct_proto_find_get (u_int16_t l3proto,u_int8_t protocol); + +extern void nf_ct_proto_put (struct nf_conntrack_protocol *p);/* Protocol registration. */ extern int nf_conntrack_protocol_register (struct nf_conntrack_protocol *proto);extern void nf_conntrack_protocol_unregister (struct nf_conntrack_protocol *proto); +/* Generic netlink helpers */ +extern int nf_ct_port_tuple_to_nfattr (struct sk_buff *skb, +const struct nf_conntrack_tuple *tuple); +extern int nf_ct_port_nfattr_to_tuple (struct nfattr *tb[], +struct nf_conntrack_tuple *t); +/* Log invalid packets */ extern unsigned int nf_ct_log_invalid;
include/net/netfilter/nf_conntrack_tuple.h +0 -0
net/ipv4/netfilter/ip_conntrack_netlink.c +0 -0
net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c +39 -0
@@ -392,6 +392,40 @@ getorigdst(struct sock *sk, int optval,return -ENOENT ; } +#if defined(CONFIG_NF_CT_NETLINK ) || \ + defined(CONFIG_NF_CT_NETLINK_MODULE ) + +#include <linux/netfilter/nfnetlink.h> +#include <linux/netfilter/nfnetlink_conntrack.h> + +static int ipv4_tuple_to_nfattr (struct sk_buff *skb, +const struct nf_conntrack_tuple *tuple) +{ +NFA_PUT (skb,CTA_IP_V4_SRC ,sizeof (u_int32_t), + &tuple->src.u3.ip); +NFA_PUT (skb,CTA_IP_V4_DST ,sizeof (u_int32_t), + &tuple->dst.u3.ip); +return 0 ; + +nfattr_failure: +return -1 ; +} + +static int ipv4_nfattr_to_tuple (struct nfattr *tb[], +struct nf_conntrack_tuple *t) +{ +if (!tb[CTA_IP_V4_SRC -1 ] || !tb[CTA_IP_V4_DST -1 ]) +return -EINVAL ; + + t->src.u3.ip = + *(u_int32_t *)NFA_DATA (tb[CTA_IP_V4_SRC -1 ]); + t->dst.u3.ip = + *(u_int32_t *)NFA_DATA (tb[CTA_IP_V4_DST -1 ]); + +return 0 ; +} +#endif +static struct nf_sockopt_ops so_getorigdst = { .pf =PF_INET , .get_optmin =SO_ORIGINAL_DST ,
@@ -408,6 +442,11 @@ struct nf_conntrack_l3proto nf_conntrack .print_conntrack = ipv4_print_conntrack, .prepare = ipv4_prepare, .get_features = ipv4_get_features, +#if defined(CONFIG_NF_CT_NETLINK ) || \ + defined(CONFIG_NF_CT_NETLINK_MODULE ) + .tuple_to_nfattr = ipv4_tuple_to_nfattr, + .nfattr_to_tuple = ipv4_nfattr_to_tuple, +#endif .me = THIS_MODULE, };
net/ipv4/netfilter/nf_conntrack_proto_icmp.c +58 -8
94 lines not on this page · raw message
net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c +39 -0
@@ -401,6 +401,40 @@ static ctl_table nf_ct_net_table[] = { };#endif +#if defined(CONFIG_NF_CT_NETLINK ) || \ + defined(CONFIG_NF_CT_NETLINK_MODULE ) + +#include <linux/netfilter/nfnetlink.h> +#include <linux/netfilter/nfnetlink_conntrack.h> + +static int ipv6_tuple_to_nfattr (struct sk_buff *skb, +const struct nf_conntrack_tuple *tuple) +{ +NFA_PUT (skb,CTA_IP_V6_SRC ,sizeof (u_int32_t) *4 , + &tuple->src.u3.ip6); +NFA_PUT (skb,CTA_IP_V6_DST ,sizeof (u_int32_t) *4 , + &tuple->dst.u3.ip6); +return 0 ; + +nfattr_failure: +return -1 ; +} + +static int ipv6_nfattr_to_tuple (struct nfattr *tb[], +struct nf_conntrack_tuple *t) +{ +if (!tb[CTA_IP_V6_SRC -1 ] || !tb[CTA_IP_V6_DST -1 ]) +return -EINVAL ; + +memcpy (t->src.u3.ip6,NFA_DATA (tb[CTA_IP_V6_SRC -1 ]), +sizeof (u_int32_t) *4 ); +memcpy (t->dst.u3.ip6,NFA_DATA (tb[CTA_IP_V6_DST -1 ]), +sizeof (u_int32_t) *4 ); + +return 0 ; +} +#endif +struct nf_conntrack_l3proto nf_conntrack_l3proto_ipv6 = { .l3proto =PF_INET6 , .name ="ipv6" ,
@@ -410,6 +444,11 @@ struct nf_conntrack_l3proto nf_conntrack .print_conntrack = ipv6_print_conntrack, .prepare = ipv6_prepare, .get_features = ipv6_get_features, +#if defined(CONFIG_NF_CT_NETLINK ) || \ + defined(CONFIG_NF_CT_NETLINK_MODULE ) + .tuple_to_nfattr = ipv6_tuple_to_nfattr, + .nfattr_to_tuple = ipv6_nfattr_to_tuple, +#endif .me = THIS_MODULE, };
net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c +1 -1
@@ -185,7 +185,7 @@ icmpv6_error_message(struct sk_buff *skbreturn -NF_ACCEPT ; } - inproto =nf_ct_find_proto (PF_INET6 , inprotonum); + inproto =__nf_ct_proto_find (PF_INET6 , inprotonum);/* Are they talking about one of our connections? */ if (!nf_ct_get_tuple(skb, inip6off, inprotoff, PF_INET6, inprotonum,
net/netfilter/Kconfig +7 -0
@@ -95,4 +95,11 @@ config NF_CONNTRACK_FTP To compile it as a module, choose M here. If unsure, say N. +config NF_CT_NETLINK + tristate 'Connection tracking netlink interface' + depends on NF_CONNTRACK && NETFILTER_NETLINK + depends on NF_CONNTRACK!=y || NETFILTER_NETLINK!=m + help + This option enables support for a netlink-based userspace interface + endmenu
net/netfilter/Makefile +3 -0
@@ -13,3 +13,6 @@ obj-$(CONFIG_NF_CONNTRACK_FTP) += nf_con # SCTP protocol connection tracking obj-$(CONFIG_NF_CT_PROTO_SCTP) += nf_conntrack_proto_sctp.o + +# netlink interface for nf_conntrack +obj-$(CONFIG_NF_CT_NETLINK) += nf_conntrack_netlink.o
net/netfilter/nf_conntrack_core.c +207 -28
394 lines not on this page · raw message
net/netfilter/nf_conntrack_proto_tcp.c +70 -0
88 lines not on this page · raw message
net/netfilter/nf_conntrack_proto_udp.c +10 -0
@@ -196,6 +196,11 @@ struct nf_conntrack_protocol nf_conntrac .packet = udp_packet, .new = udp_new, .error = udp_error4, +#if defined(CONFIG_NF_CT_NETLINK ) || \ + defined(CONFIG_NF_CT_NETLINK_MODULE ) + .tuple_to_nfattr = nf_ct_port_tuple_to_nfattr, + .nfattr_to_tuple = nf_ct_port_nfattr_to_tuple, +#endif }; struct nf_conntrack_protocol nf_conntrack_protocol_udp6 =
@@ -210,6 +215,11 @@ struct nf_conntrack_protocol nf_conntrac .packet = udp_packet, .new = udp_new, .error = udp_error6, +#if defined(CONFIG_NF_CT_NETLINK ) || \ + defined(CONFIG_NF_CT_NETLINK_MODULE ) + .tuple_to_nfattr = nf_ct_port_tuple_to_nfattr, + .nfattr_to_tuple = nf_ct_port_nfattr_to_tuple, +#endif };EXPORT_SYMBOL (nf_conntrack_protocol_udp4);
net/netfilter/nf_conntrack_standalone.c +29 -10
@@ -161,14 +161,14 @@ static int ct_seq_show(struct seq_file *if (NF_CT_DIRECTION (hash))return 0 ; - l3proto =nf_ct_find_l3proto (conntrack->tuplehash[IP_CT_DIR_ORIGINAL ] - .tuple.src.l3num); + l3proto =__nf_ct_l3proto_find (conntrack->tuplehash[IP_CT_DIR_ORIGINAL ] + .tuple.src.l3num);NF_CT_ASSERT (l3proto); - proto =nf_ct_find_proto (conntrack->tuplehash[IP_CT_DIR_ORIGINAL ] - .tuple.src.l3num, - conntrack->tuplehash[IP_CT_DIR_ORIGINAL ] - .tuple.dst.protonum); + proto =__nf_ct_proto_find (conntrack->tuplehash[IP_CT_DIR_ORIGINAL ] + .tuple.src.l3num, + conntrack->tuplehash[IP_CT_DIR_ORIGINAL ] + .tuple.dst.protonum);NF_CT_ASSERT (proto); if (seq_printf(s, "%-8s %u %-8s %u %ld ",
@@ -307,9 +307,9 @@ static int exp_seq_show(struct seq_file expect->tuple.src.l3num, expect->tuple.dst.protonum);print_tuple (s, &expect->tuple, -nf_ct_find_l3proto (expect->tuple.src.l3num), -nf_ct_find_proto (expect->tuple.src.l3num, - expect->tuple.dst.protonum)); +__nf_ct_l3proto_find (expect->tuple.src.l3num), +__nf_ct_proto_find (expect->tuple.src.l3num, + expect->tuple.dst.protonum));return seq_putc (s,'\n' ); }
@@ -847,7 +847,11 @@ EXPORT_SYMBOL(nf_conntrack_helper_unregiEXPORT_SYMBOL (nf_ct_iterate_cleanup);EXPORT_SYMBOL (__nf_ct_refresh_acct);EXPORT_SYMBOL (nf_ct_protos); -EXPORT_SYMBOL (nf_ct_find_proto); +EXPORT_SYMBOL (__nf_ct_proto_find); +EXPORT_SYMBOL (nf_ct_proto_find_get); +EXPORT_SYMBOL (nf_ct_proto_put); +EXPORT_SYMBOL (nf_ct_l3proto_find_get); +EXPORT_SYMBOL (nf_ct_l3proto_put);EXPORT_SYMBOL (nf_ct_l3protos);EXPORT_SYMBOL (nf_conntrack_expect_alloc);EXPORT_SYMBOL (nf_conntrack_expect_put);
@@ -867,3 +871,18 @@ EXPORT_SYMBOL(nf_ct_get_tuple);EXPORT_SYMBOL (nf_ct_invert_tuple);EXPORT_SYMBOL (nf_conntrack_in);EXPORT_SYMBOL (__nf_conntrack_attach); +EXPORT_SYMBOL (nf_conntrack_alloc); +EXPORT_SYMBOL (nf_conntrack_free); +EXPORT_SYMBOL (nf_conntrack_flush); +EXPORT_SYMBOL (nf_ct_remove_expectations); +EXPORT_SYMBOL (nf_ct_helper_find_get); +EXPORT_SYMBOL (nf_ct_helper_put); +EXPORT_SYMBOL (__nf_conntrack_helper_find_byname); +EXPORT_SYMBOL (__nf_conntrack_find); +EXPORT_SYMBOL (nf_ct_unlink_expect); +EXPORT_SYMBOL (nf_conntrack_hash_insert); +EXPORT_SYMBOL (__nf_conntrack_expect_find); +EXPORT_SYMBOL (nf_conntrack_expect_find); +EXPORT_SYMBOL (nf_conntrack_expect_list); +EXPORT_SYMBOL (nf_ct_port_tuple_to_nfattr); +EXPORT_SYMBOL (nf_ct_port_nfattr_to_tuple);
net/netfilter/nf_conntrack_netlink.c renamed from net/ipv4/netfilter/ip_conntrack_netlink.c +217 -194
1,095 lines not on this page · raw message
Quoted 5 lines · Pablo Neira
Hi,
Sorry for the bad timing. Here attached the take#2 of the
nf_conntrack_netlinkport. It includes a layer-3 abstraction as suggested. Attached two patches:
ok, I didn't have the time for a detailed review (and chances are undlikely that I will have anytime soon). However, if noone opposes, I would like to submit this for 2.6.16 quite soon. But not too soon (dave has a open net-2.6.16 tree, but we don't want to flood him with incremental fixes, at least not intentionally). So I'd say Pablo keeps the patch update out-of-tree until 2.6.15 is released, and at that time we push it to mainline immediately.
Signature · 5 lines
- Harald Welte <[email protected]> http://netfilter.org/ ============================================================================ "Fragmentation is like classful addressing -- an interesting early architectural error that shows how much experimentation was going on while IP was being designed." -- Paul Vixie
Quoted 6 lines · Harald Welte
Quoted 4 lines · Pablo Neira
Sorry for the bad timing. Here attached the take#2 of the
nf_conntrack_netlinkport. It includes a layer-3 abstraction as suggested. Attached two patches:
Quoted 7 lines
ok, I didn't have the time for a detailed review (and chances are undlikely that I will have anytime soon). However, if noone opposes, I would like to submit this for 2.6.16 quite soon. But not too soon (dave has a open net-2.6.16 tree, but we don't want to flood him with incremental fixes, at least not intentionally). So I'd say Pablo keeps the patch update out-of-tree until 2.6.15 is released, and at that time we push it to mainline immediately.
Fine with it. I'll keep such patch updated at my people.netfilter.org place.
Signature · 1 line
Pablo
Hi, Pablo,
From: Pablo Neira <[email protected]>
Date: Tue, 22 Nov 2005 20:22:38 +0100
Quoted 6 lines
Hi,
Sorry for the bad timing. Here attached the take#2 of the
nf_conntrack_netlinkport. It includes a layer-3 abstraction as suggested. Attached two patches:
Quoted 6 lines
- required modifications for
nf_conntrack(modifs-take2.patch)
- a diff againstip_conntrack_netlinkto make easy the review (x).The whole
nf_conntrack_netlinkfile is available at:
As far as I read roughly, it looks good. I'll read this deeply and write a patch for ICMPv6 this weekend.
Regards,
-- Yasuyuki Kozakai
[Sorry for long message]
Hi, Pablo,
I've succeeded to run 'conntrack -L' with your patch and I could see informations about IPv4 connections!
And I have some comments for fix or cleanup. Some of them can be said about ip_conntrack_netlink, too. Then I marked [nfct] or [both] on all comments.
[nfct] means 'comment on only nf_conntrack_netlink'. [both] means 'comment on both of {ip,nf}_conntrack_netlink'.
include/net/netfilter/nf_conntrack_protocol.h extern struct nf_conntrack_protocol nf_c · line 96
Quoted 3 lines
extern struct
nf_conntrack_protocol*include/net/netfilter/nf_conntrack_protocol.h +1
-1-nf_ct_find_proto(u_int16_t l3proto, u_int8_t protocol); +__nf_ct_proto_find(u_int16_t l3proto, u_int8_t protocol);
[nfct] Thanks for these cleanup :)
net/ipv4/netfilter/nf_conntrack_proto_icmp.c checksum_skipped: · line 302
Quoted 13 lines
net/ipv4/netfilter/nf_conntrack_proto_icmp.c +13
-0+static int icmp_tuple_to_nfattr (struct sk_buff *skb, +const struct nf_conntrack_tuple *t) +{ +NFA_PUT (skb,CTA_PROTO_ICMP_ID ,sizeof (u_int16_t), + &t->src.u.icmp.id); +NFA_PUT (skb,CTA_PROTO_ICMP_TYPE ,sizeof (u_int8_t), + &t->dst.u.icmp.type); +NFA_PUT (skb,CTA_PROTO_ICMP_CODE ,sizeof (u_int8_t), + &t->dst.u.icmp.code); + +if (t->dst.u.icmp.type >=sizeof (valid_new) + || !valid_new[t->dst.u.icmp.type]) +return -EINVAL ;
[both] Why this check isn't done before putting attributes ?
Quoted 7 lines
net/ipv4/netfilter/nf_conntrack_proto_icmp.c +7
-0+static int icmp_nfattr_to_tuple (struct nfattr *tb[], +struct nf_conntrack_tuple *tuple) +{ +if (!tb[CTA_PROTO_ICMP_TYPE -1 ] + || !tb[CTA_PROTO_ICMP_CODE -1 ] + || !tb[CTA_PROTO_ICMP_ID -1 ]) +return -1 ;
net/ipv4/netfilter/nf_conntrack_proto_icmp.c checksum_skipped: · line 326
Quoted 10 lines
net/ipv4/netfilter/nf_conntrack_proto_icmp.c +10
-0+ + tuple->dst.u.icmp.type = + *(u_int8_t *)NFA_DATA (tb[CTA_PROTO_ICMP_TYPE -1 ]); + tuple->dst.u.icmp.code = + *(u_int8_t *)NFA_DATA (tb[CTA_PROTO_ICMP_CODE -1 ]); + tuple->src.u.icmp.id = + *(u_int16_t *)NFA_DATA (tb[CTA_PROTO_ICMP_ID -1 ]); + +return 0 ; +}
[both] I think that it's necessary to check with valid_new[], to avoid to create the connection which nf_conntrack cannot track.
net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c static ctl_table nf_ct_net_table[] = { · line 432
Quoted 10 lines
net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c +10
-0+static int ipv6_nfattr_to_tuple (struct nfattr *tb[], +struct nf_conntrack_tuple *t) +{ +if (!tb[CTA_IP_V6_SRC -1 ] || !tb[CTA_IP_V6_DST -1 ]) +return -EINVAL ; + +memcpy (t->src.u3.ip6,NFA_DATA (tb[CTA_IP_V6_SRC -1 ]), +sizeof (u_int32_t) *4 ); +memcpy (t->dst.u3.ip6,NFA_DATA (tb[CTA_IP_V6_DST -1 ]), +sizeof (u_int32_t) *4 );
[nfct] cosmetic: Here you use t->src.u3.ip6, but you use &t->src.u3.ip6 in ipv6_tuple_to_nfattr().
net/netfilter/nf_conntrack_core.c static inline int helper_cmp(const struc · line 816
Quoted 7 lines
net/netfilter/nf_conntrack_core.c +7
-0+struct nf_conntrack_helper * +nf_ct_helper_find_get (const struct nf_conntrack_tuple *tuple) +{ +struct nf_conntrack_helper *helper; + +/* need ip_conntrack_lock to assure that helper exists until +* try_module_get() is called */
[nfct] cosmetic: Please sed s/ip_conntrack_lock/nf_conntrack_lock/.
Quoted 9 lines
net/netfilter/nf_conntrack_core.c +1
-3-void nf_conntrack_cleanup (void ) +void nf_conntrack_flush () { -int i; -/* This makes sure all current packets have passed through netfilter framework. Roll on, two-stage module delete... */
net/netfilter/nf_conntrack_core.c void nf_conntrack_cleanup(void) · line 1556
Quoted 7 lines
net/netfilter/nf_conntrack_core.c +4
-0@@ -1383,6 +1550,18 @@ void nf_conntrack_cleanup(void)schedule (); goto i_see_dead_people; } +/* wait until all references to ip_conntrack_untracked are dropped */ +while (atomic_read (&nf_conntrack_untracked.ct_general.use) >1 ) +schedule (); +}
[nfct] nice catch! I think this would better to be applied to mainline ASAP. Could you re-send this in other patch ?
OK, next is nf_conntrack_netlink.c
Quoted 13 lines
static inline int
ctnetlink_dump_tuples(structsk_buff*skb,
const structnf_conntrack_tuple*tuple){ struct nfattr *nest_parms; struct nf_conntrack_l3proto *l3proto; int ret = 0; l3proto = nf_ct_l3proto_find_get(tuple->src.l3num); nest_parms = NFA_NEST(skb, CTA_TUPLE_IP); if (likely(l3proto->tuple_to_nfattr)) ret = l3proto->tuple_to_nfattr(skb, tuple);
[nfct] if this function failed, can this stop dumping and return -1 ?
Quoted 6 lines
NFA_NEST_END(skb, nest_parms); nf_ct_l3proto_put(l3proto); nest_parms = NFA_NEST(skb, CTA_TUPLE_PROTO); ctnetlink_dump_tuples_proto(skb, tuple);
[both] Is there any reason that return value has to be ignored ?
Quoted 10 lines
static inline int
ctnetlink_dump_helpinfo(struct sk_buff *skb, const struct nf_conn *ct){ struct nfattr *nest_helper; if (!ct->helper) return 0; nest_helper = NFA_NEST(skb, CTA_HELP); NFA_PUT(skb, CTA_HELP_NAME, CTA_HELP_MAXNAMESIZE, &ct->helper->name);
[both] Oh, &ct->helper->name doesn't point intended area. Because ct->helper->name isn't declared as array.
[both] And its size may be less than CTA_HELP_MAXNAMESIZE.
Quoted 4 lines
static inline int
ctnetlink_dump_use(struct sk_buff *skb, const struct nf_conn *ct){ unsigned int use = htonl(atomic_read(&ct->ct_general.use));
[both] cosmetic(?): Why 'unsigned int' is used here ? other functions seems to use 'u_int32_t'.
Quoted 3 lines
static int ctnetlink_conntrack_event(struct
notifier_block*this,
unsigned long events, void *ptr)
{
<snip>
Quoted 25 lines
if (events & IPCT_DESTROY) {
type = IPCTNL_MSG_CT_DELETE;
group = NFNLGRP_CONNTRACK_DESTROY;
goto alloc_skb;
}
if (events & (IPCT_NEW | IPCT_RELATED)) {
type = IPCTNL_MSG_CT_NEW;
flags = NLM_F_CREATE|NLM_F_EXCL;
/* dump everything */
events = ~0UL;
group = NFNLGRP_CONNTRACK_NEW;
goto alloc_skb;
}
if (events & (IPCT_STATUS |
IPCT_PROTOINFO |
IPCT_HELPER |
IPCT_HELPINFO |
IPCT_NATINFO)) {
type = IPCTNL_MSG_CT_NEW;
group = NFNLGRP_CONNTRACK_UPDATE;
goto alloc_skb;
}
return NOTIFY_DONE;Quoted 1 line
alloc_skb:
[both] cleanup: Why don't we use 'else if ... else' instead of 'goto' ?
Quoted 4 lines
static const size_t cta_min_ip[CTA_IP_MAX] = {
[CTA_IP_V4_SRC-1] = sizeof(u_int32_t),
[CTA_IP_V4_DST-1] = sizeof(u_int32_t),
};[nfct] Can CTA_IP_V6_{SRC/DST} join them ? :)
[both] cleanup: Or, it might be better that this size check goes to l3proto->nfattr_to_tuple(), to generalize more.
Quoted 14 lines
static inline int ctnetlink_parse_tuple_proto(struct nfattr *attr,
structnf_conntrack_tuple*tuple){ struct nfattr *tb[CTA_PROTO_MAX]; struct nf_conntrack_protocol *proto; int ret = 0; DEBUGP("entered %s\n", __FUNCTION__); nfattr_parse_nested(tb, CTA_PROTO_MAX, attr); if (nfattr_bad_size(tb, CTA_PROTO_MAX, cta_min_proto)) return -EINVAL;
[both] cleanup: again, it might be better that this goes to proto->nfattr_to_tuple().
Quoted 13 lines
static int ctnetlink_parse_nat_proto(struct nfattr *attr,
const structnf_conn*ct, structip_nat_range*range){ struct nfattr *tb[CTA_PROTONAT_MAX]; struct ip_nat_protocol *npt; DEBUGP("entered %s\n", __FUNCTION__); nfattr_parse_nested(tb, CTA_PROTONAT_MAX, attr); if (nfattr_bad_size(tb, CTA_PROTONAT_MAX, cta_min_protonat)) return -EINVAL;
[both] cleanup: I think this can go to ip_nat_port_nfattr_to_range(), too.
Quoted 5 lines
static int
ctnetlink_create_conntrack(struct nfattr *cda[],
structnf_conntrack_tuple*otuple, structnf_conntrack_tuple*rtuple)
{
<snip>
Quoted 8 lines
ct->helper = nf_ct_helper_find_get(rtuple); add_timer(&ct->timeout); nf_conntrack_hash_insert(ct); if (ct->helper) nf_ct_helper_put(ct->helper);
Quoted 4 lines
#if defined(CONFIG_IP_NF_CONNTRACK_MARK) if (cda[CTA_MARK-1]) ct->mark = ntohl(*(u_int32_t *)NFA_DATA(cda[CTA_MARK-1]));#endif
[both] this should go before nf_conntrack_hash_insert(). Here is not protected with nf_conntrack_lock.
Quoted 6 lines
static int
ctnetlink_exp_fill_info(structsk_buff*skb, u32 pid, u32 seq,
int event,
int nowait,
const structnf_conntrack_expect*exp)
{
<snip>
Quoted 5 lines
nlh = NLMSG_PUT(skb, pid, seq, event, sizeof(struct nfgenmsg)); nfmsg = NLMSG_DATA(nlh); nlh->nlmsg_flags = (nowait && pid) ? NLM_F_MULTI : 0; nfmsg->nfgen_family = AF_INET;
[nfct] exp->tuple.src.l3num ?
Quoted 3 lines
static int ctnetlink_expect_event(struct
notifier_block*this,
unsigned long events, void *ptr)
{
<snip>
Quoted 6 lines
type |= NFNL_SUBSYS_CTNETLINK << 8; nlh = NLMSG_PUT(skb, 0, 0, type, sizeof(struct nfgenmsg)); nfmsg = NLMSG_DATA(nlh); nlh->nlmsg_flags = flags; nfmsg->nfgen_family = AF_INET;
[nfct] again, exp->tuple.src.l3num ?
Quoted 8 lines
nfmsg->version = NFNETLINK_V0; nfmsg->res_id = 0; if (ctnetlink_exp_dump_expect(skb, exp) < 0) goto nfattr_failure; nlh->nlmsg_len = skb->tail - b; proto = exp->tuple.dst.protonum;
[both] cleanup: proto is set only here, but it isn't used.
-- Yasuyuki Kozakai
Quoted 8 lines · Yasuyuki KOZAKAI
Quoted 8 lines
net/ipv4/netfilter/nf_conntrack_proto_icmp.c +8
-0+static int icmp_tuple_to_nfattr (struct sk_buff *skb, +const struct nf_conntrack_tuple *t) +{ +NFA_PUT (skb,CTA_PROTO_ICMP_ID ,sizeof (u_int16_t), + &t->src.u.icmp.id); +NFA_PUT (skb,CTA_PROTO_ICMP_TYPE ,sizeof (u_int8_t), + &t->dst.u.icmp.type); + NFA_PUT(skb, CTA_PROTO_ICMP_CODE, sizeof(u_int8_t),
Quoted 8 lines
net/ipv4/netfilter/nf_conntrack_proto_icmp.c checksum_skipped: · line 302
Quoted 5 lines
net/ipv4/netfilter/nf_conntrack_proto_icmp.c +5
-0+ &t->dst.u.icmp.code); + +if (t->dst.u.icmp.type >=sizeof (valid_new) + || !valid_new[t->dst.u.icmp.type]) +return -EINVAL ;[both] Why this check isn't done before putting attributes ?
Why is this check done at all? We're just dumping the tuples the kernel has already created and validated.
Quoted 12 lines
Quoted 12 lines
net/netfilter/nf_conntrack_core.c +1
-1-void nf_conntrack_cleanup (void ) +void nf_conntrack_flush (){
net/netfilter/nf_conntrack_core.c +0
-2-int i; -/* This makes sure all current packets have passed through netfilter framework. Roll on, two-stage module delete... */net/netfilter/nf_conntrack_core.c +0
-0@@ -1383,6 +1550,18 @@ void nf_conntrack_cleanup(void)schedule (); goto i_see_dead_people; }
Quoted 8 lines
net/netfilter/nf_conntrack_core.c void nf_conntrack_cleanup(void) · line 1556
Quoted 4 lines
net/netfilter/nf_conntrack_core.c +4
-0+/* wait until all references to ip_conntrack_untracked are dropped */ +while (atomic_read (&nf_conntrack_untracked.ct_general.use) >1 ) +schedule (); +}[nfct] nice catch! I think this would better to be applied to mainline ASAP. Could you re-send this in other patch ?
I agree to the fix, but disagree about abusing this function for ctnetlink. ctnetlink doesn't care about references and doesn't need to wait for untracked conntracks, it just needs to clean the table. I have a patch here for ctnetlink that replaces the use of this function by ip_ct_iterate_cleanup, which significantly speeds up flushing the table on a busy (conntrack-wise) system.
Quoted 8 lines
Quoted 8 lines
static inline int
ctnetlink_dump_helpinfo(struct sk_buff *skb, const struct nf_conn *ct){ struct nfattr *nest_helper; if (!ct->helper) return 0;
Quoted 8 lines
Quoted 2 lines
nest_helper = NFA_NEST(skb, CTA_HELP); NFA_PUT(skb, CTA_HELP_NAME, CTA_HELP_MAXNAMESIZE, &ct->helper->name);[both] Oh,
&ct->helper->namedoesn't point intended area. Becausect->helper->nameisn't declared as array.[both] And its size may be less than
CTA_HELP_MAXNAMESIZE.
And also larger. I don't think we need to limit the length at all, but we should make sure to stop at the trailing \0 byte.
Quoted 11 lines · Yasuyuki KOZAKAI
Review void nf_conntrack_cleanup(void) · line 1556
Quoted 8 lines
(quoted diff) +4
-0@@ -1383,6 +1550,18 @@ void nf_conntrack_cleanup(void) schedule(); goto i_see_dead_people; } + /* wait until all references to ip_conntrack_untracked are dropped */ + while (atomic_read(&nf_conntrack_untracked.ct_general.use) > 1) + schedule(); +}[nfct] nice catch! I think this would better to be applied to mainline ASAP.
Agreed. I've added this patch to my tree.
[NETFILTER]: Wait for untracked references in nf_conntrack module unload
Noticed by Pablo Neira <[email protected]>.
---
commit 96c75906027f008ed3a4058a606938901e9c6d99
tree 87b8bb22d35f61383fbb5f4ee6164a16cc256bb3
parent aea6c7fa24378c7f38026177c0b524719789b4be
author Patrick McHardy <[email protected]> Sun, 04 Dec 2005 17:22:02 +0100 committer Patrick McHardy <[email protected]> Sun, 04 Dec 2005 17:22:02 +0100
net/netfilter/nf_conntrack_core.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
net/netfilter/nf_conntrack_core.c +3 -0
@@ -1383,6 +1383,9 @@ void nf_conntrack_cleanup(void)schedule (); goto i_see_dead_people; } +/* wait until all references to nf_conntrack_untracked are dropped */ +while (atomic_read (&nf_conntrack_untracked.ct_general.use) >1 ) +break ;for (i =0 ; i <NF_CT_F_NUM ; i++) {if (nf_ct_cache[i].use ==0 )