From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id ACBB74CB3; Mon, 3 Sep 2018 04:30:10 +0200 (CEST) X-Amp-Result: UNSCANNABLE X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 02 Sep 2018 19:30:09 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.53,323,1531810800"; d="scan'208";a="88448657" Received: from fbsd.sh.intel.com ([10.67.104.231]) by orsmga002.jf.intel.com with ESMTP; 02 Sep 2018 19:28:35 -0700 Date: Mon, 3 Sep 2018 10:26:17 +0800 From: Tiwei Bie To: Ilya Maximets Cc: dev@dpdk.org, Maxime Coquelin , Zhihong Wang , stable@dpdk.org Message-ID: <20180903022617.GA11310@fbsd.sh.intel.com> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20180830065331eucas1p23467f23f8363577a842bc891c1b5f3e2~Plvz4erV-1928619286eucas1p2F@eucas1p2.samsung.com> User-Agent: Mutt/1.10.1 (2018-07-13) 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 02:30:11 -0000 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.