netdev - Re: [patch net-next v4 RFC 12/15] flow_dissector: introduce support for ipv6 addressses

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 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:	Sat, 25 Apr 2015 21:17:02 +0200
From:	Jiri Pirko <jiri@...nulli.us>
To:	Tom Herbert <tom@...bertland.com>
Cc:	Linux Kernel Network Developers <netdev@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Jamal Hadi Salim <jhs@...atatu.com>,
	Thomas Graf <tgraf@...g.ch>, jesse@...ira.com, kaber@...sh.net,
	Tom Herbert <therbert@...gle.com>, edumazet@...gle.com,
	alexander.h.duyck@...hat.com,
	Hannes Frederic Sowa <hannes@...essinduktion.org>,
	ast@...mgrid.com, daniel@...earbox.net,
	herbert@...dor.apana.org.au, cwang@...pensource.com,
	john.fastabend@...il.com
Subject: Re: [patch net-next v4 RFC 12/15] flow_dissector: introduce support
 for ipv6 addressses

Fri, Apr 24, 2015 at 07:28:05PM CEST, tom@...bertland.com wrote:
>Hi Jiri,
>
>Thanks for this work, I think it's a good direction! Some comments below...
>
>On Fri, Apr 24, 2015 at 8:51 AM, Jiri Pirko <jiri@...nulli.us> wrote:

...

>>  enum flow_dissector_key_id {
>>         FLOW_DISSECTOR_KEY_BASIC, /* struct flow_dissector_key_basic */
>>         FLOW_DISSECTOR_KEY_IPV4_ADDRS, /* struct flow_dissector_key_addrs */
>>         FLOW_DISSECTOR_KEY_IPV6_HASH_ADDRS, /* struct flow_dissector_key_addrs */
>>         FLOW_DISSECTOR_KEY_PORTS, /* struct flow_dissector_key_ports */
>> +       FLOW_DISSECTOR_KEY_IPV6_ADDRS, /* struct flow_dissector_key_ipv6_addrs */
>>
>And we'll want to add VLAN ID, (GRE) key-id, IPv6 flow label, maybe a
>couple more.

Definitely. I plan to extend keys for those you mentioned and more in
future. Easy extendability is the goal of this patchset.
 

>
>>         FLOW_DISSECTOR_KEY_MAX,
>>  };
>> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>> index 564288e..95e9a21 100644
>> --- a/net/core/flow_dissector.c
>> +++ b/net/core/flow_dissector.c
>> @@ -175,16 +175,29 @@ ipv6:
>>                 ip_proto = iph->nexthdr;
>>                 nhoff += sizeof(struct ipv6hdr);
>>
>> -               if (!skb_flow_dissector_uses_key(flow_dissector,
>> -                                                FLOW_DISSECTOR_KEY_IPV6_HASH_ADDRS))
>> -                       break;
>> -               key_addrs = skb_flow_dissector_target(flow_dissector,
>> -                                                     FLOW_DISSECTOR_KEY_IPV6_HASH_ADDRS,
>> -                                                     target_container);
>> +               if (skb_flow_dissector_uses_key(flow_dissector,
>> +                                               FLOW_DISSECTOR_KEY_IPV6_HASH_ADDRS)) {
>> +                       key_addrs = skb_flow_dissector_target(flow_dissector,
>> +                                                             FLOW_DISSECTOR_KEY_IPV6_HASH_ADDRS,
>> +                                                             target_container);
>>
>> -               key_addrs->src = (__force __be32)ipv6_addr_hash(&iph->saddr);
>> -               key_addrs->dst = (__force __be32)ipv6_addr_hash(&iph->daddr);
>> +                       key_addrs->src = (__force __be32)ipv6_addr_hash(&iph->saddr);
>> +                       key_addrs->dst = (__force __be32)ipv6_addr_hash(&iph->daddr);
>> +                       goto flow_label;
>> +               }
>
>So this is still folding the IPv6 addresses so that that we can fit
>into jhash_3words? Can we address this now and include the full IPv6
>address in the hash? I would propose that we extend the flow_keys
>structure (which I think you may already be doing) for full IPv6
>address, VLAN, flow label, etc., and then produce a hash across that
>whole structure. jhash2 can be used on a structure.  jhash is actually
>a very efficient hash performance wise-- the rest of flow dissection
>is likely the dominant cost anyway. We should also minimize calls to
>flow_dissector, I would propose it should be called at most once per
>packet-- I'll be reposting patches to fix that in the various qdiscs.

This hash computation is currently present in kernel. I just ported that
to new programable dissector framework.

I'm not sure how possible is the flow_keys structure extension. In this
patchset, I just maintain the current size and usage. But the thing is
that every individual caller of flow_dissect can use their own struct
which includes exactly what he needs.

Regarding the flow_dissect call reduction, I agree that it is something
we need. The question is how to pass the dissect results through the
code. For example, sch_choke uses qdisc_cb_private for passing flow_keys
struct. However qdisc_skb_cb can by only 20 bytes long (QDISC_CB_PRIV_LEN)
This can be however resolver independently of this patchset.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html