[dpdk-dev] [PATCH 0/5] lpm allocation fixes - v3

[dpdk-dev] [PATCH 0/5] lpm allocation fixes - v3

[Christian Ehrhardt]

Poking a bit on autotest revealed a few shortcomings in the lpm allocation path.
Thanks to the feedback to the first revision of the patches here v2.
Also Oliver Matz spotted similar issues and made me aware - thanks!
Integrating them revealed even more use after free / leak issues.

*updates in v3*
- lpm create/free path for v20 and v1604 got the same fixes that were
  already identified for lpm6 before

*updates in v2*
- lpm/lpm6 patches split
- following dpdk coding guidelines regarding single line if's
- adding singed-off and acked-bys gathered so far
- combine all three related patches in one series

[PATCH 1/5] lpm6: fix use after free of lpm in rte_lpm6_create
[PATCH 2/5] lpm6: fix missing free of rules_tbl and lpm
[PATCH 3/5] lpm: fix missing free of lpm
[PATCH 4/5] lpm: fix use after free of lpm in rte_lpm_create*
[PATCH 5/5] lpm: fix missing free of rules_tbl and lpm in

diffstat:
 rte_lpm.c  |   23 ++++++++++-------------
 rte_lpm6.c |   12 ++++++------
 2 files changed, 16 insertions(+), 19 deletions(-)

[dpdk-dev] [PATCH 1/5] lpm6: fix use after free of lpm in rte_lpm6_create

[Christian Ehrhardt]

In certain autotests lpm->max_rules turned out to be non initialized.
That was caused by a failing allocation for lpm->rules_tbl in rte_lpm6_create.
It then left the function via goto exit with lpm freed, but still a pointer
value being set.

In case of an allocation failure it resets lpm to NULL now, to avoid the
upper layers operate on that already freed memory.
Along that is also makes the RTE_LOG message of the failed allocation unique.

Acked-by: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
---
 lib/librte_lpm/rte_lpm6.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/librte_lpm/rte_lpm6.c b/lib/librte_lpm/rte_lpm6.c
index 6c2b293..48931cc 100644
--- a/lib/librte_lpm/rte_lpm6.c
+++ b/lib/librte_lpm/rte_lpm6.c
@@ -206,8 +206,9 @@ rte_lpm6_create(const char *name, int socket_id,
 			(size_t)rules_size, RTE_CACHE_LINE_SIZE, socket_id);
 
 	if (lpm->rules_tbl == NULL) {
-		RTE_LOG(ERR, LPM, "LPM memory allocation failed\n");
+		RTE_LOG(ERR, LPM, "LPM rules_tbl allocation failed\n");
 		rte_free(lpm);
+		lpm = NULL;
 		rte_free(te);
 		goto exit;
 	}
-- 
2.7.0

[dpdk-dev] [PATCH 2/5] lpm6: fix missing free of rules_tbl and lpm

[Christian Ehrhardt]

lpm6 autotests failed with the default alloc of 512M Memory.
While >=2500M was a workaround it became clear while debugging that it
had a leak.
One could see a lot of output like:
  LPM Test tests6[i]: FAIL
  LPM: LPM memory allocation failed

It turned out that in rte_lpm6_free
- lpm might not be freed if it didn't find a te (early return)
- lpm->rules_tbl was not freed ever

Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
---
 lib/librte_lpm/rte_lpm6.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/lib/librte_lpm/rte_lpm6.c b/lib/librte_lpm/rte_lpm6.c
index 48931cc..5abfc78 100644
--- a/lib/librte_lpm/rte_lpm6.c
+++ b/lib/librte_lpm/rte_lpm6.c
@@ -278,15 +278,14 @@ rte_lpm6_free(struct rte_lpm6 *lpm)
 		if (te->data == (void *) lpm)
 			break;
 	}
-	if (te == NULL) {
-		rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
-		return;
-	}
 
-	TAILQ_REMOVE(lpm_list, te, next);
+	if (te != NULL) {
+		TAILQ_REMOVE(lpm_list, te, next);
+	}
 
 	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
 
+	rte_free(lpm->rules_tbl);
 	rte_free(lpm);
 	rte_free(te);
 }
-- 
2.7.0

[dpdk-dev] [PATCH 3/5] lpm: fix missing free of lpm

[Christian Ehrhardt]

Fixing lpm6 regarding a similar issue showed that that in rte_lpm_free lpm
might not be freed if it didn't find a te (early return)

Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
---
 lib/librte_lpm/rte_lpm.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
index ccaaa2a..2cc87b6 100644
--- a/lib/librte_lpm/rte_lpm.c
+++ b/lib/librte_lpm/rte_lpm.c
@@ -360,13 +360,10 @@ rte_lpm_free_v20(struct rte_lpm_v20 *lpm)
 		if (te->data == (void *) lpm)
 			break;
 	}
-	if (te == NULL) {
-		rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
-		return;
+	if (te != NULL) {
+		TAILQ_REMOVE(lpm_list, te, next);
 	}
 
-	TAILQ_REMOVE(lpm_list, te, next);
-
 	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
 
 	rte_free(lpm);
-- 
2.7.0

[dpdk-dev] [PATCH 4/5] lpm: fix use after free of lpm in rte_lpm_create*

[Christian Ehrhardt]

There were further chances for a use after free by returning an already freed
pointer in rte_lpm_create for v20 and v1604.
Along that is also makes the RTE_LOG messages of the failed allocations unique.

Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
---
 lib/librte_lpm/rte_lpm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
index 2cc87b6..d21c783 100644
--- a/lib/librte_lpm/rte_lpm.c
+++ b/lib/librte_lpm/rte_lpm.c
@@ -303,8 +303,9 @@ rte_lpm_create_v1604(const char *name, int socket_id,
 			(size_t)rules_size, RTE_CACHE_LINE_SIZE, socket_id);
 
 	if (lpm->rules_tbl == NULL) {
-		RTE_LOG(ERR, LPM, "LPM memory allocation failed\n");
+		RTE_LOG(ERR, LPM, "LPM rules_tbl memory allocation failed\n");
 		rte_free(lpm);
+		lpm = NULL;
 		rte_free(te);
 		goto exit;
 	}
@@ -313,8 +314,9 @@ rte_lpm_create_v1604(const char *name, int socket_id,
 			(size_t)tbl8s_size, RTE_CACHE_LINE_SIZE, socket_id);
 
 	if (lpm->tbl8 == NULL) {
-		RTE_LOG(ERR, LPM, "LPM memory allocation failed\n");
+		RTE_LOG(ERR, LPM, "LPM tbl8 memory allocation failed\n");
 		rte_free(lpm);
+		lpm = NULL;
 		rte_free(te);
 		goto exit;
 	}
-- 
2.7.0

[dpdk-dev] [PATCH 5/5] lpm: fix missing free of rules_tbl and lpm in rte_lpm_free*

[Christian Ehrhardt]

As found in rte_lpm6_free the two lpm interfaces rte_lpm_free_v20 and
rte_lpm_free_v1604 had a leak.

rte_lpm_free_v20 might have missed to free rules_tbl
rte_lpm_free_v1604 due to an early exit might have missed to free
rules_tbl and lpm itself.

Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
---
 lib/librte_lpm/rte_lpm.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
index d21c783..e8fe33e 100644
--- a/lib/librte_lpm/rte_lpm.c
+++ b/lib/librte_lpm/rte_lpm.c
@@ -368,6 +368,7 @@ rte_lpm_free_v20(struct rte_lpm_v20 *lpm)
 
 	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
 
+	rte_free(lpm->rules_tbl);
 	rte_free(lpm);
 	rte_free(te);
 }
@@ -392,15 +393,12 @@ rte_lpm_free_v1604(struct rte_lpm *lpm)
 		if (te->data == (void *) lpm)
 			break;
 	}
-	if (te == NULL) {
-		rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
-		return;
-	}
-
-	TAILQ_REMOVE(lpm_list, te, next);
+	if (te != NULL)
+		TAILQ_REMOVE(lpm_list, te, next);
 
 	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
 
+	rte_free(lpm->rules_tbl);
 	rte_free(lpm);
 	rte_free(te);
 }
-- 
2.7.0

Re: [dpdk-dev] [PATCH 0/5] lpm allocation fixes - v3

[Olivier Matz]

On 03/16/2016 03:16 PM, Christian Ehrhardt wrote:
> Poking a bit on autotest revealed a few shortcomings in the lpm allocation path.
> Thanks to the feedback to the first revision of the patches here v2.
> Also Oliver Matz spotted similar issues and made me aware - thanks!
> Integrating them revealed even more use after free / leak issues.
> 
> *updates in v3*
> - lpm create/free path for v20 and v1604 got the same fixes that were
>   already identified for lpm6 before
> 
> *updates in v2*
> - lpm/lpm6 patches split
> - following dpdk coding guidelines regarding single line if's
> - adding singed-off and acked-bys gathered so far
> - combine all three related patches in one series
> 
> [PATCH 1/5] lpm6: fix use after free of lpm in rte_lpm6_create
> [PATCH 2/5] lpm6: fix missing free of rules_tbl and lpm
> [PATCH 3/5] lpm: fix missing free of lpm
> [PATCH 4/5] lpm: fix use after free of lpm in rte_lpm_create*
> [PATCH 5/5] lpm: fix missing free of rules_tbl and lpm in
> 
> diffstat:
>  rte_lpm.c  |   23 ++++++++++-------------
>  rte_lpm6.c |   12 ++++++------
>  2 files changed, 16 insertions(+), 19 deletions(-)
> 

Series
Acked-by: Olivier Matz <olivier.matz@6wind.com>

Just one small comment: there are additional { } in patches
2/5 and 3/5.

Thomas, do you think you can remove it while pushing?

Re: [dpdk-dev] [PATCH 0/5] lpm allocation fixes - v3

[Christian Ehrhardt]

Hi Oliver,
thanks for the ack - I had these {} fixed in v2, but accidentially dropped
when merging our code.
v3 was flawed anyway as my submission was not a proper reply-to to the
older series.

This shall not be Thomas work to do, I'll resubmit a v4 re-adding the {}
fix and properly replying to the former v2 as it was intended but failed
for v3.
I also add your acked-by and I'm eager looking forward seeing the patches
pushed then.

Kind Regards,
Christian

Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd

On Mon, Mar 21, 2016 at 2:18 PM, Olivier Matz <olivier.matz@6wind.com>
wrote:

>
>
> On 03/16/2016 03:16 PM, Christian Ehrhardt wrote:
> > Poking a bit on autotest revealed a few shortcomings in the lpm
> allocation path.
> > Thanks to the feedback to the first revision of the patches here v2.
> > Also Oliver Matz spotted similar issues and made me aware - thanks!
> > Integrating them revealed even more use after free / leak issues.
> >
> > *updates in v3*
> > - lpm create/free path for v20 and v1604 got the same fixes that were
> >   already identified for lpm6 before
> >
> > *updates in v2*
> > - lpm/lpm6 patches split
> > - following dpdk coding guidelines regarding single line if's
> > - adding singed-off and acked-bys gathered so far
> > - combine all three related patches in one series
> >
> > [PATCH 1/5] lpm6: fix use after free of lpm in rte_lpm6_create
> > [PATCH 2/5] lpm6: fix missing free of rules_tbl and lpm
> > [PATCH 3/5] lpm: fix missing free of lpm
> > [PATCH 4/5] lpm: fix use after free of lpm in rte_lpm_create*
> > [PATCH 5/5] lpm: fix missing free of rules_tbl and lpm in
> >
> > diffstat:
> >  rte_lpm.c  |   23 ++++++++++-------------
> >  rte_lpm6.c |   12 ++++++------
> >  2 files changed, 16 insertions(+), 19 deletions(-)
> >
>
> Series
> Acked-by: Olivier Matz <olivier.matz@6wind.com>
>
> Just one small comment: there are additional { } in patches
> 2/5 and 3/5.
>
> Thomas, do you think you can remove it while pushing?
>
>
>