Re: [PATCH bpf-next v2 04/11] tcp_bpf: Don't let child socket inherit parent protocol ops on copy

From: Martin Lau <kafai@fb.com>
To: Jakub Sitnicki <jakub@cloudflare.com>
Cc: "bpf@vger.kernel.org" <bpf@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"kernel-team@cloudflare.com" <kernel-team@cloudflare.com>,
	Eric Dumazet <edumazet@google.com>,
	"John Fastabend" <john.fastabend@gmail.com>,
	Lorenz Bauer <lmb@cloudflare.com>
Subject: Re: [PATCH bpf-next v2 04/11] tcp_bpf: Don't let child socket inherit parent protocol ops on copy
Date: Mon, 13 Jan 2020 23:23:01 +0000	[thread overview]
Message-ID: <20200113232257.y5cvyvs2lpzpgidr@kafai-mbp.dhcp.thefacebook.com> (raw)
In-Reply-To: <87ftgjrna5.fsf@cloudflare.com>

On Mon, Jan 13, 2020 at 11:42:42PM +0100, Jakub Sitnicki wrote:
> On Mon, Jan 13, 2020 at 11:23 PM CET, Martin Lau wrote:
> > On Fri, Jan 10, 2020 at 11:50:20AM +0100, Jakub Sitnicki wrote:
> >> Prepare for cloning listening sockets that have their protocol callbacks
> >> overridden by sk_msg. Child sockets must not inherit parent callbacks that
> >> access state stored in sk_user_data owned by the parent.
> >>
> >> Restore the child socket protocol callbacks before the it gets hashed and
> >> any of the callbacks can get invoked.
> >>
> >> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> >> ---
> >>  include/net/tcp.h        |  1 +
> >>  net/ipv4/tcp_bpf.c       | 13 +++++++++++++
> >>  net/ipv4/tcp_minisocks.c |  2 ++
> >>  3 files changed, 16 insertions(+)
> >>
> >> diff --git a/include/net/tcp.h b/include/net/tcp.h
> >> index 9dd975be7fdf..7cbf9465bb10 100644
> >> --- a/include/net/tcp.h
> >> +++ b/include/net/tcp.h
> >> @@ -2181,6 +2181,7 @@ int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
> >>  		    int nonblock, int flags, int *addr_len);
> >>  int __tcp_bpf_recvmsg(struct sock *sk, struct sk_psock *psock,
> >>  		      struct msghdr *msg, int len, int flags);
> >> +void tcp_bpf_clone(const struct sock *sk, struct sock *child);
> >>
> >>  /* Call BPF_SOCK_OPS program that returns an int. If the return value
> >>   * is < 0, then the BPF op failed (for example if the loaded BPF
> >> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> >> index f6c83747c71e..6f96320fb7cf 100644
> >> --- a/net/ipv4/tcp_bpf.c
> >> +++ b/net/ipv4/tcp_bpf.c
> >> @@ -586,6 +586,19 @@ static void tcp_bpf_close(struct sock *sk, long timeout)
> >>  	saved_close(sk, timeout);
> >>  }
> >>
> >> +/* If a child got cloned from a listening socket that had tcp_bpf
> >> + * protocol callbacks installed, we need to restore the callbacks to
> >> + * the default ones because the child does not inherit the psock state
> >> + * that tcp_bpf callbacks expect.
> >> + */
> >> +void tcp_bpf_clone(const struct sock *sk, struct sock *newsk)
> >> +{
> >> +	struct proto *prot = newsk->sk_prot;
> >> +
> >> +	if (prot->recvmsg == tcp_bpf_recvmsg)
> > A question not related to this patch (may be it is more for patch 6).
> >
> > How tcp_bpf_recvmsg may be used for a listening sock (sk here)?
> 
> It can't be used. It's a way of checking if sock has tcp_bpf callbacks
> that I copied from sk_psock_get_checked:
ic.  It seems only tcp_bpf_close and tcp_bpf_unhash may be useful.
Asking because it intuitively made me think how tcp_bpf_recvmsg/sendmsg/...etc
may be used since they are also set to listening sk.

> 
> static inline struct sk_psock *sk_psock_get_checked(struct sock *sk)
> {
> 	struct sk_psock *psock;
> 
> 	rcu_read_lock();
> 	psock = sk_psock(sk);
> 	if (psock) {
> 		if (sk->sk_prot->recvmsg != tcp_bpf_recvmsg) {
> 			psock = ERR_PTR(-EBUSY);
> 			goto out;
> 		}
>         ...
> 
> This makes me think that perhaps it deserves a well-named helper.
> 
> >
> >> +		newsk->sk_prot = sk->sk_prot_creator;
> >> +}
> >> +