[PATCH] no __initdata in netfilter?

[PATCH] no __initdata in netfilter?

[Andries Brouwer]

Stuff marked initdata that is referenced in non-init context.

diff -uprN -X /linux/dontdiff a/net/ipv4/netfilter/ip_nat_rule.c b/net/ipv4/netfilter/ip_nat_rule.c
--- a/net/ipv4/netfilter/ip_nat_rule.c	2004-10-30 21:44:11.000000000 +0200
+++ b/net/ipv4/netfilter/ip_nat_rule.c	2004-11-13 22:40:51.000000000 +0100
@@ -59,8 +59,8 @@ static struct
 	struct ipt_replace repl;
 	struct ipt_standard entries[3];
 	struct ipt_error term;
-} nat_initial_table __initdata
-= { { "nat", NAT_VALID_HOOKS, 4,
+} nat_initial_table = {
+    { "nat", NAT_VALID_HOOKS, 4,
       sizeof(struct ipt_standard) * 3 + sizeof(struct ipt_error),
       { [NF_IP_PRE_ROUTING] = 0,
 	[NF_IP_POST_ROUTING] = sizeof(struct ipt_standard),
diff -uprN -X /linux/dontdiff a/net/ipv4/netfilter/iptable_filter.c b/net/ipv4/netfilter/iptable_filter.c
--- a/net/ipv4/netfilter/iptable_filter.c	2004-10-30 21:44:11.000000000 +0200
+++ b/net/ipv4/netfilter/iptable_filter.c	2004-11-13 22:40:51.000000000 +0100
@@ -44,8 +44,8 @@ static struct
 	struct ipt_replace repl;
 	struct ipt_standard entries[3];
 	struct ipt_error term;
-} initial_table __initdata
-= { { "filter", FILTER_VALID_HOOKS, 4,
+} initial_table = {
+    { "filter", FILTER_VALID_HOOKS, 4,
       sizeof(struct ipt_standard) * 3 + sizeof(struct ipt_error),
       { [NF_IP_LOCAL_IN] = 0,
 	[NF_IP_FORWARD] = sizeof(struct ipt_standard),

Re: [netfilter-core] [PATCH] no __initdata in netfilter?

[Patrick McHardy]

Andries Brouwer wrote:

>Stuff marked initdata that is referenced in non-init context.
>
Where ? The initial tables are replaced by ipt_register_table.

Regards
Patrick

>diff -uprN -X /linux/dontdiff a/net/ipv4/netfilter/ip_nat_rule.c b/net/ipv4/netfilter/ip_nat_rule.c
>--- a/net/ipv4/netfilter/ip_nat_rule.c	2004-10-30 21:44:11.000000000 +0200
>+++ b/net/ipv4/netfilter/ip_nat_rule.c	2004-11-13 22:40:51.000000000 +0100
>@@ -59,8 +59,8 @@ static struct
> 	struct ipt_replace repl;
> 	struct ipt_standard entries[3];
> 	struct ipt_error term;
>-} nat_initial_table __initdata
>-= { { "nat", NAT_VALID_HOOKS, 4,
>+} nat_initial_table = {
>+    { "nat", NAT_VALID_HOOKS, 4,
>       sizeof(struct ipt_standard) * 3 + sizeof(struct ipt_error),
>       { [NF_IP_PRE_ROUTING] = 0,
> 	[NF_IP_POST_ROUTING] = sizeof(struct ipt_standard),
>diff -uprN -X /linux/dontdiff a/net/ipv4/netfilter/iptable_filter.c b/net/ipv4/netfilter/iptable_filter.c
>--- a/net/ipv4/netfilter/iptable_filter.c	2004-10-30 21:44:11.000000000 +0200
>+++ b/net/ipv4/netfilter/iptable_filter.c	2004-11-13 22:40:51.000000000 +0100
>@@ -44,8 +44,8 @@ static struct
> 	struct ipt_replace repl;
> 	struct ipt_standard entries[3];
> 	struct ipt_error term;
>-} initial_table __initdata
>-= { { "filter", FILTER_VALID_HOOKS, 4,
>+} initial_table = {
>+    { "filter", FILTER_VALID_HOOKS, 4,
>       sizeof(struct ipt_standard) * 3 + sizeof(struct ipt_error),
>       { [NF_IP_LOCAL_IN] = 0,
> 	[NF_IP_FORWARD] = sizeof(struct ipt_standard),
>
>  
>

Re: [netfilter-core] [PATCH] no __initdata in netfilter?

[Andries Brouwer]

On Sun, Nov 14, 2004 at 08:56:29AM +0100, Patrick McHardy wrote:
> Andries Brouwer wrote:
> 
> >Stuff marked initdata that is referenced in non-init context.
>
> Where ? The initial tables are replaced by ipt_register_table.

ip_nat_rule.c:nat_initial_table was referenced in

static struct ipt_table nat_table = {
        .table          = &nat_initial_table.repl,
	...

iptable_filter.c:initial_table was referenced in

static struct ipt_table packet_filter = {
        .table          = &initial_table.repl,
	...

This is not to say that there is a bug here, that the .init
data would actually be referenced by non-init stuff, but
it is better to convince oneself by static inspection of
the binary than by reasoning about the flow of the program.

Where the memory savings are important, the code should
be rewritten a bit.

Andries

Re: [netfilter-core] [PATCH] no __initdata in netfilter?

[Harald Welte]

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

On Sun, Nov 14, 2004 at 12:26:10PM +0100, Andries Brouwer wrote:
> On Sun, Nov 14, 2004 at 08:56:29AM +0100, Patrick McHardy wrote:
> > Andries Brouwer wrote:
> > 
> > >Stuff marked initdata that is referenced in non-init context.
> >
> > Where ? The initial tables are replaced by ipt_register_table.
> 
> ip_nat_rule.c:nat_initial_table was referenced in
> 
> static struct ipt_table nat_table = {
>         .table          = &nat_initial_table.repl,
> 	...
> 
> iptable_filter.c:initial_table was referenced in
> 
> static struct ipt_table packet_filter = {
>         .table          = &initial_table.repl,
> 	...
> 
> This is not to say that there is a bug here, that the .init
> data would actually be referenced by non-init stuff, but
> it is better to convince oneself by static inspection of
> the binary than by reasoning about the flow of the program.
> 
> Where the memory savings are important, the code should
> be rewritten a bit.

We just had a discussion, and the netfilter core team really disagrees
with this change, which apparently was merged in
http://linux.bkbits.net:8080/linux-2.5/cset@1.2055.4.50

I think it's not a good idea to waste memory by removing __initrdata,
just to make it more convenient for some static inspection tools.  This
is just the wrong way around.  

If we _know_ that it works, and there is no  bug, we could just add a
comment "This is handled correctly, since the ip_tables core copies the
data just as rulesets comming from userspace."

Pleaes pull out that change again and submit one that adds a comment, or
alternatively pick up the (incremental) change attached to this mail.  I
hope this makes your checker not spit any warnings.

Thanks.

> Andries

-- 
- Harald Welte <laforge@netfilter.org>             http://www.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 #1.2: linux-2.6.10-rc3-bk7-netfilter_initdata.patch --]
[-- Type: text/plain, Size: 17486 bytes --]


[NETFILTER] re-introduce __initdata to {arp,ip,ip6}_tables

Instead of just removing the (correct) __initdata as introduced by 
http://linux.bkbits.net:8080/linux-2.5/cset@1.2055.4.50                         
we rework the code in order to not trigger some misinterpretation by
static code checkers.

Signed-off-by: Harald Welte <laforge@netfilter.org>


diff -Nru --exclude-from /space/home/laforge/scripts/dontdiff --exclude .depend --exclude '*.o' --exclude '*.ko' --exclude '*.ver' --exclude '.*.flags' --exclude '*.orig' --exclude '*.rej' --exclude '*.cmd' --exclude '*.mod.c' --exclude '*~' linux-2.6.10-rc3-bk7/include/linux/netfilter_arp/arp_tables.h linux-2.6.10-rc3-bk7-initdata/include/linux/netfilter_arp/arp_tables.h
--- linux-2.6.10-rc3-bk7/include/linux/netfilter_arp/arp_tables.h	2004-10-18 23:53:45.000000000 +0200
+++ linux-2.6.10-rc3-bk7-initdata/include/linux/netfilter_arp/arp_tables.h	2004-12-14 12:32:06.488491120 +0100
@@ -312,9 +312,6 @@
 	/* A unique name... */
 	char name[ARPT_TABLE_MAXNAMELEN];
 
-	/* Seed table: copied in register_table */
-	struct arpt_replace *table;
-
 	/* What hooks you will enter on */
 	unsigned int valid_hooks;
 
@@ -328,7 +325,8 @@
 	struct module *me;
 };
 
-extern int arpt_register_table(struct arpt_table *table);
+extern int arpt_register_table(struct arpt_table *table,
+			       const struct arpt_replace *repl);
 extern void arpt_unregister_table(struct arpt_table *table);
 extern unsigned int arpt_do_table(struct sk_buff **pskb,
 				  unsigned int hook,
diff -Nru --exclude-from /space/home/laforge/scripts/dontdiff --exclude .depend --exclude '*.o' --exclude '*.ko' --exclude '*.ver' --exclude '.*.flags' --exclude '*.orig' --exclude '*.rej' --exclude '*.cmd' --exclude '*.mod.c' --exclude '*~' linux-2.6.10-rc3-bk7/include/linux/netfilter_ipv4/ip_tables.h linux-2.6.10-rc3-bk7-initdata/include/linux/netfilter_ipv4/ip_tables.h
--- linux-2.6.10-rc3-bk7/include/linux/netfilter_ipv4/ip_tables.h	2004-12-14 11:29:18.000000000 +0100
+++ linux-2.6.10-rc3-bk7-initdata/include/linux/netfilter_ipv4/ip_tables.h	2004-12-14 12:26:43.272627400 +0100
@@ -425,9 +425,6 @@
 	/* A unique name... */
 	char name[IPT_TABLE_MAXNAMELEN];
 
-	/* Seed table: copied in register_table */
-	struct ipt_replace *table;
-
 	/* What hooks you will enter on */
 	unsigned int valid_hooks;
 
@@ -441,7 +438,8 @@
 	struct module *me;
 };
 
-extern int ipt_register_table(struct ipt_table *table);
+extern int ipt_register_table(struct ipt_table *table,
+			      const struct ipt_replace *repl);
 extern void ipt_unregister_table(struct ipt_table *table);
 extern unsigned int ipt_do_table(struct sk_buff **pskb,
 				 unsigned int hook,
diff -Nru --exclude-from /space/home/laforge/scripts/dontdiff --exclude .depend --exclude '*.o' --exclude '*.ko' --exclude '*.ver' --exclude '.*.flags' --exclude '*.orig' --exclude '*.rej' --exclude '*.cmd' --exclude '*.mod.c' --exclude '*~' linux-2.6.10-rc3-bk7/include/linux/netfilter_ipv6/ip6_tables.h linux-2.6.10-rc3-bk7-initdata/include/linux/netfilter_ipv6/ip6_tables.h
--- linux-2.6.10-rc3-bk7/include/linux/netfilter_ipv6/ip6_tables.h	2004-12-14 11:29:18.000000000 +0100
+++ linux-2.6.10-rc3-bk7-initdata/include/linux/netfilter_ipv6/ip6_tables.h	2004-12-14 12:33:57.966543880 +0100
@@ -429,9 +429,6 @@
 	/* A unique name... */
 	char name[IP6T_TABLE_MAXNAMELEN];
 
-	/* Seed table: copied in register_table */
-	struct ip6t_replace *table;
-
 	/* What hooks you will enter on */
 	unsigned int valid_hooks;
 
@@ -445,7 +442,8 @@
 	struct module *me;
 };
 
-extern int ip6t_register_table(struct ip6t_table *table);
+extern int ip6t_register_table(struct ip6t_table *table,
+			       const struct ip6t_replace *repl);
 extern void ip6t_unregister_table(struct ip6t_table *table);
 extern unsigned int ip6t_do_table(struct sk_buff **pskb,
 				  unsigned int hook,
diff -Nru --exclude-from /space/home/laforge/scripts/dontdiff --exclude .depend --exclude '*.o' --exclude '*.ko' --exclude '*.ver' --exclude '.*.flags' --exclude '*.orig' --exclude '*.rej' --exclude '*.cmd' --exclude '*.mod.c' --exclude '*~' linux-2.6.10-rc3-bk7/net/ipv4/netfilter/arp_tables.c linux-2.6.10-rc3-bk7-initdata/net/ipv4/netfilter/arp_tables.c
--- linux-2.6.10-rc3-bk7/net/ipv4/netfilter/arp_tables.c	2004-12-14 11:29:33.000000000 +0100
+++ linux-2.6.10-rc3-bk7-initdata/net/ipv4/netfilter/arp_tables.c	2004-12-14 12:23:25.900632504 +0100
@@ -1150,7 +1150,8 @@
 	up(&arpt_mutex);
 }
 
-int arpt_register_table(struct arpt_table *table)
+int arpt_register_table(struct arpt_table *table,
+			const struct arpt_replace *repl)
 {
 	int ret;
 	struct arpt_table_info *newinfo;
@@ -1158,18 +1159,18 @@
 		= { 0, 0, 0, { 0 }, { 0 }, { } };
 
 	newinfo = vmalloc(sizeof(struct arpt_table_info)
-			  + SMP_ALIGN(table->table->size) * NR_CPUS);
+			  + SMP_ALIGN(repl->size) * NR_CPUS);
 	if (!newinfo) {
 		ret = -ENOMEM;
 		return ret;
 	}
-	memcpy(newinfo->entries, table->table->entries, table->table->size);
+	memcpy(newinfo->entries, repl->entries, repl->size);
 
 	ret = translate_table(table->name, table->valid_hooks,
-			      newinfo, table->table->size,
-			      table->table->num_entries,
-			      table->table->hook_entry,
-			      table->table->underflow);
+			      newinfo, repl->size,
+			      repl->num_entries,
+			      repl->hook_entry,
+			      repl->underflow);
 	duprintf("arpt_register_table: translate table gives %d\n", ret);
 	if (ret != 0) {
 		vfree(newinfo);
diff -Nru --exclude-from /space/home/laforge/scripts/dontdiff --exclude .depend --exclude '*.o' --exclude '*.ko' --exclude '*.ver' --exclude '.*.flags' --exclude '*.orig' --exclude '*.rej' --exclude '*.cmd' --exclude '*.mod.c' --exclude '*~' linux-2.6.10-rc3-bk7/net/ipv4/netfilter/arptable_filter.c linux-2.6.10-rc3-bk7-initdata/net/ipv4/netfilter/arptable_filter.c
--- linux-2.6.10-rc3-bk7/net/ipv4/netfilter/arptable_filter.c	2004-10-18 23:53:06.000000000 +0200
+++ linux-2.6.10-rc3-bk7-initdata/net/ipv4/netfilter/arptable_filter.c	2004-12-14 12:24:04.177813488 +0100
@@ -141,7 +141,6 @@
 
 static struct arpt_table packet_filter = {
 	.name		= "filter",
-	.table		= &initial_table.repl,
 	.valid_hooks	= FILTER_VALID_HOOKS,
 	.lock		= RW_LOCK_UNLOCKED,
 	.private	= NULL,
@@ -184,7 +183,7 @@
 	int ret, i;
 
 	/* Register table */
-	ret = arpt_register_table(&packet_filter);
+	ret = arpt_register_table(&packet_filter, &initial_table.repl);
 	if (ret < 0)
 		return ret;
 
diff -Nru --exclude-from /space/home/laforge/scripts/dontdiff --exclude .depend --exclude '*.o' --exclude '*.ko' --exclude '*.ver' --exclude '.*.flags' --exclude '*.orig' --exclude '*.rej' --exclude '*.cmd' --exclude '*.mod.c' --exclude '*~' linux-2.6.10-rc3-bk7/net/ipv4/netfilter/ip_nat_rule.c linux-2.6.10-rc3-bk7-initdata/net/ipv4/netfilter/ip_nat_rule.c
--- linux-2.6.10-rc3-bk7/net/ipv4/netfilter/ip_nat_rule.c	2004-12-14 11:29:33.000000000 +0100
+++ linux-2.6.10-rc3-bk7-initdata/net/ipv4/netfilter/ip_nat_rule.c	2004-12-14 12:30:29.584222808 +0100
@@ -59,8 +59,8 @@
 	struct ipt_replace repl;
 	struct ipt_standard entries[3];
 	struct ipt_error term;
-} nat_initial_table = {
-    { "nat", NAT_VALID_HOOKS, 4,
+} nat_initial_table __initdata
+= { { "nat", NAT_VALID_HOOKS, 4,
       sizeof(struct ipt_standard) * 3 + sizeof(struct ipt_error),
       { [NF_IP_PRE_ROUTING] = 0,
 	[NF_IP_POST_ROUTING] = sizeof(struct ipt_standard),
@@ -110,7 +110,6 @@
 
 static struct ipt_table nat_table = {
 	.name		= "nat",
-	.table		= &nat_initial_table.repl,
 	.valid_hooks	= NAT_VALID_HOOKS,
 	.lock		= RW_LOCK_UNLOCKED,
 	.me		= THIS_MODULE,
@@ -298,7 +297,7 @@
 {
 	int ret;
 
-	ret = ipt_register_table(&nat_table);
+	ret = ipt_register_table(&nat_table, &nat_initial_table.repl);
 	if (ret != 0)
 		return ret;
 	ret = ipt_register_target(&ipt_snat_reg);
diff -Nru --exclude-from /space/home/laforge/scripts/dontdiff --exclude .depend --exclude '*.o' --exclude '*.ko' --exclude '*.ver' --exclude '.*.flags' --exclude '*.orig' --exclude '*.rej' --exclude '*.cmd' --exclude '*.mod.c' --exclude '*~' linux-2.6.10-rc3-bk7/net/ipv4/netfilter/ip_tables.c linux-2.6.10-rc3-bk7-initdata/net/ipv4/netfilter/ip_tables.c
--- linux-2.6.10-rc3-bk7/net/ipv4/netfilter/ip_tables.c	2004-12-14 11:29:33.000000000 +0100
+++ linux-2.6.10-rc3-bk7-initdata/net/ipv4/netfilter/ip_tables.c	2004-12-14 12:16:39.799369280 +0100
@@ -1383,7 +1383,7 @@
 	up(&ipt_mutex);
 }
 
-int ipt_register_table(struct ipt_table *table)
+int ipt_register_table(struct ipt_table *table, const struct ipt_replace *repl)
 {
 	int ret;
 	struct ipt_table_info *newinfo;
@@ -1391,17 +1391,17 @@
 		= { 0, 0, 0, { 0 }, { 0 }, { } };
 
 	newinfo = vmalloc(sizeof(struct ipt_table_info)
-			  + SMP_ALIGN(table->table->size) * NR_CPUS);
+			  + SMP_ALIGN(repl->size) * NR_CPUS);
 	if (!newinfo)
 		return -ENOMEM;
 
-	memcpy(newinfo->entries, table->table->entries, table->table->size);
+	memcpy(newinfo->entries, repl->entries, repl->size);
 
 	ret = translate_table(table->name, table->valid_hooks,
-			      newinfo, table->table->size,
-			      table->table->num_entries,
-			      table->table->hook_entry,
-			      table->table->underflow);
+			      newinfo, repl->size,
+			      repl->num_entries,
+			      repl->hook_entry,
+			      repl->underflow);
 	if (ret != 0) {
 		vfree(newinfo);
 		return ret;
diff -Nru --exclude-from /space/home/laforge/scripts/dontdiff --exclude .depend --exclude '*.o' --exclude '*.ko' --exclude '*.ver' --exclude '.*.flags' --exclude '*.orig' --exclude '*.rej' --exclude '*.cmd' --exclude '*.mod.c' --exclude '*~' linux-2.6.10-rc3-bk7/net/ipv4/netfilter/iptable_filter.c linux-2.6.10-rc3-bk7-initdata/net/ipv4/netfilter/iptable_filter.c
--- linux-2.6.10-rc3-bk7/net/ipv4/netfilter/iptable_filter.c	2004-12-14 11:29:33.000000000 +0100
+++ linux-2.6.10-rc3-bk7-initdata/net/ipv4/netfilter/iptable_filter.c	2004-12-14 12:30:58.151879864 +0100
@@ -44,8 +44,8 @@
 	struct ipt_replace repl;
 	struct ipt_standard entries[3];
 	struct ipt_error term;
-} initial_table = {
-    { "filter", FILTER_VALID_HOOKS, 4,
+} initial_table __initdata 
+= { { "filter", FILTER_VALID_HOOKS, 4,
       sizeof(struct ipt_standard) * 3 + sizeof(struct ipt_error),
       { [NF_IP_LOCAL_IN] = 0,
 	[NF_IP_FORWARD] = sizeof(struct ipt_standard),
@@ -95,7 +95,6 @@
 
 static struct ipt_table packet_filter = {
 	.name		= "filter",
-	.table		= &initial_table.repl,
 	.valid_hooks	= FILTER_VALID_HOOKS,
 	.lock		= RW_LOCK_UNLOCKED,
 	.me		= THIS_MODULE
@@ -171,7 +170,7 @@
 	initial_table.entries[1].target.verdict = -forward - 1;
 
 	/* Register table */
-	ret = ipt_register_table(&packet_filter);
+	ret = ipt_register_table(&packet_filter, &initial_table.repl);
 	if (ret < 0)
 		return ret;
 
diff -Nru --exclude-from /space/home/laforge/scripts/dontdiff --exclude .depend --exclude '*.o' --exclude '*.ko' --exclude '*.ver' --exclude '.*.flags' --exclude '*.orig' --exclude '*.rej' --exclude '*.cmd' --exclude '*.mod.c' --exclude '*~' linux-2.6.10-rc3-bk7/net/ipv4/netfilter/iptable_mangle.c linux-2.6.10-rc3-bk7-initdata/net/ipv4/netfilter/iptable_mangle.c
--- linux-2.6.10-rc3-bk7/net/ipv4/netfilter/iptable_mangle.c	2004-10-18 23:55:06.000000000 +0200
+++ linux-2.6.10-rc3-bk7-initdata/net/ipv4/netfilter/iptable_mangle.c	2004-12-14 12:18:04.940425872 +0100
@@ -125,7 +125,6 @@
 
 static struct ipt_table packet_mangler = {
 	.name		= "mangle",
-	.table		= &initial_table.repl,
 	.valid_hooks	= MANGLE_VALID_HOOKS,
 	.lock		= RW_LOCK_UNLOCKED,
 	.me		= THIS_MODULE,
@@ -225,7 +224,7 @@
 	int ret;
 
 	/* Register table */
-	ret = ipt_register_table(&packet_mangler);
+	ret = ipt_register_table(&packet_mangler, &initial_table.repl);
 	if (ret < 0)
 		return ret;
 
diff -Nru --exclude-from /space/home/laforge/scripts/dontdiff --exclude .depend --exclude '*.o' --exclude '*.ko' --exclude '*.ver' --exclude '.*.flags' --exclude '*.orig' --exclude '*.rej' --exclude '*.cmd' --exclude '*.mod.c' --exclude '*~' linux-2.6.10-rc3-bk7/net/ipv4/netfilter/iptable_raw.c linux-2.6.10-rc3-bk7-initdata/net/ipv4/netfilter/iptable_raw.c
--- linux-2.6.10-rc3-bk7/net/ipv4/netfilter/iptable_raw.c	2004-10-18 23:55:07.000000000 +0200
+++ linux-2.6.10-rc3-bk7-initdata/net/ipv4/netfilter/iptable_raw.c	2004-12-14 12:18:34.713899616 +0100
@@ -100,7 +100,6 @@
 
 static struct ipt_table packet_raw = { 
 	.name = "raw", 
-	.table = &initial_table.repl,
 	.valid_hooks =  RAW_VALID_HOOKS, 
 	.lock = RW_LOCK_UNLOCKED, 
 	.me = THIS_MODULE
@@ -138,7 +137,7 @@
 	int ret;
 
 	/* Register table */
-	ret = ipt_register_table(&packet_raw);
+	ret = ipt_register_table(&packet_raw, &initial_table.repl);
 	if (ret < 0)
 		return ret;
 
diff -Nru --exclude-from /space/home/laforge/scripts/dontdiff --exclude .depend --exclude '*.o' --exclude '*.ko' --exclude '*.ver' --exclude '.*.flags' --exclude '*.orig' --exclude '*.rej' --exclude '*.cmd' --exclude '*.mod.c' --exclude '*~' linux-2.6.10-rc3-bk7/net/ipv6/netfilter/ip6_tables.c linux-2.6.10-rc3-bk7-initdata/net/ipv6/netfilter/ip6_tables.c
--- linux-2.6.10-rc3-bk7/net/ipv6/netfilter/ip6_tables.c	2004-12-14 11:29:34.000000000 +0100
+++ linux-2.6.10-rc3-bk7-initdata/net/ipv6/netfilter/ip6_tables.c	2004-12-14 12:20:42.441482072 +0100
@@ -1463,7 +1463,8 @@
 	up(&ip6t_mutex);
 }
 
-int ip6t_register_table(struct ip6t_table *table)
+int ip6t_register_table(struct ip6t_table *table,
+			const struct ip6t_replace *repl)
 {
 	int ret;
 	struct ip6t_table_info *newinfo;
@@ -1471,17 +1472,17 @@
 		= { 0, 0, 0, { 0 }, { 0 }, { } };
 
 	newinfo = vmalloc(sizeof(struct ip6t_table_info)
-			  + SMP_ALIGN(table->table->size) * NR_CPUS);
+			  + SMP_ALIGN(repl->size) * NR_CPUS);
 	if (!newinfo)
 		return -ENOMEM;
 
-	memcpy(newinfo->entries, table->table->entries, table->table->size);
+	memcpy(newinfo->entries, repl->entries, repl->size);
 
 	ret = translate_table(table->name, table->valid_hooks,
-			      newinfo, table->table->size,
-			      table->table->num_entries,
-			      table->table->hook_entry,
-			      table->table->underflow);
+			      newinfo, repl->size,
+			      repl->num_entries,
+			      repl->hook_entry,
+			      repl->underflow);
 	if (ret != 0) {
 		vfree(newinfo);
 		return ret;
diff -Nru --exclude-from /space/home/laforge/scripts/dontdiff --exclude .depend --exclude '*.o' --exclude '*.ko' --exclude '*.ver' --exclude '.*.flags' --exclude '*.orig' --exclude '*.rej' --exclude '*.cmd' --exclude '*.mod.c' --exclude '*~' linux-2.6.10-rc3-bk7/net/ipv6/netfilter/ip6table_filter.c linux-2.6.10-rc3-bk7-initdata/net/ipv6/netfilter/ip6table_filter.c
--- linux-2.6.10-rc3-bk7/net/ipv6/netfilter/ip6table_filter.c	2004-10-18 23:53:11.000000000 +0200
+++ linux-2.6.10-rc3-bk7-initdata/net/ipv6/netfilter/ip6table_filter.c	2004-12-14 12:21:11.749026648 +0100
@@ -94,7 +94,6 @@
 
 static struct ip6t_table packet_filter = {
 	.name		= "filter",
-	.table		= &initial_table.repl,
 	.valid_hooks	= FILTER_VALID_HOOKS,
 	.lock		= RW_LOCK_UNLOCKED,
 	.me		= THIS_MODULE,
@@ -172,7 +171,7 @@
 	initial_table.entries[1].target.verdict = -forward - 1;
 
 	/* Register table */
-	ret = ip6t_register_table(&packet_filter);
+	ret = ip6t_register_table(&packet_filter, &initial_table.repl);
 	if (ret < 0)
 		return ret;
 
diff -Nru --exclude-from /space/home/laforge/scripts/dontdiff --exclude .depend --exclude '*.o' --exclude '*.ko' --exclude '*.ver' --exclude '.*.flags' --exclude '*.orig' --exclude '*.rej' --exclude '*.cmd' --exclude '*.mod.c' --exclude '*~' linux-2.6.10-rc3-bk7/net/ipv6/netfilter/ip6table_mangle.c linux-2.6.10-rc3-bk7-initdata/net/ipv6/netfilter/ip6table_mangle.c
--- linux-2.6.10-rc3-bk7/net/ipv6/netfilter/ip6table_mangle.c	2004-10-18 23:53:51.000000000 +0200
+++ linux-2.6.10-rc3-bk7-initdata/net/ipv6/netfilter/ip6table_mangle.c	2004-12-14 12:22:18.477882320 +0100
@@ -124,7 +124,6 @@
 
 static struct ip6t_table packet_mangler = {
 	.name		= "mangle",
-	.table		= &initial_table.repl,
 	.valid_hooks	= MANGLE_VALID_HOOKS,
 	.lock		= RW_LOCK_UNLOCKED,
 	.me		= THIS_MODULE,
@@ -237,7 +236,7 @@
 	int ret;
 
 	/* Register table */
-	ret = ip6t_register_table(&packet_mangler);
+	ret = ip6t_register_table(&packet_mangler, &initial_table.repl);
 	if (ret < 0)
 		return ret;
 
diff -Nru --exclude-from /space/home/laforge/scripts/dontdiff --exclude .depend --exclude '*.o' --exclude '*.ko' --exclude '*.ver' --exclude '.*.flags' --exclude '*.orig' --exclude '*.rej' --exclude '*.cmd' --exclude '*.mod.c' --exclude '*~' linux-2.6.10-rc3-bk7/net/ipv6/netfilter/ip6table_raw.c linux-2.6.10-rc3-bk7-initdata/net/ipv6/netfilter/ip6table_raw.c
--- linux-2.6.10-rc3-bk7/net/ipv6/netfilter/ip6table_raw.c	2004-10-18 23:54:39.000000000 +0200
+++ linux-2.6.10-rc3-bk7-initdata/net/ipv6/netfilter/ip6table_raw.c	2004-12-14 12:21:51.191030552 +0100
@@ -108,7 +108,6 @@
 
 static struct ip6t_table packet_raw = { 
 	.name = "raw", 
-	.table = &initial_table.repl,
 	.valid_hooks = RAW_VALID_HOOKS, 
 	.lock = RW_LOCK_UNLOCKED, 
 	.me = THIS_MODULE
@@ -145,7 +144,7 @@
 	int ret;
 
 	/* Register table */
-	ret = ip6t_register_table(&packet_raw);
+	ret = ip6t_register_table(&packet_raw, &initial_table.repl);
 	if (ret < 0)
 		return ret;
 

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

Re: [netfilter-core] [PATCH] no __initdata in netfilter?

[Linus Torvalds]

On Tue, 14 Dec 2004, Harald Welte wrote:
> 
> Pleaes pull out that change again and submit one that adds a comment, or
> alternatively pick up the (incremental) change attached to this mail.  I
> hope this makes your checker not spit any warnings.

You guys are the ones that should do the work, and go through the proper 
channels. Do the comment, and go through Davem etc. I personally think 
that Andries' patch did the right thing, but hey, whatever. But asking 
_him_ to do something he disagrees with is just silly.

		Linus

Re: [netfilter-core] [PATCH] no __initdata in netfilter?

[Andries Brouwer]

On Tue, Dec 14, 2004 at 02:00:41PM +0100, Harald Welte wrote:

> > This is not to say that there is a bug here, that the .init
> > data would actually be referenced by non-init stuff, but
> > it is better to convince oneself by static inspection of
> > the binary than by reasoning about the flow of the program.
> > 
> > Where the memory savings are important, the code should
> > be rewritten a bit.
> 
> We just had a discussion, and the netfilter core team really disagrees
> with this change, which apparently was merged in
> http://linux.bkbits.net:8080/linux-2.5/cset@1.2055.4.50
> 
> I think it's not a good idea to waste memory by removing __initdata,
> just to make it more convenient for some static inspection tools.  This
> is just the wrong way around.
> 
> If we _know_ that it works, and there is no  bug, we could just add a
> comment "This is handled correctly, since the ip_tables core copies the
> data just as rulesets comming from userspace."

I think that argument is valid only when satisfying the static tool
is especially cumbersome or inefficient, requires ugly code, etc.
In most cases a trivial rewrite will suffice, and the result is cleaner
code, easier to maintain, fewer bugs.

You say "but today nothing is wrong". But the longer the reasoning is,
the easier one of the steps in the argument will be broken by some
trivial change. By someone who did not know about all the invariants
required by a certain piece of code.

Look

-rw-r--r--    1 aeb      aeb      26693922 Dec  3 22:33 patch-2.6.10-rc3

26 megabytes of changes. In a hundred trivial changes there is at least
one flaw. These 26 megabytes will again introduce lots of minor problems.
The more of these can be found automatically, by static analysis, the
more time is left for debugging serious stuff.

> Pleaes pull out that change again and submit one that adds a comment,

Not me.

> or alternatively pick up the (incremental) change attached to this mail.
> I hope this makes your checker not spit any warnings.

I checked, and indeed, no warnings for this patch.
But that is the only thing I checked. I would never submit it.
Probably you should send it to davem and see whether he likes it.

Andries

Re: [netfilter-core] [PATCH] no __initdata in netfilter?

[Harald Welte]

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

On Tue, Dec 14, 2004 at 07:39:11PM +0100, Andries Brouwer wrote:
> > If we _know_ that it works, and there is no  bug, we could just add a
> > comment "This is handled correctly, since the ip_tables core copies the
> > data just as rulesets comming from userspace."
> 
> I think that argument is valid only when satisfying the static tool
> is especially cumbersome or inefficient, requires ugly code, etc.
> In most cases a trivial rewrite will suffice, and the result is cleaner
> code, easier to maintain, fewer bugs.
 
yes, and that rewrite is what I did with the patch.  Not really trivial,
but a small change.

Also, you only removed __initdata from ip_tables, but not arp_tables and
ip6_tables (which should have hit the same trigger in the tool)

> You say "but today nothing is wrong". But the longer the reasoning is,
> the easier one of the steps in the argument will be broken by some
> trivial change. By someone who did not know about all the invariants
> required by a certain piece of code.

Yes, but we're not talking about VM or filesystems, but a specific
function used by ip_tables internally.  

I understand your point, though I think this is one of the cases where
your arguments apply the least. OTOTH, the __initdata structures we're
talking about are fairly big, space that you probably won't waste on
your embedded dsl router/firewall.

> > or alternatively pick up the (incremental) change attached to this mail.
> > I hope this makes your checker not spit any warnings.
> 
> I checked, and indeed, no warnings for this patch.

great.

> But that is the only thing I checked. I would never submit it.

I checked that it compiles ;)  The changes are fairly trivial.

> Probably you should send it to davem and see whether he likes it.

We'll include it with the next patchset that goes to davem for mainline
inclusion.

> Andries

-- 
- Harald Welte <laforge@netfilter.org>             http://www.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: [netfilter-core] [PATCH] no __initdata in netfilter?

[Rusty Russell]

On Tue, 2004-12-14 at 19:39 +0100, Andries Brouwer wrote:
> I think that argument is valid only when satisfying the static tool
> is especially cumbersome or inefficient, requires ugly code, etc.
> In most cases a trivial rewrite will suffice, and the result is cleaner
> code, easier to maintain, fewer bugs.

Exactly, and Harald just handed you that trivial rewrite.  It's clearer
than before, and shouldn't trip your static checker.

Your patch just ripped out the __initdata, making it suboptimal
*without* making the code clearer.

Rusty.
-- 
A bad analogy is like a leaky screwdriver -- Richard Braakman