From: ebiederm@xmission•com (Eric W. Biederman)
To: Florian Westphal <fw@strlen•de>
Cc: <netfilter-devel@vger•kernel.org>, netdev@vger•kernel.org
Subject: Re: [PATCH nf-next] netns: add and use net_ns_barrier
Date: Wed, 31 May 2017 13:13:32 -0500 [thread overview]
Message-ID: <87y3tcj3n7.fsf@xmission.com> (raw)
In-Reply-To: <20170530093812.10712-1-fw@strlen.de> (Florian Westphal's message of "Tue, 30 May 2017 11:38:12 +0200")
Florian Westphal <fw@strlen•de> writes:
> Quoting 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.
>
> Events that lead to the crash:
> 1. conntrack is created with ftp helper in netns x
> 2. This netns is 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
> via for_each_net()
>
> but because netns is already gone from list the for_each_net() loop
> doesn't include it, therefore all of these conntracks are unaffected.
>
> 6. helper module unload finishes
> 7. netns wq invokes destructor for rmmod'ed helper
>
> CC: "Eric W. Biederman" <ebiederm@xmission•com>
> Reported-by: Joe Stringer <joe@ovn•org>
> Signed-off-by: Florian Westphal <fw@strlen•de>
> ---
> Eric, I'd like an explicit (n)ack from you for this one.
This doesn't look too scary but I have the impression we have addressed
this elsewhere with a different solution.
Looking...
Ok. unregister_pernet_operations takes the net_mutex and thus
gives you this barrier automatically.
Hmm. Why isn't this working for conntrack, looking...
nf_conntrack_ftp doesn't use unregister_pernet_operations...
nf_conntract_ftp does use nf_conntrack_helpers_unregister
I think I almost see the problem.
What is the per net code that stops dealing with the nf_conntract_ftp?
I am trying to figure out if your netns_barrier is reasonable or if
it treating the symptom. I am having trouble seeing enough of what
conntrack is doing to judge.
Am I correct in understanding that the root problem is there is
something pointing to ftp_exp_policy at the time of module unload?
Eric
> include/net/net_namespace.h | 3 +++
> net/core/net_namespace.c | 17 +++++++++++++++++
> net/netfilter/nf_conntrack_core.c | 9 +++++++++
> 3 files changed, 29 insertions(+)
>
> diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
> index fe80bb48ab1f..a24a57593202 100644
> --- a/include/net/net_namespace.h
> +++ b/include/net/net_namespace.h
> @@ -158,6 +158,7 @@ extern struct net init_net;
> struct net *copy_net_ns(unsigned long flags, struct user_namespace *user_ns,
> struct net *old_net);
>
> +void net_ns_barrier(void);
> #else /* CONFIG_NET_NS */
> #include <linux/sched.h>
> #include <linux/nsproxy.h>
> @@ -168,6 +169,8 @@ static inline struct net *copy_net_ns(unsigned long flags,
> return ERR_PTR(-EINVAL);
> return old_net;
> }
> +
> +static inline void net_ns_barrier(void) {}
> #endif /* CONFIG_NET_NS */
>
>
> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
> index 1934efd4a9d4..1f15abb1d733 100644
> --- a/net/core/net_namespace.c
> +++ b/net/core/net_namespace.c
> @@ -482,6 +482,23 @@ static void cleanup_net(struct work_struct *work)
> net_drop_ns(net);
> }
> }
> +
> +/**
> + * net_ns_barrier - wait until concurrent net_cleanup_work is done
> + *
> + * cleanup_net runs from work queue and will first remove namespaces
> + * from the global list, then run net exit functions.
> + *
> + * Call this in module exit path to make sure that all netns
> + * ->exit ops have been invoked before the function is removed.
> + */
> +void net_ns_barrier(void)
> +{
> + mutex_lock(&net_mutex);
> + mutex_unlock(&net_mutex);
> +}
> +EXPORT_SYMBOL(net_ns_barrier);
> +
> static DECLARE_WORK(net_cleanup_work, cleanup_net);
>
> void __put_net(struct net *net)
> diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> index c3bd9b086dcc..ee972ee7bf81 100644
> --- a/net/netfilter/nf_conntrack_core.c
> +++ b/net/netfilter/nf_conntrack_core.c
> @@ -1720,6 +1720,8 @@ EXPORT_SYMBOL_GPL(nf_ct_iterate_cleanup_net);
> * Like nf_ct_iterate_cleanup, but first marks conntracks on the
> * unconfirmed list as dying (so they will not be inserted into
> * main table).
> + *
> + * Can only be called in module exit path.
> */
> void
> nf_ct_iterate_destroy(int (*iter)(struct nf_conn *i, void *data), void *data)
> @@ -1734,6 +1736,13 @@ nf_ct_iterate_destroy(int (*iter)(struct nf_conn *i, void *data), void *data)
> }
> rtnl_unlock();
>
> + /* Need to wait for netns cleanup worker to finish, if its
> + * running -- it might have deleted a net namespace from
> + * the global list, so our __nf_ct_unconfirmed_destroy() might
> + * not have affected all namespaces.
> + */
> + net_ns_barrier();
> +
> /* a conntrack could have been unlinked from unconfirmed list
> * before we grabbed pcpu lock in __nf_ct_unconfirmed_destroy().
> * This makes sure its inserted into conntrack table.
next prev parent reply other threads:[~2017-05-31 18:13 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-05-30 9:38 [PATCH nf-next] netns: add and use net_ns_barrier Florian Westphal
2017-05-31 16:55 ` David Miller
2017-05-31 17:46 ` Eric W. Biederman
2017-05-31 18:13 ` Eric W. Biederman [this message]
2017-05-31 20:21 ` Joe Stringer
2017-06-01 8:52 ` Florian Westphal
2017-06-12 21:47 ` Cong Wang
2017-06-13 6:16 ` Florian Westphal
2017-06-13 16:35 ` Cong Wang
2017-06-13 18:07 ` Florian Westphal
2017-06-13 19:27 ` Joe Stringer
2017-06-13 21:16 ` Cong Wang
2017-06-14 8:41 ` Pablo Neira Ayuso
2017-06-14 14:20 ` Eric W. Biederman
2017-06-12 8:47 ` Pablo Neira Ayuso
2017-06-02 9:38 ` David Laight
2017-06-02 9:53 ` Florian Westphal
2017-06-19 17:10 ` Pablo Neira Ayuso
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87y3tcj3n7.fsf@xmission.com \
--to=ebiederm@xmission$(echo .)com \
--cc=fw@strlen$(echo .)de \
--cc=netdev@vger$(echo .)kernel.org \
--cc=netfilter-devel@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox