[iproute PATCH 0/3] improve MACVLAN/MACVTAP support

[iproute PATCH 0/3] improve MACVLAN/MACVTAP support

[Phil Sutter]

While implementing support for MACVLAN_FLAG_NOPROMISC, I realized how similar
iplink_macvlan.c and iplink_macvtap.c are and that the main differences (apart
from substituting "macvlan" for "macvtap") were fixes and enhancements which
weren't applied to both files. To prevent this in the future and to share
common code, this series merges the files. In addition, it implements support
for MACVLAN_FLAG_NOPROMISC and finally documents these interface types in
ip-link.8.in.

Phil Sutter (3):
  ip: link: consolidate macvlan and macvtap
  ip: macvlan: support MACVLAN_FLAG_NOPROMISC flag
  man: ip-link: document MACVLAN/MACVTAP interface types

 ip/Makefile           |   2 +-
 ip/iplink_macvlan.c   |  68 ++++++++++++++++++++++++--------
 ip/iplink_macvtap.c   | 105 --------------------------------------------------
 man/man8/ip-link.8.in |  50 ++++++++++++++++++++++++
 4 files changed, 104 insertions(+), 121 deletions(-)
 delete mode 100644 ip/iplink_macvtap.c

-- 
2.1.2

[iproute PATCH 1/3] ip: link: consolidate macvlan and macvtap

[Phil Sutter]

After eliminating the minor differences in both files which existed
solely because features/fixes were applied to only one of them and not
the other, the remaining differences were in function naming and error
messages. The latter is addressed by using the 'id' field of struct
link_util.

Fold both files into one in order to share common code and eliminate the
chance of having fixes/enhancements applied to only one of them.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/Makefile         |   2 +-
 ip/iplink_macvlan.c |  40 +++++++++++++-------
 ip/iplink_macvtap.c | 105 ----------------------------------------------------
 3 files changed, 28 insertions(+), 119 deletions(-)
 delete mode 100644 ip/iplink_macvtap.c

diff --git a/ip/Makefile b/ip/Makefile
index d8b38ac..52b76ef 100644
--- a/ip/Makefile
+++ b/ip/Makefile
@@ -3,7 +3,7 @@ IPOBJ=ip.o ipaddress.o ipaddrlabel.o iproute.o iprule.o ipnetns.o \
     ipmaddr.o ipmonitor.o ipmroute.o ipprefix.o iptuntap.o iptoken.o \
     ipxfrm.o xfrm_state.o xfrm_policy.o xfrm_monitor.o \
     iplink_vlan.o link_veth.o link_gre.o iplink_can.o \
-    iplink_macvlan.o iplink_macvtap.o ipl2tp.o link_vti.o link_vti6.o \
+    iplink_macvlan.o ipl2tp.o link_vti.o link_vti6.o \
     iplink_vxlan.o tcp_metrics.o iplink_ipoib.o ipnetconf.o link_ip6tnl.o \
     link_iptnl.o link_gre6.o iplink_bond.o iplink_bond_slave.o iplink_hsr.o \
     iplink_bridge.o iplink_bridge_slave.o ipfou.o iplink_ipvlan.o \
diff --git a/ip/iplink_macvlan.c b/ip/iplink_macvlan.c
index 826b659..d759f0e 100644
--- a/ip/iplink_macvlan.c
+++ b/ip/iplink_macvlan.c
@@ -1,5 +1,5 @@
 /*
- * iplink_vlan.c	VLAN device support
+ * iplink_macvlan.c	macvlan/macvtap device support
  *
  *              This program is free software; you can redistribute it and/or
  *              modify it under the terms of the GNU General Public License
@@ -20,22 +20,29 @@
 #include "utils.h"
 #include "ip_common.h"
 
-static void print_explain(FILE *f)
+#define pfx_err(lu, ...) {               \
+	fprintf(stderr, "%s: ", lu->id); \
+	fprintf(stderr, __VA_ARGS__);    \
+	fprintf(stderr, "\n");           \
+}
+
+static void print_explain(struct link_util *lu, FILE *f)
 {
 	fprintf(f,
-		"Usage: ... macvlan mode { private | vepa | bridge | passthru }\n"
+		"Usage: ... %s mode { private | vepa | bridge | passthru }\n",
+		lu->id
 	);
 }
 
-static void explain(void)
+static void explain(struct link_util *lu)
 {
-	print_explain(stderr);
+	print_explain(lu, stderr);
 }
 
-static int mode_arg(void)
+static int mode_arg(const char *arg)
 {
         fprintf(stderr, "Error: argument of \"mode\" must be \"private\", "
-		"\"vepa\", \"bridge\" or \"passthru\" \n");
+		"\"vepa\", \"bridge\" or \"passthru\", not \"%s\"\n", arg);
         return -1;
 }
 
@@ -56,15 +63,14 @@ static int macvlan_parse_opt(struct link_util *lu, int argc, char **argv,
 			else if (strcmp(*argv, "passthru") == 0)
 				mode = MACVLAN_MODE_PASSTHRU;
 			else
-				return mode_arg();
-
+				return mode_arg(*argv);
 			addattr32(n, 1024, IFLA_MACVLAN_MODE, mode);
 		} else if (matches(*argv, "help") == 0) {
-			explain();
+			explain(lu);
 			return -1;
 		} else {
-			fprintf(stderr, "macvlan: unknown option \"%s\"?\n", *argv);
-			explain();
+			pfx_err(lu, "unknown option \"%s\"?", *argv);
+			explain(lu);
 			return -1;
 		}
 		argc--, argv++;
@@ -96,7 +102,7 @@ static void macvlan_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[]
 static void macvlan_print_help(struct link_util *lu, int argc, char **argv,
 	FILE *f)
 {
-	print_explain(f);
+	print_explain(lu, f);
 }
 
 struct link_util macvlan_link_util = {
@@ -106,3 +112,11 @@ struct link_util macvlan_link_util = {
 	.print_opt	= macvlan_print_opt,
 	.print_help	= macvlan_print_help,
 };
+
+struct link_util macvtap_link_util = {
+	.id		= "macvtap",
+	.maxattr	= IFLA_MACVLAN_MAX,
+	.parse_opt	= macvlan_parse_opt,
+	.print_opt	= macvlan_print_opt,
+	.print_help	= macvlan_print_help,
+};
diff --git a/ip/iplink_macvtap.c b/ip/iplink_macvtap.c
deleted file mode 100644
index 9c2cd74..0000000
--- a/ip/iplink_macvtap.c
+++ /dev/null
@@ -1,105 +0,0 @@
-/*
- * iplink_macvtap.c	macvtap device support
- *
- *              This program is free software; you can redistribute it and/or
- *              modify it under the terms of the GNU General Public License
- *              as published by the Free Software Foundation; either version
- *              2 of the License, or (at your option) any later version.
- */
-
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
-#include <sys/socket.h>
-#include <linux/if_link.h>
-
-#include "rt_names.h"
-#include "utils.h"
-#include "ip_common.h"
-
-static void print_explain(FILE *f)
-{
-	fprintf(stderr,
-		"Usage: ... macvtap mode { private | vepa | bridge | passthru }\n"
-	);
-}
-
-static void explain(void)
-{
-	print_explain(stderr);
-}
-
-static int mode_arg(const char *arg)
-{
-        fprintf(stderr, "Error: argument of \"mode\" must be \"private\", "
-		"\"vepa\", \"bridge\" or \"passthru\", not \"%s\"\n", arg);
-        return -1;
-}
-
-static int macvtap_parse_opt(struct link_util *lu, int argc, char **argv,
-			  struct nlmsghdr *n)
-{
-	while (argc > 0) {
-		if (matches(*argv, "mode") == 0) {
-			__u32 mode = 0;
-			NEXT_ARG();
-
-			if (strcmp(*argv, "private") == 0)
-				mode = MACVLAN_MODE_PRIVATE;
-			else if (strcmp(*argv, "vepa") == 0)
-				mode = MACVLAN_MODE_VEPA;
-			else if (strcmp(*argv, "bridge") == 0)
-				mode = MACVLAN_MODE_BRIDGE;
-			else if (strcmp(*argv, "passthru") == 0)
-				mode = MACVLAN_MODE_PASSTHRU;
-			else
-				return mode_arg(*argv);
-
-			addattr32(n, 1024, IFLA_MACVLAN_MODE, mode);
-		} else if (matches(*argv, "help") == 0) {
-			explain();
-			return -1;
-		} else {
-			fprintf(stderr, "macvtap: unknown command \"%s\"?\n", *argv);
-			explain();
-			return -1;
-		}
-		argc--, argv++;
-	}
-
-	return 0;
-}
-
-static void macvtap_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
-{
-	__u32 mode;
-
-	if (!tb)
-		return;
-
-	if (!tb[IFLA_MACVLAN_MODE] ||
-	    RTA_PAYLOAD(tb[IFLA_MACVLAN_MODE]) < sizeof(__u32))
-		return;
-
-	mode = rta_getattr_u32(tb[IFLA_VLAN_ID]);
-	fprintf(f, " mode %s ",
-		  mode == MACVLAN_MODE_PRIVATE ? "private"
-		: mode == MACVLAN_MODE_VEPA    ? "vepa"
-		: mode == MACVLAN_MODE_BRIDGE  ? "bridge"
-		: mode == MACVLAN_MODE_PASSTHRU  ? "passthru"
-		:				 "unknown");
-}
-
-static void macvtap_print_help(struct link_util *lu, int argc, char **argv,
-	FILE *f)
-{
-	print_explain(f);
-}
-
-struct link_util macvtap_link_util = {
-	.id		= "macvtap",
-	.maxattr	= IFLA_MACVLAN_MAX,
-	.parse_opt	= macvtap_parse_opt,
-	.print_opt	= macvtap_print_opt,
-	.print_help	= macvtap_print_help,
-};
-- 
2.1.2

[iproute PATCH 2/3] ip: macvlan: support MACVLAN_FLAG_NOPROMISC flag

[Phil Sutter]

This flag is allowed for devices in passthru mode to prevent forcing the
underlying interface into promiscuous mode.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/iplink_macvlan.c | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/ip/iplink_macvlan.c b/ip/iplink_macvlan.c
index d759f0e..f195e81 100644
--- a/ip/iplink_macvlan.c
+++ b/ip/iplink_macvlan.c
@@ -29,7 +29,7 @@
 static void print_explain(struct link_util *lu, FILE *f)
 {
 	fprintf(f,
-		"Usage: ... %s mode { private | vepa | bridge | passthru }\n",
+		"Usage: ... %s mode { private | vepa | bridge | passthru [nopromisc] }\n",
 		lu->id
 	);
 }
@@ -49,9 +49,11 @@ static int mode_arg(const char *arg)
 static int macvlan_parse_opt(struct link_util *lu, int argc, char **argv,
 			  struct nlmsghdr *n)
 {
+	__u32 mode = 0;
+	__u16 flags = 0;
+
 	while (argc > 0) {
 		if (matches(*argv, "mode") == 0) {
-			__u32 mode = 0;
 			NEXT_ARG();
 
 			if (strcmp(*argv, "private") == 0)
@@ -64,7 +66,8 @@ static int macvlan_parse_opt(struct link_util *lu, int argc, char **argv,
 				mode = MACVLAN_MODE_PASSTHRU;
 			else
 				return mode_arg(*argv);
-			addattr32(n, 1024, IFLA_MACVLAN_MODE, mode);
+		} else if (matches(*argv, "nopromisc") == 0) {
+			flags |= MACVLAN_FLAG_NOPROMISC;
 		} else if (matches(*argv, "help") == 0) {
 			explain(lu);
 			return -1;
@@ -76,12 +79,25 @@ static int macvlan_parse_opt(struct link_util *lu, int argc, char **argv,
 		argc--, argv++;
 	}
 
+	if (mode)
+		addattr32(n, 1024, IFLA_MACVLAN_MODE, mode);
+
+	if (flags) {
+		if (flags & MACVLAN_FLAG_NOPROMISC &&
+		    mode != MACVLAN_MODE_PASSTHRU) {
+			pfx_err(lu, "nopromisc flag only valid in passthru mode");
+			explain(lu);
+			return -1;
+		}
+		addattr16(n, 1024, IFLA_MACVLAN_FLAGS, flags);
+	}
 	return 0;
 }
 
 static void macvlan_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 {
 	__u32 mode;
+	__u16 flags;
 
 	if (!tb)
 		return;
@@ -97,6 +113,14 @@ static void macvlan_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[]
 		: mode == MACVLAN_MODE_BRIDGE  ? "bridge"
 		: mode == MACVLAN_MODE_PASSTHRU  ? "passthru"
 		:				 "unknown");
+
+	if (!tb[IFLA_MACVLAN_FLAGS] ||
+	    RTA_PAYLOAD(tb[IFLA_MACVLAN_FLAGS]) < sizeof(__u16))
+		return;
+
+	flags = rta_getattr_u16(tb[IFLA_MACVLAN_FLAGS]);
+	if (flags & MACVLAN_FLAG_NOPROMISC)
+		fprintf(f, "nopromisc ");
 }
 
 static void macvlan_print_help(struct link_util *lu, int argc, char **argv,
-- 
2.1.2

[iproute PATCH 3/3] man: ip-link: document MACVLAN/MACVTAP interface types

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 man/man8/ip-link.8.in | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/man/man8/ip-link.8.in b/man/man8/ip-link.8.in
index 4928249..ac6f481 100644
--- a/man/man8/ip-link.8.in
+++ b/man/man8/ip-link.8.in
@@ -768,6 +768,56 @@ the following additional arguments are supported:
 
 .in -8
 
+.TP
+MACVLAN and MACVTAP Type Support
+For a link of type
+.I MACVLAN
+or
+.I MACVTAP
+the following additional arguments are supported:
+
+.BI "ip link add link " DEVICE " name " NAME
+.BR type " { " macvlan " | " macvtap " } "
+.BR mode " { " private " | " vepa " | " bridge " | " passthru
+.BR " [ " nopromisc " ] } "
+
+.in +8
+.sp
+.BR type " { " macvlan " | " macvtap " } "
+- specifies the link type to use.
+.BR macvlan " creates just a virtual interface, while "
+.BR macvtap " in addition creates a character device "
+.BR /dev/tapX " to be used just like a " tuntap " device."
+
+.B mode private
+- Do not allow communication between
+.B macvlan
+instances on the same physical interface, even if the external switch supports
+hairpin mode.
+
+.B mode vepa
+- Virtual Ethernet Port Aggregator mode. Data from one
+.B macvlan
+instance to the other on the same physical interface is transmitted over the
+physical interface. Either the attached switch needs to support hairpin mode,
+or there must be a TCP/IP router forwarding the packets in order to allow
+communication. This is the default mode.
+
+.B mode bridge
+- In bridge mode, all endpoints are directly connected to each other,
+communication is not redirected through the physical interface's peer.
+
+.BR mode " " passthru " [ " nopromisc " ] "
+- This mode gives more power to a single endpoint, usually in
+.BR macvtap " mode. It is not allowed for more than one endpoint on the same "
+physical interface. All traffic will be forwarded to this endpoint, allowing
+virtio guests to change MAC address or set promiscuous mode in order to bridge
+the interface or create vlan interfaces on top of it. By default, this mode
+forces the underlying interface into promiscuous mode. Passing the
+.BR nopromisc " flag prevents this, so the promisc flag may be controlled "
+using standard tools.
+.in -8
+
 .SS ip link delete - delete virtual link
 
 .TP
-- 
2.1.2

Re: [iproute PATCH 0/3] improve MACVLAN/MACVTAP support

[Stephen Hemminger]

On Fri, 25 Sep 2015 14:09:48 +0200
Phil Sutter <phil@nwl.cc> wrote:

> While implementing support for MACVLAN_FLAG_NOPROMISC, I realized how similar
> iplink_macvlan.c and iplink_macvtap.c are and that the main differences (apart
> from substituting "macvlan" for "macvtap") were fixes and enhancements which
> weren't applied to both files. To prevent this in the future and to share
> common code, this series merges the files. In addition, it implements support
> for MACVLAN_FLAG_NOPROMISC and finally documents these interface types in
> ip-link.8.in.
> 
> Phil Sutter (3):
>   ip: link: consolidate macvlan and macvtap
>   ip: macvlan: support MACVLAN_FLAG_NOPROMISC flag
>   man: ip-link: document MACVLAN/MACVTAP interface types
> 
>  ip/Makefile           |   2 +-
>  ip/iplink_macvlan.c   |  68 ++++++++++++++++++++++++--------
>  ip/iplink_macvtap.c   | 105 --------------------------------------------------
>  man/man8/ip-link.8.in |  50 ++++++++++++++++++++++++
>  4 files changed, 104 insertions(+), 121 deletions(-)
>  delete mode 100644 ip/iplink_macvtap.c
> 

Looks good applied. Always love to see more code deleted..