Re: [PATCH 2/3][CTNETLINK] Atomically set/unset status bits

[Pablo Neira Ayuso <pablo@netfilter.org>]

[-- Attachment #1: Type: text/plain, Size: 434 bytes --]

Pablo Neira Ayuso wrote:
> This patch guarantees that status bits are atomically set/unset. A minor
> cleanup to save one extra useless line in the code is introduced.
> 
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Sorry again, wrong patch :(, attached the one appropiated.

-- 
The dawn of the fourth age of Linux firewalling is coming; a time of
great struggle and heroic deeds -- J.Kadlecsik got inspired by J.Morris

[-- Attachment #2: 02status-racy.patch --]
[-- Type: text/plain, Size: 2867 bytes --]

[CTNETLINK] Atomically set/unset status bits

This patch guarantees that status bits are atomically set/unset. A minor
cleanup to save one extra useless line in the code is introduced.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Index: linux-2.6.git/net/ipv4/netfilter/ip_conntrack_netlink.c
===================================================================
--- linux-2.6.git.orig/net/ipv4/netfilter/ip_conntrack_netlink.c	2006-11-28 16:39:37.000000000 +0100
+++ linux-2.6.git/net/ipv4/netfilter/ip_conntrack_netlink.c	2006-11-28 17:48:48.000000000 +0100
@@ -770,6 +770,7 @@ out:
 static inline int
 ctnetlink_change_status(struct ip_conntrack *ct, struct nfattr *cda[])
 {
+	int i;
 	unsigned long d;
 	unsigned status = ntohl(*(__be32 *)NFA_DATA(cda[CTA_STATUS-1]));
 	d = ct->status ^ status;
@@ -782,7 +783,6 @@ ctnetlink_change_status(struct ip_conntr
 		/* SEEN_REPLY bit can only be set */
 		return -EINVAL;
 
-	
 	if (d & IPS_ASSURED && !(status & IPS_ASSURED))
 		/* ASSURED bit can only be set */
 		return -EINVAL;
@@ -814,10 +814,16 @@ ctnetlink_change_status(struct ip_conntr
 #endif
 	}
 
-	/* Be careful here, modifying NAT bits can screw up things,
-	 * so don't let users modify them directly if they don't pass
-	 * ip_nat_range. */
-	ct->status |= status & ~(IPS_NAT_DONE_MASK | IPS_NAT_MASK);
+	d &= ~(IPS_NAT_DONE_MASK | IPS_NAT_MASK);
+
+	for (i = 0; i < sizeof(ct->status); i++)
+		if (d & (1 << i)) {
+			if (status & (1 << i))
+				set_bit(i, &ct->status);
+			else
+				clear_bit(i, &ct->status);
+		}
+
 	return 0;
 }
 
Index: linux-2.6.git/net/netfilter/nf_conntrack_netlink.c
===================================================================
--- linux-2.6.git.orig/net/netfilter/nf_conntrack_netlink.c	2006-11-28 16:52:13.000000000 +0100
+++ linux-2.6.git/net/netfilter/nf_conntrack_netlink.c	2006-11-28 17:48:56.000000000 +0100
@@ -779,6 +779,7 @@ out:
 static inline int
 ctnetlink_change_status(struct nf_conn *ct, struct nfattr *cda[])
 {
+	int i;
 	unsigned long d;
 	unsigned status = ntohl(*(u_int32_t *)NFA_DATA(cda[CTA_STATUS-1]));
 	d = ct->status ^ status;
@@ -791,7 +792,6 @@ ctnetlink_change_status(struct nf_conn *
 		/* SEEN_REPLY bit can only be set */
 		return -EINVAL;
 
-	
 	if (d & IPS_ASSURED && !(status & IPS_ASSURED))
 		/* ASSURED bit can only be set */
 		return -EINVAL;
@@ -823,10 +823,16 @@ ctnetlink_change_status(struct nf_conn *
 #endif
 	}
 
-	/* Be careful here, modifying NAT bits can screw up things,
-	 * so don't let users modify them directly if they don't pass
-	 * ip_nat_range. */
-	ct->status |= status & ~(IPS_NAT_DONE_MASK | IPS_NAT_MASK);
+	d &= ~(IPS_NAT_DONE_MASK | IPS_NAT_MASK);
+
+	for (i = 0; i < sizeof(ct->status); i++)
+		if (d & (1 << i)) {
+			if (status & (1 << i))
+				set_bit(i, &ct->status);
+			else
+				clear_bit(i, &ct->status);
+		}
+
 	return 0;
 }