Re: [bpf PATCH] bpf, sockmap: RCU splat with TLS redirect and strparser

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

John Fastabend wrote:
> Martin KaFai Lau wrote:
> > On Wed, Jun 24, 2020 at 02:09:23PM -0700, John Fastabend wrote:
> > > Redirect on non-TLS sockmap side has RCU lock held from sockmap code
> > > path but when called from TLS this is no longer true. The RCU section
> > > is needed because we use rcu dereference to fetch the psock of the
> > > socket we are redirecting to.
> > sk_psock_verdict_apply() is also called by sk_psock_strp_read() after
> > rcu_read_unlock().  This issue should not be limited to tls?
> 
> The base case is covered because the non-TLS case is wrapped in
> rcu_read_lock/unlock here,
> 
>  static void sk_psock_strp_data_ready(struct sock *sk)
>  {
> 	struct sk_psock *psock;
> 
> 	rcu_read_lock();
> 	psock = sk_psock(sk);
> 	if (likely(psock)) {
> 		if (tls_sw_has_ctx_rx(sk)) {
> 			psock->parser.saved_data_ready(sk);
> 		} else {
> 			write_lock_bh(&sk->sk_callback_lock);
> 			strp_data_ready(&psock->parser.strp);
> 			write_unlock_bh(&sk->sk_callback_lock);
> 		}
> 	}
> 	rcu_read_unlock();
>  }
> 
> There is a case that has existed for awhile where if a skb_clone()
> fails or alloc_skb_for_msg() fails when building a merged skb. We
> could call back into sk_psock_strp_read() from a workqueue in
> strparser that would not be covered by above sk_psock_strp_data_ready().
> This would hit the sk_psock_verdict_apply() you caught above.
> 
> We don't actually see this from selftests because in selftests we
> always return skb->len indicating a msg is a single skb. In our
> use cases this is all we've ever used to date so we've not actually
> hit the case you call out. Another case that might hit this, based
> on code review, is some of the zero copy TX cases.
> 
> To fix the above case I think its best to submit another patch
> because the Fixes tag will be different. Sound OK? I could push
> them as a two patch series if that is easier to understand.

Sorry not enough coffee this morning the fix here is enough for
both cases I'll update the commit message.

> 
> Also I should have a patch shortly to allow users to skip setting
> up a parse_msg hook for the strparser. This helps because the
> vast majority of use cases I've seen just use skb->len as the
> message deliminator. It also bypasses above concern.
> 
> Thanks,
> John