[ovs-dev] [PATCH] netdev-dpdk: Fix mempool segfault.

[Kavanagh, Mark B]

>> >
>> >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 <ian.stokes at intel.com>
>> >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 <mark.b.kavanagh at intel.com>

>>
>> A few minor comments below also.
>>
>> Cheers,
>> Mark
>>
>> >CC: Daniele Di Proietto <diproiettod at vmware.com>
>> >CC: Mark Kavanagh <mark.b.kavanagh at intel.com>
>> >---
>> > 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