[PATCH for 3.8] iproute2: Add "ip netns pids" and "ip netns identify"

[PATCH for 3.8] iproute2: Add "ip netns pids" and "ip netns identify"

[Eric W. Biederman]

Add command that go between network namespace names and process
identifiers.  The code builds and runs agains older kernels but
only works on Linux 3.8+ kernels where I have fixed stat to work
properly.

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

I don't know if this is too soon to send this patch to iproute as the
kernel code that fixes stat is currently sitting in my for-next branch
of:
git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git

and has not hit Linus's tree yet.  Still the code runs and is harmless
on older kernels so it should be harmless whatever happens with it.

 ip/ipnetns.c        |  141 +++++++++++++++++++++++++++++++++++++++++++++++++++
 man/man8/ip-netns.8 |    5 ++-
 2 files changed, 145 insertions(+), 1 deletions(-)

diff --git a/ip/ipnetns.c b/ip/ipnetns.c
index e41a598..c55fe3a 100644
--- a/ip/ipnetns.c
+++ b/ip/ipnetns.c
@@ -13,6 +13,7 @@
 #include <dirent.h>
 #include <errno.h>
 #include <unistd.h>
+#include <ctype.h>
 
 #include "utils.h"
 #include "ip_common.h"
@@ -48,6 +49,8 @@ static void usage(void)
 	fprintf(stderr, "Usage: ip netns list\n");
 	fprintf(stderr, "       ip netns add NAME\n");
 	fprintf(stderr, "       ip netns delete NAME\n");
+	fprintf(stderr, "       ip netns identify PID\n");
+	fprintf(stderr, "       ip netns pids NAME\n");
 	fprintf(stderr, "       ip netns exec NAME cmd ...\n");
 	fprintf(stderr, "       ip netns monitor\n");
 	exit(-1);
@@ -171,6 +174,138 @@ static int netns_exec(int argc, char **argv)
 	exit(-1);
 }
 
+static int is_pid(const char *str)
+{
+	int ch;
+	for (; (ch = *str); str++) {
+		if (!isdigit(ch))
+			return 0;
+	}
+	return 1;
+}
+
+static int netns_pids(int argc, char **argv)
+{
+	const char *name;
+	char net_path[MAXPATHLEN];
+	int netns;
+	struct stat netst;
+	DIR *dir;
+	struct dirent *entry;
+
+	if (argc < 1) {
+		fprintf(stderr, "No netns name specified\n");
+		return -1;
+	}
+	if (argc > 1) {
+		fprintf(stderr, "extra arguments specified\n");
+		return -1;
+	}
+
+	name = argv[0];
+	snprintf(net_path, sizeof(net_path), "%s/%s", NETNS_RUN_DIR, name);
+	netns = open(net_path, O_RDONLY);
+	if (netns < 0) {
+		fprintf(stderr, "Cannot open network namespace: %s\n",
+			strerror(errno));
+		return -1;
+	}
+	if (fstat(netns, &netst) < 0) {
+		fprintf(stderr, "Stat of netns failed: %s\n",
+			strerror(errno));
+		return -1;
+	}
+	dir = opendir("/proc/");
+	if (!dir) {
+		fprintf(stderr, "Open of /proc failed: %s\n",
+			strerror(errno));
+		return -1;
+	}
+	while((entry = readdir(dir))) {
+		char pid_net_path[MAXPATHLEN];
+		struct stat st;
+		if (!is_pid(entry->d_name))
+			continue;
+		snprintf(pid_net_path, sizeof(pid_net_path), "/proc/%s/ns/net",
+			entry->d_name);
+		if (stat(pid_net_path, &st) != 0)
+			continue;
+		if ((st.st_dev == netst.st_dev) &&
+		    (st.st_ino == netst.st_ino)) {
+			printf("%s\n", entry->d_name);
+		}
+	}
+	closedir(dir);
+	return 0;
+	
+}
+
+static int netns_identify(int argc, char **argv)
+{
+	const char *pidstr;
+	char net_path[MAXPATHLEN];
+	int netns;
+	struct stat netst;
+	DIR *dir;
+	struct dirent *entry;
+
+	if (argc < 1) {
+		fprintf(stderr, "No pid specified\n");
+		return -1;
+	}
+	if (argc > 1) {
+		fprintf(stderr, "extra arguments specified\n");
+		return -1;
+	}
+	pidstr = argv[0];
+
+	if (!is_pid(pidstr)) {
+		fprintf(stderr, "Specified string '%s' is not a pid\n",
+			pidstr);
+		return -1;
+	}
+
+	snprintf(net_path, sizeof(net_path), "/proc/%s/ns/net", pidstr);
+	netns = open(net_path, O_RDONLY);
+	if (netns < 0) {
+		fprintf(stderr, "Cannot open network namespace: %s\n",
+			strerror(errno));
+		return -1;
+	}
+	if (fstat(netns, &netst) < 0) {
+		fprintf(stderr, "Stat of netns failed: %s\n",
+			strerror(errno));
+		return -1;
+	}
+	dir = opendir(NETNS_RUN_DIR);
+	if (!dir)
+		return 0;
+
+	while((entry = readdir(dir))) {
+		char name_path[MAXPATHLEN];
+		struct stat st;
+
+		if (strcmp(entry->d_name, ".") == 0)
+			continue;
+		if (strcmp(entry->d_name, "..") == 0)
+			continue;
+
+		snprintf(name_path, sizeof(name_path), "%s/%s",	NETNS_RUN_DIR,
+			entry->d_name);
+
+		if (stat(name_path, &st) != 0)
+			continue;
+
+		if ((st.st_dev == netst.st_dev) &&
+		    (st.st_ino == netst.st_ino)) {
+			printf("%s\n", entry->d_name);
+		}
+	}
+	closedir(dir);
+	return 0;
+	
+}
+
 static int netns_delete(int argc, char **argv)
 {
 	const char *name;
@@ -298,6 +433,12 @@ int do_netns(int argc, char **argv)
 	if (matches(*argv, "delete") == 0)
 		return netns_delete(argc-1, argv+1);
 
+	if (matches(*argv, "identify") == 0)
+		return netns_identify(argc-1, argv+1);
+
+	if (matches(*argv, "pids") == 0)
+		return netns_pids(argc-1, argv+1);
+
 	if (matches(*argv, "exec") == 0)
 		return netns_exec(argc-1, argv+1);
 
diff --git a/man/man8/ip-netns.8 b/man/man8/ip-netns.8
index 349ee7e..e639836 100644
--- a/man/man8/ip-netns.8
+++ b/man/man8/ip-netns.8
@@ -1,4 +1,4 @@
-.TH IP\-NETNS 8 "20 Dec 2011" "iproute2" "Linux"
+.TH IP\-NETNS 8 "26 Dec 2012" "iproute2" "Linux"
 .SH NAME
 ip-netns \- process network namespace management
 .SH SYNOPSIS
@@ -58,6 +58,9 @@ their traditional location in /etc.
 .SS ip netns delete NAME - delete the name of a network namespace
 .SS ip netns exec NAME cmd ... - Run cmd in the named network namespace
 
+.SS ip netns pids NAME - Report processes in the named network namespace
+.SS ip netns identify PID - Report network namespaces names for process
+
 .SH EXAMPLES
 
 .SH SEE ALSO
-- 
1.7.5.4

Re: [PATCH for 3.8] iproute2: Add "ip netns pids" and "ip netns identify"

[Ben Hutchings]

On Mon, 2012-11-26 at 17:16 -0600, Eric W. Biederman wrote:
> Add command that go between network namespace names and process
> identifiers.  The code builds and runs agains older kernels but
> only works on Linux 3.8+ kernels where I have fixed stat to work
> properly.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
> 
> I don't know if this is too soon to send this patch to iproute as the
> kernel code that fixes stat is currently sitting in my for-next branch
> of:
> git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git
> 
> and has not hit Linus's tree yet.  Still the code runs and is harmless
> on older kernels so it should be harmless whatever happens with it.
> 
>  ip/ipnetns.c        |  141 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  man/man8/ip-netns.8 |    5 ++-
>  2 files changed, 145 insertions(+), 1 deletions(-)
> 
> diff --git a/ip/ipnetns.c b/ip/ipnetns.c
> index e41a598..c55fe3a 100644
> --- a/ip/ipnetns.c
> +++ b/ip/ipnetns.c
> @@ -13,6 +13,7 @@
>  #include <dirent.h>
>  #include <errno.h>
>  #include <unistd.h>
> +#include <ctype.h>
>  
>  #include "utils.h"
>  #include "ip_common.h"
> @@ -48,6 +49,8 @@ static void usage(void)
>  	fprintf(stderr, "Usage: ip netns list\n");
>  	fprintf(stderr, "       ip netns add NAME\n");
>  	fprintf(stderr, "       ip netns delete NAME\n");
> +	fprintf(stderr, "       ip netns identify PID\n");
> +	fprintf(stderr, "       ip netns pids NAME\n");
>  	fprintf(stderr, "       ip netns exec NAME cmd ...\n");
>  	fprintf(stderr, "       ip netns monitor\n");
>  	exit(-1);
> @@ -171,6 +174,138 @@ static int netns_exec(int argc, char **argv)
>  	exit(-1);
>  }
>  
> +static int is_pid(const char *str)
> +{
> +	int ch;
> +	for (; (ch = *str); str++) {
> +		if (!isdigit(ch))

ch must be cast to unsigned char before passing to isdigit().

> +			return 0;
> +	}
> +	return 1;
> +}
> +
> +static int netns_pids(int argc, char **argv)
> +{
> +	const char *name;
> +	char net_path[MAXPATHLEN];
> +	int netns;
> +	struct stat netst;
> +	DIR *dir;
> +	struct dirent *entry;
> +
> +	if (argc < 1) {
> +		fprintf(stderr, "No netns name specified\n");
> +		return -1;
> +	}
> +	if (argc > 1) {
> +		fprintf(stderr, "extra arguments specified\n");
> +		return -1;
> +	}

These, and many other return statements in this file which set the
process exit code, should return 1 (general failure) or 2 (user error)
rather than -1 (likely to be interpreted as command not found).

> +	name = argv[0];
> +	snprintf(net_path, sizeof(net_path), "%s/%s", NETNS_RUN_DIR, name);

No check for truncation?

> +	netns = open(net_path, O_RDONLY);

This file descriptor is leaked, though that probably doesn't really
matter.

[...]
> +static int netns_identify(int argc, char **argv)
> +{
> +	const char *pidstr;
> +	char net_path[MAXPATHLEN];
> +	int netns;
> +	struct stat netst;
> +	DIR *dir;
> +	struct dirent *entry;
> +
> +	if (argc < 1) {
> +		fprintf(stderr, "No pid specified\n");
> +		return -1;
> +	}
> +	if (argc > 1) {
> +		fprintf(stderr, "extra arguments specified\n");
> +		return -1;
> +	}
> +	pidstr = argv[0];
> +
> +	if (!is_pid(pidstr)) {
> +		fprintf(stderr, "Specified string '%s' is not a pid\n",
> +			pidstr);
> +		return -1;
> +	}
> +
> +	snprintf(net_path, sizeof(net_path), "/proc/%s/ns/net", pidstr);
> +	netns = open(net_path, O_RDONLY);
> +	if (netns < 0) {
> +		fprintf(stderr, "Cannot open network namespace: %s\n",
> +			strerror(errno));
> +		return -1;
> +	}
> +	if (fstat(netns, &netst) < 0) {
> +		fprintf(stderr, "Stat of netns failed: %s\n",
> +			strerror(errno));
> +		return -1;
> +	}
> +	dir = opendir(NETNS_RUN_DIR);
> +	if (!dir)
> +		return 0;

Shouldn't this be treated as an error?  Or, if you want it to succeed
when the kernel does not have netns functionality, then treat it as an
error if !dir && errno != ENOENT.

> +	while((entry = readdir(dir))) {
> +		char name_path[MAXPATHLEN];
> +		struct stat st;
> +
> +		if (strcmp(entry->d_name, ".") == 0)
> +			continue;
> +		if (strcmp(entry->d_name, "..") == 0)
> +			continue;
> +
> +		snprintf(name_path, sizeof(name_path), "%s/%s",	NETNS_RUN_DIR,
> +			entry->d_name);
> +
> +		if (stat(name_path, &st) != 0)
> +			continue;
> +
> +		if ((st.st_dev == netst.st_dev) &&
> +		    (st.st_ino == netst.st_ino)) {
> +			printf("%s\n", entry->d_name);
> +		}
> +	}
> +	closedir(dir);
> +	return 0;
> +	
> +}
[...]

It's a shame there isn't a more efficient way to do these lookups.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH for 3.8] iproute2: Add "ip netns pids" and "ip netns identify"

[Eric W. Biederman]

Ben Hutchings <bhutchings@solarflare.com> writes:

> On Mon, 2012-11-26 at 17:16 -0600, Eric W. Biederman wrote:
>> Add command that go between network namespace names and process
>> identifiers.  The code builds and runs agains older kernels but
>> only works on Linux 3.8+ kernels where I have fixed stat to work
>> properly.
>> 
>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> ---
>> 
>> I don't know if this is too soon to send this patch to iproute as the
>> kernel code that fixes stat is currently sitting in my for-next branch
>> of:
>> git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git
>> 
>> and has not hit Linus's tree yet.  Still the code runs and is harmless
>> on older kernels so it should be harmless whatever happens with it.
>> 
>>  ip/ipnetns.c        |  141 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  man/man8/ip-netns.8 |    5 ++-
>>  2 files changed, 145 insertions(+), 1 deletions(-)
>> 
>> diff --git a/ip/ipnetns.c b/ip/ipnetns.c
>> index e41a598..c55fe3a 100644
>> --- a/ip/ipnetns.c
>> +++ b/ip/ipnetns.c
>> @@ -13,6 +13,7 @@
>>  #include <dirent.h>
>>  #include <errno.h>
>>  #include <unistd.h>
>> +#include <ctype.h>
>>  
>>  #include "utils.h"
>>  #include "ip_common.h"
>> @@ -48,6 +49,8 @@ static void usage(void)
>>  	fprintf(stderr, "Usage: ip netns list\n");
>>  	fprintf(stderr, "       ip netns add NAME\n");
>>  	fprintf(stderr, "       ip netns delete NAME\n");
>> +	fprintf(stderr, "       ip netns identify PID\n");
>> +	fprintf(stderr, "       ip netns pids NAME\n");
>>  	fprintf(stderr, "       ip netns exec NAME cmd ...\n");
>>  	fprintf(stderr, "       ip netns monitor\n");
>>  	exit(-1);
>> @@ -171,6 +174,138 @@ static int netns_exec(int argc, char **argv)
>>  	exit(-1);
>>  }
>>  
>> +static int is_pid(const char *str)
>> +{
>> +	int ch;
>> +	for (; (ch = *str); str++) {
>> +		if (!isdigit(ch))
>
> ch must be cast to unsigned char before passing to isdigit().

isdigit is defined to take an int.  A legacy of the implicit casts in
the K&R C days.  Casting to unsigned char would be pointless and silly.

>> +			return 0;
>> +	}
>> +	return 1;
>> +}
>> +
>> +static int netns_pids(int argc, char **argv)
>> +{
>> +	const char *name;
>> +	char net_path[MAXPATHLEN];
>> +	int netns;
>> +	struct stat netst;
>> +	DIR *dir;
>> +	struct dirent *entry;
>> +
>> +	if (argc < 1) {
>> +		fprintf(stderr, "No netns name specified\n");
>> +		return -1;
>> +	}
>> +	if (argc > 1) {
>> +		fprintf(stderr, "extra arguments specified\n");
>> +		return -1;
>> +	}
>
> These, and many other return statements in this file which set the
> process exit code, should return 1 (general failure) or 2 (user error)
> rather than -1 (likely to be interpreted as command not found).

Good point.  

>> +	name = argv[0];
>> +	snprintf(net_path, sizeof(net_path), "%s/%s", NETNS_RUN_DIR, name);
>
> No check for truncation?

Nope.  snprintf guarantees returning a '\0' terminated string, so there
is no point in supplying a bad string to get the program to crash or act
in odd ways.  It might be more friendly to say:  "Hey silly that string
you passed me was too long to use for anyting, don't do that".  But I
don't think it is worth the code complexity to maintain the extra error
message, for something that in my experience people just don't do.

>> +	netns = open(net_path, O_RDONLY);
>
> This file descriptor is leaked, though that probably doesn't really
> matter.

Good point.  Probably worth fixing in case someone figures out how
to use these commands in batch mode.

Thanks fixed.

> [...]
>> +static int netns_identify(int argc, char **argv)
>> +{
>> +	const char *pidstr;
>> +	char net_path[MAXPATHLEN];
>> +	int netns;
>> +	struct stat netst;
>> +	DIR *dir;
>> +	struct dirent *entry;
>> +
>> +	if (argc < 1) {
>> +		fprintf(stderr, "No pid specified\n");
>> +		return -1;
>> +	}
>> +	if (argc > 1) {
>> +		fprintf(stderr, "extra arguments specified\n");
>> +		return -1;
>> +	}
>> +	pidstr = argv[0];
>> +
>> +	if (!is_pid(pidstr)) {
>> +		fprintf(stderr, "Specified string '%s' is not a pid\n",
>> +			pidstr);
>> +		return -1;
>> +	}
>> +
>> +	snprintf(net_path, sizeof(net_path), "/proc/%s/ns/net", pidstr);
>> +	netns = open(net_path, O_RDONLY);
>> +	if (netns < 0) {
>> +		fprintf(stderr, "Cannot open network namespace: %s\n",
>> +			strerror(errno));
>> +		return -1;
>> +	}
>> +	if (fstat(netns, &netst) < 0) {
>> +		fprintf(stderr, "Stat of netns failed: %s\n",
>> +			strerror(errno));
>> +		return -1;
>> +	}
>> +	dir = opendir(NETNS_RUN_DIR);
>> +	if (!dir)
>> +		return 0;
>
> Shouldn't this be treated as an error?  Or, if you want it to succeed
> when the kernel does not have netns functionality, then treat it as an
> error if !dir && errno != ENOENT.

What I want is to treat as a missing directory like an empty directory
so we don't error in the case where we simply have not named any network
namespaces yet.

But yes treating it an error when errno != ENOENT makes sense.

And is fixed in my next version.

>> +	while((entry = readdir(dir))) {
>> +		char name_path[MAXPATHLEN];
>> +		struct stat st;
>> +
>> +		if (strcmp(entry->d_name, ".") == 0)
>> +			continue;
>> +		if (strcmp(entry->d_name, "..") == 0)
>> +			continue;
>> +
>> +		snprintf(name_path, sizeof(name_path), "%s/%s",	NETNS_RUN_DIR,
>> +			entry->d_name);
>> +
>> +		if (stat(name_path, &st) != 0)
>> +			continue;
>> +
>> +		if ((st.st_dev == netst.st_dev) &&
>> +		    (st.st_ino == netst.st_ino)) {
>> +			printf("%s\n", entry->d_name);
>> +		}
>> +	}
>> +	closedir(dir);
>> +	return 0;
>> +	
>> +}
> [...]
>
> It's a shame there isn't a more efficient way to do these lookups.

Well there is no index but this just takes a single pass through
either all of the processes or all of the network namespaces.  That
is a simple O(N) algorithm and really isn't inefficient.

These two new commands really are debugging aids to make it easier
to match up processes and network namespaces.  Although at some point
a more generic version of this functionality probably needs to make it's
way into lsof and fuser as well.

I will send my updated version after I have had some sleep and
can double check everything with fresh eyes.

Eric

[PATCH iproute-3.8 0/6] ip netns bug fixes and enhancements

[Eric W. Biederman]

This is a set of patches for iproute2 3.8.  Most of the patches
are just general maintenance and cleanup the final patch adds
some commands to make it possible to find the correlation between
network namespaces and running processes.

The first patch is a significant bug fix for a condition that causes
sysfs to be unmounted on systems that choose to propogate all of the
mounts between their mount namespaces by default.

Eric W. Biederman (6):
      iproute2: Don't propogate mounts out of ip netns exec.
      iproute2: Normalize return codes in "ip netns"
      iproute2: Improve "ip netns add" failure error message
      iproute2: Make "ip netns delete" more likely to succeed
      iproute2: Fill in the ip-netns.8 manpage
      iproute2: Add "ip netns pids" and "ip netns identify"

 ip/ipnetns.c        |  236 ++++++++++++++++++++++++++++++++++++++++++++-------
 man/man8/ip-netns.8 |   64 ++++++++++++++-
 2 files changed, 268 insertions(+), 32 deletions(-)

[PATCH iproute2-3.8 1/6] iproute2: Don't propogate mounts out of ip

[Eric W. Biederman]

Some systems are now following the advice in
linux/Documentation/sharedsubtrees.txt and running with all mount
points shared between all mount namespaces by default.

After creating the mount namespace call mount on / with
MS_SLAVE|MS_REC to modify all mounts in the new mount namespace to
slave mounts if they are shared or private mounts otherwise.
Guarnateeing that changes to the mount namespace created with
"ip netns exec" don't propgate to other namespaces.

Reported-by: Petr Šabata <contyk@redhat.com>
Tested-by: Petr Šabata <contyk@redhat.com>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 ip/ipnetns.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/ip/ipnetns.c b/ip/ipnetns.c
index e41a598..f2c42ba 100644
--- a/ip/ipnetns.c
+++ b/ip/ipnetns.c
@@ -152,6 +152,12 @@ static int netns_exec(int argc, char **argv)
 		fprintf(stderr, "unshare failed: %s\n", strerror(errno));
 		return -1;
 	}
+	/* Don't let any mounts propogate back to the parent */
+	if (mount("", "/", "none", MS_SLAVE | MS_REC, NULL)) {
+		fprintf(stderr, "mount --make-rslave / failed: %s\n",
+			strerror(errno));
+		return -1;
+	}
 	/* Mount a version of /sys that describes the network namespace */
 	if (umount2("/sys", MNT_DETACH) < 0) {
 		fprintf(stderr, "umount of /sys failed: %s\n", strerror(errno));
-- 
1.7.5.4

[PATCH iproute2-3.8 2/6] iproute2: Normalize return codes in "ip netns"

[Eric W. Biederman]

Ben Hutchings pointed out that the return value of do_netns is passed
to exit and the current convention of returning -1 for failure is
inconsitent with that reality.

Return EXIT_FAILURE instead of -1 and EXIT_SUCCESS instead of 0.  To make
it clear that the return codes are expected to be passed to exit.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 ip/ipnetns.c |   56 ++++++++++++++++++++++++++------------------------------
 1 files changed, 26 insertions(+), 30 deletions(-)

diff --git a/ip/ipnetns.c b/ip/ipnetns.c
index f2c42ba..ae55090 100644
--- a/ip/ipnetns.c
+++ b/ip/ipnetns.c
@@ -40,17 +40,14 @@ static int setns(int fd, int nstype)
 }
 #endif /* HAVE_SETNS */
 
-
-static void usage(void) __attribute__((noreturn));
-
-static void usage(void)
+static int usage(void)
 {
 	fprintf(stderr, "Usage: ip netns list\n");
 	fprintf(stderr, "       ip netns add NAME\n");
 	fprintf(stderr, "       ip netns delete NAME\n");
 	fprintf(stderr, "       ip netns exec NAME cmd ...\n");
 	fprintf(stderr, "       ip netns monitor\n");
-	exit(-1);
+	return EXIT_FAILURE;
 }
 
 int get_netns_fd(const char *name)
@@ -75,7 +72,7 @@ static int netns_list(int argc, char **argv)
 
 	dir = opendir(NETNS_RUN_DIR);
 	if (!dir)
-		return 0;
+		return EXIT_SUCCESS;
 
 	while ((entry = readdir(dir)) != NULL) {
 		if (strcmp(entry->d_name, ".") == 0)
@@ -85,7 +82,7 @@ static int netns_list(int argc, char **argv)
 		printf("%s\n", entry->d_name);
 	}
 	closedir(dir);
-	return 0;
+	return EXIT_SUCCESS;
 }
 
 static void bind_etc(const char *name)
@@ -127,11 +124,11 @@ static int netns_exec(int argc, char **argv)
 
 	if (argc < 1) {
 		fprintf(stderr, "No netns name specified\n");
-		return -1;
+		return EXIT_FAILURE;
 	}
 	if (argc < 2) {
 		fprintf(stderr, "No cmd specified\n");
-		return -1;
+		return EXIT_FAILURE;
 	}
 	name = argv[0];
 	cmd = argv[1];
@@ -140,32 +137,32 @@ static int netns_exec(int argc, char **argv)
 	if (netns < 0) {
 		fprintf(stderr, "Cannot open network namespace: %s\n",
 			strerror(errno));
-		return -1;
+		return EXIT_FAILURE;
 	}
 	if (setns(netns, CLONE_NEWNET) < 0) {
 		fprintf(stderr, "seting the network namespace failed: %s\n",
 			strerror(errno));
-		return -1;
+		return EXIT_FAILURE;
 	}
 
 	if (unshare(CLONE_NEWNS) < 0) {
 		fprintf(stderr, "unshare failed: %s\n", strerror(errno));
-		return -1;
+		return EXIT_FAILURE;
 	}
 	/* Don't let any mounts propogate back to the parent */
 	if (mount("", "/", "none", MS_SLAVE | MS_REC, NULL)) {
 		fprintf(stderr, "mount --make-rslave / failed: %s\n",
 			strerror(errno));
-		return -1;
+		return EXIT_FAILURE;
 	}
 	/* Mount a version of /sys that describes the network namespace */
 	if (umount2("/sys", MNT_DETACH) < 0) {
 		fprintf(stderr, "umount of /sys failed: %s\n", strerror(errno));
-		return -1;
+		return EXIT_FAILURE;
 	}
 	if (mount(name, "/sys", "sysfs", 0, NULL) < 0) {
 		fprintf(stderr, "mount of /sys failed: %s\n",strerror(errno));
-		return -1;
+		return EXIT_FAILURE;
 	}
 
 	/* Setup bind mounts for config files in /etc */
@@ -174,7 +171,7 @@ static int netns_exec(int argc, char **argv)
 	if (execvp(cmd, argv + 1)  < 0)
 		fprintf(stderr, "exec of %s failed: %s\n",
 			cmd, strerror(errno));
-	exit(-1);
+	return EXIT_FAILURE;
 }
 
 static int netns_delete(int argc, char **argv)
@@ -184,7 +181,7 @@ static int netns_delete(int argc, char **argv)
 
 	if (argc < 1) {
 		fprintf(stderr, "No netns name specified\n");
-		return -1;
+		return EXIT_FAILURE;
 	}
 
 	name = argv[0];
@@ -193,9 +190,9 @@ static int netns_delete(int argc, char **argv)
 	if (unlink(netns_path) < 0) {
 		fprintf(stderr, "Cannot remove %s: %s\n",
 			netns_path, strerror(errno));
-		return -1;
+		return EXIT_FAILURE;
 	}
-	return 0;
+	return EXIT_SUCCESS;
 }
 
 static int netns_add(int argc, char **argv)
@@ -214,7 +211,7 @@ static int netns_add(int argc, char **argv)
 
 	if (argc < 1) {
 		fprintf(stderr, "No netns name specified\n");
-		return -1;
+		return EXIT_FAILURE;
 	}
 	name = argv[0];
 
@@ -228,7 +225,7 @@ static int netns_add(int argc, char **argv)
 	if (fd < 0) {
 		fprintf(stderr, "Could not create %s: %s\n",
 			netns_path, strerror(errno));
-		return -1;
+		return EXIT_FAILURE;
 	}
 	close(fd);
 	if (unshare(CLONE_NEWNET) < 0) {
@@ -243,11 +240,10 @@ static int netns_add(int argc, char **argv)
 			netns_path, strerror(errno));
 		goto out_delete;
 	}
-	return 0;
+	return EXIT_SUCCESS;
 out_delete:
 	netns_delete(argc, argv);
-	exit(-1);
-	return -1;
+	return EXIT_FAILURE;
 }
 
 
@@ -260,19 +256,19 @@ static int netns_monitor(int argc, char **argv)
 	if (fd < 0) {
 		fprintf(stderr, "inotify_init failed: %s\n",
 			strerror(errno));
-		return -1;
+		return EXIT_FAILURE;
 	}
 	if (inotify_add_watch(fd, NETNS_RUN_DIR, IN_CREATE | IN_DELETE) < 0) {
 		fprintf(stderr, "inotify_add_watch failed: %s\n",
 			strerror(errno));
-		return -1;
+		return EXIT_FAILURE;
 	}
 	for(;;) {
 		ssize_t len = read(fd, buf, sizeof(buf));
 		if (len < 0) {
 			fprintf(stderr, "read failed: %s\n",
 				strerror(errno));
-			return -1;
+			return EXIT_FAILURE;
 		}
 		for (event = (struct inotify_event *)buf;
 		     (char *)event < &buf[len];
@@ -283,7 +279,7 @@ static int netns_monitor(int argc, char **argv)
 				printf("delete %s\n", event->name);
 		}
 	}
-	return 0;
+	return EXIT_SUCCESS;
 }
 
 int do_netns(int argc, char **argv)
@@ -296,7 +292,7 @@ int do_netns(int argc, char **argv)
 		return netns_list(argc-1, argv+1);
 
 	if (matches(*argv, "help") == 0)
-		usage();
+		return usage();
 
 	if (matches(*argv, "add") == 0)
 		return netns_add(argc-1, argv+1);
@@ -311,5 +307,5 @@ int do_netns(int argc, char **argv)
 		return netns_monitor(argc-1, argv+1);
 
 	fprintf(stderr, "Command \"%s\" is unknown, try \"ip netns help\".\n", *argv);
-	exit(-1);
+	return EXIT_FAILURE;
 }
-- 
1.7.5.4

[PATCH iproute2-3.8 3/6] iproute2: Improve "ip netns add" failure error message

[Eric W. Biederman]

Report the name of the network namespace that could not be
added.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 ip/ipnetns.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/ip/ipnetns.c b/ip/ipnetns.c
index ae55090..4fce379 100644
--- a/ip/ipnetns.c
+++ b/ip/ipnetns.c
@@ -135,8 +135,8 @@ static int netns_exec(int argc, char **argv)
 	snprintf(net_path, sizeof(net_path), "%s/%s", NETNS_RUN_DIR, name);
 	netns = open(net_path, O_RDONLY);
 	if (netns < 0) {
-		fprintf(stderr, "Cannot open network namespace: %s\n",
-			strerror(errno));
+		fprintf(stderr, "Cannot open network namespace %s: %s\n",
+			name, strerror(errno));
 		return EXIT_FAILURE;
 	}
 	if (setns(netns, CLONE_NEWNET) < 0) {
-- 
1.7.5.4

[PATCH iproute2-3.8 4/6] iproute2: Make "ip netns delete" more likely to succeed

[Eric W. Biederman]

Sometimes "ip netns delete" fails because it can not delete the file a
network namespace was mounted on.  If this only happened when a
network namespace was really in use this would be fine, but today it
is possible to pin all network namespaces by simply having a long
running process started with "ip netns exec".

Every mount is copied when a network namespace is created so it is
impossible to prevent the mounts from getting into other mount
namespaces.  Modify all mounts in the files and subdirectories of
/var/run/netns to be shared mount points so that unmount events can
propogate, making it unlikely that "ip netns delete" will fail because
a directory is mounted in another mount namespace.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 ip/ipnetns.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/ip/ipnetns.c b/ip/ipnetns.c
index 4fce379..33765b5 100644
--- a/ip/ipnetns.c
+++ b/ip/ipnetns.c
@@ -208,6 +208,7 @@ static int netns_add(int argc, char **argv)
 	char netns_path[MAXPATHLEN];
 	const char *name;
 	int fd;
+	int made_netns_run_dir_mount = 0;
 
 	if (argc < 1) {
 		fprintf(stderr, "No netns name specified\n");
@@ -220,6 +221,29 @@ static int netns_add(int argc, char **argv)
 	/* Create the base netns directory if it doesn't exist */
 	mkdir(NETNS_RUN_DIR, S_IRWXU|S_IRGRP|S_IXGRP|S_IROTH|S_IXOTH);
 
+	/* Make it possible for network namespace mounts to propogate between
+	 * mount namespaces.  This makes it likely that a unmounting a network
+	 * namespace file in one namespace will unmount the network namespace
+	 * file in all namespaces allowing the network namespace to be freed
+	 * sooner.
+	 */
+	while (mount("", NETNS_RUN_DIR, "none", MS_SHARED | MS_REC, NULL)) {
+		/* Fail unless we need to make the mount point */
+		if (errno != EINVAL || made_netns_run_dir_mount) {
+			fprintf(stderr, "mount --make-shared %s failed: %s\n",
+				NETNS_RUN_DIR, strerror(errno));
+			return EXIT_FAILURE;
+		}
+
+		/* Upgrade NETNS_RUN_DIR to a mount point */
+		if (mount(NETNS_RUN_DIR, NETNS_RUN_DIR, "none", MS_BIND, NULL)) {
+			fprintf(stderr, "mount --bind %s %s failed: %s\n",
+				NETNS_RUN_DIR, NETNS_RUN_DIR, strerror(errno));
+			return EXIT_FAILURE;
+		}
+		made_netns_run_dir_mount = 1;
+	}
+
 	/* Create the filesystem state */
 	fd = open(netns_path, O_RDONLY|O_CREAT|O_EXCL, 0);
 	if (fd < 0) {
-- 
1.7.5.4

[PATCH iproute2-3.8 5/6] iproute2: Fill in the ip-netns.8 manpage

[Eric W. Biederman]

Document ip netns monitor.

Add a few senteces describing each command.  The manpage was looking
very scrawny.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 man/man8/ip-netns.8 |   46 +++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 45 insertions(+), 1 deletions(-)

diff --git a/man/man8/ip-netns.8 b/man/man8/ip-netns.8
index 349ee7e..ff08232 100644
--- a/man/man8/ip-netns.8
+++ b/man/man8/ip-netns.8
@@ -1,4 +1,4 @@
-.TH IP\-NETNS 8 "20 Dec 2011" "iproute2" "Linux"
+.TH IP\-NETNS 8 "16 Jan 2013" "iproute2" "Linux"
 .SH NAME
 ip-netns \- process network namespace management
 .SH SYNOPSIS
@@ -23,6 +23,9 @@ ip-netns \- process network namespace management
 .BR "ip netns exec "
 .I NETNSNAME command ...
 
+.ti -8
+.BR "ip netns monitor"
+
 .SH DESCRIPTION
 A network namespace is logically another copy of the network stack,
 with its own routes, firewall rules, and network devices.
@@ -54,11 +57,52 @@ bind mounting all of the per network namespace configure files into
 their traditional location in /etc.
 
 .SS ip netns list - show all of the named network namespaces
+
+This command displays all of the network namespaces in /var/run/netns
+
 .SS ip netns add NAME - create a new named network namespace
+
+If NAME is available in /var/run/netns/ this command creates a new
+network namespace and assigns NAME.
+
 .SS ip netns delete NAME - delete the name of a network namespace
+
+If NAME is present in /var/run/netns it is umounted and the mount
+point is removed.  If this is the last user of the network namespace the
+network namespace will be freed, otherwise the network namespace
+persists until it has no more users.  ip netns delete may fail if
+the mount point is in use in another mount namespace.
+
 .SS ip netns exec NAME cmd ... - Run cmd in the named network namespace
 
+This command allows applications that are network namespace unaware
+to be run in something other than the default network namespace with
+all of the configuration for the specified network namespace appearing
+in the customary global locations.  A network namespace and bind mounts
+are used to move files from their network namespace specific location
+to their default locations without affecting other processes.
+
+.SS ip netns monitor - Report as network namespace names are added and deleted
+
+This command watches network namespace name addition and deletion events
+and prints a line for each event it sees.
+
 .SH EXAMPLES
+.PP
+ip netns list
+.RS
+Shows the list of current named network namespaces
+.RE
+.PP
+ip netns add vpn
+.RS
+Creates a network namespace and names it vpn
+.RE
+.PP
+ip netns exec vpn ip link set lo up
+.RS
+Bring up the loopback interface in the vpn network namespace.
+.RE
 
 .SH SEE ALSO
 .br
-- 
1.7.5.4

[PATCH iproute2-3.8 6/6] iproute2: Add "ip netns pids" and "ip netns identify"

[Eric W. Biederman]

Add command that go between network namespace names and process
identifiers.  The code builds and runs agains older kernels but
only works on Linux 3.8+ kernels where I have fixed stat to work
properly.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 ip/ipnetns.c        |  148 +++++++++++++++++++++++++++++++++++++++++++++++++++
 man/man8/ip-netns.8 |   18 ++++++
 2 files changed, 166 insertions(+), 0 deletions(-)

diff --git a/ip/ipnetns.c b/ip/ipnetns.c
index 33765b5..51b1c5e 100644
--- a/ip/ipnetns.c
+++ b/ip/ipnetns.c
@@ -13,6 +13,7 @@
 #include <dirent.h>
 #include <errno.h>
 #include <unistd.h>
+#include <ctype.h>
 
 #include "utils.h"
 #include "ip_common.h"
@@ -45,6 +46,8 @@ static int usage(void)
 	fprintf(stderr, "Usage: ip netns list\n");
 	fprintf(stderr, "       ip netns add NAME\n");
 	fprintf(stderr, "       ip netns delete NAME\n");
+	fprintf(stderr, "       ip netns identify PID\n");
+	fprintf(stderr, "       ip netns pids NAME\n");
 	fprintf(stderr, "       ip netns exec NAME cmd ...\n");
 	fprintf(stderr, "       ip netns monitor\n");
 	return EXIT_FAILURE;
@@ -174,6 +177,145 @@ static int netns_exec(int argc, char **argv)
 	return EXIT_FAILURE;
 }
 
+static int is_pid(const char *str)
+{
+	int ch;
+	for (; (ch = *str); str++) {
+		if (!isdigit(ch))
+			return 0;
+	}
+	return 1;
+}
+
+static int netns_pids(int argc, char **argv)
+{
+	const char *name;
+	char net_path[MAXPATHLEN];
+	int netns;
+	struct stat netst;
+	DIR *dir;
+	struct dirent *entry;
+
+	if (argc < 1) {
+		fprintf(stderr, "No netns name specified\n");
+		return EXIT_FAILURE;
+	}
+	if (argc > 1) {
+		fprintf(stderr, "extra arguments specified\n");
+		return EXIT_FAILURE;
+	}
+
+	name = argv[0];
+	snprintf(net_path, sizeof(net_path), "%s/%s", NETNS_RUN_DIR, name);
+	netns = open(net_path, O_RDONLY);
+	if (netns < 0) {
+		fprintf(stderr, "Cannot open network namespace: %s\n",
+			strerror(errno));
+		return EXIT_FAILURE;
+	}
+	if (fstat(netns, &netst) < 0) {
+		fprintf(stderr, "Stat of netns failed: %s\n",
+			strerror(errno));
+		return EXIT_FAILURE;
+	}
+	dir = opendir("/proc/");
+	if (!dir) {
+		fprintf(stderr, "Open of /proc failed: %s\n",
+			strerror(errno));
+		return EXIT_FAILURE;
+	}
+	while((entry = readdir(dir))) {
+		char pid_net_path[MAXPATHLEN];
+		struct stat st;
+		if (!is_pid(entry->d_name))
+			continue;
+		snprintf(pid_net_path, sizeof(pid_net_path), "/proc/%s/ns/net",
+			entry->d_name);
+		if (stat(pid_net_path, &st) != 0)
+			continue;
+		if ((st.st_dev == netst.st_dev) &&
+		    (st.st_ino == netst.st_ino)) {
+			printf("%s\n", entry->d_name);
+		}
+	}
+	closedir(dir);
+	return EXIT_SUCCESS;
+	
+}
+
+static int netns_identify(int argc, char **argv)
+{
+	const char *pidstr;
+	char net_path[MAXPATHLEN];
+	int netns;
+	struct stat netst;
+	DIR *dir;
+	struct dirent *entry;
+
+	if (argc < 1) {
+		fprintf(stderr, "No pid specified\n");
+		return EXIT_FAILURE;
+	}
+	if (argc > 1) {
+		fprintf(stderr, "extra arguments specified\n");
+		return EXIT_FAILURE;
+	}
+	pidstr = argv[0];
+
+	if (!is_pid(pidstr)) {
+		fprintf(stderr, "Specified string '%s' is not a pid\n",
+			pidstr);
+		return EXIT_FAILURE;
+	}
+
+	snprintf(net_path, sizeof(net_path), "/proc/%s/ns/net", pidstr);
+	netns = open(net_path, O_RDONLY);
+	if (netns < 0) {
+		fprintf(stderr, "Cannot open network namespace: %s\n",
+			strerror(errno));
+		return EXIT_FAILURE;
+	}
+	if (fstat(netns, &netst) < 0) {
+		fprintf(stderr, "Stat of netns failed: %s\n",
+			strerror(errno));
+		return EXIT_FAILURE;
+	}
+	dir = opendir(NETNS_RUN_DIR);
+	if (!dir) {
+		/* Succeed treat a missing directory as an empty directory */
+		if (errno == ENOENT)
+			return EXIT_SUCCESS;
+
+		fprintf(stderr, "Failed to open directory %s:%s\n",
+			NETNS_RUN_DIR, strerror(errno));
+		return EXIT_FAILURE;
+	}
+
+	while((entry = readdir(dir))) {
+		char name_path[MAXPATHLEN];
+		struct stat st;
+
+		if (strcmp(entry->d_name, ".") == 0)
+			continue;
+		if (strcmp(entry->d_name, "..") == 0)
+			continue;
+
+		snprintf(name_path, sizeof(name_path), "%s/%s",	NETNS_RUN_DIR,
+			entry->d_name);
+
+		if (stat(name_path, &st) != 0)
+			continue;
+
+		if ((st.st_dev == netst.st_dev) &&
+		    (st.st_ino == netst.st_ino)) {
+			printf("%s\n", entry->d_name);
+		}
+	}
+	closedir(dir);
+	return EXIT_SUCCESS;
+	
+}
+
 static int netns_delete(int argc, char **argv)
 {
 	const char *name;
@@ -324,6 +466,12 @@ int do_netns(int argc, char **argv)
 	if (matches(*argv, "delete") == 0)
 		return netns_delete(argc-1, argv+1);
 
+	if (matches(*argv, "identify") == 0)
+		return netns_identify(argc-1, argv+1);
+
+	if (matches(*argv, "pids") == 0)
+		return netns_pids(argc-1, argv+1);
+
 	if (matches(*argv, "exec") == 0)
 		return netns_exec(argc-1, argv+1);
 
diff --git a/man/man8/ip-netns.8 b/man/man8/ip-netns.8
index ff08232..f5b025e 100644
--- a/man/man8/ip-netns.8
+++ b/man/man8/ip-netns.8
@@ -20,6 +20,14 @@ ip-netns \- process network namespace management
 .I NETNSNAME
 
 .ti -8
+.BR "ip netns identify"
+.I PID
+
+.ti -8
+.BR "ip netns pids" 
+.I NETNSNAME
+
+.ti -8
 .BR "ip netns exec "
 .I NETNSNAME command ...
 
@@ -73,6 +81,16 @@ network namespace will be freed, otherwise the network namespace
 persists until it has no more users.  ip netns delete may fail if
 the mount point is in use in another mount namespace.
 
+.SS ip netns identify PID - Report network namespaces names for process
+
+This command walks through /var/run/netns and finds all the network
+namespace names for network namespace of the specified process.
+
+.SS ip netns pids NAME - Report processes in the named network namespace
+
+This command walks through proc and finds all of the process who have
+the named network namespace as their primary network namespace.
+
 .SS ip netns exec NAME cmd ... - Run cmd in the named network namespace
 
 This command allows applications that are network namespace unaware
-- 
1.7.5.4

Re: [PATCH for 3.8] iproute2: Add "ip netns pids" and "ip netns identify"

[Ben Hutchings]

On Thu, 2013-01-17 at 16:23 -0800, Eric W. Biederman wrote:
> Ben Hutchings <bhutchings@solarflare.com> writes:
> 
> > On Mon, 2012-11-26 at 17:16 -0600, Eric W. Biederman wrote:
[...]
> >> --- a/ip/ipnetns.c
> >> +++ b/ip/ipnetns.c
[...]
> >> +static int is_pid(const char *str)
> >> +{
> >> +	int ch;
> >> +	for (; (ch = *str); str++) {
> >> +		if (!isdigit(ch))
> >
> > ch must be cast to unsigned char before passing to isdigit().
> 
> isdigit is defined to take an int.  A legacy of the implicit casts in
> the K&R C days.  Casting to unsigned char would be pointless and silly.
[...]

It's not pointless.  This is explained in the very first line of the
description in the manual page...

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH for 3.8] iproute2: Add "ip netns pids" and "ip netns identify"

[Eric W. Biederman]

Ben Hutchings <bhutchings@solarflare.com> writes:

> On Thu, 2013-01-17 at 16:23 -0800, Eric W. Biederman wrote:
>> Ben Hutchings <bhutchings@solarflare.com> writes:
>> 
>> > On Mon, 2012-11-26 at 17:16 -0600, Eric W. Biederman wrote:
> [...]
>> >> --- a/ip/ipnetns.c
>> >> +++ b/ip/ipnetns.c
> [...]
>> >> +static int is_pid(const char *str)
>> >> +{
>> >> +	int ch;
>> >> +	for (; (ch = *str); str++) {
>> >> +		if (!isdigit(ch))
>> >
>> > ch must be cast to unsigned char before passing to isdigit().
>> 
>> isdigit is defined to take an int.  A legacy of the implicit casts in
>> the K&R C days.  Casting to unsigned char would be pointless and silly.
> [...]
>
> It's not pointless.  This is explained in the very first line of the
> description in the manual page...

If it's not pointless it is an implementation bug.  The conversion to of
char to int happens implicitly whenever you pass a char.  It is
absolutely broken to have a function that takes a char converted to int
and reject the automatic conversion of char to int.

I suspect much more strongly that it is a case of poor documentation.

If isdigit can't deal with what I have passed it I will be much more
interested in writing a patch for isdigit.

That said I just dobule checked with the code below.  Negative character
values work correctly and don't cause any runtime errors.

It looks like it is time to update the manpage to remove that
confusing/wrong sentence.

Eric

#include <ctype.h>
#include <limits.h>
#include <stdio.h>

int main(int argc, char **argv)
{
	char c;
	for (c = CHAR_MIN; c < CHAR_MAX; c++) {
		printf("c: %d isdigit: %d\n",
		       c, isdigit(c));
	}
	return 0;
}

RE: [PATCH for 3.8] iproute2: Add "ip netns pids" and "ip netns identify"

[David Laight]

> >> >> +		if (!isdigit(ch))
> >> >
> >> > ch must be cast to unsigned char before passing to isdigit().
> >>
> >> isdigit is defined to take an int.  A legacy of the implicit casts in
> >> the K&R C days.  Casting to unsigned char would be pointless and silly.
> > [...]
> >
> > It's not pointless.  This is explained in the very first line of the
> > description in the manual page...
> 
> If it's not pointless it is an implementation bug.  The conversion to of
> char to int happens implicitly whenever you pass a char.  It is
> absolutely broken to have a function that takes a char converted to int
> and reject the automatic conversion of char to int.
> 
> I suspect much more strongly that it is a case of poor documentation.

All of the isxxxx() functions have an input domain of EOF and all the
values of 'char' cast to unsigned (I've forgotten the exact words).
Passing in a value that is outside the domain has an undefined effect
and is very likely to generate a core dump, even if it doesn't
dump, the returned value is likely to be wrong.

This input value matches the values returned by the stdio getc()
functions and getopt().

> If isdigit can't deal with what I have passed it I will be much more
> interested in writing a patch for isdigit.

The traditional/expected implementation of the isxxx() functions is
a macro expansion that indexes an array and checks for some bits.
gcc even has a warning about indexing arrays with 'char' that,
I suspect, is there to detect incorrect uses of the isxxx() functions.

> That said I just dobule checked with the code below.  Negative character
> values work correctly and don't cause any runtime errors.

The fact that one specific piece of code appears to work doesn't mean
that all such code will work - it won't.

In any case the functions have to differentiate between EOF and
the 256 valid values of 'char'. EOF (more or less) has to be -1
so the char bit pattern 0xff must not become -1 (it passes
isprint() in at least some locales).

I've had to do a trawl through a large amount of user code fixing
the buggy calls to the isxxxx() functions. Interestingly I didn't
actually find any code that could pass EOF as an argument!

	David

Re: [PATCH for 3.8] iproute2: Add "ip netns pids" and "ip netns identify"

[Ben Hutchings]

On Thu, 2013-01-17 at 17:27 -0800, Eric W. Biederman wrote:
> Ben Hutchings <bhutchings@solarflare.com> writes:
> 
> > On Thu, 2013-01-17 at 16:23 -0800, Eric W. Biederman wrote:
> >> Ben Hutchings <bhutchings@solarflare.com> writes:
> >> 
> >> > On Mon, 2012-11-26 at 17:16 -0600, Eric W. Biederman wrote:
> > [...]
> >> >> --- a/ip/ipnetns.c
> >> >> +++ b/ip/ipnetns.c
> > [...]
> >> >> +static int is_pid(const char *str)
> >> >> +{
> >> >> +	int ch;
> >> >> +	for (; (ch = *str); str++) {
> >> >> +		if (!isdigit(ch))
> >> >
> >> > ch must be cast to unsigned char before passing to isdigit().
> >> 
> >> isdigit is defined to take an int.  A legacy of the implicit casts in
> >> the K&R C days.  Casting to unsigned char would be pointless and silly.
> > [...]
> >
> > It's not pointless.  This is explained in the very first line of the
> > description in the manual page...
> 
> If it's not pointless it is an implementation bug.

You can either get in your time machine and go back to 1978 and fix it,
or add the cast like every C programmer who knows what the C standards
say about these functions.

> The conversion to of
> char to int happens implicitly whenever you pass a char.  It is
> absolutely broken to have a function that takes a char converted to int
> and reject the automatic conversion of char to int.

It doesn't take a char... weird but that is how it is defined.

> I suspect much more strongly that it is a case of poor documentation.
>
> If isdigit can't deal with what I have passed it I will be much more
> interested in writing a patch for isdigit.
> 
> That said I just dobule checked with the code below.  Negative character
> values work correctly and don't cause any runtime errors.
[...]

Testing on one implementation doesn't prove anything.  'char' can be
signed or unsigned depending on the architecture, and some C libraries
work around buggy applications that .  That's no reason to write another
buggy application.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH for 3.8] iproute2: Add "ip netns pids" and "ip netns identify"

[Eric W. Biederman]

Ben Hutchings <bhutchings@solarflare.com> writes:

> On Thu, 2013-01-17 at 17:27 -0800, Eric W. Biederman wrote:
>> Ben Hutchings <bhutchings@solarflare.com> writes:
>> 
>> > On Thu, 2013-01-17 at 16:23 -0800, Eric W. Biederman wrote:
>> >> Ben Hutchings <bhutchings@solarflare.com> writes:
>> >> 
>> >> > On Mon, 2012-11-26 at 17:16 -0600, Eric W. Biederman wrote:
>> > [...]
>> >> >> --- a/ip/ipnetns.c
>> >> >> +++ b/ip/ipnetns.c
>> > [...]
>> >> >> +static int is_pid(const char *str)
>> >> >> +{
>> >> >> +	int ch;
>> >> >> +	for (; (ch = *str); str++) {
>> >> >> +		if (!isdigit(ch))
>> >> >
>> >> > ch must be cast to unsigned char before passing to isdigit().
>> >> 
>> >> isdigit is defined to take an int.  A legacy of the implicit casts in
>> >> the K&R C days.  Casting to unsigned char would be pointless and silly.
>> > [...]
>> >
>> > It's not pointless.  This is explained in the very first line of the
>> > description in the manual page...
>> 
>> If it's not pointless it is an implementation bug.
>
> You can either get in your time machine and go back to 1978 and fix it,
> or add the cast like every C programmer who knows what the C standards
> say about these functions.

So I took a moment to look. The C standard is indeed does not say
anything about this and supporting signed char becomes a quality of
implementation issue.  glibc supports being passed signed character
values.

> Testing on one implementation doesn't prove anything.  'char' can be
> signed or unsigned depending on the architecture, and some C libraries
> work around buggy applications that .  That's no reason to write another
> buggy application.

This code by it's very nature is not portable.  The code is not suid
so insane level of paranoia don't need to be maintained.  The definition
in the C standard is a least common denominator requirement.  Posix
copies that least common denominator requirement.  Glibc does not
implment the least common denominator.

There is no advantage for an implemenation to implement only the least
common denominator of functionality in isdigit.  There is a huge
advantage for an implementation of the cypte functions on platforms with
signed char to have an array with 384 entries.  It is nearly humanly
impossible to remember you need to type isdigit((unsigned)string[n]),
not to mention how easy it is for casts to go wrong.

So no I do not consider programs that are not strictly conformant with
the C standard broken.  I consider implementations of isdigit that are
strictly conformat with the C standard to be canidadates for patches.
At this point I will happily add support to any ctype implemenation I
meet that has such a poor quality of implementation that you have to be
a language lawyer in top form to use isdigit properly.

Eric

RE: [PATCH for 3.8] iproute2: Add "ip netns pids" and "ip netns identify"

[David Laight]

> >> >> >> +		if (!isdigit(ch))
> >> >> >
> >> >> > ch must be cast to unsigned char before passing to isdigit().
> >> >>
> >> >> isdigit is defined to take an int.  A legacy of the implicit casts in
> >> >> the K&R C days.  Casting to unsigned char would be pointless and silly.
> >> > [...]
> >> >
> >> > It's not pointless.  This is explained in the very first line of the
> >> > description in the manual page...
> >>
> >> If it's not pointless it is an implementation bug.
> >
> > You can either get in your time machine and go back to 1978 and fix it,
> > or add the cast like every C programmer who knows what the C standards
> > say about these functions.
> 
> So I took a moment to look. The C standard is indeed does not say
> anything about this and supporting signed char becomes a quality of
> implementation issue.  glibc supports being passed signed character
> values.

You must have looked in the wrong place.
The standards documentation on all the ctype functions is very clear
about the valid input domain.

> > Testing on one implementation doesn't prove anything.  'char' can be
> > signed or unsigned depending on the architecture, and some C libraries
> > work around buggy applications that .  That's no reason to write another
> > buggy application.
> 
> This code by it's very nature is not portable.  The code is not suid
> so insane level of paranoia don't need to be maintained.

What!!!! You still don't want core dumps due to invalid input.
Especially if the input might be from a file.
I did the full trawl of NetBSD's 'src' tree because of issues with
one of the shells faulting.

> The definition
> in the C standard is a least common denominator requirement.  Posix
> copies that least common denominator requirement.  Glibc does not
> implment the least common denominator.

Unless you writes 'standards compliant' programs, you will eventually
fall foul of these sort of problems.

Are you sure that glibc will always be used? What about newlib?

There may be strict requirements on isdigit() (I think you are
allowed to subtract '0' to get a number). The same is not true
of isprint(), isprint(EOF) is FALSE, isprint(255) may be TRUE
(ij ligature).

	David

Re: [PATCH iproute-3.8 0/6] ip netns bug fixes and enhancements

[Vijay Subramanian]

>
> Eric W. Biederman (6):
>       iproute2: Don't propogate mounts out of ip netns exec.
>       iproute2: Normalize return codes in "ip netns"
>       iproute2: Improve "ip netns add" failure error message
>       iproute2: Make "ip netns delete" more likely to succeed
>       iproute2: Fill in the ip-netns.8 manpage
>       iproute2: Add "ip netns pids" and "ip netns identify"
>


Eric,
With these patches applied, I am getting the following errors:


make[1]: Entering directory `/root/tools/iproute2/ip'
gcc -Wall -Wstrict-prototypes -Werror -O2 -I../include
-DRESOLVE_HOSTNAMES -DLIBDIR=\"/usr/lib\" -DCONFDIR=\"/etc/iproute2\"
-D_GNU_SOURCE   -c -o ipaddress.o ipaddress.c
gcc -Wall -Wstrict-prototypes -Werror -O2 -I../include
-DRESOLVE_HOSTNAMES -DLIBDIR=\"/usr/lib\" -DCONFDIR=\"/etc/iproute2\"
-D_GNU_SOURCE   -c -o ipnetns.o ipnetns.c
ipnetns.c: In function ‘netns_exec’:
ipnetns.c:156: error: ‘MS_SLAVE’ undeclared (first use in this function)
ipnetns.c:156: error: (Each undeclared identifier is reported only once
ipnetns.c:156: error: for each function it appears in.)
ipnetns.c:156: error: ‘MS_REC’ undeclared (first use in this function)
ipnetns.c: In function ‘netns_add’:
ipnetns.c:372: error: ‘MS_SHARED’ undeclared (first use in this function)
ipnetns.c:372: error: ‘MS_REC’ undeclared (first use in this function)
make[1]: *** [ipnetns.o] Error 1
make[1]: Leaving directory `/root/tools/iproute2/ip'
make: *** [all] Error 2


Thanks,
Vijay

Re: [PATCH iproute-3.8 0/6] ip netns bug fixes and enhancements

[Eric W. Biederman]

Vijay Subramanian <subramanian.vijay@gmail.com> writes:

>>
>> Eric W. Biederman (6):
>>       iproute2: Don't propogate mounts out of ip netns exec.
>>       iproute2: Normalize return codes in "ip netns"
>>       iproute2: Improve "ip netns add" failure error message
>>       iproute2: Make "ip netns delete" more likely to succeed
>>       iproute2: Fill in the ip-netns.8 manpage
>>       iproute2: Add "ip netns pids" and "ip netns identify"
>>
>
>
> Eric,
> With these patches applied, I am getting the following errors:
>
>
> make[1]: Entering directory `/root/tools/iproute2/ip'
> gcc -Wall -Wstrict-prototypes -Werror -O2 -I../include
> -DRESOLVE_HOSTNAMES -DLIBDIR=\"/usr/lib\" -DCONFDIR=\"/etc/iproute2\"
> -D_GNU_SOURCE   -c -o ipaddress.o ipaddress.c
> gcc -Wall -Wstrict-prototypes -Werror -O2 -I../include
> -DRESOLVE_HOSTNAMES -DLIBDIR=\"/usr/lib\" -DCONFDIR=\"/etc/iproute2\"
> -D_GNU_SOURCE   -c -o ipnetns.o ipnetns.c
> ipnetns.c: In function ‘netns_exec’:
> ipnetns.c:156: error: ‘MS_SLAVE’ undeclared (first use in this function)
> ipnetns.c:156: error: (Each undeclared identifier is reported only once
> ipnetns.c:156: error: for each function it appears in.)
> ipnetns.c:156: error: ‘MS_REC’ undeclared (first use in this function)
> ipnetns.c: In function ‘netns_add’:
> ipnetns.c:372: error: ‘MS_SHARED’ undeclared (first use in this function)
> ipnetns.c:372: error: ‘MS_REC’ undeclared (first use in this function)
> make[1]: *** [ipnetns.o] Error 1
> make[1]: Leaving directory `/root/tools/iproute2/ip'
> make: *** [all] Error 2

On my system those defines are coming out of sys/mount.h and date back
to 2.6.5 or so.  You should have them available on your system.

What weird system are you on that doesn't export those?

I don't have a clue on where to start at a practical level.  It wouldn't
be hard to provide some #ifndef compat glue but I can't imagine why that
would be needed.

Eric

Re: [PATCH iproute-3.8 0/6] ip netns bug fixes and enhancements

[Vijay Subramanian]

>
> On my system those defines are coming out of sys/mount.h and date back
> to 2.6.5 or so.  You should have them available on your system.

>
> What weird system are you on that doesn't export those?

The broken machine is not weird at all, just running a standard
version of Ubuntu (a little older though).
Ubuntu 10.04.1 LTS (lucid).
On this machine, I have the following version of libc6. Version:
2.11.1-0ubuntu7.10

The defines are not present in sys/mount.h but are in linux/fs.h. When
I include linux/fs.h, ipnetns.c compiles fine (but causes other
problems).
As Stephen said, it is probably the version of glibc that is causing this.

I verified this by trying to compile iproute2 on a newer version of
Ubuntu (Ubuntu 12.04.1 LTS, precise) which has glibc version
2.15-0ubuntu10.3. Everything compiles fine here.  The defines are
present in both linux/fs.h and x86_64-linux-gnu/sys/mount.h

I found the libc6 version by running 'aptitude show libc6'.  Let me
know if you need any other info.

>
> I don't have a clue on where to start at a practical level.  It wouldn't
> be hard to provide some #ifndef compat glue but I can't imagine why that
> would be needed.
>
> Eric

I didn't realize it was a libc versioning error. Sorry if this was noise.

Thanks,
Vijay