* [PATCH net-next] Revert "ipv4: make net_protocol const"
@ 2017-08-28 20:23 David Ahern
2017-08-28 21:01 ` David Miller
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: David Ahern @ 2017-08-28 20:23 UTC (permalink / raw)
To: netdev, bhumirks, davem; +Cc: David Ahern
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 <bhumirks@gmail.com>
Signed-off-by: David Ahern <dsahern@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] Revert "ipv4: make net_protocol const"
2017-08-28 20:23 [PATCH net-next] Revert "ipv4: make net_protocol const" David Ahern
@ 2017-08-28 21:01 ` David Miller
2017-08-28 21:03 ` David Ahern
2017-08-28 21:18 ` Stephen Hemminger
2017-08-29 6:46 ` Bhumika Goyal
2 siblings, 1 reply; 8+ messages in thread
From: David Miller @ 2017-08-28 21:01 UTC (permalink / raw)
To: dsahern; +Cc: netdev, bhumirks
From: David Ahern <dsahern@gmail.com>
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] Revert "ipv4: make net_protocol const"
2017-08-28 21:01 ` David Miller
@ 2017-08-28 21:03 ` David Ahern
2017-08-28 21:30 ` David Miller
0 siblings, 1 reply; 8+ messages in thread
From: David Ahern @ 2017-08-28 21:03 UTC (permalink / raw)
To: David Miller; +Cc: netdev, bhumirks
On 8/28/17 3:01 PM, David Miller wrote:
> From: David Ahern <dsahern@gmail.com>
> 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.
>
Was going to do that in both places after the revert. You want it as
part of this one?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] Revert "ipv4: make net_protocol const"
2017-08-28 20:23 [PATCH net-next] Revert "ipv4: make net_protocol const" David Ahern
2017-08-28 21:01 ` David Miller
@ 2017-08-28 21:18 ` Stephen Hemminger
2017-08-29 6:46 ` Bhumika Goyal
2 siblings, 0 replies; 8+ messages in thread
From: Stephen Hemminger @ 2017-08-28 21:18 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, bhumirks, davem
On Mon, 28 Aug 2017 13:23:09 -0700
David Ahern <dsahern@gmail.com> 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 <bhumirks@gmail.com>
> Signed-off-by: David Ahern <dsahern@gmail.com>
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]);
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] Revert "ipv4: make net_protocol const"
2017-08-28 21:03 ` David Ahern
@ 2017-08-28 21:30 ` David Miller
0 siblings, 0 replies; 8+ messages in thread
From: David Miller @ 2017-08-28 21:30 UTC (permalink / raw)
To: dsahern; +Cc: netdev, bhumirks
From: David Ahern <dsahern@gmail.com>
Date: Mon, 28 Aug 2017 15:03:34 -0600
> On 8/28/17 3:01 PM, David Miller wrote:
>> From: David Ahern <dsahern@gmail.com>
>> 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.
>>
>
> 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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] Revert "ipv4: make net_protocol const"
2017-08-28 20:23 [PATCH net-next] Revert "ipv4: make net_protocol const" David Ahern
2017-08-28 21:01 ` David Miller
2017-08-28 21:18 ` Stephen Hemminger
@ 2017-08-29 6:46 ` Bhumika Goyal
2017-08-29 15:14 ` Stephen Hemminger
2017-08-29 16:47 ` David Ahern
2 siblings, 2 replies; 8+ messages in thread
From: Bhumika Goyal @ 2017-08-29 6:46 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, David Miller
On Tue, Aug 29, 2017 at 1:53 AM, David Ahern <dsahern@gmail.com> 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 <bhumirks@gmail.com>
> Signed-off-by: David Ahern <dsahern@gmail.com>
> ---
> 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
> 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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] Revert "ipv4: make net_protocol const"
2017-08-29 6:46 ` Bhumika Goyal
@ 2017-08-29 15:14 ` Stephen Hemminger
2017-08-29 16:47 ` David Ahern
1 sibling, 0 replies; 8+ messages in thread
From: Stephen Hemminger @ 2017-08-29 15:14 UTC (permalink / raw)
To: Bhumika Goyal; +Cc: David Ahern, netdev, David Miller
On Tue, 29 Aug 2017 12:16:23 +0530
Bhumika Goyal <bhumirks@gmail.com> wrote:
> On Tue, Aug 29, 2017 at 1:53 AM, David Ahern <dsahern@gmail.com> 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 <bhumirks@gmail.com>
> > Signed-off-by: David Ahern <dsahern@gmail.com>
> > ---
> > 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
>
>
> > 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
> >
You need to change inet_proto datastructure first.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] Revert "ipv4: make net_protocol const"
2017-08-29 6:46 ` Bhumika Goyal
2017-08-29 15:14 ` Stephen Hemminger
@ 2017-08-29 16:47 ` David Ahern
1 sibling, 0 replies; 8+ messages in thread
From: David Ahern @ 2017-08-29 16:47 UTC (permalink / raw)
To: Bhumika Goyal; +Cc: netdev, David Miller
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 :
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-08-29 16:47 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-28 20:23 [PATCH net-next] Revert "ipv4: make net_protocol const" David Ahern
2017-08-28 21:01 ` David Miller
2017-08-28 21:03 ` David Ahern
2017-08-28 21:30 ` David Miller
2017-08-28 21:18 ` Stephen Hemminger
2017-08-29 6:46 ` Bhumika Goyal
2017-08-29 15:14 ` Stephen Hemminger
2017-08-29 16:47 ` David Ahern
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.