ip_conntrack_alter_reply doesn't need to loop
Subsystems (2)
PATCH 2/5 ip_conntrack_alter_reply doesn't need to loop
Name: ip_conntrack_alter_reply doesn't need to loop
Author: Rusty Russell
Status: Booted on 2.6.0-test5-bk1
ip_conntrack_alter_reply checks that the reply isn't already taken, but there's little point, since there's *still* a race after it is called (which we handle at confirm time anyway).
net/ipv4/netfilter/ip_nat_core.c +16 -29
@@ -475,38 +475,25 @@ }#endif -do { -if (!get_unique_tuple (&new_tuple, &orig_tp, range, conntrack, - hooknum)) { -DEBUGP ("ip_nat_setup_info: Can't get unique for %p.\n" , - conntrack); -return NF_DROP ; - } - -#if 0 -DEBUGP ("Hook %u (%s) %p\n" , hooknum, -HOOK2MANIP (hooknum)==IP_NAT_MANIP_SRC ?"SRC" :"DST" , +if (!get_unique_tuple (&new_tuple, &orig_tp, range,conntrack,hooknum)) { +DEBUGP ("ip_nat_setup_info: Can't get unique for %p.\n" , conntrack); -DEBUGP ("Original: " ); -DUMP_TUPLE (&orig_tp); -DEBUGP ("New: " ); -DUMP_TUPLE (&new_tuple); -#endif +return NF_DROP ; + } + +/* We now have two tuples (SRCIP/SRCPT/DSTIP/DSTPT): +the original (A/B/C/D') and the mangled one (E/F/G/H'). -/* We now have two tuples (SRCIP/SRCPT/DSTIP/DSTPT): -the original (A/B/C/D') and the mangled one (E/F/G/H'). +We're only allowed to work with the SRC per-proto +part, so we create inverses of both to start, then +derive the other fields we need. */ + +/* Reply connection: simply invert the new tuple +(G/H/E/F') */ +invert_tuplepr (&reply, &new_tuple); -We're only allowed to work with the SRC per-proto -part, so we create inverses of both to start, then -derive the other fields we need. */ - -/* Reply connection: simply invert the new tuple -(G/H/E/F') */ -invert_tuplepr (&reply, &new_tuple); - -/* Alter conntrack table so it recognizes replies. -If fail this race (reply tuple now used), repeat. */ - }while (!ip_conntrack_alter_reply (conntrack, &reply)); +/* Alter conntrack table so will recognize replies. */ +ip_conntrack_alter_reply (conntrack, &reply);/* FIXME: We can simply used existing conntrack reply tuple here --RR */
include/linux/netfilter_ipv4/ip_conntrack.h +2 -3
@@ -224,9 +224,8 @@/* get master conntrack via master expectation */ #define master_ct (conntr) (conntr->master ? conntr->master->expectant : NULL) -/* Alter reply tuple (maybe alter helper). If it's already taken, -return 0 and don't do alteration. */ -extern int +/* Alter reply tuple (maybe alter helper). */ +extern void ip_conntrack_alter_reply (struct ip_conntrack *conntrack,const struct ip_conntrack_tuple *newreply);
net/ipv4/netfilter/ip_conntrack_core.c +4 -10
@@ -1013,16 +1013,12 @@return ret; } -/* Alter reply tuple (maybe alter helper). If it's already taken, -return 0 and don't do alteration. */ -int ip_conntrack_alter_reply (struct ip_conntrack *conntrack, -const struct ip_conntrack_tuple *newreply) +/* Alter reply tuple (maybe alter helper). This is for NAT, and is +implicitly racy: see __ip_conntrack_confirm */ +void ip_conntrack_alter_reply (struct ip_conntrack *conntrack, +const struct ip_conntrack_tuple *newreply) {WRITE_LOCK (&ip_conntrack_lock); -if (__ip_conntrack_find (newreply, conntrack)) { -WRITE_UNLOCK (&ip_conntrack_lock); -return 0 ; - }/* Should be unconfirmed, so not in hash table yet */ IP_NF_ASSERT (!is_confirmed (conntrack));
@@ -1033,8 +1029,6 @@if (!conntrack->master &&list_empty (&conntrack->sibling_list)) conntrack->helper =ip_ct_find_helper (newreply);WRITE_UNLOCK (&ip_conntrack_lock); - -return 1 ; }int ip_conntrack_helper_register (struct ip_conntrack_helper *me)
Signature · 1 line
A bad analogy is like a leaky screwdriver -- Richard Braakman