2.6.38 containers bug: Infinite loop in /proc/sys/net/ipv6/neigh/neigh/neigh...

In the host context a find on /proc completes, but inside an lxc container a find on /proc never completes, due to the endless loop in the title. (It's not a symlink, it seems to be a cross linked directory.)

This is vanilla 2.6.38, I can attach my .config if you think it'd help.
(The container's the lxc debian sid template but that's probably not
relevant.)

Rob

From: Rob Landley <[email protected]>
Date: Wed, 30 Mar 2011 03:29:16 -0500

Please CC: [email protected] when a bug might be related to networking, as is obviously the case here.

This bug should be fixed by the following patch:

--------------------
commit 9d2a8fa96a44ba242de3a6f56acaef7a40a97b97

Author: Eric W. Biederman <[email protected]>
Date:   Mon Mar 21 18:23:34 2011 -0700

net ipv6: Fix duplicate /proc/sys/net/ipv6/neigh directory entries.

When I was fixing issues with unregisgtering tables under /proc/sys/net/ipv6/neigh by adding a mount point it appears I missed a critical ordering issue, in the ipv6 initialization. I had not realized that ipv6_sysctl_register is called at the very end of the ipv6 initialization and in particular after we call neigh_sysctl_register from ndisc_init.

"neigh" needs to be initialized in ipv6_static_sysctl_register which is the first ipv6 table to initialized, and definitely before ndisc_init. This removes the weirdness of duplicate tables while still providing a "neigh" mount point which prevents races in sysctl unregistering.

This was initially reported at https://bugzilla.kernel.org/show_bug.cgi?id=31232

index 7cb65ef..6dcf5e7 100644
@@ -17,6 +17,16 @@
 
 static struct ctl_table empty[1];
 
+static ctl_table ipv6_static_skeleton[] = {
+	{
+		.procname	= "neigh",
+		.maxlen		= 0,
+		.mode		= 0555,
+		.child		= empty,
+	},
+	{ }
+};
+
 static ctl_table ipv6_table_template[] = {
 	{
 		.procname	= "route",

@@ -37,12 +47,6 @@ static ctl_table ipv6_table_template[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec
 	},
-	{
-		.procname	= "neigh",
-		.maxlen		= 0,
-		.mode		= 0555,
-		.child		= empty,
-	},
 	{ }
 };
 

@@ -160,7 +164,7 @@ static struct ctl_table_header *ip6_base;
 
 int ipv6_static_sysctl_register(void)
 {
-	ip6_base = register_sysctl_paths(net_ipv6_ctl_path, empty);
+	ip6_base = register_sysctl_paths(net_ipv6_ctl_path, ipv6_static_skeleton);
 	if (ip6_base == NULL)
 		return -ENOMEM;
 	return 0;

Yes, that fixed it. Pinging the stable guys to make sure this goes in a dot release.

Thanks,

Rob

From: Rob Landley <[email protected]>
Date: Wed, 30 Mar 2011 05:16:19 -0500

Please do not do this.

I let fixes soak in Linus's tree for a week or even more before I submit them to -stable.

Too late, but presumably they got your NAK.

So you think letting it soak in the merge window and the churn of -rc1 (which currently doesn't boot for me due to an ide issue) will provide more validity than a specific "There was an infinite loop in the filesystem, there is now no longer an infinite loop in the filesystem, here's the specific test for it" and visual inspection ofthe patch?

*shrug* I hear what you say. It's certainly a point of view.

Rob

From: Rob Landley <[email protected]>
Date: Wed, 30 Mar 2011 05:45:08 -0500

Yes I absolutely do think it's better to let it soak somewhere than to repeat the reason why this patch is necessary in the first place.

This patch is necessary because Eric already screwed up trying to fix this neigh dirctory under ipv6 once, and he got it wrong.

That patch looked correct and straightforward.

Yet it introduced the bug you're seeing.

Given that consideration, are you still holding steady to your opinion wise guy? :-)

The patch fixed the specific bug I hit (find /proc hanging), and didn't noticeably make the system worse in my use cases. I have a specific test case which used to fail and now succeeds. The system _without_ the patch is broken, and I weigh the possibility of breakage with the patch against that. I wasn't particularly concerned with it being the "right" fix from a long-term structural standpoint because that's what 2.6.39 is for. I just wanted the obvious breakage (a regression from 2.6.37) to go away, I tested this fix and it Worked For Me (tm), hence letting the stable guys know about it being the obvious next move.

How many people do you think are likely to even try containers under 2.6.39-rc1 or -rc2? Eric's patch introducing the bug is dated January 31, and it was in 2.6.38-rc4. Apparently nobody noticed the breakage in -rc5, -rc6, -rc7, or -rc8. His patch fixing the bug is dated one week after 2.6.38 shipped. I also noticed it in the stable kernel, not in any of the 2.6.38-rc releases. So at least two people hit this in -stable already, and nobody hit it in -dev for four -rc releases. I have tested it in 2.6.38 and it Worked For Me. Supposing the new patch did introduce subtle breakage, how is leaving it in the 2.6.39-dev series for a _month_ more likely to find it than the entire second half of the 2.6.38 development cycle (when it was at its most stable, and thus easiest to test)?

I have no idea if this patch fixes 2.6.39-rc1, because -rc1 still doesn't _boot_ in my test environment. Many other things about -rc1 are broken. That's sort of the point of -rc1. A fairly small number of people test the development branch, and this is the most unstable time of the development cycle where many things about it are _expected_ not to work. Especially right now, I don't see how letting it "soak" for a week or two in the -dev branch accomplishes anything whatsoever.

So yes, I am still holding steady to my opinion. But you're the maintainer, so my opinion doesn't really matter here.

Rob

The patch fixed the specific bug I hit (find /proc hanging), and didn't noticeably make the system worse in my use cases. I have a specific test case which used to fail and now succeeds. The system _without_ the patch is broken, and I weigh the possibility of breakage with the patch against that. I wasn't particularly concerned with it being the "right" fix from a long-term structural standpoint because that's what 2.6.39 is for. I just wanted the obvious breakage (a regression from 2.6.37) to go away, I tested this fix and it Worked For Me (tm), hence letting the stable guys know about it being the obvious next move.

How many people do you think are likely to even try containers under 2.6.39-rc1 or -rc2? Eric's patch introducing the bug is dated January 31, and it was in 2.6.38-rc4. Apparently nobody noticed the breakage in -rc5, -rc6, -rc7, or -rc8. His patch fixing the bug is dated one week after 2.6.38 shipped. I also noticed it in the stable kernel, not in any of the 2.6.38-rc releases. So at least two people hit this in -stable already, and nobody hit it in -dev for four -rc releases. I have tested it in 2.6.38 and it Worked For Me. Supposing the new patch did introduce subtle breakage, how is leaving it in the 2.6.39-dev series for a _month_ more likely to find it than the entire second half of the 2.6.38 development cycle (when it was at its most stable, and thus easiest to test)?

I have no idea if this patch fixes 2.6.39-rc1, because -rc1 still doesn't _boot_ in my test environment. Many other things about -rc1 are broken. That's sort of the point of -rc1. A fairly small number of people test the development branch, and this is the most unstable time of the development cycle where many things about it are _expected_ not to work. Especially right now, I don't see how letting it "soak" for a week or two in the -dev branch accomplishes anything whatsoever.

So yes, I am still holding steady to my opinion. But you're the maintainer, so my opinion doesn't really matter here.

Rob

From: Rob Landley <[email protected]>
Date: Wed, 30 Mar 2011 05:45:08 -0500

Yes I absolutely do think it's better to let it soak somewhere than to repeat the reason why this patch is necessary in the first place.

This patch is necessary because Eric already screwed up trying to fix this neigh dirctory under ipv6 once, and he got it wrong.

That patch looked correct and straightforward.

Yet it introduced the bug you're seeing.

Given that consideration, are you still holding steady to your opinion wise guy? :-)

Too late, but presumably they got your NAK.

So you think letting it soak in the merge window and the churn of -rc1 (which currently doesn't boot for me due to an ide issue) will provide more validity than a specific "There was an infinite loop in the filesystem, there is now no longer an infinite loop in the filesystem, here's the specific test for it" and visual inspection ofthe patch?

*shrug* I hear what you say. It's certainly a point of view.

Rob

From: Rob Landley <[email protected]>
Date: Wed, 30 Mar 2011 05:16:19 -0500

Please do not do this.

I let fixes soak in Linus's tree for a week or even more before I submit them to -stable.

Yes, that fixed it. Pinging the stable guys to make sure this goes in a dot release.

Thanks,

Rob

From: Rob Landley <[email protected]>
Date: Wed, 30 Mar 2011 03:29:16 -0500

Please CC: [email protected] when a bug might be related to networking, as is obviously the case here.

This bug should be fixed by the following patch:

--------------------
commit 9d2a8fa96a44ba242de3a6f56acaef7a40a97b97

Author: Eric W. Biederman <[email protected]>
Date:   Mon Mar 21 18:23:34 2011 -0700

net ipv6: Fix duplicate /proc/sys/net/ipv6/neigh directory entries.

When I was fixing issues with unregisgtering tables under /proc/sys/net/ipv6/neigh by adding a mount point it appears I missed a critical ordering issue, in the ipv6 initialization. I had not realized that ipv6_sysctl_register is called at the very end of the ipv6 initialization and in particular after we call neigh_sysctl_register from ndisc_init.

"neigh" needs to be initialized in ipv6_static_sysctl_register which is the first ipv6 table to initialized, and definitely before ndisc_init. This removes the weirdness of duplicate tables while still providing a "neigh" mount point which prevents races in sysctl unregistering.

This was initially reported at https://bugzilla.kernel.org/show_bug.cgi?id=31232

index 7cb65ef..6dcf5e7 100644
@@ -17,6 +17,16 @@
 
 static struct ctl_table empty[1];
 
+static ctl_table ipv6_static_skeleton[] = {
+	{
+		.procname	= "neigh",
+		.maxlen		= 0,
+		.mode		= 0555,
+		.child		= empty,
+	},
+	{ }
+};
+
 static ctl_table ipv6_table_template[] = {
 	{
 		.procname	= "route",

@@ -37,12 +47,6 @@ static ctl_table ipv6_table_template[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec
 	},
-	{
-		.procname	= "neigh",
-		.maxlen		= 0,
-		.mode		= 0555,
-		.child		= empty,
-	},
 	{ }
 };
 

@@ -160,7 +164,7 @@ static struct ctl_table_header *ip6_base;
 
 int ipv6_static_sysctl_register(void)
 {
-	ip6_base = register_sysctl_paths(net_ipv6_ctl_path, empty);
+	ip6_base = register_sysctl_paths(net_ipv6_ctl_path, ipv6_static_skeleton);
 	if (ip6_base == NULL)
 		return -ENOMEM;
 	return 0;