[dpdk-dev] thoughts on DPDK after a few days of reading sources
Hi Alejandro, thanks for the heads up - to make sure I got your correctly I assume you refer to: [1]: dpdk: http://dpdk.org/browse/dpdk/commit/?id=e61512e4066740847ced4a85ee9c3334b511468f [2]: kernel: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=033291eccbdb [3]: kernel: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=ae5515d66362b9d96cdcfce504567f0b8b7bd83e [4]: kenrel: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=03a76b60f8ba27974e2d252bc555d2c103420e15 It seems we will be shipping DPDK 2.2 and Kernel 4.4 with the next release (no commitment being made). [1] is not in DPDK 2.2, so it won't be supported right away. If it can be used without [1] I wouldn't have realized that yet. But also in the kernel it is not only disabled by default - in fact the code itself got removed [3] and only later back into 4.5 [4]. I guess, to really activate, exploit, test and eventually support it - it would be up to an explicit request to do so which can then be evaluated against the risks. IMHO I think we have to wait and see how it will be workin in DPDK >=16.x and Kernel >=4.5 this kind of referring to [3] "... so rather than support an unproven kernel interface revert it and revisit ...". Thanks for the heads up Alejandro, I didn't really think much about it before - I'll revisit it every now and then to see how development around it goes on. Christian Ehrhardt Software Engineer, Ubuntu Server Canonical Ltd On Thu, Feb 11, 2016 at 12:58 PM, Alejandro Lucero < alejandro.lucero at netronome.com> wrote: > Hi Seth, > > I do not know if you and Ubuntu know about the kernel VFIO no-iommu mode > which DPDK will use in the future (then getting rid of UIO drives). > > This implies distributions enabling that kernel VFIO mode which is not > enable by default as it is a security issue. > > It would be good to know which is the Ubuntu position regarding this issue > and if there are any date or plan for supporting this. > > Thanks > > On Thu, Feb 11, 2016 at 7:58 AM, Thomas Monjalon < > thomas.monjalon at 6wind.com> > wrote: > > > Hi, > > > > 2016-02-10 19:05, Seth Arnold: > > > I've taken some notes while reading the sources; I'm sharing them in > the > > > hopes that it's useful: on the one hand my fresh eyes may spot things > > that > > > you've overlooked, on the other hand your familiarity with the code > means > > > that you're better suited to judge what I've found. > > > > Thanks for taking time and sharing, it's very valuable. > > > > > - shellcheck reports extensive cases of forgotten quotes to prevent > word > > > splitting or globbing, potentially unused variables, error-prone > printf > > > formatting. The scripts that are going to be used at runtime should > be > > > fixed: > > > - ./debian/dpdk-init > > > - ./debian/dpdk.init > > > > These files are not in the tree. Should they? > > > > > - ./drivers/net/cxgbe/cxgbe_ethdev.c eth_cxgbe_dev_init() memory leak > in > > > out_free_adapter: that doesn't free adapter > > > - ./drivers/net/virtio/virtio_ethdev.c virtio_set_multiple_queues() > calls > > > virtio_send_command(), which performs: > > > memcpy(vq->virtio_net_hdr_mz->addr, ctrl, sizeof(struct > > virtio_pmd_ctrl)); > > > This copies a potentially huge amount of uninitialized data into > ->addr > > > because the struct virtio_pmd_ctrl ctrl was not zeroed before being > > > passed. How much of this data leaves the system? Does this require a > > > CVE? > > > > We are not used to open a CVE. > > > > [...] > > > It's nearly impossible to solve issues without error reporting. Good > > > error reporting saves admins time and money. > > > > Until now, the errors were reported on the list and most often fixed > > quickly. > > While I agree we need a more formal process (a bug tracker), I think we > > must > > be noticed of new bugs on the mailing list. > > Since nobody was against the bugzilla proposal, a deployment will be > > planned. > > http://dpdk.org/ml/archives/dev/2015-August/023012.html > > >
[dpdk-dev] thoughts on DPDK after a few days of reading sources
On Thu, Feb 11, 2016 at 8:58 AM, Thomas Monjalon wrote: > Hi, > > 2016-02-10 19:05, Seth Arnold: > > I've taken some notes while reading the sources; I'm sharing them in the > > hopes that it's useful: on the one hand my fresh eyes may spot things > that > > you've overlooked, on the other hand your familiarity with the code means > > that you're better suited to judge what I've found. > > Thanks for taking time and sharing, it's very valuable. > > > - shellcheck reports extensive cases of forgotten quotes to prevent word > > splitting or globbing, potentially unused variables, error-prone printf > > formatting. The scripts that are going to be used at runtime should be > > fixed: > > - ./debian/dpdk-init > > - ./debian/dpdk.init > > These files are not in the tree. Should they? > > You are right, they are not in tree. These few are parts of the ubuntu packaging and will be fixed by me this week. Sooner or later that might (partially?) flow back to the Debian packaging that is in tree. Christian Ehrhardt Software Engineer, Ubuntu Server Canonical Ltd [...]
[dpdk-dev] thoughts on DPDK after a few days of reading sources
Hi Seth, I do not know if you and Ubuntu know about the kernel VFIO no-iommu mode which DPDK will use in the future (then getting rid of UIO drives). This implies distributions enabling that kernel VFIO mode which is not enable by default as it is a security issue. It would be good to know which is the Ubuntu position regarding this issue and if there are any date or plan for supporting this. Thanks On Thu, Feb 11, 2016 at 7:58 AM, Thomas Monjalon wrote: > Hi, > > 2016-02-10 19:05, Seth Arnold: > > I've taken some notes while reading the sources; I'm sharing them in the > > hopes that it's useful: on the one hand my fresh eyes may spot things > that > > you've overlooked, on the other hand your familiarity with the code means > > that you're better suited to judge what I've found. > > Thanks for taking time and sharing, it's very valuable. > > > - shellcheck reports extensive cases of forgotten quotes to prevent word > > splitting or globbing, potentially unused variables, error-prone printf > > formatting. The scripts that are going to be used at runtime should be > > fixed: > > - ./debian/dpdk-init > > - ./debian/dpdk.init > > These files are not in the tree. Should they? > > > - ./drivers/net/cxgbe/cxgbe_ethdev.c eth_cxgbe_dev_init() memory leak in > > out_free_adapter: that doesn't free adapter > > - ./drivers/net/virtio/virtio_ethdev.c virtio_set_multiple_queues() calls > > virtio_send_command(), which performs: > > memcpy(vq->virtio_net_hdr_mz->addr, ctrl, sizeof(struct > virtio_pmd_ctrl)); > > This copies a potentially huge amount of uninitialized data into ->addr > > because the struct virtio_pmd_ctrl ctrl was not zeroed before being > > passed. How much of this data leaves the system? Does this require a > > CVE? > > We are not used to open a CVE. > > [...] > > It's nearly impossible to solve issues without error reporting. Good > > error reporting saves admins time and money. > > Until now, the errors were reported on the list and most often fixed > quickly. > While I agree we need a more formal process (a bug tracker), I think we > must > be noticed of new bugs on the mailing list. > Since nobody was against the bugzilla proposal, a deployment will be > planned. > http://dpdk.org/ml/archives/dev/2015-August/023012.html >
[dpdk-dev] thoughts on DPDK after a few days of reading sources
Hi, On 02/11/2016 02:58 AM, Thomas Monjalon wrote: > 2016-02-10 19:05, Seth Arnold: > [...] >> It's nearly impossible to solve issues without error reporting. Good >> error reporting saves admins time and money. > > Until now, the errors were reported on the list and most often fixed quickly. > While I agree we need a more formal process (a bug tracker), I think we must > be noticed of new bugs on the mailing list. > Since nobody was against the bugzilla proposal, a deployment will be planned. > http://dpdk.org/ml/archives/dev/2015-August/023012.html I may have misunderstood Seth's comment, but it looked like he was talking about checking errno after fopen and reporting the error with perror or strerror in the event of a non-zero return. Seth, did I understand correctly? Thanks, Dave. -- Dave Neary - NFV/SDN Community Strategy Open Source and Standards, Red Hat - http://community.redhat.com Ph: +1-978-399-2182 / Cell: +1-978-799-3338
[dpdk-dev] thoughts on DPDK after a few days of reading sources
Hi, 2016-02-10 19:05, Seth Arnold: > I've taken some notes while reading the sources; I'm sharing them in the > hopes that it's useful: on the one hand my fresh eyes may spot things that > you've overlooked, on the other hand your familiarity with the code means > that you're better suited to judge what I've found. Thanks for taking time and sharing, it's very valuable. > - shellcheck reports extensive cases of forgotten quotes to prevent word > splitting or globbing, potentially unused variables, error-prone printf > formatting. The scripts that are going to be used at runtime should be > fixed: > - ./debian/dpdk-init > - ./debian/dpdk.init These files are not in the tree. Should they? > - ./drivers/net/cxgbe/cxgbe_ethdev.c eth_cxgbe_dev_init() memory leak in > out_free_adapter: that doesn't free adapter > - ./drivers/net/virtio/virtio_ethdev.c virtio_set_multiple_queues() calls > virtio_send_command(), which performs: > memcpy(vq->virtio_net_hdr_mz->addr, ctrl, sizeof(struct virtio_pmd_ctrl)); > This copies a potentially huge amount of uninitialized data into ->addr > because the struct virtio_pmd_ctrl ctrl was not zeroed before being > passed. How much of this data leaves the system? Does this require a > CVE? We are not used to open a CVE. [...] > It's nearly impossible to solve issues without error reporting. Good > error reporting saves admins time and money. Until now, the errors were reported on the list and most often fixed quickly. While I agree we need a more formal process (a bug tracker), I think we must be noticed of new bugs on the mailing list. Since nobody was against the bugzilla proposal, a deployment will be planned. http://dpdk.org/ml/archives/dev/2015-August/023012.html
[dpdk-dev] thoughts on DPDK after a few days of reading sources
On Wed, Feb 10, 2016 at 07:05:40PM -0800, Seth Arnold wrote: > - ixgbe driver in the package is very different from the driver in the > Linux kernel -- when bugs in one are found, who is in charge of copying > the fixes between the two code bases? It's not the Linux driver. It's from BSD because DPDK uses mbufs. The MAINTAINERS file / Intel team refreshes it w/ shared code from their upstream BSD support. Matthew.
[dpdk-dev] thoughts on DPDK after a few days of reading sources
Hello, The Ubuntu distribution is looking at supporting DPDK in the 'main' component of the archive. As part of this process I spent a few days reading the DPDK sources to gauge if we can support it or not. I've taken some notes while reading the sources; I'm sharing them in the hopes that it's useful: on the one hand my fresh eyes may spot things that you've overlooked, on the other hand your familiarity with the code means that you're better suited to judge what I've found. Most of the code was very good; I am however concerned about the frequent memory allocations that use simple integer arithmetic when deciding how much to allocate without checking for integer overflows. Here's the bug tracking the Main Inclusion Request: https://bugs.launchpad.net/ubuntu/+source/dpdk/+bug/1492186 Here's the portions of the notes that I think may be most interesting to DPDK devs. It's in rough priority order so feel free to stop reading when you're bored: === Affects main codebase and should be investigated quickly: - shellcheck reports extensive cases of forgotten quotes to prevent word splitting or globbing, potentially unused variables, error-prone printf formatting. The scripts that are going to be used at runtime should be fixed: - ./debian/dpdk-init - ./debian/dpdk.init - ./tools/setup.sh ? (Hard to tell) - ./drivers/net/cxgbe/cxgbe_ethdev.c eth_cxgbe_dev_init() memory leak in out_free_adapter: that doesn't free adapter - ./drivers/net/virtio/virtio_ethdev.c virtio_set_multiple_queues() calls virtio_send_command(), which performs: memcpy(vq->virtio_net_hdr_mz->addr, ctrl, sizeof(struct virtio_pmd_ctrl)); This copies a potentially huge amount of uninitialized data into ->addr because the struct virtio_pmd_ctrl ctrl was not zeroed before being passed. How much of this data leaves the system? Does this require a CVE? - ./lib/librte_eal/common/rte_malloc.c memory allocation routines don't check for integer overflow errors: - rte_calloc_socket() - rte_calloc() - ./lib/librte_vhost/vhost_user/virtio-net-user.c user_set_mem_table() doesn't perform integer overflow checks before calling calloc() - ./lib/librte_vhost/vhost_cuse/virtio-net-cdev.c cuse_set_mem_table() doesn't perform integer overflow checks before calling calloc() - ./lib/librte_eal/linuxapp/eal/eal_xen_memory.c rte_xen_dom0_memory_init() If vma_addr = xen_get_virtual_area(_len, RTE_PGSIZE_2M); fails, vma_len is reset to RTE_PGSIZE_2M instead of seginfo[memseg_idx].size -- is this a bug? - ./lib/librte_eal/linuxapp/eal/eal_memory.c create_shared_memory() creates the hugetlb file with mode 0666 rather than 0600 -- is this a bug? Does this require a CVE? - ./lib/librte_eal/common/include/arch/ppc_64/rte_cpuflags.h rte_cpu_get_features() leaks auxv_fd - ./lib/librte_eal/common/include/arch/arm/rte_cpuflags_32.h rte_cpu_get_features() leaks auxv_fd - ./lib/librte_eal/common/include/arch/arm/rte_cpuflags_64.h rte_cpu_get_features() leaks auxv_fd Affects main codebase: - Assorted false-positives and style issues reported by cppcheck - Slightly dangerous convention of memcpy(dest, source, sizeof(source)) is used extensively; while all the instances I investigated were correct, it's still more prone to mistakes under maintenance than memcpy(dest, source, sizeof(dest)). - extensive use of *malloc() wrappers that perform multiplication to determine the size to allocate; all the cases I've seen used values that should be constrained by system configurations but the habit is dangerous compared to use of *calloc() wrappers that handle integer overflow safely. - ./app/test/test_malloc.c test_reordered_free_per_lcore() has incorrect calls to is_memory_overlap() -- p2 is 16000 bytes long, not 1000 bytes long, and this size difference is not reflected in the calls. - ./app/test/test_malloc.c test_realloc() leaks ptr1, may leak ptr9 via error path - ./app/test/test_malloc.c test_realloc() the test with error "Unexpected - ptr4 != ptr3" doesn't feel like it tests an actual promise from the API - ./lib/librte_pipeline/rte_pipeline.c rte_pipeline_table_create(), rte_pipeline_port_in_create(), rte_pipeline_port_out_create(), duplicate the array of function pointers via memcpy() rather than copying a pointer to static tables -- this may represent an easy way to save memory and improve cache hit ratios as well as potentially allow storing the tables in static memory rather than on the heap, reducing the value of these structs in potential exploits. - ixgbe driver in the package is very different from the driver in the Linux kernel -- when bugs in one are found, who is in charge of copying the fixes between the two code bases? - ./lib/librte_eal/linuxapp/kni/ethtool/ixgbe/ixgbe_ethtool.c ixgbe_get_strings() takes a buffer to write into but not the buffer length; sprintf() calls may overflow the buffer if it isn't large enough. It looks like ethtool_get_strings() may use
