Message-ID: <ZEFuFqMjO19De/e/@debian>
Date:   Thu, 20 Apr 2023 18:53:42 +0200
From:   Guillaume Nault <gnault@...hat.com>
To:     Aleksey Shumnik <ashumnik9@...il.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>, waltje@...lt.nl.mugnet.org,
        gw4pts@...pts.ampr.org, xeb@...l.ru, kuznet@....inr.ac.ru,
        rzsfl@...uni-sb.de
Subject: Re: [BUG] In af_packet.c::dev_parse_header() skb.network_header does
 not point to the network header

On Tue, Apr 11, 2023 at 05:47:34PM +0300, Aleksey Shumnik wrote:
> Dear maintainers,
> 
> I wrote the ip6gre_header_parser() function in ip6_gre.c, the
> implementation is similar to ipgre_header_parser() in ip_gre.c. (By
> the way, it is strange that this function is not implemented in
> ip6_gre.c)
> The implementation of the function is presented below.
> It should parse the ip6 header and take the source address and its
> length from there. To get a pointer to the ip header, it is logical to
> use skb_network_header(), but it does not work correctly and returns a
> pointer to payload (skb.data).

At this point, the tunnel device has already removed the outer headers.
So skb_network_header() points to the _inner_ network header.

> Also in ip_gre.c::ipgre_header_parser() skb_mac_header() returns a
> pointer to the ip header and everything works correctly (although it
> seems that this is also an error, because the pointer to the mac
> header should have been returned, and logically the
> skb_network_header() function should be applied),

Well, skb_mac_header() and skb_network_header() should point to the
inner mac and network headers respectively. However, because ip_gre
has no inner mac header, skb_mac_header() was repurposed to save the
position of the outer network header (so that ipgre_header_parse() can
find it).

> but in ip6_gre.c all
> skb_mac_header(), skb_network_header(), skb_tranport_header() returns
> a pointer to payload (skb.data).

The packet has already been decapsulated by the tunnel device: the
outer headers are gone. Therefore, the packet now starts right after
the gre header. So skb_mac_header() points there. And since ip6_gre
transports packets with no mac header, the mac header length is zero,
which means skb_network_header() equals skb_mac_header() and points to
the inner network header. Finally, as the inner network header hasn't
been parsed yet, we don't know where it ends, so skb_tranport_header()
isn't usable yet.

> This function is called when receiving a packet and parsing it in
> af_packet.c::packet_rcv() in dev_parse_header().
> The problem is that there is no way to accurately determine the
> beginning of the ip header.
> 
> diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
> index 90565b8..0d0c37b 100644
> --- a/net/ipv6/ip6_gre.c
> +++ b/net/ipv6/ip6_gre.c
> @@ -1404,8 +1404,16 @@ static int ip6gre_header(struct sk_buff *skb,
> struct net_device *dev,
>   return -t->hlen;
>  }
> 
> +static int ip6gre_header_parse(const struct sk_buff *skb, unsigned char *haddr)
> +{
> + const struct ipv6hdr *ipv6h = (const struct ipv6hdr *) skb_mac_header(skb);
> + memcpy(haddr, &ipv6h->saddr, 16);
> + return 16;
> +}
> +
>  static const struct header_ops ip6gre_header_ops = {
>   .create = ip6gre_header,
> + .parse = ip6gre_header_parse,
>  };
> 
>  static const struct net_device_ops ip6gre_netdev_ops = {
> 
> Would you answer whether this behavior is an error and why the
> behavior in ip_gre.c and ip6_gre.c differs?

For me, ip_gre should make the mac header point to the inner mac
header, which incidentally is also the inner network header.

The difference in behaviour between ip_gre and ip6_gre certainly comes
from the fact that both modules were implemented at different times, by
different people.

> Regards,
> Aleksey
> 

Powered by blists - more mailing lists