Concurrency issues with the iptables userspace program andexitcodes

Concurrency issues with the iptables userspace program andexitcodes

[Jesper Dangaard Brouer]

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

Hi

I have experienced some concurrency issues with the iptables
userspace program.  Executing an iptables (write) command while
another (write command) is running causes a failure of the command.
Well, this is the expected semantics of a non-blocking call, but the
issue is how the error situation is reported back.

The iptables command detects the situation and reports:
 "iptables: Resource temporarily unavailable"

With shell exitcode "1".  What annoys me is that the shell exitcode
is "1", which is also used for "normal" errors.  This means that my
code/scripts needs to parse output from stderr to distinguish it from
normal/expected errors.

I propose that the exitcode of "Resource temporarily unavailable"
error is changed to something unique for this situation.

Comments?

-- 
Med venlig hilsen / Best regards
  Jesper Brouer
  ComX Networks A/S
  Linux Network developer
  Cand. Scient Datalog / MSc.
  Author of http://adsl-optimizer.dk


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Concurrency issues with the iptables userspace program andexitcodes

[Patrick McHardy]

Jesper Dangaard Brouer wrote:
> I have experienced some concurrency issues with the iptables
> userspace program.  Executing an iptables (write) command while
> another (write command) is running causes a failure of the command.
> Well, this is the expected semantics of a non-blocking call, but the
> issue is how the error situation is reported back.
> 
> The iptables command detects the situation and reports:
>  "iptables: Resource temporarily unavailable"
> 
> With shell exitcode "1".  What annoys me is that the shell exitcode
> is "1", which is also used for "normal" errors.  This means that my
> code/scripts needs to parse output from stderr to distinguish it from
> normal/expected errors.
> 
> I propose that the exitcode of "Resource temporarily unavailable"
> error is changed to something unique for this situation.


Wouldn't it make more sense to just make sure you don't have iptables
commands running concurrently?

Re: Concurrency issues with the iptables userspace program andexitcodes

[Jesper Dangaard Brouer]

On Thu, 1 Jun 2006, Patrick McHardy wrote:

> Jesper Dangaard Brouer wrote:
>> I have experienced some concurrency issues with the iptables
>> userspace program.  Executing an iptables (write) command while
>> another (write command) is running causes a failure of the command.
>> Well, this is the expected semantics of a non-blocking call, but the
>> issue is how the error situation is reported back.
>>
>> The iptables command detects the situation and reports:
>>  "iptables: Resource temporarily unavailable"
>>
>> With shell exitcode "1".  What annoys me is that the shell exitcode
>> is "1", which is also used for "normal" errors.  This means that my
>> code/scripts needs to parse output from stderr to distinguish it from
>> normal/expected errors.
>>
>> I propose that the exitcode of "Resource temporarily unavailable"
>> error is changed to something unique for this situation.
>
>
> Wouldn't it make more sense to just make sure you don't have iptables
> commands running concurrently?

I already have implemented (f)locks in my code around the iptables 
invocations.  But the problem can still arise when root executes the 
iptables command from the shell. Thus, I still need to handle the 
situation in my code, and a proper exitcode would be nice.

In iptables-standalone.c it would be very easy to simply return the 
errno instead of !res (which always will return 0 or 1).  Would that be a 
feasable solution?

Hilsen
   Jesper Brouer

--
-------------------------------------------------------------------
MSc. Master of Computer Science
Dept. of Computer Science, University of Copenhagen
Author of http://www.adsl-optimizer.dk
-------------------------------------------------------------------

Someting like:

Index: iptables-standalone.c
===================================================================
--- iptables-standalone.c       (revision 6624)
+++ iptables-standalone.c       (working copy)
@@ -64,9 +64,11 @@
         if (ret)
                 ret = iptc_commit(&handle);

-       if (!ret)
+       if (!ret) {
                 fprintf(stderr, "iptables: %s\n",
                         iptc_strerror(errno));
+               exit(errno);
+       }

         exit(!ret);
  }

Re: Concurrency issues with the iptables userspace program andexitcodes

[Patrick McHardy]

Jesper Dangaard Brouer wrote:
> 
> On Thu, 1 Jun 2006, Patrick McHardy wrote:
> 
>> Wouldn't it make more sense to just make sure you don't have iptables
>> commands running concurrently?
> 
> 
> I already have implemented (f)locks in my code around the iptables
> invocations.  But the problem can still arise when root executes the
> iptables command from the shell. Thus, I still need to handle the
> situation in my code, and a proper exitcode would be nice.
> 
> In iptables-standalone.c it would be very easy to simply return the
> errno instead of !res (which always will return 0 or 1).  Would that be
> a feasable solution?

It would probably break other scripts that check for the current
(documented) exit codes. I guess adding a new one for this case
is fine, other code can't really expect that no new values are
ever added.

Re: Concurrency issues with the iptables userspace program andexitcodes

[Jesper Dangaard Brouer]

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

On Thu, 2006-06-01 at 22:45 +0200, Patrick McHardy wrote:
> Jesper Dangaard Brouer wrote:
> > 
> > On Thu, 1 Jun 2006, Patrick McHardy wrote:
> > 
> >> Wouldn't it make more sense to just make sure you don't have iptables
> >> commands running concurrently?
> > 
> > 
> > I already have implemented (f)locks in my code around the iptables
> > invocations.  But the problem can still arise when root executes the
> > iptables command from the shell. Thus, I still need to handle the
> > situation in my code, and a proper exitcode would be nice.
> > 
> > In iptables-standalone.c it would be very easy to simply return the
> > errno instead of !res (which always will return 0 or 1).  Would that be
> > a feasable solution?
> 
> It would probably break other scripts that check for the current
> (documented) exit codes. I guess adding a new one for this case
> is fine, other code can't really expect that no new values are
> ever added.

That would be fine.
For example something like the following patch?

I have tracked down where the "Resource temporarily unavailable" occurs
in libiptc.c, and marked it with a comment... if you can use it for any
thing.

-- 
Med venlig hilsen / Best regards
  Jesper Brouer
  ComX Networks A/S
  Linux Network developer
  Cand. Scient Datalog / MSc.
  Author of http://adsl-optimizer.dk


diff --exclude '*.d' --exclude-from /var/kernel/exclude_these -urpN iptables-1.3.4-pom-l7/include/iptables_common.h iptables-1.3.4-pom-l7-blocking/include/iptables_common.h
--- iptables-1.3.4-pom-l7/include/iptables_common.h	2005-10-31 20:03:49.000000000 +0100
+++ iptables-1.3.4-pom-l7-blocking/include/iptables_common.h	2006-06-08 14:19:53.000000000 +0200
@@ -5,7 +5,8 @@
 enum exittype {
 	OTHER_PROBLEM = 1,
 	PARAMETER_PROBLEM,
-	VERSION_PROBLEM
+	VERSION_PROBLEM,
+	RESOURCE_PROBLEM
 };
 
 /* this is a special 64bit data type that is 8-byte aligned */
diff --exclude '*.d' --exclude-from /var/kernel/exclude_these -urpN iptables-1.3.4-pom-l7/iptables-standalone.c iptables-1.3.4-pom-l7-blocking/iptables-standalone.c
--- iptables-1.3.4-pom-l7/iptables-standalone.c	2005-01-04 11:38:39.000000000 +0100
+++ iptables-1.3.4-pom-l7-blocking/iptables-standalone.c	2006-06-08 14:23:48.000000000 +0200
@@ -64,9 +64,13 @@ main(int argc, char *argv[])
 	if (ret)
 		ret = iptc_commit(&handle);
 
-	if (!ret)
+	if (!ret) {
 		fprintf(stderr, "iptables: %s\n",
 			iptc_strerror(errno));
+		if (errno == EAGAIN) {
+			exit(RESOURCE_PROBLEM);
+		}
+	}
 
 	exit(!ret);
 }
diff --exclude '*.d' --exclude-from /var/kernel/exclude_these -urpN iptables-1.3.4-pom-l7/libiptc/libiptc.c iptables-1.3.4-pom-l7-blocking/libiptc/libiptc.c
--- iptables-1.3.4-pom-l7/libiptc/libiptc.c	2005-10-31 20:03:49.000000000 +0100
+++ iptables-1.3.4-pom-l7-blocking/libiptc/libiptc.c	2006-06-07 10:25:02.000000000 +0200
@@ -2104,6 +2104,7 @@ TC_COMMIT(TC_HANDLE_T *handle)
 		free(repl->counters);
 		free(repl);
 		free(newcounters);
+		// The "EAGAIN: Resource temporarily unavailable" occurs here.
 		return 0;
 	}
 


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Concurrency issues with the iptables userspace program andexitcodes

[Patrick McHardy]

Jesper Dangaard Brouer wrote:
> For example something like the following patch?
> 
> I have tracked down where the "Resource temporarily unavailable" occurs
> in libiptc.c, and marked it with a comment... if you can use it for any
> thing.

Seems like you forgot to attach the patch :)

Re: Concurrency issues with the iptables userspace program andexitcodes

[Jesper Dangaard Brouer]

[-- Attachment #1.1: Type: text/plain, Size: 2426 bytes --]

On Wed, 2006-06-14 at 16:05 +0200, Patrick McHardy wrote:
> Jesper Dangaard Brouer wrote:
> > For example something like the following patch?
> > 
> > I have tracked down where the "Resource temporarily unavailable" occurs
> > in libiptc.c, and marked it with a comment... if you can use it for any
> > thing.
> 
> Seems like you forgot to attach the patch :)

Hmmm... I though I did include it inline below my signature... well lets
try again...This time I also attach it as a file attachment.

-- 
Med venlig hilsen / Best regards
  Jesper Brouer
  ComX Networks A/S
  Linux Network developer
  Cand. Scient Datalog / MSc.
  Author of http://adsl-optimizer.dk

---

diff --exclude '*.d' --exclude-from /var/kernel/exclude_these -urpN iptables-1.3.4-pom-l7/include/iptables_common.h iptables-1.3.4-pom-l7-blocking/include/iptables_common.h
--- iptables-1.3.4-pom-l7/include/iptables_common.h	2005-10-31 20:03:49.000000000 +0100
+++ iptables-1.3.4-pom-l7-blocking/include/iptables_common.h	2006-06-08 14:19:53.000000000 +0200
@@ -5,7 +5,8 @@
 enum exittype {
 	OTHER_PROBLEM = 1,
 	PARAMETER_PROBLEM,
-	VERSION_PROBLEM
+	VERSION_PROBLEM,
+	RESOURCE_PROBLEM
 };
 
 /* this is a special 64bit data type that is 8-byte aligned */
diff --exclude '*.d' --exclude-from /var/kernel/exclude_these -urpN iptables-1.3.4-pom-l7/iptables-standalone.c iptables-1.3.4-pom-l7-blocking/iptables-standalone.c
--- iptables-1.3.4-pom-l7/iptables-standalone.c	2005-01-04 11:38:39.000000000 +0100
+++ iptables-1.3.4-pom-l7-blocking/iptables-standalone.c	2006-06-08 14:23:48.000000000 +0200
@@ -64,9 +64,13 @@ main(int argc, char *argv[])
 	if (ret)
 		ret = iptc_commit(&handle);
 
-	if (!ret)
+	if (!ret) {
 		fprintf(stderr, "iptables: %s\n",
 			iptc_strerror(errno));
+		if (errno == EAGAIN) {
+			exit(RESOURCE_PROBLEM);
+		}
+	}
 
 	exit(!ret);
 }
diff --exclude '*.d' --exclude-from /var/kernel/exclude_these -urpN iptables-1.3.4-pom-l7/libiptc/libiptc.c iptables-1.3.4-pom-l7-blocking/libiptc/libiptc.c
--- iptables-1.3.4-pom-l7/libiptc/libiptc.c	2005-10-31 20:03:49.000000000 +0100
+++ iptables-1.3.4-pom-l7-blocking/libiptc/libiptc.c	2006-06-07 10:25:02.000000000 +0200
@@ -2104,6 +2104,7 @@ TC_COMMIT(TC_HANDLE_T *handle)
 		free(repl->counters);
 		free(repl);
 		free(newcounters);
+		// The "EAGAIN: Resource temporarily unavailable" occurs here.
 		return 0;
 	}
 


[-- Attachment #1.2: iptables-1.3.4-EAGAIN.diff --]
[-- Type: text/x-patch, Size: 1733 bytes --]

diff --exclude '*.d' --exclude-from /var/kernel/exclude_these -urpN iptables-1.3.4-pom-l7/include/iptables_common.h iptables-1.3.4-pom-l7-blocking/include/iptables_common.h
--- iptables-1.3.4-pom-l7/include/iptables_common.h	2005-10-31 20:03:49.000000000 +0100
+++ iptables-1.3.4-pom-l7-blocking/include/iptables_common.h	2006-06-08 14:19:53.000000000 +0200
@@ -5,7 +5,8 @@
 enum exittype {
 	OTHER_PROBLEM = 1,
 	PARAMETER_PROBLEM,
-	VERSION_PROBLEM
+	VERSION_PROBLEM,
+	RESOURCE_PROBLEM
 };
 
 /* this is a special 64bit data type that is 8-byte aligned */
diff --exclude '*.d' --exclude-from /var/kernel/exclude_these -urpN iptables-1.3.4-pom-l7/iptables-standalone.c iptables-1.3.4-pom-l7-blocking/iptables-standalone.c
--- iptables-1.3.4-pom-l7/iptables-standalone.c	2005-01-04 11:38:39.000000000 +0100
+++ iptables-1.3.4-pom-l7-blocking/iptables-standalone.c	2006-06-08 14:23:48.000000000 +0200
@@ -64,9 +64,13 @@ main(int argc, char *argv[])
 	if (ret)
 		ret = iptc_commit(&handle);
 
-	if (!ret)
+	if (!ret) {
 		fprintf(stderr, "iptables: %s\n",
 			iptc_strerror(errno));
+		if (errno == EAGAIN) {
+			exit(RESOURCE_PROBLEM);
+		}
+	}
 
 	exit(!ret);
 }
diff --exclude '*.d' --exclude-from /var/kernel/exclude_these -urpN iptables-1.3.4-pom-l7/libiptc/libiptc.c iptables-1.3.4-pom-l7-blocking/libiptc/libiptc.c
--- iptables-1.3.4-pom-l7/libiptc/libiptc.c	2005-10-31 20:03:49.000000000 +0100
+++ iptables-1.3.4-pom-l7-blocking/libiptc/libiptc.c	2006-06-07 10:25:02.000000000 +0200
@@ -2104,6 +2104,7 @@ TC_COMMIT(TC_HANDLE_T *handle)
 		free(repl->counters);
 		free(repl);
 		free(newcounters);
+		// The "EAGAIN: Resource temporarily unavailable" occurs here.
 		return 0;
 	}
 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Concurrency issues with the iptables userspace program andexitcodes

[Patrick McHardy]

Jesper Dangaard Brouer wrote:
> Hmmm... I though I did include it inline below my signature... well lets
> try again...This time I also attach it as a file attachment.

Applied, thanks. I got a reject on the libiptc part, so I just left
it out.

> diff --exclude '*.d' --exclude-from /var/kernel/exclude_these -urpN iptables-1.3.4-pom-l7/libiptc/libiptc.c iptables-1.3.4-pom-l7-blocking/libiptc/libiptc.c
> --- iptables-1.3.4-pom-l7/libiptc/libiptc.c	2005-10-31 20:03:49.000000000 +0100
> +++ iptables-1.3.4-pom-l7-blocking/libiptc/libiptc.c	2006-06-07 10:25:02.000000000 +0200
> @@ -2104,6 +2104,7 @@ TC_COMMIT(TC_HANDLE_T *handle)
>  		free(repl->counters);
>  		free(repl);
>  		free(newcounters);
> +		// The "EAGAIN: Resource temporarily unavailable" occurs here.
>  		return 0;
>  	}
>