lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Mon, 8 Aug 2016 09:20:19 -0600
From:	Philp Prindeville <philipp@...fish-solutions.com>
To:	Feng Gao <gfree.wind@...il.com>
Cc:	fgao@...vckh6395k16k5.yundunddos.com,
	"David S. Miller" <davem@...emloft.net>,
	Stephen Hemminger <stephen@...workplumber.org>,
	Pravin B Shelar <pshelar@...ira.com>,
	Tom Herbert <tom@...bertland.com>,
	Alex Duyck <aduyck@...antis.com>,
	Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH v4 1/1] rps: Inspect PPTP encapsulated by GRE to get flow
 hash

No, I was referring to anonymous structures, which is a feature of C11.

Please see the link I sent.


On 08/08/2016 03:13 AM, Feng Gao wrote:
> Hi Philip,
>
> Do you mean like the following?
>
> struct gre_full_hdr {
>          struct {
>                  __be16 flags;
>                  __be16 protocol;
>          } fixed_header;
>          __be16 csum;
>          __be16 reserved1;
>          __be32 key;
>          __be32 seq;
> } __packed;
>
> But we need struct gre_base_hdr to get the fixed header of GRE in
> function __skb_flow_dissect like the following codes.
> struct gre_base_hdr *hdr, _hdr;
> hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data, hlen, &_hdr);
>
> BTW, the original codes define one local stuct gre_hdr. Now I use the
> unified struct gre_base_hdr instead of it.
>
> Best Regards
> Feng
>
>
> On Mon, Aug 8, 2016 at 11:27 AM, Philp Prindeville
> <philipp@...fish-solutions.com> wrote:
>> Feng,
>>
>> An anonymous structure is defined here:
>> https://gcc.gnu.org/onlinedocs/gcc/Unnamed-Fields.html
>>
>> i.e.:
>>
>> struct gre_full_hdr {
>>      struct gre_base_hdr;
>>      ...
>>
>> so yes, I'm talking about making fixed_header be anonymous instead.
>>
>> -Philip
>>
>>
>>
>> On 08/07/2016 08:50 PM, Feng Gao wrote:
>>> Hi Philp,
>>>
>>> Forgive my poor English, I am not clear about the comment #1.
>>> "Can you make gre_base_hdr be anonymous?".
>>>
>>> +struct gre_full_hdr {
>>> +   struct gre_base_hdr fixed_header;
>>>
>>> Do you mean make the member "fixed_header" as anonymous or not?
>>>
>>> Best Regards
>>> Feng
>>>
>>>
>>> On Mon, Aug 8, 2016 at 5:03 AM, Philp Prindeville
>>> <philipp@...fish-solutions.com> wrote:
>>>> Inline...
>>>>
>>>>
>>>>
>>>> On 08/04/2016 01:06 AM, fgao@...vckh6395k16k5.yundunddos.com wrote:
>>>>> From: Gao Feng <fgao@...ai8.com>
>>>>>
>>>>> The PPTP is encapsulated by GRE header with that GRE_VERSION bits
>>>>> must contain one. But current GRE RPS needs the GRE_VERSION must be
>>>>> zero. So RPS does not work for PPTP traffic.
>>>>>
>>>>> In my test environment, there are four MIPS cores, and all traffic
>>>>> are passed through by PPTP. As a result, only one core is 100% busy
>>>>> while other three cores are very idle. After this patch, the usage
>>>>> of four cores are balanced well.
>>>>>
>>>>> Signed-off-by: Gao Feng <fgao@...ai8.com>
>>>>> ---
>>>>>     v4: 1) Define struct gre_full_hdr, and use sizeof its member
>>>>> directly;
>>>>>         2) Move version and routing check ahead;
>>>>>         3) Only PPTP in GRE check the ack flag;
>>>>>     v3: 1) Move struct pptp_gre_header defination into new file pptp.h
>>>>>         2) Use sizeof GRE and PPTP type instead of literal value;
>>>>>         3) Remove strict flag check for PPTP to robust;
>>>>>         4) Consolidate the codes again;
>>>>>     v2: Update according to Tom and Philp's advice.
>>>>>         1) Consolidate the codes with GRE version 0 path;
>>>>>         2) Use PPP_PROTOCOL to get ppp protol;
>>>>>         3) Set the FLOW_DIS_ENCAPSULATION flag;
>>>>>     v1: Intial Patch
>>>>>
>>>>>     drivers/net/ppp/pptp.c         |  36 +------------
>>>>>     include/net/gre.h              |  10 +++-
>>>>>     include/net/pptp.h             |  40 +++++++++++++++
>>>>>     include/uapi/linux/if_tunnel.h |   7 ++-
>>>>>     net/core/flow_dissector.c      | 113
>>>>> ++++++++++++++++++++++++++++-------------
>>>>>     5 files changed, 135 insertions(+), 71 deletions(-)
>>>>>     create mode 100644 include/net/pptp.h
>>>>>
>>>>> diff --git a/drivers/net/ppp/pptp.c b/drivers/net/ppp/pptp.c
>>>>> index ae0905e..3e68dbc 100644
>>>>> --- a/drivers/net/ppp/pptp.c
>>>>> +++ b/drivers/net/ppp/pptp.c
>>>>> @@ -37,6 +37,7 @@
>>>>>     #include <net/icmp.h>
>>>>>     #include <net/route.h>
>>>>>     #include <net/gre.h>
>>>>> +#include <net/pptp.h>
>>>>>       #include <linux/uaccess.h>
>>>>>     @@ -53,41 +54,6 @@ static struct proto pptp_sk_proto __read_mostly;
>>>>>     static const struct ppp_channel_ops pptp_chan_ops;
>>>>>     static const struct proto_ops pptp_ops;
>>>>>     -#define PPP_LCP_ECHOREQ 0x09
>>>>> -#define PPP_LCP_ECHOREP 0x0A
>>>>> -#define SC_RCV_BITS
>>>>> (SC_RCV_B7_1|SC_RCV_B7_0|SC_RCV_ODDP|SC_RCV_EVNP)
>>>>> -
>>>>> -#define MISSING_WINDOW 20
>>>>> -#define WRAPPED(curseq, lastseq)\
>>>>> -       ((((curseq) & 0xffffff00) == 0) &&\
>>>>> -       (((lastseq) & 0xffffff00) == 0xffffff00))
>>>>> -
>>>>> -#define PPTP_GRE_PROTO  0x880B
>>>>> -#define PPTP_GRE_VER    0x1
>>>>> -
>>>>> -#define PPTP_GRE_FLAG_C        0x80
>>>>> -#define PPTP_GRE_FLAG_R        0x40
>>>>> -#define PPTP_GRE_FLAG_K        0x20
>>>>> -#define PPTP_GRE_FLAG_S        0x10
>>>>> -#define PPTP_GRE_FLAG_A        0x80
>>>>> -
>>>>> -#define PPTP_GRE_IS_C(f) ((f)&PPTP_GRE_FLAG_C)
>>>>> -#define PPTP_GRE_IS_R(f) ((f)&PPTP_GRE_FLAG_R)
>>>>> -#define PPTP_GRE_IS_K(f) ((f)&PPTP_GRE_FLAG_K)
>>>>> -#define PPTP_GRE_IS_S(f) ((f)&PPTP_GRE_FLAG_S)
>>>>> -#define PPTP_GRE_IS_A(f) ((f)&PPTP_GRE_FLAG_A)
>>>>> -
>>>>> -#define PPTP_HEADER_OVERHEAD (2+sizeof(struct pptp_gre_header))
>>>>> -struct pptp_gre_header {
>>>>> -       u8  flags;
>>>>> -       u8  ver;
>>>>> -       __be16 protocol;
>>>>> -       __be16 payload_len;
>>>>> -       __be16 call_id;
>>>>> -       __be32 seq;
>>>>> -       __be32 ack;
>>>>> -} __packed;
>>>>> -
>>>>>     static struct pppox_sock *lookup_chan(u16 call_id, __be32 s_addr)
>>>>>     {
>>>>>           struct pppox_sock *sock;
>>>>> diff --git a/include/net/gre.h b/include/net/gre.h
>>>>> index 7a54a31..c469dcc 100644
>>>>> --- a/include/net/gre.h
>>>>> +++ b/include/net/gre.h
>>>>> @@ -7,9 +7,17 @@
>>>>>     struct gre_base_hdr {
>>>>>           __be16 flags;
>>>>>           __be16 protocol;
>>>>> -};
>>>>> +} __packed;
>>>>>     #define GRE_HEADER_SECTION 4
>>>>>     +struct gre_full_hdr {
>>>>> +       struct gre_base_hdr fixed_header;
>>>>
>>>> Can you make gre_base_hdr be anonymous?
>>>>
>>>>
>>>>
>>>>> +       __be16 csum;
>>>>> +       __be16 reserved1;
>>>>> +       __be32 key;
>>>>> +       __be32 seq;
>>>>> +} __packed;
>>>>> +
>>>>>     #define GREPROTO_CISCO                0
>>>>>     #define GREPROTO_PPTP         1
>>>>>     #define GREPROTO_MAX          2
>>>>> diff --git a/include/net/pptp.h b/include/net/pptp.h
>>>>> new file mode 100644
>>>>> index 0000000..301d3e2
>>>>> --- /dev/null
>>>>> +++ b/include/net/pptp.h
>>>>> @@ -0,0 +1,40 @@
>>>>> +#ifndef _NET_PPTP_H
>>>>> +#define _NET_PPTP_H
>>>>> +
>>>>> +#define PPP_LCP_ECHOREQ 0x09
>>>>> +#define PPP_LCP_ECHOREP 0x0A
>>>>> +#define SC_RCV_BITS
>>>>> (SC_RCV_B7_1|SC_RCV_B7_0|SC_RCV_ODDP|SC_RCV_EVNP)
>>>>> +
>>>>> +#define MISSING_WINDOW 20
>>>>> +#define WRAPPED(curseq, lastseq)\
>>>>> +       ((((curseq) & 0xffffff00) == 0) &&\
>>>>> +       (((lastseq) & 0xffffff00) == 0xffffff00))
>>>>> +
>>>>> +#define PPTP_GRE_PROTO  0x880B
>>>>> +#define PPTP_GRE_VER    0x1
>>>>> +
>>>>> +#define PPTP_GRE_FLAG_C 0x80
>>>>> +#define PPTP_GRE_FLAG_R 0x40
>>>>> +#define PPTP_GRE_FLAG_K 0x20
>>>>> +#define PPTP_GRE_FLAG_S 0x10
>>>>> +#define PPTP_GRE_FLAG_A 0x80
>>>>> +
>>>>> +#define PPTP_GRE_IS_C(f) ((f)&PPTP_GRE_FLAG_C)
>>>>> +#define PPTP_GRE_IS_R(f) ((f)&PPTP_GRE_FLAG_R)
>>>>> +#define PPTP_GRE_IS_K(f) ((f)&PPTP_GRE_FLAG_K)
>>>>> +#define PPTP_GRE_IS_S(f) ((f)&PPTP_GRE_FLAG_S)
>>>>> +#define PPTP_GRE_IS_A(f) ((f)&PPTP_GRE_FLAG_A)
>>>>> +
>>>>> +#define PPTP_HEADER_OVERHEAD (2+sizeof(struct pptp_gre_header))
>>>>> +struct pptp_gre_header {
>>>>> +       u8  flags;
>>>>> +       u8  ver;
>>>>> +       __be16 protocol;
>>>>> +       __be16 payload_len;
>>>>> +       __be16 call_id;
>>>>> +       __be32 seq;
>>>>> +       __be32 ack;
>>>>> +} __packed;
>>>>> +
>>>>> +
>>>>> +#endif
>>>>> diff --git a/include/uapi/linux/if_tunnel.h
>>>>> b/include/uapi/linux/if_tunnel.h
>>>>> index 1046f55..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..1186712 100644
>>>>> --- a/net/core/flow_dissector.c
>>>>> +++ b/net/core/flow_dissector.c
>>>>> @@ -6,6 +6,8 @@
>>>>>     #include <linux/if_vlan.h>
>>>>>     #include <net/ip.h>
>>>>>     #include <net/ipv6.h>
>>>>> +#include <net/gre.h>
>>>>> +#include <net/pptp.h>
>>>>>     #include <linux/igmp.h>
>>>>>     #include <linux/icmp.h>
>>>>>     #include <linux/sctp.h>
>>>>> @@ -338,32 +340,42 @@ mpls:
>>>>>     ip_proto_again:
>>>>>           switch (ip_proto) {
>>>>>           case IPPROTO_GRE: {
>>>>> -               struct gre_hdr {
>>>>> -                       __be16 flags;
>>>>> -                       __be16 proto;
>>>>> -               } *hdr, _hdr;
>>>>> +               struct gre_base_hdr *hdr, _hdr;
>>>>> +               u16 gre_ver;
>>>>> +               int offset = 0;
>>>>>                   hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr),
>>>>> data,
>>>>> hlen, &_hdr);
>>>>>                   if (!hdr)
>>>>>                           goto out_bad;
>>>>> -               /*
>>>>> -                * Only look inside GRE if version zero and no
>>>>> -                * routing
>>>>> -                */
>>>>> -               if (hdr->flags & (GRE_VERSION | GRE_ROUTING))
>>>>> +
>>>>> +               /* Only look inside GRE without routing */
>>>>> +               if (hdr->flags & GRE_ROUTING)
>>>>>                           break;
>>>>>     -             proto = hdr->proto;
>>>>> -               nhoff += 4;
>>>>> +               /* Only look inside GRE for version 0 and 1 */
>>>>> +               gre_ver = ntohs(hdr->flags & GRE_VERSION);
>>>>> +               if (gre_ver > 1)
>>>>> +                       break;
>>>>> +
>>>>> +               proto = hdr->protocol;
>>>>> +               if (gre_ver) {
>>>>> +                       /* Version1 must be PPTP, and check the flags */
>>>>> +                       if (!(proto == GRE_PROTO_PPP && (hdr->flags &
>>>>> GRE_KEY)))
>>>>> +                               break;
>>>>> +               }
>>>>> +
>>>>> +               offset += sizeof(struct gre_base_hdr);
>>>>> +
>>>>>                   if (hdr->flags & GRE_CSUM)
>>>>> -                       nhoff += 4;
>>>>> +                       offset += sizeof(((struct gre_full_hdr
>>>>> *)0)->csum)
>>>>> +
>>>>> +                                 sizeof(((struct gre_full_hdr
>>>>> *)0)->reserved1);
>>>>> +
>>>>>                   if (hdr->flags & GRE_KEY) {
>>>>>                           const __be32 *keyid;
>>>>>                           __be32 _keyid;
>>>>>     -                     keyid = __skb_header_pointer(skb, nhoff,
>>>>> sizeof(_keyid),
>>>>> +                       keyid = __skb_header_pointer(skb, nhoff +
>>>>> offset,
>>>>> sizeof(_keyid),
>>>>>                                                        data, hlen,
>>>>> &_keyid);
>>>>> -
>>>>>                           if (!keyid)
>>>>>                                   goto out_bad;
>>>>>     @@ -372,32 +384,65 @@ ip_proto_again:
>>>>>                                   key_keyid =
>>>>> skb_flow_dissector_target(flow_dissector,
>>>>>
>>>>> FLOW_DISSECTOR_KEY_GRE_KEYID,
>>>>>
>>>>> target_container);
>>>>> -                               key_keyid->keyid = *keyid;
>>>>> +                               if (gre_ver == 0)
>>>>> +                                       key_keyid->keyid = *keyid;
>>>>> +                               else
>>>>> +                                       key_keyid->keyid = *keyid &
>>>>> htonl(0xffff);
>>>>
>>>> Would be good to have a #define'd constant for the keyid mask...
>>>> Otherwise,
>>>> looks good.
>>>>
>>>> -Philip
>>>>
>>>>
>>>>>                           }
>>>>> -                       nhoff += 4;
>>>>> +                       offset += sizeof(((struct gre_full_hdr
>>>>> *)0)->key);
>>>>>                   }
>>>>> +
>>>>>                   if (hdr->flags & GRE_SEQ)
>>>>> -                       nhoff += 4;
>>>>> -               if (proto == htons(ETH_P_TEB)) {
>>>>> -                       const struct ethhdr *eth;
>>>>> -                       struct ethhdr _eth;
>>>>> -
>>>>> -                       eth = __skb_header_pointer(skb, nhoff,
>>>>> -                                                  sizeof(_eth),
>>>>> -                                                  data, hlen, &_eth);
>>>>> -                       if (!eth)
>>>>> +                       offset += sizeof(((struct pptp_gre_header
>>>>> *)0)->seq);
>>>>> +
>>>>> +               if (gre_ver == 0) {
>>>>> +                       if (proto == htons(ETH_P_TEB)) {
>>>>> +                               const struct ethhdr *eth;
>>>>> +                               struct ethhdr _eth;
>>>>> +
>>>>> +                               eth = __skb_header_pointer(skb, nhoff +
>>>>> offset,
>>>>> +                                                          sizeof(_eth),
>>>>> +                                                          data, hlen,
>>>>> &_eth);
>>>>> +                               if (!eth)
>>>>> +                                       goto out_bad;
>>>>> +                               proto = eth->h_proto;
>>>>> +                               offset += sizeof(*eth);
>>>>> +
>>>>> +                               /* Cap headers that we access via
>>>>> pointers
>>>>> at the
>>>>> +                                * end of the Ethernet header as our
>>>>> maximum alignment
>>>>> +                                * at that point is only 2 bytes.
>>>>> +                                */
>>>>> +                               if (NET_IP_ALIGN)
>>>>> +                                       hlen = (nhoff + offset);
>>>>> +                       }
>>>>> +               } else { /* version 1, must be PPTP */
>>>>> +                       u8 _ppp_hdr[PPP_HDRLEN];
>>>>> +                       u8 *ppp_hdr;
>>>>> +
>>>>> +                       if (hdr->flags & GRE_ACK)
>>>>> +                               offset += sizeof(((struct
>>>>> pptp_gre_header
>>>>> *)0)->ack);
>>>>> +
>>>>> +                       ppp_hdr = skb_header_pointer(skb, nhoff +
>>>>> offset,
>>>>> +                                                    sizeof(_ppp_hdr),
>>>>> _ppp_hdr);
>>>>> +                       if (!ppp_hdr)
>>>>>                                   goto out_bad;
>>>>> -                       proto = eth->h_proto;
>>>>> -                       nhoff += sizeof(*eth);
>>>>> -
>>>>> -                       /* Cap headers that we access via pointers at
>>>>> the
>>>>> -                        * end of the Ethernet header as our maximum
>>>>> alignment
>>>>> -                        * at that point is only 2 bytes.
>>>>> -                        */
>>>>> -                       if (NET_IP_ALIGN)
>>>>> -                               hlen = nhoff;
>>>>> +
>>>>> +                       switch (PPP_PROTOCOL(ppp_hdr)) {
>>>>> +                       case PPP_IP:
>>>>> +                               proto = htons(ETH_P_IP);
>>>>> +                               break;
>>>>> +                       case PPP_IPV6:
>>>>> +                               proto = htons(ETH_P_IPV6);
>>>>> +                               break;
>>>>> +                       default:
>>>>> +                               /* Could probably catch some more like
>>>>> MPLS */
>>>>> +                               break;
>>>>> +                       }
>>>>> +
>>>>> +                       offset += PPP_HDRLEN;
>>>>>                   }
>>>>>     +             nhoff += offset;
>>>>>                   key_control->flags |= FLOW_DIS_ENCAPSULATION;
>>>>>                   if (flags & FLOW_DISSECTOR_F_STOP_AT_ENCAP)
>>>>>                           goto out_good;
>>>>

Powered by blists - more mailing lists