[dpdk-dev] [RFC 0/4] Enforce flag checking in API's

[dpdk-dev] [RFC 0/4] Enforce flag checking in API's

[Stephen Hemminger]

Since the DPDK api's are lazy about checking for flag values
it means the flags can never be extended in future releases
without causing ABI breakage. This means we end up doing unnecessary
symbol versioning just to work around applications that might
pass in naughty bits.

This is the DPDK analog of the Linux kernel openat() problem.
Openat api was added but since kernel did not check flags it
ended up that another syscall openat2() was necessary before
the flags could be used.

Stephen Hemminger (4):
  ring: future proof flag settings
  hash: check flags on creation
  stack: check flags on creation
  cfgfile: check flags value

 lib/librte_cfgfile/rte_cfgfile.c  | 4 ++++
 lib/librte_hash/rte_cuckoo_hash.c | 9 +++++++++
 lib/librte_ring/rte_ring.c        | 7 +++++++
 lib/librte_stack/rte_stack.c      | 5 +++++
 4 files changed, 25 insertions(+)

-- 
2.20.1

[dpdk-dev] [RFC 1/4] ring: future proof flag settings

[Stephen Hemminger]

All API's should check that they support the flag values
passed. If an application passes an invalid flag it could
cause problems in later ABI.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_ring/rte_ring.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/lib/librte_ring/rte_ring.c b/lib/librte_ring/rte_ring.c
index 77e5de099b81..6cd8831649ea 100644
--- a/lib/librte_ring/rte_ring.c
+++ b/lib/librte_ring/rte_ring.c
@@ -100,6 +100,13 @@ rte_ring_init(struct rte_ring *r, const char *name, unsigned count,
 	RTE_BUILD_BUG_ON((offsetof(struct rte_ring, prod) &
 			  RTE_CACHE_LINE_MASK) != 0);
 
+	/* future proof flags, only allow supported values */
+	if (flags & ~(RING_F_SP_ENQ | RING_F_SC_DEQ)) {
+		RTE_LOG(ERR, RING,
+			"Unsupported flags requested %d\n", flags);
+		return -EINVAL;
+	}
+		
 	/* init the ring structure */
 	memset(r, 0, sizeof(*r));
 	ret = strlcpy(r->name, name, sizeof(r->name));
-- 
2.20.1

[dpdk-dev] [RFC 2/4] hash: check flags on creation

[Stephen Hemminger]

All API's should check that they support the flag values
passed. If an application passes an invalid flag it could
cause problems in later ABI.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_hash/rte_cuckoo_hash.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c
index 6af8ca42e94f..ab9cd91b4955 100644
--- a/lib/librte_hash/rte_cuckoo_hash.c
+++ b/lib/librte_hash/rte_cuckoo_hash.c
@@ -32,6 +32,9 @@
 #include "rte_hash.h"
 #include "rte_cuckoo_hash.h"
 
+/* Mask of all flags supported by this version */
+#define RTE_HASH_EXTRA_FLAGS_MASK 0x2f
+
 #define FOR_EACH_BUCKET(CURRENT_BKT, START_BUCKET)                            \
 	for (CURRENT_BKT = START_BUCKET;                                      \
 		CURRENT_BKT != NULL;                                          \
@@ -164,6 +167,12 @@ rte_hash_create(const struct rte_hash_parameters *params)
 		return NULL;
 	}
 
+	if (params->extra_flag & ~RTE_HASH_EXTRA_FLAGS_MASK) {
+		rte_errno = EINVAL;
+		RTE_LOG(ERR, HASH, "rte_hash_create: unsupported extra flags\n");
+		return NULL;
+	}
+
 	/* Validate correct usage of extra options */
 	if ((params->extra_flag & RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY) &&
 	    (params->extra_flag & RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF)) {
-- 
2.20.1

[dpdk-dev] [RFC 3/4] stack: check flags on creation

[Stephen Hemminger]

All API's should check that they support the flag values
passed. If an application passes an invalid flag it could
cause problems in later ABI.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_stack/rte_stack.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/librte_stack/rte_stack.c b/lib/librte_stack/rte_stack.c
index d19824f00439..4b09ff30d6bd 100644
--- a/lib/librte_stack/rte_stack.c
+++ b/lib/librte_stack/rte_stack.c
@@ -59,6 +59,11 @@ rte_stack_create(const char *name, unsigned int count, int socket_id,
 	unsigned int sz;
 	int ret;
 
+	if (flags & ~(RTE_STACK_F_LF)) {
+		STACK_LOG_ERR("Unsupported stack flags %#x\n", flags);
+		return NULL;
+	}
+	
 #ifdef RTE_ARCH_64
 	RTE_BUILD_BUG_ON(sizeof(struct rte_stack_lf_head) != 16);
 #else
-- 
2.20.1

[dpdk-dev] [RFC 4/4] cfgfile: check flags value

[Stephen Hemminger]

All API's should check that they support the flag values
passed. If an application passes an invalid flag it could
cause problems in later ABI.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_cfgfile/rte_cfgfile.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index 9049fd9c2319..cec82c82ee20 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -272,6 +272,10 @@ rte_cfgfile_create(int flags)
 	int i;
 	struct rte_cfgfile *cfg;
 
+	/* future proof flags usage */
+	if (flags & ~(CFG_FLAG_GLOBAL_SECTION | CFG_FLAG_EMPTY_VALUES))
+		return NULL;
+	
 	cfg = malloc(sizeof(*cfg));
 
 	if (cfg == NULL)
-- 
2.20.1

Re: [dpdk-dev] [RFC 1/4] ring: future proof flag settings

[Ananyev, Konstantin]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Stephen Hemminger
> Sent: Wednesday, February 12, 2020 11:08 PM
> To: dev@dpdk.org
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Subject: [dpdk-dev] [RFC 1/4] ring: future proof flag settings
> 
> All API's should check that they support the flag values
> passed. If an application passes an invalid flag it could
> cause problems in later ABI.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/librte_ring/rte_ring.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/lib/librte_ring/rte_ring.c b/lib/librte_ring/rte_ring.c
> index 77e5de099b81..6cd8831649ea 100644
> --- a/lib/librte_ring/rte_ring.c
> +++ b/lib/librte_ring/rte_ring.c
> @@ -100,6 +100,13 @@ rte_ring_init(struct rte_ring *r, const char *name, unsigned count,
>  	RTE_BUILD_BUG_ON((offsetof(struct rte_ring, prod) &
>  			  RTE_CACHE_LINE_MASK) != 0);
> 
> +	/* future proof flags, only allow supported values */
> +	if (flags & ~(RING_F_SP_ENQ | RING_F_SC_DEQ)) {
> +		RTE_LOG(ERR, RING,
> +			"Unsupported flags requested %d\n", flags);
> +		return -EINVAL;
> +	}
> +

Good idea, I think, but seems you forgot RING_F_EXACT_SZ.

>  	/* init the ring structure */
>  	memset(r, 0, sizeof(*r));
>  	ret = strlcpy(r->name, name, sizeof(r->name));
> --
> 2.20.1

[dpdk-dev] [PATCH v2 0/4] enforce checking on flag values in API's

[Stephen Hemminger]

Since the DPDK api's are lazy about checking for flag values
it means the flags can never be extended in future releases
without causing ABI breakage. This means we end up doing unnecessary
symbol versioning just to work around applications that might
pass in naughty bits.

This is the DPDK analog of the Linux kernel openat() problem.
Openat api was added but since kernel did not check flags it
ended up that another syscall openat2() was necessary before
the flags could be used.

v2 - rebase and fix checkpatch warnings from RFC

Stephen Hemminger (4):
  ring: future proof flag settings
  hash: check flags on creation
  stack: check flags on creation
  cfgfile: check flags value

 lib/librte_cfgfile/rte_cfgfile.c  |  4 ++++
 lib/librte_hash/rte_cuckoo_hash.c |  9 +++++++++
 lib/librte_ring/rte_ring.c        | 10 ++++++++++
 lib/librte_stack/rte_stack.c      |  5 +++++
 4 files changed, 28 insertions(+)

-- 
2.20.1

[dpdk-dev] [PATCH v2 1/4] ring: future proof flag settings

[Stephen Hemminger]

All API's should check that they support the flag values passed.
These checks ensure that the extra bits can safely be used
without risk of ABI breakage.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_ring/rte_ring.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/lib/librte_ring/rte_ring.c b/lib/librte_ring/rte_ring.c
index ebe5ccf0de68..70685121581f 100644
--- a/lib/librte_ring/rte_ring.c
+++ b/lib/librte_ring/rte_ring.c
@@ -42,6 +42,9 @@ static struct rte_tailq_elem rte_ring_tailq = {
 };
 EAL_REGISTER_TAILQ(rte_ring_tailq)
 
+/* mask of all valid flag values to ring_create() */
+#define RING_F_MASK	0x007F
+
 /* true if x is a power of 2 */
 #define POWEROF2(x) ((((x)-1) & (x)) == 0)
 
@@ -197,6 +200,13 @@ rte_ring_init(struct rte_ring *r, const char *name, unsigned count,
 	RTE_BUILD_BUG_ON(offsetof(struct rte_ring_headtail, tail) !=
 		offsetof(struct rte_ring_rts_headtail, tail.val.pos));
 
+	/* future proof flags, only allow supported values */
+	if (flags & ~RING_F_MASK) {
+		RTE_LOG(ERR, RING,
+			"Unsupported flags requested %#x\n", flags);
+		return -EINVAL;
+	}
+
 	/* init the ring structure */
 	memset(r, 0, sizeof(*r));
 	ret = strlcpy(r->name, name, sizeof(r->name));
-- 
2.20.1

[dpdk-dev] [PATCH v2 2/4] hash: check flags on creation

[Stephen Hemminger]

All API's should check that they support the flag values
passed. If an application passes an invalid flag it could
cause problems in later ABI.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_hash/rte_cuckoo_hash.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c
index 6af8ca42e94f..ab9cd91b4955 100644
--- a/lib/librte_hash/rte_cuckoo_hash.c
+++ b/lib/librte_hash/rte_cuckoo_hash.c
@@ -32,6 +32,9 @@
 #include "rte_hash.h"
 #include "rte_cuckoo_hash.h"
 
+/* Mask of all flags supported by this version */
+#define RTE_HASH_EXTRA_FLAGS_MASK 0x2f
+
 #define FOR_EACH_BUCKET(CURRENT_BKT, START_BUCKET)                            \
 	for (CURRENT_BKT = START_BUCKET;                                      \
 		CURRENT_BKT != NULL;                                          \
@@ -164,6 +167,12 @@ rte_hash_create(const struct rte_hash_parameters *params)
 		return NULL;
 	}
 
+	if (params->extra_flag & ~RTE_HASH_EXTRA_FLAGS_MASK) {
+		rte_errno = EINVAL;
+		RTE_LOG(ERR, HASH, "rte_hash_create: unsupported extra flags\n");
+		return NULL;
+	}
+
 	/* Validate correct usage of extra options */
 	if ((params->extra_flag & RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY) &&
 	    (params->extra_flag & RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF)) {
-- 
2.20.1

[dpdk-dev] [PATCH v2 3/4] stack: check flags on creation

[Stephen Hemminger]

All API's should check that they support the flag values
passed. If an application passes an invalid flag it could
cause problems in later ABI.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_stack/rte_stack.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/librte_stack/rte_stack.c b/lib/librte_stack/rte_stack.c
index d19824f00439..e58fa545fca4 100644
--- a/lib/librte_stack/rte_stack.c
+++ b/lib/librte_stack/rte_stack.c
@@ -59,6 +59,11 @@ rte_stack_create(const char *name, unsigned int count, int socket_id,
 	unsigned int sz;
 	int ret;
 
+	if (flags & ~(RTE_STACK_F_LF)) {
+		STACK_LOG_ERR("Unsupported stack flags %#x\n", flags);
+		return NULL;
+	}
+
 #ifdef RTE_ARCH_64
 	RTE_BUILD_BUG_ON(sizeof(struct rte_stack_lf_head) != 16);
 #else
-- 
2.20.1

[dpdk-dev] [PATCH v2 4/4] cfgfile: check flags value

[Stephen Hemminger]

All API's should check that they support the flag values
passed. If an application passes an invalid flag it could
cause problems in later ABI.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_cfgfile/rte_cfgfile.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index 9049fd9c2319..714717dd9007 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -272,6 +272,10 @@ rte_cfgfile_create(int flags)
 	int i;
 	struct rte_cfgfile *cfg;
 
+	/* future proof flags usage */
+	if (flags & ~(CFG_FLAG_GLOBAL_SECTION | CFG_FLAG_EMPTY_VALUES))
+		return NULL;
+
 	cfg = malloc(sizeof(*cfg));
 
 	if (cfg == NULL)
-- 
2.20.1

Re: [dpdk-dev] [PATCH v2 1/4] ring: future proof flag settings

[Honnappa Nagarahalli]

<snip>

> 
> All API's should check that they support the flag values passed.
> These checks ensure that the extra bits can safely be used without risk of ABI
> breakage.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/librte_ring/rte_ring.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/lib/librte_ring/rte_ring.c b/lib/librte_ring/rte_ring.c index
> ebe5ccf0de68..70685121581f 100644
> --- a/lib/librte_ring/rte_ring.c
> +++ b/lib/librte_ring/rte_ring.c
> @@ -42,6 +42,9 @@ static struct rte_tailq_elem rte_ring_tailq = {  };
>  EAL_REGISTER_TAILQ(rte_ring_tailq)
> 
> +/* mask of all valid flag values to ring_create() */
> +#define RING_F_MASK	0x007F
Is it better to construct this using the actual flag #defines?

> +
>  /* true if x is a power of 2 */
>  #define POWEROF2(x) ((((x)-1) & (x)) == 0)
> 
> @@ -197,6 +200,13 @@ rte_ring_init(struct rte_ring *r, const char *name,
> unsigned count,
>  	RTE_BUILD_BUG_ON(offsetof(struct rte_ring_headtail, tail) !=
>  		offsetof(struct rte_ring_rts_headtail, tail.val.pos));
> 
> +	/* future proof flags, only allow supported values */
> +	if (flags & ~RING_F_MASK) {
> +		RTE_LOG(ERR, RING,
> +			"Unsupported flags requested %#x\n", flags);
> +		return -EINVAL;
> +	}
> +
>  	/* init the ring structure */
>  	memset(r, 0, sizeof(*r));
>  	ret = strlcpy(r->name, name, sizeof(r->name));
> --
> 2.20.1

Re: [dpdk-dev] [PATCH v2 1/4] ring: future proof flag settings

[Stephen Hemminger]

On Fri, 24 Apr 2020 18:07:15 +0000
Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com> wrote:

> <snip>
> 
> > 
> > All API's should check that they support the flag values passed.
> > These checks ensure that the extra bits can safely be used without risk of ABI
> > breakage.
> > 
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> >  lib/librte_ring/rte_ring.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/lib/librte_ring/rte_ring.c b/lib/librte_ring/rte_ring.c index
> > ebe5ccf0de68..70685121581f 100644
> > --- a/lib/librte_ring/rte_ring.c
> > +++ b/lib/librte_ring/rte_ring.c
> > @@ -42,6 +42,9 @@ static struct rte_tailq_elem rte_ring_tailq = {  };
> >  EAL_REGISTER_TAILQ(rte_ring_tailq)
> > 
> > +/* mask of all valid flag values to ring_create() */
> > +#define RING_F_MASK	0x007F  
> Is it better to construct this using the actual flag #defines?

sure, but it gets long

Re: [dpdk-dev] [PATCH v2 1/4] ring: future proof flag settings

[Ananyev, Konstantin]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Stephen Hemminger
> Sent: Friday, April 24, 2020 8:02 PM
> To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Cc: dev@dpdk.org; nd <nd@arm.com>
> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ring: future proof flag settings
> 
> On Fri, 24 Apr 2020 18:07:15 +0000
> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com> wrote:
> 
> > <snip>
> >
> > >
> > > All API's should check that they support the flag values passed.
> > > These checks ensure that the extra bits can safely be used without risk of ABI
> > > breakage.
> > >
> > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > > ---
> > >  lib/librte_ring/rte_ring.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > >
> > > diff --git a/lib/librte_ring/rte_ring.c b/lib/librte_ring/rte_ring.c index
> > > ebe5ccf0de68..70685121581f 100644
> > > --- a/lib/librte_ring/rte_ring.c
> > > +++ b/lib/librte_ring/rte_ring.c
> > > @@ -42,6 +42,9 @@ static struct rte_tailq_elem rte_ring_tailq = {  };
> > >  EAL_REGISTER_TAILQ(rte_ring_tailq)
> > >
> > > +/* mask of all valid flag values to ring_create() */
> > > +#define RING_F_MASK	0x007F
> > Is it better to construct this using the actual flag #defines?
> 
> sure, but it gets long

+1 to use public defines here.

[dpdk-dev] [PATCH v3 0/4] Enforce checking on flag values in API's

[Stephen Hemminger]

The DPDK API's are lax about checking for undefined flag values.
This makes it impossible to add new bits to existing API's
without causing ABI breakage. This means we end up doing unnecessary
symbol versioning just to work around applications that might
pass in naughty bits.

This is the DPDK analog of the Linux kernel openat() problem.
Openat api was added but since kernel did not check flags it
ended up that another syscall openat2() was necessary before
the flags could be used.

v3 - define mask based on existing defines for ring and hash

Stephen Hemminger (4):
  ring: future proof flag settings
  hash: check flags on creation
  stack: check flags on creation
  cfgfile: check flags value

 lib/librte_cfgfile/rte_cfgfile.c  |  4 ++++
 lib/librte_hash/rte_cuckoo_hash.c | 14 ++++++++++++++
 lib/librte_ring/rte_ring.c        | 12 ++++++++++++
 lib/librte_stack/rte_stack.c      |  5 +++++
 4 files changed, 35 insertions(+)

-- 
2.20.1

[dpdk-dev] [PATCH v3 1/4] ring: future proof flag settings

[Stephen Hemminger]

All API's should check that they support the flag values passed.
These checks ensure that the extra bits can safely be used
without risk of ABI breakage.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_ring/rte_ring.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/lib/librte_ring/rte_ring.c b/lib/librte_ring/rte_ring.c
index ebe5ccf0de68..e16dd45a82de 100644
--- a/lib/librte_ring/rte_ring.c
+++ b/lib/librte_ring/rte_ring.c
@@ -42,6 +42,11 @@ static struct rte_tailq_elem rte_ring_tailq = {
 };
 EAL_REGISTER_TAILQ(rte_ring_tailq)
 
+/* mask of all valid flag values to ring_create() */
+#define RING_F_MASK (RING_F_SP_ENQ | RING_F_SC_DEQ | RING_F_EXACT_SZ | \
+		     RING_F_MP_RTS_ENQ | RING_F_MC_RTS_DEQ |	       \
+		     RING_F_MP_HTS_ENQ | RING_F_MC_HTS_DEQ )
+
 /* true if x is a power of 2 */
 #define POWEROF2(x) ((((x)-1) & (x)) == 0)
 
@@ -197,6 +202,13 @@ rte_ring_init(struct rte_ring *r, const char *name, unsigned count,
 	RTE_BUILD_BUG_ON(offsetof(struct rte_ring_headtail, tail) !=
 		offsetof(struct rte_ring_rts_headtail, tail.val.pos));
 
+	/* future proof flags, only allow supported values */
+	if (flags & ~RING_F_MASK) {
+		RTE_LOG(ERR, RING,
+			"Unsupported flags requested %#x\n", flags);
+		return -EINVAL;
+	}
+
 	/* init the ring structure */
 	memset(r, 0, sizeof(*r));
 	ret = strlcpy(r->name, name, sizeof(r->name));
-- 
2.20.1

[dpdk-dev] [PATCH v3 2/4] hash: check flags on creation

[Stephen Hemminger]

All API's should check that they support the flag values
passed. If an application passes an invalid flag it could
cause problems in later ABI.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_hash/rte_cuckoo_hash.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/lib/librte_hash/rte_cuckoo_hash.c b/lib/librte_hash/rte_cuckoo_hash.c
index 38767a8a164f..d54bbb47edcb 100644
--- a/lib/librte_hash/rte_cuckoo_hash.c
+++ b/lib/librte_hash/rte_cuckoo_hash.c
@@ -32,6 +32,14 @@
 #include "rte_hash.h"
 #include "rte_cuckoo_hash.h"
 
+/* Mask of all flags supported by this version */
+#define RTE_HASH_EXTRA_FLAGS_MASK (RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT | \
+				   RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD | \
+				   RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY | \
+				   RTE_HASH_EXTRA_FLAGS_EXT_TABLE |	\
+				   RTE_HASH_EXTRA_FLAGS_NO_FREE_ON_DEL | \
+				   RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF)
+
 #define FOR_EACH_BUCKET(CURRENT_BKT, START_BUCKET)                            \
 	for (CURRENT_BKT = START_BUCKET;                                      \
 		CURRENT_BKT != NULL;                                          \
@@ -164,6 +172,12 @@ rte_hash_create(const struct rte_hash_parameters *params)
 		return NULL;
 	}
 
+	if (params->extra_flag & ~RTE_HASH_EXTRA_FLAGS_MASK) {
+		rte_errno = EINVAL;
+		RTE_LOG(ERR, HASH, "rte_hash_create: unsupported extra flags\n");
+		return NULL;
+	}
+
 	/* Validate correct usage of extra options */
 	if ((params->extra_flag & RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY) &&
 	    (params->extra_flag & RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF)) {
-- 
2.20.1

[dpdk-dev] [PATCH v3 3/4] stack: check flags on creation

[Stephen Hemminger]

All API's should check that they support the flag values
passed. If an application passes an invalid flag it could
cause problems in later ABI.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_stack/rte_stack.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/librte_stack/rte_stack.c b/lib/librte_stack/rte_stack.c
index d19824f00439..e58fa545fca4 100644
--- a/lib/librte_stack/rte_stack.c
+++ b/lib/librte_stack/rte_stack.c
@@ -59,6 +59,11 @@ rte_stack_create(const char *name, unsigned int count, int socket_id,
 	unsigned int sz;
 	int ret;
 
+	if (flags & ~(RTE_STACK_F_LF)) {
+		STACK_LOG_ERR("Unsupported stack flags %#x\n", flags);
+		return NULL;
+	}
+
 #ifdef RTE_ARCH_64
 	RTE_BUILD_BUG_ON(sizeof(struct rte_stack_lf_head) != 16);
 #else
-- 
2.20.1

[dpdk-dev] [PATCH v3 4/4] cfgfile: check flags value

[Stephen Hemminger]

All API's should check that they support the flag values
passed. If an application passes an invalid flag it could
cause problems in later ABI.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_cfgfile/rte_cfgfile.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index 9049fd9c2319..714717dd9007 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -272,6 +272,10 @@ rte_cfgfile_create(int flags)
 	int i;
 	struct rte_cfgfile *cfg;
 
+	/* future proof flags usage */
+	if (flags & ~(CFG_FLAG_GLOBAL_SECTION | CFG_FLAG_EMPTY_VALUES))
+		return NULL;
+
 	cfg = malloc(sizeof(*cfg));
 
 	if (cfg == NULL)
-- 
2.20.1

Re: [dpdk-dev] [PATCH v3 1/4] ring: future proof flag settings

[Honnappa Nagarahalli]

<snip>

> 
> All API's should check that they support the flag values passed.
> These checks ensure that the extra bits can safely be used without risk of ABI
> breakage.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>

> ---
>  lib/librte_ring/rte_ring.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/lib/librte_ring/rte_ring.c b/lib/librte_ring/rte_ring.c index
> ebe5ccf0de68..e16dd45a82de 100644
> --- a/lib/librte_ring/rte_ring.c
> +++ b/lib/librte_ring/rte_ring.c
> @@ -42,6 +42,11 @@ static struct rte_tailq_elem rte_ring_tailq = {  };
>  EAL_REGISTER_TAILQ(rte_ring_tailq)
> 
> +/* mask of all valid flag values to ring_create() */ #define
> +RING_F_MASK (RING_F_SP_ENQ | RING_F_SC_DEQ | RING_F_EXACT_SZ | \
> +		     RING_F_MP_RTS_ENQ | RING_F_MC_RTS_DEQ |	       \
> +		     RING_F_MP_HTS_ENQ | RING_F_MC_HTS_DEQ )
> +
>  /* true if x is a power of 2 */
>  #define POWEROF2(x) ((((x)-1) & (x)) == 0)
> 
> @@ -197,6 +202,13 @@ rte_ring_init(struct rte_ring *r, const char *name,
> unsigned count,
>  	RTE_BUILD_BUG_ON(offsetof(struct rte_ring_headtail, tail) !=
>  		offsetof(struct rte_ring_rts_headtail, tail.val.pos));
> 
> +	/* future proof flags, only allow supported values */
> +	if (flags & ~RING_F_MASK) {
> +		RTE_LOG(ERR, RING,
> +			"Unsupported flags requested %#x\n", flags);
> +		return -EINVAL;
> +	}
> +
>  	/* init the ring structure */
>  	memset(r, 0, sizeof(*r));
>  	ret = strlcpy(r->name, name, sizeof(r->name));
> --
> 2.20.1

Re: [dpdk-dev] [PATCH v3 4/4] cfgfile: check flags value

[Bruce Richardson]

On Mon, Apr 27, 2020 at 04:16:25PM -0700, Stephen Hemminger wrote:
> All API's should check that they support the flag values
> passed. If an application passes an invalid flag it could
> cause problems in later ABI.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [dpdk-dev] [PATCH v3 0/4] Enforce checking on flag values in API's

[Bruce Richardson]

On Mon, Apr 27, 2020 at 04:16:21PM -0700, Stephen Hemminger wrote:
> The DPDK API's are lax about checking for undefined flag values.
> This makes it impossible to add new bits to existing API's
> without causing ABI breakage. This means we end up doing unnecessary
> symbol versioning just to work around applications that might
> pass in naughty bits.
> 
> This is the DPDK analog of the Linux kernel openat() problem.
> Openat api was added but since kernel did not check flags it
> ended up that another syscall openat2() was necessary before
> the flags could be used.
> 
> v3 - define mask based on existing defines for ring and hash
> 
> Stephen Hemminger (4):
>   ring: future proof flag settings
>   hash: check flags on creation
>   stack: check flags on creation
>   cfgfile: check flags value
> 
>  lib/librte_cfgfile/rte_cfgfile.c  |  4 ++++
>  lib/librte_hash/rte_cuckoo_hash.c | 14 ++++++++++++++
>  lib/librte_ring/rte_ring.c        | 12 ++++++++++++
>  lib/librte_stack/rte_stack.c      |  5 +++++
>  4 files changed, 35 insertions(+)
> 
I think this is a good idea to do in DPDK

Series-acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [dpdk-dev] [PATCH v3 0/4] Enforce checking on flag values in API's

[Ananyev, Konstantin]

> The DPDK API's are lax about checking for undefined flag values.
> This makes it impossible to add new bits to existing API's
> without causing ABI breakage. This means we end up doing unnecessary
> symbol versioning just to work around applications that might
> pass in naughty bits.
> 
> This is the DPDK analog of the Linux kernel openat() problem.
> Openat api was added but since kernel did not check flags it
> ended up that another syscall openat2() was necessary before
> the flags could be used.
> 
> v3 - define mask based on existing defines for ring and hash
> 
> Stephen Hemminger (4):
>   ring: future proof flag settings
>   hash: check flags on creation
>   stack: check flags on creation
>   cfgfile: check flags value
> 
>  lib/librte_cfgfile/rte_cfgfile.c  |  4 ++++
>  lib/librte_hash/rte_cuckoo_hash.c | 14 ++++++++++++++
>  lib/librte_ring/rte_ring.c        | 12 ++++++++++++
>  lib/librte_stack/rte_stack.c      |  5 +++++
>  4 files changed, 35 insertions(+)
> 
> --

Series Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> 2.20.1

Re: [dpdk-dev] [PATCH v3 2/4] hash: check flags on creation

[Wang, Yipeng1]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Monday, April 27, 2020 4:16 PM
> To: dev@dpdk.org
> Cc: Stephen Hemminger <stephen@networkplumber.org>; Wang, Yipeng1
> <yipeng1.wang@intel.com>; Gobriel, Sameh <sameh.gobriel@intel.com>;
> Richardson, Bruce <bruce.richardson@intel.com>
> Subject: [PATCH v3 2/4] hash: check flags on creation
> 
> All API's should check that they support the flag values passed. If an application
> passes an invalid flag it could cause problems in later ABI.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
Acked-by: Yipeng Wang <yipeng1.wang@intel.com>

Re: [dpdk-dev] [PATCH v3 3/4] stack: check flags on creation

[Eads, Gage]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Monday, April 27, 2020 6:16 PM
> To: dev@dpdk.org
> Cc: Stephen Hemminger <stephen@networkplumber.org>; Eads, Gage
> <gage.eads@intel.com>; Olivier Matz <olivier.matz@6wind.com>
> Subject: [PATCH v3 3/4] stack: check flags on creation
> 
> All API's should check that they support the flag values passed. If an application
> passes an invalid flag it could cause problems in later ABI.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
Acked-by: Gage Eads <gage.eads@intel.com>

Re: [dpdk-dev] [PATCH v3 0/4] Enforce checking on flag values in API's

[Thomas Monjalon]

28/04/2020 12:28, Bruce Richardson:
> On Mon, Apr 27, 2020 at 04:16:21PM -0700, Stephen Hemminger wrote:
> > The DPDK API's are lax about checking for undefined flag values.
> > This makes it impossible to add new bits to existing API's
> > without causing ABI breakage. This means we end up doing unnecessary
> > symbol versioning just to work around applications that might
> > pass in naughty bits.
> > 
> > This is the DPDK analog of the Linux kernel openat() problem.
> > Openat api was added but since kernel did not check flags it
> > ended up that another syscall openat2() was necessary before
> > the flags could be used.
> > 
> > v3 - define mask based on existing defines for ring and hash
> > 
> > Stephen Hemminger (4):
> >   ring: future proof flag settings
> >   hash: check flags on creation
> >   stack: check flags on creation
> >   cfgfile: check flags value
> > 
> I think this is a good idea to do in DPDK
> 
> Series-acked-by: Bruce Richardson <bruce.richardson@intel.com>

Applied, thanks