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
