| Message ID | 1528829293-23222-1-git-send-email-subashab@codeaurora.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Ahern |
| Headers | show |
| Series | [iproute2-next,v2] ip-xfrm: Add support for OUTPUT_MARK | expand |
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>
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).
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> >
> 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.
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,
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"
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(+)