[PATCH net] netfilter: nf_conntrack: Use net_mutex for helper unregistration.

[PATCH net] netfilter: nf_conntrack: Use net_mutex for helper unregistration.

[Joe Stringer]

If a user loads nf_conntrack_ftp, sends FTP traffic through a network
namespace, destroys that namespace then unloads the FTP helper module,
then the kernel will crash.

Florian's assessment of the bug:

    AFAIU following happens:

    1. ct is created with ftp helper in netns x
    2. netns x gets destroyed
    3. netns destruction is scheduled
    4. netns destruction wq starts, removes netns from global list
    5. ftp helper is unloaded, which resets all helpers of the conntracks

    ... but because netns is already gone from list the for_each_net() loop
    doesn't include it, so we do not change any of the conntracks in net
    namespaces that are already dead.

    6. netns destruction invokes destructor for rmmod'ed helper

Backtrace:

BUG: unable to handle kernel paging request at ffffffffa03d60c8
IP: [<ffffffffa00bc797>] nf_ct_helper_destroy+0x97/0x170 [nf_conntrack]
...
Oops: 0000 [#1] SMP
Workqueue: netns cleanup_net
...
Call Trace:
 [<ffffffffa00bc73c>] ? nf_ct_helper_destroy+0x3c/0x170 [nf_conntrack]
 [<ffffffffa00b6c9c>] nf_ct_delete+0x3c/0x1e0 [nf_conntrack]
 [<ffffffffa00bc9f0>] ? nf_conntrack_helper_fini+0x30/0x30 [nf_conntrack]
 [<ffffffffa00b75c8>] nf_ct_iterate_cleanup+0x258/0x270 [nf_conntrack]
 [<ffffffffa00bcf0f>] nf_ct_l3proto_pernet_unregister+0x2f/0x60 [nf_conntrack]
 [<ffffffffa00370e9>] ipv4_net_exit+0x19/0x50 [nf_conntrack_ipv4]
 [<ffffffff81668fa8>] ops_exit_list.isra.4+0x38/0x60
 [<ffffffff8166a35e>] cleanup_net+0x1be/0x290
 [<ffffffff81085b2c>] process_one_work+0x1dc/0x660
 [<ffffffff81085ab1>] ? process_one_work+0x161/0x660
 [<ffffffff810860db>] worker_thread+0x12b/0x4a0
 [<ffffffff81085fb0>] ? process_one_work+0x660/0x660
 [<ffffffff8108ca22>] kthread+0xf2/0x110
 [<ffffffff817a6c02>] ret_from_fork+0x22/0x40
 [<ffffffff8108c930>] ? kthread_create_on_node+0x220/0x220

Fix the issue by using net_mutex during conntrack helper unregistration.

Suggested-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Joe Stringer <joe@ovn.org>
---

I've boiled it down to a repro script here:
https://gist.github.com/joestringer/465328172ee8960242142572b0ffc6e1

There's nothing special about the FTP server (requires pyftpdlib):
https://github.com/openvswitch/ovs/blob/v2.5.0/tests/test-l7.py

Other script dependencies are conntrack, ip, bridge-utils, wget.

This issue may be mitigated by disabling automatic conntrack helper
assignment.

In regards to affected kernels, I looked back as far as 3.13 and I can
still reproduce the issue with the above script. I haven't looked further.
---
 net/core/net_namespace.c            | 1 +
 net/netfilter/nf_conntrack_helper.c | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index 2c2eb1b629b1..cf9058409082 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -28,6 +28,7 @@
 static LIST_HEAD(pernet_list);
 static struct list_head *first_device = &pernet_list;
 DEFINE_MUTEX(net_mutex);
+EXPORT_SYMBOL_GPL(net_mutex);
 
 LIST_HEAD(net_namespace_list);
 EXPORT_SYMBOL_GPL(net_namespace_list);
diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
index 3b40ec575cd5..6860b19be406 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -449,10 +449,10 @@ void nf_conntrack_helper_unregister(struct nf_conntrack_helper *me)
 	 */
 	synchronize_rcu();
 
-	rtnl_lock();
+	mutex_lock(&net_mutex);
 	for_each_net(net)
 		__nf_conntrack_helper_unregister(me, net);
-	rtnl_unlock();
+	mutex_unlock(&net_mutex);
 }
 EXPORT_SYMBOL_GPL(nf_conntrack_helper_unregister);
 
-- 
2.1.4

Re: [PATCH net] netfilter: nf_conntrack: Use net_mutex for helper unregistration.

[Pablo Neira Ayuso]

Hi Joe,

On Thu, May 05, 2016 at 03:50:37PM -0700, Joe Stringer wrote:
> diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
> index 3b40ec575cd5..6860b19be406 100644
> --- a/net/netfilter/nf_conntrack_helper.c
> +++ b/net/netfilter/nf_conntrack_helper.c
> @@ -449,10 +449,10 @@ void nf_conntrack_helper_unregister(struct nf_conntrack_helper *me)
>  	 */
>  	synchronize_rcu();
>  
> -	rtnl_lock();
> +	mutex_lock(&net_mutex);
>  	for_each_net(net)
>  		__nf_conntrack_helper_unregister(me, net);
> -	rtnl_unlock();
> +	mutex_unlock(&net_mutex);

This simple solution works because we have no .exit callbacks in any
of our helpers. Otherwise, the helper code may be already gone by when
the worker has a chance to run to release the netns.

If so, probably I can append this as comment to this function so we
don't forget. If we ever have .exit callbacks (I don't expect so), we
would need to wait for worker completion.

Re: [PATCH net] netfilter: nf_conntrack: Use net_mutex for helper unregistration.

[Joe Stringer]

On 6 May 2016 at 04:03, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Hi Joe,
>
> On Thu, May 05, 2016 at 03:50:37PM -0700, Joe Stringer wrote:
>> diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
>> index 3b40ec575cd5..6860b19be406 100644
>> --- a/net/netfilter/nf_conntrack_helper.c
>> +++ b/net/netfilter/nf_conntrack_helper.c
>> @@ -449,10 +449,10 @@ void nf_conntrack_helper_unregister(struct nf_conntrack_helper *me)
>>        */
>>       synchronize_rcu();
>>
>> -     rtnl_lock();
>> +     mutex_lock(&net_mutex);
>>       for_each_net(net)
>>               __nf_conntrack_helper_unregister(me, net);
>> -     rtnl_unlock();
>> +     mutex_unlock(&net_mutex);
>
> This simple solution works because we have no .exit callbacks in any
> of our helpers. Otherwise, the helper code may be already gone by when
> the worker has a chance to run to release the netns.

I'm open to any alternative solutions, but if helper code isn't doing
this yet then perhaps this fix is sufficient?

> If so, probably I can append this as comment to this function so we
> don't forget. If we ever have .exit callbacks (I don't expect so), we
> would need to wait for worker completion.

Sounds reasonable to me.

I see there's a bunch of other unregister locations like
nf_nat_l3proto_clean(), nf_nat_l4proto_clean(), nf_unregister_hook()
which might need similar treatment?

Re: [PATCH net] netfilter: nf_conntrack: Use net_mutex for helper unregistration.

[Florian Westphal]

Joe Stringer <joe@ovn.org> wrote:
> > If so, probably I can append this as comment to this function so we
> > don't forget. If we ever have .exit callbacks (I don't expect so), we
> > would need to wait for worker completion.
> 
> Sounds reasonable to me.
> 
> I see there's a bunch of other unregister locations like
> nf_nat_l3proto_clean(), nf_nat_l4proto_clean(), nf_unregister_hook()
> which might need similar treatment?

I think they are fine, hook entries are duplicated per netns so we
should not access data in a removed module.

However, we might be able to trigger the

WARN(1, "nf_unregister_net_hook: hook not found!\n");

part in nf_unregister_net_hook():

[ destroy netns -> destruction queued -> rmmod -> all hooks are
destroyed -> netns workq runs -> nf_unregister_net_hook gets called
-> hook already gone ]

For nf_nat_l3|4proto_clean I don't see a problem either, if netns
is gone all these conntracks will be zapped once the workqueue runs, even if
the iteration in those function did not see the netns anymore.

Cheers,
Florian

Re: [PATCH net] netfilter: nf_conntrack: Use net_mutex for helper unregistration.

[Joe Stringer]

On 6 May 2016 at 04:03, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Hi Joe,
>
> On Thu, May 05, 2016 at 03:50:37PM -0700, Joe Stringer wrote:
>> diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
>> index 3b40ec575cd5..6860b19be406 100644
>> --- a/net/netfilter/nf_conntrack_helper.c
>> +++ b/net/netfilter/nf_conntrack_helper.c
>> @@ -449,10 +449,10 @@ void nf_conntrack_helper_unregister(struct nf_conntrack_helper *me)
>>        */
>>       synchronize_rcu();
>>
>> -     rtnl_lock();
>> +     mutex_lock(&net_mutex);
>>       for_each_net(net)
>>               __nf_conntrack_helper_unregister(me, net);
>> -     rtnl_unlock();
>> +     mutex_unlock(&net_mutex);
>
> This simple solution works because we have no .exit callbacks in any
> of our helpers. Otherwise, the helper code may be already gone by when
> the worker has a chance to run to release the netns.
>
> If so, probably I can append this as comment to this function so we
> don't forget. If we ever have .exit callbacks (I don't expect so), we
> would need to wait for worker completion.

Hi Pablo,

Did you want me to re-spin this patch or look into another approach?

Re: [PATCH net] netfilter: nf_conntrack: Use net_mutex for helper unregistration.

[Pablo Neira Ayuso]

Cc'ing Eric Biederman.

On Mon, May 16, 2016 at 09:38:53PM -0700, Joe Stringer wrote:
> On 6 May 2016 at 04:03, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Hi Joe,
> >
> > On Thu, May 05, 2016 at 03:50:37PM -0700, Joe Stringer wrote:
> >> diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
> >> index 3b40ec575cd5..6860b19be406 100644
> >> --- a/net/netfilter/nf_conntrack_helper.c
> >> +++ b/net/netfilter/nf_conntrack_helper.c
> >> @@ -449,10 +449,10 @@ void nf_conntrack_helper_unregister(struct nf_conntrack_helper *me)
> >>        */
> >>       synchronize_rcu();
> >>
> >> -     rtnl_lock();
> >> +     mutex_lock(&net_mutex);
> >>       for_each_net(net)
> >>               __nf_conntrack_helper_unregister(me, net);
> >> -     rtnl_unlock();
> >> +     mutex_unlock(&net_mutex);
> >
> > This simple solution works because we have no .exit callbacks in any
> > of our helpers. Otherwise, the helper code may be already gone by when
> > the worker has a chance to run to release the netns.
> >
> > If so, probably I can append this as comment to this function so we
> > don't forget. If we ever have .exit callbacks (I don't expect so), we
> > would need to wait for worker completion.
> 
> Hi Pablo,
> 
> Did you want me to re-spin this patch or look into another approach?

Last time I looked into this, my impression was that we should
register each conntrack helper via register_pernet_subsys(). Then,
from the exit path I was missing something like netns_barrier() [ note
that I didn't seem to find anything similar] to ensure netns workqueue
completion before we leave from the module exit path so that the
->exit() netns callback code doesn't go away.

That would also avoid the rtnl_lock() dependency.