* [dpdk-dev] [PATCH] mempool: enforce valid flags at creation
@ 2021-10-12 7:28 David Marchand
2021-10-12 7:49 ` Andrew Rybchenko
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: David Marchand @ 2021-10-12 7:28 UTC (permalink / raw)
To: dev; +Cc: mdr, dkozlyuk, thomas, Olivier Matz, Andrew Rybchenko
If we do not enforce valid flags are passed by an application, this
application might face issues in the future when we add more flags.
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
app/test/test_mempool.c | 21 +++++++++++++++++++++
lib/mempool/rte_mempool.c | 13 +++++++++++++
lib/mempool/rte_mempool.h | 2 +-
3 files changed, 35 insertions(+), 1 deletion(-)
diff --git a/app/test/test_mempool.c b/app/test/test_mempool.c
index 7675a3e605..66bc8d86b7 100644
--- a/app/test/test_mempool.c
+++ b/app/test/test_mempool.c
@@ -205,6 +205,24 @@ static int test_mempool_creation_with_exceeded_cache_size(void)
return 0;
}
+static int test_mempool_creation_with_unknown_flag(void)
+{
+ struct rte_mempool *mp_cov;
+
+ mp_cov = rte_mempool_create("test_mempool_unknown_flag", MEMPOOL_SIZE,
+ MEMPOOL_ELT_SIZE, 0, 0,
+ NULL, NULL,
+ NULL, NULL,
+ SOCKET_ID_ANY, MEMPOOL_F_NO_IOVA_CONTIG << 1);
+
+ if (mp_cov != NULL) {
+ rte_mempool_free(mp_cov);
+ RET_ERR();
+ }
+
+ return 0;
+}
+
static struct rte_mempool *mp_spsc;
static rte_spinlock_t scsp_spinlock;
static void *scsp_obj_table[MAX_KEEP];
@@ -635,6 +653,9 @@ test_mempool(void)
if (test_mempool_creation_with_exceeded_cache_size() < 0)
GOTO_ERR(ret, err);
+ if (test_mempool_creation_with_unknown_flag() < 0)
+ GOTO_ERR(ret, err);
+
if (test_mempool_same_name_twice_creation() < 0)
GOTO_ERR(ret, err);
diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
index c5f859ae71..a2a78125f4 100644
--- a/lib/mempool/rte_mempool.c
+++ b/lib/mempool/rte_mempool.c
@@ -777,6 +777,13 @@ rte_mempool_cache_free(struct rte_mempool_cache *cache)
rte_free(cache);
}
+#define MEMPOOL_KNOWN_FLAGS ( MEMPOOL_F_NO_SPREAD \
+ | MEMPOOL_F_NO_CACHE_ALIGN \
+ | MEMPOOL_F_SP_PUT \
+ | MEMPOOL_F_SC_GET \
+ | MEMPOOL_F_POOL_CREATED \
+ | MEMPOOL_F_NO_IOVA_CONTIG \
+ )
/* create an empty mempool */
struct rte_mempool *
rte_mempool_create_empty(const char *name, unsigned n, unsigned elt_size,
@@ -806,6 +813,12 @@ rte_mempool_create_empty(const char *name, unsigned n, unsigned elt_size,
RTE_CACHE_LINE_MASK) != 0);
#endif
+ /* enforce no unknown flag is passed by the application */
+ if ((flags & ~MEMPOOL_KNOWN_FLAGS) != 0) {
+ rte_errno = EINVAL;
+ return NULL;
+ }
+
mempool_list = RTE_TAILQ_CAST(rte_mempool_tailq.head, rte_mempool_list);
/* asked for zero items */
diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
index f57ecbd6fc..5143b16a24 100644
--- a/lib/mempool/rte_mempool.h
+++ b/lib/mempool/rte_mempool.h
@@ -996,7 +996,7 @@ typedef void (rte_mempool_ctor_t)(struct rte_mempool *, void *);
* with rte_errno set appropriately. Possible rte_errno values include:
* - E_RTE_NO_CONFIG - function could not get pointer to rte_config structure
* - E_RTE_SECONDARY - function was called from a secondary process instance
- * - EINVAL - cache size provided is too large
+ * - EINVAL - cache size provided is too large or an unknown flag was passed
* - ENOSPC - the maximum number of memzones has already been allocated
* - EEXIST - a memzone with the same name already exists
* - ENOMEM - no appropriate memory area found in which to create memzone
--
2.23.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] mempool: enforce valid flags at creation
2021-10-12 7:28 [dpdk-dev] [PATCH] mempool: enforce valid flags at creation David Marchand
@ 2021-10-12 7:49 ` Andrew Rybchenko
2021-10-12 7:57 ` David Marchand
2021-10-12 10:10 ` Kinsella, Ray
2021-10-14 19:29 ` [dpdk-dev] [PATCH v2] " David Marchand
2 siblings, 1 reply; 9+ messages in thread
From: Andrew Rybchenko @ 2021-10-12 7:49 UTC (permalink / raw)
To: David Marchand, dev; +Cc: mdr, dkozlyuk, thomas, Olivier Matz
On 10/12/21 10:28 AM, David Marchand wrote:
> If we do not enforce valid flags are passed by an application, this
> application might face issues in the future when we add more flags.
Thanks. I'd even consider it as a bug and the fix to be
backported.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
A nit below, other than that:
Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
[snip]
> diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
> index c5f859ae71..a2a78125f4 100644
> --- a/lib/mempool/rte_mempool.c
> +++ b/lib/mempool/rte_mempool.c
> @@ -777,6 +777,13 @@ rte_mempool_cache_free(struct rte_mempool_cache *cache)
> rte_free(cache);
> }
>
> +#define MEMPOOL_KNOWN_FLAGS ( MEMPOOL_F_NO_SPREAD \
> + | MEMPOOL_F_NO_CACHE_ALIGN \
> + | MEMPOOL_F_SP_PUT \
> + | MEMPOOL_F_SC_GET \
> + | MEMPOOL_F_POOL_CREATED \
> + | MEMPOOL_F_NO_IOVA_CONTIG \
> + )
> /* create an empty mempool */
> struct rte_mempool *
> rte_mempool_create_empty(const char *name, unsigned n, unsigned elt_size,
> @@ -806,6 +813,12 @@ rte_mempool_create_empty(const char *name, unsigned n, unsigned elt_size,
> RTE_CACHE_LINE_MASK) != 0);
> #endif
>
> + /* enforce no unknown flag is passed by the application */
> + if ((flags & ~MEMPOOL_KNOWN_FLAGS) != 0) {
> + rte_errno = EINVAL;
> + return NULL;
> + }
> +
I think it is better to check arguments in parameters order.
So, it is a bit better to move the check to happen after
cache_size validation below.
> mempool_list = RTE_TAILQ_CAST(rte_mempool_tailq.head, rte_mempool_list);
>
> /* asked for zero items */
[snip]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] mempool: enforce valid flags at creation
2021-10-12 7:49 ` Andrew Rybchenko
@ 2021-10-12 7:57 ` David Marchand
2021-10-14 19:16 ` David Marchand
0 siblings, 1 reply; 9+ messages in thread
From: David Marchand @ 2021-10-12 7:57 UTC (permalink / raw)
To: Andrew Rybchenko
Cc: dev, Ray Kinsella, dkozlyuk, Thomas Monjalon, Olivier Matz,
Kevin Traynor, Luca Boccassi, Christian Ehrhardt,
Xueming(Steven) Li
On Tue, Oct 12, 2021 at 9:49 AM Andrew Rybchenko
<andrew.rybchenko@oktetlabs.ru> wrote:
>
> On 10/12/21 10:28 AM, David Marchand wrote:
> > If we do not enforce valid flags are passed by an application, this
> > application might face issues in the future when we add more flags.
>
> Thanks. I'd even consider it as a bug and the fix to be
> backported.
I wondered too when writing the patch.
I'd like to hear from others.
>
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
>
> A nit below, other than that:
>
> Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>
> [snip]
>
> > diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
> > index c5f859ae71..a2a78125f4 100644
> > --- a/lib/mempool/rte_mempool.c
> > +++ b/lib/mempool/rte_mempool.c
> > @@ -777,6 +777,13 @@ rte_mempool_cache_free(struct rte_mempool_cache *cache)
> > rte_free(cache);
> > }
> >
> > +#define MEMPOOL_KNOWN_FLAGS ( MEMPOOL_F_NO_SPREAD \
> > + | MEMPOOL_F_NO_CACHE_ALIGN \
> > + | MEMPOOL_F_SP_PUT \
> > + | MEMPOOL_F_SC_GET \
> > + | MEMPOOL_F_POOL_CREATED \
> > + | MEMPOOL_F_NO_IOVA_CONTIG \
> > + )
> > /* create an empty mempool */
> > struct rte_mempool *
> > rte_mempool_create_empty(const char *name, unsigned n, unsigned elt_size,
> > @@ -806,6 +813,12 @@ rte_mempool_create_empty(const char *name, unsigned n, unsigned elt_size,
> > RTE_CACHE_LINE_MASK) != 0);
> > #endif
> >
> > + /* enforce no unknown flag is passed by the application */
> > + if ((flags & ~MEMPOOL_KNOWN_FLAGS) != 0) {
> > + rte_errno = EINVAL;
> > + return NULL;
> > + }
> > +
>
> I think it is better to check arguments in parameters order.
> So, it is a bit better to move the check to happen after
> cache_size validation below.
Sounds reasonable, for v2.
--
David Marchand
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] mempool: enforce valid flags at creation
2021-10-12 7:28 [dpdk-dev] [PATCH] mempool: enforce valid flags at creation David Marchand
2021-10-12 7:49 ` Andrew Rybchenko
@ 2021-10-12 10:10 ` Kinsella, Ray
2021-10-14 19:29 ` [dpdk-dev] [PATCH v2] " David Marchand
2 siblings, 0 replies; 9+ messages in thread
From: Kinsella, Ray @ 2021-10-12 10:10 UTC (permalink / raw)
To: David Marchand, dev; +Cc: dkozlyuk, thomas, Olivier Matz, Andrew Rybchenko
On 12/10/2021 08:28, David Marchand wrote:
> If we do not enforce valid flags are passed by an application, this
> application might face issues in the future when we add more flags.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
Acked-by: Ray Kinsella <mdr@ashroe.eu>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] mempool: enforce valid flags at creation
2021-10-12 7:57 ` David Marchand
@ 2021-10-14 19:16 ` David Marchand
0 siblings, 0 replies; 9+ messages in thread
From: David Marchand @ 2021-10-14 19:16 UTC (permalink / raw)
To: Andrew Rybchenko
Cc: dev, Ray Kinsella, Dmitry Kozlyuk, Thomas Monjalon, Olivier Matz,
Kevin Traynor, Luca Boccassi, Christian Ehrhardt,
Xueming(Steven) Li
On Tue, Oct 12, 2021 at 9:57 AM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Tue, Oct 12, 2021 at 9:49 AM Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru> wrote:
> >
> > On 10/12/21 10:28 AM, David Marchand wrote:
> > > If we do not enforce valid flags are passed by an application, this
> > > application might face issues in the future when we add more flags.
> >
> > Thanks. I'd even consider it as a bug and the fix to be
> > backported.
>
> I wondered too when writing the patch.
> I'd like to hear from others.
Backporting risks breaking existing applications which were validated
with potentially invalid flags.
So I won't mark it for backport.
I'll send v2 wrt to your other comment.
Thanks Andrew.
--
David Marchand
^ permalink raw reply [flat|nested] 9+ messages in thread
* [dpdk-dev] [PATCH v2] mempool: enforce valid flags at creation
2021-10-12 7:28 [dpdk-dev] [PATCH] mempool: enforce valid flags at creation David Marchand
2021-10-12 7:49 ` Andrew Rybchenko
2021-10-12 10:10 ` Kinsella, Ray
@ 2021-10-14 19:29 ` David Marchand
2021-10-14 19:37 ` Stephen Hemminger
2 siblings, 1 reply; 9+ messages in thread
From: David Marchand @ 2021-10-14 19:29 UTC (permalink / raw)
To: dev; +Cc: dkozlyuk, thomas, Andrew Rybchenko, Ray Kinsella, Olivier Matz
If we do not enforce valid flags are passed by an application, this
application might face issues in the future when we add more flags.
Signed-off-by: David Marchand <david.marchand@redhat.com>
Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Acked-by: Ray Kinsella <mdr@ashroe.eu>
---
Changes since v1:
- fixed checkpatch warning,
- moved flags to validate them in the same order than parameters,
---
app/test/test_mempool.c | 21 +++++++++++++++++++++
lib/mempool/rte_mempool.c | 13 +++++++++++++
lib/mempool/rte_mempool.h | 2 +-
3 files changed, 35 insertions(+), 1 deletion(-)
diff --git a/app/test/test_mempool.c b/app/test/test_mempool.c
index 7675a3e605..66bc8d86b7 100644
--- a/app/test/test_mempool.c
+++ b/app/test/test_mempool.c
@@ -205,6 +205,24 @@ static int test_mempool_creation_with_exceeded_cache_size(void)
return 0;
}
+static int test_mempool_creation_with_unknown_flag(void)
+{
+ struct rte_mempool *mp_cov;
+
+ mp_cov = rte_mempool_create("test_mempool_unknown_flag", MEMPOOL_SIZE,
+ MEMPOOL_ELT_SIZE, 0, 0,
+ NULL, NULL,
+ NULL, NULL,
+ SOCKET_ID_ANY, MEMPOOL_F_NO_IOVA_CONTIG << 1);
+
+ if (mp_cov != NULL) {
+ rte_mempool_free(mp_cov);
+ RET_ERR();
+ }
+
+ return 0;
+}
+
static struct rte_mempool *mp_spsc;
static rte_spinlock_t scsp_spinlock;
static void *scsp_obj_table[MAX_KEEP];
@@ -635,6 +653,9 @@ test_mempool(void)
if (test_mempool_creation_with_exceeded_cache_size() < 0)
GOTO_ERR(ret, err);
+ if (test_mempool_creation_with_unknown_flag() < 0)
+ GOTO_ERR(ret, err);
+
if (test_mempool_same_name_twice_creation() < 0)
GOTO_ERR(ret, err);
diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
index c5f859ae71..607419ccaf 100644
--- a/lib/mempool/rte_mempool.c
+++ b/lib/mempool/rte_mempool.c
@@ -777,6 +777,13 @@ rte_mempool_cache_free(struct rte_mempool_cache *cache)
rte_free(cache);
}
+#define MEMPOOL_KNOWN_FLAGS (MEMPOOL_F_NO_SPREAD \
+ | MEMPOOL_F_NO_CACHE_ALIGN \
+ | MEMPOOL_F_SP_PUT \
+ | MEMPOOL_F_SC_GET \
+ | MEMPOOL_F_POOL_CREATED \
+ | MEMPOOL_F_NO_IOVA_CONTIG \
+ )
/* create an empty mempool */
struct rte_mempool *
rte_mempool_create_empty(const char *name, unsigned n, unsigned elt_size,
@@ -821,6 +828,12 @@ rte_mempool_create_empty(const char *name, unsigned n, unsigned elt_size,
return NULL;
}
+ /* enforce no unknown flag is passed by the application */
+ if ((flags & ~MEMPOOL_KNOWN_FLAGS) != 0) {
+ rte_errno = EINVAL;
+ return NULL;
+ }
+
/* "no cache align" imply "no spread" */
if (flags & MEMPOOL_F_NO_CACHE_ALIGN)
flags |= MEMPOOL_F_NO_SPREAD;
diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
index 04b14d7ae9..88bcbc51ef 100644
--- a/lib/mempool/rte_mempool.h
+++ b/lib/mempool/rte_mempool.h
@@ -991,7 +991,7 @@ typedef void (rte_mempool_ctor_t)(struct rte_mempool *, void *);
* with rte_errno set appropriately. Possible rte_errno values include:
* - E_RTE_NO_CONFIG - function could not get pointer to rte_config structure
* - E_RTE_SECONDARY - function was called from a secondary process instance
- * - EINVAL - cache size provided is too large
+ * - EINVAL - cache size provided is too large or an unknown flag was passed
* - ENOSPC - the maximum number of memzones has already been allocated
* - EEXIST - a memzone with the same name already exists
* - ENOMEM - no appropriate memory area found in which to create memzone
--
2.23.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH v2] mempool: enforce valid flags at creation
2021-10-14 19:29 ` [dpdk-dev] [PATCH v2] " David Marchand
@ 2021-10-14 19:37 ` Stephen Hemminger
2021-10-15 7:02 ` Andrew Rybchenko
0 siblings, 1 reply; 9+ messages in thread
From: Stephen Hemminger @ 2021-10-14 19:37 UTC (permalink / raw)
To: David Marchand
Cc: dev, dkozlyuk, thomas, Andrew Rybchenko, Ray Kinsella, Olivier Matz
On Thu, 14 Oct 2021 21:29:16 +0200
David Marchand <david.marchand@redhat.com> wrote:
> If we do not enforce valid flags are passed by an application, this
> application might face issues in the future when we add more flags.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Acked-by: Ray Kinsella <mdr@ashroe.eu>
> ---
> Changes since v1:
> - fixed checkpatch warning,
> - moved flags to validate them in the same order than parameters,
Acked-by: Stephen Hemminger <stephen@networkplumber.org>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH v2] mempool: enforce valid flags at creation
2021-10-14 19:37 ` Stephen Hemminger
@ 2021-10-15 7:02 ` Andrew Rybchenko
2021-10-15 8:26 ` David Marchand
0 siblings, 1 reply; 9+ messages in thread
From: Andrew Rybchenko @ 2021-10-15 7:02 UTC (permalink / raw)
To: Stephen Hemminger, David Marchand
Cc: dev, dkozlyuk, thomas, Ray Kinsella, Olivier Matz
On 10/14/21 10:37 PM, Stephen Hemminger wrote:
> On Thu, 14 Oct 2021 21:29:16 +0200
> David Marchand <david.marchand@redhat.com> wrote:
>
>> If we do not enforce valid flags are passed by an application, this
>> application might face issues in the future when we add more flags.
>>
>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>> Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Acked-by: Ray Kinsella <mdr@ashroe.eu>
>> ---
>> Changes since v1:
>> - fixed checkpatch warning,
>> - moved flags to validate them in the same order than parameters,
>
> Acked-by: Stephen Hemminger <stephen@networkplumber.org>
>
Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH v2] mempool: enforce valid flags at creation
2021-10-15 7:02 ` Andrew Rybchenko
@ 2021-10-15 8:26 ` David Marchand
0 siblings, 0 replies; 9+ messages in thread
From: David Marchand @ 2021-10-15 8:26 UTC (permalink / raw)
To: David Marchand
Cc: Andrew Rybchenko, Stephen Hemminger, dev, Dmitry Kozlyuk,
Thomas Monjalon, Ray Kinsella, Olivier Matz
On Fri, Oct 15, 2021 at 9:02 AM Andrew Rybchenko
<andrew.rybchenko@oktetlabs.ru> wrote:
> On 10/14/21 10:37 PM, Stephen Hemminger wrote:
> > On Thu, 14 Oct 2021 21:29:16 +0200
> > David Marchand <david.marchand@redhat.com> wrote:
> >
> >> If we do not enforce valid flags are passed by an application, this
> >> application might face issues in the future when we add more flags.
> >>
> >> Signed-off-by: David Marchand <david.marchand@redhat.com>
> >> Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >> Acked-by: Ray Kinsella <mdr@ashroe.eu>
> > Acked-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Applied, thanks.
--
David Marchand
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-10-15 8:26 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-12 7:28 [dpdk-dev] [PATCH] mempool: enforce valid flags at creation David Marchand
2021-10-12 7:49 ` Andrew Rybchenko
2021-10-12 7:57 ` David Marchand
2021-10-14 19:16 ` David Marchand
2021-10-12 10:10 ` Kinsella, Ray
2021-10-14 19:29 ` [dpdk-dev] [PATCH v2] " David Marchand
2021-10-14 19:37 ` Stephen Hemminger
2021-10-15 7:02 ` Andrew Rybchenko
2021-10-15 8:26 ` David Marchand
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).