Date:   Tue, 13 Nov 2018 23:08:50 -0800
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org
Cc:     "David S. Miller" <davem@...emloft.net>,
        Willem de Bruijn <willemb@...gle.com>,
        Steffen Klassert <steffen.klassert@...unet.com>,
        Subash Abhinov Kasiviswanathan <subashab@...eaurora.org>
Subject: Re: [PATCH net-next 01/10] udp: implement complete book-keeping for
 encap_needed



On 11/07/2018 03:38 AM, Paolo Abeni wrote:
> The *encap_needed static keys are enabled by UDP tunnels
> and several UDP encapsulations type, but they are never
> turned off. This can cause unneeded overall performance
> degradation for systems where such features are used
> transiently.
> 
> This patch introduces complete book-keeping for such keys,
> decreasing the usage at socket destruction time, if needed,
> and avoiding that the same socket could increase the key
> usage multiple times.
> 
> rfc v3 -> v1:
>  - add socket lock around udp_tunnel_encap_enable()
> 
> rfc v2 -> rfc v3:
>  - use udp_tunnel_encap_enable() in setsockopt()
> 
> Signed-off-by: Paolo Abeni <pabeni@...hat.com>
> ---
>  include/linux/udp.h      |  7 ++++++-
>  include/net/udp_tunnel.h |  6 ++++++
>  net/ipv4/udp.c           | 19 +++++++++++++------
>  net/ipv6/udp.c           | 14 +++++++++-----
>  4 files changed, 34 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/udp.h b/include/linux/udp.h
> index 320d49d85484..a4dafff407fb 100644
> --- a/include/linux/udp.h
> +++ b/include/linux/udp.h
> @@ -49,7 +49,12 @@ struct udp_sock {
>  	unsigned int	 corkflag;	/* Cork is required */
>  	__u8		 encap_type;	/* Is this an Encapsulation socket? */
>  	unsigned char	 no_check6_tx:1,/* Send zero UDP6 checksums on TX? */
> -			 no_check6_rx:1;/* Allow zero UDP6 checksums on RX? */
> +			 no_check6_rx:1,/* Allow zero UDP6 checksums on RX? */
> +			 encap_enabled:1; /* This socket enabled encap
> +					   * processing; UDP tunnels and
> +					   * different encapsulation layer set
> +					   * this
> +					   */
>  	/*
>  	 * Following member retains the information to create a UDP header
>  	 * when the socket is uncorked.
> diff --git a/include/net/udp_tunnel.h b/include/net/udp_tunnel.h
> index fe680ab6b15a..3fbe56430e3b 100644
> --- a/include/net/udp_tunnel.h
> +++ b/include/net/udp_tunnel.h
> @@ -165,6 +165,12 @@ static inline int udp_tunnel_handle_offloads(struct sk_buff *skb, bool udp_csum)
>  
>  static inline void udp_tunnel_encap_enable(struct socket *sock)
>  {
> +	struct udp_sock *up = udp_sk(sock->sk);
> +
> +	if (up->encap_enabled)
> +		return;
> +
> +	up->encap_enabled = 1;
>  #if IS_ENABLED(CONFIG_IPV6)
>  	if (sock->sk->sk_family == PF_INET6)
>  		ipv6_stub->udpv6_encap_enable();
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 1976fddb9e00..0ed715a72249 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -115,6 +115,7 @@
>  #include "udp_impl.h"
>  #include <net/sock_reuseport.h>
>  #include <net/addrconf.h>
> +#include <net/udp_tunnel.h>
>  
>  struct udp_table udp_table __read_mostly;
>  EXPORT_SYMBOL(udp_table);
> @@ -2398,11 +2399,15 @@ void udp_destroy_sock(struct sock *sk)
>  	bool slow = lock_sock_fast(sk);
>  	udp_flush_pending_frames(sk);
>  	unlock_sock_fast(sk, slow);
> -	if (static_branch_unlikely(&udp_encap_needed_key) && up->encap_type) {
> -		void (*encap_destroy)(struct sock *sk);
> -		encap_destroy = READ_ONCE(up->encap_destroy);
> -		if (encap_destroy)
> -			encap_destroy(sk);
> +	if (static_branch_unlikely(&udp_encap_needed_key)) {
> +		if (up->encap_type) {
> +			void (*encap_destroy)(struct sock *sk);
> +			encap_destroy = READ_ONCE(up->encap_destroy);
> +			if (encap_destroy)
> +				encap_destroy(sk);
> +		}
> +		if (up->encap_enabled)
> +			static_branch_disable(&udp_encap_needed_key);
>  	}
>  }
>  
> @@ -2447,7 +2452,9 @@ int udp_lib_setsockopt(struct sock *sk, int level, int optname,
>  			/* FALLTHROUGH */
>  		case UDP_ENCAP_L2TPINUDP:
>  			up->encap_type = val;
> -			udp_encap_enable();
> +			lock_sock(sk);
> +			udp_tunnel_encap_enable(sk->sk_socket);
> +			release_sock(sk);
>  			break;
>  		default:
>  			err = -ENOPROTOOPT;
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index d2d97d07ef27..fc0ce6c59ebb 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -1458,11 +1458,15 @@ void udpv6_destroy_sock(struct sock *sk)
>  	udp_v6_flush_pending_frames(sk);
>  	release_sock(sk);
>  
> -	if (static_branch_unlikely(&udpv6_encap_needed_key) && up->encap_type) {
> -		void (*encap_destroy)(struct sock *sk);
> -		encap_destroy = READ_ONCE(up->encap_destroy);
> -		if (encap_destroy)
> -			encap_destroy(sk);
> +	if (static_branch_unlikely(&udpv6_encap_needed_key)) {
> +		if (up->encap_type) {
> +			void (*encap_destroy)(struct sock *sk);
> +			encap_destroy = READ_ONCE(up->encap_destroy);
> +			if (encap_destroy)
> +				encap_destroy(sk);
> +		}
> +		if (up->encap_enabled)
> +			static_branch_disable(&udpv6_encap_needed_key);


Where is the opposite operation done ?

syzbot reported :

WARNING: CPU: 0 PID: 7564 at kernel/jump_label.c:188 static_key_disable_cpuslocked+0x2bb/0x310 kernel/jump_label.c:188
Kernel panic - not syncing: panic_on_warn set ...
CPU: 0 PID: 7564 Comm: syz-executor2 Not tainted 4.20.0-rc1-next-20181109+ #110
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x244/0x39d lib/dump_stack.c:113
 panic+0x2ad/0x55c kernel/panic.c:188
 __warn.cold.8+0x20/0x45 kernel/panic.c:540
 report_bug+0x254/0x2d0 lib/bug.c:186
 fixup_bug arch/x86/kernel/traps.c:178 [inline]
 do_error_trap+0x11b/0x200 arch/x86/kernel/traps.c:271
 do_invalid_op+0x36/0x40 arch/x86/kernel/traps.c:290
 invalid_op+0x14/0x20 arch/x86/entry/entry_64.S:969
RIP: 0010:static_key_disable_cpuslocked+0x2bb/0x310 kernel/jump_label.c:188
Code: ff eb db e8 07 da e3 ff 48 89 da 48 c7 c6 00 bf 31 88 48 c7 c7 e0 bb 31 88 e8 71 0d ad ff 0f 0b e9 0e fe ff ff e8 e5 d9 e3 ff <0f> 0b e9 2b ff ff ff 4c 89 f7 e8 56 32 27 00 e9 5e fe ff ff 48 89
RSP: 0018:ffff8801ba2b7928 EFLAGS: 00010293
RAX: ffff8801bb558340 RBX: 00000000ffffffff RCX: ffffffff819bcdf5
RDX: 0000000000000000 RSI: ffffffff819bcecb RDI: 0000000000000005
RBP: ffff8801ba2b79f0 R08: ffff8801bb558340 R09: fffffbfff17885c4
R10: fffffbfff17885c4 R11: ffffffff8bc42e23 R12: 1ffff10037456f25
R13: 1ffff10037456f31 R14: dffffc0000000000 R15: ffff8801ba2b79c8
 static_key_disable+0x1a/0x30 kernel/jump_label.c:202
 udpv6_destroy_sock+0x1e3/0x220 net/ipv6/udp.c:1492
 sk_common_release+0x6d/0x320 net/core/sock.c:3001
 udp_lib_close+0x15/0x20 include/net/udp.h:206
 inet_release+0x104/0x1f0 net/ipv4/af_inet.c:428
 inet6_release+0x50/0x70 net/ipv6/af_inet6.c:458
 __sock_release+0xd7/0x250 net/socket.c:580
 sock_close+0x19/0x20 net/socket.c:1142
 __fput+0x3bc/0xa70 fs/file_table.c:279
 ____fput+0x15/0x20 fs/file_table.c:312
 task_work_run+0x1e8/0x2a0 kernel/task_work.c:113
 tracehook_notify_resume include/linux/tracehook.h:188 [inline]
 exit_to_usermode_loop+0x318/0x380 arch/x86/entry/common.c:166
 prepare_exit_to_usermode arch/x86/entry/common.c:197 [inline]
 syscall_return_slowpath arch/x86/entry/common.c:268 [inline]
 do_syscall_64+0x6be/0x820 arch/x86/entry/common.c:293
 entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x411021
Code: 75 14 b8 03 00 00 00 0f 05 48 3d 01 f0 ff ff 0f 83 34 19 00 00 c3 48 83 ec 08 e8 0a fc ff ff 48 89 04 24 b8 03 00 00 00 0f 05 <48> 8b 3c 24 48 89 c2 e8 53 fc ff ff 48 89 d0 48 83 c4 08 48 3d 01
RSP: 002b:00007ffeb03ec0a0 EFLAGS: 00000293 ORIG_RAX: 0000000000000003
RAX: 0000000000000000 RBX: 0000000000000004 RCX: 0000000000411021
RDX: 0000000000000001 RSI: 0000000000731ce8 RDI: 0000000000000003
RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
R10: 00007ffeb03ebfd0 R11: 0000000000000293 R12: 0000000000000000
R13: 0000000000000001 R14: 0000000000000000 R15: 0000000000000002
Kernel Offset: disabled
Rebooting in 86400 seconds..

Powered by blists - more mailing lists