The link is https://patchwork.ozlabs.org/patch/655695/
Best Regards
Feng
On Thu, Aug 4, 2016 at 3:37 PM, Feng Gao <gfree.w...@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
> <phil...@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
>>> <phil...@redfish-solutions.com> wrote:
>>>>
>>>> Inline…
>>>>
>>>>> On Aug 3, 2016, at 8:52 AM, f...@48lvckh6395k16k5.yundunddos.com wrote:
>>>>>
>>>>> From: Gao Feng <f...@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 <f...@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
>>>>>
>>
