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

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

[Pablo Neira Ayuso]

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

Sorry you received this email twice

--

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>

-- 
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: 02fixstatus.patch --]
[-- Type: text/plain, Size: 1679 bytes --]

[CTNETLINK] Check for status flags existence on conntrack creation

Check that status flags are available in the netlink message received 
to create a new conntrack.

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-08 23:54:28.000000000 +0100
+++ linux-2.6.git/net/ipv4/netfilter/ip_conntrack_netlink.c	2006-11-08 23:54:55.000000000 +0100
@@ -945,9 +945,11 @@ ctnetlink_create_conntrack(struct nfattr
 	ct->timeout.expires = jiffies + ct->timeout.expires * HZ;
 	ct->status |= IPS_CONFIRMED;
 
-	err = ctnetlink_change_status(ct, cda);
-	if (err < 0)
-		goto err;
+	if (cda[CTA_STATUS-1]) {
+		err = ctnetlink_change_status(ct, cda);
+		if (err < 0)
+			goto err;
+	}
 
 	if (cda[CTA_PROTOINFO-1]) {
 		err = ctnetlink_change_protoinfo(ct, cda);
Index: linux-2.6.git/net/netfilter/nf_conntrack_netlink.c
===================================================================
--- linux-2.6.git.orig/net/netfilter/nf_conntrack_netlink.c	2006-11-08 23:55:06.000000000 +0100
+++ linux-2.6.git/net/netfilter/nf_conntrack_netlink.c	2006-11-08 23:55:49.000000000 +0100
@@ -961,9 +961,11 @@ ctnetlink_create_conntrack(struct nfattr
 	ct->timeout.expires = jiffies + ct->timeout.expires * HZ;
 	ct->status |= IPS_CONFIRMED;
 
-	err = ctnetlink_change_status(ct, cda);
-	if (err < 0)
-		goto err;
+	if (cda[CTA_STATUS-1]) {
+		err = ctnetlink_change_status(ct, cda);
+		if (err < 0)
+			goto err;
+	}
 
 	if (cda[CTA_PROTOINFO-1]) {
 		err = ctnetlink_change_protoinfo(ct, cda);

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

[Pablo Neira Ayuso]

[-- 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;
 }
 

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

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> [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.
> 
> -	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;

We already hold the lock, what is the purpose of this change?

It also changes the API, so far bits can only be set. I wonder
where this comes from, I'm pretty sure my original code allowed
to unset specific bits and the checks at the top of that function
indicate that unsetting is possible as well.

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

[Pablo Neira Ayuso]

Patrick McHardy wrote:
> We already hold the lock, what is the purpose of this change?
>
> It also changes the API, so far bits can only be set. I wonder
> where this comes from, I'm pretty sure my original code allowed
> to unset specific bits and the checks at the top of that function
> indicate that unsetting is possible as well.

Just a minor digest on this issue:

Leyend:
U = unchangeable
S = can be set
C = can be cleared

IPS_EXPECTED            U
IPS_SEEN_REPLY          S
IPS_ASSURED             S
IPS_CONFIRMED           U
IPS_NAT_MASK            U
IPS_SEQ_ADJUST          U
IPS_NAT_DONE_MASK       U
IPS_DYING               U
IPS_FIXED_TIMEOUT       S,C
IPS_PICKUP              S,C
IPS_IN_WINDOW           S,C

You are right, the lock is enough for most of them, actually my concern
is the new IPS_PICKUP that can be cleared by the TCP tracking code once
the pickup happens, such code is called outside the lock in proto->packet().

BTW, I just noticed that there are not checkings for SEQ_ADJUST, should
we add one?

-- 
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

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

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> Patrick McHardy wrote:
> 
>>We already hold the lock, what is the purpose of this change?
>>
>>It also changes the API, so far bits can only be set. I wonder
>>where this comes from, I'm pretty sure my original code allowed
>>to unset specific bits and the checks at the top of that function
>>indicate that unsetting is possible as well.
> 
> 
> Just a minor digest on this issue:
> 
> Leyend:
> U = unchangeable
> S = can be set
> C = can be cleared

I presume you mean theoretically - currently we can't clear anything.

> 
> IPS_EXPECTED            U
> IPS_SEEN_REPLY          S
> IPS_ASSURED             S
> IPS_CONFIRMED           U
> IPS_NAT_MASK            U
> IPS_SEQ_ADJUST          U
> IPS_NAT_DONE_MASK       U
> IPS_DYING               U
> IPS_FIXED_TIMEOUT       S,C
> IPS_PICKUP              S,C
> IPS_IN_WINDOW           S,C
> 
> You are right, the lock is enough for most of them, actually my concern
> is the new IPS_PICKUP that can be cleared by the TCP tracking code once
> the pickup happens, such code is called outside the lock in proto->packet().

Shouldn't that be fixed by taking the lock there instead? The iteration
over all 64 bits is quite inefficient compared to the simple assignment
we have currently.

> BTW, I just noticed that there are not checkings for SEQ_ADJUST, should
> we add one?

We do need to be able to set the SEQ_ADJUST bit, don't we? And the
related offsets of course.

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

[Pablo Neira Ayuso]

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

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>

[-- Attachment #2: 02fixstatus.patch --]
[-- Type: text/plain, Size: 1678 bytes --]

[CTNETLINK] Check for status flags existence on conntrack creation

Check that status flags are available in the netlink message received 
to create a new conntrack.

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-08 23:54:28.000000000 +0100
+++ linux-2.6.git/net/ipv4/netfilter/ip_conntrack_netlink.c	2006-11-08 23:54:55.000000000 +0100
@@ -945,9 +945,11 @@ ctnetlink_create_conntrack(struct nfattr
 	ct->timeout.expires = jiffies + ct->timeout.expires * HZ;
 	ct->status |= IPS_CONFIRMED;
 
-	err = ctnetlink_change_status(ct, cda);
-	if (err < 0)
-		goto err;
+	if (cda[CTA_STATUS-1]) {
+		err = ctnetlink_change_status(ct, cda);
+		if (err < 0)
+			goto err;
+	}
 
 	if (cda[CTA_PROTOINFO-1]) {
 		err = ctnetlink_change_protoinfo(ct, cda);
Index: linux-2.6.git/net/netfilter/nf_conntrack_netlink.c
===================================================================
--- linux-2.6.git.orig/net/netfilter/nf_conntrack_netlink.c	2006-11-08 23:55:06.000000000 +0100
+++ linux-2.6.git/net/netfilter/nf_conntrack_netlink.c	2006-11-08 23:55:49.000000000 +0100
@@ -961,9 +961,11 @@ ctnetlink_create_conntrack(struct nfattr
 	ct->timeout.expires = jiffies + ct->timeout.expires * HZ;
 	ct->status |= IPS_CONFIRMED;
 
-	err = ctnetlink_change_status(ct, cda);
-	if (err < 0)
-		goto err;
+	if (cda[CTA_STATUS-1]) {
+		err = ctnetlink_change_status(ct, cda);
+		if (err < 0)
+			goto err;
+	}
 
 	if (cda[CTA_PROTOINFO-1]) {
 		err = ctnetlink_change_protoinfo(ct, cda);