Re: [PATCH net-next iproute2 2/3] tc: Introduce tc ct action

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

On Sun, Jul 07, 2019 at 11:53:47AM +0300, Paul Blakey wrote:
> New tc action to send packets to conntrack module, commit
> them, and set a zone, labels, mark, and nat on the connection.
> 
> It can also clear the packet's conntrack state by using clear.
> 
> Usage:
>    ct clear
>    ct commit [force] [zone] [mark] [label] [nat]

Isn't the 'commit' also optional? More like
    ct [commit [force]] [zone] [mark] [label] [nat]

>    ct [nat] [zone]
> 
> Signed-off-by: Paul Blakey <paulb@mellanox.com>
> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> Signed-off-by: Yossi Kuperman <yossiku@mellanox.com>
> Acked-by: Jiri Pirko <jiri@mellanox.com>
> Acked-by: Roi Dayan <roid@mellanox.com>
> ---
...
> +static void
> +usage(void)
> +{
> +	fprintf(stderr,
> +		"Usage: ct clear\n"
> +		"	ct commit [force] [zone ZONE] [mark MASKED_MARK] [label MASKED_LABEL] [nat NAT_SPEC]\n"

Ditto here then.

> +		"	ct [nat] [zone ZONE]\n"
> +		"Where: ZONE is the conntrack zone table number\n"
> +		"	NAT_SPEC is {src|dst} addr addr1[-addr2] [port port1[-port2]]\n"
> +		"\n");
> +	exit(-1);
> +}
...

The validation below doesn't enforce that commit must be there for
such case.

> +static int
> +parse_ct(struct action_util *a, int *argc_p, char ***argv_p, int tca_id,
> +		struct nlmsghdr *n)
> +{
> +	struct tc_ct sel = {};
> +	char **argv = *argv_p;
> +	struct rtattr *tail;
> +	int argc = *argc_p;
> +	int ct_action = 0;
> +	int ret;
> +
> +	tail = addattr_nest(n, MAX_MSG, tca_id);
> +
> +	if (argc && matches(*argv, "ct") == 0)
> +		NEXT_ARG_FWD();
> +
> +	while (argc > 0) {
> +		if (matches(*argv, "zone") == 0) {
> +			NEXT_ARG();
> +
> +			if (ct_parse_u16(*argv,
> +					 TCA_CT_ZONE, TCA_CT_UNSPEC, n)) {
> +				fprintf(stderr, "ct: Illegal \"zone\"\n");
> +				return -1;
> +			}
> +		} else if (matches(*argv, "nat") == 0) {
> +			ct_action |= TCA_CT_ACT_NAT;
> +
> +			NEXT_ARG();
> +			if (matches(*argv, "src") == 0)
> +				ct_action |= TCA_CT_ACT_NAT_SRC;
> +			else if (matches(*argv, "dst") == 0)
> +				ct_action |= TCA_CT_ACT_NAT_DST;
> +			else
> +				continue;
> +
> +			NEXT_ARG();
> +			if (matches(*argv, "addr") != 0)
> +				usage();
> +
> +			NEXT_ARG();
> +			ret = ct_parse_nat_addr_range(*argv, n);
> +			if (ret) {
> +				fprintf(stderr, "ct: Illegal nat address range\n");
> +				return -1;
> +			}
> +
> +			NEXT_ARG_FWD();
> +			if (matches(*argv, "port") != 0)
> +				continue;
> +
> +			NEXT_ARG();
> +			ret = ct_parse_nat_port_range(*argv, n);
> +			if (ret) {
> +				fprintf(stderr, "ct: Illegal nat port range\n");
> +				return -1;
> +			}
> +		} else if (matches(*argv, "clear") == 0) {
> +			ct_action |= TCA_CT_ACT_CLEAR;
> +		} else if (matches(*argv, "commit") == 0) {
> +			ct_action |= TCA_CT_ACT_COMMIT;
> +		} else if (matches(*argv, "force") == 0) {
> +			ct_action |= TCA_CT_ACT_FORCE;
> +		} else if (matches(*argv, "index") == 0) {
> +			NEXT_ARG();
> +			if (get_u32(&sel.index, *argv, 10)) {
> +				fprintf(stderr, "ct: Illegal \"index\"\n");
> +				return -1;
> +			}
> +		} else if (matches(*argv, "mark") == 0) {
> +			NEXT_ARG();
> +
> +			ret = ct_parse_mark(*argv, n);
> +			if (ret) {
> +				fprintf(stderr, "ct: Illegal \"mark\"\n");
> +				return -1;
> +			}
> +		} else if (matches(*argv, "label") == 0) {
> +			NEXT_ARG();
> +
> +			ret = ct_parse_labels(*argv, n);
> +			if (ret) {
> +				fprintf(stderr, "ct: Illegal \"label\"\n");
> +				return -1;
> +			}
> +		} else if (matches(*argv, "help") == 0) {
> +			usage();
> +		} else {
> +			break;
> +		}
> +		NEXT_ARG_FWD();
> +	}
> +
> +	if (ct_action & TCA_CT_ACT_CLEAR &&
> +	    ct_action & ~TCA_CT_ACT_CLEAR) {
> +		fprintf(stderr, "ct: clear can only be used alone\n");
> +		return -1;
> +	}
> +
> +	if (ct_action & TCA_CT_ACT_NAT_SRC &&
> +	    ct_action & TCA_CT_ACT_NAT_DST) {
> +		fprintf(stderr, "ct: src and dst nat can't be used together\n");
> +		return -1;
> +	}
> +
> +	if ((ct_action & TCA_CT_ACT_COMMIT) &&
> +	    (ct_action & TCA_CT_ACT_NAT) &&
> +	    !(ct_action & (TCA_CT_ACT_NAT_SRC | TCA_CT_ACT_NAT_DST))) {
> +		fprintf(stderr, "ct: commit and nat must set src or dst\n");
> +		return -1;
> +	}
> +
> +	if (!(ct_action & TCA_CT_ACT_COMMIT) &&
> +	    (ct_action & (TCA_CT_ACT_NAT_SRC | TCA_CT_ACT_NAT_DST))) {
> +		fprintf(stderr, "ct: src or dst is only valid if commit is set\n");
> +		return -1;
> +	}
> +
> +	parse_action_control_dflt(&argc, &argv, &sel.action, false,
> +				  TC_ACT_PIPE);
> +	NEXT_ARG_FWD();
> +
> +	addattr16(n, MAX_MSG, TCA_CT_ACTION, ct_action);
> +	addattr_l(n, MAX_MSG, TCA_CT_PARMS, &sel, sizeof(sel));
> +	addattr_nest_end(n, tail);
> +
> +	*argc_p = argc;
> +	*argv_p = argv;
> +	return 0;
> +}
...