diff mbox

[net-next] netfilter: remove xt_NOTRACK

Message ID alpine.LNX.2.01.1209040539040.9352@frira.zrqbmnf.qr
State Not Applicable
Headers show

Commit Message

Jan Engelhardt Sept. 4, 2012, 3:57 a.m. UTC 
On Tuesday 2012-09-04 02:14, Maciej Żenczykowski wrote:

>+<----->if (cs->target->alias == NULL)^M
>+<-----><------>strcpy(cs->target->t->u.user.name, cs->jumpto);^M
>+<----->else^M
>+<-----><------>strcpy(cs->target->t->u.user.name, cs->target->alias);^M
>
>I'd have probably written if (cs->target->alias) copy(alias) else copy(jumpto)
>
>doesn't this all really belong in the CT files now?
>ie. libxt_CT.c not libxt_NOTRACK.c

I think so too.
Furthermore, I have refined Pablo's patch.

0. vcurrent was not updated, now done.
1. Loading libxt_NOTRACK.so would still ask the kernel for NOTRACK.0
   (function "compatible_revision"), now addressed.
2. NOTRACK.0 can now directly map to CT.1, instead of going through CT.0.
3. Do away with libxt_NOTRACK.c, and resolve the dlopen call by
   providing a symlink.

Not solved:
4. Since NOTRACK now always maps to CT, "-j NOTRACK"
   has become unusable on sufficiently old kernels.
   Should we even bother?

[ Agglomeration of two patches in git://git.inai.de/iptables master ]
--
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

Comments

Maciej Żenczykowski Sept. 4, 2012, 5:29 a.m. UTC | #1 
> I think so too.
> Furthermore, I have refined Pablo's patch.
>
> 0. vcurrent was not updated, now done.
> 1. Loading libxt_NOTRACK.so would still ask the kernel for NOTRACK.0
>    (function "compatible_revision"), now addressed.
> 2. NOTRACK.0 can now directly map to CT.1, instead of going through CT.0.
> 3. Do away with libxt_NOTRACK.c, and resolve the dlopen call by
>    providing a symlink.

Nice.

> Not solved:
> 4. Since NOTRACK now always maps to CT, "-j NOTRACK"
>    has become unusable on sufficiently old kernels.
>    Should we even bother?

Yes, we must, otherwise distros can't upgrade to latest iptables
without either patching or upgrading kernel.
It's really nice that the two aren't that tightly coupled.
Unless by old kernels you mean pre-RHEL5 kernels.
--
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
Pablo Neira Ayuso Sept. 4, 2012, 8:58 a.m. UTC | #2 
On Mon, Sep 03, 2012 at 10:29:40PM -0700, Maciej Żenczykowski wrote:
[...]
> > Not solved:
> > 4. Since NOTRACK now always maps to CT, "-j NOTRACK"
> >    has become unusable on sufficiently old kernels.
> >    Should we even bother?
> 
> Yes, we must, otherwise distros can't upgrade to latest iptables
> without either patching or upgrading kernel.

Why not? They will upgrade and they will start using the CT target
sooner than any other, which seems good to me.
--
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
Pablo Neira Ayuso Sept. 4, 2012, 1:58 p.m. UTC | #3 
On Tue, Sep 04, 2012 at 05:57:28AM +0200, Jan Engelhardt wrote:
> 
> On Tuesday 2012-09-04 02:14, Maciej Żenczykowski wrote:
> 
> >+<----->if (cs->target->alias == NULL)^M
> >+<-----><------>strcpy(cs->target->t->u.user.name, cs->jumpto);^M
> >+<----->else^M
> >+<-----><------>strcpy(cs->target->t->u.user.name, cs->target->alias);^M
> >
> >I'd have probably written if (cs->target->alias) copy(alias) else copy(jumpto)
> >
> >doesn't this all really belong in the CT files now?
> >ie. libxt_CT.c not libxt_NOTRACK.c
> 
> I think so too.
> Furthermore, I have refined Pablo's patch.
> 
> 0. vcurrent was not updated, now done.
> 1. Loading libxt_NOTRACK.so would still ask the kernel for NOTRACK.0
>    (function "compatible_revision"), now addressed.
> 2. NOTRACK.0 can now directly map to CT.1, instead of going through CT.0.
> 3. Do away with libxt_NOTRACK.c, and resolve the dlopen call by
>    providing a symlink.
> 
> Not solved:
> 4. Since NOTRACK now always maps to CT, "-j NOTRACK"
>    has become unusable on sufficiently old kernels.
>    Should we even bother?
> 
> [ Agglomeration of two patches in git://git.inai.de/iptables master ]
> diff --git a/configure.ac b/configure.ac
> index 861f5b3..a45d9ab 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -2,8 +2,8 @@
>  AC_INIT([iptables], [1.4.15])
>  
>  # See libtool.info "Libtool's versioning system"
> -libxtables_vcurrent=8
> -libxtables_vage=1
> +libxtables_vcurrent=9
> +libxtables_vage=0
>  
>  AC_CONFIG_AUX_DIR([build-aux])
>  AC_CONFIG_HEADERS([config.h])
> diff --git a/extensions/GNUmakefile.in b/extensions/GNUmakefile.in
> index 218dc3a..92ac63d 100644
> --- a/extensions/GNUmakefile.in
> +++ b/extensions/GNUmakefile.in
> @@ -39,6 +39,7 @@ endif
>  #	Wildcard module list
>  #
>  pfx_build_mod := $(patsubst ${srcdir}/libxt_%.c,%,$(sort $(wildcard ${srcdir}/libxt_*.c)))
> +pfx_build_mod += NOTRACK
>  @ENABLE_IPV4_TRUE@ pf4_build_mod := $(patsubst ${srcdir}/libipt_%.c,%,$(sort $(wildcard ${srcdir}/libipt_*.c)))
>  @ENABLE_IPV6_TRUE@ pf6_build_mod := $(patsubst ${srcdir}/libip6t_%.c,%,$(sort $(wildcard ${srcdir}/libip6t_*.c)))
>  pfx_build_mod := $(filter-out @blacklist_modules@,${pfx_build_mod})
> @@ -100,6 +101,8 @@ lib%.oo: ${srcdir}/lib%.c
>  xt_RATEEST_LIBADD   = -lm
>  xt_statistic_LIBADD = -lm
>  
> +libxt_NOTRACK.so: libxt_CT.so
> +	ln -s $< $@
>  
>  #
>  #	Static bits
> diff --git a/extensions/libxt_CT.c b/extensions/libxt_CT.c
> index 27a20e2..8012a59 100644
> --- a/extensions/libxt_CT.c
> +++ b/extensions/libxt_CT.c
> @@ -248,6 +248,13 @@ static void ct_save_v1(const void *ip, const struct xt_entry_target *target)
>  		printf(" --zone %u", info->zone);
>  }
>  
> +static void notrack_tg_init(struct xt_entry_target *target)
> +{
> +	struct xt_ct_target_info_v1 *info = (void *)target->data;
> +
> +	info->flags |= XT_CT_NOTRACK;
> +}
> +
>  static struct xtables_target ct_target_reg[] = {
>  	{
>  		.family		= NFPROTO_UNSPEC,
> @@ -274,6 +281,19 @@ static struct xtables_target ct_target_reg[] = {
>  		.x6_parse	= ct_parse_v1,
>  		.x6_options	= ct_opts_v1,
>  	},
> +	{
> +		.family		= NFPROTO_UNSPEC,
> +		.name		= "NOTRACK",
> +		.revision	= 0,
> +		.real_name	= "CT",
> +		.real_rev	= 1,
> +		.version	= XTABLES_VERSION,
> +		.size		= XT_ALIGN(sizeof(struct xt_ct_target_info_v1)),
> +		.userspacesize	= offsetof(struct xt_ct_target_info_v1, ct),
> +		.print		= ct_print_v1,
> +		.save		= ct_save_v1,
> +		.init		= notrack_tg_init,
> +	},

We also need to add support for real_rev 0 of the CT target. Just to
make sure that we don't break with old kernels.

I've pulled this and pushed out to the notrack-removal branch of
iptables. The idea would be to fix this issue above and to merge that
that couple of patches once 3.7-rc1 is released.
--
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 Sept. 4, 2012, 3:15 p.m. UTC | #4 
On Tuesday 2012-09-04 10:58, Pablo Neira Ayuso wrote:

>On Mon, Sep 03, 2012 at 10:29:40PM -0700, Maciej Żenczykowski wrote:
>[...]
>> > Not solved:
>> > 4. Since NOTRACK now always maps to CT, "-j NOTRACK"
>> >    has become unusable on sufficiently old kernels.
>> >    Should we even bother?
>> 
>> Yes, we must, otherwise distros can't upgrade to latest iptables
>> without either patching or upgrading kernel.
>
>Why not? They will upgrade and they will start using the CT target
>sooner than any other, which seems good to me.
>
>We also need to add support for real_rev 0 of the CT target. Just to            
>make sure that we don't break with old kernels.                                 

Right; but is that not what might be described as "hypocritic"?
Even after adding support for CT.0, people still need >= 2.6.34.
Where is the non-breakage for them?

(I can't say I feel /too/ bad for the RHEL folks stuck with their
ancient 2.6.32 :-P )
(And don't tell me about backports, because in general, they don't
do that for NF.)
--
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
Pablo Neira Ayuso Sept. 4, 2012, 3:58 p.m. UTC | #5 
On Tue, Sep 04, 2012 at 05:15:17PM +0200, Jan Engelhardt wrote:
> On Tuesday 2012-09-04 10:58, Pablo Neira Ayuso wrote:
> 
> >On Mon, Sep 03, 2012 at 10:29:40PM -0700, Maciej Żenczykowski wrote:
> >[...]
> >> > Not solved:
> >> > 4. Since NOTRACK now always maps to CT, "-j NOTRACK"
> >> >    has become unusable on sufficiently old kernels.
> >> >    Should we even bother?
> >> 
> >> Yes, we must, otherwise distros can't upgrade to latest iptables
> >> without either patching or upgrading kernel.
> >
> >Why not? They will upgrade and they will start using the CT target
> >sooner than any other, which seems good to me.
> >
> >We also need to add support for real_rev 0 of the CT target. Just to            
> >make sure that we don't break with old kernels.                                 
> 
> Right; but is that not what might be described as "hypocritic"?
> Even after adding support for CT.0, people still need >= 2.6.34.
> Where is the non-breakage for them?

Well yes, we have break at some point, but better if we break for
kernels before 2.6.34 than before 3.4 (CT.1 was added there) ;-).

So we're doing is just to trying to do our best to avoid the sure
breakage that will happen in upcoming 3.7 where NOTRACK will be gone.

There's only one single -stable branch that would break using recent
iptables + old kernel.

> (I can't say I feel /too/ bad for the RHEL folks stuck with their
> ancient 2.6.32 :-P )
> (And don't tell me about backports, because in general, they don't
> do that for NF.)

I'm mostly thinking of embedded people, that usually stick to really
old kernels.
--
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/configure.ac b/configure.ac
index 861f5b3..a45d9ab 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2,8 +2,8 @@ 
 AC_INIT([iptables], [1.4.15])
 
 # See libtool.info "Libtool's versioning system"
-libxtables_vcurrent=8
-libxtables_vage=1
+libxtables_vcurrent=9
+libxtables_vage=0
 
 AC_CONFIG_AUX_DIR([build-aux])
 AC_CONFIG_HEADERS([config.h])
diff --git a/extensions/GNUmakefile.in b/extensions/GNUmakefile.in
index 218dc3a..92ac63d 100644
--- a/extensions/GNUmakefile.in
+++ b/extensions/GNUmakefile.in
@@ -39,6 +39,7 @@  endif
 #	Wildcard module list
 #
 pfx_build_mod := $(patsubst ${srcdir}/libxt_%.c,%,$(sort $(wildcard ${srcdir}/libxt_*.c)))
+pfx_build_mod += NOTRACK
 @ENABLE_IPV4_TRUE@ pf4_build_mod := $(patsubst ${srcdir}/libipt_%.c,%,$(sort $(wildcard ${srcdir}/libipt_*.c)))
 @ENABLE_IPV6_TRUE@ pf6_build_mod := $(patsubst ${srcdir}/libip6t_%.c,%,$(sort $(wildcard ${srcdir}/libip6t_*.c)))
 pfx_build_mod := $(filter-out @blacklist_modules@,${pfx_build_mod})
@@ -100,6 +101,8 @@  lib%.oo: ${srcdir}/lib%.c
 xt_RATEEST_LIBADD   = -lm
 xt_statistic_LIBADD = -lm
 
+libxt_NOTRACK.so: libxt_CT.so
+	ln -s $< $@
 
 #
 #	Static bits
diff --git a/extensions/libxt_CT.c b/extensions/libxt_CT.c
index 27a20e2..8012a59 100644
--- a/extensions/libxt_CT.c
+++ b/extensions/libxt_CT.c
@@ -248,6 +248,13 @@  static void ct_save_v1(const void *ip, const struct xt_entry_target *target)
 		printf(" --zone %u", info->zone);
 }
 
+static void notrack_tg_init(struct xt_entry_target *target)
+{
+	struct xt_ct_target_info_v1 *info = (void *)target->data;
+
+	info->flags |= XT_CT_NOTRACK;
+}
+
 static struct xtables_target ct_target_reg[] = {
 	{
 		.family		= NFPROTO_UNSPEC,
@@ -274,6 +281,19 @@  static struct xtables_target ct_target_reg[] = {
 		.x6_parse	= ct_parse_v1,
 		.x6_options	= ct_opts_v1,
 	},
+	{
+		.family		= NFPROTO_UNSPEC,
+		.name		= "NOTRACK",
+		.revision	= 0,
+		.real_name	= "CT",
+		.real_rev	= 1,
+		.version	= XTABLES_VERSION,
+		.size		= XT_ALIGN(sizeof(struct xt_ct_target_info_v1)),
+		.userspacesize	= offsetof(struct xt_ct_target_info_v1, ct),
+		.print		= ct_print_v1,
+		.save		= ct_save_v1,
+		.init		= notrack_tg_init,
+	},
 };
 
 void _init(void)
diff --git a/extensions/libxt_NOTRACK.c b/extensions/libxt_NOTRACK.c
deleted file mode 100644
index ca58700..0000000
--- a/extensions/libxt_NOTRACK.c
+++ /dev/null
@@ -1,15 +0,0 @@ 
-/* Shared library add-on to iptables to add NOTRACK target support. */
-#include <xtables.h>
-
-static struct xtables_target notrack_target = {
-	.family		= NFPROTO_UNSPEC,
-	.name		= "NOTRACK",
-	.version	= XTABLES_VERSION,
-	.size		= XT_ALIGN(0),
-	.userspacesize	= XT_ALIGN(0),
-};
-
-void _init(void)
-{
-	xtables_register_target(&notrack_target);
-}
diff --git a/include/xtables.h.in b/include/xtables.h.in
index db69c03..7993414 100644
--- a/include/xtables.h.in
+++ b/include/xtables.h.in
@@ -280,11 +280,13 @@  struct xtables_target
 
 	struct xtables_target *next;
 
-
 	const char *name;
 
+	/* Real target behind this, if any. */
+	const char *real_name;
+
 	/* Revision of target (0 by default). */
-	u_int8_t revision;
+	u_int8_t revision, real_rev;
 
 	u_int16_t family;
 
diff --git a/iptables/ip6tables.c b/iptables/ip6tables.c
index b191d5d..f0ebe15 100644
--- a/iptables/ip6tables.c
+++ b/iptables/ip6tables.c
@@ -1286,8 +1286,17 @@  static void command_jump(struct iptables_command_state *cs)
 
 	cs->target->t = xtables_calloc(1, size);
 	cs->target->t->u.target_size = size;
-	strcpy(cs->target->t->u.user.name, cs->jumpto);
-	cs->target->t->u.user.revision = cs->target->revision;
+	if (cs->target->real_name == NULL) {
+		strcpy(cs->target->t->u.user.name, cs->jumpto);
+		cs->target->t->u.user.revision = cs->target->revision;
+	} else {
+		strcpy(cs->target->t->u.user.name, cs->target->real_name);
+		cs->target->t->u.user.revision = cs->target->real_rev;
+		fprintf(stderr, "WARNING: The %s target is obsolete. "
+		        "Use %s instead.\n",
+		        cs->jumpto, cs->target->real_name);
+	}
+
 	xs_init_target(cs->target);
 	if (cs->target->x6_options != NULL)
 		opts = xtables_options_xfrm(ip6tables_globals.orig_opts, opts,
diff --git a/iptables/iptables.c b/iptables/iptables.c
index 03ac63b..5d8698d 100644
--- a/iptables/iptables.c
+++ b/iptables/iptables.c
@@ -1295,8 +1295,18 @@  static void command_jump(struct iptables_command_state *cs)
 
 	cs->target->t = xtables_calloc(1, size);
 	cs->target->t->u.target_size = size;
-	strcpy(cs->target->t->u.user.name, cs->jumpto);
-	cs->target->t->u.user.revision = cs->target->revision;
+	if (cs->target->real_name == NULL) {
+		strcpy(cs->target->t->u.user.name, cs->jumpto);
+		cs->target->t->u.user.revision = cs->target->revision;
+	} else {
+		/* Alias support for userspace side */
+		strcpy(cs->target->t->u.user.name, cs->target->real_name);
+		cs->target->t->u.user.revision = cs->target->real_rev;
+		fprintf(stderr, "WARNING: The %s target is obsolete. "
+		        "Use %s instead.\n",
+		        cs->jumpto, cs->target->real_name);
+	}
+
 	xs_init_target(cs->target);
 
 	if (cs->target->x6_options != NULL)
diff --git a/libxtables/xtables.c b/libxtables/xtables.c
index d818579..4758ddc 100644
--- a/libxtables/xtables.c
+++ b/libxtables/xtables.c
@@ -944,6 +944,10 @@  void xtables_register_target(struct xtables_target *me)
 			xt_params->program_name, me->name);
 		exit(1);
 	}
+	if (me->real_name == NULL) {
+		me->real_name = me->name;
+		me->real_rev  = me->revision;
+	}
 
 	if (me->x6_options != NULL)
 		xtables_option_metavalidate(me->name, me->x6_options);
@@ -976,16 +980,16 @@  static void xtables_fully_register_pending_target(struct xtables_target *me)
 		}
 
 		/* Now we have two (or more) options, check compatibility. */
-		if (compatible_target_revision(old->name, old->revision)
-		    && old->revision > me->revision)
+		if (compatible_target_revision(old->real_name, old->real_rev)
+		    && old->real_rev > me->real_rev)
 			return;
 
 		/* See if new target can be used. */
-		if (!compatible_target_revision(me->name, me->revision))
+		if (!compatible_target_revision(me->real_name, me->real_rev))
 			return;
 
 		/* Prefer !AF_UNSPEC over AF_UNSPEC for same revision. */
-		if (old->revision == me->revision && me->family == AF_UNSPEC)
+		if (old->real_rev == me->real_rev && me->family == AF_UNSPEC)
 			return;
 
 		/* Delete old one. */