diff mbox series

[v2,iproute2] ip: support for xfrm interfaces

Message ID 20190117094037.36ea8631@aquamarine
State Changes Requested
Delegated to: David Ahern
Headers show
Series [v2,iproute2] ip: support for xfrm interfaces | expand

Commit Message

Matt Ellison Jan. 17, 2019, 2:40 p.m. UTC 
Interfaces take a 'if_id' which is an interface id which can be set on
an xfrm policy as its interface lookup key (XFRMA_IF_ID).

Signed-off-by: Matt Ellison <matt@arroyo.io>
---
 ip/Makefile                             |  2 +-
 ip/iplink.c                             |  3 +-
 ip/link_xfrm.c                          | 79 +++++++++++++++++++++++++
 man/man8/ip-link.8.in                   | 27 ++++++++-
 testsuite/tests/ip/link/add_type_xfrm.t | 32 ++++++++++
 5 files changed, 140 insertions(+), 3 deletions(-)
 create mode 100644 ip/link_xfrm.c
 create mode 100755 testsuite/tests/ip/link/add_type_xfrm.t

Comments

David Ahern Jan. 21, 2019, 4:14 p.m. UTC | #1 
On 1/17/19 7:40 AM, Matt Ellison wrote:
> +static int xfrm_parse_opt(struct link_util *lu, int argc, char **argv,
> +			  struct nlmsghdr *n)
> +{
> +	unsigned int link = 0;
> +	__u32 if_id = 0;
> +
> +	while (argc > 0) {
> +		if (!matches(*argv, "dev")) {
> +			NEXT_ARG();
> +			link = ll_name_to_index(*argv);
> +			if (!link)
> +				exit(nodev(*argv));
> +		} else if (!matches(*argv, "if_id")) {
> +			NEXT_ARG();
> +			if (get_u32(&if_id, *argv, 0))
> +				invarg("if_id", *argv);
> +		} else {
> +			xfrm_print_help(lu, argc, argv, stderr);
> +			return -1;
> +		}
> +		argc--; argv++;
> +	}
> +
> +	addattr32(n, 1024, IFLA_XFRM_IF_ID, if_id);

You always add IF_ID even if not set by user. The kernel code does not
appear to require it so why pass a default value?

> +
> +	if (link) {
> +		addattr32(n, 1024, IFLA_XFRM_LINK, link);
> +	} else {
> +		fprintf(stderr, "must specify physical device\n");
> +		return -1;
> +	}

The kernel code does appear to require this parameter, so why have this
requirement in iproute2?

> +
> +	return 0;
> +}
> +
> +static void xfrm_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
> +{
> +
> +	if (!tb)
> +		return;
> +
> +	if (tb[IFLA_XFRM_IF_ID]) {
> +		__u32 id = rta_getattr_u32(tb[IFLA_XFRM_IF_ID]);
> +
> +		print_0xhex(PRINT_ANY, "if_id", "if_id %#llx ", id);
> +
> +	}

What about IFLA_XFRM_LINK?


> diff --git a/testsuite/tests/ip/link/add_type_xfrm.t b/testsuite/tests/ip/link/add_type_xfrm.t
> new file mode 100755
> index 00000000..78ce28e0
> --- /dev/null
> +++ b/testsuite/tests/ip/link/add_type_xfrm.t
> @@ -0,0 +1,32 @@
> +#!/bin/sh
> +
> +. lib/generic.sh
> +
> +ts_log "[Testing Add XFRM Interface, With IF-ID]"
> +
> +PHYS_DEV="lo"
> +NEW_DEV="$(rand_dev)"
> +IF_ID="0xf"
> +
> +ts_ip "$0" "Add $NEW_DEV xfrm interface"    link add dev $NEW_DEV type xfrm dev $PHYS_DEV if_id $IF_ID
> +
> +ts_ip "$0" "Show $NEW_DEV xfrm interface"   -d link show dev $NEW_DEV
> +test_on "$NEW_DEV"
> +test_on "if_id $IF_ID"
> +
> +ts_ip "$0" "Del $NEW_DEV xfrm interface"   link del dev $NEW_DEV
> +
> +
> +ts_log "[Testing Add XFRM Interface, No IF-ID]"
> +
> +PHYS_DEV="lo"
> +NEW_DEV="$(rand_dev)"
> +IF_ID="0xf"
> +
> +ts_ip "$0" "Add $NEW_DEV xfrm interface"    link add dev $NEW_DEV type xfrm dev $PHYS_DEV
> +
> +ts_ip "$0" "Show $NEW_DEV xfrm interface"   -d link show dev $NEW_DEV
> +test_on "$NEW_DEV"
> +test_on_not "if_id $IF_ID"
> +
> +ts_ip "$0" "Del $NEW_DEV xfrm interface"   link del dev $NEW_DEV
> 

Thanks for adding test cases!
Matt Ellison Jan. 21, 2019, 5:05 p.m. UTC | #2 
On Mon, 21 Jan 2019 09:14:52 -0700 David Ahern <dsahern@gmail.com> wrote:

> You always add IF_ID even if not set by user. The kernel code does not
> appear to require it so why pass a default value?

0 (the default) is a valid IF_ID, so setting an interface with a non-zero IF_ID 
back to 0 is possible. I think the better solution would be to check for existing
values so an "ip link set" doesn't try and automatically use 0 if unsepcified.

> The kernel code does appear to require this parameter, so why have
> this requirement in iproute2?

Looks to be required, without the check I get:
RTNETLINK answers: No such device

> What about IFLA_XFRM_LINK?

It shows up as the parent interface when you ip link show: 

13: xfrm2@wlan0: <NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
    link/none 48:e2:44:f6:77:51 brd ff:ff:ff:ff:ff:ff

But I can print it again if you think I should.
David Ahern Jan. 21, 2019, 5:40 p.m. UTC | #3 
On 1/21/19 10:05 AM, Matt Ellison wrote:
> On Mon, 21 Jan 2019 09:14:52 -0700 David Ahern <dsahern@gmail.com> wrote:
> 
>> You always add IF_ID even if not set by user. The kernel code does not
>> appear to require it so why pass a default value?
> 
> 0 (the default) is a valid IF_ID, so setting an interface with a non-zero IF_ID 
> back to 0 is possible. I think the better solution would be to check for existing
> values so an "ip link set" doesn't try and automatically use 0 if unsepcified.

I would expect IF_ID to only be added to the request if passed on the
command line. That should handle the set link case.

> 
>> The kernel code does appear to require this parameter, so why have
>> this requirement in iproute2?
> 
> Looks to be required, without the check I get:
> RTNETLINK answers: No such device
> 
>> What about IFLA_XFRM_LINK?
> 
> It shows up as the parent interface when you ip link show: 
> 
> 13: xfrm2@wlan0: <NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
>     link/none 48:e2:44:f6:77:51 brd ff:ff:ff:ff:ff:ff
> 
> But I can print it again if you think I should.
> 

ok, so IFLA_XFRM_LINK duplicates IFLA_LINK. no need to print twice.
diff mbox series

Patch

diff --git a/ip/Makefile b/ip/Makefile
index a88f9366..7ce6e91a 100644
--- a/ip/Makefile
+++ b/ip/Makefile
@@ -5,7 +5,7 @@  IPOBJ=ip.o ipaddress.o ipaddrlabel.o iproute.o iprule.o ipnetns.o \
     ipxfrm.o xfrm_state.o xfrm_policy.o xfrm_monitor.o iplink_dummy.o \
     iplink_ifb.o iplink_nlmon.o iplink_team.o iplink_vcan.o iplink_vxcan.o \
     iplink_vlan.o link_veth.o link_gre.o iplink_can.o iplink_xdp.o \
-    iplink_macvlan.o ipl2tp.o link_vti.o link_vti6.o \
+    iplink_macvlan.o ipl2tp.o link_vti.o link_vti6.o link_xfrm.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.c b/ip/iplink.c
index b5519201..f61e570a 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -121,7 +121,8 @@  void iplink_usage(void)
 			"          bridge | bond | team | ipoib | ip6tnl | ipip | sit | vxlan |\n"
 			"          gre | gretap | erspan | ip6gre | ip6gretap | ip6erspan |\n"
 			"          vti | nlmon | team_slave | bond_slave | bridge_slave |\n"
-			"          ipvlan | ipvtap | geneve | vrf | macsec | netdevsim | rmnet }\n");
+			"          ipvlan | ipvtap | geneve | vrf | macsec | netdevsim | rmnet |\n"
+			"          xfrm }\n");
 	}
 	exit(-1);
 }
diff --git a/ip/link_xfrm.c b/ip/link_xfrm.c
new file mode 100644
index 00000000..1115fde5
--- /dev/null
+++ b/ip/link_xfrm.c
@@ -0,0 +1,79 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * link_xfrm.c	Virtual XFRM Interface driver module
+ *
+ * Authors:	Matt Ellison <matt@arroyo.io>
+ */
+
+#include <string.h>
+#include <linux/if_link.h>
+
+#include "rt_names.h"
+#include "utils.h"
+#include "ip_common.h"
+#include "tunnel.h"
+
+static void xfrm_print_help(struct link_util *lu, int argc, char **argv,
+			    FILE *f)
+{
+	fprintf(f, "Usage: ... %-4s dev PHYS_DEV [ if_id IF-ID ]\n", lu->id);
+	fprintf(f, "\nWhere: IF-ID := { 0x0..0xffffffff }\n");
+}
+
+static int xfrm_parse_opt(struct link_util *lu, int argc, char **argv,
+			  struct nlmsghdr *n)
+{
+	unsigned int link = 0;
+	__u32 if_id = 0;
+
+	while (argc > 0) {
+		if (!matches(*argv, "dev")) {
+			NEXT_ARG();
+			link = ll_name_to_index(*argv);
+			if (!link)
+				exit(nodev(*argv));
+		} else if (!matches(*argv, "if_id")) {
+			NEXT_ARG();
+			if (get_u32(&if_id, *argv, 0))
+				invarg("if_id", *argv);
+		} else {
+			xfrm_print_help(lu, argc, argv, stderr);
+			return -1;
+		}
+		argc--; argv++;
+	}
+
+	addattr32(n, 1024, IFLA_XFRM_IF_ID, if_id);
+
+	if (link) {
+		addattr32(n, 1024, IFLA_XFRM_LINK, link);
+	} else {
+		fprintf(stderr, "must specify physical device\n");
+		return -1;
+	}
+
+	return 0;
+}
+
+static void xfrm_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
+{
+
+	if (!tb)
+		return;
+
+	if (tb[IFLA_XFRM_IF_ID]) {
+		__u32 id = rta_getattr_u32(tb[IFLA_XFRM_IF_ID]);
+
+		print_0xhex(PRINT_ANY, "if_id", "if_id %#llx ", id);
+
+	}
+
+}
+
+struct link_util xfrm_link_util = {
+	.id = "xfrm",
+	.maxattr = IFLA_XFRM_MAX,
+	.parse_opt = xfrm_parse_opt,
+	.print_opt = xfrm_print_opt,
+	.print_help = xfrm_print_help,
+};
diff --git a/man/man8/ip-link.8.in b/man/man8/ip-link.8.in
index 73d37c19..53504657 100644
--- a/man/man8/ip-link.8.in
+++ b/man/man8/ip-link.8.in
@@ -221,7 +221,8 @@  ip-link \- network device configuration
 .BR vrf " |"
 .BR macsec " |"
 .BR netdevsim " |"
-.BR rmnet " ]"
+.BR rmnet " |"
+.BR xfrm " ]"
 
 .ti -8
 .IR ETYPE " := [ " TYPE " |"
@@ -350,6 +351,9 @@  Link types:
 .sp
 .BR rmnet
 - Qualcomm rmnet device
+.sp
+.BR xfrm
+- Virtual xfrm interface
 .in -8
 
 .TP
@@ -1704,6 +1708,27 @@  the following additional arguments are supported:
 
 .in -8
 
+.TP
+XFRM Type Support
+For a link of type
+.I XFRM
+the following additional arguments are supported:
+
+.BI "ip link add " DEVICE " type xfrm dev " PHYS_DEV " [ if_id " IF_ID " ]"
+
+.in +8
+.sp
+.BI dev " PHYS_DEV "
+- specifies the underlying physical interface from which transform traffic is sent and received.
+
+.sp
+.BI if_id " IF-ID "
+- specifies the hexadecimal lookup key used to send traffic to and from specific xfrm
+policies. Policies must be configured with the same key. If not set, the key defaults to
+0 and will match any policies which similarly do not have a lookup key configuration.
+
+.in -8
+
 .SS ip link delete - delete virtual link
 
 .TP
diff --git a/testsuite/tests/ip/link/add_type_xfrm.t b/testsuite/tests/ip/link/add_type_xfrm.t
new file mode 100755
index 00000000..78ce28e0
--- /dev/null
+++ b/testsuite/tests/ip/link/add_type_xfrm.t
@@ -0,0 +1,32 @@ 
+#!/bin/sh
+
+. lib/generic.sh
+
+ts_log "[Testing Add XFRM Interface, With IF-ID]"
+
+PHYS_DEV="lo"
+NEW_DEV="$(rand_dev)"
+IF_ID="0xf"
+
+ts_ip "$0" "Add $NEW_DEV xfrm interface"    link add dev $NEW_DEV type xfrm dev $PHYS_DEV if_id $IF_ID
+
+ts_ip "$0" "Show $NEW_DEV xfrm interface"   -d link show dev $NEW_DEV
+test_on "$NEW_DEV"
+test_on "if_id $IF_ID"
+
+ts_ip "$0" "Del $NEW_DEV xfrm interface"   link del dev $NEW_DEV
+
+
+ts_log "[Testing Add XFRM Interface, No IF-ID]"
+
+PHYS_DEV="lo"
+NEW_DEV="$(rand_dev)"
+IF_ID="0xf"
+
+ts_ip "$0" "Add $NEW_DEV xfrm interface"    link add dev $NEW_DEV type xfrm dev $PHYS_DEV
+
+ts_ip "$0" "Show $NEW_DEV xfrm interface"   -d link show dev $NEW_DEV
+test_on "$NEW_DEV"
+test_on_not "if_id $IF_ID"
+
+ts_ip "$0" "Del $NEW_DEV xfrm interface"   link del dev $NEW_DEV