* [dpdk-dev] [PATCH 1/5] mbuf: fix out-of-bounds access at dyn field register
2020-06-13 15:49 [dpdk-dev] [PATCH 0/5] small fixes for mbuf's dynamic field/flag feature Xiaolong Ye
@ 2020-06-13 15:49 ` Xiaolong Ye
2020-06-13 15:49 ` [dpdk-dev] [PATCH 2/5] mbuf: fix missing errno for dyn field/flag registration Xiaolong Ye
` (4 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Xiaolong Ye @ 2020-06-13 15:49 UTC (permalink / raw
To: Olivier Matz, Thomas Monjalon, Konstantin Ananyev
Cc: dev, Xiaolong Ye, stable
We should make sure off + size < sizeof(struct rte_mbuf) to avoid
possible out-of-bounds access of free_space array, there is no issue
currently due to the low bits of free_flags (which is adjacent to
free_space) are always set to 0. But we shouldn't rely on it since it's
fragile and layout of struct mbuf_dyn_shm may be changed in the future.
This patch adds boundary check explicitly to avoid potential risk of
out-of-bounds access.
Fixes: 4958ca3a443a ("mbuf: support dynamic fields and flags")
Cc: stable@dpdk.org
Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
Acked-by: Olivier Matz <olivier.matz@6wind.com>
---
lib/librte_mbuf/rte_mbuf_dyn.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/librte_mbuf/rte_mbuf_dyn.c b/lib/librte_mbuf/rte_mbuf_dyn.c
index 953e3ec31c..13d6da6d16 100644
--- a/lib/librte_mbuf/rte_mbuf_dyn.c
+++ b/lib/librte_mbuf/rte_mbuf_dyn.c
@@ -69,7 +69,8 @@ process_score(void)
for (off = 0; off < sizeof(struct rte_mbuf); off++) {
/* get the size of the free zone */
- for (size = 0; shm->free_space[off + size]; size++)
+ for (size = 0; (off + size) < sizeof(struct rte_mbuf) &&
+ shm->free_space[off + size]; size++)
;
if (size == 0)
continue;
--
2.17.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [dpdk-dev] [PATCH 2/5] mbuf: fix missing errno for dyn field/flag registration
2020-06-13 15:49 [dpdk-dev] [PATCH 0/5] small fixes for mbuf's dynamic field/flag feature Xiaolong Ye
2020-06-13 15:49 ` [dpdk-dev] [PATCH 1/5] mbuf: fix out-of-bounds access at dyn field register Xiaolong Ye
@ 2020-06-13 15:49 ` Xiaolong Ye
2020-06-25 15:31 ` Olivier Matz
2020-06-13 15:49 ` [dpdk-dev] [PATCH 3/5] mbuf: fix free_space setting for dynamic field Xiaolong Ye
` (3 subsequent siblings)
5 siblings, 1 reply; 11+ messages in thread
From: Xiaolong Ye @ 2020-06-13 15:49 UTC (permalink / raw
To: Olivier Matz, Konstantin Ananyev, Thomas Monjalon
Cc: dev, Xiaolong Ye, stable
Set rte_errno as ENOMEM when allocation failure.
Fixes: 4958ca3a443a ("mbuf: support dynamic fields and flags")
Cc: stable@dpdk.org
Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
---
lib/librte_mbuf/rte_mbuf_dyn.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/lib/librte_mbuf/rte_mbuf_dyn.c b/lib/librte_mbuf/rte_mbuf_dyn.c
index 13d6da6d16..de7d2eb9a5 100644
--- a/lib/librte_mbuf/rte_mbuf_dyn.c
+++ b/lib/librte_mbuf/rte_mbuf_dyn.c
@@ -278,12 +278,15 @@ __rte_mbuf_dynfield_register_offset(const struct rte_mbuf_dynfield *params,
mbuf_dynfield_tailq.head, mbuf_dynfield_list);
te = rte_zmalloc("MBUF_DYNFIELD_TAILQ_ENTRY", sizeof(*te), 0);
- if (te == NULL)
+ if (te == NULL) {
+ rte_errno = ENOMEM;
return -1;
+ }
mbuf_dynfield = rte_zmalloc("mbuf_dynfield", sizeof(*mbuf_dynfield), 0);
if (mbuf_dynfield == NULL) {
rte_free(te);
+ rte_errno = ENOMEM;
return -1;
}
@@ -456,12 +459,15 @@ __rte_mbuf_dynflag_register_bitnum(const struct rte_mbuf_dynflag *params,
mbuf_dynflag_tailq.head, mbuf_dynflag_list);
te = rte_zmalloc("MBUF_DYNFLAG_TAILQ_ENTRY", sizeof(*te), 0);
- if (te == NULL)
+ if (te == NULL) {
+ rte_errno = ENOMEM;
return -1;
+ }
mbuf_dynflag = rte_zmalloc("mbuf_dynflag", sizeof(*mbuf_dynflag), 0);
if (mbuf_dynflag == NULL) {
rte_free(te);
+ rte_errno = ENOMEM;
return -1;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [dpdk-dev] [PATCH 2/5] mbuf: fix missing errno for dyn field/flag registration
2020-06-13 15:49 ` [dpdk-dev] [PATCH 2/5] mbuf: fix missing errno for dyn field/flag registration Xiaolong Ye
@ 2020-06-25 15:31 ` Olivier Matz
0 siblings, 0 replies; 11+ messages in thread
From: Olivier Matz @ 2020-06-25 15:31 UTC (permalink / raw
To: Xiaolong Ye; +Cc: Konstantin Ananyev, Thomas Monjalon, dev, stable
On Sat, Jun 13, 2020 at 11:49:18PM +0800, Xiaolong Ye wrote:
> Set rte_errno as ENOMEM when allocation failure.
>
> Fixes: 4958ca3a443a ("mbuf: support dynamic fields and flags")
> Cc: stable@dpdk.org
>
> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
Good catch, I was wrongly expecting that rte_malloc() functions sets
rte_errno on error.
I wonder if the malloc API shouldn't be modified, because I can see
several places where the same kind of fix would be needed: rte_efd,
rte_cuckoo_hash, rte_fbk_hash, rte_member, ...
Anatoly, what do you think?
Acked-by: Olivier Matz <olivier.matz@6wind.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [dpdk-dev] [PATCH 3/5] mbuf: fix free_space setting for dynamic field
2020-06-13 15:49 [dpdk-dev] [PATCH 0/5] small fixes for mbuf's dynamic field/flag feature Xiaolong Ye
2020-06-13 15:49 ` [dpdk-dev] [PATCH 1/5] mbuf: fix out-of-bounds access at dyn field register Xiaolong Ye
2020-06-13 15:49 ` [dpdk-dev] [PATCH 2/5] mbuf: fix missing errno for dyn field/flag registration Xiaolong Ye
@ 2020-06-13 15:49 ` Xiaolong Ye
2020-06-25 15:53 ` Olivier Matz
2020-06-13 15:49 ` [dpdk-dev] [PATCH 4/5] mbuf: fix a dynamic field dump log Xiaolong Ye
` (2 subsequent siblings)
5 siblings, 1 reply; 11+ messages in thread
From: Xiaolong Ye @ 2020-06-13 15:49 UTC (permalink / raw
To: Olivier Matz, Thomas Monjalon, Konstantin Ananyev
Cc: dev, Xiaolong Ye, stable
The value free_space[i] is used to save the size of biggest aligned
element that can fit in the zone, current implementation has one flaw,
for example, if user registers dynfield1 (size = 4, align = 4, req = 124)
first, the free_space would be as below after registration:
0070: 08 08 08 08 08 08 08 08
0078: 08 08 08 08 00 00 00 00
Then if user continues to register dynfield2 (size = 4, align = 4),
free_space would become:
0070: 00 00 00 00 04 04 04 04
0078: 04 04 04 04 00 00 00 00
Further request dynfield3 (size = 8, align = 8) would fail to register
due to alignment requirement can't be satisfied, though there is enough
space remained in mbuf.
This patch fixes above issue by saving alignment only in aligned zone,
after the fix, above registrations order can be satisfied, free_space
would be like:
After dynfield1 registration:
0070: 08 08 08 08 08 08 08 08
0078: 04 04 04 04 00 00 00 00
After dynfield2 registration:
0070: 08 08 08 08 08 08 08 08
0078: 00 00 00 00 00 00 00 00
After dynfield3 registration:
0070: 00 00 00 00 00 00 00 00
0078: 00 00 00 00 00 00 00 00
This patch also reduces iterations in process_score() by jumping align
steps in each loop.
Fixes: 4958ca3a443a ("mbuf: support dynamic fields and flags")
Cc: stable@dpdk.org
Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
---
lib/librte_mbuf/rte_mbuf_dyn.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/lib/librte_mbuf/rte_mbuf_dyn.c b/lib/librte_mbuf/rte_mbuf_dyn.c
index de7d2eb9a5..fd51e1b68e 100644
--- a/lib/librte_mbuf/rte_mbuf_dyn.c
+++ b/lib/librte_mbuf/rte_mbuf_dyn.c
@@ -67,13 +67,16 @@ process_score(void)
shm->free_space[i] = 1;
}
- for (off = 0; off < sizeof(struct rte_mbuf); off++) {
+ off = 0;
+ while (off < sizeof(struct rte_mbuf)) {
/* get the size of the free zone */
for (size = 0; (off + size) < sizeof(struct rte_mbuf) &&
shm->free_space[off + size]; size++)
;
- if (size == 0)
+ if (size == 0) {
+ off++;
continue;
+ }
/* get the alignment of biggest object that can fit in
* the zone at this offset.
@@ -84,8 +87,10 @@ process_score(void)
;
/* save it in free_space[] */
- for (i = off; i < off + size; i++)
+ for (i = off; i < off + align; i++)
shm->free_space[i] = RTE_MAX(align, shm->free_space[i]);
+
+ off += align;
}
}
--
2.17.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [dpdk-dev] [PATCH 3/5] mbuf: fix free_space setting for dynamic field
2020-06-13 15:49 ` [dpdk-dev] [PATCH 3/5] mbuf: fix free_space setting for dynamic field Xiaolong Ye
@ 2020-06-25 15:53 ` Olivier Matz
0 siblings, 0 replies; 11+ messages in thread
From: Olivier Matz @ 2020-06-25 15:53 UTC (permalink / raw
To: Xiaolong Ye; +Cc: Thomas Monjalon, Konstantin Ananyev, dev, stable
On Sat, Jun 13, 2020 at 11:49:19PM +0800, Xiaolong Ye wrote:
> The value free_space[i] is used to save the size of biggest aligned
> element that can fit in the zone, current implementation has one flaw,
> for example, if user registers dynfield1 (size = 4, align = 4, req = 124)
> first, the free_space would be as below after registration:
>
> 0070: 08 08 08 08 08 08 08 08
> 0078: 08 08 08 08 00 00 00 00
>
> Then if user continues to register dynfield2 (size = 4, align = 4),
> free_space would become:
>
> 0070: 00 00 00 00 04 04 04 04
> 0078: 04 04 04 04 00 00 00 00
>
> Further request dynfield3 (size = 8, align = 8) would fail to register
> due to alignment requirement can't be satisfied, though there is enough
> space remained in mbuf.
>
> This patch fixes above issue by saving alignment only in aligned zone,
> after the fix, above registrations order can be satisfied, free_space
> would be like:
>
> After dynfield1 registration:
>
> 0070: 08 08 08 08 08 08 08 08
> 0078: 04 04 04 04 00 00 00 00
>
> After dynfield2 registration:
>
> 0070: 08 08 08 08 08 08 08 08
> 0078: 00 00 00 00 00 00 00 00
>
> After dynfield3 registration:
>
> 0070: 00 00 00 00 00 00 00 00
> 0078: 00 00 00 00 00 00 00 00
>
> This patch also reduces iterations in process_score() by jumping align
> steps in each loop.
>
> Fixes: 4958ca3a443a ("mbuf: support dynamic fields and flags")
> Cc: stable@dpdk.org
>
> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
Acked-by: Olivier Matz <olivier.matz@6wind.com>
Thanks!
^ permalink raw reply [flat|nested] 11+ messages in thread
* [dpdk-dev] [PATCH 4/5] mbuf: fix a dynamic field dump log
2020-06-13 15:49 [dpdk-dev] [PATCH 0/5] small fixes for mbuf's dynamic field/flag feature Xiaolong Ye
` (2 preceding siblings ...)
2020-06-13 15:49 ` [dpdk-dev] [PATCH 3/5] mbuf: fix free_space setting for dynamic field Xiaolong Ye
@ 2020-06-13 15:49 ` Xiaolong Ye
2020-06-25 15:54 ` Olivier Matz
2020-06-13 15:49 ` [dpdk-dev] [PATCH 5/5] mbuf: support to dump free_flags for dynamic flag Xiaolong Ye
2020-06-25 20:40 ` [dpdk-dev] [PATCH 0/5] small fixes for mbuf's dynamic field/flag feature Thomas Monjalon
5 siblings, 1 reply; 11+ messages in thread
From: Xiaolong Ye @ 2020-06-13 15:49 UTC (permalink / raw
To: Olivier Matz, Konstantin Ananyev, Thomas Monjalon
Cc: dev, Xiaolong Ye, stable
For each mbuf byte, free_space[i] == 0 means the space is occupied,
free_space[i] != 0 means space is free.
Fixes: 4958ca3a443a ("mbuf: support dynamic fields and flags")
Cc: stable@dpdk.org
Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
---
lib/librte_mbuf/rte_mbuf_dyn.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/librte_mbuf/rte_mbuf_dyn.c b/lib/librte_mbuf/rte_mbuf_dyn.c
index fd51e1b68e..f071651acf 100644
--- a/lib/librte_mbuf/rte_mbuf_dyn.c
+++ b/lib/librte_mbuf/rte_mbuf_dyn.c
@@ -552,7 +552,7 @@ void rte_mbuf_dyn_dump(FILE *out)
dynflag->params.name, dynflag->bitnum,
dynflag->params.flags);
}
- fprintf(out, "Free space in mbuf (0 = free, value = zone alignment):\n");
+ fprintf(out, "Free space in mbuf (0 = occupied, value = free zone alignment):\n");
for (i = 0; i < sizeof(struct rte_mbuf); i++) {
if ((i % 8) == 0)
fprintf(out, " %4.4zx: ", i);
--
2.17.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [dpdk-dev] [PATCH 5/5] mbuf: support to dump free_flags for dynamic flag
2020-06-13 15:49 [dpdk-dev] [PATCH 0/5] small fixes for mbuf's dynamic field/flag feature Xiaolong Ye
` (3 preceding siblings ...)
2020-06-13 15:49 ` [dpdk-dev] [PATCH 4/5] mbuf: fix a dynamic field dump log Xiaolong Ye
@ 2020-06-13 15:49 ` Xiaolong Ye
2020-06-25 15:55 ` Olivier Matz
2020-06-25 20:40 ` [dpdk-dev] [PATCH 0/5] small fixes for mbuf's dynamic field/flag feature Thomas Monjalon
5 siblings, 1 reply; 11+ messages in thread
From: Xiaolong Ye @ 2020-06-13 15:49 UTC (permalink / raw
To: Olivier Matz; +Cc: dev, Xiaolong Ye
Add support to dump free_flags as below format:
Free bit in mbuf->ol_flags (0 = occupied, 1 = free):
0000: 0 0 0 0 0 0 0 0
0008: 0 0 0 0 0 0 0 0
0010: 0 0 0 0 0 0 0 1
0018: 1 1 1 1 1 1 1 1
0020: 1 1 1 1 1 1 1 1
0028: 1 0 0 0 0 0 0 0
0030: 0 0 0 0 0 0 0 0
0038: 0 0 0 0 0 0 0 0
Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
---
lib/librte_mbuf/rte_mbuf_dyn.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/lib/librte_mbuf/rte_mbuf_dyn.c b/lib/librte_mbuf/rte_mbuf_dyn.c
index f071651acf..538a43f695 100644
--- a/lib/librte_mbuf/rte_mbuf_dyn.c
+++ b/lib/librte_mbuf/rte_mbuf_dyn.c
@@ -559,5 +559,13 @@ void rte_mbuf_dyn_dump(FILE *out)
fprintf(out, "%2.2x%s", shm->free_space[i],
(i % 8 != 7) ? " " : "\n");
}
+ fprintf(out, "Free bit in mbuf->ol_flags (0 = occupied, 1 = free):\n");
+ for (i = 0; i < sizeof(uint64_t) * CHAR_BIT; i++) {
+ if ((i % 8) == 0)
+ fprintf(out, " %4.4zx: ", i);
+ fprintf(out, "%1.1x%s", (shm->free_flags & (1ULL << i)) ? 1 : 0,
+ (i % 8 != 7) ? " " : "\n");
+ }
+
rte_mcfg_tailq_write_unlock();
}
--
2.17.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [dpdk-dev] [PATCH 0/5] small fixes for mbuf's dynamic field/flag feature
2020-06-13 15:49 [dpdk-dev] [PATCH 0/5] small fixes for mbuf's dynamic field/flag feature Xiaolong Ye
` (4 preceding siblings ...)
2020-06-13 15:49 ` [dpdk-dev] [PATCH 5/5] mbuf: support to dump free_flags for dynamic flag Xiaolong Ye
@ 2020-06-25 20:40 ` Thomas Monjalon
5 siblings, 0 replies; 11+ messages in thread
From: Thomas Monjalon @ 2020-06-25 20:40 UTC (permalink / raw
To: Xiaolong Ye; +Cc: dev, olivier.matz
13/06/2020 17:49, Xiaolong Ye:
> This series contains some small fixes and enhancement for mbuf's
> dynamic field/flag feature.
>
> Xiaolong Ye (5):
> mbuf: fix out-of-bounds access at dyn field register
> mbuf: fix missing errno for dyn field/flag registration
> mbuf: fix free_space setting for dynamic field
> mbuf: fix a dynamic field dump log
> mbuf: support to dump free_flags for dynamic flag
>
> lib/librte_mbuf/rte_mbuf_dyn.c | 34 +++++++++++++++++++++++++++-------
> 1 file changed, 27 insertions(+), 7 deletions(-)
Applied, thanks a lot for the nice fixes
^ permalink raw reply [flat|nested] 11+ messages in thread