[Patch] ip{,6}tables-restore -n with existing user defined chain

[Patch] ip{,6}tables-restore -n with existing user defined chain

[Charlie Brady]

I want to redefine an existing chain atomically. I can't do that with the 
iptables command, but can almost do it with iptables-restore -n. When I 
try, iptables barfs because the chain already exists. Duh! Yeah, I know 
it exists, but I want to redefine it.

I don't see the semantics of this case defined anywhere, and I can't find 
discussion of it in the archives. So I suggest that the semantics be 
redefined, so that iptables-restore -n can redefine an existing chain 
(iptables-restore without -n already does that). I really can't think why 
anyone would depend on the current semantics.

Index: ip6tables-restore.c
===================================================================
--- ip6tables-restore.c	(revision 3922)
+++ ip6tables-restore.c	(working copy)
@@ -233,12 +233,21 @@
  			}

  			if (ip6tc_builtin(chain, handle) <= 0) {
-				DEBUGP("Creating new chain '%s'\n", chain);
-				if (!ip6tc_create_chain(chain, &handle))
-					exit_error(PARAMETER_PROBLEM,
-						   "error creating chain "
-						   "'%s':%s\n", chain,
-						   strerror(errno));
+				if (noflush && ip6tc_is_chain(chain, handle)) {
+					DEBUGP("Flushing existing user defined chain '%s'\n", chain);
+					if (!ip6tc_flush_entries(chain, &handle))
+						exit_error(PARAMETER_PROBLEM,
+							   "error flushing chain "
+							   "'%s':%s\n", chain,
+							   strerror(errno));
+				} else {
+					DEBUGP("Creating new chain '%s'\n", chain);
+					if (!ip6tc_create_chain(chain, &handle))
+						exit_error(PARAMETER_PROBLEM,
+							   "error creating chain "
+							   "'%s':%s\n", chain,
+							   strerror(errno));
+				}
  			}

  			policy = strtok(NULL, " \t\n");
Index: iptables-restore.c
===================================================================
--- iptables-restore.c	(revision 3922)
+++ iptables-restore.c	(working copy)
@@ -236,12 +236,21 @@
  			}

  			if (iptc_builtin(chain, handle) <= 0) {
-				DEBUGP("Creating new chain '%s'\n", chain);
-				if (!iptc_create_chain(chain, &handle))
-					exit_error(PARAMETER_PROBLEM,
-						   "error creating chain "
-						   "'%s':%s\n", chain,
-						   strerror(errno));
+				if (noflush && iptc_is_chain(chain, handle)) {
+					DEBUGP("Flushing existing user defined chain '%s'\n", chain);
+					if (!iptc_flush_entries(chain, &handle))
+						exit_error(PARAMETER_PROBLEM,
+							   "error flushing chain "
+							   "'%s':%s\n", chain,
+							   strerror(errno));
+				} else {
+					DEBUGP("Creating new chain '%s'\n", chain);
+					if (!iptc_create_chain(chain, &handle))
+						exit_error(PARAMETER_PROBLEM,
+							   "error creating chain "
+							   "'%s':%s\n", chain,
+							   strerror(errno));
+				}
  			}

  			policy = strtok(NULL, " \t\n");

Re: [Patch] ip{, 6}tables-restore -n with existing user defined chain

[Charlie Brady]

On Wed, 18 May 2005, Charlie Brady wrote:

> I want to redefine an existing chain atomically. I can't do that with the 
> iptables command, but can almost do it with iptables-restore -n. When I try, 
> iptables barfs because the chain already exists. Duh! Yeah, I know it exists, 
> but I want to redefine it.
>
> I don't see the semantics of this case defined anywhere, and I can't find 
> discussion of it in the archives. So I suggest that the semantics be 
> redefined, so that iptables-restore -n can redefine an existing chain 
> (iptables-restore without -n already does that). I really can't think why 
> anyone would depend on the current semantics.

As Clifford Wolf said:

  Please apply the patch in the subversion repository or comment on it if you
  think it should not be included..

Thanks

---
Charlie

Re: [Patch] ip{, 6}tables-restore -n with existing user defined chain

[Jonas Berlin]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoting Charlie Brady on 2005-05-18 16:07 UTC:
> 
> I want to redefine an existing chain atomically. I can't do that with
> the iptables command, but can almost do it with iptables-restore -n.
> When I try, iptables barfs because the chain already exists. Duh! Yeah,
> I know it exists, but I want to redefine it.

One option is to make a new version with a new name and then atomically
replace jumps to the old version to use the new version:

original setup:

  iptables -N INPUT0
  iptables -A INPUT0 ...
  ...
  iptables -N FOOBAR0
  iptables -A FOOBAR0...
  iptables -A INPUT0 ... -j FOOBAR0
  ...
  iptables -A INPUT -j INPUT0

switch to new:

  iptables -N INPUT1
  iptables -A INPUT1 ...
  ...
  iptables -N FOOBAR1
  iptables -A FOOBAR1...
  iptables -A INPUT1 ... -j FOOBAR1
  ...
  iptables -R INPUT 1 -j INPUT1
  # cleanup (could be replaced by some automatic loop finding all chains
  #          named something ending in "0")
  iptables -F INPUT0
  iptables -X INPUT0
  iptables -F FOOBAR0
  iptables -X FOOBAR0

It would probably be rather easy to create some wrappers that manage the
 versioning.. Also, two versions would be enough (0 and 1).

I might just go ahead and try something like this myself, actually.. :)

- --
- - xkr47
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)

iD8DBQFCjLQ7xyF48ZTvn+4RAhTbAKC5yYTZPxsEj9A3YH3RZZuv0TzQlACgytFN
T2G76lKiEO5cWBAQ/PKVmXA=
=W20U
-----END PGP SIGNATURE-----

Re: [Patch] ip{, 6}tables-restore -n with existing user defined chain

[Charlie Brady]

On Thu, 19 May 2005, Jonas Berlin wrote:

>  iptables -N FOOBAR1
>  iptables -A FOOBAR1...
>  iptables -A INPUT1 ... -j FOOBAR1
>  ...
>  iptables -R INPUT 1 -j INPUT1
>  # cleanup (could be replaced by some automatic loop finding all chains
>  #          named something ending in "0")
>  iptables -F INPUT0
>  iptables -X INPUT0
>  iptables -F FOOBAR0
>  iptables -X FOOBAR0
>
> It would probably be rather easy to create some wrappers that manage the
> versioning.. Also, two versions would be enough (0 and 1).

Yes, indeed. That's exactly the code that I'm trying to replace with 
something neater and waaaaaay quicker. :-)

> I might just go ahead and try something like this myself, actually.. :)

By all means. Then do 1-to-1 DNAT of a 3000 port block range, on a low end 
processor. Then you'll know why I've proposed this particular change, 
which I think will inconvenience no-one.

Regards

---
Charlie

Re: [Patch] ip{, 6}tables-restore -n with existing user defined chain

[Charlie Brady]

On Thu, 19 May 2005, Charlie Brady wrote:

> Yes, indeed. That's exactly the code that I'm trying to replace with 
> something neater and waaaaaay quicker. :-)

And not subject to the same race conditions ...

---
Charlie

Re: [Patch] ip{, 6}tables-restore -n with existing user defined chain

[Carl-Daniel Hailfinger]

Jonas Berlin schrieb:
> Quoting Charlie Brady on 2005-05-18 16:07 UTC:
> 
>>>I want to redefine an existing chain atomically. I can't do that with
>>>the iptables command, but can almost do it with iptables-restore -n.
>>>When I try, iptables barfs because the chain already exists. Duh! Yeah,
>>>I know it exists, but I want to redefine it.
> 
> 
> One option is to make a new version with a new name and then atomically
> replace jumps to the old version to use the new version:

What about atomic rename instead?


> original setup:
> 
>   iptables -N INPUT0
>   iptables -A INPUT0 ...
>   ...
>   iptables -N FOOBAR0
>   iptables -A FOOBAR0...
>   iptables -A INPUT0 ... -j FOOBAR0
>   ...
>   iptables -A INPUT -j INPUT0
> 
> switch to new:
> 
>   iptables -N tmp0
>   iptables -A tmp0 ...
>   ...
>   iptables -N tmp1
>   iptables -A tmp1...
>   iptables -A tmp0 ... -j tmp1
>   ...
iptables --exchange-names tmp0:INPUT0,tmp1:FOOBAR0

>   # cleanup (could be replaced by some automatic loop finding all chains
>   #          named something ending in "0")
>   iptables -F tmp0
>   iptables -X tmp0
>   iptables -F tmp1
>   iptables -X tmp1

What do you think?

Regards,
Carl-Daniel

Re: [Patch] ip{, 6}tables-restore -n with existing user defined chain

[Charlie Brady]

On Thu, 19 May 2005, Carl-Daniel Hailfinger wrote:

> Jonas Berlin schrieb:
>> Quoting Charlie Brady on 2005-05-18 16:07 UTC:
>>
>>>> I want to redefine an existing chain atomically. I can't do that with
>>>> the iptables command, but can almost do it with iptables-restore -n.
>>>> When I try, iptables barfs because the chain already exists. Duh! Yeah,
>>>> I know it exists, but I want to redefine it.
>>
>> One option is to make a new version with a new name and then atomically
>> replace jumps to the old version to use the new version:
>
> What about atomic rename instead?

That should be a different topic of discussion. Does anyone have any 
objection to the patch as posted? IOW, does anyone depend on the current 
semantics that calling "iptables-restore -n" with a definition of a 
user-defined chain which already exists should fail, rather than redefine 
the chain as specified in the script?

---
Charlie

Re: [Patch] ip{, 6}tables-restore -n with existing user defined chain

[Patrick McHardy]

Charlie Brady wrote:
> 
> I want to redefine an existing chain atomically. I can't do that with
> the iptables command, but can almost do it with iptables-restore -n.
> When I try, iptables barfs because the chain already exists. Duh! Yeah,
> I know it exists, but I want to redefine it.
> 
> I don't see the semantics of this case defined anywhere, and I can't
> find discussion of it in the archives. So I suggest that the semantics
> be redefined, so that iptables-restore -n can redefine an existing chain
> (iptables-restore without -n already does that). I really can't think
> why anyone would depend on the current semantics.

I have no objections, but since I'm not too familiar with that code
I would like to hear Harald's opinion before applying it.

Regards
Patrick

Re: [Patch] ip{, 6}tables-restore -n with existing user defined chain

[Harald Welte]

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

On Sat, Jun 11, 2005 at 06:12:49PM +0200, Patrick McHardy wrote:
> Charlie Brady wrote:
> > 
> > I want to redefine an existing chain atomically. I can't do that with
> > the iptables command, but can almost do it with iptables-restore -n.
> > When I try, iptables barfs because the chain already exists. Duh! Yeah,
> > I know it exists, but I want to redefine it.
> > 
> > I don't see the semantics of this case defined anywhere, and I can't
> > find discussion of it in the archives. So I suggest that the semantics
> > be redefined, so that iptables-restore -n can redefine an existing chain
> > (iptables-restore without -n already does that). I really can't think
> > why anyone would depend on the current semantics.
> 
> I have no objections, but since I'm not too familiar with that code
> I would like to hear Harald's opinion before applying it.

It seems fine to me, but we alwasy need to be cautious with silently
changing semantics that people might rely on...  

but please apply...

-- 
- Harald Welte <laforge@netfilter.org>                 http://netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [Patch] ip{, 6}tables-restore -n with existing user defined chain

[Patrick McHardy]

Harald Welte wrote:
> It seems fine to me, but we alwasy need to be cautious with silently
> changing semantics that people might rely on...  
> 
> but please apply...

Thanks for your feedback. Charlie, your patch doesn't apply, apparently
because of whitespace issues:

patching file ip6tables-restore.c
Hunk #1 FAILED at 233.
1 out of 1 hunk FAILED -- saving rejects to file ip6tables-restore.c.rej
patching file iptables-restore.c
Hunk #1 FAILED at 236.
1 out of 1 hunk FAILED -- saving rejects to file iptables-restore.c.rej

Please resend as attachment.

Regards
Patrick

Re: [Patch] ip{, 6}tables-restore -n with existing user defined chain

[Charlie Brady]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 213 bytes --]


On Sun, 12 Jun 2005, Patrick McHardy wrote:

> Thanks for your feedback. Charlie, your patch doesn't apply, apparently
> because of whitespace issues:
...
> Please resend as attachment.

Attached.

Thanks
Charlie

[-- Attachment #2: Type: TEXT/PLAIN, Size: 2193 bytes --]

Index: ip6tables-restore.c
===================================================================
--- ip6tables-restore.c	(revision 3979)
+++ ip6tables-restore.c	(working copy)
@@ -233,12 +233,21 @@
 			}
 
 			if (ip6tc_builtin(chain, handle) <= 0) {
-				DEBUGP("Creating new chain '%s'\n", chain);
-				if (!ip6tc_create_chain(chain, &handle))
-					exit_error(PARAMETER_PROBLEM,
-						   "error creating chain "
-						   "'%s':%s\n", chain,
-						   strerror(errno));
+				if (noflush && ip6tc_is_chain(chain, handle)) {
+					DEBUGP("Flushing existing user defined chain '%s'\n", chain);
+					if (!ip6tc_flush_entries(chain, &handle))
+						exit_error(PARAMETER_PROBLEM,
+							   "error flushing chain "
+							   "'%s':%s\n", chain,
+							   strerror(errno));
+				} else {
+					DEBUGP("Creating new chain '%s'\n", chain);
+					if (!ip6tc_create_chain(chain, &handle))
+						exit_error(PARAMETER_PROBLEM,
+							   "error creating chain "
+							   "'%s':%s\n", chain,
+							   strerror(errno));
+				}
 			}
 
 			policy = strtok(NULL, " \t\n");
Index: iptables-restore.c
===================================================================
--- iptables-restore.c	(revision 3979)
+++ iptables-restore.c	(working copy)
@@ -236,12 +236,21 @@
 			}
 
 			if (iptc_builtin(chain, handle) <= 0) {
-				DEBUGP("Creating new chain '%s'\n", chain);
-				if (!iptc_create_chain(chain, &handle)) 
-					exit_error(PARAMETER_PROBLEM, 
-						   "error creating chain "
-						   "'%s':%s\n", chain, 
-						   strerror(errno));
+				if (noflush && iptc_is_chain(chain, handle)) {
+					DEBUGP("Flushing existing user defined chain '%s'\n", chain);
+					if (!iptc_flush_entries(chain, &handle))
+						exit_error(PARAMETER_PROBLEM,
+							   "error flushing chain "
+							   "'%s':%s\n", chain,
+							   strerror(errno));
+				} else {
+					DEBUGP("Creating new chain '%s'\n", chain);
+					if (!iptc_create_chain(chain, &handle))
+						exit_error(PARAMETER_PROBLEM,
+							   "error creating chain "
+							   "'%s':%s\n", chain,
+							   strerror(errno));
+				}
 			}
 
 			policy = strtok(NULL, " \t\n");

Re: [Patch] ip{, 6}tables-restore -n with existing user defined chain

[Patrick McHardy]

Charlie Brady wrote:
> Attached.

Applied, thanks. Could you send a patch that updates the
iptables-restore manpage to reflect this change?

Regards
Patrick

Re: [Patch] ip{, 6}tables-restore -n with existing user defined chain

[Charlie Brady]

On Sun, 12 Jun 2005, Patrick McHardy wrote:

> Charlie Brady wrote:
>> Attached.
>
> Applied, thanks. Could you send a patch that updates the
> iptables-restore manpage to reflect this change?

I'd be happy to, but I'm not sure what it would be appropriate to say in 
an altered manpage.

The current manpage still succinctly and accurately describes the function 
and usage of iptables-restore. What it doesn't do is define behaviour in 
every corner case or exception. The description IOW is incomplete. But 
that is probably true of many/most man pages.

IMO, my patch just fixes a bug.

Charlie