Re: [dpdk-dev] [PATCH v2 2/2] examples/flow_classify: add an ACL table for tcp

["Iremonger, Bernard" <bernard.iremonger@intel.com>]

Hi Sowmini,

> -----Original Message-----
> From: Sowmini Varadhan <sovaradh@linux.microsoft.com>
> Sent: Wednesday, August 18, 2021 4:01 PM
> To: sowmini05@gmail.com; Iremonger, Bernard
> <bernard.iremonger@intel.com>; dev@dpdk.org;
> sovaradh@linux.microsoft.com
> Cc: thomas@monjalon.net
> Subject: [PATCH v2 2/2] examples/flow_classify: add an ACL table for tcp
> 
> v2 updates: typo fixes for checkpatch,  bernard.iremonger comments

The above line should not be added to the commit message.
~/dpdk/devtools/check-git-log.sh -1
Wrong tag:
        v2 fixes:  typo fixes, get_tcp_flags returns -EINVAL

The check-git-log.sh  script is in your dpdk checkout, best to run this script before sending patches

> 
> The struct rte_flow_classifier can have up to
> RTE_FLOW_CLASSIFY_TABLE_MAX
> (32) classifier tables, but the existing flow_classify examples only adds a
> single table for the L4 5-tuple.
> 
> When dealing with tcp flows, we frequently want to add ACLs and filters to
> filter based on the state of the TCP connection, e.g., by looking at the tcp
> flags field.
> 
> So we enhance flow_classify to add an additional acl table for tcp 5-tuples. If
> the input-file-parser detects a filter for a tcp flow with a non-wildcard
> argument for tcp_flags, the IP4_TCP_5TUPLE table is used by flow_classify.
> 
> Signed-off-by: Sowmini Varadhan <sovaradh@linux.microsoft.com>
> ---

If you want comment on the patch updates (optional), it can be added here after the --- line

>  examples/flow_classify/flow_classify.c      | 41 +++++++---
>  lib/flow_classify/rte_flow_classify.c       | 87 +++++++++++++++++++++
>  lib/flow_classify/rte_flow_classify.h       | 19 +++++
>  lib/flow_classify/rte_flow_classify_parse.c |  8 +-
>  4 files changed, 142 insertions(+), 13 deletions(-)

This patch contains changes to the flow_classify example and the flow_classify  library.
It needs to be split in two, a patch for the flow_classify example and a patch for the flow classify library.

> 
> diff --git a/examples/flow_classify/flow_classify.c
> b/examples/flow_classify/flow_classify.c
> index ff4c8bc2f5..8f6aacae03 100644
> --- a/examples/flow_classify/flow_classify.c
> +++ b/examples/flow_classify/flow_classify.c
> @@ -723,11 +723,6 @@ add_classify_rule(struct rte_eth_ntuple_filter
> *ntuple_filter,
>  		return ret;
>  	}
> 
> -	/* XXX but this only adds table_type of
> -	 * RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_5TUPLE
> -	 * i.e., it only ever does allocate_acl_ipv4_5tuple_rule()
> -	 * so the tcp_flags is ignored!
> -	 */
>  	rule = rte_flow_classify_table_entry_add(
>  			cls_app->cls, &attr, pattern_ipv4_5tuple,
>  			actions, &key_found, &error);
> @@ -856,7 +851,8 @@ main(int argc, char *argv[])
>  	int ret;
>  	int socket_id;
>  	struct rte_table_acl_params table_acl_params;
> -	struct rte_flow_classify_table_params cls_table_params;
> +	struct rte_table_acl_params table_acl_tcp_params;
> +	struct rte_flow_classify_table_params cls_table_params[2];
>  	struct flow_classifier *cls_app;
>  	struct rte_flow_classifier_params cls_params;
>  	uint32_t size;
> @@ -923,21 +919,42 @@ main(int argc, char *argv[])
>  	memcpy(table_acl_params.field_format, ipv4_defs,
> sizeof(ipv4_defs));
> 
>  	/* initialise table create params */
> -	cls_table_params.ops = &rte_table_acl_ops;
> -	cls_table_params.arg_create = &table_acl_params;
> -	cls_table_params.type =
> RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_5TUPLE;
> +	cls_table_params[0].ops = &rte_table_acl_ops;
> +	cls_table_params[0].arg_create = &table_acl_params;
> +	cls_table_params[0].type =
> RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_5TUPLE;
> +
> +	/* initialise ACL table params */
> +	table_acl_tcp_params.name = "table_acl_ipv4_tcp_5tuple";
> +	table_acl_tcp_params.n_rules = FLOW_CLASSIFY_MAX_RULE_NUM;
> +	table_acl_tcp_params.n_rule_fields = RTE_DIM(ipv4_defs);
> +	memcpy(table_acl_tcp_params.field_format, ipv4_defs,
> +sizeof(ipv4_defs));
> +
> +	/* initialise table create params */
> +	cls_table_params[1].ops = &rte_table_acl_ops;
> +	cls_table_params[1].arg_create = &table_acl_tcp_params;
> +	cls_table_params[1].type =
> RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_TCP_5TUPLE;
> 
> -	ret = rte_flow_classify_table_create(cls_app->cls,
> &cls_table_params);
> +	ret = rte_flow_classify_table_create(cls_app->cls,
> +					     &cls_table_params[0]);
>  	if (ret) {
>  		rte_flow_classifier_free(cls_app->cls);
>  		rte_free(cls_app);
>  		rte_exit(EXIT_FAILURE, "Failed to create classifier table\n");
>  	}
> +	ret = rte_flow_classify_table_create(cls_app->cls,
> +					     &cls_table_params[1]);
> +	if (ret) {
> +		rte_flow_classifier_free(cls_app->cls);
> +		rte_free(cls_app);
> +		rte_exit(EXIT_FAILURE,
> +			 "Failed to create classifier table\n");
> +	}
> +
>  	/* >8 End of initialization of table create params. */
> 
>  	/* read file of IPv4 5 tuple rules and initialize parameters
> -	 * for rte_flow_classify_validate and
> rte_flow_classify_table_entry_add
> -	 * API's.
> +	 * for rte_flow_classify_validate and
> +	 * rte_flow_classify_table_entry_add  API's.
>  	 */
> 
>  	/* Read file of IPv4 tuple rules. 8< */ diff --git
> a/lib/flow_classify/rte_flow_classify.c b/lib/flow_classify/rte_flow_classify.c
> index d3ba2ed227..e960c3b140 100644
> --- a/lib/flow_classify/rte_flow_classify.c
> +++ b/lib/flow_classify/rte_flow_classify.c
> @@ -60,6 +60,7 @@ enum {
>  	DST_FIELD_IPV4,
>  	SRCP_FIELD_IPV4,
>  	DSTP_FIELD_IPV4,
> +	TCP_FLAGS_FIELD,

I think it would be better to rename TCP_FLAGS_FIELD to FLAGS_FIELD_TCP in line with the other values.

>  	NUM_FIELDS_IPV4
>  };
> 
> @@ -72,6 +73,7 @@ struct classify_rules {
>  	enum rte_flow_classify_rule_type type;
>  	union {
>  		struct rte_flow_classify_ipv4_5tuple ipv4_5tuple;
> +		struct rte_flow_classify_ipv4_tcp_5tuple ipv4_tcp_5tuple;
>  	} u;
>  };
> 
> @@ -477,6 +479,84 @@ allocate_acl_ipv4_5tuple_rule(struct
> rte_flow_classifier *cls)
>  	return rule;
>  }
> 
> +static struct rte_flow_classify_rule *
> +allocate_acl_ipv4_tcp_5tuple_rule(struct rte_flow_classifier *cls) {
> +	struct rte_flow_classify_rule *rule;
> +	int log_level;
> +
> +	rule = malloc(sizeof(struct rte_flow_classify_rule));
> +	if (!rule)
> +		return rule;
> +
> +	memset(rule, 0, sizeof(struct rte_flow_classify_rule));
> +	rule->id = unique_id++;
> +	rule->rules.type =
> RTE_FLOW_CLASSIFY_RULE_TYPE_IPV4_TCP_5TUPLE;
> +
> +	/* key add values */
> +	rule->u.key.key_add.priority = cls->ntuple_filter.priority;
> +	rule-
> >u.key.key_add.field_value[PROTO_FIELD_IPV4].mask_range.u8 =
> +			cls->ntuple_filter.proto_mask;
> +	rule->u.key.key_add.field_value[PROTO_FIELD_IPV4].value.u8 =
> +			cls->ntuple_filter.proto;
> +	rule->rules.u.ipv4_tcp_5tuple.proto = cls->ntuple_filter.proto;
> +	rule->rules.u.ipv4_tcp_5tuple.proto_mask =
> +			cls->ntuple_filter.proto_mask;
> +
> +	rule->u.key.key_add.field_value[SRC_FIELD_IPV4].mask_range.u32
> =
> +			cls->ntuple_filter.src_ip_mask;
> +	rule->u.key.key_add.field_value[SRC_FIELD_IPV4].value.u32 =
> +			cls->ntuple_filter.src_ip;
> +	rule->rules.u.ipv4_tcp_5tuple.src_ip_mask =
> +			cls->ntuple_filter.src_ip_mask;
> +	rule->rules.u.ipv4_tcp_5tuple.src_ip = cls->ntuple_filter.src_ip;
> +
> +	rule->u.key.key_add.field_value[DST_FIELD_IPV4].mask_range.u32
> =
> +			cls->ntuple_filter.dst_ip_mask;
> +	rule->u.key.key_add.field_value[DST_FIELD_IPV4].value.u32 =
> +			cls->ntuple_filter.dst_ip;
> +	rule->rules.u.ipv4_tcp_5tuple.dst_ip_mask =
> +			cls->ntuple_filter.dst_ip_mask;
> +	rule->rules.u.ipv4_tcp_5tuple.dst_ip = cls->ntuple_filter.dst_ip;
> +
> +	rule-
> >u.key.key_add.field_value[SRCP_FIELD_IPV4].mask_range.u16 =
> +			cls->ntuple_filter.src_port_mask;
> +	rule->u.key.key_add.field_value[SRCP_FIELD_IPV4].value.u16 =
> +			cls->ntuple_filter.src_port;
> +	rule->rules.u.ipv4_tcp_5tuple.src_port_mask =
> +			cls->ntuple_filter.src_port_mask;
> +	rule->rules.u.ipv4_tcp_5tuple.src_port = cls->ntuple_filter.src_port;
> +
> +	rule-
> >u.key.key_add.field_value[DSTP_FIELD_IPV4].mask_range.u16 =
> +			cls->ntuple_filter.dst_port_mask;
> +	rule->u.key.key_add.field_value[DSTP_FIELD_IPV4].value.u16 =
> +			cls->ntuple_filter.dst_port;
> +	rule->rules.u.ipv4_tcp_5tuple.dst_port_mask =
> +			cls->ntuple_filter.dst_port_mask;
> +	rule->rules.u.ipv4_tcp_5tuple.dst_port = cls->ntuple_filter.dst_port;
> +
> +	rule-
> >u.key.key_add.field_value[TCP_FLAGS_FIELD].mask_range.u32 =
> +			rte_be_to_cpu_32(0xff);
> +	rule->u.key.key_add.field_value[TCP_FLAGS_FIELD].value.u32 =
> +			rte_be_to_cpu_32(cls->ntuple_filter.tcp_flags);
> +	rule->rules.u.ipv4_tcp_5tuple.tcp_flags =
> +cls->ntuple_filter.tcp_flags;
> +
> +	log_level = rte_log_get_level(librte_flow_classify_logtype);
> +
> +	if (log_level == RTE_LOG_DEBUG)
> +		print_acl_ipv4_key_add(&rule->u.key.key_add);
> +
> +	/* key delete values */
> +	memcpy(&rule->u.key.key_del.field_value[PROTO_FIELD_IPV4],
> +	       &rule->u.key.key_add.field_value[PROTO_FIELD_IPV4],
> +	       NUM_FIELDS_IPV4 * sizeof(struct rte_acl_field));
> +
> +	if (log_level == RTE_LOG_DEBUG)
> +		print_acl_ipv4_key_delete(&rule->u.key.key_del);
> +
> +	return rule;
> +}
> +
>  struct rte_flow_classify_rule *
>  rte_flow_classify_table_entry_add(struct rte_flow_classifier *cls,
>  		const struct rte_flow_attr *attr,
> @@ -514,6 +594,13 @@ rte_flow_classify_table_entry_add(struct
> rte_flow_classifier *cls,
>  		rule->tbl_type = table_type;
>  		cls->table_mask |= table_type;
>  		break;
> +	case RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_TCP_5TUPLE:
> +		rule = allocate_acl_ipv4_tcp_5tuple_rule(cls);
> +		if (!rule)
> +			return NULL;
> +		rule->tbl_type = table_type;
> +		cls->table_mask |= table_type;
> +		break;
>  	default:
>  		return NULL;
>  	}
> diff --git a/lib/flow_classify/rte_flow_classify.h
> b/lib/flow_classify/rte_flow_classify.h
> index 82ea92b6a6..65585d9f92 100644
> --- a/lib/flow_classify/rte_flow_classify.h
> +++ b/lib/flow_classify/rte_flow_classify.h
> @@ -80,6 +80,8 @@ enum rte_flow_classify_rule_type {
>  	RTE_FLOW_CLASSIFY_RULE_TYPE_NONE,
>  	/** IPv4 5tuple type */
>  	RTE_FLOW_CLASSIFY_RULE_TYPE_IPV4_5TUPLE,
> +	/** IPv4 TCP 5tuple type */
> +	RTE_FLOW_CLASSIFY_RULE_TYPE_IPV4_TCP_5TUPLE,
>  };
> 
>  /** Flow classify table type */
> @@ -92,6 +94,8 @@ enum rte_flow_classify_table_type {
>  	RTE_FLOW_CLASSIFY_TABLE_ACL_VLAN_IP4_5TUPLE = 1 << 2,
>  	/** ACL QinQ IP4 5TUPLE */
>  	RTE_FLOW_CLASSIFY_TABLE_ACL_QINQ_IP4_5TUPLE = 1 << 3,
> +	/** ACL IP4 5TUPLE with tcp_flags */
> +	RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_TCP_5TUPLE = 1 << 4,
> 
>  };
> 
> @@ -131,6 +135,21 @@ struct rte_flow_classify_ipv4_5tuple {
>  	uint8_t proto_mask;      /**< Mask of L4 protocol. */
>  };
> 
> +/** IPv4 5-tuple data with tcp flags*/
> +struct rte_flow_classify_ipv4_tcp_5tuple {
> +	uint32_t dst_ip;         /**< Destination IP address in big endian. */
> +	uint32_t dst_ip_mask;    /**< Mask of destination IP address. */
> +	uint32_t src_ip;         /**< Source IP address in big endian. */
> +	uint32_t src_ip_mask;    /**< Mask of destination IP address. */
> +	uint16_t dst_port;       /**< Destination port in big endian. */
> +	uint16_t dst_port_mask;  /**< Mask of destination port. */
> +	uint16_t src_port;       /**< Source Port in big endian. */
> +	uint16_t src_port_mask;  /**< Mask of source port. */
> +	uint8_t proto;           /**< L4 protocol. */
> +	uint8_t proto_mask;      /**< Mask of L4 protocol. */
> +	uint8_t tcp_flags;       /**< Tcp only */
> +};
> +
>  /**
>   * Flow stats
>   *
> diff --git a/lib/flow_classify/rte_flow_classify_parse.c
> b/lib/flow_classify/rte_flow_classify_parse.c
> index 465330291f..fe4ee05b6f 100644
> --- a/lib/flow_classify/rte_flow_classify_parse.c
> +++ b/lib/flow_classify/rte_flow_classify_parse.c
> @@ -216,6 +216,7 @@ classify_parse_ntuple_filter(const struct
> rte_flow_attr *attr,
>  	const struct rte_flow_action_count *count;
>  	const struct rte_flow_action_mark *mark_spec;
>  	uint32_t index;
> +	bool have_tcp = false;
> 
>  	/* parse pattern */
>  	index = 0;
> @@ -375,6 +376,8 @@ classify_parse_ntuple_filter(const struct
> rte_flow_attr *attr,
>  		filter->dst_port  = tcp_spec->hdr.dst_port;
>  		filter->src_port  = tcp_spec->hdr.src_port;
>  		filter->tcp_flags = tcp_spec->hdr.tcp_flags;
> +		if (filter->tcp_flags != 0)
> +			have_tcp = true;
>  	} else if (item->type == RTE_FLOW_ITEM_TYPE_UDP) {
>  		udp_mask = item->mask;
> 
> @@ -434,7 +437,10 @@ classify_parse_ntuple_filter(const struct
> rte_flow_attr *attr,
>  		return -EINVAL;
>  	}
> 
> -	table_type = RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_5TUPLE;
> +	if (have_tcp)
> +		table_type =
> RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_TCP_5TUPLE;
> +	else
> +		table_type = RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_5TUPLE;
> 
>  	/* parse attr */
>  	/* must be input direction */
> --
> 2.17.1

Regards,

Bernard.