From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail01.arraynetworks.com.cn (mail.arraynetworks.com.cn [124.42.99.121]) by dpdk.org (Postfix) with ESMTP id AE9F96A6E for ; Tue, 14 Mar 2017 09:22:10 +0100 (CET) Received: from liuwfthinkpad (10.6.0.205) by mail01.arraynetworks.com.cn (10.3.0.251) with Microsoft SMTP Server id 14.3.123.3; Tue, 14 Mar 2017 16:18:01 +0800 From: Wenfeng Liu To: 'Yuanhan Liu' CC: , , "'Tan, Jianfeng'" References: <1489417755-17074-1-git-send-email-liuwf@arraynetworks.com.cn> <20170314064416.GI18844@yliu-dev.sh.intel.com> In-Reply-To: <20170314064416.GI18844@yliu-dev.sh.intel.com> Date: Tue, 14 Mar 2017 16:20:39 +0800 Message-ID: <005b01d29c9b$de9e83c0$9bdb8b40$@com.cn> MIME-Version: 1.0 Content-Type: text/plain; charset="gb2312" Content-Transfer-Encoding: 7bit X-Mailer: Microsoft Office Outlook 12.0 Thread-Index: AdKcjkhVJh99V0CvTu+MxvBVRnf8nwADHAkg Content-Language: zh-cn X-Originating-IP: [10.6.0.205] Subject: [dpdk-dev] =?gb2312?b?tPC4tDogW1BBVENIXSBuZXQvdmlydGlvLXVzZXI6?= =?gb2312?b?IGZpeCBvdmVyZmxvdw==?= 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: Tue, 14 Mar 2017 08:22:11 -0000 Hi Yuanhan, >On Mon, Mar 13, 2017 at 03:09:15PM +0000, Wenfeng Liu wrote: >> This commit fixes an array overflow when number of queues is higher than 8. > >Firstly, this commit log could be a bit more informative, to something >like: > > virtio-user limits the qeueue number to 8 but provides no limit > check against the queue number input from user. If a bigger queue > number (> 8) is given, there is an overflow issue. Doing a sanity > check could avoid it. > Sure, I will revise the commit log accordingly. >> >> Fixes: 37a7eb2ae816 ("net/virtio-user: add device emulation layer") >> >> Signed-off-by: Wenfeng Liu >> --- >> drivers/net/virtio/virtio_pci.h | 3 ++- >> drivers/net/virtio/virtio_user/virtio_user_dev.c | 2 +- >> drivers/net/virtio/virtio_user/virtio_user_dev.h | 6 +++--- >> drivers/net/virtio/virtio_user_ethdev.c | 7 +++++++ >> 4 files changed, 13 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/net/virtio/virtio_pci.h >> b/drivers/net/virtio/virtio_pci.h index 59e45c4..bd940b4 100644 >> --- a/drivers/net/virtio/virtio_pci.h >> +++ b/drivers/net/virtio/virtio_pci.h >> @@ -160,7 +160,8 @@ >> /* >> * Maximum number of virtqueues per device. >> */ >> -#define VIRTIO_MAX_VIRTQUEUES 8 >> +#define VIRTIO_MAX_VIRTQUEUE_PAIRS 8 >> +#define VIRTIO_MAX_VIRTQUEUES VIRTIO_MAX_VIRTQUEUE_PAIRS * 2 + 1 >> >> /* Common configuration */ >> #define VIRTIO_PCI_CAP_COMMON_CFG 1 >> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c >> b/drivers/net/virtio/virtio_user/virtio_user_dev.c >> index e7fd65f..5b81676 100644 >> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c >> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c >> @@ -234,7 +234,7 @@ int virtio_user_stop_device(struct virtio_user_dev *dev) >> uint32_t i, q; >> >> dev->vhostfd = -1; >> - for (i = 0; i < VIRTIO_MAX_VIRTQUEUES * 2 + 1; ++i) { >> + for (i = 0; i < VIRTIO_MAX_VIRTQUEUES; ++i) { > >Right, we don't need setup callfd and kickfd for the ctrl-queue. I did not remove the ctrl-queue. I just redefined the MACRO according to DRY principle: #define VIRTIO_MAX_VIRTQUEUES VIRTIO_MAX_VIRTQUEUE_PAIRS * 2 + 1 I noticed that I missed the bracket in the MACRO and will add it in next version. > >> dev->kickfds[i] = -1; >> dev->callfds[i] = -1; >> } >> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h >> b/drivers/net/virtio/virtio_user/virtio_user_dev.h >> index 6ecb91e..ba80d05 100644 >> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h >> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h >> @@ -49,8 +49,8 @@ struct virtio_user_dev { >> int *tapfds; >> >> /* for both vhost_user and vhost_kernel */ >> - int callfds[VIRTIO_MAX_VIRTQUEUES * 2 + 1]; >> - int kickfds[VIRTIO_MAX_VIRTQUEUES * 2 + 1]; >> + int callfds[VIRTIO_MAX_VIRTQUEUES]; >> + int kickfds[VIRTIO_MAX_VIRTQUEUES]; >> int mac_specified; >> uint32_t max_queue_pairs; >> uint32_t queue_pairs; >> @@ -62,7 +62,7 @@ struct virtio_user_dev { >> uint8_t status; >> uint8_t mac_addr[ETHER_ADDR_LEN]; >> char path[PATH_MAX]; >> - struct vring vrings[VIRTIO_MAX_VIRTQUEUES * 2 + 1]; >> + struct vring vrings[VIRTIO_MAX_VIRTQUEUES]; > >Have you actually tested your patch? You are removing the vring for ctrl-queue, which is wrong to me. > >> struct virtio_user_backend_ops *ops; }; >> >> diff --git a/drivers/net/virtio/virtio_user_ethdev.c >> b/drivers/net/virtio/virtio_user_ethdev.c >> index 16d1526..d476a2d 100644 >> --- a/drivers/net/virtio/virtio_user_ethdev.c >> +++ b/drivers/net/virtio/virtio_user_ethdev.c >> @@ -433,6 +433,13 @@ >> goto end; >> } >> >> + if (queues > VIRTIO_MAX_VIRTQUEUE_PAIRS) { >> + PMD_INIT_LOG(ERR, "arg %s %u exceeds the limit %u", >> + VIRTIO_USER_ARG_QUEUES_NUM, queues, >> + VIRTIO_MAX_VIRTQUEUE_PAIRS); >> + goto end; >> + } > >Yes, we need this check. So, to me, you were actually doing two things in this patch: > >- check the queue number, to avoid overflow >- remove the callfds and kickfds for ctrl-queue. > >That said, please do them in two patches. > >Thanks. > > --yliu >> + >> eth_dev = virtio_user_eth_dev_alloc(name); >> if (!eth_dev) { >> PMD_INIT_LOG(ERR, "virtio_user fails to alloc device"); >> -- >> 1.8.3.1 Regards, liuwf