[PATCH iproute2-next v2] ip-xfrm: Add support for OUTPUT_MARK

[PATCH iproute2-next v2] ip-xfrm: Add support for OUTPUT_MARK

[Subash Abhinov Kasiviswanathan]

This patch adds support for OUTPUT_MARK in xfrm state to exercise the
functionality added by kernel commit 077fbac405bf
("net: xfrm: support setting an output mark.").

Sample output with output-mark -

src 192.168.1.1 dst 192.168.1.2
        proto esp spi 0x00004321 reqid 0 mode tunnel
        replay-window 0 flag af-unspec
        mark 0x10000/0x3ffff
        output-mark 0x20000
        auth-trunc xcbc(aes) 0x3ed0af408cf5dcbf5d5d9a5fa806b211 96
        enc cbc(aes) 0x3ed0af408cf5dcbf5d5d9a5fa806b233
        anti-replay context: seq 0x0, oseq 0x0, bitmap 0x00000000

v1->v2: Moved the XFRMA_OUTPUT_MARK print after XFRMA_MARK in
xfrm_xfrma_print() as mentioned by Lorenzo

Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
---
 ip/ipxfrm.c        | 6 ++++++
 ip/xfrm_state.c    | 9 +++++++++
 man/man8/ip-xfrm.8 | 2 ++
 3 files changed, 17 insertions(+)

diff --git a/ip/ipxfrm.c b/ip/ipxfrm.c
index 12c2f72..8b88c8f 100644
--- a/ip/ipxfrm.c
+++ b/ip/ipxfrm.c
@@ -681,6 +681,12 @@ void xfrm_xfrma_print(struct rtattr *tb[], __u16 family,
 		fprintf(fp, "%s", _SL_);
 	}
 
+	if (tb[XFRMA_OUTPUT_MARK]) {
+		__u32 output_mark = rta_getattr_u32(tb[XFRMA_OUTPUT_MARK]);
+
+		fprintf(fp, "\toutput-mark 0x%x %s", output_mark, _SL_);
+	}
+
 	if (tb[XFRMA_ALG_AUTH] && !tb[XFRMA_ALG_AUTH_TRUNC]) {
 		struct rtattr *rta = tb[XFRMA_ALG_AUTH];
 
diff --git a/ip/xfrm_state.c b/ip/xfrm_state.c
index 85d959c..d005802 100644
--- a/ip/xfrm_state.c
+++ b/ip/xfrm_state.c
@@ -61,6 +61,7 @@ static void usage(void)
 	fprintf(stderr, "        [ flag FLAG-LIST ] [ sel SELECTOR ] [ LIMIT-LIST ] [ encap ENCAP ]\n");
 	fprintf(stderr, "        [ coa ADDR[/PLEN] ] [ ctx CTX ] [ extra-flag EXTRA-FLAG-LIST ]\n");
 	fprintf(stderr, "        [ offload [dev DEV] dir DIR ]\n");
+	fprintf(stderr, "        [ output-mark OUTPUT-MARK]\n");
 	fprintf(stderr, "Usage: ip xfrm state allocspi ID [ mode MODE ] [ mark MARK [ mask MASK ] ]\n");
 	fprintf(stderr, "        [ reqid REQID ] [ seq SEQ ] [ min SPI max SPI ]\n");
 	fprintf(stderr, "Usage: ip xfrm state { delete | get } ID [ mark MARK [ mask MASK ] ]\n");
@@ -322,6 +323,7 @@ static int xfrm_state_modify(int cmd, unsigned int flags, int argc, char **argv)
 		struct xfrm_user_sec_ctx sctx;
 		char    str[CTX_BUF_SIZE];
 	} ctx = {};
+	__u32 output_mark = 0;
 
 	while (argc > 0) {
 		if (strcmp(*argv, "mode") == 0) {
@@ -437,6 +439,10 @@ static int xfrm_state_modify(int cmd, unsigned int flags, int argc, char **argv)
 				invarg("value after \"offload dir\" is invalid", *argv);
 				is_offload = false;
 			}
+		} else if (strcmp(*argv, "output-mark") == 0) {
+			NEXT_ARG();
+			if (get_u32(&output_mark, *argv, 0))
+				invarg("value after \"output-mark\" is invalid", *argv);
 		} else {
 			/* try to assume ALGO */
 			int type = xfrm_algotype_getbyname(*argv);
@@ -720,6 +726,9 @@ static int xfrm_state_modify(int cmd, unsigned int flags, int argc, char **argv)
 		}
 	}
 
+	if (output_mark != 0)
+		addattr32(&req.n, sizeof(req.buf), XFRMA_OUTPUT_MARK, output_mark);
+
 	if (rtnl_open_byproto(&rth, 0, NETLINK_XFRM) < 0)
 		exit(1);
 
diff --git a/man/man8/ip-xfrm.8 b/man/man8/ip-xfrm.8
index 988cc6a..e001596 100644
--- a/man/man8/ip-xfrm.8
+++ b/man/man8/ip-xfrm.8
@@ -59,6 +59,8 @@ ip-xfrm \- transform configuration
 .IR CTX " ]"
 .RB "[ " extra-flag
 .IR EXTRA-FLAG-LIST " ]"
+.RB "[ " output-mark
+.IR OUTPUT-MARK " ]"
 
 .ti -8
 .B "ip xfrm state allocspi"
-- 
1.9.1

Re: [PATCH iproute2-next v2] ip-xfrm: Add support for OUTPUT_MARK

[Lorenzo Colitti]

On Wed, Jun 13, 2018 at 3:48 AM Subash Abhinov Kasiviswanathan
<subashab@codeaurora.org> wrote:
>
> src 192.168.1.1 dst 192.168.1.2
>         proto esp spi 0x00004321 reqid 0 mode tunnel
>         replay-window 0 flag af-unspec
>         mark 0x10000/0x3ffff
>         output-mark 0x20000

Nit: I don't know what guarantees we provide (if any) that the output
format of "ip xfrm state" does not change except to add new lines at
the end. Personally, I feel that an app or script that depends on
"auth-trunc" (or anything else, really) being on the line immediately
after "mark" is brittle and should be fixed. This is particularly true
since in general between the mark and the encryption there might be an
auth-trunc line, or an auth line, or neither. As such, adding this
line here seems OK to me.

> @@ -61,6 +61,7 @@ static void usage(void)
>         fprintf(stderr, "        [ flag FLAG-LIST ] [ sel SELECTOR ] [ LIMIT-LIST ] [ encap ENCAP ]\n");
>         fprintf(stderr, "        [ coa ADDR[/PLEN] ] [ ctx CTX ] [ extra-flag EXTRA-FLAG-LIST ]\n");
>         fprintf(stderr, "        [ offload [dev DEV] dir DIR ]\n");
> +       fprintf(stderr, "        [ output-mark OUTPUT-MARK]\n");

Nit: I think you want a space between OUTPUT-MARK and ].

Other than that,

Acked-by: Lorenzo Colitti <lorenzo@google.com>

Re: [PATCH iproute2-next v2] ip-xfrm: Add support for OUTPUT_MARK

[Stephen Hemminger]

On Wed, 13 Jun 2018 12:14:53 +0900
Lorenzo Colitti <lorenzo@google.com> wrote:

> On Wed, Jun 13, 2018 at 3:48 AM Subash Abhinov Kasiviswanathan
> <subashab@codeaurora.org> wrote:
> >
> > src 192.168.1.1 dst 192.168.1.2
> >         proto esp spi 0x00004321 reqid 0 mode tunnel
> >         replay-window 0 flag af-unspec
> >         mark 0x10000/0x3ffff
> >         output-mark 0x20000  
> 
> Nit: I don't know what guarantees we provide (if any) that the output
> format of "ip xfrm state" does not change except to add new lines at
> the end. Personally, I feel that an app or script that depends on
> "auth-trunc" (or anything else, really) being on the line immediately
> after "mark" is brittle and should be fixed. This is particularly true
> since in general between the mark and the encryption there might be an
> auth-trunc line, or an auth line, or neither. As such, adding this
> line here seems OK to me.

Scripts should use json mode. If it ever gets added to xfrm output (hint).

Re: [PATCH iproute2-next v2] ip-xfrm: Add support for OUTPUT_MARK

[David Ahern]

On 6/12/18 9:14 PM, Lorenzo Colitti wrote:
> On Wed, Jun 13, 2018 at 3:48 AM Subash Abhinov Kasiviswanathan
> <subashab@codeaurora.org> wrote:
>>
>> src 192.168.1.1 dst 192.168.1.2
>>         proto esp spi 0x00004321 reqid 0 mode tunnel
>>         replay-window 0 flag af-unspec
>>         mark 0x10000/0x3ffff
>>         output-mark 0x20000
> 
> Nit: I don't know what guarantees we provide (if any) that the output
> format of "ip xfrm state" does not change except to add new lines at
> the end. Personally, I feel that an app or script that depends on
> "auth-trunc" (or anything else, really) being on the line immediately
> after "mark" is brittle and should be fixed. This is particularly true
> since in general between the mark and the encryption there might be an
> auth-trunc line, or an auth line, or neither. As such, adding this
> line here seems OK to me.

any reason to put output-mark on its own line? Why not
	mark 0x10000/0x3ffff output-mark 0x20000

is the documentation clear on the difference between mark and output-mark?

> 
>> @@ -61,6 +61,7 @@ static void usage(void)
>>         fprintf(stderr, "        [ flag FLAG-LIST ] [ sel SELECTOR ] [ LIMIT-LIST ] [ encap ENCAP ]\n");
>>         fprintf(stderr, "        [ coa ADDR[/PLEN] ] [ ctx CTX ] [ extra-flag EXTRA-FLAG-LIST ]\n");
>>         fprintf(stderr, "        [ offload [dev DEV] dir DIR ]\n");
>> +       fprintf(stderr, "        [ output-mark OUTPUT-MARK]\n");
> 
> Nit: I think you want a space between OUTPUT-MARK and ].

yes.

> 
> Other than that,
> 
> Acked-by: Lorenzo Colitti <lorenzo@google.com>
> 

Re: [PATCH iproute2-next v2] ip-xfrm: Add support for OUTPUT_MARK

[Subash Abhinov Kasiviswanathan]

> any reason to put output-mark on its own line? Why not
> 	mark 0x10000/0x3ffff output-mark 0x20000
> 

Hi David

I will move it to the same line in v3.

> is the documentation clear on the difference between mark and 
> output-mark?

Lorenzo has described the differences in detail in the kernel commit I
had listed in the commit text of this patch. Pasting from there -

The output mark differs from the existing xfrm mark in two ways:

1. The xfrm mark is used to match xfrm policies and states, while
    the xfrm output mark is used to set the mark (and influence
    the routing) of the packets emitted by those states.
2. The existing mark is constrained to be a subset of the bits of
    the originating socket or transformed packet, but the output
    mark is arbitrary and depends only on the state.

The use of a separate mark provides additional flexibility. For
example:

- A packet subject to two transforms (e.g., transport mode inside
   tunnel mode) can have two different output marks applied to it,
   one for the transport mode SA and one for the tunnel mode SA.
- On a system where socket marks determine routing, the packets
   emitted by an IPsec tunnel can be routed based on a mark that
   is determined by the tunnel, not by the marks of the
   unencrypted packets.
- Support for setting the output marks can be introduced without
   breaking any existing setups that employ both mark-based
   routing and xfrm tunnel mode. Simply changing the code to use
   the xfrm mark for routing output packets could xfrm mark could
   change behaviour in a way that breaks these setups.

> 
>> 
>>> @@ -61,6 +61,7 @@ static void usage(void)
>>>         fprintf(stderr, "        [ flag FLAG-LIST ] [ sel SELECTOR ] 
>>> [ LIMIT-LIST ] [ encap ENCAP ]\n");
>>>         fprintf(stderr, "        [ coa ADDR[/PLEN] ] [ ctx CTX ] [ 
>>> extra-flag EXTRA-FLAG-LIST ]\n");
>>>         fprintf(stderr, "        [ offload [dev DEV] dir DIR ]\n");
>>> +       fprintf(stderr, "        [ output-mark OUTPUT-MARK]\n");
>> 
>> Nit: I think you want a space between OUTPUT-MARK and ].
> 
> yes.
> 

I will update this as well.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH iproute2-next v2] ip-xfrm: Add support for OUTPUT_MARK

[David Ahern]

On 6/13/18 11:09 PM, Subash Abhinov Kasiviswanathan wrote:
> The output mark differs from the existing xfrm mark in two ways:
> 
> 1. The xfrm mark is used to match xfrm policies and states, while
>    the xfrm output mark is used to set the mark (and influence
>    the routing) of the packets emitted by those states.
> 2. The existing mark is constrained to be a subset of the bits of
>    the originating socket or transformed packet, but the output
>    mark is arbitrary and depends only on the state.

make sure the man page update contains the description of the 2 marks
and why they are used.

Thanks,