From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mailout2.w1.samsung.com (mailout2.w1.samsung.com [210.118.77.12]) by dpdk.org (Postfix) with ESMTP id 2C6F04D3A for ; Mon, 3 Sep 2018 09:05:54 +0200 (CEST) Received: from eucas1p2.samsung.com (unknown [182.198.249.207]) by mailout2.w1.samsung.com (KnoxPortal) with ESMTP id 20180903070553euoutp0241b23d7ac5bd3ec33ae4853dac9ab1fb~Q0fwWhZcN1236912369euoutp02b for ; Mon, 3 Sep 2018 07:05:53 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout2.w1.samsung.com 20180903070553euoutp0241b23d7ac5bd3ec33ae4853dac9ab1fb~Q0fwWhZcN1236912369euoutp02b DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1535958353; bh=xXFc6/lmvrhho0r5Op2yJKFVz8ISkICT9rflrdzy9P8=; h=Subject:To:Cc:From:Date:In-Reply-To:References:From; b=ZNBa5guvG3J+HEQ1CW23QSMBwvGPG8dea3w/17kCPbQYw27sxXF7pQPHosacOHkWz r5EXC9teT9CQHs2TGnk8k7A4z31m6QjpA6T9sGnH3EIgg0fmY9A45lUWKb9i3C3gT2 SsnkIGmyZ0PRwajaHJPiFMhwctBPuUXR4hhXSJIw= Received: from eusmges1new.samsung.com (unknown [203.254.199.242]) by eucas1p2.samsung.com (KnoxPortal) with ESMTP id 20180903070552eucas1p2115ddb88d021b8cc852ac303f1a30e7a~Q0fvyy__B1500815008eucas1p20; Mon, 3 Sep 2018 07:05:52 +0000 (GMT) Received: from eucas1p1.samsung.com ( [182.198.249.206]) by eusmges1new.samsung.com (EUCPMTA) with SMTP id 49.4D.04441.05DDC8B5; Mon, 3 Sep 2018 08:05:52 +0100 (BST) Received: from eusmtrp1.samsung.com (unknown [182.198.249.138]) by eucas1p2.samsung.com (KnoxPortal) with ESMTPA id 20180903070552eucas1p2fb3bfba7191883bb84f2038a057c82fc~Q0fu924sK1503615036eucas1p21; Mon, 3 Sep 2018 07:05:52 +0000 (GMT) Received: from eusmgms2.samsung.com (unknown [182.198.249.180]) by eusmtrp1.samsung.com (KnoxPortal) with ESMTP id 20180903070551eusmtrp16e605ce17207f5a03d0561beaaf2109c~Q0fusKdmd1282512825eusmtrp1n; Mon, 3 Sep 2018 07:05:51 +0000 (GMT) X-AuditID: cbfec7f2-5e3ff70000001159-58-5b8cdd507b66 Received: from eusmtip2.samsung.com ( [203.254.199.222]) by eusmgms2.samsung.com (EUCPMTA) with SMTP id D6.A4.04128.F4DDC8B5; Mon, 3 Sep 2018 08:05:51 +0100 (BST) Received: from [106.109.129.180] (unknown [106.109.129.180]) by eusmtip2.samsung.com (KnoxPortal) with ESMTPA id 20180903070551eusmtip25edc89a875b374efe724b7573086b4e5~Q0fuPfz5V1570915709eusmtip2o; Mon, 3 Sep 2018 07:05:51 +0000 (GMT) To: Tiwei Bie Cc: dev@dpdk.org, Maxime Coquelin , Zhihong Wang , stable@dpdk.org From: Ilya Maximets Date: Mon, 3 Sep 2018 10:07:18 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20180903022617.GA11310@fbsd.sh.intel.com> Content-Language: en-GB Content-Transfer-Encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrAKsWRmVeSWpSXmKPExsWy7djPc7oBd3uiDVb/VrF492k7k8WV9p/s Fsc697BY/Ov4w26xteE/k8Xmi5OYHNg8fi1YyuqxeM9LJo/3+66yefRtWcUYwBLFZZOSmpNZ llqkb5fAlbHwZQNjwQzjijM/ohsYV2t2MXJySAiYSDSvPc3axcjFISSwglFi6e8WZgjnC6PE qxmPoDKfGSUWHb7JCtNyteEIE0RiOaPEoxOr2CGcj4wSHfsfsoFUCQu4Say/9gzMFhFQlvh2 ZiZQNwcHs0CFxIU9piBhNgEdiVOrjzCC2CwCKhKvPt5gBrFFBSIkjjxYCBbnFRCUODnzCQuI zSlgLjHp0j0wm1lAXKLpy0pWCFteYvvbOWBnSwgsYpe419PJAtFcJnH/+FSoq10kbq9YwAJh C0u8Or6FHcKWkTg9uQcqXi9xv+UlI8SgDkaJ6Yf+MUEk7CW2vD7HDvGApsT6XfoQYUeJ/12b WUDCEgJ8EjfeCkLcwycxadt0Zogwr0RHmxBEtYrE74PLmSFsKYmb7z6zT2BUmoXky1lIPpuF 5LNZCHsXMLKsYhRPLS3OTU8tNsxLLdcrTswtLs1L10vOz93ECEw3p/8d/7SD8eulpEOMAhyM Sjy8FzR7ooVYE8uKK3MPMUpwMCuJ8H5J7YoW4k1JrKxKLcqPLyrNSS0+xCjNwaIkzsunlRYt JJCeWJKanZpakFoEk2Xi4JRqYGx+MP9G3gT2E83V7LMY3p9a3+EfFvlh2tc8i7nfXhxMtFwi +kN0T+par9/CIirfe4/d1dTYGaq2Pm5/juTp272ZHvt7lK7+UNjdL9+7Wy+iOepe3B0+t2fV rBemPO6V0LxT/2FiyZTbtSFBr24/jF0u+e+125KdT/6x6/2M0RSqaL6x31FIkkOJpTgj0VCL uag4EQDpUGa4MwMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrCIsWRmVeSWpSXmKPExsVy+t/xe7r+d3uiDWa2aFu8+7SdyeJK+092 i2Ode1gs/nX8YbfY2vCfyWLzxUlMDmwevxYsZfVYvOclk8f7fVfZPPq2rGIMYInSsynKLy1J VcjILy6xVYo2tDDSM7S00DMysdQzNDaPtTIyVdK3s0lJzcksSy3St0vQy1j4soGxYIZxxZkf 0Q2MqzW7GDk5JARMJK42HGHqYuTiEBJYyiix6N8yVoiElMSPXxegbGGJP9e62CCK3jNKPLl6 ngUkISzgJrH+2jM2EFtEQFni25mZYA3MAhUSS3e9gWo4xCyx+2M7E0iCTUBH4tTqI4wgNq+A ncTXuY/ABrEIqEi8+niDGcQWFYiQWL38BStEjaDEyZlPwGo4BcwlJl26xwKxQF3iz7xLzBC2 uETTl5VQi+Ultr+dwzyBUWgWkvZZSFpmIWmZhaRlASPLKkaR1NLi3PTcYiO94sTc4tK8dL3k /NxNjMAY23bs55YdjF3vgg8xCnAwKvHwXtDsiRZiTSwrrsw9xCjBwawkwvsltStaiDclsbIq tSg/vqg0J7X4EKMp0HMTmaVEk/OB8Z9XEm9oamhuYWlobmxubGahJM573qAySkggPbEkNTs1 tSC1CKaPiYNTqoHRpnJq/Bp19QKbcq/3YUaN4rmfpXP5ez4ov177b97/rFmWDIwXnx35vqu+ bfGDWSzMXlskl553Xl29Zo7vpZdbiudvbr8sNS1m7iGlDW9K7p328nI2+/Zyzpqj/xLXv1bg 14jIWFBSnnHuPHfUFTGOl/uNPpuwTQiqV9dJ2/ywR2+5OlvcZK61SizFGYmGWsxFxYkAIln3 vscCAAA= Message-Id: <20180903070552eucas1p2fb3bfba7191883bb84f2038a057c82fc~Q0fu924sK1503615036eucas1p21@eucas1p2.samsung.com> X-CMS-MailID: 20180903070552eucas1p2fb3bfba7191883bb84f2038a057c82fc X-Msg-Generator: CA Content-Type: text/plain; charset="utf-8" X-RootMTR: 20180817113255eucas1p2042d22241265dafa241c2a6a2c949244 X-EPHeader: CA CMS-TYPE: 201P X-CMS-RootMailID: 20180817113255eucas1p2042d22241265dafa241c2a6a2c949244 References: <20180817113354.16140-1-i.maximets@samsung.com> <20180829135924eucas1p284d5711018eeba8b0ec3aaa6c84cef10~PX6Xgs7eA2716827168eucas1p22@eucas1p2.samsung.com> <20180830060642.GA7890@fbsd.sh.intel.com> <20180830065331eucas1p23467f23f8363577a842bc891c1b5f3e2~Plvz4erV-1928619286eucas1p2F@eucas1p2.samsung.com> <20180903022617.GA11310@fbsd.sh.intel.com> Subject: Re: [dpdk-dev] [PATCH] vhost: fix crash if set vring num handling failed X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 03 Sep 2018 07:05:55 -0000 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. > >