Skip to contentSkip to conversation

ctnetlink: Fix NAT info setting.

PATCH Applied Marcus Sundberg 3 participants 4 messages Dec 1, 2005 – Dec 4, 2005

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 ctnetlink: Fix NAT info setting.

Hi,

this patch fixes a latent bug with the NAT info setting in ctnetlink. (Currently I believe it's never triggered, as NAT info can not be
changed on existing conntracks)

//Marcus

Signature · 3 lines
---------------------------------------+--------------------------
  Marcus Sundberg <[email protected]>  | Firewalls with SIP & NAT
 Software Developer, Ingate Systems AB |  http://www.ingate.com/

[NETFILTER] ctnetlink: Fix NAT info setting.

ip_nat_initialized() takes enum ip_nat_manip_type as it's second argument, not a hook number. The current code was not only ugly, but also broken as IP_NAT_MANIP_SRC != NF_IP_POST_ROUTING.

net/ipv4/netfilter/ip_conntrack_netlink.c +9 -6
@@ -855,6 +855,7 @@ ctnetlink_change_status(struct ip_conntr
 		return -EINVAL;
 #else
 		unsigned int hooknum;
+		enum ip_nat_manip_type manip;
 		struct ip_nat_range range;
 
 		if (ctnetlink_parse_nat(cda, ct, &range) < 0)
@@ -867,17 +868,19 @@ ctnetlink_change_status(struct ip_conntr
 		/* This is tricky but it works. ip_nat_setup_info needs the
 		 * hook number as parameter, so let's do the correct 
 		 * conversion and run away */
-		if (status & IPS_SRC_NAT_DONE)
-			hooknum = NF_IP_POST_ROUTING; /* IP_NAT_MANIP_SRC */
-		else if (status & IPS_DST_NAT_DONE)
-			hooknum = NF_IP_PRE_ROUTING;  /* IP_NAT_MANIP_DST */
-		else 
+		if (status & IPS_SRC_NAT_DONE) {
+			hooknum = NF_IP_POST_ROUTING;
+			manip = IP_NAT_MANIP_SRC;
+		} else if (status & IPS_DST_NAT_DONE) {
+			hooknum = NF_IP_PRE_ROUTING;
+			manip = IP_NAT_MANIP_DST;
+		} else 
 			return -EINVAL; /* Missing NAT flags */
 
 		DEBUGP("NAT status: %lu\n", 
 		       status & (IPS_NAT_MASK | IPS_NAT_DONE_MASK));
 		
-		if (ip_nat_initialized(ct, hooknum))
+		if (ip_nat_initialized(ct, manip))
 			return -EEXIST;
 		ip_nat_setup_info(ct, &range, hooknum);
 
#1 lore raw
Quoted 5 lines · Marcus Sundberg

[NETFILTER] ctnetlink: Fix NAT info setting.

ip_nat_initialized() takes enum ip_nat_manip_type as it's second argument, not a hook number. The current code was not only ugly, but also broken as IP_NAT_MANIP_SRC != NF_IP_POST_ROUTING.

Thanks for the bugfix but I don't like it, it's a bit bloated. Harald, please submit the patch attached. It uses HOOK2MANIP macro instead.

Signature · 1 line
Pablo

[NETFILTER] ip_nat_initialized takes wrong parameter

ip_nat_initialized() takes enum ip_nat_manip_type as it's second argument, not a hook number.

net/ipv4/netfilter/ip_conntrack_netlink.c +1 -1
Index: netfilter-2.6.14.git/net/ipv4/netfilter/ip_conntrack_netlink.c
===================================================================
@@ -876,7 +876,7 @@ ctnetlink_change_status(struct ip_conntr
 		DEBUGP("NAT status: %lu\n", 
 		       status & (IPS_NAT_MASK | IPS_NAT_DONE_MASK));
 		
-		if (ip_nat_initialized(ct, hooknum))
+		if (ip_nat_initialized(ct, HOOK2MANIP(hooknum)))
 			return -EEXIST;
 		ip_nat_setup_info(ct, &range, hooknum);
 
#2 lore raw
Quoted 10 lines · Pablo Neira Ayuso
Quoted 5 lines · Marcus Sundberg

[NETFILTER] ctnetlink: Fix NAT info setting.

ip_nat_initialized() takes enum ip_nat_manip_type as it's second argument, not a hook number. The current code was not only ugly, but also broken as IP_NAT_MANIP_SRC != NF_IP_POST_ROUTING.

Thanks for the bugfix but I don't like it, it's a bit bloated. Harald, please submit the patch attached. It uses HOOK2MANIP macro instead.

Ah, I wasn't aware of that macro. I agree that's a nicer fix.

//Marcus

Signature · 3 lines
---------------------------------------+--------------------------
  Marcus Sundberg <[email protected]>  | Firewalls with SIP & NAT
 Software Developer, Ingate Systems AB |  http://www.ingate.com/
#3 lore raw
In reply to Pablo Neira Ayuso
Quoted 4 lines · Pablo Neira Ayuso

[NETFILTER] ip_nat_initialized takes wrong parameter

ip_nat_initialized() takes enum ip_nat_manip_type as it's second argument, not a hook number.

Applied, thanks Pablo and Marcus.