* [dpdk-dev] [PATCH] net/virtio-user: fix overflow @ 2017-03-13 15:09 Wenfeng Liu 2017-03-14 6:44 ` Yuanhan Liu 0 siblings, 1 reply; 4+ messages in thread From: Wenfeng Liu @ 2017-03-13 15:09 UTC (permalink / raw) To: yuanhan.liu, maxime.coquelin; +Cc: dev This commit fixes an array overflow when number of queues is higher than 8. Fixes: 37a7eb2ae816 ("net/virtio-user: add device emulation layer") Signed-off-by: Wenfeng Liu <liuwf@arraynetworks.com.cn> --- 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) { 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]; 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; + } + 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [dpdk-dev] [PATCH] net/virtio-user: fix overflow 2017-03-13 15:09 [dpdk-dev] [PATCH] net/virtio-user: fix overflow Wenfeng Liu @ 2017-03-14 6:44 ` Yuanhan Liu 2017-03-14 8:20 ` [dpdk-dev] 答复: " Wenfeng Liu 0 siblings, 1 reply; 4+ messages in thread From: Yuanhan Liu @ 2017-03-14 6:44 UTC (permalink / raw) To: Wenfeng Liu; +Cc: maxime.coquelin, dev, Tan, Jianfeng 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. > > Fixes: 37a7eb2ae816 ("net/virtio-user: add device emulation layer") > > Signed-off-by: Wenfeng Liu <liuwf@arraynetworks.com.cn> > --- > 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. > 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [dpdk-dev] 答复: [PATCH] net/virtio-user: fix overflow 2017-03-14 6:44 ` Yuanhan Liu @ 2017-03-14 8:20 ` Wenfeng Liu 2017-03-14 8:28 ` Yuanhan Liu 0 siblings, 1 reply; 4+ messages in thread From: Wenfeng Liu @ 2017-03-14 8:20 UTC (permalink / raw) To: 'Yuanhan Liu'; +Cc: maxime.coquelin, dev, 'Tan, Jianfeng' 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 <liuwf@arraynetworks.com.cn> >> --- >> 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [dpdk-dev] 答复: [PATCH] net/virtio-user: fix overflow 2017-03-14 8:20 ` [dpdk-dev] 答复: " Wenfeng Liu @ 2017-03-14 8:28 ` Yuanhan Liu 0 siblings, 0 replies; 4+ messages in thread From: Yuanhan Liu @ 2017-03-14 8:28 UTC (permalink / raw) To: Wenfeng Liu; +Cc: maxime.coquelin, dev, 'Tan, Jianfeng' On Tue, Mar 14, 2017 at 04:20:39PM +0800, Wenfeng Liu wrote: > 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 <liuwf@arraynetworks.com.cn> > >> --- > >> 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: Oh, right, sorry, I overlooked it. Then for this patch, it's okay to me. > #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. Yes, please. Also, please put a "Cc: stable@dpdk.org" before your Signed-of-by: it's a candidate for stable releases. --yliu ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-03-14 8:30 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-13 15:09 [dpdk-dev] [PATCH] net/virtio-user: fix overflow Wenfeng Liu 2017-03-14 6:44 ` Yuanhan Liu 2017-03-14 8:20 ` [dpdk-dev] 答复: " Wenfeng Liu 2017-03-14 8:28 ` Yuanhan Liu
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).