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

2019-06-21 Thread Xin Long 
On Fri, Jun 21, 2019 at 3:48 AM Erik Hugne  wrote:
>
> Commenting on Jon's response fist.
>
> Den tors 20 juni 2019 kl 13:26 skrev Xin Long :
> >
> > On Mon, Jun 17, 2019 at 10:28 PM Jon Maloy  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.
> The performance was largely dependant on TIPC message size, for large packets 
> there was no measurable difference, but the tunnel approach was considerably 
> faster for small packets than the kernel socket interface.
> I dont have the numbers, but i think i posted them on this list around 8 
> years ago.
>
> >[...]
> > 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()
> >
> > btw, do we have an official ipproto number for TIPC already?
>
> Not afak, but we have an IANA assigned UDP port for TIPC though, 6118.
> https://www.iana.org/assignments/service-names-port-numbers/service-names-port-numbers.xhtml?search=tipc
>
> TIPC does:
> skb_set_inner_protocol(skb, htons(ETH_P_TIPC));
> which will in turn set skb_inner_protocol_type to ENCAP_TYPE_ETHER.
> So how about implementing something similar to what's done for 
> ENCAP_TYPE_IPPROTO, but for ENCAP_TYPE_ETHER?
>
> In udp_offload.c, something in the line of:
>
> ...
> skb_udp_tunnel_segment()
> 
>
> switch (skb->inner_protocol_type) {
> case ENCAP_TYPE_ETHER:
> protocol = skb->inner_protocol;
> ops = rcu_dereference(ether_offloads[protocol]);
> if (!ops || !ops->callbacks->gso_segment)
> goto out_unlock;
> gso_inner_segment = ops->callbacks.gso_segment;
> break;
>
> 
> And obviously define ether_offloads, and corresponding ether_add_protocol and 
> ether_add_offload functions.
>
Maybe no need ether_offloads, dev_add_offload(_packet_offload) is
enough to make the callback be called by skb_mac_gso_segment() from
skb_udp_tunnel_segment(), I believe that's also what Jon does now.

It depends on which layer protocol we think TIPC. If we don't have a
plan for TIPC working over IP, a transport protocol in the future,
packet_offload is fine, otherwise, inet_offloads is also an option.


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

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

2019-06-20 Thread Erik Hugne 
Commenting on Jon's response fist.

Den tors 20 juni 2019 kl 13:26 skrev Xin Long :
>
> On Mon, Jun 17, 2019 at 10:28 PM Jon Maloy  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.
The performance was largely dependant on TIPC message size, for large
packets there was no measurable difference, but the tunnel approach was
considerably faster for small packets than the kernel socket interface.
I dont have the numbers, but i think i posted them on this list around 8
years ago.

>[...]
> 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()
>
> btw, do we have an official ipproto number for TIPC already?

Not afak, but we have an IANA assigned UDP port for TIPC though, 6118.
https://www.iana.org/assignments/service-names-port-numbers/service-names-port-numbers.xhtml?search=tipc

TIPC does:
skb_set_inner_protocol
(skb
, htons (ETH_P_TIPC
));
which will in turn set skb_inner_protocol_type to ENCAP_TYPE_ETHER.
So how about implementing something similar to what's done for
ENCAP_TYPE_IPPROTO, but for ENCAP_TYPE_ETHER?

In udp_offload.c, something in the line of:

...
skb_udp_tunnel_segment()


switch (skb->inner_protocol_type) {
case ENCAP_TYPE_ETHER:
protocol = skb->inner_protocol;
ops = rcu_dereference(ether_offloads[protocol]);
if (!ops || !ops->callbacks->gso_segment)
goto out_unlock;
gso_inner_segment = ops->callbacks.gso_segment;
break;


And obviously define ether_offloads, and corresponding ether_add_protocol
and ether_add_offload functions.

//E

___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

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

2019-06-20 Thread Jon Maloy 



> -Original Message-
> From: Xin Long 
> Sent: 20-Jun-19 07:26
> To: Jon Maloy 
> Cc: tipc-discussion@lists.sourceforge.net
> 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 
> 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: netdev-ow...@vger.kernel.org 
> On
> > > Behalf Of Xin Long
> > > Sent: 17-Jun-19 09:34
> > > To: network dev 
> > > Cc: da...@davemloft.net; Jon Maloy ; Ying
> > > Xue ; tipc-discussion@lists.sourceforge.net;
> > > Marcelo Ricardo Leitner ; Neil Horman
> > > ; Su Yanjun ; David
> > > Ahern ; syzkaller-b...@googlegroups.com; Dmitry
> > > Vyukov ; Pravin B Shelar 
> > > 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
> > >
> > >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
>

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

2019-06-20 Thread Xin Long 
On Mon, Jun 17, 2019 at 10:28 PM Jon Maloy  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()

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

> Do you have any comments on this? Could it possibly be done differently?
>
> ///jon
>
>
> > -Original Message-
> > From: netdev-ow...@vger.kernel.org  On
> > Behalf Of Xin Long
> > Sent: 17-Jun-19 09:34
> > To: network dev 
> > Cc: da...@davemloft.net; Jon Maloy ; Ying Xue
> > ; tipc-discussion@lists.sourceforge.net; Marcelo
> > Ricardo Leitner ; Neil Horman
> > ; Su Yanjun ; David
> > Ahern ; syzkaller-b...@googlegroups.com; Dmitry
> > Vyukov ; Pravin B Shelar 
> > 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
> >
> >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
>


___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

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

2019-06-17 Thread Jon Maloy 
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.
Do you have any comments on this? Could it possibly be done differently?

///jon


> -Original Message-
> From: netdev-ow...@vger.kernel.org  On
> Behalf Of Xin Long
> Sent: 17-Jun-19 09:34
> To: network dev 
> Cc: da...@davemloft.net; Jon Maloy ; Ying Xue
> ; tipc-discussion@lists.sourceforge.net; Marcelo
> Ricardo Leitner ; Neil Horman
> ; Su Yanjun ; David
> Ahern ; syzkaller-b...@googlegroups.com; Dmitry
> Vyukov ; Pravin B Shelar 
> 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
>
>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



___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion