| 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
| ||
|
Message-ID: <CA+6hz4qv5kutyjS3W0R8GeMCXRSiqADyT4uuxCUpeaFvwZVVPQ@mail.gmail.com>
Date: Mon, 1 Aug 2016 15:36:02 +0800
From: Feng Gao <gfree.wind@...il.com>
To: Philp Prindeville <philipp@...fish-solutions.com>
Cc: Tom Herbert <tom@...bertland.com>,
fgao@...vckh6395k16k5.yundunddos.com,
"David S. Miller" <davem@...emloft.net>,
Stephen Hemminger <stephen@...workplumber.org>,
Pravin B Shelar <pshelar@...ira.com>,
Alex Duyck <aduyck@...antis.com>,
Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash
Hi Philp & Tom,
Thanks your advice.
I have send the update patch some days ago.
The link is http://www.spinics.net/lists/netdev/msg388083.html.
Could you help review it please?
Best Regards
Feng
On Thu, Jul 28, 2016 at 8:46 AM, Philp Prindeville
<philipp@...fish-solutions.com> wrote:
> Inline...
>
>
>
> On 07/27/2016 06:04 PM, Tom Herbert wrote:
>>
>> On Wed, Jul 27, 2016 at 4:42 PM, <fgao@...ai8.com> wrote:
>>>
>>> From: Gao Feng <fgao@...ai8.com>
>>>
>>> The PPTP is encapsulated by GRE header with that GRE_VERSION bits
>>> must contain one. But current GRE RPS needs the GRE_VERSION must be
>>> zero. So RPS does not work for PPTP traffic.
>>>
>>> In my test environment, there are four MIPS cores, and all traffic
>>> are passed through by PPTP. As a result, only one core is 100% busy
>>> while other three cores are very idle. After this patch, the usage
>>> of four cores are balanced well.
>>>
>>> Signed-off-by: Gao Feng <fgao@...ai8.com>
>>> ---
>>> v1: Initial patch
>>>
>>> include/uapi/linux/if_tunnel.h | 5 +-
>>> net/core/flow_dissector.c | 138
>>> ++++++++++++++++++++++++++---------------
>>> 2 files changed, 92 insertions(+), 51 deletions(-)
>>>
>>> diff --git a/include/uapi/linux/if_tunnel.h
>>> b/include/uapi/linux/if_tunnel.h
>>> index 1046f55..dda4e4b 100644
>>> --- a/include/uapi/linux/if_tunnel.h
>>> +++ b/include/uapi/linux/if_tunnel.h
>>> @@ -24,9 +24,12 @@
>>> #define GRE_SEQ __cpu_to_be16(0x1000)
>>> #define GRE_STRICT __cpu_to_be16(0x0800)
>>> #define GRE_REC __cpu_to_be16(0x0700)
>>> -#define GRE_FLAGS __cpu_to_be16(0x00F8)
>>> +#define GRE_ACK __cpu_to_be16(0x0080)
>>> +#define GRE_FLAGS __cpu_to_be16(0x0078)
>>> #define GRE_VERSION __cpu_to_be16(0x0007)
>>>
>>> +#define GRE_PROTO_PPP __cpu_to_be16(0x880b)
>>> +
>>> struct ip_tunnel_parm {
>>> char name[IFNAMSIZ];
>>> int link;
>>> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>>> index 61ad43f..d95e060 100644
>>> --- a/net/core/flow_dissector.c
>>> +++ b/net/core/flow_dissector.c
>>> @@ -346,63 +346,101 @@ ip_proto_again:
>>> hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr),
>>> data, hlen, &_hdr);
>>> if (!hdr)
>>> goto out_bad;
>>> - /*
>>> - * Only look inside GRE if version zero and no
>>> - * routing
>>> - */
>>> - if (hdr->flags & (GRE_VERSION | GRE_ROUTING))
>>> - break;
>>>
>>> - proto = hdr->proto;
>>> - nhoff += 4;
>>> - if (hdr->flags & GRE_CSUM)
>>> + /*
>>> + * Only look inside GRE if version zero and no
>>> + * routing
>>
>> This comment is no longer true, GRE version 1 is being handled.
>>
>>> + */
>>> + if (!(hdr->flags & (GRE_VERSION | GRE_ROUTING))) {
>>> + proto = hdr->proto;
>>> nhoff += 4;
>>> - if (hdr->flags & GRE_KEY) {
>>> - const __be32 *keyid;
>>> - __be32 _keyid;
>>> + if (hdr->flags & GRE_CSUM)
>>> + nhoff += 4;
>>> + if (hdr->flags & GRE_KEY) {
>>> + const __be32 *keyid;
>>> + __be32 _keyid;
>>> +
>>> + keyid = __skb_header_pointer(skb, nhoff,
>>> sizeof(_keyid),
>>> + data, hlen,
>>> &_keyid);
>>> +
>>> + if (!keyid)
>>> + goto out_bad;
>>> +
>>> + if (dissector_uses_key(flow_dissector,
>>> +
>>> FLOW_DISSECTOR_KEY_GRE_KEYID)) {
>>> + key_keyid =
>>> skb_flow_dissector_target(flow_dissector,
>>> +
>>> FLOW_DISSECTOR_KEY_GRE_KEYID,
>>> +
>>> target_container);
>>> + key_keyid->keyid = *keyid;
>>> + }
>>> + nhoff += 4;
>>> + }
>>> + if (hdr->flags & GRE_SEQ)
>>> + nhoff += 4;
>>> + if (proto == htons(ETH_P_TEB)) {
>>> + const struct ethhdr *eth;
>>> + struct ethhdr _eth;
>>> +
>>> + eth = __skb_header_pointer(skb, nhoff,
>>> + sizeof(_eth),
>>> + data, hlen,
>>> &_eth);
>>> + if (!eth)
>>> + goto out_bad;
>>> + proto = eth->h_proto;
>>> + nhoff += sizeof(*eth);
>>> +
>>> + /* Cap headers that we access via
>>> pointers at the
>>> + * end of the Ethernet header as our
>>> maximum alignment
>>> + * at that point is only 2 bytes.
>>> + */
>>> + if (NET_IP_ALIGN)
>>> + hlen = nhoff;
>>> + }
>>>
>>> - keyid = __skb_header_pointer(skb, nhoff,
>>> sizeof(_keyid),
>>> - data, hlen,
>>> &_keyid);
>>> + key_control->flags |= FLOW_DIS_ENCAPSULATION;
>>> + if (flags & FLOW_DISSECTOR_F_STOP_AT_ENCAP)
>>> + goto out_good;
>>>
>>> - if (!keyid)
>>> + goto again;
>>> + } else if (hdr->proto == GRE_PROTO_PPP && (hdr->flags &
>>> GRE_VERSION) &&
>>> + (hdr->flags & GRE_KEY) &&
>>> + !(hdr->flags & (GRE_CSUM | GRE_ROUTING |
>>> GRE_STRICT | GRE_REC | GRE_FLAGS))) {
>>> + /* It should be the PPTP in GRE
>>> + * We have checked the flags according to the
>>> + * RFC 2637
>>> + */
>>> + struct ppp_frame {
>>> + /* address & control, just ignore it */
>>> + __be16 ignore;
>>> + __be16 proto;
>>> + } *frame, _frame;
>>
>> Isn't there a common definition of a PPP frame?
>
>
> If there were, it would be in <net/ppp_defs.h> but it's not. It's sort of
> implied the macros PPP_ADDRESS(), PPP_CONTROL(), and PPP_PROTOCOL().
>
> Please rewrite the patch to use these.
>
> -Philip
>
>
>>> + int offset = 0;
>>> +
>>> + /* Skip GRE header */
>>> + offset += 4;
>>> + /* Skip payload length and call id */
>>> + offset += 4;
>>> +
>>> + if (hdr->flags & GRE_SEQ)
>>> + offset += 4;
>>> +
>>> + if (hdr->flags & GRE_ACK)
>>> + offset += 4;
>>> +
>>> + frame = skb_header_pointer(skb, nhoff + offset,
>>> sizeof(_frame), &_frame);
>>> + if (!frame)
>>> goto out_bad;
>>> -
>>> - if (dissector_uses_key(flow_dissector,
>>> -
>>> FLOW_DISSECTOR_KEY_GRE_KEYID)) {
>>> - key_keyid =
>>> skb_flow_dissector_target(flow_dissector,
>>> -
>>> FLOW_DISSECTOR_KEY_GRE_KEYID,
>>> -
>>> target_container);
>>> - key_keyid->keyid = *keyid;
>>> + if (frame->proto == __constant_htons(PPP_IP)) {
>>> + nhoff += (sizeof(*frame) + offset);
>>> + proto = __constant_htons(ETH_P_IP);
>>> + goto again;
>>> + } else if (frame->proto ==
>>> __constant_htons(PPP_IPV6)) {
>>> + nhoff += (sizeof(*frame) + offset);
>>> + proto = __constant_htons(ETH_P_IPV6);
>>> + goto again;
>>
>> There's a lot of code replication here with the version 0 path. Please
>> consolidate these. Also, this looks an awful lot like ETH_P_PPP_SES,
>> can this code leverage that code?
>>
>>> }
>>> - nhoff += 4;
>>> }
>>> - if (hdr->flags & GRE_SEQ)
>>> - nhoff += 4;
>>> - if (proto == htons(ETH_P_TEB)) {
>>> - const struct ethhdr *eth;
>>> - struct ethhdr _eth;
>>> -
>>> - eth = __skb_header_pointer(skb, nhoff,
>>> - sizeof(_eth),
>>> - data, hlen, &_eth);
>>> - if (!eth)
>>> - goto out_bad;
>>> - proto = eth->h_proto;
>>> - nhoff += sizeof(*eth);
>>> -
>>> - /* Cap headers that we access via pointers at the
>>> - * end of the Ethernet header as our maximum
>>> alignment
>>> - * at that point is only 2 bytes.
>>> - */
>>> - if (NET_IP_ALIGN)
>>> - hlen = nhoff;
>>> - }
>>> -
>>> - key_control->flags |= FLOW_DIS_ENCAPSULATION;
>>> - if (flags & FLOW_DISSECTOR_F_STOP_AT_ENCAP)
>>> - goto out_good;
>>> -
>>> - goto again;
>>> + break;
>>> }
>>> case NEXTHDR_HOP:
>>> case NEXTHDR_ROUTING:
>>> --
>>> 1.9.1
>>>
>
Powered by blists - more mailing lists