| 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: Tue, 9 Aug 2016 10:25:11 +0800
From: Feng Gao <gfree.wind@...il.com>
To: Philip Prindeville <philipp_subx@...fish-solutions.com>
Cc: fgao@...vckh6395k16k5.yundunddos.com,
"David S. Miller" <davem@...emloft.net>,
Stephen Hemminger <stephen@...workplumber.org>,
Pravin B Shelar <pshelar@...ira.com>,
Tom Herbert <tom@...bertland.com>,
Alex Duyck <aduyck@...antis.com>,
Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH v5 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash
Oh, I think I really get you this time.
Please see my inline comment.
If they are right, I could send the v6 patch.....
Best Regards
Feng
On Tue, Aug 9, 2016 at 9:06 AM, Philip Prindeville
<philipp_subx@...fish-solutions.com> wrote:
> Inline...
>
>
>
> On 08/08/2016 06:37 PM, fgao@...vckh6395k16k5.yundunddos.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>
>> ---
>> v5: 1) Make fix header of gre_full_hdr as uname struct;
>> 2) Create macro GRE_PPTP_KEY_MASK;
>> v4: 1) Define struct gre_full_hdr, and use sizeof its member directly;
>> 2) Move version and routing check ahead;
>> 3) Only PPTP in GRE check the ack flag;
>> v3: 1) Move struct pptp_gre_header defination into new file pptp.h
>> 2) Use sizeof GRE and PPTP type instead of literal value;
>> 3) Remove strict flag check for PPTP to robust;
>> 4) Consolidate the codes again;
>> v2: Update according to Tom and Philp's advice.
>> 1) Consolidate the codes with GRE version 0 path;
>> 2) Use PPP_PROTOCOL to get ppp protol;
>> 3) Set the FLOW_DIS_ENCAPSULATION flag;
>> v1: Intial Patch
>>
>> drivers/net/ppp/pptp.c | 36 +------------
>> include/net/gre.h | 13 ++++-
>> include/net/pptp.h | 40 +++++++++++++++
>> include/uapi/linux/if_tunnel.h | 7 ++-
>> net/core/flow_dissector.c | 113
>> ++++++++++++++++++++++++++++-------------
>> 5 files changed, 138 insertions(+), 71 deletions(-)
>> create mode 100644 include/net/pptp.h
>>
>> diff --git a/drivers/net/ppp/pptp.c b/drivers/net/ppp/pptp.c
>> index ae0905e..3e68dbc 100644
>> --- a/drivers/net/ppp/pptp.c
>> +++ b/drivers/net/ppp/pptp.c
>> @@ -37,6 +37,7 @@
>> #include <net/icmp.h>
>> #include <net/route.h>
>> #include <net/gre.h>
>> +#include <net/pptp.h>
>> #include <linux/uaccess.h>
>> @@ -53,41 +54,6 @@ static struct proto pptp_sk_proto __read_mostly;
>> static const struct ppp_channel_ops pptp_chan_ops;
>> static const struct proto_ops pptp_ops;
>> -#define PPP_LCP_ECHOREQ 0x09
>> -#define PPP_LCP_ECHOREP 0x0A
>> -#define SC_RCV_BITS (SC_RCV_B7_1|SC_RCV_B7_0|SC_RCV_ODDP|SC_RCV_EVNP)
>> -
>> -#define MISSING_WINDOW 20
>> -#define WRAPPED(curseq, lastseq)\
>> - ((((curseq) & 0xffffff00) == 0) &&\
>> - (((lastseq) & 0xffffff00) == 0xffffff00))
>> -
>> -#define PPTP_GRE_PROTO 0x880B
>> -#define PPTP_GRE_VER 0x1
>> -
>> -#define PPTP_GRE_FLAG_C 0x80
>> -#define PPTP_GRE_FLAG_R 0x40
>> -#define PPTP_GRE_FLAG_K 0x20
>> -#define PPTP_GRE_FLAG_S 0x10
>> -#define PPTP_GRE_FLAG_A 0x80
>> -
>> -#define PPTP_GRE_IS_C(f) ((f)&PPTP_GRE_FLAG_C)
>> -#define PPTP_GRE_IS_R(f) ((f)&PPTP_GRE_FLAG_R)
>> -#define PPTP_GRE_IS_K(f) ((f)&PPTP_GRE_FLAG_K)
>> -#define PPTP_GRE_IS_S(f) ((f)&PPTP_GRE_FLAG_S)
>> -#define PPTP_GRE_IS_A(f) ((f)&PPTP_GRE_FLAG_A)
>> -
>> -#define PPTP_HEADER_OVERHEAD (2+sizeof(struct pptp_gre_header))
>> -struct pptp_gre_header {
>> - u8 flags;
>> - u8 ver;
>> - __be16 protocol;
>> - __be16 payload_len;
>> - __be16 call_id;
>> - __be32 seq;
>> - __be32 ack;
>> -} __packed;
>> -
>> static struct pppox_sock *lookup_chan(u16 call_id, __be32 s_addr)
>> {
>> struct pppox_sock *sock;
>> diff --git a/include/net/gre.h b/include/net/gre.h
>> index 7a54a31..6347f16 100644
>> --- a/include/net/gre.h
>> +++ b/include/net/gre.h
>> @@ -7,9 +7,20 @@
>> struct gre_base_hdr {
>> __be16 flags;
>> __be16 protocol;
>> -};
>> +} __packed;
>> #define GRE_HEADER_SECTION 4
>> +struct gre_full_hdr {
>> + struct {
>> + __be16 flags;
>> + __be16 protocols;
>> + };
>
>
> No, you're misunderstanding what I was saying. I was suggesting using
> "struct gre_base_hdr" here.
>
> Anyway, we can skip it. Was trying to reflect that the gre_full_hdr was
> derived from the gre_base_hdr.
struct gre_full_hdr {
struct gre_base_hdr {
__be16 flags;
__be16 protocols;
} fixed_header;
.....
};
I think you mean it is this.
Sorry about misunderstanding about that always :((
And I have got the advantage now, thanks.
>
>
>
>> + __be16 csum;
>> + __be16 reserved1;
>> + __be32 key;
>> + __be32 seq;
>> +} __packed;
>> +
>> #define GREPROTO_CISCO 0
>> #define GREPROTO_PPTP 1
>> #define GREPROTO_MAX 2
>> diff --git a/include/net/pptp.h b/include/net/pptp.h
>> new file mode 100644
>> index 0000000..301d3e2
>> --- /dev/null
>> +++ b/include/net/pptp.h
>> @@ -0,0 +1,40 @@
>> +#ifndef _NET_PPTP_H
>> +#define _NET_PPTP_H
>> +
>> +#define PPP_LCP_ECHOREQ 0x09
>> +#define PPP_LCP_ECHOREP 0x0A
>> +#define SC_RCV_BITS (SC_RCV_B7_1|SC_RCV_B7_0|SC_RCV_ODDP|SC_RCV_EVNP)
>> +
>> +#define MISSING_WINDOW 20
>> +#define WRAPPED(curseq, lastseq)\
>> + ((((curseq) & 0xffffff00) == 0) &&\
>> + (((lastseq) & 0xffffff00) == 0xffffff00))
>> +
>> +#define PPTP_GRE_PROTO 0x880B
>> +#define PPTP_GRE_VER 0x1
>> +
>> +#define PPTP_GRE_FLAG_C 0x80
>> +#define PPTP_GRE_FLAG_R 0x40
>> +#define PPTP_GRE_FLAG_K 0x20
>> +#define PPTP_GRE_FLAG_S 0x10
>> +#define PPTP_GRE_FLAG_A 0x80
>> +
>> +#define PPTP_GRE_IS_C(f) ((f)&PPTP_GRE_FLAG_C)
>> +#define PPTP_GRE_IS_R(f) ((f)&PPTP_GRE_FLAG_R)
>> +#define PPTP_GRE_IS_K(f) ((f)&PPTP_GRE_FLAG_K)
>> +#define PPTP_GRE_IS_S(f) ((f)&PPTP_GRE_FLAG_S)
>> +#define PPTP_GRE_IS_A(f) ((f)&PPTP_GRE_FLAG_A)
>> +
>> +#define PPTP_HEADER_OVERHEAD (2+sizeof(struct pptp_gre_header))
>> +struct pptp_gre_header {
>> + u8 flags;
>> + u8 ver;
>> + __be16 protocol;
>> + __be16 payload_len;
>> + __be16 call_id;
>> + __be32 seq;
>> + __be32 ack;
>> +} __packed;
>> +
>> +
>> +#endif
>> diff --git a/include/uapi/linux/if_tunnel.h
>> b/include/uapi/linux/if_tunnel.h
>> index 1046f55..0c11918 100644
>> --- a/include/uapi/linux/if_tunnel.h
>> +++ b/include/uapi/linux/if_tunnel.h
>> @@ -24,9 +24,14 @@
>> #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_VERSION_1 __cpu_to_be16(0x0001)
>> +#define GRE_PROTO_PPP __cpu_to_be16(0x880b)
>> +#define GRE_PPTP_KEY_MASK htonl(0xffff)
>
>
> Any reason to not use __cpu_to_be32() or __constant_htonl() here?
Because I met that the checkpatch.pl reports warning about "__constant_htonl()".
But I should use __cpu_to_be32 instead of htonl indeed.
>
> -Philip
>
>
>
>> +
>> 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..91028ae 100644
>> --- a/net/core/flow_dissector.c
>> +++ b/net/core/flow_dissector.c
>> @@ -6,6 +6,8 @@
>> #include <linux/if_vlan.h>
>> #include <net/ip.h>
>> #include <net/ipv6.h>
>> +#include <net/gre.h>
>> +#include <net/pptp.h>
>> #include <linux/igmp.h>
>> #include <linux/icmp.h>
>> #include <linux/sctp.h>
>> @@ -338,32 +340,42 @@ mpls:
>> ip_proto_again:
>> switch (ip_proto) {
>> case IPPROTO_GRE: {
>> - struct gre_hdr {
>> - __be16 flags;
>> - __be16 proto;
>> - } *hdr, _hdr;
>> + struct gre_base_hdr *hdr, _hdr;
>> + u16 gre_ver;
>> + int offset = 0;
>> 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))
>> +
>> + /* Only look inside GRE without routing */
>> + if (hdr->flags & GRE_ROUTING)
>> break;
>> - proto = hdr->proto;
>> - nhoff += 4;
>> + /* Only look inside GRE for version 0 and 1 */
>> + gre_ver = ntohs(hdr->flags & GRE_VERSION);
>> + if (gre_ver > 1)
>> + break;
>> +
>> + proto = hdr->protocol;
>> + if (gre_ver) {
>> + /* Version1 must be PPTP, and check the flags */
>> + if (!(proto == GRE_PROTO_PPP && (hdr->flags &
>> GRE_KEY)))
>> + break;
>> + }
>> +
>> + offset += sizeof(struct gre_base_hdr);
>> +
>> if (hdr->flags & GRE_CSUM)
>> - nhoff += 4;
>> + offset += sizeof(((struct gre_full_hdr *)0)->csum)
>> +
>> + sizeof(((struct gre_full_hdr
>> *)0)->reserved1);
>> +
>> if (hdr->flags & GRE_KEY) {
>> const __be32 *keyid;
>> __be32 _keyid;
>> - keyid = __skb_header_pointer(skb, nhoff,
>> sizeof(_keyid),
>> + keyid = __skb_header_pointer(skb, nhoff + offset,
>> sizeof(_keyid),
>> data, hlen, &_keyid);
>> -
>> if (!keyid)
>> goto out_bad;
>> @@ -372,32 +384,65 @@ ip_proto_again:
>> key_keyid =
>> skb_flow_dissector_target(flow_dissector,
>>
>> FLOW_DISSECTOR_KEY_GRE_KEYID,
>>
>> target_container);
>> - key_keyid->keyid = *keyid;
>> + if (gre_ver == 0)
>> + key_keyid->keyid = *keyid;
>> + else
>> + key_keyid->keyid = *keyid &
>> GRE_PPTP_KEY_MASK;
>> }
>> - nhoff += 4;
>> + offset += sizeof(((struct gre_full_hdr *)0)->key);
>> }
>> +
>> 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)
>> + offset += sizeof(((struct pptp_gre_header
>> *)0)->seq);
>> +
>> + if (gre_ver == 0) {
>> + if (proto == htons(ETH_P_TEB)) {
>> + const struct ethhdr *eth;
>> + struct ethhdr _eth;
>> +
>> + eth = __skb_header_pointer(skb, nhoff +
>> offset,
>> + sizeof(_eth),
>> + data, hlen,
>> &_eth);
>> + if (!eth)
>> + goto out_bad;
>> + proto = eth->h_proto;
>> + offset += 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 + offset);
>> + }
>> + } else { /* version 1, must be PPTP */
>> + u8 _ppp_hdr[PPP_HDRLEN];
>> + u8 *ppp_hdr;
>> +
>> + if (hdr->flags & GRE_ACK)
>> + offset += sizeof(((struct pptp_gre_header
>> *)0)->ack);
>> +
>> + ppp_hdr = skb_header_pointer(skb, nhoff + offset,
>> + sizeof(_ppp_hdr),
>> _ppp_hdr);
>> + if (!ppp_hdr)
>> 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;
>> +
>> + switch (PPP_PROTOCOL(ppp_hdr)) {
>> + case PPP_IP:
>> + proto = htons(ETH_P_IP);
>> + break;
>> + case PPP_IPV6:
>> + proto = htons(ETH_P_IPV6);
>> + break;
>> + default:
>> + /* Could probably catch some more like
>> MPLS */
>> + break;
>> + }
>> +
>> + offset += PPP_HDRLEN;
>> }
>> + nhoff += offset;
>> key_control->flags |= FLOW_DIS_ENCAPSULATION;
>> if (flags & FLOW_DISSECTOR_F_STOP_AT_ENCAP)
>> goto out_good;
>
>
Powered by blists - more mailing lists