From: "Xie, Huawei" <huawei.xie@intel.com>
To: Patrik Andersson <patrik.r.andersson@ericsson.com>,
"dev@dpdk.org" <dev@dpdk.org>,
Thomas Monjalon <thomas.monjalon@6wind.com>,
"Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
Yuanhan Liu <yuanhan.liu@linux.intel.com>
Subject: Re: [dpdk-dev] [RFC] vhost user: add error handling for fd > 1023
Date: Wed, 30 Mar 2016 09:05:04 +0000 [thread overview]
Message-ID: <C37D651A908B024F974696C65296B57B4C69D6C7@SHSMSX101.ccr.corp.intel.com> (raw)
In-Reply-To: <1458292380-9258-1-git-send-email-patrik.r.andersson@ericsson.com>
On 3/18/2016 5:15 PM, Patrik Andersson wrote:
> Protect against DPDK crash when allocation of listen fd >= 1023.
> For events on fd:s >1023, the current implementation will trigger
> an abort due to access outside of allocated bit mask.
>
> Corrections would include:
>
> * Match fdset_add() signature in fd_man.c to fd_man.h
> * Handling of return codes from fdset_add()
> * Addition of check of fd number in fdset_add_fd()
>
> ---
>
> The rationale behind the suggested code change is that,
> fdset_event_dispatch() could attempt access outside of the FD_SET
> bitmask if there is an event on a file descriptor that in turn
> looks up a virtio file descriptor with a value > 1023.
> Such an attempt will lead to an abort() and a restart of any
> vswitch using DPDK.
>
> A discussion topic exist in the ovs-discuss mailing list that can
> provide a little more background:
>
> http://openvswitch.org/pipermail/discuss/2016-February/020243.html
Thanks for catching this. Could you give more details on how to
accumulating fds?
The buggy code is based on the fact that FD_SETSIZE limits the number of
file descriptors, which might be true on Windows. However from the
manual, it says clearly it limits the value of file descriptors.
The use of select is for portability. I have been wondering if it is
truly that important. Use poll could simplify the code a bit, for
example we need add timeout to select so that another thread could
insert/remove a fd into/from the monitored list.
Any comments on using poll/epoll?
>
> Signed-off-by: Patrik Andersson <patrik.r.andersson@ericsson.com>
> ---
> lib/librte_vhost/vhost_user/fd_man.c | 11 ++++++-----
> lib/librte_vhost/vhost_user/vhost-net-user.c | 22 ++++++++++++++++++++--
> 2 files changed, 26 insertions(+), 7 deletions(-)
>
> diff --git a/lib/librte_vhost/vhost_user/fd_man.c b/lib/librte_vhost/vhost_user/fd_man.c
> index 087aaed..c691339 100644
> --- a/lib/librte_vhost/vhost_user/fd_man.c
> +++ b/lib/librte_vhost/vhost_user/fd_man.c
> @@ -71,20 +71,22 @@ fdset_find_free_slot(struct fdset *pfdset)
> return fdset_find_fd(pfdset, -1);
> }
>
> -static void
> +static int
> fdset_add_fd(struct fdset *pfdset, int idx, int fd,
> fd_cb rcb, fd_cb wcb, void *dat)
> {
> struct fdentry *pfdentry;
>
> - if (pfdset == NULL || idx >= MAX_FDS)
seems we had better change the definition of MAX_FDS and
MAX_VHOST_SERVER to FD_SETSIZE or add some build time check.
> - return;
> + if (pfdset == NULL || idx >= MAX_FDS || fd >= FD_SETSIZE)
> + return -1;
>
> pfdentry = &pfdset->fd[idx];
> pfdentry->fd = fd;
> pfdentry->rcb = rcb;
> pfdentry->wcb = wcb;
> pfdentry->dat = dat;
> +
> + return 0;
> }
>
> /**
> @@ -150,12 +152,11 @@ fdset_add(struct fdset *pfdset, int fd, fd_cb rcb, fd_cb wcb, void *dat)
>
> /* Find a free slot in the list. */
> i = fdset_find_free_slot(pfdset);
> - if (i == -1) {
> + if (i == -1 || fdset_add_fd(pfdset, i, fd, rcb, wcb, dat) < 0) {
> pthread_mutex_unlock(&pfdset->fd_mutex);
> return -2;
> }
>
> - fdset_add_fd(pfdset, i, fd, rcb, wcb, dat);
> pfdset->num++;
>
> pthread_mutex_unlock(&pfdset->fd_mutex);
> diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.c b/lib/librte_vhost/vhost_user/vhost-net-user.c
> index df2bd64..778851d 100644
> --- a/lib/librte_vhost/vhost_user/vhost-net-user.c
> +++ b/lib/librte_vhost/vhost_user/vhost-net-user.c
> @@ -288,6 +288,7 @@ vserver_new_vq_conn(int fd, void *dat, __rte_unused int *remove)
> int fh;
> struct vhost_device_ctx vdev_ctx = { (pid_t)0, 0 };
> unsigned int size;
> + int ret;
>
> conn_fd = accept(fd, NULL, NULL);
> RTE_LOG(INFO, VHOST_CONFIG,
> @@ -317,8 +318,15 @@ vserver_new_vq_conn(int fd, void *dat, __rte_unused int *remove)
>
> ctx->vserver = vserver;
> ctx->fh = fh;
> - fdset_add(&g_vhost_server.fdset,
> + ret = fdset_add(&g_vhost_server.fdset,
> conn_fd, vserver_message_handler, NULL, ctx);
> + if (ret < 0) {
> + free(ctx);
> + close(conn_fd);
> + RTE_LOG(ERR, VHOST_CONFIG,
> + "failed to add fd %d into vhost server fdset\n",
> + conn_fd);
> + }
> }
>
> /* callback when there is message on the connfd */
> @@ -453,6 +461,7 @@ int
> rte_vhost_driver_register(const char *path)
> {
> struct vhost_server *vserver;
> + int ret;
>
> pthread_mutex_lock(&g_vhost_server.server_mutex);
>
> @@ -478,8 +487,17 @@ rte_vhost_driver_register(const char *path)
>
> vserver->path = strdup(path);
>
> - fdset_add(&g_vhost_server.fdset, vserver->listenfd,
> + ret = fdset_add(&g_vhost_server.fdset, vserver->listenfd,
> vserver_new_vq_conn, NULL, vserver);
> + if (ret < 0) {
> + pthread_mutex_unlock(&g_vhost_server.server_mutex);
> + RTE_LOG(ERR, VHOST_CONFIG,
> + "failed to add listen fd %d to vhost server fdset\n",
> + vserver->listenfd);
> + free(vserver->path);
> + free(vserver);
> + return -1;
> + }
>
> g_vhost_server.server[g_vhost_server.vserver_cnt++] = vserver;
> pthread_mutex_unlock(&g_vhost_server.server_mutex);
next prev parent reply other threads:[~2016-03-30 9:05 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-18 9:13 Patrik Andersson
2016-03-30 9:05 ` Xie, Huawei [this message]
2016-04-05 8:40 ` Patrik Andersson R
2016-04-07 14:49 ` Christian Ehrhardt
2016-04-08 6:47 ` Xie, Huawei
2016-04-11 6:06 ` Patrik Andersson R
2016-04-11 9:34 ` Christian Ehrhardt
2016-04-11 11:21 ` Patrik Andersson R
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=C37D651A908B024F974696C65296B57B4C69D6C7@SHSMSX101.ccr.corp.intel.com \
--to=huawei.xie@intel.com \
--cc=dev@dpdk.org \
--cc=konstantin.ananyev@intel.com \
--cc=patrik.r.andersson@ericsson.com \
--cc=thomas.monjalon@6wind.com \
--cc=yuanhan.liu@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).