netdev - Re: [PATCH net-next v2 01/12] flow_dissector: Parse ETH_P

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CALx6S35b-YPCaen7D0THQ++giSM6cXJHVOtysg1pi5itKT-mFA@mail.gmail.com>
Date: Tue, 20 Aug 2024 09:42:05 -0700
From: Tom Herbert <tom@...bertland.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: davem@...emloft.net, kuba@...nel.org, edumazet@...gle.com, 
	netdev@...r.kernel.org, felipe@...anda.io
Subject: Re: [PATCH net-next v2 01/12] flow_dissector: Parse ETH_P_TEB and
 move out of GRE

On Tue, Aug 20, 2024 at 9:30 AM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> Tom Herbert wrote:
> > On Fri, Aug 16, 2024 at 11:54 AM Willem de Bruijn
> > <willemdebruijn.kernel@...il.com> wrote:
> > >
> > > Tom Herbert wrote:
> > > > ETH_P_TEB (Trans Ether Bridging) is the EtherType to carry
> > > > a plain Etherent frame. Add case in skb_flow_dissect to parse
> > > > packets of this type
> > > >
> > > > If the GRE protocol is ETH_P_TEB then just process that as any
> > > > another EtherType since it's now supported in the main loop
> > > >
> > > > Signed-off-by: Tom Herbert <tom@...bertland.com>
> > >
> > > Reviewed-by: Willem de Bruijn <willemb@...gle.com>
> > >
> > > > -     if (gre_ver == 0) {
> > > > -             if (*p_proto == htons(ETH_P_TEB)) {
> > > > -                     const struct ethhdr *eth;
> > > > -                     struct ethhdr _eth;
> > > > -
> > > > -                     eth = __skb_header_pointer(skb, *p_nhoff + offset,
> > > > -                                                sizeof(_eth),
> > > > -                                                data, *p_hlen, &_eth);
> > > > -                     if (!eth)
> > > > -                             return FLOW_DISSECT_RET_OUT_BAD;
> > > > -                     *p_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)
> > > > -                             *p_hlen = *p_nhoff + offset;
> > > > -             }
> > > > -     } else { /* version 1, must be PPTP */
> > >
> > > > @@ -1284,6 +1268,27 @@ bool __skb_flow_dissect(const struct net *net,
> > > >
> > > >               break;
> > > >       }
> > > > +     case 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;
> > >
> > > I wonder why this exists. But besides the point of this move.
> >
> > Willem,
> >
> > Ethernet header breaks 4-byte alignment of encapsulated protocols
> > since it's 14 bytes, so the NET_IP_ALIGN can be used on architectures
> > that don't like unaligned loads.
>
> I understand how NET_IP_ALIGN is used by drivers.
>
> I don't understand its use here in the flow dissector. Why is hlen
> capped if it is set?

Willem,

For the real Ethernet header the receive skbuf is offset by two so
that device places the packet such that the Ethernet payload, i.e. IP
header, is aligned to four bytes (14+2=16 which will be offset of IP
header). When a packets contains an encapsulated Ethernet header, the
offset of the header is aligned to four bytes which means the payload
of that Ethernet header, i.e. an encapsulated IP header, is not four
byte aligned and neither are any subsequent headers (TCP, UDP, etc.).
On some architectures, performing unaligned loads is expensive
compared to aligned loads, so hlen is being capped here to avoid
having flow dissector do that on unaligned headers after the Ethernet
header. It's a tradeoff between performance and deeper flow
dissection.

Tom