diff mbox series

[iproute2] ip: reset netns after each command in batch mode

Message ID 20190607101313.8561-1-mcroce@redhat.com
State Superseded
Delegated to: stephen hemminger
Headers show
Series [iproute2] ip: reset netns after each command in batch mode | expand

Commit Message

Matteo Croce June 7, 2019, 10:13 a.m. UTC
When creating a new netns or executing a program into an existing one,
the unshare() or setns() calls will change the current netns.
In batch mode, this can run commands on the wrong interfaces, as the
ifindex value is meaningful only in the current netns. For example, this
command fails because veth-c doesn't exists in the init netns:

    # ip -b - <<-'EOF'
        netns add client
        link add name veth-c type veth peer veth-s netns client
        addr add 192.168.2.1/24 dev veth-c
    EOF
    Cannot find device "veth-c"
    Command failed -:7

But if there are two devices with the same name in the init and new netns,
ip will build a wrong ll_map with indexes belonging to the new netns,
and will execute actions in the init netns using this wrong mapping.
This script will flush all eth0 addresses and bring it down, as it has
the same ifindex of veth0 in the new netns:

    # ip addr
    1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN group default qlen 1000
        link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
        inet 127.0.0.1/8 scope host lo
           valid_lft forever preferred_lft forever
    2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP group default qlen 1000
        link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
        inet 192.168.122.76/24 brd 192.168.122.255 scope global dynamic eth0
           valid_lft 3598sec preferred_lft 3598sec

    # ip -b - <<-'EOF'
        netns add client
        link add name veth0 type veth peer name veth1
        link add name veth-ns type veth peer name veth0 netns client
        link set veth0 down
        address flush veth0
    EOF

    # ip addr
    1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN group default qlen 1000
        link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
        inet 127.0.0.1/8 scope host lo
           valid_lft forever preferred_lft forever
    2: eth0: <BROADCAST,MULTICAST> mtu 1500 qdisc mq state DOWN group default qlen 1000
        link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
    3: veth1@veth0: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN group default qlen 1000
        link/ether c2:db:d0:34:13:4a brd ff:ff:ff:ff:ff:ff
    4: veth0@veth1: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN group default qlen 1000
        link/ether ca:9d:6b:5f:5f:8f brd ff:ff:ff:ff:ff:ff
    5: veth-ns@if2: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN group default qlen 1000
        link/ether 32:ef:22:df:51:0a brd ff:ff:ff:ff:ff:ff link-netns client

The same issue can be triggered by the netns exec subcommand with a
sligthy different script:

    # ip netns add client
    # ip -b - <<-'EOF'
        netns exec client true
        link add name veth0 type veth peer name veth1
        link add name veth-ns type veth peer name veth0 netns client
        link set veth0 down
        address flush veth0
    EOF

Fix this by adding two netns_{save,reset} functions, which are used
to get a file descriptor for the init netns, and restore it after
each batch command.
netns_save() is called before the unshare() or setns(),
while netns_restore() is called after each command.

Fixes: 0dc34c7713bb ("iproute2: Add processless network namespace support")
Reviewed-and-tested-by: Andrea Claudi <aclaudi@redhat.com>
Signed-off-by: Matteo Croce <mcroce@redhat.com>
---
 include/namespace.h |  2 ++
 ip/ip.c             |  1 +
 ip/ipnetns.c        |  1 +
 lib/namespace.c     | 26 ++++++++++++++++++++++++++
 4 files changed, 30 insertions(+)

Comments

Stephen Hemminger June 7, 2019, 3:25 p.m. UTC | #1
On Fri,  7 Jun 2019 12:13:13 +0200
Matteo Croce <mcroce@redhat.com> wrote:

> +void netns_restore(void)
> +{
> +	if (saved_netns != -1) {

If saved_netns is -1 then it is a program bug becase
no save was done? then do something?


> +		if (!setns(saved_netns, CLONE_NEWNET)) {
> +			close(saved_netns);
> +			saved_netns = -1;
> +		} else {
> +			perror("setns");

If you are going to look for errors. then you need to either
return the error or cause the program to exit.
You don't want later commands in the batch to be applied
to wrong namespace.
Matteo Croce June 7, 2019, 4:38 p.m. UTC | #2
On Fri, Jun 7, 2019 at 5:25 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Fri,  7 Jun 2019 12:13:13 +0200
> Matteo Croce <mcroce@redhat.com> wrote:
>
> > +void netns_restore(void)
> > +{
> > +     if (saved_netns != -1) {
>
> If saved_netns is -1 then it is a program bug becase
> no save was done? then do something?
>

saved_netns can be -1 if you execute a batch which doesn't never
change the current netns, e.g: only link, addr, route etc.
In this case netns_restore() will do nothing as there is nothing to restore

> > +             if (!setns(saved_netns, CLONE_NEWNET)) {
> > +                     close(saved_netns);
> > +                     saved_netns = -1;
> > +             } else {
> > +                     perror("setns");
>
> If you are going to look for errors. then you need to either
> return the error or cause the program to exit.
> You don't want later commands in the batch to be applied
> to wrong namespace.

Right. A failure in restoring a saved netns means that something bad
happened, so better stop.

Regards,

--
Matteo Croce
per aspera ad upstream
diff mbox series

Patch

diff --git a/include/namespace.h b/include/namespace.h
index e47f9b5d..89cdda11 100644
--- a/include/namespace.h
+++ b/include/namespace.h
@@ -49,6 +49,8 @@  static inline int setns(int fd, int nstype)
 }
 #endif /* HAVE_SETNS */
 
+void netns_save(void);
+void netns_restore(void);
 int netns_switch(char *netns);
 int netns_get_fd(const char *netns);
 int netns_foreach(int (*func)(char *nsname, void *arg), void *arg);
diff --git a/ip/ip.c b/ip/ip.c
index e4131714..d64d43e1 100644
--- a/ip/ip.c
+++ b/ip/ip.c
@@ -158,6 +158,7 @@  static int batch(const char *name)
 			if (!force)
 				break;
 		}
+		netns_restore();
 	}
 	if (line)
 		free(line);
diff --git a/ip/ipnetns.c b/ip/ipnetns.c
index 52aefacf..e4788e75 100644
--- a/ip/ipnetns.c
+++ b/ip/ipnetns.c
@@ -707,6 +707,7 @@  static int netns_add(int argc, char **argv, bool create)
 	close(fd);
 
 	if (create) {
+		netns_save();
 		if (unshare(CLONE_NEWNET) < 0) {
 			fprintf(stderr, "Failed to create a new network namespace \"%s\": %s\n",
 				name, strerror(errno));
diff --git a/lib/namespace.c b/lib/namespace.c
index 06ae0a48..11ba0f86 100644
--- a/lib/namespace.c
+++ b/lib/namespace.c
@@ -15,6 +15,30 @@ 
 #include "utils.h"
 #include "namespace.h"
 
+static int saved_netns = -1;
+
+/* Obtain a FD for the current namespace, so we can reenter it later */
+void netns_save(void)
+{
+	if (saved_netns == -1) {
+		saved_netns = open("/proc/self/ns/net", O_RDONLY | O_CLOEXEC);
+		if (saved_netns == -1)
+			perror("Cannot open init namespace");
+	}
+}
+
+void netns_restore(void)
+{
+	if (saved_netns != -1) {
+		if (!setns(saved_netns, CLONE_NEWNET)) {
+			close(saved_netns);
+			saved_netns = -1;
+		} else {
+			perror("setns");
+		}
+	}
+}
+
 static void bind_etc(const char *name)
 {
 	char etc_netns_path[sizeof(NETNS_ETC_DIR) + NAME_MAX];
@@ -61,6 +85,8 @@  int netns_switch(char *name)
 		return -1;
 	}
 
+	netns_save();
+
 	if (setns(netns, CLONE_NEWNET) < 0) {
 		fprintf(stderr, "setting the network namespace \"%s\" failed: %s\n",
 			name, strerror(errno));