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

[Aleksey Shumnik <ashumnik9@gmail.com>]

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.