| Message ID | CAF=yD-J8JMyVq3Qt2n+L-EmpxacJ=mD7-KdsjgO3YB3LAzbKMQ@mail.gmail.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, Aug 8, 2017 at 9:48 PM, Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > Thanks for the report, David, and sorry for the breakage. > > I am not able to reproduce the issue with my qemu setup with vhost-net > with experimental_zcopytx so far. I did just reproduce the (well, a) panic in sock_zerocopy_put. It is indeed in the mm_unaccount_pinned_pages code that tests uarg->mmp: (gdb) list *(sock_zerocopy_put+0x26) 0xffffffff81620316 is in sock_zerocopy_put (net/core/skbuff.c:933). 928 } 929 930 static void mm_unaccount_pinned_pages(struct mmpin *mmp) 931 { 932 if (mmp->user) { 933 atomic_long_sub(mmp->num_pg, &mmp->user->locked_vm); 934 free_uid(mmp->user); This gives me more confidence that the previous fix is sufficient. I will have to revise it to avoid this path for all zerocopy paths besides msg_zerocopy.
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c index ba08b78ed630..e1e96d97de71 100644 --- a/drivers/vhost/net.c +++ b/drivers/vhost/net.c @@ -253,7 +253,7 @@ static int vhost_net_set_ubuf_info(struct vhost_net *n) zcopy = vhost_net_zcopy_mask & (0x1 << i); if (!zcopy) continue; - n->vqs[i].ubuf_info = kmalloc(sizeof(*n->vqs[i].ubuf_info) * + n->vqs[i].ubuf_info = kzalloc(sizeof(*n->vqs[i].ubuf_info) * UIO_MAXIOV, GFP_KERNEL); if (!n->vqs[i].ubuf_info) goto err; Less critical is correctly returning whether the operation completed without resorting to copying. Boolean uarg->zerocopy is undefined. This should not cause a kernel panic, as the vhost driver must handle both cases safely. Only msg_zerocopy sets bot SKBTX_ZEROCOPY_FRAG and SKBTX_DEV_ZEROCOPY, which is one way to identify this special case. diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 8c0708d2e5e6..7fb8b11ba8f6 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -1273,7 +1273,10 @@ static inline void skb_zcopy_clear(struct sk_buff *skb, bool zerocopy) struct ubuf_info *uarg = skb_zcopy(skb); if (uarg) { - uarg->zerocopy = uarg->zerocopy && zerocopy; + if (skb_shinfo(skb)->tx_flags & SKBTX_SHARED_FRAG) + uarg->zerocopy = uarg->zerocopy && zerocopy; + else + uarg->zerocopy = zerocopy; sock_zerocopy_put(uarg); skb_shinfo(skb)->tx_flags &= ~SKBTX_ZEROCOPY_FRAG;
Thanks for the report, David, and sorry for the breakage. I am not able to reproduce the issue with my qemu setup with vhost-net with experimental_zcopytx so far. But looking at the code from that codepath point of view, I do see that there are incorrect assumptions on ubuf_info fields being initialized anytime skb_zcopy(skb) is true, that are not true for the legacy zerocopy case. Specifically, uarg->mmp and uarg->zerocopy are only valid for msg_zerocopy. The first can conceivably result in dereferencing a garbage pointer if an ubuf_info from vhost is passed that does not have this field properly initialized. I will take a deeper look. As a first attempt, the following may fix the issue for this vhost case (only): } On Tue, Aug 8, 2017 at 6:14 PM, David Ahern <dsahern@gmail.com> wrote: > Willem: > > I updated my host server this morning to top of net-next -- commit > 53b948356554. I am not doing anything fancy or intentionally using the > zerocopy code. I launch a VM with vhost and attempt to login via ssh. > Doing that triggers a panic in the host at sock_zerocopy_put. The > attached is a snapshot of the console -- best I can get for the stack trace. > > David