[dpdk-dev] [PATCH v3] mempool: Add sanity check when secondary link-in less mempools than primary

[dpdk-dev] [PATCH v3] mempool: Add sanity check when secondary link-in less mempools than primary

[Jean Tourrilhes]

If the mempool ops the caller wants to use is not registered, the
library will segfault in an obscure way when trying to use that
mempool. It's better to catch it early and warn the user.

If the primary and secondary process were build using different build
systems, the list of constructors included by the linker in each
binary might be different. Mempools are registered via constructors, so
the linker magic will directly impact which mempools are registered with
the primary and the secondary.
DPDK currently assumes that the secondary has a superset of the
mempools registered at the primary, and they are in the same order
(same index in primary and secondary). In some build scenario, the
secondary might not initialise any mempools at all.

This would also catch cases where there is a bug in the mempool
registration, or some memory corruptions, but this has not been
observed.

Signed-off-by: Jean Tourrilhes <jt@labs.hpe.com>
---
 lib/librte_mempool/rte_mempool.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
index e94e56f..bbb6723 100644
--- a/lib/librte_mempool/rte_mempool.c
+++ b/lib/librte_mempool/rte_mempool.c
@@ -1273,6 +1273,29 @@ rte_mempool_lookup(const char *name)
 		return NULL;
 	}
 
+	/* Sanity check : secondary may have initialised less mempools
+	 * than primary due to linker and constructor magic. Or maybe
+	 * there is a mempool corruption or bug. In any case, we can't
+	 * go on, we will segfault in an obscure way.
+	 * This does not detect the cases where the constructor order
+	 * is different between primary and secondary or where the
+	 * index points to the wrong ops. This would require more
+	 * extensive changes, and is much less likely. Jean II */
+	if (mp->ops_index >= (int32_t) rte_mempool_ops_table.num_ops) {
+		unsigned i;
+		/* Dump list of mempool ops for further investigation.
+		 * In most cases, list is empty... */
+		for (i = 0; i < rte_mempool_ops_table.num_ops; i++) {
+			RTE_LOG(ERR, EAL, "Registered mempool[%d] is %s\n",
+				i, rte_mempool_ops_table.ops[i].name);
+		}
+		/* Do not dump mempool list itself, it will segfault. */
+		rte_panic("Cannot find ops for mempool, ops_index %d, "
+			  "num_ops %d - maybe due to build process or "
+			  "linker configuration\n",
+			  mp->ops_index, rte_mempool_ops_table.num_ops);
+	}
+
 	return mp;
 }
 

Re: [dpdk-dev] [PATCH v3] mempool: Add sanity check when secondary link-in less mempools than primary

[Ferruh Yigit]

On 11/10/2016 11:25 PM, jt at labs.hpe.com (Jean Tourrilhes) wrote:
> If the mempool ops the caller wants to use is not registered, the
> library will segfault in an obscure way when trying to use that
> mempool. It's better to catch it early and warn the user.
> 
> If the primary and secondary process were build using different build
> systems, the list of constructors included by the linker in each
> binary might be different. Mempools are registered via constructors, so
> the linker magic will directly impact which mempools are registered with
> the primary and the secondary.
> DPDK currently assumes that the secondary has a superset of the
> mempools registered at the primary, and they are in the same order
> (same index in primary and secondary). In some build scenario, the
> secondary might not initialise any mempools at all.
> 
> This would also catch cases where there is a bug in the mempool
> registration, or some memory corruptions, but this has not been
> observed.
> 
> Signed-off-by: Jean Tourrilhes <jt at labs.hpe.com>

Hi Jean,

There is no comment on the patch for a long time, more than two years, updating
patch status as rejected, if it is still relevant please let us know.

Sorry for any inconvenience caused.

For record, patch: https://patches.dpdk.org/patch/17000/

> ---
>  lib/librte_mempool/rte_mempool.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
> index e94e56f..bbb6723 100644
> --- a/lib/librte_mempool/rte_mempool.c
> +++ b/lib/librte_mempool/rte_mempool.c
> @@ -1273,6 +1273,29 @@ rte_mempool_lookup(const char *name)
>  		return NULL;
>  	}
>  
> +	/* Sanity check : secondary may have initialised less mempools
> +	 * than primary due to linker and constructor magic. Or maybe
> +	 * there is a mempool corruption or bug. In any case, we can't
> +	 * go on, we will segfault in an obscure way.
> +	 * This does not detect the cases where the constructor order
> +	 * is different between primary and secondary or where the
> +	 * index points to the wrong ops. This would require more
> +	 * extensive changes, and is much less likely. Jean II */
> +	if (mp->ops_index >= (int32_t) rte_mempool_ops_table.num_ops) {
> +		unsigned i;
> +		/* Dump list of mempool ops for further investigation.
> +		 * In most cases, list is empty... */
> +		for (i = 0; i < rte_mempool_ops_table.num_ops; i++) {
> +			RTE_LOG(ERR, EAL, "Registered mempool[%d] is %s\n",
> +				i, rte_mempool_ops_table.ops[i].name);
> +		}
> +		/* Do not dump mempool list itself, it will segfault. */
> +		rte_panic("Cannot find ops for mempool, ops_index %d, "
> +			  "num_ops %d - maybe due to build process or "
> +			  "linker configuration\n",
> +			  mp->ops_index, rte_mempool_ops_table.num_ops);
> +	}
> +
>  	return mp;
>  }
>  
>