RE: [PATCH bpf-next v2 03/11] net, sk_msg: Clear sk_user_data pointer on clone if tagged

[John Fastabend <john.fastabend@gmail.com>]

Jakub Sitnicki wrote:
> sk_user_data can hold a pointer to an object that is not intended to be
> shared between the parent socket and the child that gets a pointer copy on
> clone. This is the case when sk_user_data points at reference-counted
> object, like struct sk_psock.
> 
> One way to resolve it is to tag the pointer with a no-copy flag by
> repurposing its lowest bit. Based on the bit-flag value we clear the child
> sk_user_data pointer after cloning the parent socket.
> 
> The no-copy flag is stored in the pointer itself as opposed to externally,
> say in socket flags, to guarantee that the pointer and the flag are copied
> from parent to child socket in an atomic fashion. Parent socket state is
> subject to change while copying, we don't hold any locks at that time.
> 
> This approach relies on an assumption that sk_user_data holds a pointer to
> an object aligned to 2 or more bytes. A manual audit of existing users of
> rcu_dereference_sk_user_data helper confirms it. Also, an RCU-protected
> sk_user_data is not likely to hold a pointer to a char value or a
> pathological case of "struct { char c; }". To be safe, warn when the
> flag-bit is set when setting sk_user_data to catch any future misuses.
> 
> It is worth considering why clearing sk_user_data unconditionally is not an
> option. There exist users, DRBD, NVMe, and Xen drivers being among them,
> that rely on the pointer being copied when cloning the listening socket.
> 
> Potentially we could distinguish these users by checking if the listening
> socket has been created in kernel-space via sock_create_kern, and hence has
> sk_kern_sock flag set. However, this is not the case for NVMe and Xen
> drivers, which create sockets without marking them as belonging to the
> kernel.
> 
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> ---

LGTM.
Acked-by: John Fastabend <john.fastabend@gmail.com>

> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index e6ffdb47b619..f6c83747c71e 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -535,6 +535,10 @@ static void tcp_bpf_remove(struct sock *sk, struct sk_psock *psock)
>  {
>  	struct sk_psock_link *link;
>  
> +	/* Did a child socket inadvertently inherit parent's psock? */
> +	if (WARN_ON(sk != psock->sk))
> +		return;
> +

Not sure if this is needed. We would probably have hit problems before
we get here anyways for example if the parent sock was deleted while
the child is still around. I think I would just drop it.

>  	while ((link = sk_psock_link_pop(psock))) {
>  		sk_psock_unlink(sk, link);
>  		sk_psock_free_link(link);
> -- 
> 2.24.1
>