search for: vhost_vq_init_access

Currently, under certain circumstances vhost_init_is_le does just a part of the initialization job, and depends on vhost_reset_is_le being called too. For this reason vhost_vq_init_access used to call vhost_reset_is_le when vq->private_data is NULL. This is not only counter intuitive, but also real a problem because it breaks vhost_net. The bug was introduced to vhost_net with commit 2751c9882b94 ("vhost: cross-endian support for legacy devices"). The symptom is corrupt...

Currently, under certain circumstances vhost_init_is_le does just a part of the initialization job, and depends on vhost_reset_is_le being called too. For this reason vhost_vq_init_access used to call vhost_reset_is_le when vq->private_data is NULL. This is not only counter intuitive, but also real a problem because it breaks vhost_net. The bug was introduced to vhost_net with commit 2751c9882b94 ("vhost: cross-endian support for legacy devices"). The symptom is corrupt...

On 01/30/2017 08:06 PM, Greg Kurz wrote: >> Currently, under certain circumstances vhost_init_is_le does just a part >> of the initialization job, and depends on vhost_reset_is_le being called >> too. For this reason vhost_vq_init_access used to call vhost_reset_is_le >> when vq->private_data is NULL. This is not only counter intuitive, but >> also real a problem because it breaks vhost_net. The bug was introduced to >> vhost_net with commit 2751c9882b94 ("vhost: cross-endian support for >> legacy de...

On 01/30/2017 08:06 PM, Greg Kurz wrote: >> Currently, under certain circumstances vhost_init_is_le does just a part >> of the initialization job, and depends on vhost_reset_is_le being called >> too. For this reason vhost_vq_init_access used to call vhost_reset_is_le >> when vq->private_data is NULL. This is not only counter intuitive, but >> also real a problem because it breaks vhost_net. The bug was introduced to >> vhost_net with commit 2751c9882b94 ("vhost: cross-endian support for >> legacy de...

This series is a new tentative to have cleaner cross-endian code. Patches 1/3 is new: it fixes a side-effect in case vhost_init_used() fails. Patch 2/3 comes from v1: it renames cross-endian helpers Patch 3/3 is new: it simply renames vhost_init_used() as suggested by Michael. --- Greg Kurz (3): vhost: fix error path in vhost_init_used() vhost: rename cross-endian helpers

This series is a new tentative to have cleaner cross-endian code. Patches 1/3 is new: it fixes a side-effect in case vhost_init_used() fails. Patch 2/3 comes from v1: it renames cross-endian helpers Patch 3/3 is new: it simply renames vhost_init_used() as suggested by Michael. --- Greg Kurz (3): vhost: fix error path in vhost_init_used() vhost: rename cross-endian helpers

...tain circumstances vhost_net corrupts avail.idx by using wrong endianness. I managed to track the problem down (I'm pretty sure). It boils down to the following. When stopping vhost userspace (QEMU) calls vhost_net_set_backend with the fd argument set to -1, this leads to is_le being reset in vhost_vq_init_access. On a BE system resetting to legacy means resetting to BE. Usually this is not a problem, but in the case when oldubufs is not zero (which is not likely if no network stress applied) it is a problem. That code path needs to write avail.idx, and ends up using wrong endianness when doing that (but o...

...tain circumstances vhost_net corrupts avail.idx by using wrong endianness. I managed to track the problem down (I'm pretty sure). It boils down to the following. When stopping vhost userspace (QEMU) calls vhost_net_set_backend with the fd argument set to -1, this leads to is_le being reset in vhost_vq_init_access. On a BE system resetting to legacy means resetting to BE. Usually this is not a problem, but in the case when oldubufs is not zero (which is not likely if no network stress applied) it is a problem. That code path needs to write avail.idx, and ends up using wrong endianness when doing that (but o...

...ail.idx by > using wrong endianness. > > I managed to track the problem down (I'm pretty sure). It boils down to > the following. > > When stopping vhost userspace (QEMU) calls vhost_net_set_backend with > the fd argument set to -1, this leads to is_le being reset in > vhost_vq_init_access. On a BE system resetting to legacy means resetting > to BE. Usually this is not a problem, but in the case when oldubufs is not > zero (which is not likely if no network stress applied) it is a problem. > That code path needs to write avail.idx, and ends up using wrong > endianness wh...

On Mon, 30 Jan 2017 11:09:36 +0100 Halil Pasic <pasic at linux.vnet.ibm.com> wrote: > Currently, under certain circumstances vhost_init_is_le does just a part > of the initialization job, and depends on vhost_reset_is_le being called > too. For this reason vhost_vq_init_access used to call vhost_reset_is_le > when vq->private_data is NULL. This is not only counter intuitive, but > also real a problem because it breaks vhost_net. The bug was introduced to > vhost_net with commit 2751c9882b94 ("vhost: cross-endian support for > legacy devices"). Th...

...oid handle_tx(struct vhost_net *net) % UIO_MAXIOV; } vhost_discard_vq_desc(vq, 1); + if (err == -EAGAIN) + vhost_net_enable_vq(net, vq); break; } if (err != len) @@ -1145,9 +1148,11 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd) r = vhost_vq_init_access(vq); if (r) goto err_used; - r = vhost_net_enable_vq(n, vq); - if (r) - goto err_used; + if (index == VHOST_NET_VQ_RX) { + r = vhost_net_enable_vq(n, vq); + if (r) + goto err_used; + } oldubufs = nvq->ubufs; nvq->ubufs = ubufs; -- 2.7.4

...oid handle_tx(struct vhost_net *net) % UIO_MAXIOV; } vhost_discard_vq_desc(vq, 1); + if (err == -EAGAIN) + vhost_net_enable_vq(net, vq); break; } if (err != len) @@ -1145,9 +1148,11 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd) r = vhost_vq_init_access(vq); if (r) goto err_used; - r = vhost_net_enable_vq(n, vq); - if (r) - goto err_used; + if (index == VHOST_NET_VQ_RX) { + r = vhost_net_enable_vq(n, vq); + if (r) + goto err_used; + } oldubufs = nvq->ubufs; nvq->ubufs = ubufs; -- 2.7.4

...dex 16c2cb6..5dd6c05 100644 > > > --- a/drivers/vhost/vhost.h > > > +++ b/drivers/vhost/vhost.h > > > @@ -199,6 +199,7 @@ int __vhost_get_vq_desc(struct vhost_virtqueue *vq, > > > void vhost_discard_vq_desc(struct vhost_virtqueue *, int n); > > > int vhost_vq_init_access(struct vhost_virtqueue *); > > > +int vhost_add_used_idx(struct vhost_virtqueue *vq, int n); > > > int vhost_add_used(struct vhost_virtqueue *, unsigned int head, int len); > > > int vhost_add_used_n(struct vhost_virtqueue *, struct vring_used_elem *heads, > > &...

...dex 16c2cb6..5dd6c05 100644 > > > --- a/drivers/vhost/vhost.h > > > +++ b/drivers/vhost/vhost.h > > > @@ -199,6 +199,7 @@ int __vhost_get_vq_desc(struct vhost_virtqueue *vq, > > > void vhost_discard_vq_desc(struct vhost_virtqueue *, int n); > > > int vhost_vq_init_access(struct vhost_virtqueue *); > > > +int vhost_add_used_idx(struct vhost_virtqueue *vq, int n); > > > int vhost_add_used(struct vhost_virtqueue *, unsigned int head, int len); > > > int vhost_add_used_n(struct vhost_virtqueue *, struct vring_used_elem *heads, > > &...

...6:13PM +0100, Halil Pasic wrote: > > > On 01/30/2017 08:06 PM, Greg Kurz wrote: > >> Currently, under certain circumstances vhost_init_is_le does just a part > >> of the initialization job, and depends on vhost_reset_is_le being called > >> too. For this reason vhost_vq_init_access used to call vhost_reset_is_le > >> when vq->private_data is NULL. This is not only counter intuitive, but > >> also real a problem because it breaks vhost_net. The bug was introduced to > >> vhost_net with commit 2751c9882b94 ("vhost: cross-endian support for &gt...

...vhost/vhost.h b/drivers/vhost/vhost.h > index 16c2cb6..5dd6c05 100644 > --- a/drivers/vhost/vhost.h > +++ b/drivers/vhost/vhost.h > @@ -199,6 +199,7 @@ int __vhost_get_vq_desc(struct vhost_virtqueue *vq, > void vhost_discard_vq_desc(struct vhost_virtqueue *, int n); > > int vhost_vq_init_access(struct vhost_virtqueue *); > +int vhost_add_used_idx(struct vhost_virtqueue *vq, int n); > int vhost_add_used(struct vhost_virtqueue *, unsigned int head, int len); > int vhost_add_used_n(struct vhost_virtqueue *, struct vring_used_elem *heads, > unsigned count); Please chang...

...vhost/vhost.h b/drivers/vhost/vhost.h > index 16c2cb6..5dd6c05 100644 > --- a/drivers/vhost/vhost.h > +++ b/drivers/vhost/vhost.h > @@ -199,6 +199,7 @@ int __vhost_get_vq_desc(struct vhost_virtqueue *vq, > void vhost_discard_vq_desc(struct vhost_virtqueue *, int n); > > int vhost_vq_init_access(struct vhost_virtqueue *); > +int vhost_add_used_idx(struct vhost_virtqueue *vq, int n); > int vhost_add_used(struct vhost_virtqueue *, unsigned int head, int len); > int vhost_add_used_n(struct vhost_virtqueue *, struct vring_used_elem *heads, > unsigned count); Please chang...

...rivate_data) { > - vhost_disable_is_le(vq); > - return 0; > - } > > - vhost_enable_is_le(vq); > + if (!vq->private_data) > + return 0; > > r = vhost_update_used_flags(vq); > if (r) Looking at how callers use this, maybe we should just rename init_used to vhost_vq_init_access. The _used suffix was a hint that we access the vq used ring. But maybe what callers care about is that it must be called after access_ok. > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h > index d3f767448a72..88d86f45f756 100644 > --- a/drivers/vhost/vhost.h > +++ b/driver...

...rivate_data) { > - vhost_disable_is_le(vq); > - return 0; > - } > > - vhost_enable_is_le(vq); > + if (!vq->private_data) > + return 0; > > r = vhost_update_used_flags(vq); > if (r) Looking at how callers use this, maybe we should just rename init_used to vhost_vq_init_access. The _used suffix was a hint that we access the vq used ring. But maybe what callers care about is that it must be called after access_ok. > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h > index d3f767448a72..88d86f45f756 100644 > --- a/drivers/vhost/vhost.h > +++ b/driver...

...vhost.c | 4 +--- >> 1 file changed, 1 insertion(+), 3 deletions(-) >> >> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c >> index d643260..08072a2 100644 >> --- a/drivers/vhost/vhost.c >> +++ b/drivers/vhost/vhost.c >> @@ -1714,10 +1714,8 @@ int vhost_vq_init_access(struct vhost_virtqueue *vq) >> int r; >> bool is_le = vq->is_le; >> >> - if (!vq->private_data) { >> - vhost_reset_is_le(vq); >> + if (!vq->private_data) >> return 0; >> - } >...