| 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
| ||
|
Date: Mon, 5 Dec 2016 09:29:45 -0800
From: Tom Herbert <tom@...bertland.com>
To: Simon Horman <simon.horman@...ronome.com>
Cc: Jiri Pirko <jiri@...nulli.us>, David Miller <davem@...emloft.net>,
Linux Kernel Network Developers <netdev@...r.kernel.org>,
Jay Vosburgh <j.vosburgh@...il.com>,
Veaceslav Falico <vfalico@...il.com>,
Andy Gospodarek <andy@...yhouse.net>,
Jamal Hadi Salim <jhs@...atatu.com>,
Jiri Pirko <jiri@...lanox.com>
Subject: Re: [PATCH v2 net-next 1/2] flow dissector: ICMP support
On Mon, Dec 5, 2016 at 6:23 AM, Simon Horman <simon.horman@...ronome.com> wrote:
> On Sat, Dec 03, 2016 at 10:52:32AM -0800, Tom Herbert wrote:
>> On Sat, Dec 3, 2016 at 2:49 AM, Jiri Pirko <jiri@...nulli.us> wrote:
>> > Fri, Dec 02, 2016 at 09:31:41PM CET, simon.horman@...ronome.com wrote:
>> >>Allow dissection of ICMP(V6) type and code. This re-uses transport layer
>> >>port dissection code as although ICMP is not a transport protocol and their
>> >>type and code are not ports this allows sharing of both code and storage.
>> >>
>> >>Signed-off-by: Simon Horman <simon.horman@...ronome.com>
>
> ...
>
>> > Digging into this a bit more. I think it would be much nice not to mix
>> > up l4 ports and icmp stuff.
>> >
>> > How about to have FLOW_DISSECTOR_KEY_ICMP
>> > and
>> > struct flow_dissector_key_icmp {
>> > u8 type;
>> > u8 code;
>> > };
>> >
>> > The you can make this structure and struct flow_dissector_key_ports into
>> > an union in struct flow_keys.
>> >
>> > Looks much cleaner to me.
>
> Hi Jiri,
>
> I wondered about that too. The main reason that I didn't do this the first
> time around is that I wasn't sure what to do to differentiate between ports
> and icmp in fl_init_dissector() given that using a union would could to
> mask bits being set in both if they are set in either.
>
> I've taken a stab at addressing that below along with implementing your
> suggestion. If the treatment in fl_init_dissector() is acceptable
> perhaps something similar should be done for FLOW_DISSECTOR_KEY_IPV[46]_ADDRS
> too?
>
>> I agree, this patch adds to many conditionals into the fast path for
>> ICMP handling. Neither is there much point in using type and code as
>> input to the packet hash.
>
> Hi Tom,
>
> I'm not sure that I follow this. A packet may be ICMP or not regardless of
> if FLOW_DISSECTOR_KEY_PORT (and now FLOW_DISSECTOR_KEY_ICMP) port is set or
> not. I'd appreciate some guidance here.
>
>
> First-pass at implementing Jiri's suggestion follows:
>
> diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
> index 5540dfa18872..475cd121496f 100644
> --- a/include/net/flow_dissector.h
> +++ b/include/net/flow_dissector.h
> @@ -91,14 +91,10 @@ struct flow_dissector_key_addrs {
>
> /**
> * flow_dissector_key_ports:
> - * @ports: port numbers of Transport header or
> - * type and code of ICMP header
> + * @ports: port numbers of Transport header
> * ports: source (high) and destination (low) port numbers
> * src: source port number
> * dst: destination port number
> - * icmp: ICMP type (high) and code (low)
> - * type: ICMP type
> - * type: ICMP code
> */
> struct flow_dissector_key_ports {
> union {
> @@ -107,6 +103,18 @@ struct flow_dissector_key_ports {
> __be16 src;
> __be16 dst;
> };
> + };
> +};
> +
> +/**
> + * flow_dissector_key_icmp:
> + * @ports: type and code of ICMP header
> + * icmp: ICMP type (high) and code (low)
> + * type: ICMP type
> + * code: ICMP code
> + */
> +struct flow_dissector_key_icmp {
> + union {
> __be16 icmp;
> struct {
> u8 type;
> @@ -133,6 +141,7 @@ enum flow_dissector_key_id {
> FLOW_DISSECTOR_KEY_IPV4_ADDRS, /* struct flow_dissector_key_ipv4_addrs */
> FLOW_DISSECTOR_KEY_IPV6_ADDRS, /* struct flow_dissector_key_ipv6_addrs */
> FLOW_DISSECTOR_KEY_PORTS, /* struct flow_dissector_key_ports */
> + FLOW_DISSECTOR_KEY_ICMP, /* struct flow_dissector_key_icmp */
> FLOW_DISSECTOR_KEY_ETH_ADDRS, /* struct flow_dissector_key_eth_addrs */
> FLOW_DISSECTOR_KEY_TIPC_ADDRS, /* struct flow_dissector_key_tipc_addrs */
> FLOW_DISSECTOR_KEY_VLAN, /* struct flow_dissector_key_flow_vlan */
> @@ -171,7 +180,10 @@ struct flow_keys {
> struct flow_dissector_key_tags tags;
> struct flow_dissector_key_vlan vlan;
> struct flow_dissector_key_keyid keyid;
> - struct flow_dissector_key_ports ports;
> + union {
> + struct flow_dissector_key_ports ports;
> + struct flow_dissector_key_icmp icmp;
> + };
> struct flow_dissector_key_addrs addrs;
> };
>
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 0584b4bb4390..33e5fa2b3e87 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -139,6 +139,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
> struct flow_dissector_key_basic *key_basic;
> struct flow_dissector_key_addrs *key_addrs;
> struct flow_dissector_key_ports *key_ports;
> + struct flow_dissector_key_icmp *key_icmp;
> struct flow_dissector_key_tags *key_tags;
> struct flow_dissector_key_vlan *key_vlan;
> struct flow_dissector_key_keyid *key_keyid;
> @@ -559,18 +560,25 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
> break;
> }
>
> - if (dissector_uses_key(flow_dissector,
> - FLOW_DISSECTOR_KEY_PORTS)) {
> - key_ports = skb_flow_dissector_target(flow_dissector,
> - FLOW_DISSECTOR_KEY_PORTS,
> - target_container);
> - if (flow_protos_are_icmp_any(proto, ip_proto))
> - key_ports->icmp = skb_flow_get_be16(skb, nhoff, data,
> - hlen);
> - else
> + if (flow_protos_are_icmp_any(proto, ip_proto)) {
> + if (dissector_uses_key(flow_dissector,
> + FLOW_DISSECTOR_KEY_ICMP)) {
> + key_icmp = skb_flow_dissector_target(flow_dissector,
> + FLOW_DISSECTOR_KEY_ICMP,
> + target_container);
> + key_icmp->icmp = skb_flow_get_be16(skb, nhoff, data,
> + hlen);
> + }
> + } else {
> + if (dissector_uses_key(flow_dissector,
> + FLOW_DISSECTOR_KEY_PORTS)) {
> + key_ports = skb_flow_dissector_target(flow_dissector,
> + FLOW_DISSECTOR_KEY_PORTS,
> + target_container);
> key_ports->ports = __skb_flow_get_ports(skb, nhoff,
> ip_proto, data,
> hlen);
> + }
> }
>
> out_good:
> @@ -975,6 +983,10 @@ static const struct flow_dissector_key flow_keys_dissector_keys[] = {
> .offset = offsetof(struct flow_keys, ports),
> },
> {
> + .key_id = FLOW_DISSECTOR_KEY_ICMP,
> + .offset = offsetof(struct flow_keys, icmp),
> + },
This is the problem I was referring to. This adds ICMP to the keys for
computing the skb hash and the ICMP type and code are in a union with
port numbers so they are in the range over which the hash is computed.
This means that we are treating type and code as some sort of flow and
and so different type/code between the same src/dst could follow
different paths in ECMP. For the purposes of computing a packet hash I
don't think we want ICMP information included. This might be a matter
of not putting FLOW_DISSECTOR_KEY_ICMP in flow_keys_dissector_keys,
where we need this information we could define another structure that
has all the same fields as in flow_keys_dissector_keys and include
FLOW_DISSECTOR_KEY_ICMP. Alternatively, the flow_dissector_key_icmp
could also be outside of the area that is used in the hash: that is no
in flow_keys_hash_start(keys) to flow_keys_hash_length(keys), keyval);
> + {
> .key_id = FLOW_DISSECTOR_KEY_VLAN,
> .offset = offsetof(struct flow_keys, vlan),
> },
> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> index c96b2a349779..f4ba69bd362f 100644
> --- a/net/sched/cls_flower.c
> +++ b/net/sched/cls_flower.c
> @@ -38,7 +38,10 @@ struct fl_flow_key {
> struct flow_dissector_key_ipv4_addrs ipv4;
> struct flow_dissector_key_ipv6_addrs ipv6;
> };
> - struct flow_dissector_key_ports tp;
> + union {
> + struct flow_dissector_key_ports tp;
> + struct flow_dissector_key_icmp icmp;
> + };
When an ICMP packet is encapsulated within UDP then icmp overwrites
valid port information with is a stronger signal of the flow (unless
FLOW_DISSECTOR_F_STOP_AT_ENCAP is set). This is another reason not to
use a union with ports.
Tom
> struct flow_dissector_key_keyid enc_key_id;
> union {
> struct flow_dissector_key_ipv4_addrs enc_ipv4;
> @@ -511,19 +514,23 @@ static int fl_set_key(struct net *net, struct nlattr **tb,
> &mask->tp.dst, TCA_FLOWER_KEY_SCTP_DST_MASK,
> sizeof(key->tp.dst));
> } else if (flow_basic_key_is_icmpv4(&key->basic)) {
> - fl_set_key_val(tb, &key->tp.type, TCA_FLOWER_KEY_ICMPV4_TYPE,
> - &mask->tp.type, TCA_FLOWER_KEY_ICMPV4_TYPE_MASK,
> - sizeof(key->tp.type));
> - fl_set_key_val(tb, &key->tp.code, TCA_FLOWER_KEY_ICMPV4_CODE,
> - &mask->tp.code, TCA_FLOWER_KEY_ICMPV4_CODE_MASK,
> - sizeof(key->tp.code));
> + fl_set_key_val(tb, &key->icmp.type, TCA_FLOWER_KEY_ICMPV4_TYPE,
> + &mask->icmp.type,
> + TCA_FLOWER_KEY_ICMPV4_TYPE_MASK,
> + sizeof(key->icmp.type));
> + fl_set_key_val(tb, &key->icmp.code, TCA_FLOWER_KEY_ICMPV4_CODE,
> + &mask->icmp.code,
> + TCA_FLOWER_KEY_ICMPV4_CODE_MASK,
> + sizeof(key->icmp.code));
> } else if (flow_basic_key_is_icmpv6(&key->basic)) {
> - fl_set_key_val(tb, &key->tp.type, TCA_FLOWER_KEY_ICMPV6_TYPE,
> - &mask->tp.type, TCA_FLOWER_KEY_ICMPV6_TYPE_MASK,
> - sizeof(key->tp.type));
> - fl_set_key_val(tb, &key->tp.code, TCA_FLOWER_KEY_ICMPV4_CODE,
> - &mask->tp.code, TCA_FLOWER_KEY_ICMPV4_CODE_MASK,
> - sizeof(key->tp.code));
> + fl_set_key_val(tb, &key->icmp.type, TCA_FLOWER_KEY_ICMPV6_TYPE,
> + &mask->icmp.type,
> + TCA_FLOWER_KEY_ICMPV6_TYPE_MASK,
> + sizeof(key->icmp.type));
> + fl_set_key_val(tb, &key->icmp.code, TCA_FLOWER_KEY_ICMPV4_CODE,
> + &mask->icmp.code,
> + TCA_FLOWER_KEY_ICMPV4_CODE_MASK,
> + sizeof(key->icmp.code));
> }
>
> if (tb[TCA_FLOWER_KEY_ENC_IPV4_SRC] ||
> @@ -609,15 +616,16 @@ static int fl_init_hashtable(struct cls_fl_head *head,
> keys[cnt].key_id = id; \
> keys[cnt].offset = FL_KEY_MEMBER_OFFSET(member); \
> cnt++; \
> - } while(0);
> + } while(0)
>
> #define FL_KEY_SET_IF_MASKED(mask, keys, cnt, id, member) \
> do { \
> if (FL_KEY_IS_MASKED(mask, member)) \
> FL_KEY_SET(keys, cnt, id, member); \
> - } while(0);
> + } while(0)
>
> static void fl_init_dissector(struct cls_fl_head *head,
> + struct cls_fl_filter *f,
> struct fl_flow_mask *mask)
> {
> struct flow_dissector_key keys[FLOW_DISSECTOR_KEY_MAX];
> @@ -631,8 +639,12 @@ static void fl_init_dissector(struct cls_fl_head *head,
> FLOW_DISSECTOR_KEY_IPV4_ADDRS, ipv4);
> FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
> FLOW_DISSECTOR_KEY_IPV6_ADDRS, ipv6);
> - FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
> - FLOW_DISSECTOR_KEY_PORTS, tp);
> + if (flow_basic_key_is_icmpv4(&f->key.basic))
> + FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
> + FLOW_DISSECTOR_KEY_ICMP, icmp);
> + else
> + FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
> + FLOW_DISSECTOR_KEY_PORTS, tp);
> FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
> FLOW_DISSECTOR_KEY_VLAN, vlan);
> FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
> @@ -652,6 +664,7 @@ static void fl_init_dissector(struct cls_fl_head *head,
> }
>
> static int fl_check_assign_mask(struct cls_fl_head *head,
> + struct cls_fl_filter *f,
> struct fl_flow_mask *mask)
> {
> int err;
> @@ -672,7 +685,7 @@ static int fl_check_assign_mask(struct cls_fl_head *head,
> memcpy(&head->mask, mask, sizeof(head->mask));
> head->mask_assigned = true;
>
> - fl_init_dissector(head, mask);
> + fl_init_dissector(head, f, mask);
>
> return 0;
> }
> @@ -785,7 +798,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
> if (err)
> goto errout;
>
> - err = fl_check_assign_mask(head, &mask);
> + err = fl_check_assign_mask(head, fnew, &mask);
> if (err)
> goto errout;
>
> @@ -1000,24 +1013,24 @@ static int fl_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
> sizeof(key->tp.dst))))
> goto nla_put_failure;
> else if (flow_basic_key_is_icmpv4(&key->basic) &&
> - (fl_dump_key_val(skb, &key->tp.type,
> - TCA_FLOWER_KEY_ICMPV4_TYPE, &mask->tp.type,
> + (fl_dump_key_val(skb, &key->icmp.type,
> + TCA_FLOWER_KEY_ICMPV4_TYPE, &mask->icmp.type,
> TCA_FLOWER_KEY_ICMPV4_TYPE_MASK,
> - sizeof(key->tp.type)) ||
> - fl_dump_key_val(skb, &key->tp.code,
> - TCA_FLOWER_KEY_ICMPV4_CODE, &mask->tp.code,
> + sizeof(key->icmp.type)) ||
> + fl_dump_key_val(skb, &key->icmp.code,
> + TCA_FLOWER_KEY_ICMPV4_CODE, &mask->icmp.code,
> TCA_FLOWER_KEY_ICMPV4_CODE_MASK,
> - sizeof(key->tp.code))))
> + sizeof(key->icmp.code))))
> goto nla_put_failure;
> else if (flow_basic_key_is_icmpv6(&key->basic) &&
> - (fl_dump_key_val(skb, &key->tp.type,
> - TCA_FLOWER_KEY_ICMPV6_TYPE, &mask->tp.type,
> + (fl_dump_key_val(skb, &key->icmp.type,
> + TCA_FLOWER_KEY_ICMPV6_TYPE, &mask->icmp.type,
> TCA_FLOWER_KEY_ICMPV6_TYPE_MASK,
> - sizeof(key->tp.type)) ||
> - fl_dump_key_val(skb, &key->tp.code,
> - TCA_FLOWER_KEY_ICMPV6_CODE, &mask->tp.code,
> + sizeof(key->icmp.type)) ||
> + fl_dump_key_val(skb, &key->icmp.code,
> + TCA_FLOWER_KEY_ICMPV6_CODE, &mask->icmp.code,
> TCA_FLOWER_KEY_ICMPV6_CODE_MASK,
> - sizeof(key->tp.code))))
> + sizeof(key->icmp.code))))
> goto nla_put_failure;
>
> if (key->enc_control.addr_type == FLOW_DISSECTOR_KEY_IPV4_ADDRS &&
Powered by blists - more mailing lists