[PATCH nf-next] netns: add and use net_ns_barrier

[PATCH nf-next] netns: add and use net_ns_barrier

[Florian Westphal]

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.

 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.
-- 
2.13.0

Re: [PATCH nf-next] netns: add and use net_ns_barrier

[David Miller]

From: Florian Westphal <fw@strlen.de>
Date: Tue, 30 May 2017 11:38:12 +0200

> 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.

Indeed, Eric, please do.

Otherwise I'm fine with the generic parts:

Acked-by: David S. Miller <davem@davemloft.net>

Re: [PATCH nf-next] netns: add and use net_ns_barrier

[Eric W. Biederman]

David Miller <davem@davemloft.net> writes:

> From: Florian Westphal <fw@strlen.de>
> Date: Tue, 30 May 2017 11:38:12 +0200
>
>> 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.
>
> Indeed, Eric, please do.

Taking a look now.  The original didn't make it's way into my inbox.  I
just have a copy from netdev.  Florian there may be a bit of an email
black hole between us.

> Otherwise I'm fine with the generic parts:
>
> Acked-by: David S. Miller <davem@davemloft.net>

Eric

Re: [PATCH nf-next] netns: add and use net_ns_barrier

[Eric W. Biederman]

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.

Re: [PATCH nf-next] netns: add and use net_ns_barrier

[Joe Stringer]

On 31 May 2017 at 11:13, Eric W. Biederman <ebiederm@xmission.com> wrote:
> 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?

I believe it's just that there is a conntrack entry with a 'struct
nf_conn_help' whose 'helper' parameter is pointing to the 'struct
nf_conntrack_helper ftp[...]' that is declared immediately above the
ftp_exp_policy. nf_ct_helper_destroy() would expect that
nfct_help(ct)->helper is NULL if the module was unloaded, but because
the netns was removed from the global list prior to unloading the FTP
module, there was no way to clear it. So, when the netns workqueue
invokes the destruction of all conntrack entries in the namespace, it
calls down to delete each conntrack entry and the ones with helpers
attached point to the now-unloaded helper module.

If I follow correctly, the approach proposed in the patch here ensures that:

1) nf_conntrack_helper_unregister() disengages the helper so it won't
be attached to new connections.
2) nf_conntrack_helper_unregister() clears the expectations for this helper.
3) nf_ct_iterate_destroy() iterates through all namespaces to clear
per-netns unconfirmed lists so they don't have any references to the
helper.
4) (NEW) we barrier on net_mutex to ensure that any concurrent netns
workqueue deletion which may have removed a netns prior to (3) will
finish. Now we know that we have iterated all net namespaces.
5) Iterate through the conntrack table, applying the 'unhelp'
operation to all conntrack entries. This removes the helper from all
entries while leaving the entries in the table.

Once this is all done, the helper module can finally unload.

FWIW, the previous iteration is here:
https://www.mail-archive.com/netdev@vger.kernel.org/msg109343.html

Joe

Re: [PATCH nf-next] netns: add and use net_ns_barrier

[Florian Westphal]

Eric W. Biederman <ebiederm@xmission.com> wrote:
> 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?

Joe described it nicely, problem is that after unload we may have
conntracks that still have a nf_conn_help extension attached that
has a pointer to a structure that resided in the (unloaded) module.

Normally these references should have been NULL'd out by
nf_ct_iterate_destroy(), however, there is a small chance that
its for_each_net() misses namespaces already removed-from-list by
concurrent netns workqueue cleanup.

I guess another solution to fix this would be to add dummy pernet
ops to all conntrack helpers so they block on unregister_pernet_subsys().

But thats rather ugly IMO since they don't have any notion of a net
namespace in first place.

RE: [PATCH nf-next] netns: add and use net_ns_barrier

[David Laight]

From: Florian Westphal
> Sent: 30 May 2017 10:38
> 
> 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
> 
...
>  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 problem I see is that nothing obvious guarantees that the cleanup worker
has actually started.

	David

Re: [PATCH nf-next] netns: add and use net_ns_barrier

[Florian Westphal]

David Laight <David.Laight@ACULAB.COM> wrote:
> From: Florian Westphal
> > Sent: 30 May 2017 10:38
> > 
> > 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
> > 
> ...
> >  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 problem I see is that nothing obvious guarantees that the cleanup worker
> has actually started.

If it hasn't even started, the earlier for_each_net() has seen all
net namespaces and we managed to clear helper extensions of all conntracks.

Same in case it has finished already: netns cleanup work queue has
free'd all the affected conntracks we might have missed.

We only are in trouble if netns work queue is running concurrently:
netns cleanup first removes net namespaces from the global list,
so nf_ct_iterate_destroy might have missed these.

Re: [PATCH nf-next] netns: add and use net_ns_barrier

[Pablo Neira Ayuso]

On Wed, May 31, 2017 at 01:13:32PM -0500, Eric W. Biederman wrote:
> 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.

Eric, so you hold your nose there and I take this ;-)

Or let me know if you want a different path.

Thanks !

Re: [PATCH nf-next] netns: add and use net_ns_barrier

[Cong Wang]

On Thu, Jun 1, 2017 at 1:52 AM, Florian Westphal <fw@strlen.de> wrote:
> Joe described it nicely, problem is that after unload we may have
> conntracks that still have a nf_conn_help extension attached that
> has a pointer to a structure that resided in the (unloaded) module.

Why not hold a refcnt for its module?

Re: [PATCH nf-next] netns: add and use net_ns_barrier

[Florian Westphal]

Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Thu, Jun 1, 2017 at 1:52 AM, Florian Westphal <fw@strlen.de> wrote:
> > Joe described it nicely, problem is that after unload we may have
> > conntracks that still have a nf_conn_help extension attached that
> > has a pointer to a structure that resided in the (unloaded) module.
> 
> Why not hold a refcnt for its module?

That would work as well.

I'm not sure its nice to disallow rmmod of helper modules if they are
used by a connection however.

Right now you can "rmmod nf_conntrack_foo" at any time and this should
work just fine without first having to flush affected conntracks
manually.

Re: [PATCH nf-next] netns: add and use net_ns_barrier

[Cong Wang]

On Mon, Jun 12, 2017 at 11:16 PM, Florian Westphal <fw@strlen.de> wrote:
> Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> On Thu, Jun 1, 2017 at 1:52 AM, Florian Westphal <fw@strlen.de> wrote:
>> > Joe described it nicely, problem is that after unload we may have
>> > conntracks that still have a nf_conn_help extension attached that
>> > has a pointer to a structure that resided in the (unloaded) module.
>>
>> Why not hold a refcnt for its module?
>
> That would work as well.
>
> I'm not sure its nice to disallow rmmod of helper modules if they are
> used by a connection however.

I am _not_ suggesting to disallow rmmod.

>
> Right now you can "rmmod nf_conntrack_foo" at any time and this should
> work just fine without first having to flush affected conntracks
> manually.

My point is that since netns wq could invoke code of that module,
why it doesn't hold a refcnt of that module?

I am not familiar with netfilter code base so not sure if that is
hard to do or not, but it looks more elegant than this barrier.

Re: [PATCH nf-next] netns: add and use net_ns_barrier

[Florian Westphal]

Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Mon, Jun 12, 2017 at 11:16 PM, Florian Westphal <fw@strlen.de> wrote:
> > Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >> On Thu, Jun 1, 2017 at 1:52 AM, Florian Westphal <fw@strlen.de> wrote:
> >> > Joe described it nicely, problem is that after unload we may have
> >> > conntracks that still have a nf_conn_help extension attached that
> >> > has a pointer to a structure that resided in the (unloaded) module.
> >>
> >> Why not hold a refcnt for its module?
> >
> > That would work as well.
> >
> > I'm not sure its nice to disallow rmmod of helper modules if they are
> > used by a connection however.
> 
> I am _not_ suggesting to disallow rmmod.

My point was that if you hold reference counts to the module users
will need to manually flush conntrack table (or at least manually
remove affected connections), else rmmod won't work as refcount might be
gt 0.

> > Right now you can "rmmod nf_conntrack_foo" at any time and this should
> > work just fine without first having to flush affected conntracks
> > manually.
> 
> My point is that since netns wq could invoke code of that module,
> why it doesn't hold a refcnt of that module?

*shrug*, I did not write this stuff.

Historically it wasn't needed because we just clear out the helper area
in the affected conntracks (i.e, future packets are not inspected by
the helper anymore).

When conntracks were made per-netns this problem was added as we're not
guaranteed to see all net namespace because module_exit and netns cleanup
can run concurrently.

We can still use the "old" model if we guarantee that we wait for
netns cleanup to finish (which is what this patch does).

The alternative, as you pointed out, is to take a module reference for
each conntrack that uses the helper (and put again when connection is
destroyed).

I don't really care that much except that if we go for the latter
solution users cannot "just rmmod" the module anymore but might have
to manually remove the affected connections first.

Pablo, I can rework things to do module get/put but I ask for explicit
instructions to do so (I prefer the "barrier" approach).

Re: [PATCH nf-next] netns: add and use net_ns_barrier

[Joe Stringer]

On 13 June 2017 at 11:07, Florian Westphal <fw@strlen.de> wrote:
> Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> On Mon, Jun 12, 2017 at 11:16 PM, Florian Westphal <fw@strlen.de> wrote:
>> > Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> >> On Thu, Jun 1, 2017 at 1:52 AM, Florian Westphal <fw@strlen.de> wrote:
>> >> > Joe described it nicely, problem is that after unload we may have
>> >> > conntracks that still have a nf_conn_help extension attached that
>> >> > has a pointer to a structure that resided in the (unloaded) module.
>> >>
>> >> Why not hold a refcnt for its module?
>> >
>> > That would work as well.
>> >
>> > I'm not sure its nice to disallow rmmod of helper modules if they are
>> > used by a connection however.
>>
>> I am _not_ suggesting to disallow rmmod.
>
> My point was that if you hold reference counts to the module users
> will need to manually flush conntrack table (or at least manually
> remove affected connections), else rmmod won't work as refcount might be
> gt 0.
>
>> > Right now you can "rmmod nf_conntrack_foo" at any time and this should
>> > work just fine without first having to flush affected conntracks
>> > manually.
>>
>> My point is that since netns wq could invoke code of that module,
>> why it doesn't hold a refcnt of that module?
>
> *shrug*, I did not write this stuff.
>
> Historically it wasn't needed because we just clear out the helper area
> in the affected conntracks (i.e, future packets are not inspected by
> the helper anymore).
>
> When conntracks were made per-netns this problem was added as we're not
> guaranteed to see all net namespace because module_exit and netns cleanup
> can run concurrently.
>
> We can still use the "old" model if we guarantee that we wait for
> netns cleanup to finish (which is what this patch does).
>
> The alternative, as you pointed out, is to take a module reference for
> each conntrack that uses the helper (and put again when connection is
> destroyed).
>
> I don't really care that much except that if we go for the latter
> solution users cannot "just rmmod" the module anymore but might have
> to manually remove the affected connections first.

The barrier approach sounds less surprising from user perspective for
this very reason.

Re: [PATCH nf-next] netns: add and use net_ns_barrier

[Cong Wang]

On Tue, Jun 13, 2017 at 11:07 AM, Florian Westphal <fw@strlen.de> wrote:
> Historically it wasn't needed because we just clear out the helper area
> in the affected conntracks (i.e, future packets are not inspected by
> the helper anymore).
>
> When conntracks were made per-netns this problem was added as we're not
> guaranteed to see all net namespace because module_exit and netns cleanup
> can run concurrently.
>
> We can still use the "old" model if we guarantee that we wait for
> netns cleanup to finish (which is what this patch does).
>
> The alternative, as you pointed out, is to take a module reference for
> each conntrack that uses the helper (and put again when connection is
> destroyed).

Yeah, this is exactly what I am suggesting and I fully expect this could
need more work than this barrier.

>
> I don't really care that much except that if we go for the latter
> solution users cannot "just rmmod" the module anymore but might have
> to manually remove the affected connections first.

This is not bad because the module is indeed being used in this scenario
so EBUSY is expected, or do we need to guarantee conntrack modules
are not held by existing connections?

Re: [PATCH nf-next] netns: add and use net_ns_barrier

[Pablo Neira Ayuso]

Hi!

On Tue, Jun 13, 2017 at 09:35:20AM -0700, Cong Wang wrote:
> On Mon, Jun 12, 2017 at 11:16 PM, Florian Westphal <fw@strlen.de> wrote:
> > Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >> On Thu, Jun 1, 2017 at 1:52 AM, Florian Westphal <fw@strlen.de> wrote:
> >> > Joe described it nicely, problem is that after unload we may have
> >> > conntracks that still have a nf_conn_help extension attached that
> >> > has a pointer to a structure that resided in the (unloaded) module.
> >>
> >> Why not hold a refcnt for its module?
> >
> > That would work as well.
> >
> > I'm not sure its nice to disallow rmmod of helper modules if they are
> > used by a connection however.
> 
> I am _not_ suggesting to disallow rmmod.
> 
> >
> > Right now you can "rmmod nf_conntrack_foo" at any time and this should
> > work just fine without first having to flush affected conntracks
> > manually.
> 
> My point is that since netns wq could invoke code of that module,
> why it doesn't hold a refcnt of that module?
> 
> I am not familiar with netfilter code base so not sure if that is
> hard to do or not, but it looks more elegant than this barrier.

Florian has added a new native interface to integrate helpers into
nftables in a much better way than we do now, that allows much more
fine grain configuration. This new interface bumps refcounts on
helpers as you suggest.

However, we still have to sort of keep the existing behaviour around,
people has been relying on this rmmod feature to globally disable
helpers. It's very old thing indeed and as you can see, very sparse
grain for the netns era... But still I think we need this.

So I'm inclined to take this, and keep an eye to deprecate this
behaviour in a several years ahead once. Probably we can get rid of
this barrier at some point.

Re: [PATCH nf-next] netns: add and use net_ns_barrier

[Eric W. Biederman]

Pablo Neira Ayuso <pablo@netfilter.org> writes:

> Hi!
>
> On Tue, Jun 13, 2017 at 09:35:20AM -0700, Cong Wang wrote:
>> On Mon, Jun 12, 2017 at 11:16 PM, Florian Westphal <fw@strlen.de> wrote:
>> > Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> >> On Thu, Jun 1, 2017 at 1:52 AM, Florian Westphal <fw@strlen.de> wrote:
>> >> > Joe described it nicely, problem is that after unload we may have
>> >> > conntracks that still have a nf_conn_help extension attached that
>> >> > has a pointer to a structure that resided in the (unloaded) module.
>> >>
>> >> Why not hold a refcnt for its module?
>> >
>> > That would work as well.
>> >
>> > I'm not sure its nice to disallow rmmod of helper modules if they are
>> > used by a connection however.
>> 
>> I am _not_ suggesting to disallow rmmod.
>> 
>> >
>> > Right now you can "rmmod nf_conntrack_foo" at any time and this should
>> > work just fine without first having to flush affected conntracks
>> > manually.
>> 
>> My point is that since netns wq could invoke code of that module,
>> why it doesn't hold a refcnt of that module?
>> 
>> I am not familiar with netfilter code base so not sure if that is
>> hard to do or not, but it looks more elegant than this barrier.
>
> Florian has added a new native interface to integrate helpers into
> nftables in a much better way than we do now, that allows much more
> fine grain configuration. This new interface bumps refcounts on
> helpers as you suggest.
>
> However, we still have to sort of keep the existing behaviour around,
> people has been relying on this rmmod feature to globally disable
> helpers. It's very old thing indeed and as you can see, very sparse
> grain for the netns era... But still I think we need this.
>
> So I'm inclined to take this, and keep an eye to deprecate this
> behaviour in a several years ahead once. Probably we can get rid of
> this barrier at some point.

Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>

If it works I don't have any problems with the code and it sounds like
it works.

My apologies for the delay.  There is an email black hole between Forian
and myself and I missed his replies.  Which gave me a very distored
picture of the conversation.

Eric

Re: [PATCH nf-next] netns: add and use net_ns_barrier

[Pablo Neira Ayuso]

On Tue, May 30, 2017 at 11:38:12AM +0200, Florian Westphal wrote:
> 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

Applied, thanks everyone.