* [PATCH RFC iproute2-next v2] tc: add support for act ctinfo
@ 2019-05-31 8:10 ldir.kdb
2019-06-02 7:33 ` Kevin 'ldir' Darbyshire-Bryant
2019-06-02 18:50 ` [PATCH RFC iproute2-next v3] tc: add support for action act_ctinfo Kevin Darbyshire-Bryant
0 siblings, 2 replies; 10+ messages in thread
From: ldir.kdb @ 2019-05-31 8:10 UTC (permalink / raw)
To: netdev, stephen; +Cc: Kevin Darbyshire-Bryant
From: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
ctinfo is an action restoring data stored in conntrack marks to various
fields. At present it has two independent modes of operation,
restoration of DSCP into IPv4/v6 diffserv and restoration of conntrack
marks into packet skb marks.
It understands a number of parameters specific to this action in
additional to the usual action syntax. Each operating mode is
independent of the other so all options are err, optional, however not
specifying at least one mode is a bit pointless.
Usage: ... ctinfo [dscp mask[/statemask]] [cpmark [mask]] [zone ZONE]
[CONTROL] [index <INDEX>]\n"
DSCP mode
dscp enables copying of a DSCP store in the conntrack mark into the
ipv4/v6 diffserv field. The mask is a 32bit field and specifies where
in the conntrack mark the DSCP value is stored. It must be 6 contiguous
bits long, e.g. 0xfc000000 would restore the DSCP from the upper 6 bits
of the conntrack mark.
The DSCP copying may be optionally controlled by a statemask. The
statemask is a 32bit field, usually with a single bit set and must not
overlap the dscp mask. The DSCP restore operation will only take place
if the corresponding bit/s in conntrack mark yield a non zero result.
eg. dscp 0xfc000000/0x01000000 would retrieve the DSCP from the top 6
bits, whilst using bit 25 as a flag to do so. Bit 26 is unused in this
example.
CPMARK mode
cpmark enables copying of the conntrack mark to the packet skb mark. In
this mode it is completely equivalent to the existing act_connmark.
Additional functionality is provided by the optional mask parameter,
whereby the stored conntrack mark is logically anded with the cpmark
mask before being stored into skb mark. This allows shared usage of the
conntrack mark between applications.
eg. cpmark 0x00ffffff would restore only the lower 24 bits of the
conntrack mark, thus may be useful in the event that the upper 8 bits
are used by the DSCP function.
Usage: ... ctinfo [dscp mask[/statemask]] [cpmark [mask]] [zone ZONE]
[CONTROL] [index <INDEX>]
where :
dscp MASK is the bitmask to restore DSCP
STATEMASK is the bitmask to determine conditional restoring
cpmark MASK mask applied to restored packet mark
ZONE is the conntrack zone
CONTROL := reclassify | pipe | drop | continue | ok |
goto chain <CHAIN_INDEX>
Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
---
v2 - fix whitespace issue in pkt_cls
fix most warnings from checkpatch - some lines still over 80 chars
due to long TLV names.
include/uapi/linux/pkt_cls.h | 1 +
include/uapi/linux/tc_act/tc_ctinfo.h | 34 ++++
tc/Makefile | 1 +
tc/m_ctinfo.c | 251 ++++++++++++++++++++++++++
4 files changed, 287 insertions(+)
create mode 100644 include/uapi/linux/tc_act/tc_ctinfo.h
create mode 100644 tc/m_ctinfo.c
diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index 51a0496f..a93680fc 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -105,6 +105,7 @@ enum tca_id {
TCA_ID_IFE = TCA_ACT_IFE,
TCA_ID_SAMPLE = TCA_ACT_SAMPLE,
/* other actions go here */
+ TCA_ID_CTINFO,
__TCA_ID_MAX = 255
};
diff --git a/include/uapi/linux/tc_act/tc_ctinfo.h b/include/uapi/linux/tc_act/tc_ctinfo.h
new file mode 100644
index 00000000..da803e05
--- /dev/null
+++ b/include/uapi/linux/tc_act/tc_ctinfo.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef __UAPI_TC_CTINFO_H
+#define __UAPI_TC_CTINFO_H
+
+#include <linux/types.h>
+#include <linux/pkt_cls.h>
+
+struct tc_ctinfo {
+ tc_gen;
+};
+
+enum {
+ TCA_CTINFO_UNSPEC,
+ TCA_CTINFO_PAD,
+ TCA_CTINFO_TM,
+ TCA_CTINFO_ACT,
+ TCA_CTINFO_ZONE,
+ TCA_CTINFO_PARMS_DSCP_MASK,
+ TCA_CTINFO_PARMS_DSCP_STATEMASK,
+ TCA_CTINFO_PARMS_CPMARK_MASK,
+ TCA_CTINFO_STATS_DSCP_SET,
+ TCA_CTINFO_STATS_DSCP_ERROR,
+ TCA_CTINFO_STATS_CPMARK_SET,
+ __TCA_CTINFO_MAX
+};
+
+#define TCA_CTINFO_MAX (__TCA_CTINFO_MAX - 1)
+
+enum {
+ CTINFO_MODE_DSCP = BIT(0),
+ CTINFO_MODE_CPMARK = BIT(1)
+};
+
+#endif
diff --git a/tc/Makefile b/tc/Makefile
index 1a305cf4..60abddee 100644
--- a/tc/Makefile
+++ b/tc/Makefile
@@ -48,6 +48,7 @@ TCMODULES += m_csum.o
TCMODULES += m_simple.o
TCMODULES += m_vlan.o
TCMODULES += m_connmark.o
+TCMODULES += m_ctinfo.o
TCMODULES += m_bpf.o
TCMODULES += m_tunnel_key.o
TCMODULES += m_sample.o
diff --git a/tc/m_ctinfo.c b/tc/m_ctinfo.c
new file mode 100644
index 00000000..daa08201
--- /dev/null
+++ b/tc/m_ctinfo.c
@@ -0,0 +1,251 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * m_ctinfo.c netfilter ctinfo mark action
+ *
+ * Copyright (c) 2019 Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <string.h>
+#include "utils.h"
+#include "tc_util.h"
+#include <linux/tc_act/tc_ctinfo.h>
+
+static void
+explain(void)
+{
+ fprintf(stderr,
+ "Usage: ... ctinfo [dscp mask[/statemask]] [cpmark [mask]] [zone ZONE] [CONTROL] [index <INDEX>]\n"
+ "where :\n"
+ "\tdscp MASK bitmask location of stored DSCP\n"
+ "\t STATEMASK bitmask to determine conditional restoring\n"
+ "\tcpmark MASK mask applied to mark on restoration\n"
+ "\tZONE is the conntrack zone\n"
+ "\tCONTROL := reclassify | pipe | drop | continue | ok |\n"
+ "\t goto chain <CHAIN_INDEX>\n");
+}
+
+static void
+usage(void)
+{
+ explain();
+ exit(-1);
+}
+
+static int
+parse_ctinfo(struct action_util *a, int *argc_p, char ***argv_p, int tca_id,
+ struct nlmsghdr *n)
+{
+ unsigned int cpmarkmask = 0, dscpmask = 0, dscpstatemask = 0;
+ struct tc_ctinfo sel = {};
+ unsigned short zone = 0;
+ char **argv = *argv_p;
+ struct rtattr *tail;
+ int argc = *argc_p;
+ int ok = 0;
+
+ while (argc > 0) {
+ if (matches(*argv, "ctinfo") == 0) {
+ ok = 1;
+ argc--;
+ argv++;
+ } else if (matches(*argv, "help") == 0) {
+ usage();
+ } else {
+ break;
+ }
+
+ }
+
+ if (!ok) {
+ explain();
+ return -1;
+ }
+
+ if (argc) {
+ if (matches(*argv, "dscp") == 0) {
+ NEXT_ARG();
+ char *slash;
+
+ slash = strchr(*argv, '/');
+ if (slash)
+ *slash = '\0';
+ if (get_u32(&dscpmask, *argv, 0)) {
+ fprintf(stderr,
+ "ctinfo: Illegal dscp \"mask\"\n");
+ return -1;
+ }
+ if (slash && get_u32(&dscpstatemask, slash + 1, 0)) {
+ fprintf(stderr,
+ "ctinfo: Illegal dscp \"statemask\"\n");
+ return -1;
+ }
+ argc--;
+ argv++;
+ }
+ }
+
+ /* cpmark has optional mask parameter, so the next arg might not */
+ /* exist, or it might be the next option, or it may actually be a */
+ /* 32bit mask */
+ if (argc) {
+ if (matches(*argv, "cpmark") == 0) {
+ if (NEXT_ARG_OK()) {
+ NEXT_ARG_FWD();
+ if (!get_u32(&cpmarkmask, *argv, 0))
+ NEXT_ARG_FWD(); /* was a mask */
+ else /* not a mask, use default */
+ cpmarkmask = ~0;
+ } else {
+ NEXT_ARG_FWD();
+ }
+ }
+ }
+
+ if (argc) {
+ if (matches(*argv, "zone") == 0) {
+ NEXT_ARG();
+ if (get_u16(&zone, *argv, 10)) {
+ fprintf(stderr, "ctinfo: Illegal \"zone\"\n");
+ return -1;
+ }
+ argc--;
+ argv++;
+ }
+ }
+
+ parse_action_control_dflt(&argc, &argv, &sel.action,
+ false, TC_ACT_PIPE);
+
+ if (argc) {
+ if (matches(*argv, "index") == 0) {
+ NEXT_ARG();
+ if (get_u32(&sel.index, *argv, 10)) {
+ fprintf(stderr, "ctinfo: Illegal \"index\"\n");
+ return -1;
+ }
+ argc--;
+ argv++;
+ }
+ }
+
+ tail = addattr_nest(n, MAX_MSG, tca_id);
+ addattr_l(n, MAX_MSG, TCA_CTINFO_ACT, &sel, sizeof(sel));
+ if (zone)
+ addattr16(n, MAX_MSG,
+ TCA_CTINFO_ZONE, zone);
+
+ if (dscpmask)
+ addattr32(n, MAX_MSG,
+ TCA_CTINFO_PARMS_DSCP_MASK, dscpmask);
+
+ if (dscpstatemask)
+ addattr32(n, MAX_MSG,
+ TCA_CTINFO_PARMS_DSCP_STATEMASK, dscpstatemask);
+
+ if (cpmarkmask)
+ addattr32(n, MAX_MSG,
+ TCA_CTINFO_PARMS_CPMARK_MASK, cpmarkmask);
+
+ addattr_nest_end(n, tail);
+
+ *argc_p = argc;
+ *argv_p = argv;
+ return 0;
+}
+
+static int print_ctinfo(struct action_util *au, FILE *f, struct rtattr *arg)
+{
+ unsigned int cpmarkmask = ~0, dscpmask = 0, dscpstatemask = 0;
+ struct rtattr *tb[TCA_CTINFO_MAX + 1];
+ unsigned short zone = 0;
+ struct tc_ctinfo *ci;
+
+ if (arg == NULL)
+ return -1;
+
+ parse_rtattr_nested(tb, TCA_CTINFO_MAX, arg);
+ if (!tb[TCA_CTINFO_ACT]) {
+ print_string(PRINT_FP, NULL, "%s",
+ "[NULL ctinfo action parameters]");
+ return -1;
+ }
+
+ ci = RTA_DATA(tb[TCA_CTINFO_ACT]);
+
+ if (tb[TCA_CTINFO_PARMS_DSCP_MASK])
+ if (RTA_PAYLOAD(tb[TCA_CTINFO_PARMS_DSCP_MASK]) >=
+ sizeof(__u32))
+ dscpmask = rta_getattr_u32(tb[TCA_CTINFO_PARMS_DSCP_MASK]);
+ else
+ print_string(PRINT_FP, NULL, "%s",
+ "[invalid dscp mask parameter]");
+
+ if (tb[TCA_CTINFO_PARMS_DSCP_STATEMASK])
+ if (RTA_PAYLOAD(tb[TCA_CTINFO_PARMS_DSCP_STATEMASK]) >=
+ sizeof(__u32))
+ dscpstatemask = rta_getattr_u32(tb[TCA_CTINFO_PARMS_DSCP_STATEMASK]);
+ else
+ print_string(PRINT_FP, NULL, "%s",
+ "[invalid dscp statemask parameter]");
+
+ if (tb[TCA_CTINFO_PARMS_CPMARK_MASK])
+ if (RTA_PAYLOAD(tb[TCA_CTINFO_PARMS_CPMARK_MASK]) >=
+ sizeof(__u32))
+ cpmarkmask = rta_getattr_u32(tb[TCA_CTINFO_PARMS_CPMARK_MASK]);
+ else
+ print_string(PRINT_FP, NULL, "%s",
+ "[invalid cpmark mask parameter]");
+
+ if (tb[TCA_CTINFO_ZONE] && RTA_PAYLOAD(tb[TCA_CTINFO_ZONE]) >=
+ sizeof(__u16))
+ zone = rta_getattr_u16(tb[TCA_CTINFO_ZONE]);
+
+ print_string(PRINT_ANY, "kind", "%s ", "ctinfo");
+ print_hu(PRINT_ANY, "zone", "zone %u", zone);
+ print_action_control(f, " ", ci->action, "");
+
+ print_string(PRINT_FP, NULL, "%s", _SL_);
+ print_uint(PRINT_ANY, "index", "\t index %u", ci->index);
+ print_int(PRINT_ANY, "ref", " ref %d", ci->refcnt);
+ print_int(PRINT_ANY, "bind", " bind %d", ci->bindcnt);
+
+ if (tb[TCA_CTINFO_PARMS_DSCP_MASK]) {
+ print_0xhex(PRINT_ANY, "dscpmask", " dscp %#010llx", dscpmask);
+ print_0xhex(PRINT_ANY, "dscpstatemask", "/%#010llx",
+ dscpstatemask);
+ }
+
+ if (tb[TCA_CTINFO_PARMS_CPMARK_MASK])
+ print_0xhex(PRINT_ANY, "mark", " mark %#010llx", cpmarkmask);
+
+ if (show_stats) {
+ if (tb[TCA_CTINFO_TM]) {
+ struct tcf_t *tm = RTA_DATA(tb[TCA_CTINFO_TM]);
+
+ print_tm(f, tm);
+ }
+
+ if (tb[TCA_CTINFO_STATS_DSCP_SET])
+ print_lluint(PRINT_ANY, "dscpset", " DSCP set %llu",
+ rta_getattr_u64(tb[TCA_CTINFO_STATS_DSCP_SET]));
+ if (tb[TCA_CTINFO_STATS_DSCP_ERROR])
+ print_lluint(PRINT_ANY, "dscperror", " error %llu",
+ rta_getattr_u64(tb[TCA_CTINFO_STATS_DSCP_ERROR]));
+
+ if (tb[TCA_CTINFO_STATS_CPMARK_SET])
+ print_lluint(PRINT_ANY, "cpmarkset", " CPMARK set %llu",
+ rta_getattr_u64(tb[TCA_CTINFO_STATS_CPMARK_SET]));
+ }
+ print_string(PRINT_FP, NULL, "%s", _SL_);
+
+ return 0;
+}
+
+struct action_util ctinfo_action_util = {
+ .id = "ctinfo",
+ .parse_aopt = parse_ctinfo,
+ .print_aopt = print_ctinfo,
+};
--
2.20.1 (Apple Git-117)
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH RFC iproute2-next v2] tc: add support for act ctinfo
2019-05-31 8:10 [PATCH RFC iproute2-next v2] tc: add support for act ctinfo ldir.kdb
@ 2019-06-02 7:33 ` Kevin 'ldir' Darbyshire-Bryant
2019-06-02 18:50 ` [PATCH RFC iproute2-next v3] tc: add support for action act_ctinfo Kevin Darbyshire-Bryant
1 sibling, 0 replies; 10+ messages in thread
From: Kevin 'ldir' Darbyshire-Bryant @ 2019-06-02 7:33 UTC (permalink / raw)
To: Kevin 'ldir' Darbyshire-Bryant; +Cc: netdev, stephen
Please ignore this patch. Have just realised I’ve sent completely
the wrong thing. Somehow managed to send the kernel space patch
again which is already accepted. I will send a v3 of the user space
patch shortly.
Apologies.
Kevin
> On 31 May 2019, at 09:10, ldir.kdb@icloud.com wrote:
>
> From: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
>
> ctinfo is an action restoring data stored in conntrack marks to various
> fields. At present it has two independent modes of operation,
> restoration of DSCP into IPv4/v6 diffserv and restoration of conntrack
> marks into packet skb marks.
>
> It understands a number of parameters specific to this action in
> additional to the usual action syntax. Each operating mode is
> independent of the other so all options are err, optional, however not
> specifying at least one mode is a bit pointless.
>
> Usage: ... ctinfo [dscp mask[/statemask]] [cpmark [mask]] [zone ZONE]
> [CONTROL] [index <INDEX>]\n"
>
> DSCP mode
>
> dscp enables copying of a DSCP store in the conntrack mark into the
> ipv4/v6 diffserv field. The mask is a 32bit field and specifies where
> in the conntrack mark the DSCP value is stored. It must be 6 contiguous
> bits long, e.g. 0xfc000000 would restore the DSCP from the upper 6 bits
> of the conntrack mark.
>
> The DSCP copying may be optionally controlled by a statemask. The
> statemask is a 32bit field, usually with a single bit set and must not
> overlap the dscp mask. The DSCP restore operation will only take place
> if the corresponding bit/s in conntrack mark yield a non zero result.
>
> eg. dscp 0xfc000000/0x01000000 would retrieve the DSCP from the top 6
> bits, whilst using bit 25 as a flag to do so. Bit 26 is unused in this
> example.
>
> CPMARK mode
>
> cpmark enables copying of the conntrack mark to the packet skb mark. In
> this mode it is completely equivalent to the existing act_connmark.
> Additional functionality is provided by the optional mask parameter,
> whereby the stored conntrack mark is logically anded with the cpmark
> mask before being stored into skb mark. This allows shared usage of the
> conntrack mark between applications.
>
> eg. cpmark 0x00ffffff would restore only the lower 24 bits of the
> conntrack mark, thus may be useful in the event that the upper 8 bits
> are used by the DSCP function.
>
> Usage: ... ctinfo [dscp mask[/statemask]] [cpmark [mask]] [zone ZONE]
> [CONTROL] [index <INDEX>]
> where :
> dscp MASK is the bitmask to restore DSCP
> STATEMASK is the bitmask to determine conditional restoring
> cpmark MASK mask applied to restored packet mark
> ZONE is the conntrack zone
> CONTROL := reclassify | pipe | drop | continue | ok |
> goto chain <CHAIN_INDEX>
>
> Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
> ---
> v2 - fix whitespace issue in pkt_cls
> fix most warnings from checkpatch - some lines still over 80 chars
> due to long TLV names.
> include/uapi/linux/pkt_cls.h | 1 +
> include/uapi/linux/tc_act/tc_ctinfo.h | 34 ++++
> tc/Makefile | 1 +
> tc/m_ctinfo.c | 251 ++++++++++++++++++++++++++
> 4 files changed, 287 insertions(+)
> create mode 100644 include/uapi/linux/tc_act/tc_ctinfo.h
> create mode 100644 tc/m_ctinfo.c
>
> diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
> index 51a0496f..a93680fc 100644
> --- a/include/uapi/linux/pkt_cls.h
> +++ b/include/uapi/linux/pkt_cls.h
> @@ -105,6 +105,7 @@ enum tca_id {
> TCA_ID_IFE = TCA_ACT_IFE,
> TCA_ID_SAMPLE = TCA_ACT_SAMPLE,
> /* other actions go here */
> + TCA_ID_CTINFO,
> __TCA_ID_MAX = 255
> };
>
> diff --git a/include/uapi/linux/tc_act/tc_ctinfo.h b/include/uapi/linux/tc_act/tc_ctinfo.h
> new file mode 100644
> index 00000000..da803e05
> --- /dev/null
> +++ b/include/uapi/linux/tc_act/tc_ctinfo.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +#ifndef __UAPI_TC_CTINFO_H
> +#define __UAPI_TC_CTINFO_H
> +
> +#include <linux/types.h>
> +#include <linux/pkt_cls.h>
> +
> +struct tc_ctinfo {
> + tc_gen;
> +};
> +
> +enum {
> + TCA_CTINFO_UNSPEC,
> + TCA_CTINFO_PAD,
> + TCA_CTINFO_TM,
> + TCA_CTINFO_ACT,
> + TCA_CTINFO_ZONE,
> + TCA_CTINFO_PARMS_DSCP_MASK,
> + TCA_CTINFO_PARMS_DSCP_STATEMASK,
> + TCA_CTINFO_PARMS_CPMARK_MASK,
> + TCA_CTINFO_STATS_DSCP_SET,
> + TCA_CTINFO_STATS_DSCP_ERROR,
> + TCA_CTINFO_STATS_CPMARK_SET,
> + __TCA_CTINFO_MAX
> +};
> +
> +#define TCA_CTINFO_MAX (__TCA_CTINFO_MAX - 1)
> +
> +enum {
> + CTINFO_MODE_DSCP = BIT(0),
> + CTINFO_MODE_CPMARK = BIT(1)
> +};
> +
> +#endif
> diff --git a/tc/Makefile b/tc/Makefile
> index 1a305cf4..60abddee 100644
> --- a/tc/Makefile
> +++ b/tc/Makefile
> @@ -48,6 +48,7 @@ TCMODULES += m_csum.o
> TCMODULES += m_simple.o
> TCMODULES += m_vlan.o
> TCMODULES += m_connmark.o
> +TCMODULES += m_ctinfo.o
> TCMODULES += m_bpf.o
> TCMODULES += m_tunnel_key.o
> TCMODULES += m_sample.o
> diff --git a/tc/m_ctinfo.c b/tc/m_ctinfo.c
> new file mode 100644
> index 00000000..daa08201
> --- /dev/null
> +++ b/tc/m_ctinfo.c
> @@ -0,0 +1,251 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * m_ctinfo.c netfilter ctinfo mark action
> + *
> + * Copyright (c) 2019 Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <string.h>
> +#include "utils.h"
> +#include "tc_util.h"
> +#include <linux/tc_act/tc_ctinfo.h>
> +
> +static void
> +explain(void)
> +{
> + fprintf(stderr,
> + "Usage: ... ctinfo [dscp mask[/statemask]] [cpmark [mask]] [zone ZONE] [CONTROL] [index <INDEX>]\n"
> + "where :\n"
> + "\tdscp MASK bitmask location of stored DSCP\n"
> + "\t STATEMASK bitmask to determine conditional restoring\n"
> + "\tcpmark MASK mask applied to mark on restoration\n"
> + "\tZONE is the conntrack zone\n"
> + "\tCONTROL := reclassify | pipe | drop | continue | ok |\n"
> + "\t goto chain <CHAIN_INDEX>\n");
> +}
> +
> +static void
> +usage(void)
> +{
> + explain();
> + exit(-1);
> +}
> +
> +static int
> +parse_ctinfo(struct action_util *a, int *argc_p, char ***argv_p, int tca_id,
> + struct nlmsghdr *n)
> +{
> + unsigned int cpmarkmask = 0, dscpmask = 0, dscpstatemask = 0;
> + struct tc_ctinfo sel = {};
> + unsigned short zone = 0;
> + char **argv = *argv_p;
> + struct rtattr *tail;
> + int argc = *argc_p;
> + int ok = 0;
> +
> + while (argc > 0) {
> + if (matches(*argv, "ctinfo") == 0) {
> + ok = 1;
> + argc--;
> + argv++;
> + } else if (matches(*argv, "help") == 0) {
> + usage();
> + } else {
> + break;
> + }
> +
> + }
> +
> + if (!ok) {
> + explain();
> + return -1;
> + }
> +
> + if (argc) {
> + if (matches(*argv, "dscp") == 0) {
> + NEXT_ARG();
> + char *slash;
> +
> + slash = strchr(*argv, '/');
> + if (slash)
> + *slash = '\0';
> + if (get_u32(&dscpmask, *argv, 0)) {
> + fprintf(stderr,
> + "ctinfo: Illegal dscp \"mask\"\n");
> + return -1;
> + }
> + if (slash && get_u32(&dscpstatemask, slash + 1, 0)) {
> + fprintf(stderr,
> + "ctinfo: Illegal dscp \"statemask\"\n");
> + return -1;
> + }
> + argc--;
> + argv++;
> + }
> + }
> +
> + /* cpmark has optional mask parameter, so the next arg might not */
> + /* exist, or it might be the next option, or it may actually be a */
> + /* 32bit mask */
> + if (argc) {
> + if (matches(*argv, "cpmark") == 0) {
> + if (NEXT_ARG_OK()) {
> + NEXT_ARG_FWD();
> + if (!get_u32(&cpmarkmask, *argv, 0))
> + NEXT_ARG_FWD(); /* was a mask */
> + else /* not a mask, use default */
> + cpmarkmask = ~0;
> + } else {
> + NEXT_ARG_FWD();
> + }
> + }
> + }
> +
> + if (argc) {
> + if (matches(*argv, "zone") == 0) {
> + NEXT_ARG();
> + if (get_u16(&zone, *argv, 10)) {
> + fprintf(stderr, "ctinfo: Illegal \"zone\"\n");
> + return -1;
> + }
> + argc--;
> + argv++;
> + }
> + }
> +
> + parse_action_control_dflt(&argc, &argv, &sel.action,
> + false, TC_ACT_PIPE);
> +
> + if (argc) {
> + if (matches(*argv, "index") == 0) {
> + NEXT_ARG();
> + if (get_u32(&sel.index, *argv, 10)) {
> + fprintf(stderr, "ctinfo: Illegal \"index\"\n");
> + return -1;
> + }
> + argc--;
> + argv++;
> + }
> + }
> +
> + tail = addattr_nest(n, MAX_MSG, tca_id);
> + addattr_l(n, MAX_MSG, TCA_CTINFO_ACT, &sel, sizeof(sel));
> + if (zone)
> + addattr16(n, MAX_MSG,
> + TCA_CTINFO_ZONE, zone);
> +
> + if (dscpmask)
> + addattr32(n, MAX_MSG,
> + TCA_CTINFO_PARMS_DSCP_MASK, dscpmask);
> +
> + if (dscpstatemask)
> + addattr32(n, MAX_MSG,
> + TCA_CTINFO_PARMS_DSCP_STATEMASK, dscpstatemask);
> +
> + if (cpmarkmask)
> + addattr32(n, MAX_MSG,
> + TCA_CTINFO_PARMS_CPMARK_MASK, cpmarkmask);
> +
> + addattr_nest_end(n, tail);
> +
> + *argc_p = argc;
> + *argv_p = argv;
> + return 0;
> +}
> +
> +static int print_ctinfo(struct action_util *au, FILE *f, struct rtattr *arg)
> +{
> + unsigned int cpmarkmask = ~0, dscpmask = 0, dscpstatemask = 0;
> + struct rtattr *tb[TCA_CTINFO_MAX + 1];
> + unsigned short zone = 0;
> + struct tc_ctinfo *ci;
> +
> + if (arg == NULL)
> + return -1;
> +
> + parse_rtattr_nested(tb, TCA_CTINFO_MAX, arg);
> + if (!tb[TCA_CTINFO_ACT]) {
> + print_string(PRINT_FP, NULL, "%s",
> + "[NULL ctinfo action parameters]");
> + return -1;
> + }
> +
> + ci = RTA_DATA(tb[TCA_CTINFO_ACT]);
> +
> + if (tb[TCA_CTINFO_PARMS_DSCP_MASK])
> + if (RTA_PAYLOAD(tb[TCA_CTINFO_PARMS_DSCP_MASK]) >=
> + sizeof(__u32))
> + dscpmask = rta_getattr_u32(tb[TCA_CTINFO_PARMS_DSCP_MASK]);
> + else
> + print_string(PRINT_FP, NULL, "%s",
> + "[invalid dscp mask parameter]");
> +
> + if (tb[TCA_CTINFO_PARMS_DSCP_STATEMASK])
> + if (RTA_PAYLOAD(tb[TCA_CTINFO_PARMS_DSCP_STATEMASK]) >=
> + sizeof(__u32))
> + dscpstatemask = rta_getattr_u32(tb[TCA_CTINFO_PARMS_DSCP_STATEMASK]);
> + else
> + print_string(PRINT_FP, NULL, "%s",
> + "[invalid dscp statemask parameter]");
> +
> + if (tb[TCA_CTINFO_PARMS_CPMARK_MASK])
> + if (RTA_PAYLOAD(tb[TCA_CTINFO_PARMS_CPMARK_MASK]) >=
> + sizeof(__u32))
> + cpmarkmask = rta_getattr_u32(tb[TCA_CTINFO_PARMS_CPMARK_MASK]);
> + else
> + print_string(PRINT_FP, NULL, "%s",
> + "[invalid cpmark mask parameter]");
> +
> + if (tb[TCA_CTINFO_ZONE] && RTA_PAYLOAD(tb[TCA_CTINFO_ZONE]) >=
> + sizeof(__u16))
> + zone = rta_getattr_u16(tb[TCA_CTINFO_ZONE]);
> +
> + print_string(PRINT_ANY, "kind", "%s ", "ctinfo");
> + print_hu(PRINT_ANY, "zone", "zone %u", zone);
> + print_action_control(f, " ", ci->action, "");
> +
> + print_string(PRINT_FP, NULL, "%s", _SL_);
> + print_uint(PRINT_ANY, "index", "\t index %u", ci->index);
> + print_int(PRINT_ANY, "ref", " ref %d", ci->refcnt);
> + print_int(PRINT_ANY, "bind", " bind %d", ci->bindcnt);
> +
> + if (tb[TCA_CTINFO_PARMS_DSCP_MASK]) {
> + print_0xhex(PRINT_ANY, "dscpmask", " dscp %#010llx", dscpmask);
> + print_0xhex(PRINT_ANY, "dscpstatemask", "/%#010llx",
> + dscpstatemask);
> + }
> +
> + if (tb[TCA_CTINFO_PARMS_CPMARK_MASK])
> + print_0xhex(PRINT_ANY, "mark", " mark %#010llx", cpmarkmask);
> +
> + if (show_stats) {
> + if (tb[TCA_CTINFO_TM]) {
> + struct tcf_t *tm = RTA_DATA(tb[TCA_CTINFO_TM]);
> +
> + print_tm(f, tm);
> + }
> +
> + if (tb[TCA_CTINFO_STATS_DSCP_SET])
> + print_lluint(PRINT_ANY, "dscpset", " DSCP set %llu",
> + rta_getattr_u64(tb[TCA_CTINFO_STATS_DSCP_SET]));
> + if (tb[TCA_CTINFO_STATS_DSCP_ERROR])
> + print_lluint(PRINT_ANY, "dscperror", " error %llu",
> + rta_getattr_u64(tb[TCA_CTINFO_STATS_DSCP_ERROR]));
> +
> + if (tb[TCA_CTINFO_STATS_CPMARK_SET])
> + print_lluint(PRINT_ANY, "cpmarkset", " CPMARK set %llu",
> + rta_getattr_u64(tb[TCA_CTINFO_STATS_CPMARK_SET]));
> + }
> + print_string(PRINT_FP, NULL, "%s", _SL_);
> +
> + return 0;
> +}
> +
> +struct action_util ctinfo_action_util = {
> + .id = "ctinfo",
> + .parse_aopt = parse_ctinfo,
> + .print_aopt = print_ctinfo,
> +};
> --
> 2.20.1 (Apple Git-117)
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH RFC iproute2-next v3] tc: add support for action act_ctinfo
2019-05-31 8:10 [PATCH RFC iproute2-next v2] tc: add support for act ctinfo ldir.kdb
2019-06-02 7:33 ` Kevin 'ldir' Darbyshire-Bryant
@ 2019-06-02 18:50 ` Kevin Darbyshire-Bryant
2019-06-02 20:39 ` Toke Høiland-Jørgensen
2019-06-03 13:50 ` [PATCH RFC iproute2-next v4] " Kevin Darbyshire-Bryant
1 sibling, 2 replies; 10+ messages in thread
From: Kevin Darbyshire-Bryant @ 2019-06-02 18:50 UTC (permalink / raw)
To: netdev; +Cc: ldir, stephen
ctinfo is an action restoring data stored in conntrack marks to various
fields. At present it has two independent modes of operation,
restoration of DSCP into IPv4/v6 diffserv and restoration of conntrack
marks into packet skb marks.
It understands a number of parameters specific to this action in
additional to the usual action syntax. Each operating mode is
independent of the other so all options are optional, however not
specifying at least one mode is a bit pointless.
Usage: ... ctinfo [dscp mask[/statemask]] [cpmark [mask]] [zone ZONE]
[CONTROL] [index <INDEX>]
DSCP mode
dscp enables copying of a DSCP store in the conntrack mark into the
ipv4/v6 diffserv field. The mask is a 32bit field and specifies where
in the conntrack mark the DSCP value is stored. It must be 6 contiguous
bits long, e.g. 0xfc000000 would restore the DSCP from the upper 6 bits
of the conntrack mark.
The DSCP copying may be optionally controlled by a statemask. The
statemask is a 32bit field, usually with a single bit set and must not
overlap the dscp mask. The DSCP restore operation will only take place
if the corresponding bit/s in conntrack mark yield a non zero result.
eg. dscp 0xfc000000/0x01000000 would retrieve the DSCP from the top 6
bits, whilst using bit 25 as a flag to do so. Bit 26 is unused in this
example.
CPMARK mode
cpmark enables copying of the conntrack mark to the packet skb mark. In
this mode it is completely equivalent to the existing act_connmark.
Additional functionality is provided by the optional mask parameter,
whereby the stored conntrack mark is logically anded with the cpmark
mask before being stored into skb mark. This allows shared usage of the
conntrack mark between applications.
eg. cpmark 0x00ffffff would restore only the lower 24 bits of the
conntrack mark, thus may be useful in the event that the upper 8 bits
are used by the DSCP function.
Usage: ... ctinfo [dscp mask[/statemask]] [cpmark [mask]] [zone ZONE]
[CONTROL] [index <INDEX>]
where :
dscp MASK is the bitmask to restore DSCP
STATEMASK is the bitmask to determine conditional restoring
cpmark MASK mask applied to restored packet mark
ZONE is the conntrack zone
CONTROL := reclassify | pipe | drop | continue | ok |
goto chain <CHAIN_INDEX>
Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
---
v2 - fix whitespace issue in pkt_cls
fix most warnings from checkpatch - some lines still over 80 chars
due to long TLV names.
v3 - fix some dangling else warnings.
refactor stats printing to please checkpatch.
send zone TLV even if default '0' zone.
now checkpatch clean even though I think some of the formatting
is horrible :-)
sending via google's smtp 'cos MS' exchange office365 appears
to mangle patches from git send-email.
include/uapi/linux/pkt_cls.h | 1 +
include/uapi/linux/tc_act/tc_ctinfo.h | 34 ++++
tc/Makefile | 1 +
tc/m_ctinfo.c | 262 ++++++++++++++++++++++++++
4 files changed, 298 insertions(+)
create mode 100644 include/uapi/linux/tc_act/tc_ctinfo.h
create mode 100644 tc/m_ctinfo.c
diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index 51a0496f..a93680fc 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -105,6 +105,7 @@ enum tca_id {
TCA_ID_IFE = TCA_ACT_IFE,
TCA_ID_SAMPLE = TCA_ACT_SAMPLE,
/* other actions go here */
+ TCA_ID_CTINFO,
__TCA_ID_MAX = 255
};
diff --git a/include/uapi/linux/tc_act/tc_ctinfo.h b/include/uapi/linux/tc_act/tc_ctinfo.h
new file mode 100644
index 00000000..da803e05
--- /dev/null
+++ b/include/uapi/linux/tc_act/tc_ctinfo.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef __UAPI_TC_CTINFO_H
+#define __UAPI_TC_CTINFO_H
+
+#include <linux/types.h>
+#include <linux/pkt_cls.h>
+
+struct tc_ctinfo {
+ tc_gen;
+};
+
+enum {
+ TCA_CTINFO_UNSPEC,
+ TCA_CTINFO_PAD,
+ TCA_CTINFO_TM,
+ TCA_CTINFO_ACT,
+ TCA_CTINFO_ZONE,
+ TCA_CTINFO_PARMS_DSCP_MASK,
+ TCA_CTINFO_PARMS_DSCP_STATEMASK,
+ TCA_CTINFO_PARMS_CPMARK_MASK,
+ TCA_CTINFO_STATS_DSCP_SET,
+ TCA_CTINFO_STATS_DSCP_ERROR,
+ TCA_CTINFO_STATS_CPMARK_SET,
+ __TCA_CTINFO_MAX
+};
+
+#define TCA_CTINFO_MAX (__TCA_CTINFO_MAX - 1)
+
+enum {
+ CTINFO_MODE_DSCP = BIT(0),
+ CTINFO_MODE_CPMARK = BIT(1)
+};
+
+#endif
diff --git a/tc/Makefile b/tc/Makefile
index 1a305cf4..60abddee 100644
--- a/tc/Makefile
+++ b/tc/Makefile
@@ -48,6 +48,7 @@ TCMODULES += m_csum.o
TCMODULES += m_simple.o
TCMODULES += m_vlan.o
TCMODULES += m_connmark.o
+TCMODULES += m_ctinfo.o
TCMODULES += m_bpf.o
TCMODULES += m_tunnel_key.o
TCMODULES += m_sample.o
diff --git a/tc/m_ctinfo.c b/tc/m_ctinfo.c
new file mode 100644
index 00000000..af5102bf
--- /dev/null
+++ b/tc/m_ctinfo.c
@@ -0,0 +1,262 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * m_ctinfo.c netfilter ctinfo mark action
+ *
+ * Copyright (c) 2019 Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <string.h>
+#include "utils.h"
+#include "tc_util.h"
+#include <linux/tc_act/tc_ctinfo.h>
+
+static void
+explain(void)
+{
+ fprintf(stderr,
+ "Usage: ... ctinfo [dscp mask[/statemask]] [cpmark [mask]] [zone ZONE] [CONTROL] [index <INDEX>]\n"
+ "where :\n"
+ "\tdscp MASK bitmask location of stored DSCP\n"
+ "\t STATEMASK bitmask to determine conditional restoring\n"
+ "\tcpmark MASK mask applied to mark on restoration\n"
+ "\tZONE is the conntrack zone\n"
+ "\tCONTROL := reclassify | pipe | drop | continue | ok |\n"
+ "\t goto chain <CHAIN_INDEX>\n");
+}
+
+static void
+usage(void)
+{
+ explain();
+ exit(-1);
+}
+
+static int
+parse_ctinfo(struct action_util *a, int *argc_p, char ***argv_p, int tca_id,
+ struct nlmsghdr *n)
+{
+ unsigned int cpmarkmask = 0, dscpmask = 0, dscpstatemask = 0;
+ struct tc_ctinfo sel = {};
+ unsigned short zone = 0;
+ char **argv = *argv_p;
+ struct rtattr *tail;
+ int argc = *argc_p;
+ int ok = 0;
+
+ while (argc > 0) {
+ if (matches(*argv, "ctinfo") == 0) {
+ ok = 1;
+ argc--;
+ argv++;
+ } else if (matches(*argv, "help") == 0) {
+ usage();
+ } else {
+ break;
+ }
+
+ }
+
+ if (!ok) {
+ explain();
+ return -1;
+ }
+
+ if (argc) {
+ if (matches(*argv, "dscp") == 0) {
+ NEXT_ARG();
+ char *slash;
+
+ slash = strchr(*argv, '/');
+ if (slash)
+ *slash = '\0';
+ if (get_u32(&dscpmask, *argv, 0)) {
+ fprintf(stderr,
+ "ctinfo: Illegal dscp \"mask\"\n");
+ return -1;
+ }
+ if (slash && get_u32(&dscpstatemask, slash + 1, 0)) {
+ fprintf(stderr,
+ "ctinfo: Illegal dscp \"statemask\"\n");
+ return -1;
+ }
+ argc--;
+ argv++;
+ }
+ }
+
+ /* cpmark has optional mask parameter, so the next arg might not */
+ /* exist, or it might be the next option, or it may actually be a */
+ /* 32bit mask */
+ if (argc) {
+ if (matches(*argv, "cpmark") == 0) {
+ if (NEXT_ARG_OK()) {
+ NEXT_ARG_FWD();
+ if (!get_u32(&cpmarkmask, *argv, 0))
+ NEXT_ARG_FWD(); /* was a mask */
+ else /* not a mask, use default */
+ cpmarkmask = ~0;
+ } else {
+ NEXT_ARG_FWD();
+ }
+ }
+ }
+
+ if (argc) {
+ if (matches(*argv, "zone") == 0) {
+ NEXT_ARG();
+ if (get_u16(&zone, *argv, 10)) {
+ fprintf(stderr, "ctinfo: Illegal \"zone\"\n");
+ return -1;
+ }
+ argc--;
+ argv++;
+ }
+ }
+
+ parse_action_control_dflt(&argc, &argv, &sel.action,
+ false, TC_ACT_PIPE);
+
+ if (argc) {
+ if (matches(*argv, "index") == 0) {
+ NEXT_ARG();
+ if (get_u32(&sel.index, *argv, 10)) {
+ fprintf(stderr, "ctinfo: Illegal \"index\"\n");
+ return -1;
+ }
+ argc--;
+ argv++;
+ }
+ }
+
+ tail = addattr_nest(n, MAX_MSG, tca_id);
+ addattr_l(n, MAX_MSG, TCA_CTINFO_ACT, &sel, sizeof(sel));
+ addattr16(n, MAX_MSG, TCA_CTINFO_ZONE, zone);
+
+ if (dscpmask)
+ addattr32(n, MAX_MSG,
+ TCA_CTINFO_PARMS_DSCP_MASK, dscpmask);
+
+ if (dscpstatemask)
+ addattr32(n, MAX_MSG,
+ TCA_CTINFO_PARMS_DSCP_STATEMASK, dscpstatemask);
+
+ if (cpmarkmask)
+ addattr32(n, MAX_MSG,
+ TCA_CTINFO_PARMS_CPMARK_MASK, cpmarkmask);
+
+ addattr_nest_end(n, tail);
+
+ *argc_p = argc;
+ *argv_p = argv;
+ return 0;
+}
+
+static void print_ctinfo_stats(FILE *f, struct rtattr *tb[TCA_CTINFO_MAX + 1])
+{
+ struct tcf_t *tm;
+
+ if (tb[TCA_CTINFO_TM]) {
+ tm = RTA_DATA(tb[TCA_CTINFO_TM]);
+
+ print_tm(f, tm);
+ }
+
+ if (tb[TCA_CTINFO_STATS_DSCP_SET])
+ print_lluint(PRINT_ANY, "dscpset", " DSCP set %llu",
+ rta_getattr_u64(tb[TCA_CTINFO_STATS_DSCP_SET]));
+ if (tb[TCA_CTINFO_STATS_DSCP_ERROR])
+ print_lluint(PRINT_ANY, "dscperror", " error %llu",
+ rta_getattr_u64(tb[TCA_CTINFO_STATS_DSCP_ERROR]));
+
+ if (tb[TCA_CTINFO_STATS_CPMARK_SET])
+ print_lluint(PRINT_ANY, "cpmarkset", " CPMARK set %llu",
+ rta_getattr_u64(tb[TCA_CTINFO_STATS_CPMARK_SET]));
+}
+
+static int print_ctinfo(struct action_util *au, FILE *f, struct rtattr *arg)
+{
+ unsigned int cpmarkmask = ~0, dscpmask = 0, dscpstatemask = 0;
+ struct rtattr *tb[TCA_CTINFO_MAX + 1];
+ unsigned short zone = 0;
+ struct tc_ctinfo *ci;
+
+ if (arg == NULL)
+ return -1;
+
+ parse_rtattr_nested(tb, TCA_CTINFO_MAX, arg);
+ if (!tb[TCA_CTINFO_ACT]) {
+ print_string(PRINT_FP, NULL, "%s",
+ "[NULL ctinfo action parameters]");
+ return -1;
+ }
+
+ ci = RTA_DATA(tb[TCA_CTINFO_ACT]);
+
+ if (tb[TCA_CTINFO_PARMS_DSCP_MASK]) {
+ if (RTA_PAYLOAD(tb[TCA_CTINFO_PARMS_DSCP_MASK]) >=
+ sizeof(__u32))
+ dscpmask = rta_getattr_u32(
+ tb[TCA_CTINFO_PARMS_DSCP_MASK]);
+ else
+ print_string(PRINT_FP, NULL, "%s",
+ "[invalid dscp mask parameter]");
+ }
+
+ if (tb[TCA_CTINFO_PARMS_DSCP_STATEMASK]) {
+ if (RTA_PAYLOAD(tb[TCA_CTINFO_PARMS_DSCP_STATEMASK]) >=
+ sizeof(__u32))
+ dscpstatemask = rta_getattr_u32(
+ tb[TCA_CTINFO_PARMS_DSCP_STATEMASK]);
+ else
+ print_string(PRINT_FP, NULL, "%s",
+ "[invalid dscp statemask parameter]");
+ }
+
+ if (tb[TCA_CTINFO_PARMS_CPMARK_MASK]) {
+ if (RTA_PAYLOAD(tb[TCA_CTINFO_PARMS_CPMARK_MASK]) >=
+ sizeof(__u32))
+ cpmarkmask = rta_getattr_u32(
+ tb[TCA_CTINFO_PARMS_CPMARK_MASK]);
+ else
+ print_string(PRINT_FP, NULL, "%s",
+ "[invalid cpmark mask parameter]");
+ }
+
+ if (tb[TCA_CTINFO_ZONE] && RTA_PAYLOAD(tb[TCA_CTINFO_ZONE]) >=
+ sizeof(__u16))
+ zone = rta_getattr_u16(tb[TCA_CTINFO_ZONE]);
+
+ print_string(PRINT_ANY, "kind", "%s ", "ctinfo");
+ print_hu(PRINT_ANY, "zone", "zone %u", zone);
+ print_action_control(f, " ", ci->action, "");
+
+ print_string(PRINT_FP, NULL, "%s", _SL_);
+ print_uint(PRINT_ANY, "index", "\t index %u", ci->index);
+ print_int(PRINT_ANY, "ref", " ref %d", ci->refcnt);
+ print_int(PRINT_ANY, "bind", " bind %d", ci->bindcnt);
+
+ if (tb[TCA_CTINFO_PARMS_DSCP_MASK]) {
+ print_0xhex(PRINT_ANY, "dscpmask", " dscp %#010llx", dscpmask);
+ print_0xhex(PRINT_ANY, "dscpstatemask", "/%#010llx",
+ dscpstatemask);
+ }
+
+ if (tb[TCA_CTINFO_PARMS_CPMARK_MASK])
+ print_0xhex(PRINT_ANY, "mark", " mark %#010llx", cpmarkmask);
+
+ if (show_stats)
+ print_ctinfo_stats(f, tb);
+
+ print_string(PRINT_FP, NULL, "%s", _SL_);
+
+ return 0;
+}
+
+struct action_util ctinfo_action_util = {
+ .id = "ctinfo",
+ .parse_aopt = parse_ctinfo,
+ .print_aopt = print_ctinfo,
+};
--
2.20.1 (Apple Git-117)
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH RFC iproute2-next v3] tc: add support for action act_ctinfo
2019-06-02 18:50 ` [PATCH RFC iproute2-next v3] tc: add support for action act_ctinfo Kevin Darbyshire-Bryant
@ 2019-06-02 20:39 ` Toke Høiland-Jørgensen
2019-06-02 21:55 ` Kevin 'ldir' Darbyshire-Bryant
2019-06-03 13:50 ` [PATCH RFC iproute2-next v4] " Kevin Darbyshire-Bryant
1 sibling, 1 reply; 10+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-06-02 20:39 UTC (permalink / raw)
To: Kevin Darbyshire-Bryant, netdev; +Cc: ldir, stephen
Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk> writes:
> ctinfo is an action restoring data stored in conntrack marks to various
> fields. At present it has two independent modes of operation,
> restoration of DSCP into IPv4/v6 diffserv and restoration of conntrack
> marks into packet skb marks.
>
> It understands a number of parameters specific to this action in
> additional to the usual action syntax. Each operating mode is
> independent of the other so all options are optional, however not
> specifying at least one mode is a bit pointless.
>
> Usage: ... ctinfo [dscp mask[/statemask]] [cpmark [mask]] [zone ZONE]
> [CONTROL] [index <INDEX>]
Yay, bikeshedding time! :)
As I said in reply to the kernel patch, the "X/Y" syntax usually means
"<value>/<mask applied to value>", where here they are just two
semi-related mask values. So I think it would be better to just make
'statemask' its own parameter.
Other than that, just a few nits, below...
> DSCP mode
>
> dscp enables copying of a DSCP store in the conntrack mark into the
> ipv4/v6 diffserv field. The mask is a 32bit field and specifies where
> in the conntrack mark the DSCP value is stored. It must be 6 contiguous
> bits long, e.g. 0xfc000000 would restore the DSCP from the upper 6 bits
> of the conntrack mark.
>
> The DSCP copying may be optionally controlled by a statemask. The
> statemask is a 32bit field, usually with a single bit set and must not
> overlap the dscp mask. The DSCP restore operation will only take place
> if the corresponding bit/s in conntrack mark yield a non zero result.
>
> eg. dscp 0xfc000000/0x01000000 would retrieve the DSCP from the top 6
> bits, whilst using bit 25 as a flag to do so. Bit 26 is unused in this
> example.
>
> CPMARK mode
>
> cpmark enables copying of the conntrack mark to the packet skb mark. In
> this mode it is completely equivalent to the existing act_connmark.
> Additional functionality is provided by the optional mask parameter,
> whereby the stored conntrack mark is logically anded with the cpmark
> mask before being stored into skb mark. This allows shared usage of the
> conntrack mark between applications.
>
> eg. cpmark 0x00ffffff would restore only the lower 24 bits of the
> conntrack mark, thus may be useful in the event that the upper 8 bits
> are used by the DSCP function.
>
> Usage: ... ctinfo [dscp mask[/statemask]] [cpmark [mask]] [zone ZONE]
> [CONTROL] [index <INDEX>]
> where :
> dscp MASK is the bitmask to restore DSCP
> STATEMASK is the bitmask to determine conditional restoring
> cpmark MASK mask applied to restored packet mark
> ZONE is the conntrack zone
> CONTROL := reclassify | pipe | drop | continue | ok |
> goto chain <CHAIN_INDEX>
>
> Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
>
> ---
> v2 - fix whitespace issue in pkt_cls
> fix most warnings from checkpatch - some lines still over 80 chars
> due to long TLV names.
> v3 - fix some dangling else warnings.
> refactor stats printing to please checkpatch.
> send zone TLV even if default '0' zone.
> now checkpatch clean even though I think some of the formatting
> is horrible :-)
> sending via google's smtp 'cos MS' exchange office365 appears
> to mangle patches from git send-email.
Ah, so it wasn't just me having problems ;)
> include/uapi/linux/pkt_cls.h | 1 +
> include/uapi/linux/tc_act/tc_ctinfo.h | 34 ++++
> tc/Makefile | 1 +
> tc/m_ctinfo.c | 262 ++++++++++++++++++++++++++
> 4 files changed, 298 insertions(+)
> create mode 100644 include/uapi/linux/tc_act/tc_ctinfo.h
> create mode 100644 tc/m_ctinfo.c
>
> diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
> index 51a0496f..a93680fc 100644
> --- a/include/uapi/linux/pkt_cls.h
> +++ b/include/uapi/linux/pkt_cls.h
> @@ -105,6 +105,7 @@ enum tca_id {
> TCA_ID_IFE = TCA_ACT_IFE,
> TCA_ID_SAMPLE = TCA_ACT_SAMPLE,
> /* other actions go here */
> + TCA_ID_CTINFO,
> __TCA_ID_MAX = 255
> };
>
> diff --git a/include/uapi/linux/tc_act/tc_ctinfo.h b/include/uapi/linux/tc_act/tc_ctinfo.h
> new file mode 100644
> index 00000000..da803e05
> --- /dev/null
> +++ b/include/uapi/linux/tc_act/tc_ctinfo.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +#ifndef __UAPI_TC_CTINFO_H
> +#define __UAPI_TC_CTINFO_H
> +
> +#include <linux/types.h>
> +#include <linux/pkt_cls.h>
> +
> +struct tc_ctinfo {
> + tc_gen;
> +};
> +
> +enum {
> + TCA_CTINFO_UNSPEC,
> + TCA_CTINFO_PAD,
> + TCA_CTINFO_TM,
> + TCA_CTINFO_ACT,
> + TCA_CTINFO_ZONE,
> + TCA_CTINFO_PARMS_DSCP_MASK,
> + TCA_CTINFO_PARMS_DSCP_STATEMASK,
> + TCA_CTINFO_PARMS_CPMARK_MASK,
> + TCA_CTINFO_STATS_DSCP_SET,
> + TCA_CTINFO_STATS_DSCP_ERROR,
> + TCA_CTINFO_STATS_CPMARK_SET,
> + __TCA_CTINFO_MAX
> +};
> +
> +#define TCA_CTINFO_MAX (__TCA_CTINFO_MAX - 1)
> +
> +enum {
> + CTINFO_MODE_DSCP = BIT(0),
> + CTINFO_MODE_CPMARK = BIT(1)
> +};
> +
> +#endif
> diff --git a/tc/Makefile b/tc/Makefile
> index 1a305cf4..60abddee 100644
> --- a/tc/Makefile
> +++ b/tc/Makefile
> @@ -48,6 +48,7 @@ TCMODULES += m_csum.o
> TCMODULES += m_simple.o
> TCMODULES += m_vlan.o
> TCMODULES += m_connmark.o
> +TCMODULES += m_ctinfo.o
> TCMODULES += m_bpf.o
> TCMODULES += m_tunnel_key.o
> TCMODULES += m_sample.o
> diff --git a/tc/m_ctinfo.c b/tc/m_ctinfo.c
> new file mode 100644
> index 00000000..af5102bf
> --- /dev/null
> +++ b/tc/m_ctinfo.c
> @@ -0,0 +1,262 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * m_ctinfo.c netfilter ctinfo mark action
> + *
> + * Copyright (c) 2019 Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <string.h>
> +#include "utils.h"
> +#include "tc_util.h"
> +#include <linux/tc_act/tc_ctinfo.h>
> +
> +static void
> +explain(void)
> +{
> + fprintf(stderr,
> + "Usage: ... ctinfo [dscp mask[/statemask]] [cpmark [mask]] [zone ZONE] [CONTROL] [index <INDEX>]\n"
> + "where :\n"
> + "\tdscp MASK bitmask location of stored DSCP\n"
> + "\t STATEMASK bitmask to determine conditional restoring\n"
> + "\tcpmark MASK mask applied to mark on restoration\n"
> + "\tZONE is the conntrack zone\n"
> + "\tCONTROL := reclassify | pipe | drop | continue | ok |\n"
> + "\t goto chain <CHAIN_INDEX>\n");
> +}
> +
> +static void
> +usage(void)
> +{
> + explain();
> + exit(-1);
> +}
> +
> +static int
> +parse_ctinfo(struct action_util *a, int *argc_p, char ***argv_p, int tca_id,
> + struct nlmsghdr *n)
> +{
> + unsigned int cpmarkmask = 0, dscpmask = 0, dscpstatemask = 0;
> + struct tc_ctinfo sel = {};
> + unsigned short zone = 0;
> + char **argv = *argv_p;
> + struct rtattr *tail;
> + int argc = *argc_p;
> + int ok = 0;
> +
> + while (argc > 0) {
> + if (matches(*argv, "ctinfo") == 0) {
> + ok = 1;
> + argc--;
> + argv++;
> + } else if (matches(*argv, "help") == 0) {
> + usage();
> + } else {
> + break;
> + }
> +
> + }
> +
> + if (!ok) {
> + explain();
> + return -1;
> + }
> +
> + if (argc) {
> + if (matches(*argv, "dscp") == 0) {
> + NEXT_ARG();
> + char *slash;
> +
> + slash = strchr(*argv, '/');
> + if (slash)
> + *slash = '\0';
> + if (get_u32(&dscpmask, *argv, 0)) {
> + fprintf(stderr,
> + "ctinfo: Illegal dscp \"mask\"\n");
> + return -1;
> + }
Not strictly necessary, but would it be nicer to the user to do the
'must be exactly 6 bits' test here as well, in order to provide a
friendlier error message than just 'invalid'?
Could also just add this via extack on the kernel side, of course...
> + if (slash && get_u32(&dscpstatemask, slash + 1, 0)) {
> + fprintf(stderr,
> + "ctinfo: Illegal dscp \"statemask\"\n");
> + return -1;
> + }
> + argc--;
> + argv++;
> + }
> + }
> +
> + /* cpmark has optional mask parameter, so the next arg might not */
> + /* exist, or it might be the next option, or it may actually be a */
> + /* 32bit mask */
> + if (argc) {
> + if (matches(*argv, "cpmark") == 0) {
> + if (NEXT_ARG_OK()) {
> + NEXT_ARG_FWD();
> + if (!get_u32(&cpmarkmask, *argv, 0))
> + NEXT_ARG_FWD(); /* was a mask */
> + else /* not a mask, use default */
> + cpmarkmask = ~0;
> + } else {
> + NEXT_ARG_FWD();
> + }
> + }
> + }
> +
> + if (argc) {
> + if (matches(*argv, "zone") == 0) {
> + NEXT_ARG();
> + if (get_u16(&zone, *argv, 10)) {
> + fprintf(stderr, "ctinfo: Illegal \"zone\"\n");
> + return -1;
> + }
> + argc--;
> + argv++;
> + }
> + }
> +
> + parse_action_control_dflt(&argc, &argv, &sel.action,
> + false, TC_ACT_PIPE);
> +
> + if (argc) {
> + if (matches(*argv, "index") == 0) {
> + NEXT_ARG();
> + if (get_u32(&sel.index, *argv, 10)) {
> + fprintf(stderr, "ctinfo: Illegal \"index\"\n");
> + return -1;
> + }
> + argc--;
> + argv++;
> + }
> + }
> +
> + tail = addattr_nest(n, MAX_MSG, tca_id);
> + addattr_l(n, MAX_MSG, TCA_CTINFO_ACT, &sel, sizeof(sel));
> + addattr16(n, MAX_MSG, TCA_CTINFO_ZONE, zone);
> +
> + if (dscpmask)
> + addattr32(n, MAX_MSG,
> + TCA_CTINFO_PARMS_DSCP_MASK, dscpmask);
> +
> + if (dscpstatemask)
> + addattr32(n, MAX_MSG,
> + TCA_CTINFO_PARMS_DSCP_STATEMASK, dscpstatemask);
> +
> + if (cpmarkmask)
> + addattr32(n, MAX_MSG,
> + TCA_CTINFO_PARMS_CPMARK_MASK, cpmarkmask);
> +
> + addattr_nest_end(n, tail);
> +
> + *argc_p = argc;
> + *argv_p = argv;
> + return 0;
> +}
> +
> +static void print_ctinfo_stats(FILE *f, struct rtattr *tb[TCA_CTINFO_MAX + 1])
> +{
> + struct tcf_t *tm;
> +
> + if (tb[TCA_CTINFO_TM]) {
> + tm = RTA_DATA(tb[TCA_CTINFO_TM]);
> +
> + print_tm(f, tm);
> + }
> +
> + if (tb[TCA_CTINFO_STATS_DSCP_SET])
> + print_lluint(PRINT_ANY, "dscpset", " DSCP set %llu",
> + rta_getattr_u64(tb[TCA_CTINFO_STATS_DSCP_SET]));
> + if (tb[TCA_CTINFO_STATS_DSCP_ERROR])
> + print_lluint(PRINT_ANY, "dscperror", " error %llu",
> + rta_getattr_u64(tb[TCA_CTINFO_STATS_DSCP_ERROR]));
> +
> + if (tb[TCA_CTINFO_STATS_CPMARK_SET])
> + print_lluint(PRINT_ANY, "cpmarkset", " CPMARK set %llu",
> + rta_getattr_u64(tb[TCA_CTINFO_STATS_CPMARK_SET]));
> +}
> +
> +static int print_ctinfo(struct action_util *au, FILE *f, struct rtattr *arg)
> +{
> + unsigned int cpmarkmask = ~0, dscpmask = 0, dscpstatemask = 0;
> + struct rtattr *tb[TCA_CTINFO_MAX + 1];
> + unsigned short zone = 0;
> + struct tc_ctinfo *ci;
> +
> + if (arg == NULL)
> + return -1;
> +
> + parse_rtattr_nested(tb, TCA_CTINFO_MAX, arg);
> + if (!tb[TCA_CTINFO_ACT]) {
> + print_string(PRINT_FP, NULL, "%s",
> + "[NULL ctinfo action parameters]");
> + return -1;
> + }
> +
> + ci = RTA_DATA(tb[TCA_CTINFO_ACT]);
> +
> + if (tb[TCA_CTINFO_PARMS_DSCP_MASK]) {
> + if (RTA_PAYLOAD(tb[TCA_CTINFO_PARMS_DSCP_MASK]) >=
> + sizeof(__u32))
> + dscpmask = rta_getattr_u32(
> + tb[TCA_CTINFO_PARMS_DSCP_MASK]);
> + else
> + print_string(PRINT_FP, NULL, "%s",
> + "[invalid dscp mask parameter]");
> + }
> +
> + if (tb[TCA_CTINFO_PARMS_DSCP_STATEMASK]) {
> + if (RTA_PAYLOAD(tb[TCA_CTINFO_PARMS_DSCP_STATEMASK]) >=
> + sizeof(__u32))
> + dscpstatemask = rta_getattr_u32(
> + tb[TCA_CTINFO_PARMS_DSCP_STATEMASK]);
> + else
> + print_string(PRINT_FP, NULL, "%s",
> + "[invalid dscp statemask parameter]");
> + }
> +
> + if (tb[TCA_CTINFO_PARMS_CPMARK_MASK]) {
> + if (RTA_PAYLOAD(tb[TCA_CTINFO_PARMS_CPMARK_MASK]) >=
> + sizeof(__u32))
> + cpmarkmask = rta_getattr_u32(
> + tb[TCA_CTINFO_PARMS_CPMARK_MASK]);
> + else
> + print_string(PRINT_FP, NULL, "%s",
> + "[invalid cpmark mask parameter]");
> + }
> +
> + if (tb[TCA_CTINFO_ZONE] && RTA_PAYLOAD(tb[TCA_CTINFO_ZONE]) >=
> + sizeof(__u16))
> + zone = rta_getattr_u16(tb[TCA_CTINFO_ZONE]);
> +
> + print_string(PRINT_ANY, "kind", "%s ", "ctinfo");
> + print_hu(PRINT_ANY, "zone", "zone %u", zone);
> + print_action_control(f, " ", ci->action, "");
> +
> + print_string(PRINT_FP, NULL, "%s", _SL_);
> + print_uint(PRINT_ANY, "index", "\t index %u", ci->index);
> + print_int(PRINT_ANY, "ref", " ref %d", ci->refcnt);
> + print_int(PRINT_ANY, "bind", " bind %d", ci->bindcnt);
> +
> + if (tb[TCA_CTINFO_PARMS_DSCP_MASK]) {
> + print_0xhex(PRINT_ANY, "dscpmask", " dscp %#010llx", dscpmask);
> + print_0xhex(PRINT_ANY, "dscpstatemask", "/%#010llx",
> + dscpstatemask);
> + }
> +
> + if (tb[TCA_CTINFO_PARMS_CPMARK_MASK])
> + print_0xhex(PRINT_ANY, "mark", " mark %#010llx",
> cpmarkmask);
Should be 'cpmark' not 'mark', no?
> +
> + if (show_stats)
> + print_ctinfo_stats(f, tb);
> +
> + print_string(PRINT_FP, NULL, "%s", _SL_);
> +
> + return 0;
> +}
> +
> +struct action_util ctinfo_action_util = {
> + .id = "ctinfo",
> + .parse_aopt = parse_ctinfo,
> + .print_aopt = print_ctinfo,
> +};
> --
> 2.20.1 (Apple Git-117)
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC iproute2-next v3] tc: add support for action act_ctinfo
2019-06-02 20:39 ` Toke Høiland-Jørgensen
@ 2019-06-02 21:55 ` Kevin 'ldir' Darbyshire-Bryant
2019-06-03 9:15 ` Toke Høiland-Jørgensen
0 siblings, 1 reply; 10+ messages in thread
From: Kevin 'ldir' Darbyshire-Bryant @ 2019-06-02 21:55 UTC (permalink / raw)
To: Toke Høiland-Jørgensen; +Cc: netdev, stephen
> On 2 Jun 2019, at 21:39, Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk> writes:
>
>> ctinfo is an action restoring data stored in conntrack marks to various
>> fields. At present it has two independent modes of operation,
>> restoration of DSCP into IPv4/v6 diffserv and restoration of conntrack
>> marks into packet skb marks.
>>
>> It understands a number of parameters specific to this action in
>> additional to the usual action syntax. Each operating mode is
>> independent of the other so all options are optional, however not
>> specifying at least one mode is a bit pointless.
>>
>> Usage: ... ctinfo [dscp mask[/statemask]] [cpmark [mask]] [zone ZONE]
>> [CONTROL] [index <INDEX>]
>
> Yay, bikeshedding time! :)
I see your bikeshed and raise you… a bus shelter :-)
>
> As I said in reply to the kernel patch, the "X/Y" syntax usually means
> "<value>/<mask applied to value>", where here they are just two
> semi-related mask values. So I think it would be better to just make
> 'statemask' its own parameter.
Instead of creating another keyword how about we drop the ‘/‘ and
make it a space separated optional parameter to ‘dscp’? eg.
Usage: ... ctinfo [dscp mask [statemask]] [cpmark [mask]] blah blah
> Other than that, just a few nits, below...
>
>> DSCP mode
>>
>> dscp enables copying of a DSCP store in the conntrack mark into the
>> ipv4/v6 diffserv field. The mask is a 32bit field and specifies where
>> in the conntrack mark the DSCP value is stored. It must be 6 contiguous
>> bits long, e.g. 0xfc000000 would restore the DSCP from the upper 6 bits
>> of the conntrack mark.
>>
>> The DSCP copying may be optionally controlled by a statemask. The
>> statemask is a 32bit field, usually with a single bit set and must not
>> overlap the dscp mask. The DSCP restore operation will only take place
>> if the corresponding bit/s in conntrack mark yield a non zero result.
>>
>> eg. dscp 0xfc000000/0x01000000 would retrieve the DSCP from the top 6
>> bits, whilst using bit 25 as a flag to do so. Bit 26 is unused in this
>> example.
>>
>> CPMARK mode
>>
>> cpmark enables copying of the conntrack mark to the packet skb mark. In
>> this mode it is completely equivalent to the existing act_connmark.
>> Additional functionality is provided by the optional mask parameter,
>> whereby the stored conntrack mark is logically anded with the cpmark
>> mask before being stored into skb mark. This allows shared usage of the
>> conntrack mark between applications.
>>
>> eg. cpmark 0x00ffffff would restore only the lower 24 bits of the
>> conntrack mark, thus may be useful in the event that the upper 8 bits
>> are used by the DSCP function.
>>
>> Usage: ... ctinfo [dscp mask[/statemask]] [cpmark [mask]] [zone ZONE]
>> [CONTROL] [index <INDEX>]
>> where :
>> dscp MASK is the bitmask to restore DSCP
>> STATEMASK is the bitmask to determine conditional restoring
>> cpmark MASK mask applied to restored packet mark
>> ZONE is the conntrack zone
>> CONTROL := reclassify | pipe | drop | continue | ok |
>> goto chain <CHAIN_INDEX>
>>
>> Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
>>
>> ---
>> v2 - fix whitespace issue in pkt_cls
>> fix most warnings from checkpatch - some lines still over 80 chars
>> due to long TLV names.
>> v3 - fix some dangling else warnings.
>> refactor stats printing to please checkpatch.
>> send zone TLV even if default '0' zone.
>> now checkpatch clean even though I think some of the formatting
>> is horrible :-)
>> sending via google's smtp 'cos MS' exchange office365 appears
>> to mangle patches from git send-email.
>
> Ah, so it wasn't just me having problems ;)
No, though I’m still not clear what’s going on or when Microsoft
improved(tm) it :-/
>
>> include/uapi/linux/pkt_cls.h | 1 +
>> include/uapi/linux/tc_act/tc_ctinfo.h | 34 ++++
>> tc/Makefile | 1 +
>> tc/m_ctinfo.c | 262 ++++++++++++++++++++++++++
>> 4 files changed, 298 insertions(+)
>> create mode 100644 include/uapi/linux/tc_act/tc_ctinfo.h
>> create mode 100644 tc/m_ctinfo.c
>>
>> diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
>> index 51a0496f..a93680fc 100644
>> --- a/include/uapi/linux/pkt_cls.h
>> +++ b/include/uapi/linux/pkt_cls.h
>> @@ -105,6 +105,7 @@ enum tca_id {
>> TCA_ID_IFE = TCA_ACT_IFE,
>> TCA_ID_SAMPLE = TCA_ACT_SAMPLE,
>> /* other actions go here */
>> + TCA_ID_CTINFO,
>> __TCA_ID_MAX = 255
>> };
>>
>> diff --git a/include/uapi/linux/tc_act/tc_ctinfo.h b/include/uapi/linux/tc_act/tc_ctinfo.h
>> new file mode 100644
>> index 00000000..da803e05
>> --- /dev/null
>> +++ b/include/uapi/linux/tc_act/tc_ctinfo.h
>> @@ -0,0 +1,34 @@
>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>> +#ifndef __UAPI_TC_CTINFO_H
>> +#define __UAPI_TC_CTINFO_H
>> +
>> +#include <linux/types.h>
>> +#include <linux/pkt_cls.h>
>> +
>> +struct tc_ctinfo {
>> + tc_gen;
>> +};
>> +
>> +enum {
>> + TCA_CTINFO_UNSPEC,
>> + TCA_CTINFO_PAD,
>> + TCA_CTINFO_TM,
>> + TCA_CTINFO_ACT,
>> + TCA_CTINFO_ZONE,
>> + TCA_CTINFO_PARMS_DSCP_MASK,
>> + TCA_CTINFO_PARMS_DSCP_STATEMASK,
>> + TCA_CTINFO_PARMS_CPMARK_MASK,
>> + TCA_CTINFO_STATS_DSCP_SET,
>> + TCA_CTINFO_STATS_DSCP_ERROR,
>> + TCA_CTINFO_STATS_CPMARK_SET,
>> + __TCA_CTINFO_MAX
>> +};
>> +
>> +#define TCA_CTINFO_MAX (__TCA_CTINFO_MAX - 1)
>> +
>> +enum {
>> + CTINFO_MODE_DSCP = BIT(0),
>> + CTINFO_MODE_CPMARK = BIT(1)
>> +};
>> +
>> +#endif
>> diff --git a/tc/Makefile b/tc/Makefile
>> index 1a305cf4..60abddee 100644
>> --- a/tc/Makefile
>> +++ b/tc/Makefile
>> @@ -48,6 +48,7 @@ TCMODULES += m_csum.o
>> TCMODULES += m_simple.o
>> TCMODULES += m_vlan.o
>> TCMODULES += m_connmark.o
>> +TCMODULES += m_ctinfo.o
>> TCMODULES += m_bpf.o
>> TCMODULES += m_tunnel_key.o
>> TCMODULES += m_sample.o
>> diff --git a/tc/m_ctinfo.c b/tc/m_ctinfo.c
>> new file mode 100644
>> index 00000000..af5102bf
>> --- /dev/null
>> +++ b/tc/m_ctinfo.c
>> @@ -0,0 +1,262 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * m_ctinfo.c netfilter ctinfo mark action
>> + *
>> + * Copyright (c) 2019 Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
>> + */
>> +
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <unistd.h>
>> +#include <string.h>
>> +#include "utils.h"
>> +#include "tc_util.h"
>> +#include <linux/tc_act/tc_ctinfo.h>
>> +
>> +static void
>> +explain(void)
>> +{
>> + fprintf(stderr,
>> + "Usage: ... ctinfo [dscp mask[/statemask]] [cpmark [mask]] [zone ZONE] [CONTROL] [index <INDEX>]\n"
>> + "where :\n"
>> + "\tdscp MASK bitmask location of stored DSCP\n"
>> + "\t STATEMASK bitmask to determine conditional restoring\n"
>> + "\tcpmark MASK mask applied to mark on restoration\n"
>> + "\tZONE is the conntrack zone\n"
>> + "\tCONTROL := reclassify | pipe | drop | continue | ok |\n"
>> + "\t goto chain <CHAIN_INDEX>\n");
>> +}
>> +
>> +static void
>> +usage(void)
>> +{
>> + explain();
>> + exit(-1);
>> +}
>> +
>> +static int
>> +parse_ctinfo(struct action_util *a, int *argc_p, char ***argv_p, int tca_id,
>> + struct nlmsghdr *n)
>> +{
>> + unsigned int cpmarkmask = 0, dscpmask = 0, dscpstatemask = 0;
>> + struct tc_ctinfo sel = {};
>> + unsigned short zone = 0;
>> + char **argv = *argv_p;
>> + struct rtattr *tail;
>> + int argc = *argc_p;
>> + int ok = 0;
>> +
>> + while (argc > 0) {
>> + if (matches(*argv, "ctinfo") == 0) {
>> + ok = 1;
>> + argc--;
>> + argv++;
>> + } else if (matches(*argv, "help") == 0) {
>> + usage();
>> + } else {
>> + break;
>> + }
>> +
>> + }
>> +
>> + if (!ok) {
>> + explain();
>> + return -1;
>> + }
>> +
>> + if (argc) {
>> + if (matches(*argv, "dscp") == 0) {
>> + NEXT_ARG();
>> + char *slash;
>> +
>> + slash = strchr(*argv, '/');
>> + if (slash)
>> + *slash = '\0';
>> + if (get_u32(&dscpmask, *argv, 0)) {
>> + fprintf(stderr,
>> + "ctinfo: Illegal dscp \"mask\"\n");
>> + return -1;
>> + }
>
> Not strictly necessary, but would it be nicer to the user to do the
> 'must be exactly 6 bits' test here as well, in order to provide a
> friendlier error message than just 'invalid’?
Agreed. Have implemented that in v4, need to test before sending..both after
sleep :-)
>
> Could also just add this via extack on the kernel side, of course…
May I please dodge that particular bullet on this occasion?
>
>> + if (slash && get_u32(&dscpstatemask, slash + 1, 0)) {
>> + fprintf(stderr,
>> + "ctinfo: Illegal dscp \"statemask\"\n");
>> + return -1;
>> + }
>> + argc--;
>> + argv++;
>> + }
>> + }
>> +
>> + /* cpmark has optional mask parameter, so the next arg might not */
>> + /* exist, or it might be the next option, or it may actually be a */
>> + /* 32bit mask */
>> + if (argc) {
>> + if (matches(*argv, "cpmark") == 0) {
>> + if (NEXT_ARG_OK()) {
>> + NEXT_ARG_FWD();
>> + if (!get_u32(&cpmarkmask, *argv, 0))
>> + NEXT_ARG_FWD(); /* was a mask */
>> + else /* not a mask, use default */
>> + cpmarkmask = ~0;
>> + } else {
>> + NEXT_ARG_FWD();
>> + }
>> + }
>> + }
>> +
>> + if (argc) {
>> + if (matches(*argv, "zone") == 0) {
>> + NEXT_ARG();
>> + if (get_u16(&zone, *argv, 10)) {
>> + fprintf(stderr, "ctinfo: Illegal \"zone\"\n");
>> + return -1;
>> + }
>> + argc--;
>> + argv++;
>> + }
>> + }
>> +
>> + parse_action_control_dflt(&argc, &argv, &sel.action,
>> + false, TC_ACT_PIPE);
>> +
>> + if (argc) {
>> + if (matches(*argv, "index") == 0) {
>> + NEXT_ARG();
>> + if (get_u32(&sel.index, *argv, 10)) {
>> + fprintf(stderr, "ctinfo: Illegal \"index\"\n");
>> + return -1;
>> + }
>> + argc--;
>> + argv++;
>> + }
>> + }
>> +
>> + tail = addattr_nest(n, MAX_MSG, tca_id);
>> + addattr_l(n, MAX_MSG, TCA_CTINFO_ACT, &sel, sizeof(sel));
>> + addattr16(n, MAX_MSG, TCA_CTINFO_ZONE, zone);
>> +
>> + if (dscpmask)
>> + addattr32(n, MAX_MSG,
>> + TCA_CTINFO_PARMS_DSCP_MASK, dscpmask);
>> +
>> + if (dscpstatemask)
>> + addattr32(n, MAX_MSG,
>> + TCA_CTINFO_PARMS_DSCP_STATEMASK, dscpstatemask);
>> +
>> + if (cpmarkmask)
>> + addattr32(n, MAX_MSG,
>> + TCA_CTINFO_PARMS_CPMARK_MASK, cpmarkmask);
>> +
>> + addattr_nest_end(n, tail);
>> +
>> + *argc_p = argc;
>> + *argv_p = argv;
>> + return 0;
>> +}
>> +
>> +static void print_ctinfo_stats(FILE *f, struct rtattr *tb[TCA_CTINFO_MAX + 1])
>> +{
>> + struct tcf_t *tm;
>> +
>> + if (tb[TCA_CTINFO_TM]) {
>> + tm = RTA_DATA(tb[TCA_CTINFO_TM]);
>> +
>> + print_tm(f, tm);
>> + }
>> +
>> + if (tb[TCA_CTINFO_STATS_DSCP_SET])
>> + print_lluint(PRINT_ANY, "dscpset", " DSCP set %llu",
>> + rta_getattr_u64(tb[TCA_CTINFO_STATS_DSCP_SET]));
>> + if (tb[TCA_CTINFO_STATS_DSCP_ERROR])
>> + print_lluint(PRINT_ANY, "dscperror", " error %llu",
>> + rta_getattr_u64(tb[TCA_CTINFO_STATS_DSCP_ERROR]));
>> +
>> + if (tb[TCA_CTINFO_STATS_CPMARK_SET])
>> + print_lluint(PRINT_ANY, "cpmarkset", " CPMARK set %llu",
>> + rta_getattr_u64(tb[TCA_CTINFO_STATS_CPMARK_SET]));
>> +}
>> +
>> +static int print_ctinfo(struct action_util *au, FILE *f, struct rtattr *arg)
>> +{
>> + unsigned int cpmarkmask = ~0, dscpmask = 0, dscpstatemask = 0;
>> + struct rtattr *tb[TCA_CTINFO_MAX + 1];
>> + unsigned short zone = 0;
>> + struct tc_ctinfo *ci;
>> +
>> + if (arg == NULL)
>> + return -1;
>> +
>> + parse_rtattr_nested(tb, TCA_CTINFO_MAX, arg);
>> + if (!tb[TCA_CTINFO_ACT]) {
>> + print_string(PRINT_FP, NULL, "%s",
>> + "[NULL ctinfo action parameters]");
>> + return -1;
>> + }
>> +
>> + ci = RTA_DATA(tb[TCA_CTINFO_ACT]);
>> +
>> + if (tb[TCA_CTINFO_PARMS_DSCP_MASK]) {
>> + if (RTA_PAYLOAD(tb[TCA_CTINFO_PARMS_DSCP_MASK]) >=
>> + sizeof(__u32))
>> + dscpmask = rta_getattr_u32(
>> + tb[TCA_CTINFO_PARMS_DSCP_MASK]);
>> + else
>> + print_string(PRINT_FP, NULL, "%s",
>> + "[invalid dscp mask parameter]");
>> + }
>> +
>> + if (tb[TCA_CTINFO_PARMS_DSCP_STATEMASK]) {
>> + if (RTA_PAYLOAD(tb[TCA_CTINFO_PARMS_DSCP_STATEMASK]) >=
>> + sizeof(__u32))
>> + dscpstatemask = rta_getattr_u32(
>> + tb[TCA_CTINFO_PARMS_DSCP_STATEMASK]);
>> + else
>> + print_string(PRINT_FP, NULL, "%s",
>> + "[invalid dscp statemask parameter]");
>> + }
>> +
>> + if (tb[TCA_CTINFO_PARMS_CPMARK_MASK]) {
>> + if (RTA_PAYLOAD(tb[TCA_CTINFO_PARMS_CPMARK_MASK]) >=
>> + sizeof(__u32))
>> + cpmarkmask = rta_getattr_u32(
>> + tb[TCA_CTINFO_PARMS_CPMARK_MASK]);
>> + else
>> + print_string(PRINT_FP, NULL, "%s",
>> + "[invalid cpmark mask parameter]");
>> + }
>> +
>> + if (tb[TCA_CTINFO_ZONE] && RTA_PAYLOAD(tb[TCA_CTINFO_ZONE]) >=
>> + sizeof(__u16))
>> + zone = rta_getattr_u16(tb[TCA_CTINFO_ZONE]);
>> +
>> + print_string(PRINT_ANY, "kind", "%s ", "ctinfo");
>> + print_hu(PRINT_ANY, "zone", "zone %u", zone);
>> + print_action_control(f, " ", ci->action, "");
>> +
>> + print_string(PRINT_FP, NULL, "%s", _SL_);
>> + print_uint(PRINT_ANY, "index", "\t index %u", ci->index);
>> + print_int(PRINT_ANY, "ref", " ref %d", ci->refcnt);
>> + print_int(PRINT_ANY, "bind", " bind %d", ci->bindcnt);
>> +
>> + if (tb[TCA_CTINFO_PARMS_DSCP_MASK]) {
>> + print_0xhex(PRINT_ANY, "dscpmask", " dscp %#010llx", dscpmask);
>> + print_0xhex(PRINT_ANY, "dscpstatemask", "/%#010llx",
>> + dscpstatemask);
>> + }
>> +
>> + if (tb[TCA_CTINFO_PARMS_CPMARK_MASK])
>> + print_0xhex(PRINT_ANY, "mark", " mark %#010llx",
>> cpmarkmask);
>
> Should be 'cpmark' not 'mark', no?
Yes it should. The one that got away - squished :-)
Thanks for looking through my code, can’t be a pleasant task, and spotting
my bits of code blindness.
Kevin
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC iproute2-next v3] tc: add support for action act_ctinfo
2019-06-02 21:55 ` Kevin 'ldir' Darbyshire-Bryant
@ 2019-06-03 9:15 ` Toke Høiland-Jørgensen
0 siblings, 0 replies; 10+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-06-03 9:15 UTC (permalink / raw)
To: Kevin 'ldir' Darbyshire-Bryant; +Cc: netdev, stephen
Kevin 'ldir' Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk> writes:
>> On 2 Jun 2019, at 21:39, Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk> writes:
>>
>>> ctinfo is an action restoring data stored in conntrack marks to various
>>> fields. At present it has two independent modes of operation,
>>> restoration of DSCP into IPv4/v6 diffserv and restoration of conntrack
>>> marks into packet skb marks.
>>>
>>> It understands a number of parameters specific to this action in
>>> additional to the usual action syntax. Each operating mode is
>>> independent of the other so all options are optional, however not
>>> specifying at least one mode is a bit pointless.
>>>
>>> Usage: ... ctinfo [dscp mask[/statemask]] [cpmark [mask]] [zone ZONE]
>>> [CONTROL] [index <INDEX>]
>>
>> Yay, bikeshedding time! :)
>
> I see your bikeshed and raise you… a bus shelter :-)
>
>>
>> As I said in reply to the kernel patch, the "X/Y" syntax usually means
>> "<value>/<mask applied to value>", where here they are just two
>> semi-related mask values. So I think it would be better to just make
>> 'statemask' its own parameter.
>
> Instead of creating another keyword how about we drop the ‘/‘ and
> make it a space separated optional parameter to ‘dscp’? eg.
>
> Usage: ... ctinfo [dscp mask [statemask]] [cpmark [mask]] blah blah
Sure, I can live with that :)
>> Other than that, just a few nits, below...
>>
>>> DSCP mode
>>>
>>> dscp enables copying of a DSCP store in the conntrack mark into the
>>> ipv4/v6 diffserv field. The mask is a 32bit field and specifies where
>>> in the conntrack mark the DSCP value is stored. It must be 6 contiguous
>>> bits long, e.g. 0xfc000000 would restore the DSCP from the upper 6 bits
>>> of the conntrack mark.
>>>
>>> The DSCP copying may be optionally controlled by a statemask. The
>>> statemask is a 32bit field, usually with a single bit set and must not
>>> overlap the dscp mask. The DSCP restore operation will only take place
>>> if the corresponding bit/s in conntrack mark yield a non zero result.
>>>
>>> eg. dscp 0xfc000000/0x01000000 would retrieve the DSCP from the top 6
>>> bits, whilst using bit 25 as a flag to do so. Bit 26 is unused in this
>>> example.
>>>
>>> CPMARK mode
>>>
>>> cpmark enables copying of the conntrack mark to the packet skb mark. In
>>> this mode it is completely equivalent to the existing act_connmark.
>>> Additional functionality is provided by the optional mask parameter,
>>> whereby the stored conntrack mark is logically anded with the cpmark
>>> mask before being stored into skb mark. This allows shared usage of the
>>> conntrack mark between applications.
>>>
>>> eg. cpmark 0x00ffffff would restore only the lower 24 bits of the
>>> conntrack mark, thus may be useful in the event that the upper 8 bits
>>> are used by the DSCP function.
>>>
>>> Usage: ... ctinfo [dscp mask[/statemask]] [cpmark [mask]] [zone ZONE]
>>> [CONTROL] [index <INDEX>]
>>> where :
>>> dscp MASK is the bitmask to restore DSCP
>>> STATEMASK is the bitmask to determine conditional restoring
>>> cpmark MASK mask applied to restored packet mark
>>> ZONE is the conntrack zone
>>> CONTROL := reclassify | pipe | drop | continue | ok |
>>> goto chain <CHAIN_INDEX>
>>>
>>> Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
>>>
>>> ---
>>> v2 - fix whitespace issue in pkt_cls
>>> fix most warnings from checkpatch - some lines still over 80 chars
>>> due to long TLV names.
>>> v3 - fix some dangling else warnings.
>>> refactor stats printing to please checkpatch.
>>> send zone TLV even if default '0' zone.
>>> now checkpatch clean even though I think some of the formatting
>>> is horrible :-)
>>> sending via google's smtp 'cos MS' exchange office365 appears
>>> to mangle patches from git send-email.
>>
>> Ah, so it wasn't just me having problems ;)
>
> No, though I’m still not clear what’s going on or when Microsoft
> improved(tm) it :-/
I think it just re-encodes it as base64 instead of quoted-printable.
Which makes 'git am' take the whitespace very literally; or maybe
Exchange is also recoding the whitespace in the process...
>>> include/uapi/linux/pkt_cls.h | 1 +
>>> include/uapi/linux/tc_act/tc_ctinfo.h | 34 ++++
>>> tc/Makefile | 1 +
>>> tc/m_ctinfo.c | 262 ++++++++++++++++++++++++++
>>> 4 files changed, 298 insertions(+)
>>> create mode 100644 include/uapi/linux/tc_act/tc_ctinfo.h
>>> create mode 100644 tc/m_ctinfo.c
>>>
>>> diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
>>> index 51a0496f..a93680fc 100644
>>> --- a/include/uapi/linux/pkt_cls.h
>>> +++ b/include/uapi/linux/pkt_cls.h
>>> @@ -105,6 +105,7 @@ enum tca_id {
>>> TCA_ID_IFE = TCA_ACT_IFE,
>>> TCA_ID_SAMPLE = TCA_ACT_SAMPLE,
>>> /* other actions go here */
>>> + TCA_ID_CTINFO,
>>> __TCA_ID_MAX = 255
>>> };
>>>
>>> diff --git a/include/uapi/linux/tc_act/tc_ctinfo.h b/include/uapi/linux/tc_act/tc_ctinfo.h
>>> new file mode 100644
>>> index 00000000..da803e05
>>> --- /dev/null
>>> +++ b/include/uapi/linux/tc_act/tc_ctinfo.h
>>> @@ -0,0 +1,34 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>>> +#ifndef __UAPI_TC_CTINFO_H
>>> +#define __UAPI_TC_CTINFO_H
>>> +
>>> +#include <linux/types.h>
>>> +#include <linux/pkt_cls.h>
>>> +
>>> +struct tc_ctinfo {
>>> + tc_gen;
>>> +};
>>> +
>>> +enum {
>>> + TCA_CTINFO_UNSPEC,
>>> + TCA_CTINFO_PAD,
>>> + TCA_CTINFO_TM,
>>> + TCA_CTINFO_ACT,
>>> + TCA_CTINFO_ZONE,
>>> + TCA_CTINFO_PARMS_DSCP_MASK,
>>> + TCA_CTINFO_PARMS_DSCP_STATEMASK,
>>> + TCA_CTINFO_PARMS_CPMARK_MASK,
>>> + TCA_CTINFO_STATS_DSCP_SET,
>>> + TCA_CTINFO_STATS_DSCP_ERROR,
>>> + TCA_CTINFO_STATS_CPMARK_SET,
>>> + __TCA_CTINFO_MAX
>>> +};
>>> +
>>> +#define TCA_CTINFO_MAX (__TCA_CTINFO_MAX - 1)
>>> +
>>> +enum {
>>> + CTINFO_MODE_DSCP = BIT(0),
>>> + CTINFO_MODE_CPMARK = BIT(1)
>>> +};
>>> +
>>> +#endif
>>> diff --git a/tc/Makefile b/tc/Makefile
>>> index 1a305cf4..60abddee 100644
>>> --- a/tc/Makefile
>>> +++ b/tc/Makefile
>>> @@ -48,6 +48,7 @@ TCMODULES += m_csum.o
>>> TCMODULES += m_simple.o
>>> TCMODULES += m_vlan.o
>>> TCMODULES += m_connmark.o
>>> +TCMODULES += m_ctinfo.o
>>> TCMODULES += m_bpf.o
>>> TCMODULES += m_tunnel_key.o
>>> TCMODULES += m_sample.o
>>> diff --git a/tc/m_ctinfo.c b/tc/m_ctinfo.c
>>> new file mode 100644
>>> index 00000000..af5102bf
>>> --- /dev/null
>>> +++ b/tc/m_ctinfo.c
>>> @@ -0,0 +1,262 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/*
>>> + * m_ctinfo.c netfilter ctinfo mark action
>>> + *
>>> + * Copyright (c) 2019 Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
>>> + */
>>> +
>>> +#include <stdio.h>
>>> +#include <stdlib.h>
>>> +#include <unistd.h>
>>> +#include <string.h>
>>> +#include "utils.h"
>>> +#include "tc_util.h"
>>> +#include <linux/tc_act/tc_ctinfo.h>
>>> +
>>> +static void
>>> +explain(void)
>>> +{
>>> + fprintf(stderr,
>>> + "Usage: ... ctinfo [dscp mask[/statemask]] [cpmark [mask]] [zone ZONE] [CONTROL] [index <INDEX>]\n"
>>> + "where :\n"
>>> + "\tdscp MASK bitmask location of stored DSCP\n"
>>> + "\t STATEMASK bitmask to determine conditional restoring\n"
>>> + "\tcpmark MASK mask applied to mark on restoration\n"
>>> + "\tZONE is the conntrack zone\n"
>>> + "\tCONTROL := reclassify | pipe | drop | continue | ok |\n"
>>> + "\t goto chain <CHAIN_INDEX>\n");
>>> +}
>>> +
>>> +static void
>>> +usage(void)
>>> +{
>>> + explain();
>>> + exit(-1);
>>> +}
>>> +
>>> +static int
>>> +parse_ctinfo(struct action_util *a, int *argc_p, char ***argv_p, int tca_id,
>>> + struct nlmsghdr *n)
>>> +{
>>> + unsigned int cpmarkmask = 0, dscpmask = 0, dscpstatemask = 0;
>>> + struct tc_ctinfo sel = {};
>>> + unsigned short zone = 0;
>>> + char **argv = *argv_p;
>>> + struct rtattr *tail;
>>> + int argc = *argc_p;
>>> + int ok = 0;
>>> +
>>> + while (argc > 0) {
>>> + if (matches(*argv, "ctinfo") == 0) {
>>> + ok = 1;
>>> + argc--;
>>> + argv++;
>>> + } else if (matches(*argv, "help") == 0) {
>>> + usage();
>>> + } else {
>>> + break;
>>> + }
>>> +
>>> + }
>>> +
>>> + if (!ok) {
>>> + explain();
>>> + return -1;
>>> + }
>>> +
>>> + if (argc) {
>>> + if (matches(*argv, "dscp") == 0) {
>>> + NEXT_ARG();
>>> + char *slash;
>>> +
>>> + slash = strchr(*argv, '/');
>>> + if (slash)
>>> + *slash = '\0';
>>> + if (get_u32(&dscpmask, *argv, 0)) {
>>> + fprintf(stderr,
>>> + "ctinfo: Illegal dscp \"mask\"\n");
>>> + return -1;
>>> + }
>>
>> Not strictly necessary, but would it be nicer to the user to do the
>> 'must be exactly 6 bits' test here as well, in order to provide a
>> friendlier error message than just 'invalid’?
>
> Agreed. Have implemented that in v4, need to test before sending..both after
> sleep :-)
Cool
>>
>> Could also just add this via extack on the kernel side, of course…
>
> May I please dodge that particular bullet on this occasion?
Haha, yeah, otherwise I would have insisted on it on the kernel
submission ;)
>>
>>> + if (slash && get_u32(&dscpstatemask, slash + 1, 0)) {
>>> + fprintf(stderr,
>>> + "ctinfo: Illegal dscp \"statemask\"\n");
>>> + return -1;
>>> + }
>>> + argc--;
>>> + argv++;
>>> + }
>>> + }
>>> +
>>> + /* cpmark has optional mask parameter, so the next arg might not */
>>> + /* exist, or it might be the next option, or it may actually be a */
>>> + /* 32bit mask */
>>> + if (argc) {
>>> + if (matches(*argv, "cpmark") == 0) {
>>> + if (NEXT_ARG_OK()) {
>>> + NEXT_ARG_FWD();
>>> + if (!get_u32(&cpmarkmask, *argv, 0))
>>> + NEXT_ARG_FWD(); /* was a mask */
>>> + else /* not a mask, use default */
>>> + cpmarkmask = ~0;
>>> + } else {
>>> + NEXT_ARG_FWD();
>>> + }
>>> + }
>>> + }
>>> +
>>> + if (argc) {
>>> + if (matches(*argv, "zone") == 0) {
>>> + NEXT_ARG();
>>> + if (get_u16(&zone, *argv, 10)) {
>>> + fprintf(stderr, "ctinfo: Illegal \"zone\"\n");
>>> + return -1;
>>> + }
>>> + argc--;
>>> + argv++;
>>> + }
>>> + }
>>> +
>>> + parse_action_control_dflt(&argc, &argv, &sel.action,
>>> + false, TC_ACT_PIPE);
>>> +
>>> + if (argc) {
>>> + if (matches(*argv, "index") == 0) {
>>> + NEXT_ARG();
>>> + if (get_u32(&sel.index, *argv, 10)) {
>>> + fprintf(stderr, "ctinfo: Illegal \"index\"\n");
>>> + return -1;
>>> + }
>>> + argc--;
>>> + argv++;
>>> + }
>>> + }
>>> +
>>> + tail = addattr_nest(n, MAX_MSG, tca_id);
>>> + addattr_l(n, MAX_MSG, TCA_CTINFO_ACT, &sel, sizeof(sel));
>>> + addattr16(n, MAX_MSG, TCA_CTINFO_ZONE, zone);
>>> +
>>> + if (dscpmask)
>>> + addattr32(n, MAX_MSG,
>>> + TCA_CTINFO_PARMS_DSCP_MASK, dscpmask);
>>> +
>>> + if (dscpstatemask)
>>> + addattr32(n, MAX_MSG,
>>> + TCA_CTINFO_PARMS_DSCP_STATEMASK, dscpstatemask);
>>> +
>>> + if (cpmarkmask)
>>> + addattr32(n, MAX_MSG,
>>> + TCA_CTINFO_PARMS_CPMARK_MASK, cpmarkmask);
>>> +
>>> + addattr_nest_end(n, tail);
>>> +
>>> + *argc_p = argc;
>>> + *argv_p = argv;
>>> + return 0;
>>> +}
>>> +
>>> +static void print_ctinfo_stats(FILE *f, struct rtattr *tb[TCA_CTINFO_MAX + 1])
>>> +{
>>> + struct tcf_t *tm;
>>> +
>>> + if (tb[TCA_CTINFO_TM]) {
>>> + tm = RTA_DATA(tb[TCA_CTINFO_TM]);
>>> +
>>> + print_tm(f, tm);
>>> + }
>>> +
>>> + if (tb[TCA_CTINFO_STATS_DSCP_SET])
>>> + print_lluint(PRINT_ANY, "dscpset", " DSCP set %llu",
>>> + rta_getattr_u64(tb[TCA_CTINFO_STATS_DSCP_SET]));
>>> + if (tb[TCA_CTINFO_STATS_DSCP_ERROR])
>>> + print_lluint(PRINT_ANY, "dscperror", " error %llu",
>>> + rta_getattr_u64(tb[TCA_CTINFO_STATS_DSCP_ERROR]));
>>> +
>>> + if (tb[TCA_CTINFO_STATS_CPMARK_SET])
>>> + print_lluint(PRINT_ANY, "cpmarkset", " CPMARK set %llu",
>>> + rta_getattr_u64(tb[TCA_CTINFO_STATS_CPMARK_SET]));
>>> +}
>>> +
>>> +static int print_ctinfo(struct action_util *au, FILE *f, struct rtattr *arg)
>>> +{
>>> + unsigned int cpmarkmask = ~0, dscpmask = 0, dscpstatemask = 0;
>>> + struct rtattr *tb[TCA_CTINFO_MAX + 1];
>>> + unsigned short zone = 0;
>>> + struct tc_ctinfo *ci;
>>> +
>>> + if (arg == NULL)
>>> + return -1;
>>> +
>>> + parse_rtattr_nested(tb, TCA_CTINFO_MAX, arg);
>>> + if (!tb[TCA_CTINFO_ACT]) {
>>> + print_string(PRINT_FP, NULL, "%s",
>>> + "[NULL ctinfo action parameters]");
>>> + return -1;
>>> + }
>>> +
>>> + ci = RTA_DATA(tb[TCA_CTINFO_ACT]);
>>> +
>>> + if (tb[TCA_CTINFO_PARMS_DSCP_MASK]) {
>>> + if (RTA_PAYLOAD(tb[TCA_CTINFO_PARMS_DSCP_MASK]) >=
>>> + sizeof(__u32))
>>> + dscpmask = rta_getattr_u32(
>>> + tb[TCA_CTINFO_PARMS_DSCP_MASK]);
>>> + else
>>> + print_string(PRINT_FP, NULL, "%s",
>>> + "[invalid dscp mask parameter]");
>>> + }
>>> +
>>> + if (tb[TCA_CTINFO_PARMS_DSCP_STATEMASK]) {
>>> + if (RTA_PAYLOAD(tb[TCA_CTINFO_PARMS_DSCP_STATEMASK]) >=
>>> + sizeof(__u32))
>>> + dscpstatemask = rta_getattr_u32(
>>> + tb[TCA_CTINFO_PARMS_DSCP_STATEMASK]);
>>> + else
>>> + print_string(PRINT_FP, NULL, "%s",
>>> + "[invalid dscp statemask parameter]");
>>> + }
>>> +
>>> + if (tb[TCA_CTINFO_PARMS_CPMARK_MASK]) {
>>> + if (RTA_PAYLOAD(tb[TCA_CTINFO_PARMS_CPMARK_MASK]) >=
>>> + sizeof(__u32))
>>> + cpmarkmask = rta_getattr_u32(
>>> + tb[TCA_CTINFO_PARMS_CPMARK_MASK]);
>>> + else
>>> + print_string(PRINT_FP, NULL, "%s",
>>> + "[invalid cpmark mask parameter]");
>>> + }
>>> +
>>> + if (tb[TCA_CTINFO_ZONE] && RTA_PAYLOAD(tb[TCA_CTINFO_ZONE]) >=
>>> + sizeof(__u16))
>>> + zone = rta_getattr_u16(tb[TCA_CTINFO_ZONE]);
>>> +
>>> + print_string(PRINT_ANY, "kind", "%s ", "ctinfo");
>>> + print_hu(PRINT_ANY, "zone", "zone %u", zone);
>>> + print_action_control(f, " ", ci->action, "");
>>> +
>>> + print_string(PRINT_FP, NULL, "%s", _SL_);
>>> + print_uint(PRINT_ANY, "index", "\t index %u", ci->index);
>>> + print_int(PRINT_ANY, "ref", " ref %d", ci->refcnt);
>>> + print_int(PRINT_ANY, "bind", " bind %d", ci->bindcnt);
>>> +
>>> + if (tb[TCA_CTINFO_PARMS_DSCP_MASK]) {
>>> + print_0xhex(PRINT_ANY, "dscpmask", " dscp %#010llx", dscpmask);
>>> + print_0xhex(PRINT_ANY, "dscpstatemask", "/%#010llx",
>>> + dscpstatemask);
>>> + }
>>> +
>>> + if (tb[TCA_CTINFO_PARMS_CPMARK_MASK])
>>> + print_0xhex(PRINT_ANY, "mark", " mark %#010llx",
>>> cpmarkmask);
>>
>> Should be 'cpmark' not 'mark', no?
>
> Yes it should. The one that got away - squished :-)
>
> Thanks for looking through my code, can’t be a pleasant task, and spotting
> my bits of code blindness.
>
> Kevin
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH RFC iproute2-next v4] tc: add support for action act_ctinfo
2019-06-02 18:50 ` [PATCH RFC iproute2-next v3] tc: add support for action act_ctinfo Kevin Darbyshire-Bryant
2019-06-02 20:39 ` Toke Høiland-Jørgensen
@ 2019-06-03 13:50 ` Kevin Darbyshire-Bryant
2019-06-03 20:14 ` Toke Høiland-Jørgensen
2019-06-05 18:23 ` Joe Perches
1 sibling, 2 replies; 10+ messages in thread
From: Kevin Darbyshire-Bryant @ 2019-06-03 13:50 UTC (permalink / raw)
To: ldir; +Cc: netdev, stephen
ctinfo is an action restoring data stored in conntrack marks to various
fields. At present it has two independent modes of operation,
restoration of DSCP into IPv4/v6 diffserv and restoration of conntrack
marks into packet skb marks.
It understands a number of parameters specific to this action in
additional to the usual action syntax. Each operating mode is
independent of the other so all options are optional, however not
specifying at least one mode is a bit pointless.
Usage: ... ctinfo [dscp mask [statemask]] [cpmark [mask]] [zone ZONE]
[CONTROL] [index <INDEX>]
DSCP mode
dscp enables copying of a DSCP store in the conntrack mark into the
ipv4/v6 diffserv field. The mask is a 32bit field and specifies where
in the conntrack mark the DSCP value is stored. It must be 6 contiguous
bits long, e.g. 0xfc000000 would restore the DSCP from the upper 6 bits
of the conntrack mark.
The DSCP copying may be optionally controlled by a statemask. The
statemask is a 32bit field, usually with a single bit set and must not
overlap the dscp mask. The DSCP restore operation will only take place
if the corresponding bit/s in conntrack mark yield a non zero result.
eg. dscp 0xfc000000/0x01000000 would retrieve the DSCP from the top 6
bits, whilst using bit 25 as a flag to do so. Bit 26 is unused in this
example.
CPMARK mode
cpmark enables copying of the conntrack mark to the packet skb mark. In
this mode it is completely equivalent to the existing act_connmark.
Additional functionality is provided by the optional mask parameter,
whereby the stored conntrack mark is logically anded with the cpmark
mask before being stored into skb mark. This allows shared usage of the
conntrack mark between applications.
eg. cpmark 0x00ffffff would restore only the lower 24 bits of the
conntrack mark, thus may be useful in the event that the upper 8 bits
are used by the DSCP function.
Usage: ... ctinfo [dscp mask [statemask]] [cpmark [mask]] [zone ZONE]
[CONTROL] [index <INDEX>]
where :
dscp MASK is the bitmask to restore DSCP
STATEMASK is the bitmask to determine conditional restoring
cpmark MASK mask applied to restored packet mark
ZONE is the conntrack zone
CONTROL := reclassify | pipe | drop | continue | ok |
goto chain <CHAIN_INDEX>
Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
---
v2 - fix whitespace issue in pkt_cls
fix most warnings from checkpatch - some lines still over 80 chars
due to long TLV names.
v3 - fix some dangling else warnings.
refactor stats printing to please checkpatch.
send zone TLV even if default '0' zone.
now checkpatch clean even though I think some of the formatting
is horrible :-)
sending via google's smtp 'cos MS' exchange office365 appears
to mangle patches from git send-email.
v4 - use the NEXT_ARG macros throughout.
fix printing typo use 'cpmark' instead of 'mark'.
use space separator between dscp mask & optional statemask and
update usage as a result.
validate dscp mask & statemask and print friendlier warnings
than "invalid".
fix cpmark option default value handling bug.
include/uapi/linux/pkt_cls.h | 1 +
include/uapi/linux/tc_act/tc_ctinfo.h | 34 ++++
tc/Makefile | 1 +
tc/m_ctinfo.c | 268 ++++++++++++++++++++++++++
4 files changed, 304 insertions(+)
create mode 100644 include/uapi/linux/tc_act/tc_ctinfo.h
create mode 100644 tc/m_ctinfo.c
diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index 51a0496f..a93680fc 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -105,6 +105,7 @@ enum tca_id {
TCA_ID_IFE = TCA_ACT_IFE,
TCA_ID_SAMPLE = TCA_ACT_SAMPLE,
/* other actions go here */
+ TCA_ID_CTINFO,
__TCA_ID_MAX = 255
};
diff --git a/include/uapi/linux/tc_act/tc_ctinfo.h b/include/uapi/linux/tc_act/tc_ctinfo.h
new file mode 100644
index 00000000..da803e05
--- /dev/null
+++ b/include/uapi/linux/tc_act/tc_ctinfo.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef __UAPI_TC_CTINFO_H
+#define __UAPI_TC_CTINFO_H
+
+#include <linux/types.h>
+#include <linux/pkt_cls.h>
+
+struct tc_ctinfo {
+ tc_gen;
+};
+
+enum {
+ TCA_CTINFO_UNSPEC,
+ TCA_CTINFO_PAD,
+ TCA_CTINFO_TM,
+ TCA_CTINFO_ACT,
+ TCA_CTINFO_ZONE,
+ TCA_CTINFO_PARMS_DSCP_MASK,
+ TCA_CTINFO_PARMS_DSCP_STATEMASK,
+ TCA_CTINFO_PARMS_CPMARK_MASK,
+ TCA_CTINFO_STATS_DSCP_SET,
+ TCA_CTINFO_STATS_DSCP_ERROR,
+ TCA_CTINFO_STATS_CPMARK_SET,
+ __TCA_CTINFO_MAX
+};
+
+#define TCA_CTINFO_MAX (__TCA_CTINFO_MAX - 1)
+
+enum {
+ CTINFO_MODE_DSCP = BIT(0),
+ CTINFO_MODE_CPMARK = BIT(1)
+};
+
+#endif
diff --git a/tc/Makefile b/tc/Makefile
index 1a305cf4..60abddee 100644
--- a/tc/Makefile
+++ b/tc/Makefile
@@ -48,6 +48,7 @@ TCMODULES += m_csum.o
TCMODULES += m_simple.o
TCMODULES += m_vlan.o
TCMODULES += m_connmark.o
+TCMODULES += m_ctinfo.o
TCMODULES += m_bpf.o
TCMODULES += m_tunnel_key.o
TCMODULES += m_sample.o
diff --git a/tc/m_ctinfo.c b/tc/m_ctinfo.c
new file mode 100644
index 00000000..5e451f87
--- /dev/null
+++ b/tc/m_ctinfo.c
@@ -0,0 +1,268 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * m_ctinfo.c netfilter ctinfo mark action
+ *
+ * Copyright (c) 2019 Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <string.h>
+#include "utils.h"
+#include "tc_util.h"
+#include <linux/tc_act/tc_ctinfo.h>
+
+static void
+explain(void)
+{
+ fprintf(stderr,
+ "Usage: ... ctinfo [dscp mask [statemask]] [cpmark [mask]] [zone ZONE] [CONTROL] [index <INDEX>]\n"
+ "where :\n"
+ "\tdscp MASK bitmask location of stored DSCP\n"
+ "\t STATEMASK bitmask to determine conditional restoring\n"
+ "\tcpmark MASK mask applied to mark on restoration\n"
+ "\tZONE is the conntrack zone\n"
+ "\tCONTROL := reclassify | pipe | drop | continue | ok |\n"
+ "\t goto chain <CHAIN_INDEX>\n");
+}
+
+static void
+usage(void)
+{
+ explain();
+ exit(-1);
+}
+
+static int
+parse_ctinfo(struct action_util *a, int *argc_p, char ***argv_p, int tca_id,
+ struct nlmsghdr *n)
+{
+ unsigned int cpmarkmask = 0, dscpmask = 0, dscpstatemask = 0;
+ struct tc_ctinfo sel = {};
+ unsigned short zone = 0;
+ char **argv = *argv_p;
+ struct rtattr *tail;
+ int argc = *argc_p;
+ int ok = 0;
+ __u8 i;
+
+ while (argc > 0) {
+ if (matches(*argv, "ctinfo") == 0) {
+ ok = 1;
+ NEXT_ARG_FWD();
+ } else if (matches(*argv, "help") == 0) {
+ usage();
+ } else {
+ break;
+ }
+
+ }
+
+ if (!ok) {
+ explain();
+ return -1;
+ }
+
+ if (argc) {
+ if (matches(*argv, "dscp") == 0) {
+ NEXT_ARG();
+ if (get_u32(&dscpmask, *argv, 0)) {
+ fprintf(stderr,
+ "ctinfo: Illegal dscp \"mask\"\n");
+ return -1;
+ }
+ if (NEXT_ARG_OK()) {
+ NEXT_ARG_FWD();
+ if (!get_u32(&dscpstatemask, *argv, 0))
+ NEXT_ARG_FWD(); /* was a statemask */
+ } else {
+ NEXT_ARG_FWD();
+ }
+ }
+ }
+
+ /* cpmark has optional mask parameter, so the next arg might not */
+ /* exist, or it might be the next option, or it may actually be a */
+ /* 32bit mask */
+ if (argc) {
+ if (matches(*argv, "cpmark") == 0) {
+ cpmarkmask = ~0;
+ if (NEXT_ARG_OK()) {
+ NEXT_ARG_FWD();
+ if (!get_u32(&cpmarkmask, *argv, 0))
+ NEXT_ARG_FWD(); /* was a mask */
+ } else {
+ NEXT_ARG_FWD();
+ }
+ }
+ }
+
+ if (argc) {
+ if (matches(*argv, "zone") == 0) {
+ NEXT_ARG();
+ if (get_u16(&zone, *argv, 10)) {
+ fprintf(stderr, "ctinfo: Illegal \"zone\"\n");
+ return -1;
+ }
+ NEXT_ARG_FWD();
+ }
+ }
+
+ parse_action_control_dflt(&argc, &argv, &sel.action,
+ false, TC_ACT_PIPE);
+
+ if (argc) {
+ if (matches(*argv, "index") == 0) {
+ NEXT_ARG();
+ if (get_u32(&sel.index, *argv, 10)) {
+ fprintf(stderr, "ctinfo: Illegal \"index\"\n");
+ return -1;
+ }
+ NEXT_ARG_FWD();
+ }
+ }
+
+ if (dscpmask & dscpstatemask) {
+ fprintf(stderr,
+ "ctinfo: dscp mask & statemask must NOT overlap\n");
+ return -1;
+ }
+
+ i = ffs(dscpmask);
+ if (i && ((~0 & (dscpmask >> (i - 1))) != 0x3f)) {
+ fprintf(stderr,
+ "ctinfo: dscp mask must be 6 contiguous bits long\n");
+ return -1;
+ }
+
+ tail = addattr_nest(n, MAX_MSG, tca_id);
+ addattr_l(n, MAX_MSG, TCA_CTINFO_ACT, &sel, sizeof(sel));
+ addattr16(n, MAX_MSG, TCA_CTINFO_ZONE, zone);
+
+ if (dscpmask)
+ addattr32(n, MAX_MSG,
+ TCA_CTINFO_PARMS_DSCP_MASK, dscpmask);
+
+ if (dscpstatemask)
+ addattr32(n, MAX_MSG,
+ TCA_CTINFO_PARMS_DSCP_STATEMASK, dscpstatemask);
+
+ if (cpmarkmask)
+ addattr32(n, MAX_MSG,
+ TCA_CTINFO_PARMS_CPMARK_MASK, cpmarkmask);
+
+ addattr_nest_end(n, tail);
+
+ *argc_p = argc;
+ *argv_p = argv;
+ return 0;
+}
+
+static void print_ctinfo_stats(FILE *f, struct rtattr *tb[TCA_CTINFO_MAX + 1])
+{
+ struct tcf_t *tm;
+
+ if (tb[TCA_CTINFO_TM]) {
+ tm = RTA_DATA(tb[TCA_CTINFO_TM]);
+
+ print_tm(f, tm);
+ }
+
+ if (tb[TCA_CTINFO_STATS_DSCP_SET])
+ print_lluint(PRINT_ANY, "dscpset", " DSCP set %llu",
+ rta_getattr_u64(tb[TCA_CTINFO_STATS_DSCP_SET]));
+ if (tb[TCA_CTINFO_STATS_DSCP_ERROR])
+ print_lluint(PRINT_ANY, "dscperror", " error %llu",
+ rta_getattr_u64(tb[TCA_CTINFO_STATS_DSCP_ERROR]));
+
+ if (tb[TCA_CTINFO_STATS_CPMARK_SET])
+ print_lluint(PRINT_ANY, "cpmarkset", " CPMARK set %llu",
+ rta_getattr_u64(tb[TCA_CTINFO_STATS_CPMARK_SET]));
+}
+
+static int print_ctinfo(struct action_util *au, FILE *f, struct rtattr *arg)
+{
+ unsigned int cpmarkmask = ~0, dscpmask = 0, dscpstatemask = 0;
+ struct rtattr *tb[TCA_CTINFO_MAX + 1];
+ unsigned short zone = 0;
+ struct tc_ctinfo *ci;
+
+ if (arg == NULL)
+ return -1;
+
+ parse_rtattr_nested(tb, TCA_CTINFO_MAX, arg);
+ if (!tb[TCA_CTINFO_ACT]) {
+ print_string(PRINT_FP, NULL, "%s",
+ "[NULL ctinfo action parameters]");
+ return -1;
+ }
+
+ ci = RTA_DATA(tb[TCA_CTINFO_ACT]);
+
+ if (tb[TCA_CTINFO_PARMS_DSCP_MASK]) {
+ if (RTA_PAYLOAD(tb[TCA_CTINFO_PARMS_DSCP_MASK]) >=
+ sizeof(__u32))
+ dscpmask = rta_getattr_u32(
+ tb[TCA_CTINFO_PARMS_DSCP_MASK]);
+ else
+ print_string(PRINT_FP, NULL, "%s",
+ "[invalid dscp mask parameter]");
+ }
+
+ if (tb[TCA_CTINFO_PARMS_DSCP_STATEMASK]) {
+ if (RTA_PAYLOAD(tb[TCA_CTINFO_PARMS_DSCP_STATEMASK]) >=
+ sizeof(__u32))
+ dscpstatemask = rta_getattr_u32(
+ tb[TCA_CTINFO_PARMS_DSCP_STATEMASK]);
+ else
+ print_string(PRINT_FP, NULL, "%s",
+ "[invalid dscp statemask parameter]");
+ }
+
+ if (tb[TCA_CTINFO_PARMS_CPMARK_MASK]) {
+ if (RTA_PAYLOAD(tb[TCA_CTINFO_PARMS_CPMARK_MASK]) >=
+ sizeof(__u32))
+ cpmarkmask = rta_getattr_u32(
+ tb[TCA_CTINFO_PARMS_CPMARK_MASK]);
+ else
+ print_string(PRINT_FP, NULL, "%s",
+ "[invalid cpmark mask parameter]");
+ }
+
+ if (tb[TCA_CTINFO_ZONE] && RTA_PAYLOAD(tb[TCA_CTINFO_ZONE]) >=
+ sizeof(__u16))
+ zone = rta_getattr_u16(tb[TCA_CTINFO_ZONE]);
+
+ print_string(PRINT_ANY, "kind", "%s ", "ctinfo");
+ print_hu(PRINT_ANY, "zone", "zone %u", zone);
+ print_action_control(f, " ", ci->action, "");
+
+ print_string(PRINT_FP, NULL, "%s", _SL_);
+ print_uint(PRINT_ANY, "index", "\t index %u", ci->index);
+ print_int(PRINT_ANY, "ref", " ref %d", ci->refcnt);
+ print_int(PRINT_ANY, "bind", " bind %d", ci->bindcnt);
+
+ if (tb[TCA_CTINFO_PARMS_DSCP_MASK]) {
+ print_0xhex(PRINT_ANY, "dscpmask", " dscp %#010llx", dscpmask);
+ print_0xhex(PRINT_ANY, "dscpstatemask", " %#010llx",
+ dscpstatemask);
+ }
+
+ if (tb[TCA_CTINFO_PARMS_CPMARK_MASK])
+ print_0xhex(PRINT_ANY, "cpmark", " cpmark %#010llx",
+ cpmarkmask);
+
+ if (show_stats)
+ print_ctinfo_stats(f, tb);
+
+ print_string(PRINT_FP, NULL, "%s", _SL_);
+
+ return 0;
+}
+
+struct action_util ctinfo_action_util = {
+ .id = "ctinfo",
+ .parse_aopt = parse_ctinfo,
+ .print_aopt = print_ctinfo,
+};
--
2.20.1 (Apple Git-117)
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH RFC iproute2-next v4] tc: add support for action act_ctinfo
2019-06-03 13:50 ` [PATCH RFC iproute2-next v4] " Kevin Darbyshire-Bryant
@ 2019-06-03 20:14 ` Toke Høiland-Jørgensen
2019-06-05 18:23 ` Joe Perches
1 sibling, 0 replies; 10+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-06-03 20:14 UTC (permalink / raw)
To: Kevin Darbyshire-Bryant, ldir; +Cc: netdev, stephen
Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk> writes:
> ctinfo is an action restoring data stored in conntrack marks to various
> fields. At present it has two independent modes of operation,
> restoration of DSCP into IPv4/v6 diffserv and restoration of conntrack
> marks into packet skb marks.
>
> It understands a number of parameters specific to this action in
> additional to the usual action syntax. Each operating mode is
> independent of the other so all options are optional, however not
> specifying at least one mode is a bit pointless.
>
> Usage: ... ctinfo [dscp mask [statemask]] [cpmark [mask]] [zone ZONE]
> [CONTROL] [index <INDEX>]
>
> DSCP mode
>
> dscp enables copying of a DSCP store in the conntrack mark into the
> ipv4/v6 diffserv field. The mask is a 32bit field and specifies where
> in the conntrack mark the DSCP value is stored. It must be 6 contiguous
> bits long, e.g. 0xfc000000 would restore the DSCP from the upper 6 bits
> of the conntrack mark.
>
> The DSCP copying may be optionally controlled by a statemask. The
> statemask is a 32bit field, usually with a single bit set and must not
> overlap the dscp mask. The DSCP restore operation will only take place
> if the corresponding bit/s in conntrack mark yield a non zero result.
>
> eg. dscp 0xfc000000/0x01000000 would retrieve the DSCP from the top 6
> bits, whilst using bit 25 as a flag to do so. Bit 26 is unused in this
> example.
>
> CPMARK mode
>
> cpmark enables copying of the conntrack mark to the packet skb mark. In
> this mode it is completely equivalent to the existing act_connmark.
> Additional functionality is provided by the optional mask parameter,
> whereby the stored conntrack mark is logically anded with the cpmark
> mask before being stored into skb mark. This allows shared usage of the
> conntrack mark between applications.
>
> eg. cpmark 0x00ffffff would restore only the lower 24 bits of the
> conntrack mark, thus may be useful in the event that the upper 8 bits
> are used by the DSCP function.
>
> Usage: ... ctinfo [dscp mask [statemask]] [cpmark [mask]] [zone ZONE]
> [CONTROL] [index <INDEX>]
> where :
> dscp MASK is the bitmask to restore DSCP
> STATEMASK is the bitmask to determine conditional restoring
> cpmark MASK mask applied to restored packet mark
> ZONE is the conntrack zone
> CONTROL := reclassify | pipe | drop | continue | ok |
> goto chain <CHAIN_INDEX>
>
> Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
>
> ---
> v2 - fix whitespace issue in pkt_cls
> fix most warnings from checkpatch - some lines still over 80 chars
> due to long TLV names.
> v3 - fix some dangling else warnings.
> refactor stats printing to please checkpatch.
> send zone TLV even if default '0' zone.
> now checkpatch clean even though I think some of the formatting
> is horrible :-)
> sending via google's smtp 'cos MS' exchange office365 appears
> to mangle patches from git send-email.
> v4 - use the NEXT_ARG macros throughout.
> fix printing typo use 'cpmark' instead of 'mark'.
> use space separator between dscp mask & optional statemask and
> update usage as a result.
> validate dscp mask & statemask and print friendlier warnings
> than "invalid".
> fix cpmark option default value handling bug.
No further comments on this version; you should probably re-submit
without the RFC tag, though.
Feel free to add my
Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
when you do...
-Toke
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC iproute2-next v4] tc: add support for action act_ctinfo
2019-06-03 13:50 ` [PATCH RFC iproute2-next v4] " Kevin Darbyshire-Bryant
2019-06-03 20:14 ` Toke Høiland-Jørgensen
@ 2019-06-05 18:23 ` Joe Perches
2019-06-05 18:54 ` Stephen Hemminger
1 sibling, 1 reply; 10+ messages in thread
From: Joe Perches @ 2019-06-05 18:23 UTC (permalink / raw)
To: Kevin Darbyshire-Bryant; +Cc: netdev, stephen
On Mon, 2019-06-03 at 14:50 +0100, Kevin Darbyshire-Bryant wrote:
> ctinfo is an action restoring data stored in conntrack marks to various
> fields. At present it has two independent modes of operation,
> restoration of DSCP into IPv4/v6 diffserv and restoration of conntrack
> marks into packet skb marks.
[]
> v2 - fix whitespace issue in pkt_cls
> fix most warnings from checkpatch - some lines still over 80 chars
> due to long TLV names.
> v3 - fix some dangling else warnings.
> refactor stats printing to please checkpatch.
> send zone TLV even if default '0' zone.
> now checkpatch clean even though I think some of the formatting
> is horrible :-)
Strict 80 column limits with long identifiers are just silly.
I don't know how strictly enforced the iproute2 80 column limit
actually is, but I suggest ignoring that limit where appropriate.
e.g.:
> diff --git a/tc/m_ctinfo.c b/tc/m_ctinfo.c
[]
> +static int print_ctinfo(struct action_util *au, FILE *f, struct rtattr *arg)
> +{
[]
> + if (tb[TCA_CTINFO_PARMS_DSCP_MASK]) {
> + if (RTA_PAYLOAD(tb[TCA_CTINFO_PARMS_DSCP_MASK]) >=
> + sizeof(__u32))
I think code like this should just be single line.
> + if (tb[TCA_CTINFO_PARMS_DSCP_STATEMASK]) {
> + if (RTA_PAYLOAD(tb[TCA_CTINFO_PARMS_DSCP_STATEMASK]) >=
> + sizeof(__u32))
> + dscpstatemask = rta_getattr_u32(
> + tb[TCA_CTINFO_PARMS_DSCP_STATEMASK]);
here too, etc...
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC iproute2-next v4] tc: add support for action act_ctinfo
2019-06-05 18:23 ` Joe Perches
@ 2019-06-05 18:54 ` Stephen Hemminger
0 siblings, 0 replies; 10+ messages in thread
From: Stephen Hemminger @ 2019-06-05 18:54 UTC (permalink / raw)
To: Joe Perches; +Cc: Kevin Darbyshire-Bryant, netdev
On Wed, 05 Jun 2019 11:23:59 -0700
Joe Perches <joe@perches.com> wrote:
> Strict 80 column limits with long identifiers are just silly.
>
> I don't know how strictly enforced the iproute2 80 column limit
> actually is, but I suggest ignoring that limit where appropriate.
Not strictly enforced, but long identifiers are discouraged.
Overly deep nesting which is the main cause of long lines
is also discouraged.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-06-05 18:54 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-31 8:10 [PATCH RFC iproute2-next v2] tc: add support for act ctinfo ldir.kdb
2019-06-02 7:33 ` Kevin 'ldir' Darbyshire-Bryant
2019-06-02 18:50 ` [PATCH RFC iproute2-next v3] tc: add support for action act_ctinfo Kevin Darbyshire-Bryant
2019-06-02 20:39 ` Toke Høiland-Jørgensen
2019-06-02 21:55 ` Kevin 'ldir' Darbyshire-Bryant
2019-06-03 9:15 ` Toke Høiland-Jørgensen
2019-06-03 13:50 ` [PATCH RFC iproute2-next v4] " Kevin Darbyshire-Bryant
2019-06-03 20:14 ` Toke Høiland-Jørgensen
2019-06-05 18:23 ` Joe Perches
2019-06-05 18:54 ` Stephen Hemminger
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.