Message-ID: <87a7eziuxi.fsf@toke.dk>
Date:   Sun, 02 Jun 2019 22:39:05 +0200
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     Kevin Darbyshire-Bryant <ldir@...byshire-bryant.me.uk>,
        netdev@...r.kernel.org
Cc:     ldir@...byshire-bryant.me.uk, stephen@...workplumber.org
Subject: Re: [PATCH RFC iproute2-next v3] tc: add support for action act_ctinfo

Kevin Darbyshire-Bryant <ldir@...byshire-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@...byshire-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@...byshire-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)

Powered by blists - more mailing lists