Subject: Re: [PATCH v2 bpf-next 1/3] bpf: net: Add SO_DETACH_REUSEPORT_BPF

From: Stanislav Fomichev <[email-masked]>
Date: Wed, 12 Jun 2019 17:02:15 -0700

On 06/12, Martin KaFai Lau wrote:
> There is SO_ATTACH_REUSEPORT_[CE]BPF but there is no DETACH.
> This patch adds SO_DETACH_REUSEPORT_BPF sockopt.  The same
> sockopt can be used to undo both SO_ATTACH_REUSEPORT_[CE]BPF.
> 
> reseport_detach_prog() is added and it is mostly a mirror
> of the existing reuseport_attach_prog().  The differences are,
> it does not call reuseport_alloc() and returns -ENOENT when
> there is no old prog.
> 
> Cc: Craig Gallek <[email-masked]>
> Signed-off-by: Martin KaFai Lau <[email-masked]>
> ---
>  arch/alpha/include/uapi/asm/socket.h  |  2 ++
>  arch/mips/include/uapi/asm/socket.h   |  2 ++
>  arch/parisc/include/uapi/asm/socket.h |  2 ++
>  arch/sparc/include/uapi/asm/socket.h  |  2 ++
>  include/net/sock_reuseport.h          |  2 ++
>  include/uapi/asm-generic/socket.h     |  2 ++
>  net/core/sock.c                       |  4 ++++
>  net/core/sock_reuseport.c             | 24 ++++++++++++++++++++++++
>  8 files changed, 40 insertions(+)
> 
> diff --git a/arch/alpha/include/uapi/asm/socket.h b/arch/alpha/include/uapi/asm/socket.h
> index 976e89b116e5..de6c4df61082 100644
> --- a/arch/alpha/include/uapi/asm/socket.h
> +++ b/arch/alpha/include/uapi/asm/socket.h
> @@ -122,6 +122,8 @@
>  #define SO_RCVTIMEO_NEW         66
>  #define SO_SNDTIMEO_NEW         67
>  
> +#define SO_DETACH_REUSEPORT_BPF 68
> +
>  #if !defined(__KERNEL__)
>  
>  #if __BITS_PER_LONG == 64
> diff --git a/arch/mips/include/uapi/asm/socket.h b/arch/mips/include/uapi/asm/socket.h
> index d41765cfbc6e..d0a9ed2ca2d6 100644
> --- a/arch/mips/include/uapi/asm/socket.h
> +++ b/arch/mips/include/uapi/asm/socket.h
> @@ -133,6 +133,8 @@
>  #define SO_RCVTIMEO_NEW         66
>  #define SO_SNDTIMEO_NEW         67
>  
> +#define SO_DETACH_REUSEPORT_BPF 68
> +
>  #if !defined(__KERNEL__)
>  
>  #if __BITS_PER_LONG == 64
> diff --git a/arch/parisc/include/uapi/asm/socket.h b/arch/parisc/include/uapi/asm/socket.h
> index 66c5dd245ac7..10173c32195e 100644
> --- a/arch/parisc/include/uapi/asm/socket.h
> +++ b/arch/parisc/include/uapi/asm/socket.h
> @@ -114,6 +114,8 @@
>  #define SO_RCVTIMEO_NEW         0x4040
>  #define SO_SNDTIMEO_NEW         0x4041
>  
> +#define SO_DETACH_REUSEPORT_BPF 0x4042
> +
>  #if !defined(__KERNEL__)
>  
>  #if __BITS_PER_LONG == 64
> diff --git a/arch/sparc/include/uapi/asm/socket.h b/arch/sparc/include/uapi/asm/socket.h
> index 9265a9eece15..1895ac112a24 100644
> --- a/arch/sparc/include/uapi/asm/socket.h
> +++ b/arch/sparc/include/uapi/asm/socket.h
> @@ -115,6 +115,8 @@
>  #define SO_RCVTIMEO_NEW          0x0044
>  #define SO_SNDTIMEO_NEW          0x0045
>  
> +#define SO_DETACH_REUSEPORT_BPF  0x0046
> +
>  #if !defined(__KERNEL__)
>  
>  
> diff --git a/include/net/sock_reuseport.h b/include/net/sock_reuseport.h
> index 8a5f70c7cdf2..d9112de85261 100644
> --- a/include/net/sock_reuseport.h
> +++ b/include/net/sock_reuseport.h
> @@ -35,6 +35,8 @@ extern struct sock *reuseport_select_sock(struct sock *sk,
>  					  struct sk_buff *skb,
>  					  int hdr_len);
>  extern int reuseport_attach_prog(struct sock *sk, struct bpf_prog *prog);
> +extern int reuseport_detach_prog(struct sock *sk);
> +
>  int reuseport_get_id(struct sock_reuseport *reuse);
>  
>  #endif  /* _SOCK_REUSEPORT_H */
> diff --git a/include/uapi/asm-generic/socket.h b/include/uapi/asm-generic/socket.h
> index 8c1391c89171..77f7c1638eb1 100644
> --- a/include/uapi/asm-generic/socket.h
> +++ b/include/uapi/asm-generic/socket.h
> @@ -117,6 +117,8 @@
>  #define SO_RCVTIMEO_NEW         66
>  #define SO_SNDTIMEO_NEW         67
>  
> +#define SO_DETACH_REUSEPORT_BPF 68
> +
>  #if !defined(__KERNEL__)
>  
>  #if __BITS_PER_LONG == 64 || (defined(__x86_64__) && defined(__ILP32__))
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 75b1c950b49f..06be30737b69 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -1045,6 +1045,10 @@ int sock_setsockopt(struct socket *sock, int level, int optname,
>  		}
>  		break;
>  
> +	case SO_DETACH_REUSEPORT_BPF:
> +		ret = reuseport_detach_prog(sk);
> +		break;
> +
>  	case SO_DETACH_FILTER:
>  		ret = sk_detach_filter(sk);
>  		break;
> diff --git a/net/core/sock_reuseport.c b/net/core/sock_reuseport.c
> index dc4aefdf2a08..e0cb29469fa7 100644
> --- a/net/core/sock_reuseport.c
> +++ b/net/core/sock_reuseport.c
> @@ -332,3 +332,27 @@ int reuseport_attach_prog(struct sock *sk, struct bpf_prog *prog)
>  	return 0;
>  }
>  EXPORT_SYMBOL(reuseport_attach_prog);
> +
> +int reuseport_detach_prog(struct sock *sk)
> +{
> +	struct sock_reuseport *reuse;
> +	struct bpf_prog *old_prog;
> +
> +	if (!rcu_access_pointer(sk->sk_reuseport_cb))
> +		return sk->sk_reuseport ? -ENOENT : -EINVAL;
> +
> +	spin_lock_bh(&reuseport_lock);
> +	reuse = rcu_dereference_protected(sk->sk_reuseport_cb,
> +					  lockdep_is_held(&reuseport_lock));

[..]
> +	old_prog = rcu_dereference_protected(reuse->prog,
> +					     lockdep_is_held(&reuseport_lock));
> +	RCU_INIT_POINTER(reuse->prog, NULL);
Nit, optionally can do the following instead:

	struct bpf_prog *old_prog = NULL;

	...

	spin_lock_bh(&reuseport_lock);
	reuse = rcu_dereference_protected(...);
	rcu_swap_protected(reuse->prog, old_prog, lockdep_is_held(...));
	spin_unlock_bh(&reuseport_lock);

	if (!old_prog)
		....

	...

rcu_swap_protected does rcu_dereference_protected+rcu_assign_pointer,
can save you one line :-)

> +	spin_unlock_bh(&reuseport_lock);
> +
> +	if (!old_prog)
> +		return -ENOENT;
> +
> +	sk_reuseport_prog_free(old_prog);
> +	return 0;
> +}
> +EXPORT_SYMBOL(reuseport_detach_prog);
> -- 
> 2.17.1
>