From mboxrd@z Thu Jan 1 00:00:00 1970 From: Robert Shearman Subject: Re: [PATCH net] lwtunnel: fix autoload of lwt modules Date: Tue, 17 Jan 2017 10:04:36 +0000 Message-ID: References: <1484631185-9428-1-git-send-email-dsa@cumulusnetworks.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit To: David Ahern , Return-path: Received: from mx0b-000f0801.pphosted.com ([67.231.152.113]:54200 "EHLO mx0a-000f0801.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750803AbdAQKmG (ORCPT ); Tue, 17 Jan 2017 05:42:06 -0500 In-Reply-To: <1484631185-9428-1-git-send-email-dsa@cumulusnetworks.com> Sender: netdev-owner@vger.kernel.org List-ID: On 17/01/17 05:33, David Ahern wrote: > Trying to add an mpls encap route when the MPLS modules are not loaded > hangs. For example: > > CONFIG_MPLS=y > CONFIG_NET_MPLS_GSO=m > CONFIG_MPLS_ROUTING=m > CONFIG_MPLS_IPTUNNEL=m > > $ ip route add 10.10.10.10/32 encap mpls 100 via inet 10.100.1.2 > > The ip command hangs: > root 880 826 0 21:25 pts/0 00:00:00 ip route add 10.10.10.10/32 encap mpls 100 via inet 10.100.1.2 > > $ cat /proc/880/stack > [] call_usermodehelper_exec+0xd6/0x134 > [] __request_module+0x27b/0x30a > [] lwtunnel_build_state+0xe4/0x178 > [] fib_create_info+0x47f/0xdd4 > [] fib_table_insert+0x90/0x41f > [] inet_rtm_newroute+0x4b/0x52 > ... > > modprobe is trying to load rtnl-lwt-MPLS: > > root 881 5 0 21:25 ? 00:00:00 /sbin/modprobe -q -- rtnl-lwt-MPLS > > and it hangs after loading mpls_router: > > $ cat /proc/881/stack > [] rtnl_lock+0x12/0x14 > [] register_netdevice_notifier+0x16/0x179 > [] mpls_init+0x25/0x1000 [mpls_router] > [] do_one_initcall+0x8e/0x13f > [] do_init_module+0x5a/0x1e5 > [] load_module+0x13bd/0x17d6 > ... > > The problem is that lwtunnel_build_state is called with rtnl lock > held preventing mpls_init from registering. Fix by dropping the > lock before invoking request_module. > > Fixes: 745041e2aaf1 ("lwtunnel: autoload of lwt modules") > Signed-off-by: David Ahern > --- > This is a best guess at the Fixes. > > net/core/lwtunnel.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/net/core/lwtunnel.c b/net/core/lwtunnel.c > index a5d4e866ce88..c14ee4d62a8a 100644 > --- a/net/core/lwtunnel.c > +++ b/net/core/lwtunnel.c > @@ -120,7 +120,9 @@ int lwtunnel_build_state(struct net_device *dev, u16 encap_type, > > if (encap_type_str) { > rcu_read_unlock(); > + __rtnl_unlock(); > request_module("rtnl-lwt-%s", encap_type_str); > + rtnl_lock(); > rcu_read_lock(); > ops = rcu_dereference(lwtun_encaps[encap_type]); > } > Is it safe to release the rtnl lock here? E.g. neither fib_get_nhs nor fib_create_info take a reference on the device and further up inet_rtm_newroute has a pointer to the struct fib_table without taking a reference. Unfortunately, I think more invasive changes are required to solve this. I'm happy to work on solving this. Thanksm Rob