* [PATCH] netconsole: Initialize after all core networking drivers @ 2015-12-17 23:52 Calvin Owens 2015-12-18 1:08 ` Eric Dumazet 2015-12-18 1:10 ` Stephen Hemminger 0 siblings, 2 replies; 11+ messages in thread From: Calvin Owens @ 2015-12-17 23:52 UTC (permalink / raw) To: davem, shm, izumi.taku, linville, dsa Cc: netdev, linux-kernel, kernel-team, calvinowens With built-in netconsole and IXGBE, configuring netconsole via the kernel cmdline results in the following panic at boot: netpoll: netconsole: device eth0 not up yet, forcing it usb 2-1: new high-speed USB device number 2 using ehci-pci ixgbe 0000:03:00.0: registered PHC device on eth0 BUG: unable to handle kernel NULL pointer dereference at 0000000000000810 <snip> Call Trace: [<ffffffff81578661>] ? vxlan_get_rx_port+0x41/0xa0 [<ffffffff81586828>] ixgbe_open+0x4e8/0x540 [<ffffffff8168045c>] __dev_open+0xac/0x120 [<ffffffff81680506>] dev_open+0x36/0x70 [<ffffffff8169abec>] netpoll_setup+0x23c/0x300 [<ffffffff8169a66a>] ? netpoll_parse_options+0x19a/0x200 [<ffffffff81d797a8>] ? option_setup+0x1f/0x1f [<ffffffff81d79882>] init_netconsole+0xda/0x262 [<ffffffff81d797a8>] ? option_setup+0x1f/0x1f [<ffffffff810003a8>] do_one_initcall+0x88/0x1b0 [<ffffffff81d31144>] kernel_init_freeable+0x14a/0x1e3 [<ffffffff81d308f1>] ? do_early_param+0x8c/0x8c [<ffffffff81778610>] ? rest_init+0x80/0x80 [<ffffffff8177861e>] kernel_init+0xe/0xe0 [<ffffffff8177dc5f>] ret_from_fork+0x3f/0x70 [<ffffffff81778610>] ? rest_init+0x80/0x80 This happens because IXGBE assumes that vxlan has already been initialized. The cleanest way to fix this is to just initialize netconsole after all the other core networking stuff has completed. Signed-off-by: Calvin Owens <calvinowens@fb.com> --- drivers/net/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/Makefile b/drivers/net/Makefile index 900b0c5..31557d0 100644 --- a/drivers/net/Makefile +++ b/drivers/net/Makefile @@ -15,7 +15,6 @@ obj-$(CONFIG_MACVTAP) += macvtap.o obj-$(CONFIG_MII) += mii.o obj-$(CONFIG_MDIO) += mdio.o obj-$(CONFIG_NET) += Space.o loopback.o -obj-$(CONFIG_NETCONSOLE) += netconsole.o obj-$(CONFIG_PHYLIB) += phy/ obj-$(CONFIG_RIONET) += rionet.o obj-$(CONFIG_NET_TEAM) += team/ @@ -26,6 +25,7 @@ obj-$(CONFIG_VXLAN) += vxlan.o obj-$(CONFIG_GENEVE) += geneve.o obj-$(CONFIG_NLMON) += nlmon.o obj-$(CONFIG_NET_VRF) += vrf.o +obj-$(CONFIG_NETCONSOLE) += netconsole.o # # Networking Drivers -- 2.5.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] netconsole: Initialize after all core networking drivers 2015-12-17 23:52 [PATCH] netconsole: Initialize after all core networking drivers Calvin Owens @ 2015-12-18 1:08 ` Eric Dumazet 2015-12-18 1:46 ` Calvin Owens 2015-12-18 1:10 ` Stephen Hemminger 1 sibling, 1 reply; 11+ messages in thread From: Eric Dumazet @ 2015-12-18 1:08 UTC (permalink / raw) To: Calvin Owens Cc: davem, shm, izumi.taku, linville, dsa, netdev, linux-kernel, kernel-team On Thu, 2015-12-17 at 15:52 -0800, Calvin Owens wrote: > With built-in netconsole and IXGBE, configuring netconsole via the kernel > cmdline results in the following panic at boot: > > netpoll: netconsole: device eth0 not up yet, forcing it > usb 2-1: new high-speed USB device number 2 using ehci-pci > ixgbe 0000:03:00.0: registered PHC device on eth0 > BUG: unable to handle kernel NULL pointer dereference at 0000000000000810 > <snip> > Call Trace: > [<ffffffff81578661>] ? vxlan_get_rx_port+0x41/0xa0 > [<ffffffff81586828>] ixgbe_open+0x4e8/0x540 > [<ffffffff8168045c>] __dev_open+0xac/0x120 > [<ffffffff81680506>] dev_open+0x36/0x70 > [<ffffffff8169abec>] netpoll_setup+0x23c/0x300 > [<ffffffff8169a66a>] ? netpoll_parse_options+0x19a/0x200 > [<ffffffff81d797a8>] ? option_setup+0x1f/0x1f > [<ffffffff81d79882>] init_netconsole+0xda/0x262 > [<ffffffff81d797a8>] ? option_setup+0x1f/0x1f > [<ffffffff810003a8>] do_one_initcall+0x88/0x1b0 > [<ffffffff81d31144>] kernel_init_freeable+0x14a/0x1e3 > [<ffffffff81d308f1>] ? do_early_param+0x8c/0x8c > [<ffffffff81778610>] ? rest_init+0x80/0x80 > [<ffffffff8177861e>] kernel_init+0xe/0xe0 > [<ffffffff8177dc5f>] ret_from_fork+0x3f/0x70 > [<ffffffff81778610>] ? rest_init+0x80/0x80 > > This happens because IXGBE assumes that vxlan has already been initialized. > The cleanest way to fix this is to just initialize netconsole after all the > other core networking stuff has completed. > > Signed-off-by: Calvin Owens <calvinowens@fb.com> > --- > drivers/net/Makefile | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/Makefile b/drivers/net/Makefile > index 900b0c5..31557d0 100644 > --- a/drivers/net/Makefile > +++ b/drivers/net/Makefile > @@ -15,7 +15,6 @@ obj-$(CONFIG_MACVTAP) += macvtap.o > obj-$(CONFIG_MII) += mii.o > obj-$(CONFIG_MDIO) += mdio.o > obj-$(CONFIG_NET) += Space.o loopback.o > -obj-$(CONFIG_NETCONSOLE) += netconsole.o > obj-$(CONFIG_PHYLIB) += phy/ > obj-$(CONFIG_RIONET) += rionet.o > obj-$(CONFIG_NET_TEAM) += team/ > @@ -26,6 +25,7 @@ obj-$(CONFIG_VXLAN) += vxlan.o > obj-$(CONFIG_GENEVE) += geneve.o > obj-$(CONFIG_NLMON) += nlmon.o > obj-$(CONFIG_NET_VRF) += vrf.o > +obj-$(CONFIG_NETCONSOLE) += netconsole.o > > # > # Networking Drivers Looks odd to rely on link order, but we might already rely on this... Have you considered using device_initcall() instead of late_initcall() in vxlan ? In any case, a comment would really be good to avoid future mistakes. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] netconsole: Initialize after all core networking drivers 2015-12-18 1:08 ` Eric Dumazet @ 2015-12-18 1:46 ` Calvin Owens 2015-12-23 23:03 ` Calvin Owens 0 siblings, 1 reply; 11+ messages in thread From: Calvin Owens @ 2015-12-18 1:46 UTC (permalink / raw) To: Eric Dumazet Cc: davem, shm, izumi.taku, linville, dsa, netdev, linux-kernel, kernel-team On Thursday 12/17 at 17:08 -0800, Eric Dumazet wrote: > On Thu, 2015-12-17 at 15:52 -0800, Calvin Owens wrote: > > With built-in netconsole and IXGBE, configuring netconsole via the kernel > > cmdline results in the following panic at boot: > > > > netpoll: netconsole: device eth0 not up yet, forcing it > > usb 2-1: new high-speed USB device number 2 using ehci-pci > > ixgbe 0000:03:00.0: registered PHC device on eth0 > > BUG: unable to handle kernel NULL pointer dereference at 0000000000000810 > > <snip> > > Call Trace: > > [<ffffffff81578661>] ? vxlan_get_rx_port+0x41/0xa0 > > [<ffffffff81586828>] ixgbe_open+0x4e8/0x540 > > [<ffffffff8168045c>] __dev_open+0xac/0x120 > > [<ffffffff81680506>] dev_open+0x36/0x70 > > [<ffffffff8169abec>] netpoll_setup+0x23c/0x300 > > [<ffffffff8169a66a>] ? netpoll_parse_options+0x19a/0x200 > > [<ffffffff81d797a8>] ? option_setup+0x1f/0x1f > > [<ffffffff81d79882>] init_netconsole+0xda/0x262 > > [<ffffffff81d797a8>] ? option_setup+0x1f/0x1f > > [<ffffffff810003a8>] do_one_initcall+0x88/0x1b0 > > [<ffffffff81d31144>] kernel_init_freeable+0x14a/0x1e3 > > [<ffffffff81d308f1>] ? do_early_param+0x8c/0x8c > > [<ffffffff81778610>] ? rest_init+0x80/0x80 > > [<ffffffff8177861e>] kernel_init+0xe/0xe0 > > [<ffffffff8177dc5f>] ret_from_fork+0x3f/0x70 > > [<ffffffff81778610>] ? rest_init+0x80/0x80 > > > > This happens because IXGBE assumes that vxlan has already been initialized. > > The cleanest way to fix this is to just initialize netconsole after all the > > other core networking stuff has completed. > > > > Signed-off-by: Calvin Owens <calvinowens@fb.com> > > --- > > drivers/net/Makefile | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/Makefile b/drivers/net/Makefile > > index 900b0c5..31557d0 100644 > > --- a/drivers/net/Makefile > > +++ b/drivers/net/Makefile > > @@ -15,7 +15,6 @@ obj-$(CONFIG_MACVTAP) += macvtap.o > > obj-$(CONFIG_MII) += mii.o > > obj-$(CONFIG_MDIO) += mdio.o > > obj-$(CONFIG_NET) += Space.o loopback.o > > -obj-$(CONFIG_NETCONSOLE) += netconsole.o > > obj-$(CONFIG_PHYLIB) += phy/ > > obj-$(CONFIG_RIONET) += rionet.o > > obj-$(CONFIG_NET_TEAM) += team/ > > @@ -26,6 +25,7 @@ obj-$(CONFIG_VXLAN) += vxlan.o > > obj-$(CONFIG_GENEVE) += geneve.o > > obj-$(CONFIG_NLMON) += nlmon.o > > obj-$(CONFIG_NET_VRF) += vrf.o > > +obj-$(CONFIG_NETCONSOLE) += netconsole.o > > > > # > > # Networking Drivers > > > Looks odd to rely on link order, but we might already rely on this... > > Have you considered using device_initcall() instead of late_initcall() > in vxlan ? I'll look. As-is though, I think a similar problem would happen if you tried to use a virtio_net device with netconsole= cmdline (although that is admittedly a bizarre use case). The Makefile patch seemed like the best way to ensure this can't recur elsewhere. > In any case, a comment would really be good to avoid future mistakes. Good point, I'll add something. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] netconsole: Initialize after all core networking drivers 2015-12-18 1:46 ` Calvin Owens @ 2015-12-23 23:03 ` Calvin Owens 2015-12-24 6:54 ` [PATCH] vxlan: Properly depend on ipv6 and revert to module_init() kbuild test robot ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Calvin Owens @ 2015-12-23 23:03 UTC (permalink / raw) To: Eric Dumazet Cc: davem, shm, izumi.taku, linville, dsa, netdev, linux-kernel, kernel-team, Cong Wang On Thursday 12/17 at 17:46 -0800, Calvin Owens wrote: > On Thursday 12/17 at 17:08 -0800, Eric Dumazet wrote: > > On Thu, 2015-12-17 at 15:52 -0800, Calvin Owens wrote: > > > With built-in netconsole and IXGBE, configuring netconsole via the kernel > > > cmdline results in the following panic at boot: > > > > > > netpoll: netconsole: device eth0 not up yet, forcing it > > > usb 2-1: new high-speed USB device number 2 using ehci-pci > > > ixgbe 0000:03:00.0: registered PHC device on eth0 > > > BUG: unable to handle kernel NULL pointer dereference at 0000000000000810 > > > <snip> > > > Call Trace: > > > [<ffffffff81578661>] ? vxlan_get_rx_port+0x41/0xa0 > > > [<ffffffff81586828>] ixgbe_open+0x4e8/0x540 > > > [<ffffffff8168045c>] __dev_open+0xac/0x120 > > > [<ffffffff81680506>] dev_open+0x36/0x70 > > > [<ffffffff8169abec>] netpoll_setup+0x23c/0x300 > > > [<ffffffff8169a66a>] ? netpoll_parse_options+0x19a/0x200 > > > [<ffffffff81d797a8>] ? option_setup+0x1f/0x1f > > > [<ffffffff81d79882>] init_netconsole+0xda/0x262 > > > [<ffffffff81d797a8>] ? option_setup+0x1f/0x1f > > > [<ffffffff810003a8>] do_one_initcall+0x88/0x1b0 > > > [<ffffffff81d31144>] kernel_init_freeable+0x14a/0x1e3 > > > [<ffffffff81d308f1>] ? do_early_param+0x8c/0x8c > > > [<ffffffff81778610>] ? rest_init+0x80/0x80 > > > [<ffffffff8177861e>] kernel_init+0xe/0xe0 > > > [<ffffffff8177dc5f>] ret_from_fork+0x3f/0x70 > > > [<ffffffff81778610>] ? rest_init+0x80/0x80 > > > > > > This happens because IXGBE assumes that vxlan has already been initialized. > > > The cleanest way to fix this is to just initialize netconsole after all the > > > other core networking stuff has completed. > > > > > > Signed-off-by: Calvin Owens <calvinowens@fb.com> > > > --- > > > drivers/net/Makefile | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/Makefile b/drivers/net/Makefile > > > index 900b0c5..31557d0 100644 > > > --- a/drivers/net/Makefile > > > +++ b/drivers/net/Makefile > > > @@ -15,7 +15,6 @@ obj-$(CONFIG_MACVTAP) += macvtap.o > > > obj-$(CONFIG_MII) += mii.o > > > obj-$(CONFIG_MDIO) += mdio.o > > > obj-$(CONFIG_NET) += Space.o loopback.o > > > -obj-$(CONFIG_NETCONSOLE) += netconsole.o > > > obj-$(CONFIG_PHYLIB) += phy/ > > > obj-$(CONFIG_RIONET) += rionet.o > > > obj-$(CONFIG_NET_TEAM) += team/ > > > @@ -26,6 +25,7 @@ obj-$(CONFIG_VXLAN) += vxlan.o > > > obj-$(CONFIG_GENEVE) += geneve.o > > > obj-$(CONFIG_NLMON) += nlmon.o > > > obj-$(CONFIG_NET_VRF) += vrf.o > > > +obj-$(CONFIG_NETCONSOLE) += netconsole.o > > > > > > # > > > # Networking Drivers > > > > > > Looks odd to rely on link order, but we might already rely on this... > > > > Have you considered using device_initcall() instead of late_initcall() > > in vxlan ? > > I'll look. So this does work, but commit 7332a13b038be05c explicitly changed it to late_initcall() because of dependencies on IPv6: When vxlan is compiled as builtin, its init code runs before IPv6 init, this could cause problems if we create IPv6 socket in the latter patch. So I guess something like the following patch is needed to go that route? It's ugly, IMHO the Makefile patch is cleaner... Stephen / Cong, what do you think? > As-is though, I think a similar problem would happen if you > tried to use a virtio_net device with netconsole= cmdline (although that > is admittedly a bizarre use case). The Makefile patch seemed like the > best way to ensure this can't recur elsewhere. I misunderstood this, it works fine as is. ---8<--- From: Calvin Owens <calvinowens@fb.com> Subject: [PATCH] vxlan: Properly depend on ipv6 and revert to module_init() Commit 7332a13b038be05c ("vxlan: defer vxlan init as late as possible") changed vxlan to use late_initcall(), because vxlan relies on ipv6 being loaded when a new device is opened. This causes netconsole to panic at boot when configured via the kernel cmdline on an IXGBE NIC, because ixgbe_open() assumes that vxlan has already been initialized: netpoll: netconsole: device eth0 not up yet, forcing it ixgbe 0000:03:00.0: registered PHC device on eth0 BUG: unable to handle kernel NULL pointer dereference at 0000000000000810 <snip> Call Trace: [<ffffffff81578661>] ? vxlan_get_rx_port+0x41/0xa0 [<ffffffff81586828>] ixgbe_open+0x4e8/0x540 [<ffffffff8168045c>] __dev_open+0xac/0x120 [<ffffffff81680506>] dev_open+0x36/0x70 [<ffffffff8169abec>] netpoll_setup+0x23c/0x300 [<ffffffff8169a66a>] ? netpoll_parse_options+0x19a/0x200 [<ffffffff81d797a8>] ? option_setup+0x1f/0x1f [<ffffffff81d79882>] init_netconsole+0xda/0x262 [<ffffffff81d797a8>] ? option_setup+0x1f/0x1f [<ffffffff810003a8>] do_one_initcall+0x88/0x1b0 [<ffffffff81d31144>] kernel_init_freeable+0x14a/0x1e3 [<ffffffff81d308f1>] ? do_early_param+0x8c/0x8c [<ffffffff81778610>] ? rest_init+0x80/0x80 [<ffffffff8177861e>] kernel_init+0xe/0xe0 [<ffffffff8177dc5f>] ret_from_fork+0x3f/0x70 [<ffffffff81778610>] ? rest_init+0x80/0x80 This patch addresses the issue cited in 7332a13b038be05c by making vxlan actually check if ipv6 is loaded, and reverts it to module_init() so that it becomes device_initcall() when built-in, eliminating the netconsole issue. The ipv6 module is permanent, so there's no need to actually do the usual module_get/module_put dance: once we find it loaded, we can just assume that it always will be. AFAICS, nothing actually ends up calling vxlan_open() during initcalls, so in the (IPV6=y && VXLAN=y) case we can't end up there before ipv6 has initialized. Signed-off-by: Calvin Owens <calvinowens@fb.com> --- drivers/net/vxlan.c | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c index ba363ce..e3061b7 100644 --- a/drivers/net/vxlan.c +++ b/drivers/net/vxlan.c @@ -158,6 +158,36 @@ static int vxlan_nla_put_addr(struct sk_buff *skb, int attr, return nla_put_in_addr(skb, attr, ip->sin.sin_addr.s_addr); } +#if IS_MODULE(CONFIG_IPV6) +MODULE_SOFTDEP("pre: ipv6"); + +/* + * IPv6 is permanent, so once we find it loaded we can just assume it always + * will be, and avoid walking the module_list every time we open a new vxlan + * device. + */ +static bool vxlan_ipv6_is_loaded(void) +{ + static bool ipv6_loaded = false; + + if (!ipv6_loaded) { + mutex_lock(&module_mutex); + ipv6_loaded = !!find_module("ipv6"); + mutex_unlock(&module_mutex); + } + + return ipv6_loaded; +} + +#elif IS_BUILTIN(CONFIG_IPV6) + +static bool vxlan_ipv6_is_loaded(void) +{ + return true; +} + +#endif /* IS_{MODULE,BUILTIN}(CONFIG_IPV6) */ + #else /* !CONFIG_IPV6 */ static inline @@ -2628,6 +2658,9 @@ static struct socket *vxlan_create_sock(struct net *net, bool ipv6, memset(&udp_conf, 0, sizeof(udp_conf)); if (ipv6) { + if (!vxlan_ipv6_is_loaded()) + return ERR_PTR(-EAFNOSUPPORT); + udp_conf.family = AF_INET6; udp_conf.use_udp6_rx_checksums = !(flags & VXLAN_F_UDP_ZERO_CSUM6_RX); @@ -3259,7 +3292,7 @@ out1: destroy_workqueue(vxlan_wq); return rc; } -late_initcall(vxlan_init_module); +module_init(vxlan_init_module); static void __exit vxlan_cleanup_module(void) { -- 2.5.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] vxlan: Properly depend on ipv6 and revert to module_init() 2015-12-23 23:03 ` Calvin Owens @ 2015-12-24 6:54 ` kbuild test robot 2015-12-24 10:25 ` [PATCH] netconsole: Initialize after all core networking drivers Hannes Frederic Sowa 2015-12-28 23:24 ` Cong Wang 2 siblings, 0 replies; 11+ messages in thread From: kbuild test robot @ 2015-12-24 6:54 UTC (permalink / raw) To: Calvin Owens Cc: kbuild-all, Eric Dumazet, davem, shm, izumi.taku, linville, dsa, netdev, linux-kernel, kernel-team, Cong Wang [-- Attachment #1: Type: text/plain, Size: 1628 bytes --] Hi Calvin, [auto build test ERROR on net-next/master] [also build test ERROR on v4.4-rc6 next-20151223] url: https://github.com/0day-ci/linux/commits/Calvin-Owens/vxlan-Properly-depend-on-ipv6-and-revert-to-module_init/20151224-070557 config: i386-randconfig-b0-12241357 (attached as .config) reproduce: # save the attached .config to linux build tree make ARCH=i386 All error/warnings (new ones prefixed by >>): In file included from include/linux/linkage.h:4:0, from include/linux/kernel.h:6, from drivers/net/vxlan.c:13: drivers/net/vxlan.c: In function 'vxlan_create_sock': >> drivers/net/vxlan.c:2661:8: error: implicit declaration of function 'vxlan_ipv6_is_loaded' [-Werror=implicit-function-declaration] if (!vxlan_ipv6_is_loaded()) ^ include/linux/compiler.h:147:28: note: in definition of macro '__trace_if' if (__builtin_constant_p((cond)) ? !!(cond) : \ ^ >> drivers/net/vxlan.c:2661:3: note: in expansion of macro 'if' if (!vxlan_ipv6_is_loaded()) ^ cc1: some warnings being treated as errors vim +/vxlan_ipv6_is_loaded +2661 drivers/net/vxlan.c 2655 struct udp_port_cfg udp_conf; 2656 int err; 2657 2658 memset(&udp_conf, 0, sizeof(udp_conf)); 2659 2660 if (ipv6) { > 2661 if (!vxlan_ipv6_is_loaded()) 2662 return ERR_PTR(-EAFNOSUPPORT); 2663 2664 udp_conf.family = AF_INET6; --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/octet-stream, Size: 26768 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] netconsole: Initialize after all core networking drivers 2015-12-23 23:03 ` Calvin Owens 2015-12-24 6:54 ` [PATCH] vxlan: Properly depend on ipv6 and revert to module_init() kbuild test robot @ 2015-12-24 10:25 ` Hannes Frederic Sowa 2015-12-28 23:21 ` Cong Wang 2015-12-28 23:24 ` Cong Wang 2 siblings, 1 reply; 11+ messages in thread From: Hannes Frederic Sowa @ 2015-12-24 10:25 UTC (permalink / raw) To: Calvin Owens, Eric Dumazet Cc: davem, shm, izumi.taku, linville, dsa, netdev, linux-kernel, kernel-team, Cong Wang Hi, On 24.12.2015 00:03, Calvin Owens wrote: > On Thursday 12/17 at 17:46 -0800, Calvin Owens wrote: >> On Thursday 12/17 at 17:08 -0800, Eric Dumazet wrote: >>> On Thu, 2015-12-17 at 15:52 -0800, Calvin Owens wrote: >>>> With built-in netconsole and IXGBE, configuring netconsole via the kernel >>>> cmdline results in the following panic at boot: >>>> >>>> netpoll: netconsole: device eth0 not up yet, forcing it >>>> usb 2-1: new high-speed USB device number 2 using ehci-pci >>>> ixgbe 0000:03:00.0: registered PHC device on eth0 >>>> BUG: unable to handle kernel NULL pointer dereference at 0000000000000810 >>>> <snip> >>>> Call Trace: >>>> [<ffffffff81578661>] ? vxlan_get_rx_port+0x41/0xa0 >>>> [<ffffffff81586828>] ixgbe_open+0x4e8/0x540 >>>> [<ffffffff8168045c>] __dev_open+0xac/0x120 >>>> [<ffffffff81680506>] dev_open+0x36/0x70 >>>> [<ffffffff8169abec>] netpoll_setup+0x23c/0x300 >>>> [<ffffffff8169a66a>] ? netpoll_parse_options+0x19a/0x200 >>>> [<ffffffff81d797a8>] ? option_setup+0x1f/0x1f >>>> [<ffffffff81d79882>] init_netconsole+0xda/0x262 >>>> [<ffffffff81d797a8>] ? option_setup+0x1f/0x1f >>>> [<ffffffff810003a8>] do_one_initcall+0x88/0x1b0 >>>> [<ffffffff81d31144>] kernel_init_freeable+0x14a/0x1e3 >>>> [<ffffffff81d308f1>] ? do_early_param+0x8c/0x8c >>>> [<ffffffff81778610>] ? rest_init+0x80/0x80 >>>> [<ffffffff8177861e>] kernel_init+0xe/0xe0 >>>> [<ffffffff8177dc5f>] ret_from_fork+0x3f/0x70 >>>> [<ffffffff81778610>] ? rest_init+0x80/0x80 >>>> >>>> This happens because IXGBE assumes that vxlan has already been initialized. >>>> The cleanest way to fix this is to just initialize netconsole after all the >>>> other core networking stuff has completed. >>>> >>>> Signed-off-by: Calvin Owens <calvinowens@fb.com> >>>> --- >>>> drivers/net/Makefile | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/net/Makefile b/drivers/net/Makefile >>>> index 900b0c5..31557d0 100644 >>>> --- a/drivers/net/Makefile >>>> +++ b/drivers/net/Makefile >>>> @@ -15,7 +15,6 @@ obj-$(CONFIG_MACVTAP) += macvtap.o >>>> obj-$(CONFIG_MII) += mii.o >>>> obj-$(CONFIG_MDIO) += mdio.o >>>> obj-$(CONFIG_NET) += Space.o loopback.o >>>> -obj-$(CONFIG_NETCONSOLE) += netconsole.o >>>> obj-$(CONFIG_PHYLIB) += phy/ >>>> obj-$(CONFIG_RIONET) += rionet.o >>>> obj-$(CONFIG_NET_TEAM) += team/ >>>> @@ -26,6 +25,7 @@ obj-$(CONFIG_VXLAN) += vxlan.o >>>> obj-$(CONFIG_GENEVE) += geneve.o >>>> obj-$(CONFIG_NLMON) += nlmon.o >>>> obj-$(CONFIG_NET_VRF) += vrf.o >>>> +obj-$(CONFIG_NETCONSOLE) += netconsole.o >>>> >>>> # >>>> # Networking Drivers >>> >>> >>> Looks odd to rely on link order, but we might already rely on this... >>> >>> Have you considered using device_initcall() instead of late_initcall() >>> in vxlan ? >> >> I'll look. > > So this does work, but commit 7332a13b038be05c explicitly changed it to > late_initcall() because of dependencies on IPv6: > > When vxlan is compiled as builtin, its init code > runs before IPv6 init, this could cause problems > if we create IPv6 socket in the latter patch. > > So I guess something like the following patch is needed to go that > route? It's ugly, IMHO the Makefile patch is cleaner... > > Stephen / Cong, what do you think? > >> As-is though, I think a similar problem would happen if you >> tried to use a virtio_net device with netconsole= cmdline (although that >> is admittedly a bizarre use case). The Makefile patch seemed like the >> best way to ensure this can't recur elsewhere. > > I misunderstood this, it works fine as is. > > > ---8<--- > From: Calvin Owens <calvinowens@fb.com> > Subject: [PATCH] vxlan: Properly depend on ipv6 and revert to module_init() > > Commit 7332a13b038be05c ("vxlan: defer vxlan init as late as possible") > changed vxlan to use late_initcall(), because vxlan relies on ipv6 being > loaded when a new device is opened. > > This causes netconsole to panic at boot when configured via the kernel > cmdline on an IXGBE NIC, because ixgbe_open() assumes that vxlan has > already been initialized: > > netpoll: netconsole: device eth0 not up yet, forcing it > ixgbe 0000:03:00.0: registered PHC device on eth0 > BUG: unable to handle kernel NULL pointer dereference at 0000000000000810 > <snip> > Call Trace: > [<ffffffff81578661>] ? vxlan_get_rx_port+0x41/0xa0 > [<ffffffff81586828>] ixgbe_open+0x4e8/0x540 > [<ffffffff8168045c>] __dev_open+0xac/0x120 > [<ffffffff81680506>] dev_open+0x36/0x70 > [<ffffffff8169abec>] netpoll_setup+0x23c/0x300 > [<ffffffff8169a66a>] ? netpoll_parse_options+0x19a/0x200 > [<ffffffff81d797a8>] ? option_setup+0x1f/0x1f > [<ffffffff81d79882>] init_netconsole+0xda/0x262 > [<ffffffff81d797a8>] ? option_setup+0x1f/0x1f > [<ffffffff810003a8>] do_one_initcall+0x88/0x1b0 > [<ffffffff81d31144>] kernel_init_freeable+0x14a/0x1e3 > [<ffffffff81d308f1>] ? do_early_param+0x8c/0x8c > [<ffffffff81778610>] ? rest_init+0x80/0x80 > [<ffffffff8177861e>] kernel_init+0xe/0xe0 > [<ffffffff8177dc5f>] ret_from_fork+0x3f/0x70 > [<ffffffff81778610>] ? rest_init+0x80/0x80 > > This patch addresses the issue cited in 7332a13b038be05c by making vxlan > actually check if ipv6 is loaded, and reverts it to module_init() so > that it becomes device_initcall() when built-in, eliminating the > netconsole issue. > > The ipv6 module is permanent, so there's no need to actually do the > usual module_get/module_put dance: once we find it loaded, we can just > assume that it always will be. > > AFAICS, nothing actually ends up calling vxlan_open() during initcalls, > so in the (IPV6=y && VXLAN=y) case we can't end up there before ipv6 has > initialized. > > Signed-off-by: Calvin Owens <calvinowens@fb.com> This architecture just sucks. :( ixgbe should not have to call into vxlan but vxlan has to call to ixgbe. Thus the vxlan_get_rx_port is absolutely unnecessary and should be removed. This also lets ixgbe depend on vxlan which is absurd. Simply let vxlan_get_rx_port be called from vxlan_notifier_block on NETDEV_REGISTER or NETDEV_UP events, which is already available. For the second vxlan_get_rx_port case, which is a IXGBE_FLAG2_VXLAN_REREG_NEEDED needed event, I would suggest we also push that over to the vxlan_notifier_block, maybe with a new event type for the notifiers. After this change ixgbe would not depend on vxlan module any more. Thanks, Hannes ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] netconsole: Initialize after all core networking drivers 2015-12-24 10:25 ` [PATCH] netconsole: Initialize after all core networking drivers Hannes Frederic Sowa @ 2015-12-28 23:21 ` Cong Wang 2015-12-29 13:59 ` Hannes Frederic Sowa 0 siblings, 1 reply; 11+ messages in thread From: Cong Wang @ 2015-12-28 23:21 UTC (permalink / raw) To: Hannes Frederic Sowa Cc: Calvin Owens, Eric Dumazet, David Miller, Shrijeet Mukherjee, izumi.taku, John Linville, David Ahern, netdev, linux-kernel, kernel-team On Thu, Dec 24, 2015 at 2:25 AM, Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > Hi, > > On 24.12.2015 00:03, Calvin Owens wrote: >> This patch addresses the issue cited in 7332a13b038be05c by making vxlan >> actually check if ipv6 is loaded, and reverts it to module_init() so >> that it becomes device_initcall() when built-in, eliminating the >> netconsole issue. >> >> The ipv6 module is permanent, so there's no need to actually do the >> usual module_get/module_put dance: once we find it loaded, we can just >> assume that it always will be. >> >> AFAICS, nothing actually ends up calling vxlan_open() during initcalls, >> so in the (IPV6=y && VXLAN=y) case we can't end up there before ipv6 has >> initialized. >> >> Signed-off-by: Calvin Owens <calvinowens@fb.com> > > This architecture just sucks. :( > > > ixgbe should not have to call into vxlan but vxlan has to call to ixgbe. > Thus the vxlan_get_rx_port is absolutely unnecessary and should be > removed. This also lets ixgbe depend on vxlan which is absurd. > > Simply let vxlan_get_rx_port be called from vxlan_notifier_block on > NETDEV_REGISTER or NETDEV_UP events, which is already available. > > For the second vxlan_get_rx_port case, which is a > IXGBE_FLAG2_VXLAN_REREG_NEEDED needed event, I would suggest we also > push that over to the vxlan_notifier_block, maybe with a new event type > for the notifiers. > > After this change ixgbe would not depend on vxlan module any more. > Agreed with this direction, but not yet convinced with this approach. My feeling is that we should move this piece of code into core networking, so that both vxlan and ixgbe driver can call it without worrying about the dependencies. It looks like all we need here is a per-netns structure about family and ports for all vxlans. But I could overlook something here. Thanks. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] netconsole: Initialize after all core networking drivers 2015-12-28 23:21 ` Cong Wang @ 2015-12-29 13:59 ` Hannes Frederic Sowa 0 siblings, 0 replies; 11+ messages in thread From: Hannes Frederic Sowa @ 2015-12-29 13:59 UTC (permalink / raw) To: Cong Wang Cc: Calvin Owens, Eric Dumazet, David Miller, Shrijeet Mukherjee, izumi.taku, John Linville, David Ahern, netdev, linux-kernel, kernel-team On 29.12.2015 00:21, Cong Wang wrote: > On Thu, Dec 24, 2015 at 2:25 AM, Hannes Frederic Sowa > <hannes@stressinduktion.org> wrote: >> Hi, >> >> On 24.12.2015 00:03, Calvin Owens wrote: >>> This patch addresses the issue cited in 7332a13b038be05c by making vxlan >>> actually check if ipv6 is loaded, and reverts it to module_init() so >>> that it becomes device_initcall() when built-in, eliminating the >>> netconsole issue. >>> >>> The ipv6 module is permanent, so there's no need to actually do the >>> usual module_get/module_put dance: once we find it loaded, we can just >>> assume that it always will be. >>> >>> AFAICS, nothing actually ends up calling vxlan_open() during initcalls, >>> so in the (IPV6=y && VXLAN=y) case we can't end up there before ipv6 has >>> initialized. >>> >>> Signed-off-by: Calvin Owens <calvinowens@fb.com> >> >> This architecture just sucks. :( >> >> >> ixgbe should not have to call into vxlan but vxlan has to call to ixgbe. >> Thus the vxlan_get_rx_port is absolutely unnecessary and should be >> removed. This also lets ixgbe depend on vxlan which is absurd. >> >> Simply let vxlan_get_rx_port be called from vxlan_notifier_block on >> NETDEV_REGISTER or NETDEV_UP events, which is already available. >> >> For the second vxlan_get_rx_port case, which is a >> IXGBE_FLAG2_VXLAN_REREG_NEEDED needed event, I would suggest we also >> push that over to the vxlan_notifier_block, maybe with a new event type >> for the notifiers. >> >> After this change ixgbe would not depend on vxlan module any more. >> > > Agreed with this direction, but not yet convinced with this approach. > > My feeling is that we should move this piece of code into core > networking, so that both vxlan and ixgbe driver can call it without > worrying about the dependencies. It looks like all we need here > is a per-netns structure about family and ports for all vxlans. > But I could overlook something here. We actually have that already in terms of net_generic(vxlan_net_id). We will also need that for geneve and other offloads supported by hardware. Your approach seems to be basically the same like mine, just in your proposal the driver calls to all the helper functions, while in my proposal the vxlan/geneve and other layers can update the offload settings on the hardware. As the notifier_blocks are already available and registered, why not just use them? Thanks, Hannes ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] netconsole: Initialize after all core networking drivers 2015-12-23 23:03 ` Calvin Owens 2015-12-24 6:54 ` [PATCH] vxlan: Properly depend on ipv6 and revert to module_init() kbuild test robot 2015-12-24 10:25 ` [PATCH] netconsole: Initialize after all core networking drivers Hannes Frederic Sowa @ 2015-12-28 23:24 ` Cong Wang 2 siblings, 0 replies; 11+ messages in thread From: Cong Wang @ 2015-12-28 23:24 UTC (permalink / raw) To: Calvin Owens Cc: Eric Dumazet, David Miller, Shrijeet Mukherjee, izumi.taku, John Linville, David Ahern, netdev, linux-kernel, kernel-team On Wed, Dec 23, 2015 at 3:03 PM, Calvin Owens <calvinowens@fb.com> wrote: > Commit 7332a13b038be05c ("vxlan: defer vxlan init as late as possible") > changed vxlan to use late_initcall(), because vxlan relies on ipv6 being > loaded when a new device is opened. > > This causes netconsole to panic at boot when configured via the kernel > cmdline on an IXGBE NIC, because ixgbe_open() assumes that vxlan has > already been initialized: BTW, it is not fair to blame my commit above, since it was merged long before ixgbe or any hardware driver has vxlan support. Not a big deal, just FYI. ;) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] netconsole: Initialize after all core networking drivers 2015-12-17 23:52 [PATCH] netconsole: Initialize after all core networking drivers Calvin Owens 2015-12-18 1:08 ` Eric Dumazet @ 2015-12-18 1:10 ` Stephen Hemminger 2015-12-18 1:39 ` Calvin Owens 1 sibling, 1 reply; 11+ messages in thread From: Stephen Hemminger @ 2015-12-18 1:10 UTC (permalink / raw) To: Calvin Owens Cc: davem, shm, izumi.taku, linville, dsa, netdev, linux-kernel, kernel-team On Thu, 17 Dec 2015 15:52:39 -0800 Calvin Owens <calvinowens@fb.com> wrote: > With built-in netconsole and IXGBE, configuring netconsole via the kernel > cmdline results in the following panic at boot: > > netpoll: netconsole: device eth0 not up yet, forcing it > usb 2-1: new high-speed USB device number 2 using ehci-pci > ixgbe 0000:03:00.0: registered PHC device on eth0 > BUG: unable to handle kernel NULL pointer dereference at 0000000000000810 > <snip> > Call Trace: > [<ffffffff81578661>] ? vxlan_get_rx_port+0x41/0xa0 > [<ffffffff81586828>] ixgbe_open+0x4e8/0x540 > [<ffffffff8168045c>] __dev_open+0xac/0x120 > [<ffffffff81680506>] dev_open+0x36/0x70 > [<ffffffff8169abec>] netpoll_setup+0x23c/0x300 > [<ffffffff8169a66a>] ? netpoll_parse_options+0x19a/0x200 > [<ffffffff81d797a8>] ? option_setup+0x1f/0x1f > [<ffffffff81d79882>] init_netconsole+0xda/0x262 > [<ffffffff81d797a8>] ? option_setup+0x1f/0x1f > [<ffffffff810003a8>] do_one_initcall+0x88/0x1b0 > [<ffffffff81d31144>] kernel_init_freeable+0x14a/0x1e3 > [<ffffffff81d308f1>] ? do_early_param+0x8c/0x8c > [<ffffffff81778610>] ? rest_init+0x80/0x80 > [<ffffffff8177861e>] kernel_init+0xe/0xe0 > [<ffffffff8177dc5f>] ret_from_fork+0x3f/0x70 > [<ffffffff81778610>] ? rest_init+0x80/0x80 > > This happens because IXGBE assumes that vxlan has already been initialized. > The cleanest way to fix this is to just initialize netconsole after all the > other core networking stuff has completed. > > Signed-off-by: Calvin Owens <calvinowens@fb.com> Fixing this by changing Makefile order is too fragile. You are depending on the fact that Makefile order determines link order and that determines initialization order. Down that path demons lie. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] netconsole: Initialize after all core networking drivers 2015-12-18 1:10 ` Stephen Hemminger @ 2015-12-18 1:39 ` Calvin Owens 0 siblings, 0 replies; 11+ messages in thread From: Calvin Owens @ 2015-12-18 1:39 UTC (permalink / raw) To: Stephen Hemminger Cc: davem, shm, izumi.taku, linville, dsa, netdev, linux-kernel, kernel-team On Thursday 12/17 at 17:10 -0800, Stephen Hemminger wrote: > On Thu, 17 Dec 2015 15:52:39 -0800 > Calvin Owens <calvinowens@fb.com> wrote: > > > With built-in netconsole and IXGBE, configuring netconsole via the kernel > > cmdline results in the following panic at boot: > > > > netpoll: netconsole: device eth0 not up yet, forcing it > > usb 2-1: new high-speed USB device number 2 using ehci-pci > > ixgbe 0000:03:00.0: registered PHC device on eth0 > > BUG: unable to handle kernel NULL pointer dereference at 0000000000000810 > > <snip> > > Call Trace: > > [<ffffffff81578661>] ? vxlan_get_rx_port+0x41/0xa0 > > [<ffffffff81586828>] ixgbe_open+0x4e8/0x540 > > [<ffffffff8168045c>] __dev_open+0xac/0x120 > > [<ffffffff81680506>] dev_open+0x36/0x70 > > [<ffffffff8169abec>] netpoll_setup+0x23c/0x300 > > [<ffffffff8169a66a>] ? netpoll_parse_options+0x19a/0x200 > > [<ffffffff81d797a8>] ? option_setup+0x1f/0x1f > > [<ffffffff81d79882>] init_netconsole+0xda/0x262 > > [<ffffffff81d797a8>] ? option_setup+0x1f/0x1f > > [<ffffffff810003a8>] do_one_initcall+0x88/0x1b0 > > [<ffffffff81d31144>] kernel_init_freeable+0x14a/0x1e3 > > [<ffffffff81d308f1>] ? do_early_param+0x8c/0x8c > > [<ffffffff81778610>] ? rest_init+0x80/0x80 > > [<ffffffff8177861e>] kernel_init+0xe/0xe0 > > [<ffffffff8177dc5f>] ret_from_fork+0x3f/0x70 > > [<ffffffff81778610>] ? rest_init+0x80/0x80 > > > > This happens because IXGBE assumes that vxlan has already been initialized. > > The cleanest way to fix this is to just initialize netconsole after all the > > other core networking stuff has completed. > > > > Signed-off-by: Calvin Owens <calvinowens@fb.com> > > Fixing this by changing Makefile order is too fragile. > You are depending on the fact that Makefile order determines link order > and that determines initialization order. Down that path demons lie. Hmm, include/linux/init.h explicitly states that "Ordering inside the subsections is determined by link order", and has since before the beginning of the history in Git. I agree it seems magic/scary, but as Eric says I would imagine this behavior is relied upon in other places. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2015-12-29 13:59 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-12-17 23:52 [PATCH] netconsole: Initialize after all core networking drivers Calvin Owens 2015-12-18 1:08 ` Eric Dumazet 2015-12-18 1:46 ` Calvin Owens 2015-12-23 23:03 ` Calvin Owens 2015-12-24 6:54 ` [PATCH] vxlan: Properly depend on ipv6 and revert to module_init() kbuild test robot 2015-12-24 10:25 ` [PATCH] netconsole: Initialize after all core networking drivers Hannes Frederic Sowa 2015-12-28 23:21 ` Cong Wang 2015-12-29 13:59 ` Hannes Frederic Sowa 2015-12-28 23:24 ` Cong Wang 2015-12-18 1:10 ` Stephen Hemminger 2015-12-18 1:39 ` Calvin Owens
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).