diff mbox

iptables: restore NOTRACK functionality, target aliasing

Message ID 1349656356-10180-1-git-send-email-jengelh@inai.de
State Accepted
Headers show

Commit Message

Jan Engelhardt Oct. 8, 2012, 12:32 a.m. UTC 
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(-)

Comments

Pablo Neira Ayuso Oct. 8, 2012, 8:37 a.m. UTC | #1 
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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jan Engelhardt Oct. 8, 2012, 12:02 p.m. UTC | #2 
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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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. */