[PATCH] net/ipv4/ip_gre.c net/ipv6/ip6_gre.c: ip and gre header are recorded twice

[PATCH] net/ipv4/ip_gre.c net/ipv6/ip6_gre.c: ip and gre header are recorded twice

[Aleksey Shumnik]

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  |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

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
*** net/inv6/ip6_gre.c
--- net/inv6/ip6_gre.c
***************
*** 1356,1400 ****
  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,
--- 1356,1361 ----
diff -c a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
*** net/ipv4/ip_gre.c
--- net/ipv4/ip_gre.c
***************
*** 831,873 ****
     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)
--- 831,836 ----

--
Aleksey Shumnik

Re: [PATCH] net/ipv4/ip_gre.c net/ipv6/ip6_gre.c: ip and gre header are recorded twice

[Jakub Kicinski]

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 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 ? 

Re: [PATCH] net/ipv4/ip_gre.c net/ipv6/ip6_gre.c: ip and gre header are recorded twice

[Aleksey Shumnik]

On Thu, Jun 23, 2022 at 3:19 AM Jakub Kicinski <kuba@kernel.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,

Re: [PATCH] net/ipv4/ip_gre.c net/ipv6/ip6_gre.c: ip and gre header are recorded twice

[Jakub Kicinski]

On Thu, 23 Jun 2022 16:51:31 +0300 Aleksey Shumnik wrote:
> > What socket type is the ping you have using?  
> 
> I use SOCK_DGRAM

Strange.

> > 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 can't think why that'd be a feature. Could add this case to selftests
to show how to repro and catch regressions?

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

Nope, still mangled.

Re: [PATCH] net/ipv4/ip_gre.c net/ipv6/ip6_gre.c: ip and gre header are recorded twice

[Aleksey Shumnik]

On Fri, Jun 24, 2022 at 6:26 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > > What socket type is the ping you have using?
> >
> > I use SOCK_DGRAM
>
> Strange.

Why is it strange?

> > I want to find out, the creation of gre and ip header twice, is it a
> > feature or a bug?
>
> I can't think why that'd be a feature. Could add this case to selftests
> to show how to repro and catch regressions?

I don't really know how to do it, but I'll try
If we just talk about selftests/net, then everything has passed

> > I did everything according to the instructions, hope everything is
> > correct this time.
>
> Nope, still mangled.

Strangely, everything works fine for me

Re: [PATCH] net/ipv4/ip_gre.c net/ipv6/ip6_gre.c: ip and gre header are recorded twice

[Jakub Kicinski]

On Fri, 24 Jun 2022 16:51:41 +0300 Aleksey Shumnik wrote:
> On Fri, Jun 24, 2022 at 6:26 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > > I use SOCK_DGRAM  
> >
> > Strange.  
> 
> Why is it strange?

I meant surprising, I'd have thought we could miss something like that
for RAW sockets maybe but DGRAM/ICMP should work.

> > > I want to find out, the creation of gre and ip header twice, is it a
> > > feature or a bug?  
> >
> > I can't think why that'd be a feature. Could add this case to selftests
> > to show how to repro and catch regressions?  
> 
> I don't really know how to do it, but I'll try
> If we just talk about selftests/net, then everything has passed

What I'm looking for is a bash(?) script which sets up the tunnel sends
a packet and checks if the headers are valid.

> > > I did everything according to the instructions, hope everything is
> > > correct this time.  
> >
> > Nope, still mangled.  
> 
> Strangely, everything works fine for me

Depends on definition of "works", are you saying you can download this:

https://lore.kernel.org/all/CAJGXZLiNo=G=5889sPyiCZVjRf65Ygov3=DWFgKmay+Dy3wCYw@mail.gmail.com/raw

which is your email in text form and `git am` will accept that as a
patch?

Re: [PATCH] net/ipv4/ip_gre.c net/ipv6/ip6_gre.c: ip and gre header are recorded twice

[Aleksey Shumnik]

On Fri, Jun 24, 2022 at 8:17 PM Jakub Kicinski <kuba@kernel.org> wrote:
> On Fri, 24 Jun 2022 16:51:41 +0300 Aleksey Shumnik wrote:
> > On Fri, Jun 24, 2022 at 6:26 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > > > I use SOCK_DGRAM
> > >
> > > Strange.
> >
> > Why is it strange?
>
> I meant surprising, I'd have thought we could miss something like that
> for RAW sockets maybe but DGRAM/ICMP should work.

I was surprised too.

> > > > I want to find out, the creation of gre and ip header twice, is it a
> > > > feature or a bug?
> > >
> > > I can't think why that'd be a feature. Could add this case to selftests
> > > to show how to repro and catch regressions?
> >
> > I don't really know how to do it, but I'll try
> > If we just talk about selftests/net, then everything has passed
>
> What I'm looking for is a bash(?) script which sets up the tunnel sends
> a packet and checks if the headers are valid.

I'm creating a file "mgre0" on spok, and use ifup to create the interface:

auto mgre0
iface mgre0 inet6 static
address 2001:470::1
netmask 64
pre-up ip tunnel add mgre0 mode ip6gre local 4444::1111 key 1 ttl 64 tos inherit
pre-up ethtool -K mgre0 tx off > /dev/null
pre-up ip link set mgre0 mtu 1400
pre-up ip link set mgre0 multicast on
post-down ip link del mgre0

do the same on hub:

auto mgre0
iface mgre0 inet6 static
address 2001:470::100
netmask 64
pre-up ip tunnel add mgre0 mode ip6gre local 4444::4444 key 1 ttl 64 tos inherit
pre-up ethtool -K mgre0 tx off > /dev/null
pre-up ip link set mgre0 mtu 1400
pre-up ip link set mgre0 multicast on
post-down ip link del mgre0

then I use ip neigh to add an entry to the neighbors table
spok:
$ ip -6 neigh add 2001:470::100 lladdr 4444::4444 dev mgre0

hub:
$ ip -6 neigh add 2001:470::1 lladdr 4444::1111 dev mgre0

and then ping hub from spok
$ ping 2001:470::100

To check if the headers are valid, I use tcpdump and look at the packets

> > > > I did everything according to the instructions, hope everything is
> > > > correct this time.
> > >
> > > Nope, still mangled.
> >
> > Strangely, everything works fine for me
>
> Depends on definition of "works", are you saying you can download this:
>
> https://lore.kernel.org/all/CAJGXZLiNo=G=5889sPyiCZVjRf65Ygov3=DWFgKmay+Dy3wCYw@mail.gmail.com/raw
>
> which is your email in text form and `git am` will accept that as a
> patch?

I use the kernel version 5.10.70.
I copy the code (starting from "diff" and up to the end), create a
patch file and apply it to the kernel.

Re: [PATCH] net/ipv4/ip_gre.c net/ipv6/ip6_gre.c: ip and gre header are recorded twice

[Jakub Kicinski]

On Tue, 28 Jun 2022 18:18:27 +0300 Aleksey Shumnik wrote:
> pre-up ip tunnel add mgre0 mode ip6gre local 4444::1111 key 1 ttl 64 tos inherit

I can't get GRE6 tunnels to work as NBMA net at all.
AFAICT ip6gre_tunnel_xmit() takes the endpoint addresses straight 
from the netdev, only ip6tnl seems to be doing a lookup.
Am I doing it wrong?

Re: [PATCH] net/ipv4/ip_gre.c net/ipv6/ip6_gre.c: ip and gre header are recorded twice

[Jakub Kicinski]

On Fri, 1 Jul 2022 18:31:51 -0700 Jakub Kicinski wrote:
> On Tue, 28 Jun 2022 18:18:27 +0300 Aleksey Shumnik wrote:
> > pre-up ip tunnel add mgre0 mode ip6gre local 4444::1111 key 1 ttl 64 tos inherit  
> 
> I can't get GRE6 tunnels to work as NBMA net at all.
> AFAICT ip6gre_tunnel_xmit() takes the endpoint addresses straight 
> from the netdev, only ip6tnl seems to be doing a lookup.
> Am I doing it wrong?

If it's just v4 could perhaps be commit fdafed459998 ("ip_gre: set
dev->hard_header_len and dev->needed_headroom properly"). Would you 
be able to try some kernel older than 5.8?

Re: [PATCH] net/ipv4/ip_gre.c net/ipv6/ip6_gre.c: ip and gre header are recorded twice

[Aleksey Shumnik]

On Sat, Jul 2, 2022 at 4:42 AM Jakub Kicinski <kuba@kernel.org> wrote:
> On Fri, 1 Jul 2022 18:31:51 -0700 Jakub Kicinski wrote:
> > On Tue, 28 Jun 2022 18:18:27 +0300 Aleksey Shumnik wrote:
> > > pre-up ip tunnel add mgre0 mode ip6gre local 4444::1111 key 1 ttl 64 tos inherit
> >
> > I can't get GRE6 tunnels to work as NBMA net at all.
> > AFAICT ip6gre_tunnel_xmit() takes the endpoint addresses straight
> > from the netdev, only ip6tnl seems to be doing a lookup.
> > Am I doing it wrong?

What exactly is the problem, may you describe it?
Have you added entries to the neighbors table?

> If it's just v4 could perhaps be commit fdafed459998 ("ip_gre: set
> dev->hard_header_len and dev->needed_headroom properly"). Would you
> be able to try some kernel older than 5.8?

I'll try.
dev->hard_header_len and dev->needed_headroom are set properly, but
the problem remains.
The problem with v4 is that the ip and gre headers are created 2 times
(1st in ipgre_header() and 2nd in gre_build_header() and
iptunnel_xmit()), they are overwritten, so that there is one gre and
one ip header in the packet.
Why take unnecessary actions if it could be created once.
v6 has the same problem, but also the packet has 2 same ip6 and gre
headers, duplication occurs, that is, they are not overwritten as in
v4.
v6 doesn't even have dev->hard_header_len.

Re: [PATCH] net/ipv4/ip_gre.c net/ipv6/ip6_gre.c: ip and gre header are recorded twice

[Jakub Kicinski]

On Thu, 7 Jul 2022 19:41:23 +0300 Aleksey Shumnik wrote:
> On Sat, Jul 2, 2022 at 4:42 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > On Fri, 1 Jul 2022 18:31:51 -0700 Jakub Kicinski wrote:  
> > > On Tue, 28 Jun 2022 18:18:27 +0300 Aleksey Shumnik wrote:  
> > > > pre-up ip tunnel add mgre0 mode ip6gre local 4444::1111 key 1 ttl 64 tos inherit  
> > >
> > > I can't get GRE6 tunnels to work as NBMA net at all.
> > > AFAICT ip6gre_tunnel_xmit() takes the endpoint addresses straight
> > > from the netdev, only ip6tnl seems to be doing a lookup.
> > > Am I doing it wrong?  
> 
> What exactly is the problem, may you describe it?
> Have you added entries to the neighbors table?

Yeah, I've added the neigh entries (although the v6 addresses had to 
be massaged a little for ip neigh to take them, the commands from the
email don't work cause iproute2 doesn't support :: in lladdr, AFAICT).

What I've seen in tracing was that I hit:

ip6gre_tunnel_xmit() -> ip6_tnl_xmit_ctl() -> ip6_tnl_get_cap()

that returns IP6_TNL_F_CAP_PER_PACKET

so back to ip6gre_tunnel_xmit() -> goto tx_err -> error, drop

packet never leaves the interface.

> > If it's just v4 could perhaps be commit fdafed459998 ("ip_gre: set
> > dev->hard_header_len and dev->needed_headroom properly"). Would you
> > be able to try some kernel older than 5.8?  
> 
> I'll try.
> dev->hard_header_len and dev->needed_headroom are set properly, but
> the problem remains.
> The problem with v4 is that the ip and gre headers are created 2 times
> (1st in ipgre_header() and 2nd in gre_build_header() and
> iptunnel_xmit()), they are overwritten, so that there is one gre and
> one ip header in the packet.
> Why take unnecessary actions if it could be created once.
> v6 has the same problem, but also the packet has 2 same ip6 and gre
> headers, duplication occurs, that is, they are not overwritten as in
> v4.
> v6 doesn't even have dev->hard_header_len.

Hm, so you did get v6 to repro? Not sure what I'm doing wrong, I'm
trying to repro with a net namespace over veth but that can't be it...

Re: [PATCH] net/ipv4/ip_gre.c net/ipv6/ip6_gre.c: ip and gre header are recorded twice

[Aleksey Shumnik]

On Fri, Jul 8, 2022 at 2:23 AM Jakub Kicinski <kuba@kernel.org> wrote:
> On Thu, 7 Jul 2022 19:41:23 +0300 Aleksey Shumnik wrote:
>
> Yeah, I've added the neigh entries (although the v6 addresses had to
> be massaged a little for ip neigh to take them, the commands from the
> email don't work cause iproute2 doesn't support :: in lladdr, AFAICT).
>
> What I've seen in tracing was that I hit:
>
> ip6gre_tunnel_xmit() -> ip6_tnl_xmit_ctl() -> ip6_tnl_get_cap()
>
> that returns IP6_TNL_F_CAP_PER_PACKET
>
> so back to ip6gre_tunnel_xmit() -> goto tx_err -> error, drop
>
> packet never leaves the interface.

I skipped this check so that the packets wouldn't drop.
I compared the implementations of ip_gre.c and ip6_gre.c and I
concluded that in ip6_tnl_xmit_ctl() instead of tunnel params
(&ip6_tnl->parms.laddr and &ip6_tnl->parms.raddr) it is better to use
skb network header (ipv6_hdr(skb)->saddr and ipv6_hdr(skb)->daddr).
It is illogical to use the tunnel parameters, because if we have an
NBMA connection, the addresses will not be set in the tunnel
parameters and packets will always drop on ip6_tnl_xmit_ctl().

> Hm, so you did get v6 to repro? Not sure what I'm doing wrong, I'm
> trying to repro with a net namespace over veth but that can't be it...

Yes, just skip ip6_tnl_xmit_ctl().

Re: [PATCH] net/ipv4/ip_gre.c net/ipv6/ip6_gre.c: ip and gre header are recorded twice

[Jakub Kicinski]

On Thu, 28 Jul 2022 16:54:01 +0300 Aleksey Shumnik wrote:
> On Fri, Jul 8, 2022 at 2:23 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > On Thu, 7 Jul 2022 19:41:23 +0300 Aleksey Shumnik wrote:
> >
> > Yeah, I've added the neigh entries (although the v6 addresses had to
> > be massaged a little for ip neigh to take them, the commands from the
> > email don't work cause iproute2 doesn't support :: in lladdr, AFAICT).
> >
> > What I've seen in tracing was that I hit:
> >
> > ip6gre_tunnel_xmit() -> ip6_tnl_xmit_ctl() -> ip6_tnl_get_cap()
> >
> > that returns IP6_TNL_F_CAP_PER_PACKET
> >
> > so back to ip6gre_tunnel_xmit() -> goto tx_err -> error, drop
> >
> > packet never leaves the interface.  
> 
> I skipped this check so that the packets wouldn't drop.
> I compared the implementations of ip_gre.c and ip6_gre.c and I
> concluded that in ip6_tnl_xmit_ctl() instead of tunnel params
> (&ip6_tnl->parms.laddr and &ip6_tnl->parms.raddr) it is better to use
> skb network header (ipv6_hdr(skb)->saddr and ipv6_hdr(skb)->daddr).
> It is illogical to use the tunnel parameters, because if we have an
> NBMA connection, the addresses will not be set in the tunnel
> parameters and packets will always drop on ip6_tnl_xmit_ctl().
> 
> > Hm, so you did get v6 to repro? Not sure what I'm doing wrong, I'm
> > trying to repro with a net namespace over veth but that can't be it...  
> 
> Yes, just skip ip6_tnl_xmit_ctl().

Mm. Having to remove checks for packets to pass thru makes it seem like 
a lot less of a bug.

Re: [PATCH] net/ipv4/ip_gre.c net/ipv6/ip6_gre.c: ip and gre header are recorded twice

[Aleksey Shumnik]

On Thu, Jul 28, 2022 at 6:17 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 28 Jul 2022 16:54:01 +0300 Aleksey Shumnik wrote:
> > On Fri, Jul 8, 2022 at 2:23 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > > On Thu, 7 Jul 2022 19:41:23 +0300 Aleksey Shumnik wrote:
> > >
> > > Yeah, I've added the neigh entries (although the v6 addresses had to
> > > be massaged a little for ip neigh to take them, the commands from the
> > > email don't work cause iproute2 doesn't support :: in lladdr, AFAICT).
> > >
> > > What I've seen in tracing was that I hit:
> > >
> > > ip6gre_tunnel_xmit() -> ip6_tnl_xmit_ctl() -> ip6_tnl_get_cap()
> > >
> > > that returns IP6_TNL_F_CAP_PER_PACKET
> > >
> > > so back to ip6gre_tunnel_xmit() -> goto tx_err -> error, drop
> > >
> > > packet never leaves the interface.
> >
> > I skipped this check so that the packets wouldn't drop.
> > I compared the implementations of ip_gre.c and ip6_gre.c and I
> > concluded that in ip6_tnl_xmit_ctl() instead of tunnel params
> > (&ip6_tnl->parms.laddr and &ip6_tnl->parms.raddr) it is better to use
> > skb network header (ipv6_hdr(skb)->saddr and ipv6_hdr(skb)->daddr).
> > It is illogical to use the tunnel parameters, because if we have an
> > NBMA connection, the addresses will not be set in the tunnel
> > parameters and packets will always drop on ip6_tnl_xmit_ctl().
> >
> > > Hm, so you did get v6 to repro? Not sure what I'm doing wrong, I'm
> > > trying to repro with a net namespace over veth but that can't be it...
> >
> > Yes, just skip ip6_tnl_xmit_ctl().
>
> Mm. Having to remove checks for packets to pass thru makes it seem like
> a lot less of a bug.

I don't agree. It is a bug.
When sending a packet over the NBMA network, the following sequence of
functions occurs:

ip6gre_tunnel_xmit() -> ip6_tnl_xmit_ctl() -> ip6_tnl_get_cap() ->
  ...
  if (ltype == IPV6_ADDR_ANY || rtype == IPV6_ADDR_ANY) {
      flags = IP6_TNL_F_CAP_PER_PACKET;
  ...

After that, the packages are dropped, but if you skip ip6_tnl_xmit_ctl()

ip6gre_tunnel_xmit() -> ip6gre_xmit_ipv4() / ip6gre_xmit_ipv6() /
ip6gre_xmit_other() -> __gre6_xmit() -> ip6_tnl_xmit() ->
  ...
  /* NBMA tunnel */
  if (ipv6_addr_any(&t->parms.raddr)) {
  ...

It is strange that at first when checking addr_type == IPV6_ADDR_ANY
packages are dropped,
but after that there is ipv6_addr_any(addr) which leads to
neigh_lookup() end etc.
It turns out that the same check leads to different actions. In
addition, due to the fact that the package is dropped, there is no
neighbor_lookup and the package will not be sent.
It looks like ip6_gre supports NBMA, but does not allow it to work,
because of this bug.