All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed
* [ANNOUNCE] iptables 1.4.16.1 release
@ 2012-10-07 23:17 Pablo Neira Ayuso
  2012-10-07 23:51 ` Pablo Neira Ayuso
  2012-10-08  0:14 ` Jan Engelhardt
  0 siblings, 2 replies; 6+ messages in thread
From: Pablo Neira Ayuso @ 2012-10-07 23:17 UTC (permalink / raw
  To: netfilter-devel; +Cc: netdev, netfilter, netfilter-announce, lwn

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

On Mon, Oct 08, 2012 at 12:24:41AM +0200, Pablo Neira Ayuso wrote:
Hi!

The Netfilter project proudly presents:

        iptables 1.4.16.1

This release fixes a major breakage introduced by:

commit cd2f9bdbb7f9b737e5d640aafeb78bcd8e3a7adf
Author: Jan Engelhardt <jengelh@inai.de>
Date:   Tue Sep 4 05:24:47 2012 +0200

    iptables: support for target aliases

This is really unfortunate, it seems this patch has been pushed
mainstream without sufficient testing. We are really sorry for the
inconvenience. Please, don't use 1.4.16, this bug reders it
completely useless.

See ChangeLog that comes attached to this email for more details.

You can download it from:

http://www.netfilter.org/projects/iptables/downloads.html
ftp://ftp.netfilter.org/pub/iptables/

Have fun!

[-- Attachment #2: changes-iptables-1.4.16.1.txt --]
[-- Type: text/plain, Size: 91 bytes --]

Pablo Neira Ayuso (2):
      iptables: fix standard target
      bump version to 1.4.16.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [ANNOUNCE] iptables 1.4.16.1 release
  2012-10-07 23:17 [ANNOUNCE] iptables 1.4.16.1 release Pablo Neira Ayuso
@ 2012-10-07 23:51 ` Pablo Neira Ayuso
  2012-10-08  0:14 ` Jan Engelhardt
  1 sibling, 0 replies; 6+ messages in thread
From: Pablo Neira Ayuso @ 2012-10-07 23:51 UTC (permalink / raw
  To: netfilter-devel

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

On Mon, Oct 08, 2012 at 01:17:06AM +0200, Pablo Neira Ayuso wrote:
> On Mon, Oct 08, 2012 at 12:24:41AM +0200, Pablo Neira Ayuso wrote:
> Hi!
> 
> The Netfilter project proudly presents:
> 
>         iptables 1.4.16.1
> 
> This release fixes a major breakage introduced by:
> 
> commit cd2f9bdbb7f9b737e5d640aafeb78bcd8e3a7adf
> Author: Jan Engelhardt <jengelh@inai.de>
> Date:   Tue Sep 4 05:24:47 2012 +0200
> 
>     iptables: support for target aliases
> 
> This is really unfortunate, it seems this patch has been pushed
> mainstream without sufficient testing. We are really sorry for the
> inconvenience. Please, don't use 1.4.16, this bug reders it
> completely useless.
> 
> See ChangeLog that comes attached to this email for more details.

I got everything working back but the new target alias infrastructure
while rushing to fix the breakage.

The patch attached should get the target aliasing working. This is not
serious though, the only target that can benefit from this is feature
is NOTRACK, and it will be removed in the upcoming 3.7, ie. there is
no kernel available that can benefit from this feature.

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 1032 bytes --]

diff --git a/iptables/ip6tables.c b/iptables/ip6tables.c
index faddb71..1683621 100644
--- a/iptables/ip6tables.c
+++ b/iptables/ip6tables.c
@@ -1286,7 +1286,7 @@ static void command_jump(struct iptables_command_state *cs)
 
 	cs->target->t = xtables_calloc(1, size);
 	cs->target->t->u.target_size = size;
-	if (cs->target->real_name != NULL)
+	if (strcmp(cs->target->real_name, "standard") == 0)
 		strcpy(cs->target->t->u.user.name, cs->jumpto);
 	else
 		strcpy(cs->target->t->u.user.name, cs->target->real_name);
diff --git a/iptables/iptables.c b/iptables/iptables.c
index 96cea64..ae22ead 100644
--- a/iptables/iptables.c
+++ b/iptables/iptables.c
@@ -1295,7 +1295,7 @@ static void command_jump(struct iptables_command_state *cs)
 
 	cs->target->t = xtables_calloc(1, size);
 	cs->target->t->u.target_size = size;
-	if (cs->target->real_name != NULL)
+	if (strcmp(cs->target->real_name, "standard") == 0)
 		strcpy(cs->target->t->u.user.name, cs->jumpto);
 	else
 		strcpy(cs->target->t->u.user.name, cs->target->real_name);

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [ANNOUNCE] iptables 1.4.16.1 release
  2012-10-07 23:17 [ANNOUNCE] iptables 1.4.16.1 release Pablo Neira Ayuso
  2012-10-07 23:51 ` Pablo Neira Ayuso
@ 2012-10-08  0:14 ` Jan Engelhardt
  2012-10-08  0:32   ` [PATCH] iptables: restore NOTRACK functionality, target aliasing Jan Engelhardt
  1 sibling, 1 reply; 6+ messages in thread
From: Jan Engelhardt @ 2012-10-08  0:14 UTC (permalink / raw
  To: Pablo Neira Ayuso
  Cc: netfilter-devel, netdev, netfilter, netfilter-announce, lwn


On Monday 2012-10-08 01:17, Pablo Neira Ayuso wrote:
>The Netfilter project proudly presents:
>
>        iptables 1.4.16.1
>
>iptables -I INPUT -j ACCEPT
>says:
>iptables: No chain/target/match by that name.
>This also breaks iptables-restore, of course. Jan, you'll have to explain
>me how you have tested this.

This was tested by adding rules with different targets that had both
aliases defined and those without.

 ./iptables/xtables-multi main4 -t raw -N foo
 ./iptables/xtables-multi main4 -t raw -A foo -j NOTRACK
 with kernels that had xt_CT and no xt_CT at all

 ./iptables/xtables-multi main4 -N foo
 ./iptables/xtables-multi main4 -A foo -m state --state NEW
 with kernels that had xt_conntrack.3, and xt_conntrack.3 removed
 (leaving only xt_conntrack.2)

 ./iptables/xtables-multi main4 -t raw -N bar
 ./iptables/xtables-multi main4 -t raw -A bar -j MARK --set-xmark 1
 ./iptables/xtables-multi main4 -t raw -A foo -j bar

plus of course the "standard" (no pun intended) testsuite that we
had so far:

 # ./iptables/xtables-multi restore6 tests/options-most.rules 
 WARNING: --localtz is being replaced by --kerneltz, since "local" is ambiguous.
 Note the kernel timezone has caveats - see manpage for details.

As you spotted, options-most.rules did not include -j <verdict>.

While v1.4.16-1-g2aaa7ec fixes -j verdict, it breaks NOTRACK in all
instances. To reuse a line, "you'll have to explain me how you have
tested this."

A patch to what I think should fly is posted as a reply hereto.
Please give that a spin.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH] iptables: restore NOTRACK functionality, target aliasing
  2012-10-08  0:14 ` Jan Engelhardt
@ 2012-10-08  0:32   ` Jan Engelhardt
  2012-10-08  8:37     ` Pablo Neira Ayuso
  0 siblings, 1 reply; 6+ messages in thread
From: Jan Engelhardt @ 2012-10-08  0:32 UTC (permalink / raw
  To: pablo; +Cc: netfilter-devel, jengelh

Commit v1.4.16-1-g2aaa7ec is testing for real_name (not) being NULL
which was always false (true). real_name was never NULL, so cs->jumpto
would always be used, which rendered -j NOTRACK unusable, since the
chosen real name.revision is for example NOTRACK.1, which does not exist
at the kernel side.

	# ./iptables/xtables-multi main4 -t raw -A foo -j NOTRACK
	dbg: Using NOTRACK.1
	WARNING: The NOTRACK target is obsolete. Use CT instead.
	iptables: Protocol wrong type for socket.

To reasonably support the extra-special verdict names, make it so that
real_name remains NULL when an extension defined no alias, which we can
then use to determine whether the user entered an alias name (which
needs to be followed) or not.

Signed-off-by: Jan Engelhardt <jengelh@inai.de>
---

(This is a try at a resend since I have not gotten back this very email
from majordomo as a list subscriber...)

Patch downloadable via
	git://git.inai.de/iptables master
as well

 iptables/ip6tables.c |   21 ++++++++++++++-------
 iptables/iptables.c  |   23 +++++++++++++++--------
 libxtables/xtables.c |   26 ++++++++++++++------------
 3 files changed, 43 insertions(+), 27 deletions(-)

diff --git a/iptables/ip6tables.c b/iptables/ip6tables.c
index faddb71..0f32b32 100644
--- a/iptables/ip6tables.c
+++ b/iptables/ip6tables.c
@@ -1286,15 +1286,19 @@ static void command_jump(struct iptables_command_state *cs)
 
 	cs->target->t = xtables_calloc(1, size);
 	cs->target->t->u.target_size = size;
-	if (cs->target->real_name != NULL)
+	if (cs->target->real_name == NULL) {
+		/*
+		 * Spooky action at a distance: Verdicts like ACCEPT, DROP,
+		 * etc. are translated to the real name in libiptc instead.
+		 */
 		strcpy(cs->target->t->u.user.name, cs->jumpto);
-	else
+	} else {
 		strcpy(cs->target->t->u.user.name, cs->target->real_name);
-	cs->target->t->u.user.revision = cs->target->revision;
-	if (cs->target->real_name != cs->target->name)
 		fprintf(stderr, "WARNING: The %s target is obsolete. "
 		        "Use %s instead.\n",
 		        cs->jumpto, cs->target->real_name);
+	}
+	cs->target->t->u.user.revision = cs->target->revision;
 
 	xs_init_target(cs->target);
 	if (cs->target->x6_options != NULL)
@@ -1322,11 +1326,14 @@ static void command_match(struct iptables_command_state *cs)
 	size = XT_ALIGN(sizeof(struct xt_entry_match)) + m->size;
 	m->m = xtables_calloc(1, size);
 	m->m->u.match_size = size;
-	strcpy(m->m->u.user.name, m->real_name);
-	m->m->u.user.revision = m->revision;
-	if (m->real_name != m->name)
+	if (m->real_name == NULL) {
+		strcpy(m->m->u.user.name, m->name);
+	} else {
+		strcpy(m->m->u.user.name, m->real_name);
 		fprintf(stderr, "WARNING: The %s match is obsolete. "
 		        "Use %s instead.\n", m->name, m->real_name);
+	}
+	m->m->u.user.revision = m->revision;
 
 	xs_init_match(m);
 	if (m == m->next)
diff --git a/iptables/iptables.c b/iptables/iptables.c
index 96cea64..cd34401 100644
--- a/iptables/iptables.c
+++ b/iptables/iptables.c
@@ -1295,16 +1295,20 @@ static void command_jump(struct iptables_command_state *cs)
 
 	cs->target->t = xtables_calloc(1, size);
 	cs->target->t->u.target_size = size;
-	if (cs->target->real_name != NULL)
+	if (cs->target->real_name == NULL) {
+		/*
+		 * Spooky action at a distance: Verdicts like ACCEPT, DROP,
+		 * etc. are translated to the real name in libiptc instead.
+		 */
 		strcpy(cs->target->t->u.user.name, cs->jumpto);
-	else
-		strcpy(cs->target->t->u.user.name, cs->target->real_name);
-	cs->target->t->u.user.revision = cs->target->revision;
-	if (cs->target->real_name != cs->target->name)
+	} else {
 		/* Alias support for userspace side */
+		strcpy(cs->target->t->u.user.name, cs->target->real_name);
 		fprintf(stderr, "WARNING: The %s target is obsolete. "
 		        "Use %s instead.\n",
 		        cs->jumpto, cs->target->real_name);
+	}
+	cs->target->t->u.user.revision = cs->target->revision;
 
 	xs_init_target(cs->target);
 
@@ -1333,11 +1337,14 @@ static void command_match(struct iptables_command_state *cs)
 	size = XT_ALIGN(sizeof(struct xt_entry_match)) + m->size;
 	m->m = xtables_calloc(1, size);
 	m->m->u.match_size = size;
-	strcpy(m->m->u.user.name, m->real_name);
-	m->m->u.user.revision = m->revision;
-	if (m->real_name != m->name)
+	if (m->real_name == NULL) {
+		strcpy(m->m->u.user.name, m->name);
+	} else {
+		strcpy(m->m->u.user.name, m->real_name);
 		fprintf(stderr, "WARNING: The %s match is obsolete. "
 		        "Use %s instead.\n", m->name, m->real_name);
+	}
+	m->m->u.user.revision = m->revision;
 
 	xs_init_match(m);
 	if (m == m->next)
diff --git a/libxtables/xtables.c b/libxtables/xtables.c
index 82c3643..4c91286 100644
--- a/libxtables/xtables.c
+++ b/libxtables/xtables.c
@@ -848,8 +848,6 @@ void xtables_register_match(struct xtables_match *me)
 		exit(1);
 	}
 
-	if (me->real_name == NULL)
-		me->real_name = me->name;
 	if (me->x6_options != NULL)
 		xtables_option_metavalidate(me->name, me->x6_options);
 	if (me->extra_opts != NULL)
@@ -905,9 +903,9 @@ xtables_mt_prefer(bool a_alias, unsigned int a_rev, unsigned int a_fam,
 static int xtables_match_prefer(const struct xtables_match *a,
 				const struct xtables_match *b)
 {
-	return xtables_mt_prefer(a->name != a->real_name,
+	return xtables_mt_prefer(a->real_name != NULL,
 				 a->revision, a->family,
-				 b->name != b->real_name,
+				 b->real_name != NULL,
 				 b->revision, b->family);
 }
 
@@ -919,15 +917,16 @@ static int xtables_target_prefer(const struct xtables_target *a,
 	 * xtables_register_*; the direct pointer comparison here is therefore
 	 * legitimate to detect an alias.
 	 */
-	return xtables_mt_prefer(a->name != a->real_name,
+	return xtables_mt_prefer(a->real_name != NULL,
 				 a->revision, a->family,
-				 b->name != b->real_name,
+				 b->real_name != NULL,
 				 b->revision, b->family);
 }
 
 static void xtables_fully_register_pending_match(struct xtables_match *me)
 {
 	struct xtables_match **i, *old;
+	const char *rn;
 	int compare;
 
 	old = xtables_find_match(me->name, XTF_DURING_LOAD, NULL);
@@ -941,12 +940,14 @@ static void xtables_fully_register_pending_match(struct xtables_match *me)
 		}
 
 		/* Now we have two (or more) options, check compatibility. */
+		rn = (old->real_name != NULL) ? old->real_name : old->name;
 		if (compare > 0 &&
-		    compatible_match_revision(old->real_name, old->revision))
+		    compatible_match_revision(rn, old->revision))
 			return;
 
 		/* See if new match can be used. */
-		if (!compatible_match_revision(me->real_name, me->revision))
+		rn = (me->real_name != NULL) ? me->real_name : me->name;
+		if (!compatible_match_revision(rn, me->revision))
 			return;
 
 		/* Delete old one. */
@@ -1005,8 +1006,6 @@ void xtables_register_target(struct xtables_target *me)
 		exit(1);
 	}
 
-	if (me->real_name == NULL)
-		me->real_name = me->name;
 	if (me->x6_options != NULL)
 		xtables_option_metavalidate(me->name, me->x6_options);
 	if (me->extra_opts != NULL)
@@ -1024,6 +1023,7 @@ void xtables_register_target(struct xtables_target *me)
 static void xtables_fully_register_pending_target(struct xtables_target *me)
 {
 	struct xtables_target *old;
+	const char *rn;
 	int compare;
 
 	old = xtables_find_target(me->name, XTF_DURING_LOAD);
@@ -1039,12 +1039,14 @@ static void xtables_fully_register_pending_target(struct xtables_target *me)
 		}
 
 		/* Now we have two (or more) options, check compatibility. */
+		rn = (old->real_name != NULL) ? old->real_name : old->name;
 		if (compare > 0 &&
-		    compatible_target_revision(old->real_name, old->revision))
+		    compatible_target_revision(rn, old->revision))
 			return;
 
 		/* See if new target can be used. */
-		if (!compatible_target_revision(me->real_name, me->revision))
+		rn = (me->real_name != NULL) ? me->real_name : me->name;
+		if (!compatible_target_revision(rn, me->revision))
 			return;
 
 		/* Delete old one. */
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] iptables: restore NOTRACK functionality, target aliasing
  2012-10-08  0:32   ` [PATCH] iptables: restore NOTRACK functionality, target aliasing Jan Engelhardt
@ 2012-10-08  8:37     ` Pablo Neira Ayuso
  2012-10-08 12:02       ` Jan Engelhardt
  0 siblings, 1 reply; 6+ messages in thread
From: Pablo Neira Ayuso @ 2012-10-08  8:37 UTC (permalink / raw
  To: Jan Engelhardt; +Cc: netfilter-devel

On Mon, Oct 08, 2012 at 02:32:36AM +0200, Jan Engelhardt wrote:
> Commit v1.4.16-1-g2aaa7ec is testing for real_name (not) being NULL
> which was always false (true). real_name was never NULL, so cs->jumpto
> would always be used, which rendered -j NOTRACK unusable, since the
> chosen real name.revision is for example NOTRACK.1, which does not exist
> at the kernel side.
> 
> 	# ./iptables/xtables-multi main4 -t raw -A foo -j NOTRACK
> 	dbg: Using NOTRACK.1
> 	WARNING: The NOTRACK target is obsolete. Use CT instead.
> 	iptables: Protocol wrong type for socket.
> 
> To reasonably support the extra-special verdict names, make it so that
> real_name remains NULL when an extension defined no alias, which we can
> then use to determine whether the user entered an alias name (which
> needs to be followed) or not.

I have applied this and made a new release.

I kindly told you. I don't want late patches to hit iptables if I'm
about to release it, ie. close to when the Linux kernel comes out.

The reason was that chances to hit bugs and not noticing becomes
higher. In other words, stick to conservative mode.

Let this serve as proof of it.

You disregarded my advice and now we have this shame, three releases
in one day just because of rushing.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] iptables: restore NOTRACK functionality, target aliasing
  2012-10-08  8:37     ` Pablo Neira Ayuso
@ 2012-10-08 12:02       ` Jan Engelhardt
  0 siblings, 0 replies; 6+ messages in thread
From: Jan Engelhardt @ 2012-10-08 12:02 UTC (permalink / raw
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

On Monday 2012-10-08 10:37, Pablo Neira Ayuso wrote:

>On Mon, Oct 08, 2012 at 02:32:36AM +0200, Jan Engelhardt wrote:
>> Commit v1.4.16-1-g2aaa7ec is testing for real_name (not) being NULL
>> which was always false (true). real_name was never NULL, so cs->jumpto
>> would always be used, which rendered -j NOTRACK unusable, since the
>> chosen real name.revision is for example NOTRACK.1, which does not exist
>> at the kernel side.
>> 
>> 	# ./iptables/xtables-multi main4 -t raw -A foo -j NOTRACK
>> 	dbg: Using NOTRACK.1
>> 	WARNING: The NOTRACK target is obsolete. Use CT instead.
>> 	iptables: Protocol wrong type for socket.
>> 
>> To reasonably support the extra-special verdict names, make it so that
>> real_name remains NULL when an extension defined no alias, which we can
>> then use to determine whether the user entered an alias name (which
>> needs to be followed) or not.
>
>I have applied this and made a new release.
>
>I kindly told you. I don't want late patches to hit iptables if I'm
>about to release it, ie. close to when the Linux kernel comes out.
>The reason was that chances to hit bugs and not noticing becomes
>higher. In other words, stick to conservative mode.

That is clear now, but was not when when the Alias Support patches
went in. Sorry for that.

As for the documentation updates ("doc:" labeled patches in
http://marc.info/?l=netfilter-devel&m=134903982604088&w=2 ), you gave
an OK, but then infuriated over them being merged, though
documentation generally does not cause bugs.


>
>Let this serve as proof of it.
>
>You disregarded my advice and now we have this shame, three releases
>in one day just because of rushing.

I will heed your words, but I do also think that letting patches sit
in git for longer may not necessarily buy more. For -j verdict,
maybe. But in general, iptables firewalling & networking users do not
use git so much, but they love tarballs, also because that gets
spread out by distros in a glimpse.

Here is an instance of iproute2 where an issue also only got noticed
after the tar got out:

http://archlinux.2023198.n4.nabble.com/ip-addr-show-doesn-t-show-my-ipv4-address-after-update-is-this-expected-behaviour-td4658792.html
http://forums.funtoo.org/viewtopic.php?id=1526
(this subsequently also got posted to netdev)

There is also that one NLM_F_MULTI issue in libnfnetlink, which
slept for 2 months in git before becoming a tar.


If, and only if, the "next" branch is empty (which was the case for
libnfnetlink), I do not think synchronization to the kernel release
schedule makes sense. But that is just my opinion..



Something completely different:

There seems to be something happening in your git cherry-pick process
causing the time to shift. Commit
ed03bc396b861ae0302b5cc9b3826cf58ef6aa24 from
git://git.inai.de/iptables has AuthorDate 2012-10-08 01:54:48+0200,
but commit dd43527cb6bdf3d469100850ca10dcd2fb761304 in iptables has
2012-10-07 14:32:36+0000. I am not sure if your clock is off, since
the CommitDate looks accurate.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2012-10-08 12:02 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-07 23:17 [ANNOUNCE] iptables 1.4.16.1 release Pablo Neira Ayuso
2012-10-07 23:51 ` Pablo Neira Ayuso
2012-10-08  0:14 ` Jan Engelhardt
2012-10-08  0:32   ` [PATCH] iptables: restore NOTRACK functionality, target aliasing Jan Engelhardt
2012-10-08  8:37     ` Pablo Neira Ayuso
2012-10-08 12:02       ` Jan Engelhardt

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.