From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id B4C612716 for ; Wed, 30 Mar 2016 11:05:08 +0200 (CEST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga101.jf.intel.com with ESMTP; 30 Mar 2016 02:05:08 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,415,1455004800"; d="scan'208";a="944331259" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by orsmga002.jf.intel.com with ESMTP; 30 Mar 2016 02:05:07 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 30 Mar 2016 02:05:07 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.136]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.132]) with mapi id 14.03.0248.002; Wed, 30 Mar 2016 17:05:05 +0800 From: "Xie, Huawei" To: Patrik Andersson , "dev@dpdk.org" , Thomas Monjalon , "Ananyev, Konstantin" , Yuanhan Liu Thread-Topic: [dpdk-dev] [RFC] vhost user: add error handling for fd > 1023 Thread-Index: AdGKYz+H4+lvwAL9SXmR+Rrs6lvXdQ== Date: Wed, 30 Mar 2016 09:05:04 +0000 Message-ID: References: <1458292380-9258-1-git-send-email-patrik.r.andersson@ericsson.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [RFC] vhost user: add error handling for fd > 1023 X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 30 Mar 2016 09:05:09 -0000 On 3/18/2016 5:15 PM, Patrik Andersson wrote:=0A= > Protect against DPDK crash when allocation of listen fd >=3D 1023.=0A= > For events on fd:s >1023, the current implementation will trigger=0A= > an abort due to access outside of allocated bit mask.=0A= >=0A= > Corrections would include:=0A= >=0A= > * Match fdset_add() signature in fd_man.c to fd_man.h=0A= > * Handling of return codes from fdset_add()=0A= > * Addition of check of fd number in fdset_add_fd()=0A= >=0A= > ---=0A= >=0A= > The rationale behind the suggested code change is that,=0A= > fdset_event_dispatch() could attempt access outside of the FD_SET=0A= > bitmask if there is an event on a file descriptor that in turn=0A= > looks up a virtio file descriptor with a value > 1023.=0A= > Such an attempt will lead to an abort() and a restart of any=0A= > vswitch using DPDK.=0A= >=0A= > A discussion topic exist in the ovs-discuss mailing list that can=0A= > provide a little more background:=0A= >=0A= > http://openvswitch.org/pipermail/discuss/2016-February/020243.html=0A= =0A= Thanks for catching this. Could you give more details on how to=0A= accumulating fds?=0A= The buggy code is based on the fact that FD_SETSIZE limits the number of=0A= file descriptors, which might be true on Windows. However from the=0A= manual, it says clearly it limits the value of file descriptors.=0A= The use of select is for portability. I have been wondering if it is=0A= truly that important. Use poll could simplify the code a bit, for=0A= example we need add timeout to select so that another thread could=0A= insert/remove a fd into/from the monitored list.=0A= =0A= Any comments on using poll/epoll?=0A= =0A= >=0A= > Signed-off-by: Patrik Andersson =0A= > ---=0A= > lib/librte_vhost/vhost_user/fd_man.c | 11 ++++++-----=0A= > lib/librte_vhost/vhost_user/vhost-net-user.c | 22 ++++++++++++++++++++--= =0A= > 2 files changed, 26 insertions(+), 7 deletions(-)=0A= >=0A= > diff --git a/lib/librte_vhost/vhost_user/fd_man.c b/lib/librte_vhost/vhos= t_user/fd_man.c=0A= > index 087aaed..c691339 100644=0A= > --- a/lib/librte_vhost/vhost_user/fd_man.c=0A= > +++ b/lib/librte_vhost/vhost_user/fd_man.c=0A= > @@ -71,20 +71,22 @@ fdset_find_free_slot(struct fdset *pfdset)=0A= > return fdset_find_fd(pfdset, -1);=0A= > }=0A= > =0A= > -static void=0A= > +static int=0A= > fdset_add_fd(struct fdset *pfdset, int idx, int fd,=0A= > fd_cb rcb, fd_cb wcb, void *dat)=0A= > {=0A= > struct fdentry *pfdentry;=0A= > =0A= > - if (pfdset =3D=3D NULL || idx >=3D MAX_FDS)=0A= =0A= seems we had better change the definition of MAX_FDS and=0A= MAX_VHOST_SERVER to FD_SETSIZE or add some build time check.=0A= =0A= > - return;=0A= > + if (pfdset =3D=3D NULL || idx >=3D MAX_FDS || fd >=3D FD_SETSIZE)=0A= > + return -1;=0A= > =0A= > pfdentry =3D &pfdset->fd[idx];=0A= > pfdentry->fd =3D fd;=0A= > pfdentry->rcb =3D rcb;=0A= > pfdentry->wcb =3D wcb;=0A= > pfdentry->dat =3D dat;=0A= > +=0A= > + return 0;=0A= > }=0A= > =0A= > /**=0A= > @@ -150,12 +152,11 @@ fdset_add(struct fdset *pfdset, int fd, fd_cb rcb, = fd_cb wcb, void *dat)=0A= > =0A= > /* Find a free slot in the list. */=0A= > i =3D fdset_find_free_slot(pfdset);=0A= > - if (i =3D=3D -1) {=0A= > + if (i =3D=3D -1 || fdset_add_fd(pfdset, i, fd, rcb, wcb, dat) < 0) {=0A= > pthread_mutex_unlock(&pfdset->fd_mutex);=0A= > return -2;=0A= > }=0A= > =0A= > - fdset_add_fd(pfdset, i, fd, rcb, wcb, dat);=0A= > pfdset->num++;=0A= > =0A= > pthread_mutex_unlock(&pfdset->fd_mutex);=0A= > diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.c b/lib/librte_vh= ost/vhost_user/vhost-net-user.c=0A= > index df2bd64..778851d 100644=0A= > --- a/lib/librte_vhost/vhost_user/vhost-net-user.c=0A= > +++ b/lib/librte_vhost/vhost_user/vhost-net-user.c=0A= > @@ -288,6 +288,7 @@ vserver_new_vq_conn(int fd, void *dat, __rte_unused i= nt *remove)=0A= > int fh;=0A= > struct vhost_device_ctx vdev_ctx =3D { (pid_t)0, 0 };=0A= > unsigned int size;=0A= > + int ret;=0A= > =0A= > conn_fd =3D accept(fd, NULL, NULL);=0A= > RTE_LOG(INFO, VHOST_CONFIG,=0A= > @@ -317,8 +318,15 @@ vserver_new_vq_conn(int fd, void *dat, __rte_unused = int *remove)=0A= > =0A= > ctx->vserver =3D vserver;=0A= > ctx->fh =3D fh;=0A= > - fdset_add(&g_vhost_server.fdset,=0A= > + ret =3D fdset_add(&g_vhost_server.fdset,=0A= > conn_fd, vserver_message_handler, NULL, ctx);=0A= > + if (ret < 0) {=0A= > + free(ctx);=0A= > + close(conn_fd);=0A= > + RTE_LOG(ERR, VHOST_CONFIG,=0A= > + "failed to add fd %d into vhost server fdset\n",=0A= > + conn_fd);=0A= > + }=0A= > }=0A= > =0A= > /* callback when there is message on the connfd */=0A= > @@ -453,6 +461,7 @@ int=0A= > rte_vhost_driver_register(const char *path)=0A= > {=0A= > struct vhost_server *vserver;=0A= > + int ret;=0A= > =0A= > pthread_mutex_lock(&g_vhost_server.server_mutex);=0A= > =0A= > @@ -478,8 +487,17 @@ rte_vhost_driver_register(const char *path)=0A= > =0A= > vserver->path =3D strdup(path);=0A= > =0A= > - fdset_add(&g_vhost_server.fdset, vserver->listenfd,=0A= > + ret =3D fdset_add(&g_vhost_server.fdset, vserver->listenfd,=0A= > vserver_new_vq_conn, NULL, vserver);=0A= > + if (ret < 0) {=0A= > + pthread_mutex_unlock(&g_vhost_server.server_mutex);=0A= > + RTE_LOG(ERR, VHOST_CONFIG,=0A= > + "failed to add listen fd %d to vhost server fdset\n",=0A= > + vserver->listenfd);=0A= > + free(vserver->path);=0A= > + free(vserver);=0A= > + return -1;=0A= > + }=0A= > =0A= > g_vhost_server.server[g_vhost_server.vserver_cnt++] =3D vserver;=0A= > pthread_mutex_unlock(&g_vhost_server.server_mutex);=0A= =0A=