Halil Pasic
2017-Jan-27 12:24 UTC
[BUG/RFC] vhost: net: big endian viring access despite virtio 1
On 01/26/2017 08:20 PM, Michael S. Tsirkin wrote:> On Thu, Jan 26, 2017 at 06:39:14PM +0100, Halil Pasic wrote: >> >> Hi! >> >> Recently I have been investigating some strange migration problems on >> s390x. >> >> It turned out under certain circumstances vhost_net corrupts avail.idx by >> using wrong endianness.[..]>> -------------------------8<-------------- >> >From b26e2bbdc03832a0204ee2b42967a1b49a277dc8 Mon Sep 17 00:00:00 2001 >> From: Halil Pasic <pasic at linux.vnet.ibm.com> >> Date: Thu, 26 Jan 2017 00:06:15 +0100 >> Subject: [PATCH] vhost: remove useless/dangerous reset of is_le >> >> The reset of is_le does no good, but it contributes its fair share to a >> bug in vhost_net, which occurs if we have some oldubufs when stopping and >> setting a fd = -1 as a backend. Instead of doing something convoluted in >> vhost_net, let's just get rid of the reset. >> >> Signed-off-by: Halil Pasic <pasic at linux.vnet.ibm.com> >> Fixes: commit 2751c9882b94 >> --- >> drivers/vhost/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; >> - } >> >> vhost_init_is_le(vq); > > > I think you do need to reset it, just maybe within vhost_init_is_le. > > if (vhost_has_feature(vq, VIRTIO_F_VERSION_1)) > vq->is_le = true; > else > vhost_reset_is_le(vq); > >That is a very good point! I have overlooked that while the CONFIG_VHOST_CROSS_ENDIAN_LEGACY variant static void vhost_init_is_le(struct vhost_virtqueue *vq) { /* Note for legacy virtio: user_be is initialized at reset time * according to the host endianness. If userspace does not set an * explicit endianness, the default behavior is native endian, as * expected by legacy virtio. */ vq->is_le = vhost_has_feature(vq, VIRTIO_F_VERSION_1) || !vq->user_be; } is fine the other variant static void vhost_init_is_le(struct vhost_virtqueue *vq) { if (vhost_has_feature(vq, VIRTIO_F_VERSION_1)) vq->is_le = true; } is a very strange initializer (makes assumptions about the state to be initialized). I agree, setting native endianness there sounds very reasonable. I have a question regarding readability. IMHO the relationship of reset_is_le and int_is_le is a bit confusing, and I'm afraid it could become even more confusing with using reset in one of the init_is_le's. How about we do the following? static void vhost_init_is_le(struct vhost_virtqueue *vq) { if (vhost_has_feature(vq, VIRTIO_F_VERSION_1)) vq->is_le = true; + else + vq->is_le = virtio_legacy_is_little_endian(); } static void vhost_reset_is_le(struct vhost_virtqueue *vq) { - vq->is_le = virtio_legacy_is_little_endian(); + vhost_init_is_le(vq); } That way we would have correct endianness both after reset and after init, I think :). Thank you very much! Halil
Greg Kurz
2017-Jan-27 13:37 UTC
[BUG/RFC] vhost: net: big endian viring access despite virtio 1
On Fri, 27 Jan 2017 13:24:13 +0100 Halil Pasic <pasic at linux.vnet.ibm.com> wrote:> On 01/26/2017 08:20 PM, Michael S. Tsirkin wrote: > > On Thu, Jan 26, 2017 at 06:39:14PM +0100, Halil Pasic wrote: > >> > >> Hi! > >> > >> Recently I have been investigating some strange migration problems on > >> s390x. > >> > >> It turned out under certain circumstances vhost_net corrupts avail.idx by > >> using wrong endianness. > > [..] > > >> -------------------------8<-------------- > >> >From b26e2bbdc03832a0204ee2b42967a1b49a277dc8 Mon Sep 17 00:00:00 2001 > >> From: Halil Pasic <pasic at linux.vnet.ibm.com> > >> Date: Thu, 26 Jan 2017 00:06:15 +0100 > >> Subject: [PATCH] vhost: remove useless/dangerous reset of is_le > >> > >> The reset of is_le does no good, but it contributes its fair share to a > >> bug in vhost_net, which occurs if we have some oldubufs when stopping and > >> setting a fd = -1 as a backend. Instead of doing something convoluted in > >> vhost_net, let's just get rid of the reset. > >> > >> Signed-off-by: Halil Pasic <pasic at linux.vnet.ibm.com> > >> Fixes: commit 2751c9882b94 > >> --- > >> drivers/vhost/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; > >> - } > >> > >> vhost_init_is_le(vq); > > > > > > I think you do need to reset it, just maybe within vhost_init_is_le. > > > > if (vhost_has_feature(vq, VIRTIO_F_VERSION_1)) > > vq->is_le = true; > > else > > vhost_reset_is_le(vq); > > > > > > That is a very good point! I have overlooked that while the > CONFIG_VHOST_CROSS_ENDIAN_LEGACY variant > > static void vhost_init_is_le(struct vhost_virtqueue *vq) > { > /* Note for legacy virtio: user_be is initialized at reset time > * according to the host endianness. If userspace does not set an > * explicit endianness, the default behavior is native endian, as > * expected by legacy virtio. > */ > vq->is_le = vhost_has_feature(vq, VIRTIO_F_VERSION_1) || !vq->user_be; > } > > is fine the other variant > > static void vhost_init_is_le(struct vhost_virtqueue *vq) > { > if (vhost_has_feature(vq, VIRTIO_F_VERSION_1)) > vq->is_le = true; > } > is a very strange initializer (makes assumptions about the state > to be initialized). > > I agree, setting native endianness there sounds very reasonable. > > I have a question regarding readability. IMHO the relationship > of reset_is_le and int_is_le is a bit confusing, and I'm afraid > it could become even more confusing with using reset in one of > the init_is_le's. > > How about we do the following? > > static void vhost_init_is_le(struct vhost_virtqueue *vq) > { > if (vhost_has_feature(vq, VIRTIO_F_VERSION_1)) > vq->is_le = true; > + else > + vq->is_le = virtio_legacy_is_little_endian(); > > } > > static void vhost_reset_is_le(struct vhost_virtqueue *vq) > { > - vq->is_le = virtio_legacy_is_little_endian(); > + vhost_init_is_le(vq); > } > > That way we would have correct endianness both after reset > and after init, I think :). >Yes, I think this is what we need. Cheers. -- Greg> Thank you very much! > > Halil > >
Michael S. Tsirkin
2017-Jan-29 00:35 UTC
[BUG/RFC] vhost: net: big endian viring access despite virtio 1
On Fri, Jan 27, 2017 at 02:37:47PM +0100, Greg Kurz wrote:> On Fri, 27 Jan 2017 13:24:13 +0100 > Halil Pasic <pasic at linux.vnet.ibm.com> wrote: > > > On 01/26/2017 08:20 PM, Michael S. Tsirkin wrote: > > > On Thu, Jan 26, 2017 at 06:39:14PM +0100, Halil Pasic wrote: > > >> > > >> Hi! > > >> > > >> Recently I have been investigating some strange migration problems on > > >> s390x. > > >> > > >> It turned out under certain circumstances vhost_net corrupts avail.idx by > > >> using wrong endianness. > > > > [..] > > > > >> -------------------------8<-------------- > > >> >From b26e2bbdc03832a0204ee2b42967a1b49a277dc8 Mon Sep 17 00:00:00 2001 > > >> From: Halil Pasic <pasic at linux.vnet.ibm.com> > > >> Date: Thu, 26 Jan 2017 00:06:15 +0100 > > >> Subject: [PATCH] vhost: remove useless/dangerous reset of is_le > > >> > > >> The reset of is_le does no good, but it contributes its fair share to a > > >> bug in vhost_net, which occurs if we have some oldubufs when stopping and > > >> setting a fd = -1 as a backend. Instead of doing something convoluted in > > >> vhost_net, let's just get rid of the reset. > > >> > > >> Signed-off-by: Halil Pasic <pasic at linux.vnet.ibm.com> > > >> Fixes: commit 2751c9882b94 > > >> --- > > >> drivers/vhost/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; > > >> - } > > >> > > >> vhost_init_is_le(vq); > > > > > > > > > I think you do need to reset it, just maybe within vhost_init_is_le. > > > > > > if (vhost_has_feature(vq, VIRTIO_F_VERSION_1)) > > > vq->is_le = true; > > > else > > > vhost_reset_is_le(vq); > > > > > > > > > > That is a very good point! I have overlooked that while the > > CONFIG_VHOST_CROSS_ENDIAN_LEGACY variant > > > > static void vhost_init_is_le(struct vhost_virtqueue *vq) > > { > > /* Note for legacy virtio: user_be is initialized at reset time > > * according to the host endianness. If userspace does not set an > > * explicit endianness, the default behavior is native endian, as > > * expected by legacy virtio. > > */ > > vq->is_le = vhost_has_feature(vq, VIRTIO_F_VERSION_1) || !vq->user_be; > > } > > > > is fine the other variant > > > > static void vhost_init_is_le(struct vhost_virtqueue *vq) > > { > > if (vhost_has_feature(vq, VIRTIO_F_VERSION_1)) > > vq->is_le = true; > > } > > is a very strange initializer (makes assumptions about the state > > to be initialized). > > > > I agree, setting native endianness there sounds very reasonable. > > > > I have a question regarding readability. IMHO the relationship > > of reset_is_le and int_is_le is a bit confusing, and I'm afraid > > it could become even more confusing with using reset in one of > > the init_is_le's. > > > > How about we do the following? > > > > static void vhost_init_is_le(struct vhost_virtqueue *vq) > > { > > if (vhost_has_feature(vq, VIRTIO_F_VERSION_1)) > > vq->is_le = true; > > + else > > + vq->is_le = virtio_legacy_is_little_endian(); > > > > } > > > > static void vhost_reset_is_le(struct vhost_virtqueue *vq) > > { > > - vq->is_le = virtio_legacy_is_little_endian(); > > + vhost_init_is_le(vq); > > } > > > > That way we would have correct endianness both after reset > > and after init, I think :). > > > > Yes, I think this is what we need. > > Cheers.OK, pls test this patch.> -- > Greg > > > Thank you very much! > > > > Halil > > > >
Reasonably Related Threads
- [BUG/RFC] vhost: net: big endian viring access despite virtio 1
- [BUG/RFC] vhost: net: big endian viring access despite virtio 1
- [BUG/RFC] vhost: net: big endian viring access despite virtio 1
- [BUG/RFC] vhost: net: big endian viring access despite virtio 1
- [BUG/RFC] vhost: net: big endian viring access despite virtio 1