Re: [tipc-discussion] [PATCH net 0/3] net: fix quite a few dst_cache crashes reported by syzbot

[Jon M. <jon...@er...>]

> -----Original Message-----
> From: Xin Long <luc...@gm...>
> Sent: 20-Jun-19 07:26
> To: Jon Maloy <jon...@er...>
> Cc: tip...@li...
> Subject: Re: [PATCH net 0/3] net: fix quite a few dst_cache crashes reported
> by syzbot
> 
> On Mon, Jun 17, 2019 at 10:28 PM Jon Maloy <jon...@er...>
> wrote:
> >
> > Hi Xin,
> > As I remember the discussion around introduction of UDP media a few years
> ago, the developer, Erik Huge, only chose to register TIPC as a udp tunnel user
> instead of regular udp user because it provides a more efficient way to receive
> packet in kernel space.
> > With UDP tunnel, we could receive packet directly in a callback, while TIPC
> had to run in a work queue thread in order to read packets from the socket.
> So, in reality we don't need any tunnel at all. Another upside is that it is
> possible to hook in a GSO callback function from the tunnel user, something I
> am uncertain if we can do as a regular UDP user.
> 
> Right, udp tunnel was invented for this kind of encapsulation.
> 
> To implement this gso callback, we need to require an ipproto number for
> TIPC, and register the callback into inet_offloads by inet_add_offload().
> And on tx path set:
> skb->encapsulation = 1,
> skb_shinfo(skb)->gso_type|= SKB_GSO_UDP_TUNNEL,
> skb->inner_protocol_type = ENCAP_TYPE_IPPROTO.
> 
> Then it will be called by:
> dev_queue_xmit() .. -> skb_mac_gso_segment() ... ->
> udp4_ufo_fragment() -> skb_udp_tunnel_segment() ->
> skb_udp_tunnel_segment() -> tipc_gso_fragment()

This has already been done.

> 
> btw, do we have an official ipproto number for TIPC already?

No, I didn't push for this, since I was uncertain if it would be needed.

I did an experiment with this, as follows:
- I defined a new IPPROTO_TIPC and added it to the relevant locations in the network stack.
- I created a raw socket and sent packets from that via the ip_build_and_send_pkt() function.
- I registered a new tipc_ip_rcv() function via the inet_add_protocol() function.

This of course would require a new TIPC_GSO type, but we would avoid the horrendously deep call chain we currently have SKB_GSO_UDP_TUNNEL. 
I am pretty sure this was one of the reasons I was unable to improve performance this way.
I am also uncertain how a new IP protocol would be handled by existing routers and filters.

///jon


> 
> > Do you have any comments on this? Could it possibly be done differently?
> >
> > ///jon
> >
> >
> > > -----Original Message-----
> > > From: net...@vg... <net...@vg...>
> On
> > > Behalf Of Xin Long
> > > Sent: 17-Jun-19 09:34
> > > To: network dev <ne...@vg...>
> > > Cc: da...@da...; Jon Maloy <jon...@er...>; Ying
> > > Xue <yin...@wi...>; tip...@li...;
> > > Marcelo Ricardo Leitner <mar...@gm...>; Neil Horman
> > > <nh...@tu...>; Su Yanjun <suy...@cn...>; David
> > > Ahern <ds...@gm...>; syz...@go...; Dmitry
> > > Vyukov <dv...@go...>; Pravin B Shelar <ps...@ni...>
> > > Subject: [PATCH net 0/3] net: fix quite a few dst_cache crashes
> > > reported by syzbot
> > >
> > > There are two kinds of crashes reported many times by syzbot with no
> > > reproducer. Call Traces are like:
> > >
> > >      BUG: KASAN: slab-out-of-bounds in rt_cache_valid+0x158/0x190
> > >      net/ipv4/route.c:1556
> > >        rt_cache_valid+0x158/0x190 net/ipv4/route.c:1556
> > >        __mkroute_output net/ipv4/route.c:2332 [inline]
> > >        ip_route_output_key_hash_rcu+0x819/0x2d50
> net/ipv4/route.c:2564
> > >        ip_route_output_key_hash+0x1ef/0x360 net/ipv4/route.c:2393
> > >        __ip_route_output_key include/net/route.h:125 [inline]
> > >        ip_route_output_flow+0x28/0xc0 net/ipv4/route.c:2651
> > >        ip_route_output_key include/net/route.h:135 [inline]
> > >      ...
> > >
> > >    or:
> > >
> > >      kasan: GPF could be caused by NULL-ptr deref or user memory access
> > >      RIP: 0010:dst_dev_put+0x24/0x290 net/core/dst.c:168
> > >        <IRQ>
> > >        rt_fibinfo_free_cpus net/ipv4/fib_semantics.c:200 [inline]
> > >        free_fib_info_rcu+0x2e1/0x490 net/ipv4/fib_semantics.c:217
> > >        __rcu_reclaim kernel/rcu/rcu.h:240 [inline]
> > >        rcu_do_batch kernel/rcu/tree.c:2437 [inline]
> > >        invoke_rcu_callbacks kernel/rcu/tree.c:2716 [inline]
> > >        rcu_process_callbacks+0x100a/0x1ac0 kernel/rcu/tree.c:2697
> > >      ...
> > >
> > > They were caused by the fib_nh_common percpu member
> > > 'nhc_pcpu_rth_output'
> > > overwritten by another percpu variable 'dev->tstats' access overflow
> > > in tipc udp media xmit path when counting packets on a non tunnel device.
> > >
> > > The fix is to make udp tunnel work with no tunnel device by allowing
> > > not to count packets on the tstats when the tunnel dev is NULL in
> > > Patches 1/3 and 2/3, then pass a NULL tunnel dev in tipc_udp_tunnel() in
> Patch 3/3.
> > >
> > > Xin Long (3):
> > >   ip_tunnel: allow not to count pkts on tstats by setting skb's dev to
> > >     NULL
> > >   ip6_tunnel: allow not to count pkts on tstats by passing dev as NULL
> > >   tipc: pass tunnel dev as NULL to udp_tunnel(6)_xmit_skb
> > >
> > >  include/net/ip6_tunnel.h  | 9 ++++++---  net/ipv4/ip_tunnel_core.c
> > > | 9
> > > ++++++---
> > >  net/tipc/udp_media.c      | 8 +++-----
> > >  3 files changed, 15 insertions(+), 11 deletions(-)
> > >
> > > --
> > > 2.1.0
> >