| Message ID | 20210618160635.703845-1-asbjorn@asbjorn.st |
|---|---|
| State | New |
| Headers | show |
| Series | [iproute2,1/2] tc: pedit: parse_cmd: add flags argument | expand |
[ looks fine to me; adding tc folks to make sure they see it ] On 6/18/21 10:06 AM, Asbjørn Sloth Tønnesen wrote: > Implement a decrement operation for ttl and hoplimit. > > Since this is just syntactic sugar, it goes that: > > tc filter add ... action pedit ex munge ip ttl dec ... > tc filter add ... action pedit ex munge ip6 hoplimit dec ... > > is just a more readable version of this: > > tc filter add ... action pedit ex munge ip ttl add 0xff ... > tc filter add ... action pedit ex munge ip6 hoplimit add 0xff ... > > This feature was suggested by some pseudo tc examples in Mellanox's > documentation[1], but wasn't present in neither their mlnx-iproute2 > nor iproute2. > > Tested with skip_sw on Mellanox ConnectX-6 Dx. > > [1] https://docs.mellanox.com/pages/viewpage.action?pageId=47033989 > > v3: > - Use dedicated flags argument in parse_cmd() (David Ahern) > - Minor rewording of the man page > > v2: > - Fix whitespace issue (Stephen Hemminger) > - Add to usage info in explain() > > Signed-off-by: Asbjørn Sloth Tønnesen <asbjorn@asbjorn.st> > --- > man/man8/tc-pedit.8 | 8 +++++++- > tc/m_pedit.c | 25 +++++++++++++++++++------ > tc/m_pedit.h | 4 ++++ > tc/p_ip.c | 2 +- > tc/p_ip6.c | 2 +- > 5 files changed, 32 insertions(+), 9 deletions(-) > > diff --git a/man/man8/tc-pedit.8 b/man/man8/tc-pedit.8 > index 376ad4a8..15159ddd 100644 > --- a/man/man8/tc-pedit.8 > +++ b/man/man8/tc-pedit.8 > @@ -77,6 +77,7 @@ pedit - generic packet editor action > .IR VAL " | " > .BR add > .IR VAL " | " > +.BR decrement " | " > .BR preserve " } [ " retain > .IR RVAL " ]" > > @@ -96,7 +97,7 @@ chosen automatically based on the header field size. > .B ex > Use extended pedit. > .I EXTENDED_LAYERED_OP > -and the add > +and the add/decrement > .I CMD_SPEC > are allowed only in this mode. > .TP > @@ -288,6 +289,11 @@ is defined by the size of the addressed header field in > .IR EXTENDED_LAYERED_OP . > This operation is supported only for extended layered op. > .TP > +.BI decrement > +Decrease the addressed data by one. > +This operation is supported only for > +.BR ip " " ttl " and " ip6 " " hoplimit "." > +.TP > .B preserve > Keep the addressed data as is. > .TP > diff --git a/tc/m_pedit.c b/tc/m_pedit.c > index b745c379..54949e43 100644 > --- a/tc/m_pedit.c > +++ b/tc/m_pedit.c > @@ -41,7 +41,7 @@ static void explain(void) > "\t\tATC:= at <atval> offmask <maskval> shift <shiftval>\n" > "\t\tNOTE: offval is byte offset, must be multiple of 4\n" > "\t\tNOTE: maskval is a 32 bit hex number\n \t\tNOTE: shiftval is a shift value\n" > - "\t\tCMD:= clear | invert | set <setval>| add <addval> | retain\n" > + "\t\tCMD:= clear | invert | set <setval> | add <addval> | decrement | retain\n" > "\t<LAYERED>:= ip <ipdata> | ip6 <ip6data>\n" > " \t\t| udp <udpdata> | tcp <tcpdata> | icmp <icmpdata>\n" > "\tCONTROL:= reclassify | pipe | drop | continue | pass |\n" > @@ -360,15 +360,24 @@ int parse_cmd(int *argc_p, char ***argv_p, __u32 len, int type, __u32 retain, > if (matches(*argv, "add") == 0) > tkey->cmd = TCA_PEDIT_KEY_EX_CMD_ADD; > > - if (!sel->extended && tkey->cmd) { > - fprintf(stderr, > - "Non extended mode. only 'set' command is supported\n"); > - return -1; > - } > + if (!sel->extended && tkey->cmd) > + goto non_ext_only_set_cmd; > > NEXT_ARG(); > if (parse_val(&argc, &argv, val, type)) > return -1; > + } else if (matches(*argv, "decrement") == 0) { > + if ((flags & PEDIT_ALLOW_DEC) == 0) { > + fprintf(stderr, > + "decrement command is not supported for this field\n"); > + return -1; > + } > + > + if (!sel->extended) > + goto non_ext_only_set_cmd; > + > + tkey->cmd = TCA_PEDIT_KEY_EX_CMD_ADD; > + *v = retain; /* decrement by overflow */ > } else if (matches(*argv, "preserve") == 0) { > retain = 0; > } else { > @@ -431,6 +440,10 @@ done: > *argv_p = argv; > return res; > > +non_ext_only_set_cmd: > + fprintf(stderr, > + "Non extended mode. only 'set' command is supported\n"); > + return -1; > } > > static int parse_offset(int *argc_p, char ***argv_p, struct m_pedit_sel *sel, > diff --git a/tc/m_pedit.h b/tc/m_pedit.h > index 7398f66d..549bcf86 100644 > --- a/tc/m_pedit.h > +++ b/tc/m_pedit.h > @@ -39,6 +39,10 @@ > > #define PEDITKINDSIZ 16 > > +enum m_pedit_flags { > + PEDIT_ALLOW_DEC = 1<<0, > +}; > + > struct m_pedit_key { > __u32 mask; /* AND */ > __u32 val; /*XOR */ > diff --git a/tc/p_ip.c b/tc/p_ip.c > index 2d1643d0..8eed9e8d 100644 > --- a/tc/p_ip.c > +++ b/tc/p_ip.c > @@ -68,7 +68,7 @@ parse_ip(int *argc_p, char ***argv_p, > if (strcmp(*argv, "ttl") == 0) { > NEXT_ARG(); > tkey->off = 8; > - res = parse_cmd(&argc, &argv, 1, TU32, RU8, sel, tkey, 0); > + res = parse_cmd(&argc, &argv, 1, TU32, RU8, sel, tkey, PEDIT_ALLOW_DEC); > goto done; > } > if (strcmp(*argv, "protocol") == 0) { > diff --git a/tc/p_ip6.c b/tc/p_ip6.c > index f9d5d3b0..f855c59e 100644 > --- a/tc/p_ip6.c > +++ b/tc/p_ip6.c > @@ -71,7 +71,7 @@ parse_ip6(int *argc_p, char ***argv_p, > if (strcmp(*argv, "hoplimit") == 0) { > NEXT_ARG(); > tkey->off = 7; > - res = parse_cmd(&argc, &argv, 1, TU32, RU8, sel, tkey, 0); > + res = parse_cmd(&argc, &argv, 1, TU32, RU8, sel, tkey, PEDIT_ALLOW_DEC); > goto done; > } > if (strcmp(*argv, "traffic_class") == 0) { >
Hi Asbjørn, On 2021-06-22 11:39 a.m., David Ahern wrote: > [ looks fine to me; adding tc folks to make sure they see it ] > > On 6/18/21 10:06 AM, Asbjørn Sloth Tønnesen wrote: >> Implement a decrement operation for ttl and hoplimit. >> >> Since this is just syntactic sugar, it goes that: >> >> tc filter add ... action pedit ex munge ip ttl dec ... >> tc filter add ... action pedit ex munge ip6 hoplimit dec ... >> >> is just a more readable version of this: >> >> tc filter add ... action pedit ex munge ip ttl add 0xff ... >> tc filter add ... action pedit ex munge ip6 hoplimit add 0xff ... >> So you "add" essentially one's complement of the value you are trying to decrement with? >> This feature was suggested by some pseudo tc examples in Mellanox's >> documentation[1], but wasn't present in neither their mlnx-iproute2 >> nor iproute2. >> >> Tested with skip_sw on Mellanox ConnectX-6 Dx. >> >> [1] https://docs.mellanox.com/pages/viewpage.action?pageId=47033989 I didnt see an example which showed using "dec" but what you described above makes sense. And the changes below look sane. So: Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> cheers, jamal
Hi Jamal, Thank you for your review. On 6/24/21 8:24 PM, Jamal Hadi Salim wrote: > So you "add" essentially one's complement of the value you are trying to > decrement with? Almost (off by one), it's basically decrement by overflowing it, which is safe since the operation is masked. One should however have another rule, that matches on TTL == 1. so it can get a proper ICMP error. Decrementing TTL by one was actually the prime example presented when Amir Vadai introduced TCA_PEDIT_KEY_EX_CMD_ADD. kernel 853a14ba net/act_pedit: Introduce 'add' operation [1] iproute2 8d193d96 tc/pedit: p_ip: introduce editing ttl header [2] [1] https://git.kernel.org/torvalds/c/853a14ba [2] https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=8d193d96 >>> This feature was suggested by some pseudo tc examples in Mellanox's >>> documentation[1], but wasn't present in neither their mlnx-iproute2 >>> nor iproute2. >>> >>> Tested with skip_sw on Mellanox ConnectX-6 Dx. >>> >>> [1] https://docs.mellanox.com/pages/viewpage.action?pageId=47033989 > > > I didnt see an example which showed using "dec" but what you described > above makes sense. It is indeed a large document, so to be more specific: https://docs.mellanox.com/pages/viewpage.action?pageId=47033989#highlighter_159101 > Using TC rules: > IPv4: > tc filter add [..] munge ip ttl dec [..] > IPv6: > tc filter add [..] munge ipv6 hlimit dec [..] "ipv6 hlimit" should obviously be "ip6 hoplimit". -- Best regards Asbjørn Sloth Tønnesen
On 6/18/21 10:06 AM, Asbjørn Sloth Tønnesen wrote: > Implement a decrement operation for ttl and hoplimit. > > Since this is just syntactic sugar, it goes that: > > tc filter add ... action pedit ex munge ip ttl dec ... > tc filter add ... action pedit ex munge ip6 hoplimit dec ... > > is just a more readable version of this: > > tc filter add ... action pedit ex munge ip ttl add 0xff ... > tc filter add ... action pedit ex munge ip6 hoplimit add 0xff ... > > This feature was suggested by some pseudo tc examples in Mellanox's > documentation[1], but wasn't present in neither their mlnx-iproute2 > nor iproute2. > > Tested with skip_sw on Mellanox ConnectX-6 Dx. > > [1] https://docs.mellanox.com/pages/viewpage.action?pageId=47033989 > > v3: > - Use dedicated flags argument in parse_cmd() (David Ahern) > - Minor rewording of the man page > > v2: > - Fix whitespace issue (Stephen Hemminger) > - Add to usage info in explain() > > Signed-off-by: Asbjørn Sloth Tønnesen <asbjorn@asbjorn.st> > --- > man/man8/tc-pedit.8 | 8 +++++++- > tc/m_pedit.c | 25 +++++++++++++++++++------ > tc/m_pedit.h | 4 ++++ > tc/p_ip.c | 2 +- > tc/p_ip6.c | 2 +- > 5 files changed, 32 insertions(+), 9 deletions(-) > applied both to iproute2-next. Thanks,
diff --git a/tc/m_pedit.c b/tc/m_pedit.c index 74c91e8d..b745c379 100644 --- a/tc/m_pedit.c +++ b/tc/m_pedit.c @@ -330,7 +330,7 @@ static int parse_val(int *argc_p, char ***argv_p, __u32 *val, int type) } int parse_cmd(int *argc_p, char ***argv_p, __u32 len, int type, __u32 retain, - struct m_pedit_sel *sel, struct m_pedit_key *tkey) + struct m_pedit_sel *sel, struct m_pedit_key *tkey, int flags) { __u32 mask[4] = { 0 }; __u32 val[4] = { 0 }; @@ -502,7 +502,7 @@ done: NEXT_ARG(); } - res = parse_cmd(&argc, &argv, len, TU32, retain, sel, tkey); + res = parse_cmd(&argc, &argv, len, TU32, retain, sel, tkey, 0); *argc_p = argc; *argv_p = argv; diff --git a/tc/m_pedit.h b/tc/m_pedit.h index 5d3628a7..7398f66d 100644 --- a/tc/m_pedit.h +++ b/tc/m_pedit.h @@ -73,5 +73,5 @@ struct m_pedit_util { int parse_cmd(int *argc_p, char ***argv_p, __u32 len, int type, __u32 retain, - struct m_pedit_sel *sel, struct m_pedit_key *tkey); + struct m_pedit_sel *sel, struct m_pedit_key *tkey, int flags); #endif diff --git a/tc/p_eth.c b/tc/p_eth.c index 674f9c11..7b6b61f8 100644 --- a/tc/p_eth.c +++ b/tc/p_eth.c @@ -41,21 +41,21 @@ parse_eth(int *argc_p, char ***argv_p, if (strcmp(*argv, "type") == 0) { NEXT_ARG(); tkey->off = 12; - res = parse_cmd(&argc, &argv, 2, TU32, RU16, sel, tkey); + res = parse_cmd(&argc, &argv, 2, TU32, RU16, sel, tkey, 0); goto done; } if (strcmp(*argv, "dst") == 0) { NEXT_ARG(); tkey->off = 0; - res = parse_cmd(&argc, &argv, 6, TMAC, RU32, sel, tkey); + res = parse_cmd(&argc, &argv, 6, TMAC, RU32, sel, tkey, 0); goto done; } if (strcmp(*argv, "src") == 0) { NEXT_ARG(); tkey->off = 6; - res = parse_cmd(&argc, &argv, 6, TMAC, RU32, sel, tkey); + res = parse_cmd(&argc, &argv, 6, TMAC, RU32, sel, tkey, 0); goto done; } diff --git a/tc/p_ip.c b/tc/p_ip.c index c385ac6d..2d1643d0 100644 --- a/tc/p_ip.c +++ b/tc/p_ip.c @@ -40,13 +40,13 @@ parse_ip(int *argc_p, char ***argv_p, if (strcmp(*argv, "src") == 0) { NEXT_ARG(); tkey->off = 12; - res = parse_cmd(&argc, &argv, 4, TIPV4, RU32, sel, tkey); + res = parse_cmd(&argc, &argv, 4, TIPV4, RU32, sel, tkey, 0); goto done; } if (strcmp(*argv, "dst") == 0) { NEXT_ARG(); tkey->off = 16; - res = parse_cmd(&argc, &argv, 4, TIPV4, RU32, sel, tkey); + res = parse_cmd(&argc, &argv, 4, TIPV4, RU32, sel, tkey, 0); goto done; } /* jamal - look at these and make them either old or new @@ -56,64 +56,64 @@ parse_ip(int *argc_p, char ***argv_p, if (strcmp(*argv, "tos") == 0 || matches(*argv, "dsfield") == 0) { NEXT_ARG(); tkey->off = 1; - res = parse_cmd(&argc, &argv, 1, TU32, RU8, sel, tkey); + res = parse_cmd(&argc, &argv, 1, TU32, RU8, sel, tkey, 0); goto done; } if (strcmp(*argv, "ihl") == 0) { NEXT_ARG(); tkey->off = 0; - res = parse_cmd(&argc, &argv, 1, TU32, 0x0f, sel, tkey); + res = parse_cmd(&argc, &argv, 1, TU32, 0x0f, sel, tkey, 0); goto done; } if (strcmp(*argv, "ttl") == 0) { NEXT_ARG(); tkey->off = 8; - res = parse_cmd(&argc, &argv, 1, TU32, RU8, sel, tkey); + res = parse_cmd(&argc, &argv, 1, TU32, RU8, sel, tkey, 0); goto done; } if (strcmp(*argv, "protocol") == 0) { NEXT_ARG(); tkey->off = 9; - res = parse_cmd(&argc, &argv, 1, TU32, RU8, sel, tkey); + res = parse_cmd(&argc, &argv, 1, TU32, RU8, sel, tkey, 0); goto done; } /* jamal - fix this */ if (matches(*argv, "precedence") == 0) { NEXT_ARG(); tkey->off = 1; - res = parse_cmd(&argc, &argv, 1, TU32, RU8, sel, tkey); + res = parse_cmd(&argc, &argv, 1, TU32, RU8, sel, tkey, 0); goto done; } /* jamal - validate this at some point */ if (strcmp(*argv, "nofrag") == 0) { NEXT_ARG(); tkey->off = 6; - res = parse_cmd(&argc, &argv, 1, TU32, 0x3F, sel, tkey); + res = parse_cmd(&argc, &argv, 1, TU32, 0x3F, sel, tkey, 0); goto done; } /* jamal - validate this at some point */ if (strcmp(*argv, "firstfrag") == 0) { NEXT_ARG(); tkey->off = 6; - res = parse_cmd(&argc, &argv, 1, TU32, 0x1F, sel, tkey); + res = parse_cmd(&argc, &argv, 1, TU32, 0x1F, sel, tkey, 0); goto done; } if (strcmp(*argv, "ce") == 0) { NEXT_ARG(); tkey->off = 6; - res = parse_cmd(&argc, &argv, 1, TU32, 0x80, sel, tkey); + res = parse_cmd(&argc, &argv, 1, TU32, 0x80, sel, tkey, 0); goto done; } if (strcmp(*argv, "df") == 0) { NEXT_ARG(); tkey->off = 6; - res = parse_cmd(&argc, &argv, 1, TU32, 0x40, sel, tkey); + res = parse_cmd(&argc, &argv, 1, TU32, 0x40, sel, tkey, 0); goto done; } if (strcmp(*argv, "mf") == 0) { NEXT_ARG(); tkey->off = 6; - res = parse_cmd(&argc, &argv, 1, TU32, 0x20, sel, tkey); + res = parse_cmd(&argc, &argv, 1, TU32, 0x20, sel, tkey, 0); goto done; } @@ -126,25 +126,25 @@ parse_ip(int *argc_p, char ***argv_p, if (strcmp(*argv, "dport") == 0) { NEXT_ARG(); tkey->off = 22; - res = parse_cmd(&argc, &argv, 2, TU32, RU16, sel, tkey); + res = parse_cmd(&argc, &argv, 2, TU32, RU16, sel, tkey, 0); goto done; } if (strcmp(*argv, "sport") == 0) { NEXT_ARG(); tkey->off = 20; - res = parse_cmd(&argc, &argv, 2, TU32, RU16, sel, tkey); + res = parse_cmd(&argc, &argv, 2, TU32, RU16, sel, tkey, 0); goto done; } if (strcmp(*argv, "icmp_type") == 0) { NEXT_ARG(); tkey->off = 20; - res = parse_cmd(&argc, &argv, 1, TU32, RU8, sel, tkey); + res = parse_cmd(&argc, &argv, 1, TU32, RU8, sel, tkey, 0); goto done; } if (strcmp(*argv, "icmp_code") == 0) { NEXT_ARG(); tkey->off = 20; - res = parse_cmd(&argc, &argv, 1, TU32, RU8, sel, tkey); + res = parse_cmd(&argc, &argv, 1, TU32, RU8, sel, tkey, 0); goto done; } return -1; diff --git a/tc/p_ip6.c b/tc/p_ip6.c index 83a6ae81..f9d5d3b0 100644 --- a/tc/p_ip6.c +++ b/tc/p_ip6.c @@ -41,43 +41,43 @@ parse_ip6(int *argc_p, char ***argv_p, if (strcmp(*argv, "src") == 0) { NEXT_ARG(); tkey->off = 8; - res = parse_cmd(&argc, &argv, 16, TIPV6, RU32, sel, tkey); + res = parse_cmd(&argc, &argv, 16, TIPV6, RU32, sel, tkey, 0); goto done; } if (strcmp(*argv, "dst") == 0) { NEXT_ARG(); tkey->off = 24; - res = parse_cmd(&argc, &argv, 16, TIPV6, RU32, sel, tkey); + res = parse_cmd(&argc, &argv, 16, TIPV6, RU32, sel, tkey, 0); goto done; } if (strcmp(*argv, "flow_lbl") == 0) { NEXT_ARG(); tkey->off = 0; - res = parse_cmd(&argc, &argv, 4, TU32, 0x0007ffff, sel, tkey); + res = parse_cmd(&argc, &argv, 4, TU32, 0x0007ffff, sel, tkey, 0); goto done; } if (strcmp(*argv, "payload_len") == 0) { NEXT_ARG(); tkey->off = 4; - res = parse_cmd(&argc, &argv, 2, TU32, RU16, sel, tkey); + res = parse_cmd(&argc, &argv, 2, TU32, RU16, sel, tkey, 0); goto done; } if (strcmp(*argv, "nexthdr") == 0) { NEXT_ARG(); tkey->off = 6; - res = parse_cmd(&argc, &argv, 1, TU32, RU8, sel, tkey); + res = parse_cmd(&argc, &argv, 1, TU32, RU8, sel, tkey, 0); goto done; } if (strcmp(*argv, "hoplimit") == 0) { NEXT_ARG(); tkey->off = 7; - res = parse_cmd(&argc, &argv, 1, TU32, RU8, sel, tkey); + res = parse_cmd(&argc, &argv, 1, TU32, RU8, sel, tkey, 0); goto done; } if (strcmp(*argv, "traffic_class") == 0) { NEXT_ARG(); tkey->off = 1; - res = parse_cmd(&argc, &argv, 1, TU32, RU8, sel, tkey); + res = parse_cmd(&argc, &argv, 1, TU32, RU8, sel, tkey, 0); /* Shift the field by 4 bits on success. */ if (!res) { diff --git a/tc/p_tcp.c b/tc/p_tcp.c index d2dbfd71..ec7b08a2 100644 --- a/tc/p_tcp.c +++ b/tc/p_tcp.c @@ -41,21 +41,21 @@ parse_tcp(int *argc_p, char ***argv_p, if (strcmp(*argv, "sport") == 0) { NEXT_ARG(); tkey->off = 0; - res = parse_cmd(&argc, &argv, 2, TU32, RU16, sel, tkey); + res = parse_cmd(&argc, &argv, 2, TU32, RU16, sel, tkey, 0); goto done; } if (strcmp(*argv, "dport") == 0) { NEXT_ARG(); tkey->off = 2; - res = parse_cmd(&argc, &argv, 2, TU32, RU16, sel, tkey); + res = parse_cmd(&argc, &argv, 2, TU32, RU16, sel, tkey, 0); goto done; } if (strcmp(*argv, "flags") == 0) { NEXT_ARG(); tkey->off = 13; - res = parse_cmd(&argc, &argv, 1, TU32, RU8, sel, tkey); + res = parse_cmd(&argc, &argv, 1, TU32, RU8, sel, tkey, 0); goto done; } diff --git a/tc/p_udp.c b/tc/p_udp.c index bab456de..742955e6 100644 --- a/tc/p_udp.c +++ b/tc/p_udp.c @@ -41,14 +41,14 @@ parse_udp(int *argc_p, char ***argv_p, if (strcmp(*argv, "sport") == 0) { NEXT_ARG(); tkey->off = 0; - res = parse_cmd(&argc, &argv, 2, TU32, RU16, sel, tkey); + res = parse_cmd(&argc, &argv, 2, TU32, RU16, sel, tkey, 0); goto done; } if (strcmp(*argv, "dport") == 0) { NEXT_ARG(); tkey->off = 2; - res = parse_cmd(&argc, &argv, 2, TU32, RU16, sel, tkey); + res = parse_cmd(&argc, &argv, 2, TU32, RU16, sel, tkey, 0); goto done; }
This patch just prepares the flags argument, so it's available to the next patch. Signed-off-by: Asbjørn Sloth Tønnesen <asbjorn@asbjorn.st> --- tc/m_pedit.c | 4 ++-- tc/m_pedit.h | 2 +- tc/p_eth.c | 6 +++--- tc/p_ip.c | 32 ++++++++++++++++---------------- tc/p_ip6.c | 14 +++++++------- tc/p_tcp.c | 6 +++--- tc/p_udp.c | 4 ++-- 7 files changed, 34 insertions(+), 34 deletions(-)