Re: network namespace and kernel bind issue

Re: network namespace and kernel bind issue

[Eric W. Biederman]

Stephen Hemminger <shemminger@vyatta.com> writes:

> On Mon, 1 Oct 2012 14:16:09 -0700
> Stephen Hemminger <shemminger@vyatta.com> wrote:
>
>> When testing VXLAN I noticed that the kernel bind seems to be a problem for
>> network tunnels. The init_net function is called repeatedly for the same
>> network namespace!

It definitely should not be.

>> 1. Create vxlan device:
>>  # ip li add vxlan0 type vxlan id 11 group 239.1.1.1 dev eth0
>>  # dmesg | tail
>> [11580.671016] vxlan: vxlan_init_net in net 1

Net 1?  What are you printing out?  It isn't the net_id by any chance?

>> 2. Start Chrome (or other application using namespaces)
>>  
>>   dmesg | tail
>> [11587.371195] vxlan: vxlan_init_net in net 1
>> [11587.371211] vxlan: bind for UDP socket 0.0.0.0:8472 (-98)
>> 
>> 
>> Isn't init_net supposed to be unique. The current semantics also break
>> L2TP.

The init method should be called exactly once per network namespace.

The timing of the init methods you report seems correct.

The vxlan code isn't in net-next or I would take a look.

I took a quick look at l2tp and the code is doing some weird things.
There are a bunch of references to &init_net that I would expect
to references to either sk_net() or dev_net().  

Adding support for multiple network namespaces and then reaching
out to the initial network namespace for things is definitely a recipe
for getting confused.

So my blind guess would be that someone half implemented network
namespace support for l2tp and vxlan copied the bugs.

Eric


>> This is with 3.6.0-rc7-net-next
>
> Here is back trace from where duplicate network namespace init gets done.

> [13532.579900] vxlan: bind for UDP socket 0.0.0.0:8472 (-98)
> [13532.579903] ------------[ cut here ]------------
> [13532.579906] WARNING: at drivers/net/vxlan.c:1148 vxlan_init_net+0xc9/0x126 [vxlan]()
> [13532.579907] Hardware name: System Product Name
> [13532.579908] Modules linked in: vxlan nfnetlink_log nfnetlink ip6table_filter ip6_tables ebtable_nat ebtables ipt_MASQUERADE iptable_nat nf_nat_ipv4 nf_nat nf_conntrack_ipv4 nf_defrag_ipv4 xt_state nf_conntrack ipt_REJECT xt_CHECKSUM iptable_mangle xt_tcpudp iptable_filter ip_tables x_tables tun bridge stp llc cpufreq_stats cpufreq_powersave cpufreq_conservative cpufreq_userspace binfmt_misc fuse loop snd_hda_codec_hdmi snd_hda_codec_realtek i915 hid_belkin hid_generic snd_hda_intel evdev snd_hda_codec drm_kms_helper snd_hwdep drm snd_pcm_oss snd_pcm psmouse microcode snd_page_alloc serio_raw pcspkr i2c_i801 snd_timer i2c_algo_bit i2c_core acpi_cpufreq mperf processor video button btrfs libcrc32c lzo_compress zlib_deflate crc32c_intel ghash_clmulni_intel aesni_intel aes_x86_64 ablk_hel
 per cryptd usbhid hid ixgbe r8169 mii mdio thermal [last unloaded: vxlan]
> [13532.579965] Pid: 7130, comm: chromium-sandbo Not tainted 3.6.0-rc7-net-next+ #10
> [13532.579966] Call Trace:
> [13532.579972]  [<ffffffff8106674e>] warn_slowpath_common+0x83/0x9c
> [13532.579974]  [<ffffffff81066781>] warn_slowpath_null+0x1a/0x1c
> [13532.579976]  [<ffffffffa03ea87d>] vxlan_init_net+0xc9/0x126 [vxlan]
> [13532.579980]  [<ffffffff8136b4dd>] ops_init+0xcd/0xfc
> [13532.579982]  [<ffffffff8136b824>] setup_net+0x51/0xd8
> [13532.579984]  [<ffffffff8136bd37>] copy_net_ns+0x6c/0xd7
> [13532.579987]  [<ffffffff810882bb>] create_new_namespaces+0xd8/0x14f
> [13532.579989]  [<ffffffff81088417>] copy_namespaces+0x69/0x9e
> [13532.579991]  [<ffffffff81065b10>] copy_process.part.27+0x12ae/0x12f5
> [13532.579994]  [<ffffffff8144170f>] ? do_page_fault+0x2fb/0x37c
> [13532.579997]  [<ffffffff8111d4d8>] ? might_fault+0x5c/0xac
> [13532.579998]  [<ffffffff81065cb2>] do_fork+0x120/0x2fc
> [13532.580001]  [<ffffffff810e7343>] ? time_hardirqs_off+0x15/0x2a
> [13532.580004]  [<ffffffff8143ea53>] ? error_sti+0x5/0x6
> [13532.580007]  [<ffffffff810a7204>] ? trace_hardirqs_off_caller+0x3f/0x9e
> [13532.580009]  [<ffffffff8143e646>] ? retint_swapgs+0xe/0x13
> [13532.580012]  [<ffffffff8103e541>] sys_clone+0x28/0x2a
> [13532.580014]  [<ffffffff814450e3>] stub_clone+0x13/0x20
> [13532.580016]  [<ffffffff81444d92>] ? system_call_fastpath+0x16/0x1b
> [13532.580018] ---[ end trace 2c2b222e23a4d880 ]---
> [13573.765721] vxlan: bind for UDP socket 0.0.0.0:8472 (-98)

Re: network namespace and kernel bind issue

[Stephen Hemminger]

On Mon, 01 Oct 2012 15:40:56 -0700
ebiederm@xmission.com (Eric W. Biederman) wrote:

> Stephen Hemminger <shemminger@vyatta.com> writes:
> 
> > On Mon, 1 Oct 2012 14:16:09 -0700
> > Stephen Hemminger <shemminger@vyatta.com> wrote:
> >
> >> When testing VXLAN I noticed that the kernel bind seems to be a problem for
> >> network tunnels. The init_net function is called repeatedly for the same
> >> network namespace!
> 
> It definitely should not be.
> 
> >> 1. Create vxlan device:
> >>  # ip li add vxlan0 type vxlan id 11 group 239.1.1.1 dev eth0
> >>  # dmesg | tail
> >> [11580.671016] vxlan: vxlan_init_net in net 1
> 
> Net 1?  What are you printing out?  It isn't the net_id by any chance?

Yes it is the net_id which is passed to net_generic() to find the
per-namespace data structure.

> 
> >> 2. Start Chrome (or other application using namespaces)
> >>  
> >>   dmesg | tail
> >> [11587.371195] vxlan: vxlan_init_net in net 1
> >> [11587.371211] vxlan: bind for UDP socket 0.0.0.0:8472 (-98)
> >> 
> >> 
> >> Isn't init_net supposed to be unique. The current semantics also break
> >> L2TP.
> 
> The init method should be called exactly once per network namespace.
> 
> The timing of the init methods you report seems correct.
> 
> The vxlan code isn't in net-next or I would take a look.
> 
> I took a quick look at l2tp and the code is doing some weird things.
> There are a bunch of references to &init_net that I would expect
> to references to either sk_net() or dev_net().  
> 
> Adding support for multiple network namespaces and then reaching
> out to the initial network namespace for things is definitely a recipe
> for getting confused.
> 
> So my blind guess would be that someone half implemented network
> namespace support for l2tp and vxlan copied the bugs.

The vxlan driver has one UDP socket per namespace.
There are no references to init_net in it.

I think the problem is the call chain
      copy_net_ns -> setup_net -> ops_init
There is nothing that nothing increments the id after register_pernet_operations.

Shouldn't there be an increment so each new namespace gets a unique id?

--- a/net/core/net_namespace.c	2012-08-15 08:59:22.938704423 -0700
+++ b/net/core/net_namespace.c	2012-10-01 15:54:50.293088913 -0700
@@ -161,6 +161,7 @@ static __net_init int setup_net(struct n
 #endif
 
 	list_for_each_entry(ops, &pernet_list, list) {
+		++*ops->id;
 		error = ops_init(ops, net);
 		if (error < 0)
 			goto out_undo;


Or maybe you need to keep track of IDR map for each pernet_operations structure?

Re: network namespace and kernel bind issue

[Eric W. Biederman]

Stephen Hemminger <shemminger@vyatta.com> writes:

> On Mon, 01 Oct 2012 15:40:56 -0700
> ebiederm@xmission.com (Eric W. Biederman) wrote:
>
>> Stephen Hemminger <shemminger@vyatta.com> writes:
>> 
>> > On Mon, 1 Oct 2012 14:16:09 -0700
>> > Stephen Hemminger <shemminger@vyatta.com> wrote:
>> >
>> >> When testing VXLAN I noticed that the kernel bind seems to be a problem for
>> >> network tunnels. The init_net function is called repeatedly for the same
>> >> network namespace!
>> 
>> It definitely should not be.
>> 
>> >> 1. Create vxlan device:
>> >>  # ip li add vxlan0 type vxlan id 11 group 239.1.1.1 dev eth0
>> >>  # dmesg | tail
>> >> [11580.671016] vxlan: vxlan_init_net in net 1
>> 
>> Net 1?  What are you printing out?  It isn't the net_id by any chance?
>
> Yes it is the net_id which is passed to net_generic() to find the
> per-namespace data structure.

Yes.  net_id is just an index and is the same in all network namespaces.
net_id should only be different for different instances of per_net
operations.

>> >> 2. Start Chrome (or other application using namespaces)
>> >>  
>> >>   dmesg | tail
>> >> [11587.371195] vxlan: vxlan_init_net in net 1
>> >> [11587.371211] vxlan: bind for UDP socket 0.0.0.0:8472 (-98)
>> >> 
>> >> 
>> >> Isn't init_net supposed to be unique. The current semantics also break
>> >> L2TP.
>> 
>> The init method should be called exactly once per network namespace.
>> 
>> The timing of the init methods you report seems correct.
>> 
>> The vxlan code isn't in net-next or I would take a look.
>> 
>> I took a quick look at l2tp and the code is doing some weird things.
>> There are a bunch of references to &init_net that I would expect
>> to references to either sk_net() or dev_net().  
>> 
>> Adding support for multiple network namespaces and then reaching
>> out to the initial network namespace for things is definitely a recipe
>> for getting confused.
>> 
>> So my blind guess would be that someone half implemented network
>> namespace support for l2tp and vxlan copied the bugs.
>
> The vxlan driver has one UDP socket per namespace.
> There are no references to init_net in it.

Then my guess is that you have an ordering problem.  Attempting
to initialize a vxlan before ipv4 is initialized or some such.

> I think the problem is the call chain
>       copy_net_ns -> setup_net -> ops_init
> There is nothing that nothing increments the id after register_pernet_operations.
>
> Shouldn't there be an increment so each new namespace gets a unique id?

No.

There are some extra pointers at the end of struct net and the id is
which of those pointers your subsystem gets to use.  net_generic returns
your pointer value.

I can see the confusion but the id is definitely not a namespace id.

Eric

Re: network namespace and kernel bind issue

[Stephen Hemminger]

On Mon, 01 Oct 2012 16:11:07 -0700
ebiederm@xmission.com (Eric W. Biederman) wrote:

> Then my guess is that you have an ordering problem.  Attempting
> to initialize a vxlan before ipv4 is initialized or some such.

Isn't there a gurantee that init operations are called in the order
they registered?

Re: network namespace and kernel bind issue

[Eric W. Biederman]

Stephen Hemminger <shemminger@vyatta.com> writes:

> On Mon, 01 Oct 2012 16:11:07 -0700
> ebiederm@xmission.com (Eric W. Biederman) wrote:
>
>> Then my guess is that you have an ordering problem.  Attempting
>> to initialize a vxlan before ipv4 is initialized or some such.
>
> Isn't there a gurantee that init operations are called in the order
> they registered?

Yes.  With the caveat that all things registered with
register_pernet_subsys are called before register_pernet_device.

So if you are a registering as a subsystem the loopback device won't
have been registered yet.

So if there is some requirement that I'm not seeing that the loopback
device needs to be registered or possibly even registered and brought up
before we can bind to a port you could easily be hitting that.

[11587.371211] vxlan: bind for UDP socket 0.0.0.0:8472 (-98)

>From this one clue it does look like the trace is:
inet_bind
   udp4_get_port
       udp_lib_get_port

And it does look like the only possible failure when a port number
is passed in is for the port to be genuinly in use.

Ok.  So I tracked down your patch so I could find the relevant code.

+static __net_init int vxlan_init_net(struct net *net)
+{
....
+	/* Create UDP socket for encapsulation receive. */
+	rc = sock_create_kern(AF_INET, SOCK_DGRAM, IPPROTO_UDP, &vn->sock);
+	if (rc < 0) {
+		pr_debug("UDP socket create failed\n");
+		return rc;
+	}

And this is where we have the issue.

sock_create_kern only creates sockets in the initial network namespace.
There is inet_ctl_sock_create which comes closer to what you want
but I expect you want your socket to be hashed.

Still we need to do something here to avoid have a socket in the
network namespace that has a reference count on the network namespace
and keeps the network namespace from exiting.

We very clearly don't have a good interface for handling this at
the moment.  I am drawing a blank at the moment on exactly what
such an interface should look like.

What we have is certainly error prone for use inside the kernel.
I have a suspicion the nfs server code that uses __sock_create
has the potential to forever pin a network namespace.

int sock_create_netns(struct net *net, int family, int type, int protocol,
                         struct socket **res)
{
        int err;
	err = __sock_create(&init_net, family, type, protocol, res, 1);
        if (err == 0) {
 	       sk_change_net(sock->sk, net);
        return err;
}

Although I am beginning to suspect we should do the silly refcount
avoidance for all in kernel sockets, and just pass the kern parameter
all of the way down to sk_alloc, so it can get the refcounting right
the first time.

However for the bug fix for the merge window (since it appears Dave
merged this code). 

I suggest you just add the sk_change_net and change the socket release
to sk_release_kern in release_net.  At least that is localized, and
doesn't require us to clean up the API for in kernel sockets in a rush.

Eric


+	vxlan_addr.sin_port = htons(vxlan_port);
+
+	rc = kernel_bind(vn->sock, (struct sockaddr *) &vxlan_addr,
+			 sizeof(vxlan_addr));
+	if (rc < 0) {
+		pr_debug("bind for UDP socket %pI4:%u (%d)\n",
+			 &vxlan_addr.sin_addr, ntohs(vxlan_addr.sin_port), rc);
+		sock_release(vn->sock);
+		vn->sock = NULL;
+		return rc;
+	}


Eric

Re: network namespace and kernel bind issue

[Stephen Hemminger]

The problem was vxlan wasn't doing sk_change_net on the created socket.

I'm testing that fix.

The long term fix is to change sock_create_kern() to take a 'struct net'
argument. This would avoid the trap of having to change the namespace.
Also several places using __sock_create() could use it.

L2TP still looks to have several namespace related issues.

[PATCH net-next] vxlan: put UDP socket in correct namespace

[Stephen Hemminger]

Move vxlan UDP socket to correct network namespace

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/net/vxlan.c	2012-10-01 17:18:30.776513263 -0700
+++ b/drivers/net/vxlan.c	2012-10-01 17:42:28.340411631 -0700
@@ -1136,6 +1136,9 @@ static __net_init int vxlan_init_net(str
 		pr_debug("UDP socket create failed\n");
 		return rc;
 	}
+	/* Put in proper namespace */
+	sk = vn->sock->sk;
+	sk_change_net(sk, net);
 
 	vxlan_addr.sin_port = htons(vxlan_port);
 
@@ -1150,7 +1153,6 @@ static __net_init int vxlan_init_net(str
 	}
 
 	/* Disable multicast loopback */
-	sk = vn->sock->sk;
 	inet_sk(sk)->mc_loop = 0;
 
 	/* Mark socket as an encapsulation socket. */

Re: [PATCH net-next] vxlan: put UDP socket in correct namespace

[Eric W. Biederman]

Stephen Hemminger <shemminger@vyatta.com> writes:

> Move vxlan UDP socket to correct network namespace

You also need to replease sock_release with
sk_release_kernel.

Otherwise you will decrement the network namespace count
below zero, when sock_release is called.

Eric

> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
>
> --- a/drivers/net/vxlan.c	2012-10-01 17:18:30.776513263 -0700
> +++ b/drivers/net/vxlan.c	2012-10-01 17:42:28.340411631 -0700
> @@ -1136,6 +1136,9 @@ static __net_init int vxlan_init_net(str
>  		pr_debug("UDP socket create failed\n");
>  		return rc;
>  	}
> +	/* Put in proper namespace */
> +	sk = vn->sock->sk;
> +	sk_change_net(sk, net);
>  
>  	vxlan_addr.sin_port = htons(vxlan_port);
>  
> @@ -1150,7 +1153,6 @@ static __net_init int vxlan_init_net(str
>  	}
>  
>  	/* Disable multicast loopback */
> -	sk = vn->sock->sk;
>  	inet_sk(sk)->mc_loop = 0;
>  
>  	/* Mark socket as an encapsulation socket. */

[PATCHv2 net-next] vxlan: put UDP socket in correct namespace

[Stephen Hemminger]

Move vxlan UDP socket to correct network namespace

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
---
v2 need to change sock_release to sk_release_kernel

--- a/drivers/net/vxlan.c	2012-10-01 17:18:30.776513263 -0700
+++ b/drivers/net/vxlan.c	2012-10-01 21:47:40.435979178 -0700
@@ -1136,6 +1136,9 @@ static __net_init int vxlan_init_net(str
 		pr_debug("UDP socket create failed\n");
 		return rc;
 	}
+	/* Put in proper namespace */
+	sk = vn->sock->sk;
+	sk_change_net(sk, net);
 
 	vxlan_addr.sin_port = htons(vxlan_port);
 
@@ -1144,13 +1147,12 @@ static __net_init int vxlan_init_net(str
 	if (rc < 0) {
 		pr_debug("bind for UDP socket %pI4:%u (%d)\n",
 			 &vxlan_addr.sin_addr, ntohs(vxlan_addr.sin_port), rc);
-		sock_release(vn->sock);
+		sk_release_kernel(sk);
 		vn->sock = NULL;
 		return rc;
 	}
 
 	/* Disable multicast loopback */
-	sk = vn->sock->sk;
 	inet_sk(sk)->mc_loop = 0;
 
 	/* Mark socket as an encapsulation socket. */
@@ -1169,7 +1171,7 @@ static __net_exit void vxlan_exit_net(st
 	struct vxlan_net *vn = net_generic(net, vxlan_net_id);
 
 	if (vn->sock) {
-		sock_release(vn->sock);
+		sk_release_kernel(vn->sock->sk);
 		vn->sock = NULL;
 	}
 }

Re: network namespace and kernel bind issue

[Julian Anastasov]

	Hello,

On Mon, 1 Oct 2012, Stephen Hemminger wrote:

> The problem was vxlan wasn't doing sk_change_net on the created socket.
> 
> I'm testing that fix.
> 
> The long term fix is to change sock_create_kern() to take a 'struct net'
> argument. This would avoid the trap of having to change the namespace.
> Also several places using __sock_create() could use it.
> 
> L2TP still looks to have several namespace related issues.

	There should be also .netnsok = true in l2tp_nl_family
as final step.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCHv2 net-next] vxlan: put UDP socket in correct namespace

[Eric W. Biederman]

Stephen Hemminger <shemminger@vyatta.com> writes:

> Move vxlan UDP socket to correct network namespace
>
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Looks good from here.

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

> ---
> v2 need to change sock_release to sk_release_kernel
>
> --- a/drivers/net/vxlan.c	2012-10-01 17:18:30.776513263 -0700
> +++ b/drivers/net/vxlan.c	2012-10-01 21:47:40.435979178 -0700
> @@ -1136,6 +1136,9 @@ static __net_init int vxlan_init_net(str
>  		pr_debug("UDP socket create failed\n");
>  		return rc;
>  	}
> +	/* Put in proper namespace */
> +	sk = vn->sock->sk;
> +	sk_change_net(sk, net);
>  
>  	vxlan_addr.sin_port = htons(vxlan_port);
>  
> @@ -1144,13 +1147,12 @@ static __net_init int vxlan_init_net(str
>  	if (rc < 0) {
>  		pr_debug("bind for UDP socket %pI4:%u (%d)\n",
>  			 &vxlan_addr.sin_addr, ntohs(vxlan_addr.sin_port), rc);
> -		sock_release(vn->sock);
> +		sk_release_kernel(sk);
>  		vn->sock = NULL;
>  		return rc;
>  	}
>  
>  	/* Disable multicast loopback */
> -	sk = vn->sock->sk;
>  	inet_sk(sk)->mc_loop = 0;
>  
>  	/* Mark socket as an encapsulation socket. */
> @@ -1169,7 +1171,7 @@ static __net_exit void vxlan_exit_net(st
>  	struct vxlan_net *vn = net_generic(net, vxlan_net_id);
>  
>  	if (vn->sock) {
> -		sock_release(vn->sock);
> +		sk_release_kernel(vn->sock->sk);
>  		vn->sock = NULL;
>  	}
>  }
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: network namespace and kernel bind issue

[Stephen Hemminger]

On Tue, 2 Oct 2012 09:15:46 +0300 (EEST)
Julian Anastasov <ja@ssi.bg> wrote:

> 
> 	Hello,
> 
> On Mon, 1 Oct 2012, Stephen Hemminger wrote:
> 
> > The problem was vxlan wasn't doing sk_change_net on the created socket.
> > 
> > I'm testing that fix.
> > 
> > The long term fix is to change sock_create_kern() to take a 'struct net'
> > argument. This would avoid the trap of having to change the namespace.
> > Also several places using __sock_create() could use it.
> > 
> > L2TP still looks to have several namespace related issues.
> 
> 	There should be also .netnsok = true in l2tp_nl_family
> as final step.
> 
> Regards
> 
> --
> Julian Anastasov <ja@ssi.bg>

There are actually lots more net namespace issues in L2TP.
For example session and tunnel counts are global, not per namespace.

Re: [PATCHv2 net-next] vxlan: put UDP socket in correct namespace

[David Miller]

From: ebiederm@xmission.com (Eric W. Biederman)
Date: Tue, 02 Oct 2012 08:18:13 -0700

> Stephen Hemminger <shemminger@vyatta.com> writes:
> 
>> Move vxlan UDP socket to correct network namespace
>>
>> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> Looks good from here.
> 
> Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>

Applied, thanks.