Skip to contentSkip to conversation

no __initdata in netfilter?

PATCH In review Andries Brouwer 6 participants 8 messages Nov 14, 2004 – Dec 15, 2004

Subsystems (2)
Networking netfilter
lore
Share & tools
lore

Andries Brouwer

author
#0 lore raw

PATCH 1/1 no __initdata in netfilter?

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

net/ipv4/netfilter/ip_nat_rule.c +2 -2
diff -uprN -X /linux/dontdiff a/net/ipv4/netfilter/ip_nat_rule.c b/net/ipv4/netfilter/ip_nat_rule.c
@@ -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),
net/ipv4/netfilter/iptable_filter.c +2 -2
diff -uprN -X /linux/dontdiff a/net/ipv4/netfilter/iptable_filter.c b/net/ipv4/netfilter/iptable_filter.c
@@ -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),
#1 lore raw
Quoted 2 lines · Andries Brouwer

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

Where ? The initial tables are replaced by ipt_register_table.

Regards
Patrick

Quoted 31 lines

diff -uprN -X /linux/dontdiff a/net/ipv4/netfilter/ip_nat_rule.c b/net/ipv4/netfilter/ip_nat_rule.c

net/ipv4/netfilter/ip_nat_rule.c +2 -2
@@ -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

net/ipv4/netfilter/iptable_filter.c +2 -2
@@ -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),
#2 lore raw
Quoted 5 lines · Patrick McHardy
Quoted 1 line · Andries Brouwer

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

#3 lore raw
Quoted 19 lines · Andries Brouwer
Quoted 5 lines · Patrick McHardy
Quoted 1 line · Andries Brouwer

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,
...

Quoted 7 lines

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/[email protected]

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.

Quoted 1 line

Andries

Signature · 5 lines
- Harald Welte <[email protected]>             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

[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/[email protected]
we rework the code in order to not trigger some misinterpretation by static code checkers.

Signed-off-by Harald Welte <[email protected]>
include/linux/netfilter_arp/arp_tables.h +2 -4
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
@@ -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,
include/linux/netfilter_ipv4/ip_tables.h +2 -4
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
@@ -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,
include/linux/netfilter_ipv6/ip6_tables.h +2 -4
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
@@ -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,
net/ipv4/netfilter/arp_tables.c +8 -7
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
@@ -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);
net/ipv4/netfilter/arptable_filter.c +1 -2
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
@@ -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;
net/ipv4/netfilter/ip_nat_rule.c +3 -4
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
@@ -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);
net/ipv4/netfilter/ip_tables.c +7 -7
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
@@ -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;
net/ipv4/netfilter/iptable_filter.c +3 -4
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
@@ -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;
net/ipv4/netfilter/iptable_mangle.c +1 -2
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
@@ -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;
net/ipv4/netfilter/iptable_raw.c +1 -2
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
@@ -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;
net/ipv6/netfilter/ip6_tables.c +8 -7
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
@@ -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;
net/ipv6/netfilter/ip6table_filter.c +1 -2
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
@@ -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;
net/ipv6/netfilter/ip6table_mangle.c +1 -2
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
@@ -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;
net/ipv6/netfilter/ip6table_raw.c +1 -2
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
@@ -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;
#4 lore raw
Quoted 4 lines · Harald Welte

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

author
#5 lore raw
In reply to Harald Welte
Quoted 12 lines · Harald Welte
Quoted 7 lines

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/[email protected]

Quoted 7 lines

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.

Quoted 1 line

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

Not me.

Quoted 2 lines

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

#6 lore raw
Quoted 8 lines · Andries Brouwer
Quoted 3 lines

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)

Quoted 4 lines

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.

Quoted 4 lines
Quoted 2 lines

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.

Quoted 1 line

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

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

Quoted 1 line

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.

Quoted 1 line

Andries

Signature · 5 lines
- Harald Welte <[email protected]>             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
#7 lore raw
In reply to Andries Brouwer
Quoted 4 lines · Andries Brouwer

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.

Signature · 1 line
A bad analogy is like a leaky screwdriver -- Richard Braakman