lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 26 Nov 2018 16:23:12 -0800
From:   "Nambiar, Amritha" <amritha.nambiar@...el.com>
To:     David Ahern <dsahern@...il.com>, stephen@...workplumber.org,
        netdev@...r.kernel.org
Cc:     jakub.kicinski@...ronome.com, sridhar.samudrala@...el.com,
        jhs@...atatu.com, xiyou.wangcong@...il.com, jiri@...nulli.us
Subject: Re: [iproute2-next PATCH v4] tc: flower: Classify packets based port
 ranges

On 11/21/2018 1:42 PM, David Ahern wrote:
> On 11/20/18 11:17 PM, Amritha Nambiar wrote:
>> diff --git a/tc/f_flower.c b/tc/f_flower.c
>> index 65fca04..722647d 100644
>> --- a/tc/f_flower.c
>> +++ b/tc/f_flower.c
>> @@ -494,6 +494,68 @@ static int flower_parse_port(char *str, __u8 ip_proto,
>>  	return 0;
>>  }
>>  
>> +static int flower_port_range_attr_type(__u8 ip_proto, enum flower_endpoint type,
>> +				       __be16 *min_port_type,
>> +				       __be16 *max_port_type)
>> +{
>> +	if (ip_proto == IPPROTO_TCP || ip_proto == IPPROTO_UDP ||
>> +	    ip_proto == IPPROTO_SCTP) {
>> +		if (type == FLOWER_ENDPOINT_SRC) {
>> +			*min_port_type = TCA_FLOWER_KEY_PORT_SRC_MIN;
>> +			*max_port_type = TCA_FLOWER_KEY_PORT_SRC_MAX;
>> +		} else {
>> +			*min_port_type = TCA_FLOWER_KEY_PORT_DST_MIN;
>> +			*max_port_type = TCA_FLOWER_KEY_PORT_DST_MAX;
>> +		}
>> +	} else {
>> +		return -1;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int flower_parse_port_range(__be16 *min, __be16 *max, __u8 ip_proto,
> 
> why not just min and max directly since they are not set here but only
> referenced by value. Also, you do not parse anything in this function so
> the helper is misnamed.
> 
> But I think this can be done simpler using what was done in ip/iprule.c ...
> 

Okay, will modify this.

> 
>> +				   enum flower_endpoint endpoint,
>> +				   struct nlmsghdr *n)
>> +{
>> +	__be16 min_port_type, max_port_type;
>> +
>> +	if (htons(*max) <= htons(*min)) {
>> +		fprintf(stderr, "max value should be greater than min value\n");
>> +		return -1;
>> +	}
>> +
>> +	if (flower_port_range_attr_type(ip_proto, endpoint, &min_port_type,
>> +					&max_port_type))
>> +		return -1;
>> +
>> +	addattr16(n, MAX_MSG, min_port_type, *min);
>> +	addattr16(n, MAX_MSG, max_port_type, *max);
>> +
>> +	return 0;
>> +}
>> +
>> +static int get_range(__be16 *min, __be16 *max, char *argv)
>> +{
>> +	char *r;
>> +
>> +	r = strchr(argv, '-');
>> +	if (r) {
>> +		*r = '\0';
>> +		if (get_be16(min, argv, 10)) {
>> +			fprintf(stderr, "invalid min range\n");
>> +			return -1;
>> +		}
>> +		if (get_be16(max, r + 1, 10)) {
>> +			fprintf(stderr, "invalid max range\n");
>> +			return -1;
>> +		}
>> +	} else {
>> +		return -1;
>> +	}
>> +	return 0;
>> +}
>> +
>>  #define TCP_FLAGS_MAX_MASK 0xfff
>>  
>>  static int flower_parse_tcp_flags(char *str, int flags_type, int mask_type,
>> @@ -1061,20 +1123,47 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,
>>  				return -1;
>>  			}
>>  		} else if (matches(*argv, "dst_port") == 0) {
>> +			__be16 min, max;
>> +
>>  			NEXT_ARG();
>> -			ret = flower_parse_port(*argv, ip_proto,
>> -						FLOWER_ENDPOINT_DST, n);
>> -			if (ret < 0) {
>> -				fprintf(stderr, "Illegal \"dst_port\"\n");
>> -				return -1;
>> +
>> +			if (!get_range(&min, &max, *argv)) {
>> +				ret = flower_parse_port_range(&min, &max,
>> +							      ip_proto,
>> +							      FLOWER_ENDPOINT_DST,
>> +							      n);
>> +				if (ret < 0) {
>> +					fprintf(stderr, "Illegal \"dst_port range\"\n");
>> +					return -1;
>> +				}
>> +			} else {
>> +				ret = flower_parse_port(*argv, ip_proto,
>> +							FLOWER_ENDPOINT_DST, n);
>> +				if (ret < 0) {
>> +					fprintf(stderr, "Illegal \"dst_port\"\n");
>> +					return -1;
>> +				}
> 
> Take a look at ip/iprule.c, line 921:
> 		} else if (strcmp(*argv, "sport") == 0) {
> 			...
> 		}
> 
> Using sscanf and handling the ret to be 1 or 2 should simplify the above.
> 

Okay, will simplify using sscanf.

>>  			}
>>  		} else if (matches(*argv, "src_port") == 0) {
>> +			__be16 min, max;
>> +
>>  			NEXT_ARG();
>> -			ret = flower_parse_port(*argv, ip_proto,
>> -						FLOWER_ENDPOINT_SRC, n);
>> -			if (ret < 0) {
>> -				fprintf(stderr, "Illegal \"src_port\"\n");
>> -				return -1;
>> +			if (!get_range(&min, &max, *argv)) {
>> +				ret = flower_parse_port_range(&min, &max,
>> +							      ip_proto,
>> +							      FLOWER_ENDPOINT_SRC,
>> +							      n);
>> +				if (ret < 0) {
>> +					fprintf(stderr, "Illegal \"src_port range\"\n");
>> +					return -1;
>> +				}
>> +			} else {
>> +				ret = flower_parse_port(*argv, ip_proto,
>> +							FLOWER_ENDPOINT_SRC, n);
>> +				if (ret < 0) {
>> +					fprintf(stderr, "Illegal \"src_port\"\n");
>> +					return -1;
>> +				}
>>  			}
>>  		} else if (matches(*argv, "tcp_flags") == 0) {
>>  			NEXT_ARG();
>> @@ -1490,6 +1579,22 @@ static void flower_print_port(char *name, struct rtattr *attr)
>>  	print_hu(PRINT_ANY, name, namefrm, rta_getattr_be16(attr));
>>  }
>>  
>> +static void flower_print_port_range(char *name, struct rtattr *min_attr,
>> +				    struct rtattr *max_attr)
>> +{
>> +	SPRINT_BUF(namefrm);
>> +	SPRINT_BUF(out);
>> +	size_t done;
>> +
>> +	if (!min_attr || !max_attr)
>> +		return;
>> +
>> +	done = sprintf(out, "%u", rta_getattr_be16(min_attr));
>> +	sprintf(out + done, "-%u", rta_getattr_be16(max_attr));
>> +	sprintf(namefrm, "\n  %s %%s", name);
>> +	print_string(PRINT_ANY, name, namefrm, out);
>> +}
>> +
>>  static void flower_print_tcp_flags(const char *name, struct rtattr *flags_attr,
>>  				   struct rtattr *mask_attr)
>>  {
>> @@ -1678,6 +1783,7 @@ static int flower_print_opt(struct filter_util *qu, FILE *f,
>>  			    struct rtattr *opt, __u32 handle)
>>  {
>>  	struct rtattr *tb[TCA_FLOWER_MAX + 1];
>> +	__be16 min_port_type, max_port_type;
>>  	int nl_type, nl_mask_type;
>>  	__be16 eth_type = 0;
>>  	__u8 ip_proto = 0xff;
>> @@ -1796,6 +1902,16 @@ static int flower_print_opt(struct filter_util *qu, FILE *f,
>>  	if (nl_type >= 0)
>>  		flower_print_port("src_port", tb[nl_type]);
>>  
>> +	if (!flower_port_range_attr_type(ip_proto, FLOWER_ENDPOINT_DST,
>> +					 &min_port_type, &max_port_type))
>> +		flower_print_port_range("dst_port range",
> 
> I am no json expert, but I do not recall any other place where a space
> is used in the name field for json output.
> 

Will remove the space and range keyword.

> Can tc flower use something similar to ip ru with single port or port
> range handled like this?
> 
>     },{
>         "priority": 32764,
>         "src": "172.16.1.0",
>         "srclen": 24,
>         "ipproto": "tcp",
>         "sport": 1100,
>         "table": "main"
>     },{
>         "priority": 32765,
>         "src": "172.16.1.0",
>         "srclen": 24,
>         "ipproto": "tcp",
>         "sport_start": 1000,
>         "sport_end": 1010,
>         "table": "main"
>     },{
> 
> 

Does it have to be separate fields "sport_start" and "sport_end"?
Removing the space and 'range' keyword will make the output format
consistent with the input format and print as "sport <number>" for
single port and "sport <start>-<end>" for range.
Example:

... flower ip_proto tcp src_port 12 skip_hw action will print as:
  ip_proto tcp
  src_port 12
  skip_hw
  not_in_hw
        action

... flower ip_proto tcp src_port 100-200 skip_hw action :
  ip_proto tcp
  src_port 100-200
  skip_hw
  not_in_hw
        action
>> +					tb[min_port_type], tb[max_port_type]);
>> +
>> +	if (!flower_port_range_attr_type(ip_proto, FLOWER_ENDPOINT_SRC,
>> +					 &min_port_type, &max_port_type))
>> +		flower_print_port_range("src_port range",
>> +					tb[min_port_type], tb[max_port_type]);
>> +
>>  	flower_print_tcp_flags("tcp_flags", tb[TCA_FLOWER_KEY_TCP_FLAGS],
>>  			       tb[TCA_FLOWER_KEY_TCP_FLAGS_MASK]);
>>  
>>
> 

Powered by blists - more mailing lists