[PATCH net-next] Revert "ipv4: make net_protocol const"

Subsystems: networking [general], the rest · Maintainers: "David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Linus Torvalds

STALE3345d

8 messages, 4 authors, 2017-08-29 · open the first message on its own page

[PATCH net-next] Revert "ipv4: make net_protocol const"

From: David Ahern <hidden>
Date: 2017-08-28 20:23:14 

This reverts commit aa8db499ea67cff1f5f049033810ffede2fe5ae4.

Early demux structs can not be made const. Doing so results in:
[   84.967355] BUG: unable to handle kernel paging request at ffffffff81684b10
[   84.969272] IP: proc_configure_early_demux+0x1e/0x3d
[   84.970544] PGD 1a0a067
[   84.970546] P4D 1a0a067
[   84.971212] PUD 1a0b063
[   84.971733] PMD 80000000016001e1

[   84.972669] Oops: 0003 [#1] SMP
[   84.973065] Modules linked in: ip6table_filter ip6_tables veth vrf
[   84.973833] CPU: 0 PID: 955 Comm: sysctl Not tainted 4.13.0-rc6+ #22
[   84.974612] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014
[   84.975855] task: ffff88003854ce00 task.stack: ffffc900005a4000
[   84.976580] RIP: 0010:proc_configure_early_demux+0x1e/0x3d
[   84.977253] RSP: 0018:ffffc900005a7dd0 EFLAGS: 00010246
[   84.977891] RAX: ffffffff81684b10 RBX: 0000000000000001 RCX: 0000000000000000
[   84.978759] RDX: 0000000000000000 RSI: 0000000000000006 RDI: 0000000000000000
[   84.979628] RBP: ffffc900005a7dd0 R08: 0000000000000000 R09: 0000000000000000
[   84.980501] R10: 0000000000000001 R11: 0000000000000008 R12: 0000000000000001
[   84.981373] R13: ffffffffffffffea R14: ffffffff81a9b4c0 R15: 0000000000000002
[   84.982249] FS:  00007feb237b7700(0000) GS:ffff88003fc00000(0000) knlGS:0000000000000000
[   84.983231] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   84.983941] CR2: ffffffff81684b10 CR3: 0000000038492000 CR4: 00000000000406f0
[   84.984817] Call Trace:
[   84.985133]  proc_tcp_early_demux+0x29/0x30

I think this is the second time such a patch has been reverted.

Cc: Bhumika Goyal <redacted>
Signed-off-by: David Ahern <redacted>
---
Bhumika: How are you testing these constify changes? In this case a simple
sysctl -w net.ipv4.tcp_early_demux=1 would have shown the problem

 net/ipv4/af_inet.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 19aee073ba29..d678820e4306 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -1596,7 +1596,7 @@ static const struct net_protocol igmp_protocol = {
 };
 #endif
 
-static const struct net_protocol tcp_protocol = {
+static struct net_protocol tcp_protocol = {
 	.early_demux	=	tcp_v4_early_demux,
 	.early_demux_handler =  tcp_v4_early_demux,
 	.handler	=	tcp_v4_rcv,
@@ -1606,7 +1606,7 @@ static const struct net_protocol tcp_protocol = {
 	.icmp_strict_tag_validation = 1,
 };
 
-static const struct net_protocol udp_protocol = {
+static struct net_protocol udp_protocol = {
 	.early_demux =	udp_v4_early_demux,
 	.early_demux_handler =	udp_v4_early_demux,
 	.handler =	udp_rcv,
-- 
2.1.4

Re: [PATCH net-next] Revert "ipv4: make net_protocol const"

From: David Miller <davem@davemloft.net>
Date: 2017-08-28 21:01:35 

From: David Ahern <redacted>
Date: Mon, 28 Aug 2017 13:23:09 -0700

This reverts commit aa8db499ea67cff1f5f049033810ffede2fe5ae4.
...
I think this is the second time such a patch has been reverted.
Then please add a comment, it will help prevent this from happening
again.

Re: [PATCH net-next] Revert "ipv4: make net_protocol const"

From: David Ahern <hidden>
Date: 2017-08-28 21:03:36 

On 8/28/17 3:01 PM, David Miller wrote:
From: David Ahern <redacted>
Date: Mon, 28 Aug 2017 13:23:09 -0700
quoted
This reverts commit aa8db499ea67cff1f5f049033810ffede2fe5ae4.
...
quoted
I think this is the second time such a patch has been reverted.
Then please add a comment, it will help prevent this from happening
again.
Was going to do that in both places after the revert. You want it as
part of this one?

Re: [PATCH net-next] Revert "ipv4: make net_protocol const"

From: Stephen Hemminger <stephen@networkplumber.org>
Date: 2017-08-28 21:18:15 

On Mon, 28 Aug 2017 13:23:09 -0700
David Ahern [off-list ref] wrote:

This reverts commit aa8db499ea67cff1f5f049033810ffede2fe5ae4.

Early demux structs can not be made const. Doing so results in:
[   84.967355] BUG: unable to handle kernel paging request at ffffffff81684b10
[   84.969272] IP: proc_configure_early_demux+0x1e/0x3d
[   84.970544] PGD 1a0a067
[   84.970546] P4D 1a0a067
[   84.971212] PUD 1a0b063
[   84.971733] PMD 80000000016001e1

[   84.972669] Oops: 0003 [#1] SMP
[   84.973065] Modules linked in: ip6table_filter ip6_tables veth vrf
[   84.973833] CPU: 0 PID: 955 Comm: sysctl Not tainted 4.13.0-rc6+ #22
[   84.974612] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014
[   84.975855] task: ffff88003854ce00 task.stack: ffffc900005a4000
[   84.976580] RIP: 0010:proc_configure_early_demux+0x1e/0x3d
[   84.977253] RSP: 0018:ffffc900005a7dd0 EFLAGS: 00010246
[   84.977891] RAX: ffffffff81684b10 RBX: 0000000000000001 RCX: 0000000000000000
[   84.978759] RDX: 0000000000000000 RSI: 0000000000000006 RDI: 0000000000000000
[   84.979628] RBP: ffffc900005a7dd0 R08: 0000000000000000 R09: 0000000000000000
[   84.980501] R10: 0000000000000001 R11: 0000000000000008 R12: 0000000000000001
[   84.981373] R13: ffffffffffffffea R14: ffffffff81a9b4c0 R15: 0000000000000002
[   84.982249] FS:  00007feb237b7700(0000) GS:ffff88003fc00000(0000) knlGS:0000000000000000
[   84.983231] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   84.983941] CR2: ffffffff81684b10 CR3: 0000000038492000 CR4: 00000000000406f0
[   84.984817] Call Trace:
[   84.985133]  proc_tcp_early_demux+0x29/0x30

I think this is the second time such a patch has been reverted.

Cc: Bhumika Goyal <redacted>
Signed-off-by: David Ahern <redacted>
This would have been caught at compile time if you tried setting inet_protos to const.
Start with that if you want to give it a try.



diff --git a/include/net/protocol.h b/include/net/protocol.h
index 65ba335b0e7e..373fa92d33ff 100644
--- a/include/net/protocol.h
+++ b/include/net/protocol.h
@@ -93,7 +93,7 @@ struct inet_protosw {
 #define INET_PROTOSW_PERMANENT 0x02  /* Permanent protocols are unremovable. */
 #define INET_PROTOSW_ICSK      0x04  /* Is this an inet_connection_sock? */
 
-extern struct net_protocol __rcu *inet_protos[MAX_INET_PROTOS];
+extern const struct net_protocol __rcu *inet_protos[MAX_INET_PROTOS];
 extern const struct net_offload __rcu *inet_offloads[MAX_INET_PROTOS];
 extern const struct net_offload __rcu *inet6_offloads[MAX_INET_PROTOS];
 
diff --git a/net/ipv4/protocol.c b/net/ipv4/protocol.c
index 32a691b7ce2c..4b7c0ec65251 100644
--- a/net/ipv4/protocol.c
+++ b/net/ipv4/protocol.c
@@ -28,7 +28,7 @@
 #include <linux/spinlock.h>
 #include <net/protocol.h>
 
-struct net_protocol __rcu *inet_protos[MAX_INET_PROTOS] __read_mostly;
+const struct net_protocol __rcu *inet_protos[MAX_INET_PROTOS] __read_mostly;
 const struct net_offload __rcu *inet_offloads[MAX_INET_PROTOS] __read_mostly;
 EXPORT_SYMBOL(inet_offloads);


net/ipv4/sysctl_net_ipv4.c: In function ‘proc_configure_early_demux’:
net/ipv4/sysctl_net_ipv4.c:310:9: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
  ipprot = rcu_dereference(inet_protos[protocol]);

Re: [PATCH net-next] Revert "ipv4: make net_protocol const"

From: David Miller <davem@davemloft.net>
Date: 2017-08-28 21:30:58 

From: David Ahern <redacted>
Date: Mon, 28 Aug 2017 15:03:34 -0600

On 8/28/17 3:01 PM, David Miller wrote:
quoted
From: David Ahern <redacted>
Date: Mon, 28 Aug 2017 13:23:09 -0700
quoted
This reverts commit aa8db499ea67cff1f5f049033810ffede2fe5ae4.
...
quoted
I think this is the second time such a patch has been reverted.
Then please add a comment, it will help prevent this from happening
again.
Was going to do that in both places after the revert. You want it as
part of this one?
You're right a clean revert first is better.

I'll apply this now, thanks David.

Re: [PATCH net-next] Revert "ipv4: make net_protocol const"

From: Bhumika Goyal <hidden>
Date: 2017-08-29 06:46:25 

On Tue, Aug 29, 2017 at 1:53 AM, David Ahern [off-list ref] wrote:
This reverts commit aa8db499ea67cff1f5f049033810ffede2fe5ae4.

Early demux structs can not be made const. Doing so results in:
[   84.967355] BUG: unable to handle kernel paging request at ffffffff81684b10
[   84.969272] IP: proc_configure_early_demux+0x1e/0x3d
[   84.970544] PGD 1a0a067
[   84.970546] P4D 1a0a067
[   84.971212] PUD 1a0b063
[   84.971733] PMD 80000000016001e1

[   84.972669] Oops: 0003 [#1] SMP
[   84.973065] Modules linked in: ip6table_filter ip6_tables veth vrf
[   84.973833] CPU: 0 PID: 955 Comm: sysctl Not tainted 4.13.0-rc6+ #22
[   84.974612] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014
[   84.975855] task: ffff88003854ce00 task.stack: ffffc900005a4000
[   84.976580] RIP: 0010:proc_configure_early_demux+0x1e/0x3d
[   84.977253] RSP: 0018:ffffc900005a7dd0 EFLAGS: 00010246
[   84.977891] RAX: ffffffff81684b10 RBX: 0000000000000001 RCX: 0000000000000000
[   84.978759] RDX: 0000000000000000 RSI: 0000000000000006 RDI: 0000000000000000
[   84.979628] RBP: ffffc900005a7dd0 R08: 0000000000000000 R09: 0000000000000000
[   84.980501] R10: 0000000000000001 R11: 0000000000000008 R12: 0000000000000001
[   84.981373] R13: ffffffffffffffea R14: ffffffff81a9b4c0 R15: 0000000000000002
[   84.982249] FS:  00007feb237b7700(0000) GS:ffff88003fc00000(0000) knlGS:0000000000000000
[   84.983231] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   84.983941] CR2: ffffffff81684b10 CR3: 0000000038492000 CR4: 00000000000406f0
[   84.984817] Call Trace:
[   84.985133]  proc_tcp_early_demux+0x29/0x30

I think this is the second time such a patch has been reverted.

Cc: Bhumika Goyal <redacted>
Signed-off-by: David Ahern <redacted>
---
Bhumika: How are you testing these constify changes? In this case a simple
sysctl -w net.ipv4.tcp_early_demux=1 would have shown the problem
I am compile testing them. In this case I did:  make
net/ipv4/af_inet.o and it compiled. Is this error because of
typecasting net_protocol inside inet_add_protocol function?

Thanks,
Bhumika
quoted hunk
 net/ipv4/af_inet.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 19aee073ba29..d678820e4306 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -1596,7 +1596,7 @@ static const struct net_protocol igmp_protocol = {
 };
 #endif

-static const struct net_protocol tcp_protocol = {
+static struct net_protocol tcp_protocol = {
        .early_demux    =       tcp_v4_early_demux,
        .early_demux_handler =  tcp_v4_early_demux,
        .handler        =       tcp_v4_rcv,
@@ -1606,7 +1606,7 @@ static const struct net_protocol tcp_protocol = {
        .icmp_strict_tag_validation = 1,
 };

-static const struct net_protocol udp_protocol = {
+static struct net_protocol udp_protocol = {
        .early_demux =  udp_v4_early_demux,
        .early_demux_handler =  udp_v4_early_demux,
        .handler =      udp_rcv,
--
2.1.4

Re: [PATCH net-next] Revert "ipv4: make net_protocol const"

From: Stephen Hemminger <stephen@networkplumber.org>
Date: 2017-08-29 15:14:06 

On Tue, 29 Aug 2017 12:16:23 +0530
Bhumika Goyal [off-list ref] wrote:

On Tue, Aug 29, 2017 at 1:53 AM, David Ahern [off-list ref] wrote:
quoted
This reverts commit aa8db499ea67cff1f5f049033810ffede2fe5ae4.

Early demux structs can not be made const. Doing so results in:
[   84.967355] BUG: unable to handle kernel paging request at ffffffff81684b10
[   84.969272] IP: proc_configure_early_demux+0x1e/0x3d
[   84.970544] PGD 1a0a067
[   84.970546] P4D 1a0a067
[   84.971212] PUD 1a0b063
[   84.971733] PMD 80000000016001e1

[   84.972669] Oops: 0003 [#1] SMP
[   84.973065] Modules linked in: ip6table_filter ip6_tables veth vrf
[   84.973833] CPU: 0 PID: 955 Comm: sysctl Not tainted 4.13.0-rc6+ #22
[   84.974612] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014
[   84.975855] task: ffff88003854ce00 task.stack: ffffc900005a4000
[   84.976580] RIP: 0010:proc_configure_early_demux+0x1e/0x3d
[   84.977253] RSP: 0018:ffffc900005a7dd0 EFLAGS: 00010246
[   84.977891] RAX: ffffffff81684b10 RBX: 0000000000000001 RCX: 0000000000000000
[   84.978759] RDX: 0000000000000000 RSI: 0000000000000006 RDI: 0000000000000000
[   84.979628] RBP: ffffc900005a7dd0 R08: 0000000000000000 R09: 0000000000000000
[   84.980501] R10: 0000000000000001 R11: 0000000000000008 R12: 0000000000000001
[   84.981373] R13: ffffffffffffffea R14: ffffffff81a9b4c0 R15: 0000000000000002
[   84.982249] FS:  00007feb237b7700(0000) GS:ffff88003fc00000(0000) knlGS:0000000000000000
[   84.983231] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   84.983941] CR2: ffffffff81684b10 CR3: 0000000038492000 CR4: 00000000000406f0
[   84.984817] Call Trace:
[   84.985133]  proc_tcp_early_demux+0x29/0x30

I think this is the second time such a patch has been reverted.

Cc: Bhumika Goyal <redacted>
Signed-off-by: David Ahern <redacted>
---
Bhumika: How are you testing these constify changes? In this case a simple
sysctl -w net.ipv4.tcp_early_demux=1 would have shown the problem
 
I am compile testing them. In this case I did:  make
net/ipv4/af_inet.o and it compiled. Is this error because of

34 more quoted lines

You need to change inet_proto datastructure first.

Re: [PATCH net-next] Revert "ipv4: make net_protocol const"

From: David Ahern <hidden>
Date: 2017-08-29 16:47:17 

On 8/29/17 12:46 AM, Bhumika Goyal wrote:
I am compile testing them. In this case I did:  make
net/ipv4/af_inet.o and it compiled. Is this error because of
typecasting net_protocol inside inet_add_protocol function?
Compile testing is not sufficient. Marking data structures read-only
leaves potential land mines for others to trip over at run-time.

You are making these changes based on a code analysis. Extend the
analysis to elements of the structure. In this case early_demux is the
first element. There are only 9 references to early_demux and cscope
very quickly tells you it can be changed:

7 sysctl_net_ipv4.c proc_configure_early_demux  312 ipprot->early_demux
= enabled ? ipprot->early_demux_handler :
8 sysctl_net_ipv4.c proc_configure_early_demux  318 ip6prot->early_demux
= enabled ? ip6prot->early_demux_handler :
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help