[PATCH 0/3] lpm allocation fixes - v2

[PATCH 0/3] lpm allocation fixes - v2

[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:

*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/3] lpm6: fix use after free of lpm in rte_lpm6_create
[PATCH 2/3] lpm6: fix missing free of rules_tbl and lpm
[PATCH 3/3] lpm: fix missing free of lpm

 rte_lpm.c  |    8 ++------
 rte_lpm6.c |   11 +++++------
 2 files changed, 7 insertions(+), 12 deletions(-)

[PATCH 1/3] 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

[PATCH 2/3] 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 | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/lib/librte_lpm/rte_lpm6.c b/lib/librte_lpm/rte_lpm6.c
index 48931cc..4c44cd7 100644
--- a/lib/librte_lpm/rte_lpm6.c
+++ b/lib/librte_lpm/rte_lpm6.c
@@ -278,15 +278,13 @@ 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

[PATCH 3/3] 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 | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
index ccaaa2a..d5fa1f8 100644
--- a/lib/librte_lpm/rte_lpm.c
+++ b/lib/librte_lpm/rte_lpm.c
@@ -360,12 +360,8 @@ 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;
-	}
-
-	TAILQ_REMOVE(lpm_list, te, next);
+	if (te != NULL)
+		TAILQ_REMOVE(lpm_list, te, next);
 
 	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
 
-- 
2.7.0

Re: [PATCH 3/3] lpm: fix missing free of lpm

[Olivier MATZ]

Hi Christian,

On 03/16/2016 01:33 PM, Christian Ehrhardt wrote:
> 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 | 8 ++------
>   1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
> index ccaaa2a..d5fa1f8 100644
> --- a/lib/librte_lpm/rte_lpm.c
> +++ b/lib/librte_lpm/rte_lpm.c
> @@ -360,12 +360,8 @@ 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;
> -	}
> -
> -	TAILQ_REMOVE(lpm_list, te, next);
> +	if (te != NULL)
> +		TAILQ_REMOVE(lpm_list, te, next);
>
>   	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
>
>

I've just seen you had already posted a series on this topic.
It looks that some free() are missing in lpm.c:

Could you please check my version of the patch (which was not as
complete as your series)?
http://dpdk.org/dev/patchwork/patch/11526/

Regards,
Olivier

Re: [PATCH 3/3] lpm: fix missing free of lpm

[Christian Ehrhardt]

Hi,
looking at it I think we have intersections but also parts of yours that I
missed.
More than that while applying your changes I found other potential
use-after free cases.

I'll wrap that all up together in a v3 of my series.

Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd

On Wed, Mar 16, 2016 at 2:14 PM, Olivier MATZ <olivier.matz@6wind.com>
wrote:

> Hi Christian,
>
> On 03/16/2016 01:33 PM, Christian Ehrhardt wrote:
>
>> 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 | 8 ++------
>>   1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
>> index ccaaa2a..d5fa1f8 100644
>> --- a/lib/librte_lpm/rte_lpm.c
>> +++ b/lib/librte_lpm/rte_lpm.c
>> @@ -360,12 +360,8 @@ 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;
>> -       }
>> -
>> -       TAILQ_REMOVE(lpm_list, te, next);
>> +       if (te != NULL)
>> +               TAILQ_REMOVE(lpm_list, te, next);
>>
>>         rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
>>
>>
>>
> I've just seen you had already posted a series on this topic.
> It looks that some free() are missing in lpm.c:
>
> Could you please check my version of the patch (which was not as
> complete as your series)?
> http://dpdk.org/dev/patchwork/patch/11526/
>
> Regards,
> Olivier
>

[PATCH v4 0/5] lpm allocation fixes

[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 v4*
- re-removing the { } on single line ifs accidentially droped in v3
- adding the ack of Oliver Matz

*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

Christian Ehrhardt (5):
  lpm6: fix use after free of lpm in rte_lpm6_create
  lpm6: fix missing free of rules_tbl and lpm
  lpm: fix missing free of lpm
  lpm: fix use after free of lpm in rte_lpm_create*
  lpm: fix missing free of rules_tbl and lpm in rte_lpm_free*

 lib/librte_lpm/rte_lpm.c  | 24 ++++++++++--------------
 lib/librte_lpm/rte_lpm6.c | 11 +++++------
 2 files changed, 15 insertions(+), 20 deletions(-)

-- 
2.7.3

[PATCH v4 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>
Acked-by: Olivier Matz <olivier.matz@6wind.com>
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.3

[PATCH v4 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>
Acked-by: Olivier Matz <olivier.matz@6wind.com>
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
---
 lib/librte_lpm/rte_lpm6.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/lib/librte_lpm/rte_lpm6.c b/lib/librte_lpm/rte_lpm6.c
index 48931cc..4c44cd7 100644
--- a/lib/librte_lpm/rte_lpm6.c
+++ b/lib/librte_lpm/rte_lpm6.c
@@ -278,15 +278,13 @@ 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.3

[PATCH v4 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>
Acked-by: Olivier Matz <olivier.matz@6wind.com>
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
---
 lib/librte_lpm/rte_lpm.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
index ccaaa2a..d5fa1f8 100644
--- a/lib/librte_lpm/rte_lpm.c
+++ b/lib/librte_lpm/rte_lpm.c
@@ -360,12 +360,8 @@ 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;
-	}
-
-	TAILQ_REMOVE(lpm_list, te, next);
+	if (te != NULL)
+		TAILQ_REMOVE(lpm_list, te, next);
 
 	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
 
-- 
2.7.3

[PATCH v4 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.

Acked-by: Olivier Matz <olivier.matz@6wind.com>
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 d5fa1f8..59ce5a7 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.3

[PATCH v4 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.

Acked-by: Olivier Matz <olivier.matz@6wind.com>
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 59ce5a7..af5811c 100644
--- a/lib/librte_lpm/rte_lpm.c
+++ b/lib/librte_lpm/rte_lpm.c
@@ -367,6 +367,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);
 }
@@ -391,15 +392,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.3

Re: [PATCH v4 0/5] lpm allocation fixes

[Thomas Monjalon]

2016-03-21 15:06, 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 v4*
> - re-removing the { } on single line ifs accidentially droped in v3
> - adding the ack of Oliver Matz
> 
> *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
> 
> Christian Ehrhardt (5):
>   lpm6: fix use after free of lpm in rte_lpm6_create
>   lpm6: fix missing free of rules_tbl and lpm
>   lpm: fix missing free of lpm
>   lpm: fix use after free of lpm in rte_lpm_create*
>   lpm: fix missing free of rules_tbl and lpm in rte_lpm_free*

Applied, thanks