On Thu, 25 Apr 2019 11:49:18 -0700, John Fastabend wrote: > On 4/25/19 11:30 AM, Jakub Kicinski wrote: > > On Thu, 25 Apr 2019 09:02:50 -0700, John Fastabend wrote: > >> Series of fixes for sockmap and ktls, see patches for descriptions. > >> > >> v2: fix build issue for CONFIG_TLS_DEVICE and fixup couple comments from > >> Jakub. > > > > Ah, right my comment about the rx side sleeping was fairly nonsensical, > > the locking issues is that the work queue tries to lock the same socket. > > > > Right. > > > But I'm hitting some nasties, there is a UAF on a non-offload socket, > > and offload dies fairly hard. It _could_ be my offload patches on top, > > but "they worked yesterday". Digging deeper on the offload side, > > here's the UAF: > > hmm OK I see what is happening. I could also only enable the unhash for > SW/SW base proto. So only with, > > prot[TLS_SW][TLS_SW].unhash > > There is this on the offload side did I smash it somehow? > > prot[TLS_HW_RECORD][TLS_HW_RECORD].unhash = tls_hw_unhash; Um, I think you're good there, note that the TLS_HW_RECORD thing is not the nice packet-based offload, it's the TOE stuff from Chelsio. I'm using TLS_HW. > Also I have this in my stack, Thanks, I will toss this in. > commit 01628cbabdf2fbf0b710a399f54ae005d0963f3f (HEAD -> ktls-fixes, > refs/patches/ktls-fixes/bpf-sockmap-only-stop-strp-if) > Author: John Fastabend <john.fastabend@xxxxxxxxx> > Date: Wed Apr 24 15:55:55 2019 -0700 > > bpf: sockmap, only stop/flush strp if it was enabled at some point > > If we try to call strp_done on a parser that has never been > initialized, because the sockmap user is only using TX side for > example we get the following error. > > > [ 883.422081] WARNING: CPU: 1 PID: 208 at kernel/workqueue.c:3030 > __flush_work+0x1ca/0x1e0 > ... > [ 883.422095] Workqueue: events sk_psock_destroy_deferred > [ 883.422097] RIP: 0010:__flush_work+0x1ca/0x1e0 > > > This had been wrapped in a 'if (psock->parser.enabled)' logic which > was broken because the strp_done() was never actually being called > because we do a strp_stop() earlier in the tear down logic will > set parser.enabled to false. This could result in a use after free > if work was still in the queue and was resolved by the patch here, > 1d79895aef18f ("sk_msg: Always cancel strp work before freeing the > psock"). However, calling strp_stop(), done by the patch marked in > the fixes tag, only is useful if we never initialized a strp parser > program and never initialized the strp to start with. Because if > we had initialized a stream parser strp_stop() would have beencalled > by sk_psock_drop() earlier in the tear down process. By forcing the > strp to stop we get past the WARNING in strp_done that checks > the stopped flag but calling cancel_work_sync on work that has never > been initialized is also wrong and generates the warning above. > > To fix check if the parser program exists. If the program exists > then the strp work has been initialized and must be sync'd and > cancelled before free'ing any structures. If no program exists we > never initialized the stream parser in the first place so skip the > sync/cancel logic implemented by strp_done. > > Finally, remove the strp_done its not needed and in the case where > we are using the > stream parser has already been called. > > Fixes: e8e3437762ad9 ("bpf: Stop the psock parser before canceling > its work") > Signed-off-by: John Fastabend <john.fastabend@xxxxxxxxx> > > diff --git a/net/core/skmsg.c b/net/core/skmsg.c > index 782ae9eb4dce..4b4b9ad4bb86 100644 > --- a/net/core/skmsg.c > +++ b/net/core/skmsg.c > @@ -555,8 +555,12 @@ static void sk_psock_destroy_deferred(struct > work_struct *gc) > struct sk_psock *psock = container_of(gc, struct sk_psock, gc); > > /* No sk_callback_lock since already detached. */ > - strp_stop(&psock->parser.strp); > - strp_done(&psock->parser.strp); > + > + /* Parser has been stopped */ > + if (psock->progs.skb_parser) > + strp_stop(&psock->parser.strp); > + strp_done(&psock->parser.strp); > + } > > cancel_work_sync(&psock->work);