* [dpdk-dev] [PATCH] vhost: fix crash if set vring num handling failed [not found] <CGME20180817113255eucas1p2042d22241265dafa241c2a6a2c949244@eucas1p2.samsung.com> @ 2018-08-17 11:33 ` Ilya Maximets 2018-08-29 14:00 ` Ilya Maximets [not found] ` <CGME20180903101059eucas1p2030d1e9077a03bc5d0613ba0311e1eab@eucas1p2.samsung.com> 0 siblings, 2 replies; 9+ messages in thread From: Ilya Maximets @ 2018-08-17 11:33 UTC (permalink / raw) To: dev; +Cc: Maxime Coquelin, Tiwei Bie, Zhihong Wang, Ilya Maximets, stable Allocation failures of shadow used ring and batched copy array are not recoverable and leads to the segmentation faults like this on the receiving/transmission path: Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 0x7f913fecf0 (LWP 43625)] in copy_desc_to_mbuf () at /lib/librte_vhost/virtio_net.c:760 760 batch_copy[vq->batch_copy_nb_elems].dst = This could be easily reproduced in case of low memory or big number of vhost-user ports. Fix that by propagating error to the upper layer which will end up with disconnection. Fixes: f689586bc060 ("vhost: shadow used ring update") Cc: stable@dpdk.org Signed-off-by: Ilya Maximets <i.maximets@samsung.com> --- lib/librte_vhost/vhost_user.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c index 9aa1ce118..4c7fd57fb 100644 --- a/lib/librte_vhost/vhost_user.c +++ b/lib/librte_vhost/vhost_user.c @@ -1693,7 +1693,9 @@ vhost_user_msg_handler(int vid, int fd) break; case VHOST_USER_SET_VRING_NUM: - vhost_user_set_vring_num(dev, &msg); + ret = vhost_user_set_vring_num(dev, &msg); + if (ret) + return -1; break; case VHOST_USER_SET_VRING_ADDR: vhost_user_set_vring_addr(&dev, &msg); -- 2.17.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] vhost: fix crash if set vring num handling failed 2018-08-17 11:33 ` [dpdk-dev] [PATCH] vhost: fix crash if set vring num handling failed Ilya Maximets @ 2018-08-29 14:00 ` Ilya Maximets 2018-08-30 6:06 ` Tiwei Bie [not found] ` <CGME20180903101059eucas1p2030d1e9077a03bc5d0613ba0311e1eab@eucas1p2.samsung.com> 1 sibling, 1 reply; 9+ messages in thread From: Ilya Maximets @ 2018-08-29 14:00 UTC (permalink / raw) To: dev; +Cc: Maxime Coquelin, Tiwei Bie, Zhihong Wang, stable Any thoughts on this? Best regards, Ilya Maximets. On 17.08.2018 14:33, Ilya Maximets wrote: > Allocation failures of shadow used ring and batched copy array > are not recoverable and leads to the segmentation faults like > this on the receiving/transmission path: > > Program received signal SIGSEGV, Segmentation fault. > [Switching to Thread 0x7f913fecf0 (LWP 43625)] > in copy_desc_to_mbuf () at /lib/librte_vhost/virtio_net.c:760 > 760 batch_copy[vq->batch_copy_nb_elems].dst = > > This could be easily reproduced in case of low memory or big > number of vhost-user ports. Fix that by propagating error to > the upper layer which will end up with disconnection. > > Fixes: f689586bc060 ("vhost: shadow used ring update") > Cc: stable@dpdk.org > > Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > --- > lib/librte_vhost/vhost_user.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c > index 9aa1ce118..4c7fd57fb 100644 > --- a/lib/librte_vhost/vhost_user.c > +++ b/lib/librte_vhost/vhost_user.c > @@ -1693,7 +1693,9 @@ vhost_user_msg_handler(int vid, int fd) > break; > > case VHOST_USER_SET_VRING_NUM: > - vhost_user_set_vring_num(dev, &msg); > + ret = vhost_user_set_vring_num(dev, &msg); > + if (ret) > + return -1; > break; > case VHOST_USER_SET_VRING_ADDR: > vhost_user_set_vring_addr(&dev, &msg); > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] vhost: fix crash if set vring num handling failed 2018-08-29 14:00 ` Ilya Maximets @ 2018-08-30 6:06 ` Tiwei Bie 2018-08-30 6:54 ` Ilya Maximets 0 siblings, 1 reply; 9+ messages in thread From: Tiwei Bie @ 2018-08-30 6:06 UTC (permalink / raw) To: Ilya Maximets; +Cc: dev, Maxime Coquelin, Zhihong Wang, stable On Wed, Aug 29, 2018 at 05:00:47PM +0300, Ilya Maximets wrote: > Any thoughts on this? Hi Ilya, Currently, the errors happened during messages handling aren't handled properly. E.g. in vhost_user_set_mem_table(), similar issues [1] may happen too. We might want to find a way to fix this once and for all. [1] https://github.com/DPDK/dpdk/blob/3605968c2fa7/lib/librte_vhost/vhost_user.c#L826 https://github.com/DPDK/dpdk/blob/3605968c2fa7/lib/librte_vhost/vhost_user.c#L837 https://github.com/DPDK/dpdk/blob/3605968c2fa7/lib/librte_vhost/vhost_user.c#L887 Thanks > > Best regards, Ilya Maximets. > > On 17.08.2018 14:33, Ilya Maximets wrote: > > Allocation failures of shadow used ring and batched copy array > > are not recoverable and leads to the segmentation faults like > > this on the receiving/transmission path: > > > > Program received signal SIGSEGV, Segmentation fault. > > [Switching to Thread 0x7f913fecf0 (LWP 43625)] > > in copy_desc_to_mbuf () at /lib/librte_vhost/virtio_net.c:760 > > 760 batch_copy[vq->batch_copy_nb_elems].dst = > > > > This could be easily reproduced in case of low memory or big > > number of vhost-user ports. Fix that by propagating error to > > the upper layer which will end up with disconnection. > > > > Fixes: f689586bc060 ("vhost: shadow used ring update") > > Cc: stable@dpdk.org > > > > Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > > --- > > lib/librte_vhost/vhost_user.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c > > index 9aa1ce118..4c7fd57fb 100644 > > --- a/lib/librte_vhost/vhost_user.c > > +++ b/lib/librte_vhost/vhost_user.c > > @@ -1693,7 +1693,9 @@ vhost_user_msg_handler(int vid, int fd) > > break; > > > > case VHOST_USER_SET_VRING_NUM: > > - vhost_user_set_vring_num(dev, &msg); > > + ret = vhost_user_set_vring_num(dev, &msg); > > + if (ret) > > + return -1; > > break; > > case VHOST_USER_SET_VRING_ADDR: > > vhost_user_set_vring_addr(&dev, &msg); > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] vhost: fix crash if set vring num handling failed 2018-08-30 6:06 ` Tiwei Bie @ 2018-08-30 6:54 ` Ilya Maximets 2018-09-03 2:26 ` Tiwei Bie 0 siblings, 1 reply; 9+ messages in thread From: Ilya Maximets @ 2018-08-30 6:54 UTC (permalink / raw) To: Tiwei Bie; +Cc: dev, Maxime Coquelin, Zhihong Wang, stable On 30.08.2018 09:06, Tiwei Bie wrote: > On Wed, Aug 29, 2018 at 05:00:47PM +0300, Ilya Maximets wrote: >> Any thoughts on this? > > Hi Ilya, > > Currently, the errors happened during messages handling > aren't handled properly. E.g. in vhost_user_set_mem_table(), > similar issues [1] may happen too. We might want to find > a way to fix this once and for all. > > [1] > https://github.com/DPDK/dpdk/blob/3605968c2fa7/lib/librte_vhost/vhost_user.c#L826 > https://github.com/DPDK/dpdk/blob/3605968c2fa7/lib/librte_vhost/vhost_user.c#L837 > https://github.com/DPDK/dpdk/blob/3605968c2fa7/lib/librte_vhost/vhost_user.c#L887 > > Thanks Hi. I understand your position position and, actually, I looked at other functions while preparing this fix. There is one difference between vhost_user_set_mem_table() and vhost_user_set_vring_num(). The first one is able to reply bad status. But yes, you're right that we should handle all the errors. What about following patch (compile tested only): ---- diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c index 9aa1ce118..63d145b2d 100644 --- a/lib/librte_vhost/vhost_user.c +++ b/lib/librte_vhost/vhost_user.c @@ -1014,7 +1014,7 @@ vhost_user_set_vring_call(struct virtio_net *dev, struct VhostUserMsg *pmsg) vq->callfd = file.fd; } -static void +static int vhost_user_set_vring_kick(struct virtio_net **pdev, struct VhostUserMsg *pmsg) { struct vhost_vring_file file; @@ -1032,7 +1032,7 @@ vhost_user_set_vring_kick(struct virtio_net **pdev, struct VhostUserMsg *pmsg) /* Interpret ring addresses only when ring is started. */ dev = translate_ring_addresses(dev, file.index); if (!dev) - return; + return -1; *pdev = dev; @@ -1049,6 +1049,7 @@ vhost_user_set_vring_kick(struct virtio_net **pdev, struct VhostUserMsg *pmsg) if (vq->kickfd >= 0) close(vq->kickfd); vq->kickfd = file.fd; + return 0; } static void @@ -1172,14 +1173,19 @@ vhost_user_get_protocol_features(struct virtio_net *dev, msg->size = sizeof(msg->payload.u64); } -static void +static int vhost_user_set_protocol_features(struct virtio_net *dev, uint64_t protocol_features) { - if (protocol_features & ~VHOST_USER_PROTOCOL_FEATURES) - return; + if (protocol_features & ~VHOST_USER_PROTOCOL_FEATURES) { + RTE_LOG(ERR, VHOST_CONFIG, + "(%d) received invalid protocol features.\n", + dev->vid); + return -1; + } dev->protocol_features = protocol_features; + return 0; } static int @@ -1657,8 +1663,6 @@ vhost_user_msg_handler(int vid, int fd) break; case VHOST_USER_SET_FEATURES: ret = vhost_user_set_features(dev, msg.payload.u64); - if (ret) - return -1; break; case VHOST_USER_GET_PROTOCOL_FEATURES: @@ -1666,14 +1670,14 @@ vhost_user_msg_handler(int vid, int fd) send_vhost_reply(fd, &msg); break; case VHOST_USER_SET_PROTOCOL_FEATURES: - vhost_user_set_protocol_features(dev, msg.payload.u64); + ret = vhost_user_set_protocol_features(dev, msg.payload.u64); break; case VHOST_USER_SET_OWNER: - vhost_user_set_owner(); + ret = vhost_user_set_owner(); break; case VHOST_USER_RESET_OWNER: - vhost_user_reset_owner(dev); + ret = vhost_user_reset_owner(dev); break; case VHOST_USER_SET_MEM_TABLE: @@ -1681,8 +1685,9 @@ vhost_user_msg_handler(int vid, int fd) break; case VHOST_USER_SET_LOG_BASE: - vhost_user_set_log_base(dev, &msg); - + ret = vhost_user_set_log_base(dev, &msg); + if (ret) + goto skip_to_reply; /* it needs a reply */ msg.size = sizeof(msg.payload.u64); send_vhost_reply(fd, &msg); @@ -1693,23 +1698,25 @@ vhost_user_msg_handler(int vid, int fd) break; case VHOST_USER_SET_VRING_NUM: - vhost_user_set_vring_num(dev, &msg); + ret = vhost_user_set_vring_num(dev, &msg); break; case VHOST_USER_SET_VRING_ADDR: - vhost_user_set_vring_addr(&dev, &msg); + ret = vhost_user_set_vring_addr(&dev, &msg); break; case VHOST_USER_SET_VRING_BASE: - vhost_user_set_vring_base(dev, &msg); + ret = vhost_user_set_vring_base(dev, &msg); break; case VHOST_USER_GET_VRING_BASE: - vhost_user_get_vring_base(dev, &msg); + ret = vhost_user_get_vring_base(dev, &msg); + if (ret) + goto skip_to_reply; msg.size = sizeof(msg.payload.state); send_vhost_reply(fd, &msg); break; case VHOST_USER_SET_VRING_KICK: - vhost_user_set_vring_kick(&dev, &msg); + ret = vhost_user_set_vring_kick(&dev, &msg); break; case VHOST_USER_SET_VRING_CALL: vhost_user_set_vring_call(dev, &msg); @@ -1728,10 +1735,10 @@ vhost_user_msg_handler(int vid, int fd) break; case VHOST_USER_SET_VRING_ENABLE: - vhost_user_set_vring_enable(dev, &msg); + ret = vhost_user_set_vring_enable(dev, &msg); break; case VHOST_USER_SEND_RARP: - vhost_user_send_rarp(dev, &msg); + ret = vhost_user_send_rarp(dev, &msg); break; case VHOST_USER_NET_SET_MTU: @@ -1752,7 +1759,7 @@ vhost_user_msg_handler(int vid, int fd) } skip_to_post_handle: - if (dev->extern_ops.post_msg_handle) { + if (!ret && dev->extern_ops.post_msg_handle) { uint32_t need_reply; ret = (*dev->extern_ops.post_msg_handle)( @@ -1772,6 +1779,10 @@ vhost_user_msg_handler(int vid, int fd) msg.payload.u64 = !!ret; msg.size = sizeof(msg.payload.u64); send_vhost_reply(fd, &msg); + } else if (ret) { + RTE_LOG(ERR, VHOST_CONFIG, + "vhost message handling failed.\n"); + return -1; } if (!(dev->flags & VIRTIO_DEV_RUNNING) && virtio_is_ready(dev)) { ---- Best regards, Ilya Maximets. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] vhost: fix crash if set vring num handling failed 2018-08-30 6:54 ` Ilya Maximets @ 2018-09-03 2:26 ` Tiwei Bie 2018-09-03 7:07 ` Ilya Maximets 0 siblings, 1 reply; 9+ messages in thread From: Tiwei Bie @ 2018-09-03 2:26 UTC (permalink / raw) To: Ilya Maximets; +Cc: dev, Maxime Coquelin, Zhihong Wang, stable On Thu, Aug 30, 2018 at 09:54:50AM +0300, Ilya Maximets wrote: > On 30.08.2018 09:06, Tiwei Bie wrote: > > On Wed, Aug 29, 2018 at 05:00:47PM +0300, Ilya Maximets wrote: > >> Any thoughts on this? > > > > Hi Ilya, > > > > Currently, the errors happened during messages handling > > aren't handled properly. E.g. in vhost_user_set_mem_table(), > > similar issues [1] may happen too. We might want to find > > a way to fix this once and for all. > > > > [1] > > https://github.com/DPDK/dpdk/blob/3605968c2fa7/lib/librte_vhost/vhost_user.c#L826 > > https://github.com/DPDK/dpdk/blob/3605968c2fa7/lib/librte_vhost/vhost_user.c#L837 > > https://github.com/DPDK/dpdk/blob/3605968c2fa7/lib/librte_vhost/vhost_user.c#L887 > > > > Thanks > > Hi. I understand your position position and, actually, I looked at > other functions while preparing this fix. There is one difference > between vhost_user_set_mem_table() and vhost_user_set_vring_num(). > The first one is able to reply bad status. > But yes, you're right that we should handle all the errors. > What about following patch (compile tested only): > ---- > diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c > index 9aa1ce118..63d145b2d 100644 > --- a/lib/librte_vhost/vhost_user.c > +++ b/lib/librte_vhost/vhost_user.c > @@ -1014,7 +1014,7 @@ vhost_user_set_vring_call(struct virtio_net *dev, struct VhostUserMsg *pmsg) > vq->callfd = file.fd; > } > > -static void > +static int > vhost_user_set_vring_kick(struct virtio_net **pdev, struct VhostUserMsg *pmsg) > { > struct vhost_vring_file file; > @@ -1032,7 +1032,7 @@ vhost_user_set_vring_kick(struct virtio_net **pdev, struct VhostUserMsg *pmsg) > /* Interpret ring addresses only when ring is started. */ > dev = translate_ring_addresses(dev, file.index); > if (!dev) > - return; > + return -1; > > *pdev = dev; > > @@ -1049,6 +1049,7 @@ vhost_user_set_vring_kick(struct virtio_net **pdev, struct VhostUserMsg *pmsg) > if (vq->kickfd >= 0) > close(vq->kickfd); > vq->kickfd = file.fd; > + return 0; > } > > static void > @@ -1172,14 +1173,19 @@ vhost_user_get_protocol_features(struct virtio_net *dev, > msg->size = sizeof(msg->payload.u64); > } > > -static void > +static int > vhost_user_set_protocol_features(struct virtio_net *dev, > uint64_t protocol_features) > { > - if (protocol_features & ~VHOST_USER_PROTOCOL_FEATURES) > - return; > + if (protocol_features & ~VHOST_USER_PROTOCOL_FEATURES) { > + RTE_LOG(ERR, VHOST_CONFIG, > + "(%d) received invalid protocol features.\n", > + dev->vid); > + return -1; > + } > > dev->protocol_features = protocol_features; > + return 0; > } > > static int > @@ -1657,8 +1663,6 @@ vhost_user_msg_handler(int vid, int fd) > break; > case VHOST_USER_SET_FEATURES: > ret = vhost_user_set_features(dev, msg.payload.u64); > - if (ret) > - return -1; > break; > > case VHOST_USER_GET_PROTOCOL_FEATURES: > @@ -1666,14 +1670,14 @@ vhost_user_msg_handler(int vid, int fd) > send_vhost_reply(fd, &msg); > break; > case VHOST_USER_SET_PROTOCOL_FEATURES: > - vhost_user_set_protocol_features(dev, msg.payload.u64); > + ret = vhost_user_set_protocol_features(dev, msg.payload.u64); > break; > > case VHOST_USER_SET_OWNER: > - vhost_user_set_owner(); > + ret = vhost_user_set_owner(); > break; > case VHOST_USER_RESET_OWNER: > - vhost_user_reset_owner(dev); > + ret = vhost_user_reset_owner(dev); > break; > > case VHOST_USER_SET_MEM_TABLE: > @@ -1681,8 +1685,9 @@ vhost_user_msg_handler(int vid, int fd) > break; > > case VHOST_USER_SET_LOG_BASE: > - vhost_user_set_log_base(dev, &msg); > - > + ret = vhost_user_set_log_base(dev, &msg); > + if (ret) > + goto skip_to_reply; > /* it needs a reply */ > msg.size = sizeof(msg.payload.u64); > send_vhost_reply(fd, &msg); > @@ -1693,23 +1698,25 @@ vhost_user_msg_handler(int vid, int fd) > break; > > case VHOST_USER_SET_VRING_NUM: > - vhost_user_set_vring_num(dev, &msg); > + ret = vhost_user_set_vring_num(dev, &msg); > break; > case VHOST_USER_SET_VRING_ADDR: > - vhost_user_set_vring_addr(&dev, &msg); > + ret = vhost_user_set_vring_addr(&dev, &msg); > break; > case VHOST_USER_SET_VRING_BASE: > - vhost_user_set_vring_base(dev, &msg); > + ret = vhost_user_set_vring_base(dev, &msg); > break; > > case VHOST_USER_GET_VRING_BASE: > - vhost_user_get_vring_base(dev, &msg); > + ret = vhost_user_get_vring_base(dev, &msg); > + if (ret) > + goto skip_to_reply; > msg.size = sizeof(msg.payload.state); > send_vhost_reply(fd, &msg); > break; > > case VHOST_USER_SET_VRING_KICK: > - vhost_user_set_vring_kick(&dev, &msg); > + ret = vhost_user_set_vring_kick(&dev, &msg); > break; > case VHOST_USER_SET_VRING_CALL: > vhost_user_set_vring_call(dev, &msg); > @@ -1728,10 +1735,10 @@ vhost_user_msg_handler(int vid, int fd) > break; > > case VHOST_USER_SET_VRING_ENABLE: > - vhost_user_set_vring_enable(dev, &msg); > + ret = vhost_user_set_vring_enable(dev, &msg); > break; > case VHOST_USER_SEND_RARP: > - vhost_user_send_rarp(dev, &msg); > + ret = vhost_user_send_rarp(dev, &msg); > break; > > case VHOST_USER_NET_SET_MTU: > @@ -1752,7 +1759,7 @@ vhost_user_msg_handler(int vid, int fd) > } > > skip_to_post_handle: > - if (dev->extern_ops.post_msg_handle) { > + if (!ret && dev->extern_ops.post_msg_handle) { > uint32_t need_reply; > > ret = (*dev->extern_ops.post_msg_handle)( > @@ -1772,6 +1779,10 @@ vhost_user_msg_handler(int vid, int fd) > msg.payload.u64 = !!ret; > msg.size = sizeof(msg.payload.u64); > send_vhost_reply(fd, &msg); > + } else if (ret) { > + RTE_LOG(ERR, VHOST_CONFIG, > + "vhost message handling failed.\n"); > + return -1; > } The basic idea behind above code is to drop the connection if we can't report the error to QEMU when error happens. It looks good to me. Thanks > > if (!(dev->flags & VIRTIO_DEV_RUNNING) && virtio_is_ready(dev)) { > > ---- > > Best regards, Ilya Maximets. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] vhost: fix crash if set vring num handling failed 2018-09-03 2:26 ` Tiwei Bie @ 2018-09-03 7:07 ` Ilya Maximets 0 siblings, 0 replies; 9+ messages in thread From: Ilya Maximets @ 2018-09-03 7:07 UTC (permalink / raw) To: Tiwei Bie; +Cc: dev, Maxime Coquelin, Zhihong Wang, stable On 03.09.2018 05:26, Tiwei Bie wrote: > On Thu, Aug 30, 2018 at 09:54:50AM +0300, Ilya Maximets wrote: >> On 30.08.2018 09:06, Tiwei Bie wrote: >>> On Wed, Aug 29, 2018 at 05:00:47PM +0300, Ilya Maximets wrote: >>>> Any thoughts on this? >>> >>> Hi Ilya, >>> >>> Currently, the errors happened during messages handling >>> aren't handled properly. E.g. in vhost_user_set_mem_table(), >>> similar issues [1] may happen too. We might want to find >>> a way to fix this once and for all. >>> >>> [1] >>> https://github.com/DPDK/dpdk/blob/3605968c2fa7/lib/librte_vhost/vhost_user.c#L826 >>> https://github.com/DPDK/dpdk/blob/3605968c2fa7/lib/librte_vhost/vhost_user.c#L837 >>> https://github.com/DPDK/dpdk/blob/3605968c2fa7/lib/librte_vhost/vhost_user.c#L887 >>> >>> Thanks >> >> Hi. I understand your position position and, actually, I looked at >> other functions while preparing this fix. There is one difference >> between vhost_user_set_mem_table() and vhost_user_set_vring_num(). >> The first one is able to reply bad status. >> But yes, you're right that we should handle all the errors. >> What about following patch (compile tested only): >> ---- >> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c >> index 9aa1ce118..63d145b2d 100644 >> --- a/lib/librte_vhost/vhost_user.c >> +++ b/lib/librte_vhost/vhost_user.c >> @@ -1014,7 +1014,7 @@ vhost_user_set_vring_call(struct virtio_net *dev, struct VhostUserMsg *pmsg) >> vq->callfd = file.fd; >> } >> >> -static void >> +static int >> vhost_user_set_vring_kick(struct virtio_net **pdev, struct VhostUserMsg *pmsg) >> { >> struct vhost_vring_file file; >> @@ -1032,7 +1032,7 @@ vhost_user_set_vring_kick(struct virtio_net **pdev, struct VhostUserMsg *pmsg) >> /* Interpret ring addresses only when ring is started. */ >> dev = translate_ring_addresses(dev, file.index); >> if (!dev) >> - return; >> + return -1; >> >> *pdev = dev; >> >> @@ -1049,6 +1049,7 @@ vhost_user_set_vring_kick(struct virtio_net **pdev, struct VhostUserMsg *pmsg) >> if (vq->kickfd >= 0) >> close(vq->kickfd); >> vq->kickfd = file.fd; >> + return 0; >> } >> >> static void >> @@ -1172,14 +1173,19 @@ vhost_user_get_protocol_features(struct virtio_net *dev, >> msg->size = sizeof(msg->payload.u64); >> } >> >> -static void >> +static int >> vhost_user_set_protocol_features(struct virtio_net *dev, >> uint64_t protocol_features) >> { >> - if (protocol_features & ~VHOST_USER_PROTOCOL_FEATURES) >> - return; >> + if (protocol_features & ~VHOST_USER_PROTOCOL_FEATURES) { >> + RTE_LOG(ERR, VHOST_CONFIG, >> + "(%d) received invalid protocol features.\n", >> + dev->vid); >> + return -1; >> + } >> >> dev->protocol_features = protocol_features; >> + return 0; >> } >> >> static int >> @@ -1657,8 +1663,6 @@ vhost_user_msg_handler(int vid, int fd) >> break; >> case VHOST_USER_SET_FEATURES: >> ret = vhost_user_set_features(dev, msg.payload.u64); >> - if (ret) >> - return -1; >> break; >> >> case VHOST_USER_GET_PROTOCOL_FEATURES: >> @@ -1666,14 +1670,14 @@ vhost_user_msg_handler(int vid, int fd) >> send_vhost_reply(fd, &msg); >> break; >> case VHOST_USER_SET_PROTOCOL_FEATURES: >> - vhost_user_set_protocol_features(dev, msg.payload.u64); >> + ret = vhost_user_set_protocol_features(dev, msg.payload.u64); >> break; >> >> case VHOST_USER_SET_OWNER: >> - vhost_user_set_owner(); >> + ret = vhost_user_set_owner(); >> break; >> case VHOST_USER_RESET_OWNER: >> - vhost_user_reset_owner(dev); >> + ret = vhost_user_reset_owner(dev); >> break; >> >> case VHOST_USER_SET_MEM_TABLE: >> @@ -1681,8 +1685,9 @@ vhost_user_msg_handler(int vid, int fd) >> break; >> >> case VHOST_USER_SET_LOG_BASE: >> - vhost_user_set_log_base(dev, &msg); >> - >> + ret = vhost_user_set_log_base(dev, &msg); >> + if (ret) >> + goto skip_to_reply; >> /* it needs a reply */ >> msg.size = sizeof(msg.payload.u64); >> send_vhost_reply(fd, &msg); >> @@ -1693,23 +1698,25 @@ vhost_user_msg_handler(int vid, int fd) >> break; >> >> case VHOST_USER_SET_VRING_NUM: >> - vhost_user_set_vring_num(dev, &msg); >> + ret = vhost_user_set_vring_num(dev, &msg); >> break; >> case VHOST_USER_SET_VRING_ADDR: >> - vhost_user_set_vring_addr(&dev, &msg); >> + ret = vhost_user_set_vring_addr(&dev, &msg); >> break; >> case VHOST_USER_SET_VRING_BASE: >> - vhost_user_set_vring_base(dev, &msg); >> + ret = vhost_user_set_vring_base(dev, &msg); >> break; >> >> case VHOST_USER_GET_VRING_BASE: >> - vhost_user_get_vring_base(dev, &msg); >> + ret = vhost_user_get_vring_base(dev, &msg); >> + if (ret) >> + goto skip_to_reply; >> msg.size = sizeof(msg.payload.state); >> send_vhost_reply(fd, &msg); >> break; >> >> case VHOST_USER_SET_VRING_KICK: >> - vhost_user_set_vring_kick(&dev, &msg); >> + ret = vhost_user_set_vring_kick(&dev, &msg); >> break; >> case VHOST_USER_SET_VRING_CALL: >> vhost_user_set_vring_call(dev, &msg); >> @@ -1728,10 +1735,10 @@ vhost_user_msg_handler(int vid, int fd) >> break; >> >> case VHOST_USER_SET_VRING_ENABLE: >> - vhost_user_set_vring_enable(dev, &msg); >> + ret = vhost_user_set_vring_enable(dev, &msg); >> break; >> case VHOST_USER_SEND_RARP: >> - vhost_user_send_rarp(dev, &msg); >> + ret = vhost_user_send_rarp(dev, &msg); >> break; >> >> case VHOST_USER_NET_SET_MTU: >> @@ -1752,7 +1759,7 @@ vhost_user_msg_handler(int vid, int fd) >> } >> >> skip_to_post_handle: >> - if (dev->extern_ops.post_msg_handle) { >> + if (!ret && dev->extern_ops.post_msg_handle) { >> uint32_t need_reply; >> >> ret = (*dev->extern_ops.post_msg_handle)( >> @@ -1772,6 +1779,10 @@ vhost_user_msg_handler(int vid, int fd) >> msg.payload.u64 = !!ret; >> msg.size = sizeof(msg.payload.u64); >> send_vhost_reply(fd, &msg); >> + } else if (ret) { >> + RTE_LOG(ERR, VHOST_CONFIG, >> + "vhost message handling failed.\n"); >> + return -1; >> } > > The basic idea behind above code is to drop the connection > if we can't report the error to QEMU when error happens. It > looks good to me. > > Thanks OK. Good. I'll prepare a proper patch. Best regards, Ilya Maximets. >> >> if (!(dev->flags & VIRTIO_DEV_RUNNING) && virtio_is_ready(dev)) { >> >> ---- >> >> Best regards, Ilya Maximets. > > ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: <CGME20180903101059eucas1p2030d1e9077a03bc5d0613ba0311e1eab@eucas1p2.samsung.com>]
* [dpdk-dev] [PATCH v2] vhost-user: drop connection on message handling failures [not found] ` <CGME20180903101059eucas1p2030d1e9077a03bc5d0613ba0311e1eab@eucas1p2.samsung.com> @ 2018-09-03 10:12 ` Ilya Maximets 2018-09-10 13:51 ` Maxime Coquelin 2018-09-11 9:25 ` Maxime Coquelin 0 siblings, 2 replies; 9+ messages in thread From: Ilya Maximets @ 2018-09-03 10:12 UTC (permalink / raw) To: dev; +Cc: Maxime Coquelin, Tiwei Bie, Zhihong Wang, Ilya Maximets, stable There are a lot of cases where vhost-user massage handling could fail and end up in a fully not recoverable state. For example, allocation failures of shadow used ring and batched copy array are not recoverable and leads to the segmentation faults like this on the receiving/transmission path: Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 0x7f913fecf0 (LWP 43625)] in copy_desc_to_mbuf () at /lib/librte_vhost/virtio_net.c:760 760 batch_copy[vq->batch_copy_nb_elems].dst = This could be easily reproduced in case of low memory or big number of vhost-user ports. Fix that by propagating error to the upper layer which will end up with disconnection in case we can not report to the message sender when the error happens. Fixes: f689586bc060 ("vhost: shadow used ring update") Cc: stable@dpdk.org Signed-off-by: Ilya Maximets <i.maximets@samsung.com> --- v2: * Patch changed to cover most of possible failures at once. [Tiwei Bie] lib/librte_vhost/vhost_user.c | 51 +++++++++++++++++++++-------------- 1 file changed, 31 insertions(+), 20 deletions(-) diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c index a2d4c9ffc..7997b890f 100644 --- a/lib/librte_vhost/vhost_user.c +++ b/lib/librte_vhost/vhost_user.c @@ -1012,7 +1012,7 @@ vhost_user_set_vring_call(struct virtio_net *dev, struct VhostUserMsg *pmsg) vq->callfd = file.fd; } -static void +static int vhost_user_set_vring_kick(struct virtio_net **pdev, struct VhostUserMsg *pmsg) { struct vhost_vring_file file; @@ -1030,7 +1030,7 @@ vhost_user_set_vring_kick(struct virtio_net **pdev, struct VhostUserMsg *pmsg) /* Interpret ring addresses only when ring is started. */ dev = translate_ring_addresses(dev, file.index); if (!dev) - return; + return -1; *pdev = dev; @@ -1047,6 +1047,7 @@ vhost_user_set_vring_kick(struct virtio_net **pdev, struct VhostUserMsg *pmsg) if (vq->kickfd >= 0) close(vq->kickfd); vq->kickfd = file.fd; + return 0; } static void @@ -1170,14 +1171,19 @@ vhost_user_get_protocol_features(struct virtio_net *dev, msg->size = sizeof(msg->payload.u64); } -static void +static int vhost_user_set_protocol_features(struct virtio_net *dev, uint64_t protocol_features) { - if (protocol_features & ~VHOST_USER_PROTOCOL_FEATURES) - return; + if (protocol_features & ~VHOST_USER_PROTOCOL_FEATURES) { + RTE_LOG(ERR, VHOST_CONFIG, + "(%d) received invalid protocol features.\n", + dev->vid); + return -1; + } dev->protocol_features = protocol_features; + return 0; } static int @@ -1655,8 +1661,6 @@ vhost_user_msg_handler(int vid, int fd) break; case VHOST_USER_SET_FEATURES: ret = vhost_user_set_features(dev, msg.payload.u64); - if (ret) - return -1; break; case VHOST_USER_GET_PROTOCOL_FEATURES: @@ -1664,14 +1668,14 @@ vhost_user_msg_handler(int vid, int fd) send_vhost_reply(fd, &msg); break; case VHOST_USER_SET_PROTOCOL_FEATURES: - vhost_user_set_protocol_features(dev, msg.payload.u64); + ret = vhost_user_set_protocol_features(dev, msg.payload.u64); break; case VHOST_USER_SET_OWNER: - vhost_user_set_owner(); + ret = vhost_user_set_owner(); break; case VHOST_USER_RESET_OWNER: - vhost_user_reset_owner(dev); + ret = vhost_user_reset_owner(dev); break; case VHOST_USER_SET_MEM_TABLE: @@ -1679,8 +1683,9 @@ vhost_user_msg_handler(int vid, int fd) break; case VHOST_USER_SET_LOG_BASE: - vhost_user_set_log_base(dev, &msg); - + ret = vhost_user_set_log_base(dev, &msg); + if (ret) + goto skip_to_reply; /* it needs a reply */ msg.size = sizeof(msg.payload.u64); send_vhost_reply(fd, &msg); @@ -1691,23 +1696,25 @@ vhost_user_msg_handler(int vid, int fd) break; case VHOST_USER_SET_VRING_NUM: - vhost_user_set_vring_num(dev, &msg); + ret = vhost_user_set_vring_num(dev, &msg); break; case VHOST_USER_SET_VRING_ADDR: - vhost_user_set_vring_addr(&dev, &msg); + ret = vhost_user_set_vring_addr(&dev, &msg); break; case VHOST_USER_SET_VRING_BASE: - vhost_user_set_vring_base(dev, &msg); + ret = vhost_user_set_vring_base(dev, &msg); break; case VHOST_USER_GET_VRING_BASE: - vhost_user_get_vring_base(dev, &msg); + ret = vhost_user_get_vring_base(dev, &msg); + if (ret) + goto skip_to_reply; msg.size = sizeof(msg.payload.state); send_vhost_reply(fd, &msg); break; case VHOST_USER_SET_VRING_KICK: - vhost_user_set_vring_kick(&dev, &msg); + ret = vhost_user_set_vring_kick(&dev, &msg); break; case VHOST_USER_SET_VRING_CALL: vhost_user_set_vring_call(dev, &msg); @@ -1726,10 +1733,10 @@ vhost_user_msg_handler(int vid, int fd) break; case VHOST_USER_SET_VRING_ENABLE: - vhost_user_set_vring_enable(dev, &msg); + ret = vhost_user_set_vring_enable(dev, &msg); break; case VHOST_USER_SEND_RARP: - vhost_user_send_rarp(dev, &msg); + ret = vhost_user_send_rarp(dev, &msg); break; case VHOST_USER_NET_SET_MTU: @@ -1750,7 +1757,7 @@ vhost_user_msg_handler(int vid, int fd) } skip_to_post_handle: - if (dev->extern_ops.post_msg_handle) { + if (!ret && dev->extern_ops.post_msg_handle) { uint32_t need_reply; ret = (*dev->extern_ops.post_msg_handle)( @@ -1770,6 +1777,10 @@ vhost_user_msg_handler(int vid, int fd) msg.payload.u64 = !!ret; msg.size = sizeof(msg.payload.u64); send_vhost_reply(fd, &msg); + } else if (ret) { + RTE_LOG(ERR, VHOST_CONFIG, + "vhost message handling failed.\n"); + return -1; } if (!(dev->flags & VIRTIO_DEV_RUNNING) && virtio_is_ready(dev)) { -- 2.17.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH v2] vhost-user: drop connection on message handling failures 2018-09-03 10:12 ` [dpdk-dev] [PATCH v2] vhost-user: drop connection on message handling failures Ilya Maximets @ 2018-09-10 13:51 ` Maxime Coquelin 2018-09-11 9:25 ` Maxime Coquelin 1 sibling, 0 replies; 9+ messages in thread From: Maxime Coquelin @ 2018-09-10 13:51 UTC (permalink / raw) To: Ilya Maximets, dev; +Cc: Tiwei Bie, Zhihong Wang, stable On 09/03/2018 12:12 PM, Ilya Maximets wrote: > There are a lot of cases where vhost-user massage handling > could fail and end up in a fully not recoverable state. For > example, allocation failures of shadow used ring and batched > copy array are not recoverable and leads to the segmentation > faults like this on the receiving/transmission path: > > Program received signal SIGSEGV, Segmentation fault. > [Switching to Thread 0x7f913fecf0 (LWP 43625)] > in copy_desc_to_mbuf () at /lib/librte_vhost/virtio_net.c:760 > 760 batch_copy[vq->batch_copy_nb_elems].dst = > > This could be easily reproduced in case of low memory or big > number of vhost-user ports. > > Fix that by propagating error to the upper layer which will > end up with disconnection in case we can not report to > the message sender when the error happens. > > Fixes: f689586bc060 ("vhost: shadow used ring update") > Cc:stable@dpdk.org > > Signed-off-by: Ilya Maximets<i.maximets@samsung.com> > --- > > v2: > * Patch changed to cover most of possible failures at once. [Tiwei Bie] > > lib/librte_vhost/vhost_user.c | 51 +++++++++++++++++++++-------------- > 1 file changed, 31 insertions(+), 20 deletions(-) Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com> Thanks! Maxime ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH v2] vhost-user: drop connection on message handling failures 2018-09-03 10:12 ` [dpdk-dev] [PATCH v2] vhost-user: drop connection on message handling failures Ilya Maximets 2018-09-10 13:51 ` Maxime Coquelin @ 2018-09-11 9:25 ` Maxime Coquelin 1 sibling, 0 replies; 9+ messages in thread From: Maxime Coquelin @ 2018-09-11 9:25 UTC (permalink / raw) To: Ilya Maximets, dev; +Cc: Tiwei Bie, Zhihong Wang, stable On 09/03/2018 12:12 PM, Ilya Maximets wrote: > There are a lot of cases where vhost-user massage handling > could fail and end up in a fully not recoverable state. For > example, allocation failures of shadow used ring and batched > copy array are not recoverable and leads to the segmentation > faults like this on the receiving/transmission path: > > Program received signal SIGSEGV, Segmentation fault. > [Switching to Thread 0x7f913fecf0 (LWP 43625)] > in copy_desc_to_mbuf () at /lib/librte_vhost/virtio_net.c:760 > 760 batch_copy[vq->batch_copy_nb_elems].dst = > > This could be easily reproduced in case of low memory or big > number of vhost-user ports. > > Fix that by propagating error to the upper layer which will > end up with disconnection in case we can not report to > the message sender when the error happens. > > Fixes: f689586bc060 ("vhost: shadow used ring update") > Cc: stable@dpdk.org > > Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > --- > > v2: > * Patch changed to cover most of possible failures at once. [Tiwei Bie] > > lib/librte_vhost/vhost_user.c | 51 +++++++++++++++++++++-------------- > 1 file changed, 31 insertions(+), 20 deletions(-) Applied to dpdk-next-virtio/master. Thanks! Maxime ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-09-11 9:25 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20180817113255eucas1p2042d22241265dafa241c2a6a2c949244@eucas1p2.samsung.com>
2018-08-17 11:33 ` [dpdk-dev] [PATCH] vhost: fix crash if set vring num handling failed Ilya Maximets
2018-08-29 14:00 ` Ilya Maximets
2018-08-30 6:06 ` Tiwei Bie
2018-08-30 6:54 ` Ilya Maximets
2018-09-03 2:26 ` Tiwei Bie
2018-09-03 7:07 ` Ilya Maximets
[not found] ` <CGME20180903101059eucas1p2030d1e9077a03bc5d0613ba0311e1eab@eucas1p2.samsung.com>
2018-09-03 10:12 ` [dpdk-dev] [PATCH v2] vhost-user: drop connection on message handling failures Ilya Maximets
2018-09-10 13:51 ` Maxime Coquelin
2018-09-11 9:25 ` Maxime Coquelin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).