* [PATCH v3 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash
@ 2016-08-03 14:52 fgao
2016-08-03 16:15 ` Tom Herbert
2016-08-03 20:43 ` Philip Prindeville
0 siblings, 2 replies; 8+ messages in thread
From: fgao @ 2016-08-03 14:52 UTC (permalink / raw)
To: davem, philipp, stephen, pshelar, tom, aduyck, netdev
Cc: gfree.wind, Gao Feng
From: Gao Feng <fgao@ikuai8.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@ikuai8.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
+
+#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..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);
+
+ 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);
}
- 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);
+
+ 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash
2016-08-03 14:52 [PATCH v3 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash fgao
@ 2016-08-03 16:15 ` Tom Herbert
2016-08-03 23:47 ` Feng Gao
2016-08-03 20:43 ` Philip Prindeville
1 sibling, 1 reply; 8+ messages in thread
From: Tom Herbert @ 2016-08-03 16:15 UTC (permalink / raw)
To: Gao Feng
Cc: David S. Miller, Philp Prindeville, Stephen Hemminger,
Pravin B Shelar, Alex Duyck, Linux Kernel Network Developers,
Feng Gao
On Wed, Aug 3, 2016 at 7:52 AM, <fgao@ikuai8.com> wrote:
> From: Gao Feng <fgao@ikuai8.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@ikuai8.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
> +
> +#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..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);
> +
> + 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);
> }
> - 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);
> +
> + 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
>
HI Feng,
Please be careful about order of processing GRE options, keyid must be
handled first. GRE_ACK looks like the only new field that needs to be
considered for v1. Also, the keyid in v1 is split into two 16 bit
fields; the first is payload length which is not usable for entropy,
but the second (Call ID) does look useful for that. I would suggest
the IPPROTO_GRE could look something like:
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 61ad43f..736b531 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -342,15 +342,20 @@ ip_proto_again:
__be16 flags;
__be16 proto;
} *hdr, _hdr;
+ u8 gre_ver;
hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr),
data, hlen, &_hdr);
if (!hdr)
goto out_bad;
+
+ /* Only look inside GRE for versions 0 and 1 */
+ gre_ver = hdr->flags & GRE_VERSION;
+ if (gre_ver > 1)
+ break;
/*
- * Only look inside GRE if version zero and no
- * routing
+ * Only look inside GRE if no routing
*/
- if (hdr->flags & (GRE_VERSION | GRE_ROUTING))
+ if (hdr->flags & GRE_ROUTING)
break;
proto = hdr->proto;
@@ -372,30 +377,62 @@ 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 &
htonl(0xffff);
}
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;
+ if (gre_ver == 0) {
+ 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;
+ }
+ } else { /* Version 1 */
+ if (hdr->flags & GRE_ACK)
+ offset += 4;
+
+ 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;
+
+ 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;
+ }
}
key_control->flags |= FLOW_DIS_ENCAPSULATION;
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash
2016-08-03 14:52 [PATCH v3 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash fgao
2016-08-03 16:15 ` Tom Herbert
@ 2016-08-03 20:43 ` Philip Prindeville
2016-08-03 23:58 ` Feng Gao
1 sibling, 1 reply; 8+ messages in thread
From: Philip Prindeville @ 2016-08-03 20:43 UTC (permalink / raw)
To: fgao
Cc: David S. Miller, Stephen Hemminger, Pravin B Shelar, Tom Herbert,
Alex Duyck, netdev, gfree.wind
[-- Attachment #1: Type: text/plain, Size: 10411 bytes --]
Inline…
> On Aug 3, 2016, at 8:52 AM, fgao@48lvckh6395k16k5.yundunddos.com wrote:
>
> From: Gao Feng <fgao@ikuai8.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@ikuai8.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?
> +
> +#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.
> +
> +
> +#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
>
[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 496 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash
2016-08-03 16:15 ` Tom Herbert
@ 2016-08-03 23:47 ` Feng Gao
0 siblings, 0 replies; 8+ messages in thread
From: Feng Gao @ 2016-08-03 23:47 UTC (permalink / raw)
To: Tom Herbert
Cc: Gao Feng, David S. Miller, Philp Prindeville, Stephen Hemminger,
Pravin B Shelar, Alex Duyck, Linux Kernel Network Developers
Hi Tom,
inline comments
On Thu, Aug 4, 2016 at 12:15 AM, Tom Herbert <tom@herbertland.com> wrote:
> On Wed, Aug 3, 2016 at 7:52 AM, <fgao@ikuai8.com> wrote:
>> From: Gao Feng <fgao@ikuai8.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@ikuai8.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
>> +
>> +#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..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);
>> +
>> + 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);
>> }
>> - 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);
>> +
>> + 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
>>
>
> HI Feng,
>
> Please be careful about order of processing GRE options, keyid must be
> handled first.
Thanks your reminder. But I think the keyid should be processed
secondly and I did it in v4 patch.
Because the keyid option is processed secondly in original codes.
> GRE_ACK looks like the only new field that needs to be
> considered for v1. Also, the keyid in v1 is split into two 16 bit
> fields; the first is payload length which is not usable for entropy,
> but the second (Call ID) does look useful for that. I would suggest
> the IPPROTO_GRE could look something like:
Ok, I get it.
>
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 61ad43f..736b531 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -342,15 +342,20 @@ ip_proto_again:
> __be16 flags;
> __be16 proto;
> } *hdr, _hdr;
> + u8 gre_ver;
>
> hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr),
> data, hlen, &_hdr);
> if (!hdr)
> goto out_bad;
> +
> + /* Only look inside GRE for versions 0 and 1 */
> + gre_ver = hdr->flags & GRE_VERSION;
> + if (gre_ver > 1)
> + break;
> /*
> - * Only look inside GRE if version zero and no
> - * routing
> + * Only look inside GRE if no routing
> */
> - if (hdr->flags & (GRE_VERSION | GRE_ROUTING))
> + if (hdr->flags & GRE_ROUTING)
> break;
>
> proto = hdr->proto;
> @@ -372,30 +377,62 @@ 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 &
> htonl(0xffff);
> }
> 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;
> + if (gre_ver == 0) {
> + 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;
> + }
> + } else { /* Version 1 */
> + if (hdr->flags & GRE_ACK)
> + offset += 4;
> +
> + 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;
> +
> + 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;
> + }
> }
>
> key_control->flags |= FLOW_DIS_ENCAPSULATION;
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash
2016-08-03 20:43 ` Philip Prindeville
@ 2016-08-03 23:58 ` Feng Gao
2016-08-04 0:33 ` Philp Prindeville
0 siblings, 1 reply; 8+ messages in thread
From: Feng Gao @ 2016-08-03 23:58 UTC (permalink / raw)
To: Philip Prindeville
Cc: fgao, David S. Miller, Stephen Hemminger, Pravin B Shelar,
Tom Herbert, Alex Duyck, Linux Kernel Network Developers
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@redfish-solutions.com> wrote:
> Inline…
>
>> On Aug 3, 2016, at 8:52 AM, fgao@48lvckh6395k16k5.yundunddos.com wrote:
>>
>> From: Gao Feng <fgao@ikuai8.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@ikuai8.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
>>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash
2016-08-03 23:58 ` Feng Gao
@ 2016-08-04 0:33 ` Philp Prindeville
2016-08-04 7:37 ` Feng Gao
0 siblings, 1 reply; 8+ messages in thread
From: Philp Prindeville @ 2016-08-04 0:33 UTC (permalink / raw)
To: Feng Gao
Cc: fgao, David S. Miller, Stephen Hemminger, Pravin B Shelar,
Tom Herbert, Alex Duyck, Linux Kernel Network Developers
Inline
On 08/03/2016 05:58 PM, Feng Gao wrote:
> 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@redfish-solutions.com> wrote:
>> Inline…
>>
>>> On Aug 3, 2016, at 8:52 AM, fgao@48lvckh6395k16k5.yundunddos.com wrote:
>>>
>>> From: Gao Feng <fgao@ikuai8.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@ikuai8.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.
Yup, sorry. Missed that.
>
>>
>>> +
>>> +#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 ?
Sorry, I phrased that poorly. Yes, they're both defined (in different
headers)... but when you're parsing the v0 header you're not referencing
the gre_base_hdr members to calculate your offsets.
-Philip
>
>>
>>> +
>>> +
>>> +#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
>>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash
2016-08-04 0:33 ` Philp Prindeville
@ 2016-08-04 7:37 ` Feng Gao
2016-08-04 13:11 ` Feng Gao
0 siblings, 1 reply; 8+ messages in thread
From: Feng Gao @ 2016-08-04 7:37 UTC (permalink / raw)
To: Philp Prindeville
Cc: fgao, David S. Miller, Stephen Hemminger, Pravin B Shelar,
Tom Herbert, Alex Duyck, Linux Kernel Network Developers
Hi Tom & Philp,
The v4 patch is sent already.
Could you help review again please?
Tom,
I follow your modification.
Philp,
I define one new struct gre_full_hdr which contains the completed gre
header, for example, csum, key, and so on.
And these members are not defined in gre_base_hdr.
It is only used to offset the sizeof type.
BTW, I find the struct and macro about pptp and gre are redundant.
I want to refactor them in other patches.
On Thu, Aug 4, 2016 at 8:33 AM, Philp Prindeville
<philipp@redfish-solutions.com> wrote:
> Inline
>
>
>
> On 08/03/2016 05:58 PM, Feng Gao wrote:
>>
>> 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@redfish-solutions.com> wrote:
>>>
>>> Inline…
>>>
>>>> On Aug 3, 2016, at 8:52 AM, fgao@48lvckh6395k16k5.yundunddos.com wrote:
>>>>
>>>> From: Gao Feng <fgao@ikuai8.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@ikuai8.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.
>
>
> Yup, sorry. Missed that.
>
>
>
>>
>>>
>>>> +
>>>> +#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 ?
>
>
> Sorry, I phrased that poorly. Yes, they're both defined (in different
> headers)... but when you're parsing the v0 header you're not referencing the
> gre_base_hdr members to calculate your offsets.
>
> -Philip
>
>
>
>>
>>>
>>>> +
>>>> +
>>>> +#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
>>>>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash
2016-08-04 7:37 ` Feng Gao
@ 2016-08-04 13:11 ` Feng Gao
0 siblings, 0 replies; 8+ messages in thread
From: Feng Gao @ 2016-08-04 13:11 UTC (permalink / raw)
To: Philp Prindeville
Cc: fgao, David S. Miller, Stephen Hemminger, Pravin B Shelar,
Tom Herbert, Alex Duyck, Linux Kernel Network Developers
The link is https://patchwork.ozlabs.org/patch/655695/
Best Regards
Feng
On Thu, Aug 4, 2016 at 3:37 PM, Feng Gao <gfree.wind@gmail.com> wrote:
> Hi Tom & Philp,
>
> The v4 patch is sent already.
> Could you help review again please?
>
> Tom,
> I follow your modification.
>
> Philp,
> I define one new struct gre_full_hdr which contains the completed gre
> header, for example, csum, key, and so on.
> And these members are not defined in gre_base_hdr.
> It is only used to offset the sizeof type.
>
> BTW, I find the struct and macro about pptp and gre are redundant.
> I want to refactor them in other patches.
>
> On Thu, Aug 4, 2016 at 8:33 AM, Philp Prindeville
> <philipp@redfish-solutions.com> wrote:
>> Inline
>>
>>
>>
>> On 08/03/2016 05:58 PM, Feng Gao wrote:
>>>
>>> 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@redfish-solutions.com> wrote:
>>>>
>>>> Inline…
>>>>
>>>>> On Aug 3, 2016, at 8:52 AM, fgao@48lvckh6395k16k5.yundunddos.com wrote:
>>>>>
>>>>> From: Gao Feng <fgao@ikuai8.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@ikuai8.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.
>>
>>
>> Yup, sorry. Missed that.
>>
>>
>>
>>>
>>>>
>>>>> +
>>>>> +#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 ?
>>
>>
>> Sorry, I phrased that poorly. Yes, they're both defined (in different
>> headers)... but when you're parsing the v0 header you're not referencing the
>> gre_base_hdr members to calculate your offsets.
>>
>> -Philip
>>
>>
>>
>>>
>>>>
>>>>> +
>>>>> +
>>>>> +#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
>>>>>
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-08-04 13:23 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-03 14:52 [PATCH v3 1/1] rps: Inspect PPTP encapsulated by GRE to get flow hash fgao
2016-08-03 16:15 ` Tom Herbert
2016-08-03 23:47 ` Feng Gao
2016-08-03 20:43 ` Philip Prindeville
2016-08-03 23:58 ` Feng Gao
2016-08-04 0:33 ` Philp Prindeville
2016-08-04 7:37 ` Feng Gao
2016-08-04 13:11 ` Feng Gao
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.