| 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+6hz4q6vH_2Roa00xF1GaCw9rJmCP6EUt1m7GzE6o3O98_iAQ@mail.gmail.com>
Date: Thu, 4 Aug 2016 07:58:06 +0800
From: Feng Gao <gfree.wind@...il.com>
To: Philip Prindeville <philipp@...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 v3 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash
inline comment.
There are two comments that I am not clear.
Best Regards
Feng
On Thu, Aug 4, 2016 at 4:43 AM, Philip Prindeville
<philipp@...fish-solutions.com> wrote:
> Inline…
>
>> On Aug 3, 2016, at 8:52 AM, 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>
>> ---
>> 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/pptp.h | 40 ++++++++++++
>> include/uapi/linux/if_tunnel.h | 7 +-
>> net/core/flow_dissector.c | 141 +++++++++++++++++++++++++----------------
>> 4 files changed, 134 insertions(+), 90 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/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
>
> What about macros for accessing the lower 3 bits of the version?
There is already one macro "GRE_VERSION" as the mask to get version.
>
>
>> +
>> +#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;
>
>
> What about a definition of a V0 (RFC-1701) packet? We’re handling both, so it makes sense to define both.
I don't get you. The struct "gre_base_hdr" is defined in gre.h. Do you
mean define them in same file ?
>
>
>> +
>> +
>> +#endif
>> diff --git a/include/uapi/linux/if_tunnel.h b/include/uapi/linux/if_tunnel.h
>> index 1046f55..7d889db 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)
>> +
>> +
>> 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..52b7c3c 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,71 +340,102 @@ 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;
>>
>> 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)
>> - nhoff += 4;
>> - if (hdr->flags & GRE_KEY) {
>> - const __be32 *keyid;
>> - __be32 _keyid;
>> + /* Only look inside GRE without routing */
>> + if (!(hdr->flags & GRE_ROUTING)) {
>> + int offset = 0;
>>
>> - keyid = __skb_header_pointer(skb, nhoff, sizeof(_keyid),
>> - data, hlen, &_keyid);
>> + proto = hdr->protocol;
>>
>> - if (!keyid)
>> - goto out_bad;
>> + if (hdr->flags & GRE_VERSION) {
>> + /* Maybe PPTP in GRE */
>> + if (!(proto == GRE_PROTO_PPP && (hdr->flags & GRE_KEY) &&
>> + (hdr->flags & GRE_VERSION) == GRE_VERSION_1))
>> + break;
>> + }
>>
>> - 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;
>> + offset += sizeof(struct gre_base_hdr);
>> +
>> + if (hdr->flags & GRE_CSUM)
>> + offset += sizeof(__be32);
>
>
> This doesn’t tell me as much as taking the sizeof() of the particular field (by name) in the packet that you’re skipping. Best way to do this is naming the field in the structure…
>
>
>> +
>> + if (hdr->flags & GRE_KEY) {
>> + const __be32 *keyid;
>> + __be32 _keyid;
>> +
>> + keyid = __skb_header_pointer(skb, nhoff + offset, 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;
>> + }
>> + offset += sizeof(_keyid);
>
>
> Same issue here.
>
>
>> }
>> - 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;
>> + if (hdr->flags & GRE_SEQ)
>> + offset += sizeof(((struct pptp_gre_header *)0)->seq);
>> +
>> + if (hdr->flags & GRE_ACK)
>> + offset += sizeof(((struct pptp_gre_header *)0)->ack);
>
>
> Much better.
>
> -Philip
>
>
>> +
>> + if (proto == GRE_PROTO_PPP) {
>> + u8 _ppp_hdr[PPP_HDRLEN];
>> + u8 *ppp_hdr;
>> +
>> + ppp_hdr = skb_header_pointer(skb, nhoff + offset,
>> + sizeof(_ppp_hdr), _ppp_hdr);
>> + if (!ppp_hdr)
>> + goto out_bad;
>> +
>> + proto = PPP_PROTOCOL(ppp_hdr);
>> + if (proto == PPP_IP)
>> + proto = htons(ETH_P_IP);
>> + else if (proto == PPP_IPV6)
>> + proto = htons(ETH_P_IPV6);
>> + else
>> + break;
>> +
>> + offset += PPP_HDRLEN;
>> + } else 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);
>> + }
>>
>> - goto again;
>> + nhoff += offset;
>> + 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