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

[Paul Blakey <paulb@mellanox.com>]

On 7/8/2019 8:54 PM, Marcelo Ricardo Leitner wrote:
> 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.


In commit msg and here, it means there is multiple modes of operation. I 
think it's easier to split those.

"ct clear" to clear it , not other options can be added here.

"ct commit  [force].... " sends to conntrack and commit a connection, 
and only for commit can you specify force mark  label, and nat with 
nat_spec....

and the last one, "ct [nat] [zone ZONE]" is to just send the packet to 
conntrack on some zone [optional], restore nat [optional].


>
>> +		"	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.
which case? commit is optional. the above are the three valid patterns.
>> +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;
>> +}
> ...