>> > >> >The dpdk_mp_get() function can return a NULL pointer which leads to a >> >segfault when a mempool cannot be created. The lack of a return value >> >check for the function netdev_dpdk_mempool_configure() when called in >> >netdev_dpdk_reconfigure() can result in a segfault also as a NULL >> >pointer for the mempool will be passed to rte_eth_rx_queue_setup(). >> > >> >Fix this by adding appropriate NULL pointer and return value checks to >> >dpdk_mp_get() and netdev_dpdk_reconfigure(), also flag when a memory >> >configuration error occurs in dpdk_vhost_reconfigure_helper(). >> > >> >Signed-off-by: Ian Stokes <[email protected]> >> >Fixes: 2ae3d542 ("netdev-dpdk: Refactor dpdk_mp_get().") >> >Fixes: 0072e931 ("netdev-dpdk: add support for jumbo frames") >> >> Hi Ian - this patch doesn't fix 0072e931, as all of the error checking >> that you've provided here was already in that commit. >> Could you remove this line please? > >Thanks for the feedback Mark, > >The commit 0072e931 introduced the call to netdev_dpdk_mempool_configure() in >netdev_dpdk_reconfigure(), however it's missing a check for the return value >(You can see >other examples of the return value being checked throughout that commit). > >Without this check there is the possibility that a null pointer for the >mempool will be >accessed during the creation of rxqs which happens later in the >netdev_dpdk_reconfigure() >function. > >As such I think its ok to keep the fixes tags above, thoughts?
Sounds good Ian - thanks for the catch! Acked-by: Mark Kavanagh <[email protected]> >> >> A few minor comments below also. >> >> Cheers, >> Mark >> >> >CC: Daniele Di Proietto <[email protected]> >> >CC: Mark Kavanagh <[email protected]> >> >--- >> > lib/netdev-dpdk.c | 18 ++++++++++++++---- >> > 1 files changed, 14 insertions(+), 4 deletions(-) >> > >> >diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index >> >ee53c4c..62f4409 100644 >> >--- a/lib/netdev-dpdk.c >> >+++ b/lib/netdev-dpdk.c >> >@@ -529,8 +529,9 @@ dpdk_mp_get(int socket_id, int mtu) >> > } >> > } >> > >> >- dmp = dpdk_mp_create(socket_id, mtu); >> >- ovs_list_push_back(&dpdk_mp_list, &dmp->list_node); >> >+ if ((dmp = dpdk_mp_create(socket_id, mtu))) { >> >+ ovs_list_push_back(&dpdk_mp_list, &dmp->list_node); >> >+ } >> > >> > out: >> > ovs_mutex_unlock(&dpdk_mp_mutex); >> >@@ -3131,7 +3132,11 @@ netdev_dpdk_reconfigure(struct netdev *netdev) >> > >> > if (dev->mtu != dev->requested_mtu >> > || dev->socket_id != dev->requested_socket_id) { >> >- netdev_dpdk_mempool_configure(dev); >> >+ if ((err = netdev_dpdk_mempool_configure(dev))) { >> >+ VLOG_ERR("Mempool configuration failed for device %s: %s", >> >> This log probably isn't needed, as netdev_dpdk_mempool_configure already >> flags the same thing on error. >> In any event, missing a newline char in the error string. >Sure thing, I can remove in the V2. >> >> >+ dev->up.name, rte_strerror(err)); >> >+ goto out; >> >+ } >> > } >> > >> > netdev->n_txq = dev->requested_n_txq; @@ -3160,6 +3165,7 @@ >> >dpdk_vhost_reconfigure_helper(struct netdev_dpdk *dev) { >> > dev->up.n_txq = dev->requested_n_txq; >> > dev->up.n_rxq = dev->requested_n_rxq; >> >+ int err; >> > >> > /* Enable TX queue 0 by default if it wasn't disabled. */ >> > if (dev->tx_q[0].map == OVS_VHOST_QUEUE_MAP_UNKNOWN) { @@ -3170,9 >> >+3176,13 @@ dpdk_vhost_reconfigure_helper(struct netdev_dpdk *dev) >> > >> > if (dev->requested_socket_id != dev->socket_id >> > || dev->requested_mtu != dev->mtu) { >> >- if (!netdev_dpdk_mempool_configure(dev)) { >> >+ if (!(err = netdev_dpdk_mempool_configure(dev))) { >> > netdev_change_seq_changed(&dev->up); >> > } >> >+ else { >> >+ VLOG_ERR("Mempool configuration failed for device %s: %s", >> >+ dev->up.name, rte_strerror(err)); >> >> Ditto >Will remove for the v2. >> >> >+ } >> > } >> > >> > if (!dev->dpdk_mp) { >> >-- >> >1.7.0.7 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
