search for: ubuf_info

vhost_net_clear_ubuf_info didn't clear ubuf_info after kfree, this could trigger double free. Fix this and simplify this code to make it more robust: make sure ubuf info is always freed through vhost_net_clear_ubuf_info. Reported-by: Tommi Rantala <tt.rantala at gmail.com> Signed-off-by: Michael S. Tsirkin <ms...

Two patches fixing the fallout from the vhost cleanup in 3.10. Thanks to Tommi Rantala who reported the issue. Tommi, could you please confirm this fixes the crashes for you? Michael S. Tsirkin (2): vhost: check owner before we overwrite ubuf_info vhost: fix ubuf_info cleanup drivers/vhost/net.c | 26 +++++++++++--------------- drivers/vhost/vhost.c | 8 +++++++- drivers/vhost/vhost.h | 1 + 3 files changed, 19 insertions(+), 16 deletions(-) -- MST

Two patches fixing the fallout from the vhost cleanup in 3.10. Thanks to Tommi Rantala who reported the issue. Tommi, could you please confirm this fixes the crashes for you? Michael S. Tsirkin (2): vhost: check owner before we overwrite ubuf_info vhost: fix ubuf_info cleanup drivers/vhost/net.c | 26 +++++++++++--------------- drivers/vhost/vhost.c | 8 +++++++- drivers/vhost/vhost.h | 1 + 3 files changed, 19 insertions(+), 16 deletions(-) -- MST

...struct vhost_net_virtqueue { struct vhost_virtqueue vq; + /* vhost zerocopy support fields below: */ + /* last used idx for outstanding DMA zerocopy buffers */ + int upend_idx; + /* first used idx for DMA done zerocopy buffers */ + int done_idx; + /* an array of userspace buffers info */ + struct ubuf_info *ubuf_info; + /* Reference counting for outstanding ubufs. + * Protected by vq mutex. Writers must also take device mutex. */ + struct vhost_ubuf_ref *ubufs; }; struct vhost_net { @@ -92,6 +108,88 @@ struct vhost_net { bool tx_flush; }; +static unsigned vhost_zcopy_mask __read_mostly; + +...

...struct vhost_net_virtqueue { struct vhost_virtqueue vq; + /* vhost zerocopy support fields below: */ + /* last used idx for outstanding DMA zerocopy buffers */ + int upend_idx; + /* first used idx for DMA done zerocopy buffers */ + int done_idx; + /* an array of userspace buffers info */ + struct ubuf_info *ubuf_info; + /* Reference counting for outstanding ubufs. + * Protected by vq mutex. Writers must also take device mutex. */ + struct vhost_ubuf_ref *ubufs; }; struct vhost_net { @@ -92,6 +108,88 @@ struct vhost_net { bool tx_flush; }; +static unsigned vhost_zcopy_mask __read_mostly; + +...

...tions(+), 1 deletion(-) diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c index 959b1cd..ec6fb3f 100644 --- a/drivers/vhost/net.c +++ b/drivers/vhost/net.c @@ -339,7 +339,8 @@ static void handle_tx(struct vhost_net *net) msg.msg_controllen = 0; ubufs = NULL; } else { - struct ubuf_info *ubuf = &vq->ubuf_info[head]; + struct ubuf_info *ubuf; + ubuf = vq->ubuf_info + vq->upend_idx; vq->heads[vq->upend_idx].len = VHOST_DMA_IN_PROGRESS; -- MST

...tions(+), 1 deletion(-) diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c index 959b1cd..ec6fb3f 100644 --- a/drivers/vhost/net.c +++ b/drivers/vhost/net.c @@ -339,7 +339,8 @@ static void handle_tx(struct vhost_net *net) msg.msg_controllen = 0; ubufs = NULL; } else { - struct ubuf_info *ubuf = &vq->ubuf_info[head]; + struct ubuf_info *ubuf; + ubuf = vq->ubuf_info + vq->upend_idx; vq->heads[vq->upend_idx].len = VHOST_DMA_IN_PROGRESS; -- MST

Hi Michael, I'm a bit confused about why ubuf_info and heads are UIO_MAXIOV length arrays, rather than being the size of the ring? In particular, this is suspicious: linux/drivers/vhost/net.c:342: struct ubuf_info *ubuf = &vq->ubuf_info[head]; And it seems to assume we trust head: a malicious guest could put the same head entry in the rin...

Hi Michael, I'm a bit confused about why ubuf_info and heads are UIO_MAXIOV length arrays, rather than being the size of the ring? In particular, this is suspicious: linux/drivers/vhost/net.c:342: struct ubuf_info *ubuf = &vq->ubuf_info[head]; And it seems to assume we trust head: a malicious guest could put the same head entry in the rin...

...ntil no zcopy is outstanding. What's the logic behind (nvq->upend_idx + 1) % UIO_MAXIOV != nvq->done_idx here? > + && vhost_net_tx_select_zcopy(net); > > /* use msg_control to pass vhost zerocopy ubuf info to skb */ > if (zcopy_used) { > + struct ubuf_info *ubuf; > + ubuf = nvq->ubuf_info + nvq->upend_idx; > + > vq->heads[nvq->upend_idx].id = head; > - if (!vhost_net_tx_select_zcopy(net) || > - len < VHOST_GOODCOPY_LEN) { > - /* copy don't need to wait for DMA done */ > - vq->heads[nvq-&g...

...ntil no zcopy is outstanding. What's the logic behind (nvq->upend_idx + 1) % UIO_MAXIOV != nvq->done_idx here? > + && vhost_net_tx_select_zcopy(net); > > /* use msg_control to pass vhost zerocopy ubuf info to skb */ > if (zcopy_used) { > + struct ubuf_info *ubuf; > + ubuf = nvq->ubuf_info + nvq->upend_idx; > + > vq->heads[nvq->upend_idx].id = head; > - if (!vhost_net_tx_select_zcopy(net) || > - len < VHOST_GOODCOPY_LEN) { > - /* copy don't need to wait for DMA done */ > - vq->heads[nvq-&g...

If device has an owner, we shouldn't touch ubuf_info since it might be in use. Signed-off-by: Michael S. Tsirkin <mst at redhat.com> --- drivers/vhost/net.c | 4 ++++ drivers/vhost/vhost.c | 8 +++++++- drivers/vhost/vhost.h | 1 + 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c index...

tun supports zero copy transmit since 0690899b4d4501b3505be069b9a687e68ccbe15b, however you can only enable this mode if you know your workload does not trigger heavy guest to host/host to guest traffic - otherwise you get a (minor) performance regression. This patchset addresses this problem by notifying the owner device when callback is invoked because of a data copy. This makes it possible to

tun supports zero copy transmit since 0690899b4d4501b3505be069b9a687e68ccbe15b, however you can only enable this mode if you know your workload does not trigger heavy guest to host/host to guest traffic - otherwise you get a (minor) performance regression. This patchset addresses this problem by notifying the owner device when callback is invoked because of a data copy. This makes it possible to

...t;heads[vq->upend_idx].id = head; + if (len < VHOST_GOODCOPY_LEN) { + /* copy don't need to wait for DMA done */ + vq->heads[vq->upend_idx].len = + VHOST_DMA_DONE_LEN; + msg.msg_control = NULL; + msg.msg_controllen = 0; + ubufs = NULL; + } else { + struct ubuf_info *ubuf = &vq->ubuf_info[head]; + + vq->heads[vq->upend_idx].len = len; + ubuf->callback = vhost_zerocopy_callback; + ubuf->arg = vq->ubufs; + ubuf->desc = vq->upend_idx; + msg.msg_control = ubuf; + msg.msg_controllen = sizeof(ubuf); + ubufs = vq->u...

...t;heads[vq->upend_idx].id = head; + if (len < VHOST_GOODCOPY_LEN) { + /* copy don't need to wait for DMA done */ + vq->heads[vq->upend_idx].len = + VHOST_DMA_DONE_LEN; + msg.msg_control = NULL; + msg.msg_controllen = 0; + ubufs = NULL; + } else { + struct ubuf_info *ubuf = &vq->ubuf_info[head]; + + vq->heads[vq->upend_idx].len = len; + ubuf->callback = vhost_zerocopy_callback; + ubuf->arg = vq->ubufs; + ubuf->desc = vq->upend_idx; + msg.msg_control = ubuf; + msg.msg_controllen = sizeof(ubuf); + ubufs = vq->u...

...: Shirley <xma at us.ibm.com> Signed-off-by: Michael S. Tsirkin <mst at redhat.com> --- I run some light tests and this seems to work fine for me. Given that the new functionality is disabled by default, it seems a pretty safe patch to merge for 3.1. Changes from v10: don't leak ubuf_info don't allocate ubuf_info for non zero copy vqs Changes from v9: coding style prettification suggested by Jesper Juhl drivers/vhost/net.c | 77 +++++++++++++++++++++++++++++- drivers/vhost/vhost.c | 128 +++++++++++++++++++++++++++++++++++++++++++------ drivers/vhost/vhost.h | 31 ++++...

...: Shirley <xma at us.ibm.com> Signed-off-by: Michael S. Tsirkin <mst at redhat.com> --- I run some light tests and this seems to work fine for me. Given that the new functionality is disabled by default, it seems a pretty safe patch to merge for 3.1. Changes from v10: don't leak ubuf_info don't allocate ubuf_info for non zero copy vqs Changes from v9: coding style prettification suggested by Jesper Juhl drivers/vhost/net.c | 77 +++++++++++++++++++++++++++++- drivers/vhost/vhost.c | 128 +++++++++++++++++++++++++++++++++++++++++++------ drivers/vhost/vhost.h | 31 ++++...

tun supports zero copy transmit since 0690899b4d4501b3505be069b9a687e68ccbe15b, however you can only enable this mode if you know your workload does not trigger heavy guest to host/host to guest traffic - otherwise you get a (minor) performance regression. This patchset addresses this problem by notifying the owner device when callback is invoked because of a data copy. This makes it possible to

tun supports zero copy transmit since 0690899b4d4501b3505be069b9a687e68ccbe15b, however you can only enable this mode if you know your workload does not trigger heavy guest to host/host to guest traffic - otherwise you get a (minor) performance regression. This patchset addresses this problem by notifying the owner device when callback is invoked because of a data copy. This makes it possible to