[PATCH net] lwtunnel: fix autoload of lwt modules

[PATCH net] lwtunnel: fix autoload of lwt modules

[David Ahern]

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
    [<ffffffff81065a9b>] call_usermodehelper_exec+0xd6/0x134
    [<ffffffff81065efc>] __request_module+0x27b/0x30a
    [<ffffffff814542f6>] lwtunnel_build_state+0xe4/0x178
    [<ffffffff814aa1e4>] fib_create_info+0x47f/0xdd4
    [<ffffffff814ae451>] fib_table_insert+0x90/0x41f
    [<ffffffff814a8010>] 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
    [<ffffffff81441537>] rtnl_lock+0x12/0x14
    [<ffffffff8142ca2a>] register_netdevice_notifier+0x16/0x179
    [<ffffffffa0033025>] mpls_init+0x25/0x1000 [mpls_router]
    [<ffffffff81000471>] do_one_initcall+0x8e/0x13f
    [<ffffffff81119961>] do_init_module+0x5a/0x1e5
    [<ffffffff810bd070>] 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 <dsa@cumulusnetworks.com>
---
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]);
 		}
-- 
2.1.4

Re: [PATCH net] lwtunnel: fix autoload of lwt modules

[Robert Shearman]

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
>     [<ffffffff81065a9b>] call_usermodehelper_exec+0xd6/0x134
>     [<ffffffff81065efc>] __request_module+0x27b/0x30a
>     [<ffffffff814542f6>] lwtunnel_build_state+0xe4/0x178
>     [<ffffffff814aa1e4>] fib_create_info+0x47f/0xdd4
>     [<ffffffff814ae451>] fib_table_insert+0x90/0x41f
>     [<ffffffff814a8010>] 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
>     [<ffffffff81441537>] rtnl_lock+0x12/0x14
>     [<ffffffff8142ca2a>] register_netdevice_notifier+0x16/0x179
>     [<ffffffffa0033025>] mpls_init+0x25/0x1000 [mpls_router]
>     [<ffffffff81000471>] do_one_initcall+0x8e/0x13f
>     [<ffffffff81119961>] do_init_module+0x5a/0x1e5
>     [<ffffffff810bd070>] 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 <dsa@cumulusnetworks.com>
> ---
> 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

Re: [PATCH net] lwtunnel: fix autoload of lwt modules

[David Ahern]

On 1/17/17 3:04 AM, Robert Shearman wrote:
> 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.

fib tables can not be deleted so that reference is safe. You are right about the dev reference in the IPv4 path but the thing is build_state does not need the device reference.

Roopa: do you recall why dev is passed to build_state? Nothing about the encap should require a device reference and none of the current encaps use it.

> 
> Unfortunately, I think more invasive changes are required to solve this. I'm happy to work on solving this.

I have a patch that removes passing dev to build_state. Walking the remainder of the code I do not see any more references that would be affected by dropping rtnl here on the add path. Still looking at the delete path.

Re: [PATCH net] lwtunnel: fix autoload of lwt modules

[Robert Shearman]

On 17/01/17 17:07, David Ahern wrote:
> On 1/17/17 3:04 AM, Robert Shearman wrote:
>> 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.
>
> fib tables can not be deleted so that reference is safe.

Looks like we can free a table on unmerging. Admittedly this is the 
local table so is unlikely to be one that lwtunnels are used on, but 
does it not need to be safe against races anyway?

> You are right about the dev reference in the IPv4 path but the thing is build_state does not need the device reference.
>
> Roopa: do you recall why dev is passed to build_state? Nothing about the encap should require a device reference and none of the current encaps use it.
>
>>
>> Unfortunately, I think more invasive changes are required to solve this. I'm happy to work on solving this.
>
> I have a patch that removes passing dev to build_state. Walking the remainder of the code I do not see any more references that would be affected by dropping rtnl here on the add path. Still looking at the delete path.
>

Ok, I'll continue looking too and let you know if there's anything else 
that pops up.

Having said that, even if we eliminate all the unreferenced objects in 
the current code, what are the chances that we'll be able to keep it 
this way going forward? Perhaps it would be safer to retry the insert 
operation from as close to the start as possible?

Thanks,
Rob

Re: [PATCH net] lwtunnel: fix autoload of lwt modules

[David Ahern]

On 1/17/17 10:26 AM, Robert Shearman wrote:
> Ok, I'll continue looking too and let you know if there's anything else that pops up.
> 
> Having said that, even if we eliminate all the unreferenced objects in the current code, what are the chances that we'll be able to keep it this way going forward? Perhaps it would be safer to retry the insert operation from as close to the start as possible?

handling restart for all code paths seems a bit risky for 4.10. Perhaps then the best course for 4.10 and older stable releases is to remove the autoload code from lwtunnel. It can be re-added once the recovery path is handled.

Re: [PATCH net] lwtunnel: fix autoload of lwt modules

[David Miller]

From: David Ahern <dsa@cumulusnetworks.com>
Date: Tue, 17 Jan 2017 11:04:35 -0700

> handling restart for all code paths seems a bit risky for
> 4.10. Perhaps then the best course for 4.10 and older stable
> releases is to remove the autoload code from lwtunnel. It can be
> re-added once the recovery path is handled.

Indeed, we would need to handle the restart rather far up in the call chain
above where the newroute message gets processed.

But, that being said, I don't think we can legitimately just remove the
autoload functionality.  It's been there for close to a full year and
I guarantee people will get burned if we take it away.

We have to find a way to fix this.

Re: [PATCH net] lwtunnel: fix autoload of lwt modules

[David Ahern]

On 1/17/17 1:38 PM, David Miller wrote:
> But, that being said, I don't think we can legitimately just remove the
> autoload functionality.  It's been there for close to a full year and
> I guarantee people will get burned if we take it away.
> 
> We have to find a way to fix this.

I have patch that removes the dev from build_state. Nothing about an encap state should require a device and none of the current encaps use it so it can be removed without a problem. After that the only reference I have noted is to a table and the only table that can be deleted - and invalidating the pointer - is the local table and that only happens once on fib_unmerge.

In short seems like removing the dev + the current patch dropping the lock fixes the current deadlock problem and should be fine.

Robert: do you agree?

Re: [PATCH net] lwtunnel: fix autoload of lwt modules

[David Miller]

From: David Ahern <dsa@cumulusnetworks.com>
Date: Tue, 17 Jan 2017 13:46:22 -0700

> In short seems like removing the dev + the current patch dropping
> the lock fixes the current deadlock problem and should be fine.

What about the state recorded by fib_get_nhs() and similar?  There is
a mapping from ifindex to ->nh_dev which would be invalidated if the
RTNL semaphore is dropped.

It won't get updated by device events, which is what normally happens,
because the fib_info is not in any of the fib_trie tables yet.

So I think you still have a huge problem without doing proper restarts.

Re: [PATCH net] lwtunnel: fix autoload of lwt modules

[David Ahern]

On 1/17/17 1:54 PM, David Miller wrote:
> From: David Ahern <dsa@cumulusnetworks.com>
> Date: Tue, 17 Jan 2017 13:46:22 -0700
> 
>> In short seems like removing the dev + the current patch dropping
>> the lock fixes the current deadlock problem and should be fine.
> 
> What about the state recorded by fib_get_nhs() and similar?  There is
> a mapping from ifindex to ->nh_dev which would be invalidated if the
> RTNL semaphore is dropped.

As far as I can see through the call to build_state all device indices came from the user and have not been validated yet (once the dev arg to build_state is removed; sent that patch for net-next). The device index validation happens later in fib_create_info with the call to fib_check_nh (or dev_get_by_index for host scope).

I sent an alternative approach that pulls the module loading into a separate function that is called while creating the fib_config. Performance heavy for multipath but solves the autoload without delving into the restart problem.