[dpdk-dev,v2,1/4] net/mlx: add debug checks to glue structure

Message ID 20180202164050.13017-2-adrien.mazarguil@6wind.com
State Accepted, archived
Delegated to: Ferruh Yigit
Headers show

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Adrien Mazarguil Feb. 2, 2018, 4:46 p.m.
This code should catch mistakes early if a glue structure member is added
without a corresponding implementation in the library.

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
 drivers/net/mlx4/mlx4.c | 9 +++++++++
 drivers/net/mlx5/mlx5.c | 9 +++++++++
 2 files changed, 18 insertions(+)

Comments

Marcelo Ricardo Leitner Feb. 5, 2018, 12:27 p.m. | #1
On Fri, Feb 02, 2018 at 05:46:12PM +0100, Adrien Mazarguil wrote:
> This code should catch mistakes early if a glue structure member is added
> without a corresponding implementation in the library.
> 
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> ---
>  drivers/net/mlx4/mlx4.c | 9 +++++++++
>  drivers/net/mlx5/mlx5.c | 9 +++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
> index 50a55ee52..201d39b6e 100644
> --- a/drivers/net/mlx4/mlx4.c
> +++ b/drivers/net/mlx4/mlx4.c
> @@ -799,6 +799,15 @@ rte_mlx4_pmd_init(void)
>  		return;
>  	assert(mlx4_glue);
>  #endif
> +#ifndef NDEBUG
> +	/* Glue structure must not contain any NULL pointers. */
> +	{
> +		unsigned int i;
> +
> +		for (i = 0; i != sizeof(*mlx4_glue) / sizeof(void *); ++i)
> +			assert(((const void *const *)mlx4_glue)[i]);
> +	}

This code will not catch the case on which mlx4_glue on PMD is smaller
than the one on the glue lib. Although this would be safe, as the PMD
won't place calls for functions that it is not aware of, I guess we
don't want to allow such situation anyhow.

One way to handle this is to add a size field and let the PMD check if
the size is the same. As this code is walking through it as an array
of pointers, an union around it to keep the alignment may be welcomed.

Also, this code would do read beyond buffer in the opposite case, when
mlx4_glue for the PMD is larger than the one on the glue lib.

> +#endif
>  	mlx4_glue->fork_init();
>  	rte_pci_register(&mlx4_driver);
>  }
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
> index 544599b01..050cfac0d 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -1142,6 +1142,15 @@ rte_mlx5_pmd_init(void)
>  		return;
>  	assert(mlx5_glue);
>  #endif
> +#ifndef NDEBUG
> +	/* Glue structure must not contain any NULL pointers. */
> +	{
> +		unsigned int i;
> +
> +		for (i = 0; i != sizeof(*mlx5_glue) / sizeof(void *); ++i)
> +			assert(((const void *const *)mlx5_glue)[i]);
> +	}
> +#endif
>  	mlx5_glue->fork_init();
>  	rte_pci_register(&mlx5_driver);
>  }
> -- 
> 2.11.0
>
Adrien Mazarguil Feb. 5, 2018, 1:31 p.m. | #2
On Mon, Feb 05, 2018 at 10:27:10AM -0200, Marcelo Ricardo Leitner wrote:
> On Fri, Feb 02, 2018 at 05:46:12PM +0100, Adrien Mazarguil wrote:
> > This code should catch mistakes early if a glue structure member is added
> > without a corresponding implementation in the library.
> > 
> > Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > ---
> >  drivers/net/mlx4/mlx4.c | 9 +++++++++
> >  drivers/net/mlx5/mlx5.c | 9 +++++++++
> >  2 files changed, 18 insertions(+)
> > 
> > diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
> > index 50a55ee52..201d39b6e 100644
> > --- a/drivers/net/mlx4/mlx4.c
> > +++ b/drivers/net/mlx4/mlx4.c
> > @@ -799,6 +799,15 @@ rte_mlx4_pmd_init(void)
> >  		return;
> >  	assert(mlx4_glue);
> >  #endif
> > +#ifndef NDEBUG
> > +	/* Glue structure must not contain any NULL pointers. */
> > +	{
> > +		unsigned int i;
> > +
> > +		for (i = 0; i != sizeof(*mlx4_glue) / sizeof(void *); ++i)
> > +			assert(((const void *const *)mlx4_glue)[i]);
> > +	}
> 
> This code will not catch the case on which mlx4_glue on PMD is smaller
> than the one on the glue lib. Although this would be safe, as the PMD
> won't place calls for functions that it is not aware of, I guess we
> don't want to allow such situation anyhow.
> 
> One way to handle this is to add a size field and let the PMD check if
> the size is the same. As this code is walking through it as an array
> of pointers, an union around it to keep the alignment may be welcomed.
> 
> Also, this code would do read beyond buffer in the opposite case, when
> mlx4_glue for the PMD is larger than the one on the glue lib.

While I generally agree, this block is pure debugging code not compiled in
by default and meant for PMD developers, not end users. It happened to me
while moving all these calls to the glue structure where I missed a couple
of functions and is a way to ensure such mistakes would be caught early on.

The version check that comes afterward is actually is the safe check you're
thinking about. Based on that, the PMD can be confident the symbol in
question has the expected properties. No need to check for NULLs.

If you think this code doesn't have its place in a PMD, I can remove it; the
version check should be enough. In my opinion it's better to keep it for
safety though, please confirm.

Patch

diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
index 50a55ee52..201d39b6e 100644
--- a/drivers/net/mlx4/mlx4.c
+++ b/drivers/net/mlx4/mlx4.c
@@ -799,6 +799,15 @@  rte_mlx4_pmd_init(void)
 		return;
 	assert(mlx4_glue);
 #endif
+#ifndef NDEBUG
+	/* Glue structure must not contain any NULL pointers. */
+	{
+		unsigned int i;
+
+		for (i = 0; i != sizeof(*mlx4_glue) / sizeof(void *); ++i)
+			assert(((const void *const *)mlx4_glue)[i]);
+	}
+#endif
 	mlx4_glue->fork_init();
 	rte_pci_register(&mlx4_driver);
 }
diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index 544599b01..050cfac0d 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -1142,6 +1142,15 @@  rte_mlx5_pmd_init(void)
 		return;
 	assert(mlx5_glue);
 #endif
+#ifndef NDEBUG
+	/* Glue structure must not contain any NULL pointers. */
+	{
+		unsigned int i;
+
+		for (i = 0; i != sizeof(*mlx5_glue) / sizeof(void *); ++i)
+			assert(((const void *const *)mlx5_glue)[i]);
+	}
+#endif
 	mlx5_glue->fork_init();
 	rte_pci_register(&mlx5_driver);
 }