Date:   Thu, 23 Jun 2022 16:51:31 +0300
From:   Aleksey Shumnik <ashumnik9@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        kuznet@....inr.ac.ru, xeb@...l.ru
Subject: Re: [PATCH] net/ipv4/ip_gre.c net/ipv6/ip6_gre.c: ip and gre header
 are recorded twice

On Thu, Jun 23, 2022 at 3:19 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Tue, 21 Jun 2022 16:48:09 +0300 Aleksey Shumnik wrote:
> > Subject: [PATCH] net/ipv4/ip_gre.c net/ipv6/ip6_gre.c: ip and gre header are  recorded twice
> > Date: Tue, 21 Jun 2022 16:48:09 +0300
> >
> > Dear Maintainers,
> >
> > I tried to ping IPv6 hub address on the mGRE interface from the spok
> > and found some problem:
> > I caught packets and saw that there are 2 identical IP and GRE headers
> > (when use IPv4 there is no duplication)
> > Below is the package structure:
> > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> > | eth | iph (1) | greh (1) | iph (1) | greh (1) | iph (2) | greh (2) |  icmp  |
> > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>
> What socket type is the ping you have using?

I use SOCK_DGRAM

> > I found cause of the problem, in ip_gre.c and ip6_gre.c IP and GRE
> > headers created twice, first time in ip gre_header() and
> > ip6gre_header() and second time in __gre_xmit(), so I deleted
> > unnecessary creation of headers and everything started working as it
> > should.
> > Below is a patch to eliminate the problem of duplicate headers:
> >
> > diff -c a/net/inv6/ip6_gre.c b/net/inv6/ip6_gre.c
>
> The patch looks strangely mangled, it's white space damaged and refers
> to a net/inv6 which does not exist.
>
> Could you regenerate your changes using git? git commit / format-patch
> / send-email ?

Thanks a lot for the answer!
I want to find out, the creation of gre and ip header twice, is it a
feature or a bug?

I did everything according to the instructions, hope everything is
correct this time.

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 3b9cd48..5e8907b 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -836,43 +836,6 @@ static int ipgre_tunnel_ctl(struct net_device
*dev, struct ip_tunnel_parm *p,
    ftp fec0:6666:6666::193.233.7.65
    ...
  */
-static int ipgre_header(struct sk_buff *skb, struct net_device *dev,
- unsigned short type,
- const void *daddr, const void *saddr, unsigned int len)
-{
- struct ip_tunnel *t = netdev_priv(dev);
- struct iphdr *iph;
- struct gre_base_hdr *greh;
-
- iph = skb_push(skb, t->hlen + sizeof(*iph));
- greh = (struct gre_base_hdr *)(iph+1);
- greh->flags = gre_tnl_flags_to_gre_flags(t->parms.o_flags);
- greh->protocol = htons(type);
-
- memcpy(iph, &t->parms.iph, sizeof(struct iphdr));
-
- /* Set the source hardware address. */
- if (saddr)
- memcpy(&iph->saddr, saddr, 4);
- if (daddr)
- memcpy(&iph->daddr, daddr, 4);
- if (iph->daddr)
- return t->hlen + sizeof(*iph);
-
- return -(t->hlen + sizeof(*iph));
-}
-
-static int ipgre_header_parse(const struct sk_buff *skb, unsigned char *haddr)
-{
- const struct iphdr *iph = (const struct iphdr *) skb_mac_header(skb);
- memcpy(haddr, &iph->saddr, 4);
- return 4;
-}
-
-static const struct header_ops ipgre_header_ops = {
- .create = ipgre_header,
- .parse = ipgre_header_parse,
-};

 #ifdef CONFIG_NET_IPGRE_BROADCAST
 static int ipgre_open(struct net_device *dev)
diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 4e37f7c..add7c5c 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -1358,45 +1358,6 @@ done:
  return err;
 }

-static int ip6gre_header(struct sk_buff *skb, struct net_device *dev,
- unsigned short type, const void *daddr,
- const void *saddr, unsigned int len)
-{
- struct ip6_tnl *t = netdev_priv(dev);
- struct ipv6hdr *ipv6h;
- __be16 *p;
-
- ipv6h = skb_push(skb, t->hlen + sizeof(*ipv6h));
- ip6_flow_hdr(ipv6h, 0, ip6_make_flowlabel(dev_net(dev), skb,
-   t->fl.u.ip6.flowlabel,
-   true, &t->fl.u.ip6));
- ipv6h->hop_limit = t->parms.hop_limit;
- ipv6h->nexthdr = NEXTHDR_GRE;
- ipv6h->saddr = t->parms.laddr;
- ipv6h->daddr = t->parms.raddr;
-
- p = (__be16 *)(ipv6h + 1);
- p[0] = t->parms.o_flags;
- p[1] = htons(type);
-
- /*
- * Set the source hardware address.
- */
-
- if (saddr)
- memcpy(&ipv6h->saddr, saddr, sizeof(struct in6_addr));
- if (daddr)
- memcpy(&ipv6h->daddr, daddr, sizeof(struct in6_addr));
- if (!ipv6_addr_any(&ipv6h->daddr))
- return t->hlen;
-
- return -t->hlen;
-}
-
-static const struct header_ops ip6gre_header_ops = {
- .create = ip6gre_header,
-};
-
 static const struct net_device_ops ip6gre_netdev_ops = {
  .ndo_init = ip6gre_tunnel_init,
  .ndo_uninit = ip6gre_tunnel_uninit,

Powered by blists - more mailing lists