[dpdk-dev] lib/lpm:fix two issues in the delete_depth_small()

  Fix two issues in the delete_depth_small() function.
 
1> The control is not strict in this function.
 
In the following structure,
struct rte_lpm_tbl24_entry {
        union {
                uint8_t next_hop;
                uint8_t tbl8_gindex;
        };
     uint8_t ext_entry :1;
}
 
When ext_entry = 0, use next_hop.only to process rte_lpm_tbl24_entry.
 
When ext_entry = 1, use tbl8_gindex to process the rte_lpm_tbl8_entry.
 
When using LPM24 + 8 algorithm, it will use ext_entry to decide to process rte_lpm_tbl24_entry structure or rte_lpm_tbl8_entry structure. 
If a route is deleted, the prefix of previous route is used to override the deleted route. when (lpm->tbl24[i].ext_entry == 0 && lpm->tbl24[i].depth > depth) 
it should be ignored, but due to the incorrect logic, the next_hop is used as tbl8_gindex and will process the rte_lpm_tbl8_entry.
 
2> Initialization of rte_lpm_tbl8_entry is incorrect in this function 
 
In this function, use new rte_lpm_tbl8_entry we call A to replace the old rte_lpm_tbl8_entry. But the valid_group do not set VALID, so it will be INVALID.
Then when adding a new route which depth is > 24,the tbl8_alloc() function will search the rte_lpm_tbl8_entrys to find INVALID valid_group, 
and it will return the A to the add_depth_big function, so A's data is overridden.

Signed-off-by: NaNa <nana.nn@alibaba-inc.com>

---
 lib/librte_lpm/rte_lpm.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)
  

Acked by：<fengsong> yuke.hyk@alibaba-inc.com

-----邮件原件-----
发件人: nana.nn [mailto:nana.nn@alibaba-inc.com] 
发送时间: 2015年10月28日 12:02
收件人: "洪余柯(风松)"
抄送: dev@dpdk.org; Jijiang Liu
主题: Re: [PATCH] lib/lpm:fix two issues in the delete_depth_small()

HI:
     yuke，please acked-by~ 

On Oct 28, 2015, at 11:44 AM, Jijiang Liu <jijiang.liu@intel.com> wrote:

> Fix two issues in the delete_depth_small() function.
> 
> 1> The control is not strict in this function.
> 
> In the following structure,
> struct rte_lpm_tbl24_entry {
>        union {
>                uint8_t next_hop;
>                uint8_t tbl8_gindex;
>        };
>     uint8_t ext_entry :1;
> }
> 
> When ext_entry = 0, use next_hop.only to process rte_lpm_tbl24_entry.
> 
> When ext_entry = 1, use tbl8_gindex to process the rte_lpm_tbl8_entry.
> 
> When using LPM24 + 8 algorithm, it will use ext_entry to decide to process rte_lpm_tbl24_entry structure or rte_lpm_tbl8_entry structure. 
> If a route is deleted, the prefix of previous route is used to override the deleted route. when (lpm->tbl24[i].ext_entry == 0 && lpm->tbl24[i].depth > depth) 
> it should be ignored, but due to the incorrect logic, the next_hop is used as tbl8_gindex and will process the rte_lpm_tbl8_entry.
> 
> 2> Initialization of rte_lpm_tbl8_entry is incorrect in this function 
> 
> In this function, use new rte_lpm_tbl8_entry we call A to replace the old rte_lpm_tbl8_entry. But the valid_group do not set VALID, so it will be INVALID.
> Then when adding a new route which depth is > 24,the tbl8_alloc() function will search the rte_lpm_tbl8_entrys to find INVALID valid_group, 
> and it will return the A to the add_depth_big function, so A's data is overridden.
> 
> Signed-off-by: NaNa <nana.nn@alibaba-inc.com>
> 
> ---
> lib/librte_lpm/rte_lpm.c |    7 +++----
> 1 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
> index 163ba3c..3981452 100644
> --- a/lib/librte_lpm/rte_lpm.c
> +++ b/lib/librte_lpm/rte_lpm.c
> @@ -734,8 +734,7 @@ delete_depth_small(struct rte_lpm *lpm, uint32_t ip_masked,
> 			if (lpm->tbl24[i].ext_entry == 0 &&
> 					lpm->tbl24[i].depth <= depth ) {
> 				lpm->tbl24[i].valid = INVALID;
> -			}
> -			else {
> +			} else if (lpm->tbl24[i].ext_entry == 1) {
> 				/*
> 				 * If TBL24 entry is extended, then there has
> 				 * to be a rule with depth >= 25 in the
> @@ -770,6 +769,7 @@ delete_depth_small(struct rte_lpm *lpm, uint32_t ip_masked,
> 
> 		struct rte_lpm_tbl8_entry new_tbl8_entry = {
> 			.valid = VALID,
> +			.valid_group = VALID,
> 			.depth = sub_rule_depth,
> 			.next_hop = lpm->rules_tbl
> 			[sub_rule_index].next_hop,
> @@ -780,8 +780,7 @@ delete_depth_small(struct rte_lpm *lpm, uint32_t ip_masked,
> 			if (lpm->tbl24[i].ext_entry == 0 &&
> 					lpm->tbl24[i].depth <= depth ) {
> 				lpm->tbl24[i] = new_tbl24_entry;
> -			}
> -			else {
> +			} else  if (lpm->tbl24[i].ext_entry == 1) {
> 				/*
> 				 * If TBL24 entry is extended, then there has
> 				 * to be a rule with depth >= 25 in the
> -- 
> 1.7.7.6
  

On Wed, Oct 28, 2015 at 11:44:15AM +0800, Jijiang Liu wrote:
> Fix two issues in the delete_depth_small() function.
>  
> 1> The control is not strict in this function.
>  
> In the following structure,
> struct rte_lpm_tbl24_entry {
>         union {
>                 uint8_t next_hop;
>                 uint8_t tbl8_gindex;
>         };
>      uint8_t ext_entry :1;
> }
>  
> When ext_entry = 0, use next_hop.only to process rte_lpm_tbl24_entry.
>  
> When ext_entry = 1, use tbl8_gindex to process the rte_lpm_tbl8_entry.
>  
> When using LPM24 + 8 algorithm, it will use ext_entry to decide to process rte_lpm_tbl24_entry structure or rte_lpm_tbl8_entry structure. 
> If a route is deleted, the prefix of previous route is used to override the deleted route. when (lpm->tbl24[i].ext_entry == 0 && lpm->tbl24[i].depth > depth) 
> it should be ignored, but due to the incorrect logic, the next_hop is used as tbl8_gindex and will process the rte_lpm_tbl8_entry.
>  
> 2> Initialization of rte_lpm_tbl8_entry is incorrect in this function 
>  
> In this function, use new rte_lpm_tbl8_entry we call A to replace the old rte_lpm_tbl8_entry. But the valid_group do not set VALID, so it will be INVALID.
> Then when adding a new route which depth is > 24,the tbl8_alloc() function will search the rte_lpm_tbl8_entrys to find INVALID valid_group, 
> and it will return the A to the add_depth_big function, so A's data is overridden.
> 
> Signed-off-by: NaNa <nana.nn@alibaba-inc.com>
> 

Hi NaNa, Jijiang,

since this patch contains two separate fixes, it would be better split into
two separate patches, one for each fix. Also, please add a "Fixes" line to
the commit log.

Are there still plans for a unit test to demonstrate the bug(s) and make it easy
for us to verify the fix?

Regards,
/Bruce
  

On 10/28/2015 03:40 PM, Bruce Richardson wrote:
> On Wed, Oct 28, 2015 at 11:44:15AM +0800, Jijiang Liu wrote:
>> Fix two issues in the delete_depth_small() function.
>>  
>> 1> The control is not strict in this function.
>>  
>> In the following structure,
>> struct rte_lpm_tbl24_entry {
>>         union {
>>                 uint8_t next_hop;
>>                 uint8_t tbl8_gindex;
>>         };
>>      uint8_t ext_entry :1;
>> }
>>  
>> When ext_entry = 0, use next_hop.only to process rte_lpm_tbl24_entry.
>>  
>> When ext_entry = 1, use tbl8_gindex to process the rte_lpm_tbl8_entry.
>>  
>> When using LPM24 + 8 algorithm, it will use ext_entry to decide to process rte_lpm_tbl24_entry structure or rte_lpm_tbl8_entry structure. 
>> If a route is deleted, the prefix of previous route is used to override the deleted route. when (lpm->tbl24[i].ext_entry == 0 && lpm->tbl24[i].depth > depth) 
>> it should be ignored, but due to the incorrect logic, the next_hop is used as tbl8_gindex and will process the rte_lpm_tbl8_entry.
>>  
>> 2> Initialization of rte_lpm_tbl8_entry is incorrect in this function 
>>  
>> In this function, use new rte_lpm_tbl8_entry we call A to replace the old rte_lpm_tbl8_entry. But the valid_group do not set VALID, so it will be INVALID.
>> Then when adding a new route which depth is > 24,the tbl8_alloc() function will search the rte_lpm_tbl8_entrys to find INVALID valid_group, 
>> and it will return the A to the add_depth_big function, so A's data is overridden.
>>
>> Signed-off-by: NaNa <nana.nn@alibaba-inc.com>
>>
> Hi NaNa, Jijiang,
>
> since this patch contains two separate fixes, it would be better split into
> two separate patches, one for each fix. Also, please add a "Fixes" line to
> the commit log.
>
> Are there still plans for a unit test to demonstrate the bug(s) and make it easy
> for us to verify the fix?
>
> Regards,
> /Bruce
Hello,

It's the same fix as the one sent here (which contained some tests,
maybe we can use them ?)
http://dpdk.org/ml/archives/dev/2015-October/025871.html .
For what is worth, we are using those fix at my company and they are
fixing the described bug.
  

On Wed, Oct 28, 2015 at 05:55:59PM +0100, Nikita Kozlov wrote:
> On 10/28/2015 03:40 PM, Bruce Richardson wrote:
> > On Wed, Oct 28, 2015 at 11:44:15AM +0800, Jijiang Liu wrote:
> >> Fix two issues in the delete_depth_small() function.
> >>  
> >> 1> The control is not strict in this function.
> >>  
> >> In the following structure,
> >> struct rte_lpm_tbl24_entry {
> >>         union {
> >>                 uint8_t next_hop;
> >>                 uint8_t tbl8_gindex;
> >>         };
> >>      uint8_t ext_entry :1;
> >> }
> >>  
> >> When ext_entry = 0, use next_hop.only to process rte_lpm_tbl24_entry.
> >>  
> >> When ext_entry = 1, use tbl8_gindex to process the rte_lpm_tbl8_entry.
> >>  
> >> When using LPM24 + 8 algorithm, it will use ext_entry to decide to process rte_lpm_tbl24_entry structure or rte_lpm_tbl8_entry structure. 
> >> If a route is deleted, the prefix of previous route is used to override the deleted route. when (lpm->tbl24[i].ext_entry == 0 && lpm->tbl24[i].depth > depth) 
> >> it should be ignored, but due to the incorrect logic, the next_hop is used as tbl8_gindex and will process the rte_lpm_tbl8_entry.
> >>  
> >> 2> Initialization of rte_lpm_tbl8_entry is incorrect in this function 
> >>  
> >> In this function, use new rte_lpm_tbl8_entry we call A to replace the old rte_lpm_tbl8_entry. But the valid_group do not set VALID, so it will be INVALID.
> >> Then when adding a new route which depth is > 24,the tbl8_alloc() function will search the rte_lpm_tbl8_entrys to find INVALID valid_group, 
> >> and it will return the A to the add_depth_big function, so A's data is overridden.
> >>
> >> Signed-off-by: NaNa <nana.nn@alibaba-inc.com>
> >>
> > Hi NaNa, Jijiang,
> >
> > since this patch contains two separate fixes, it would be better split into
> > two separate patches, one for each fix. Also, please add a "Fixes" line to
> > the commit log.
> >
> > Are there still plans for a unit test to demonstrate the bug(s) and make it easy
> > for us to verify the fix?
> >
> > Regards,
> > /Bruce
> Hello,
> 
> It's the same fix as the one sent here (which contained some tests,
> maybe we can use them ?)
> http://dpdk.org/ml/archives/dev/2015-October/025871.html .
> For what is worth, we are using those fix at my company and they are
> fixing the described bug.
> 
Ok, great, so there are tests available. Unfortunately, the previous patches
haven't come through correctly, for example, see the tests patch in patchwork:
http://dpdk.org/dev/patchwork/patch/7934/
Given that the fix appears to work for you, it's something we need to get into
the release with or without tests, but with cleaned up tests would be better,
obviously. :-)

/Bruce
  

On Thu, Oct 29, 2015 at 10:41:45AM +0800, nana.nn wrote:
> Hi Bruce:
> 	Should I send the test unit as a DPDK patch, or just the program for you to demonstrate the bugs? 
>         
> 	
> 	Thank you very much!
> 
> 
> Regards
> 
> Na Na
> 

A patch to add a unit test for the bug would be best.
/Bruce

> 
> 
> 
> On Oct 28, 2015, at 10:40 PM, Bruce Richardson <bruce.richardson@intel.com> wrote:
> 
> > On Wed, Oct 28, 2015 at 11:44:15AM +0800, Jijiang Liu wrote:
> >> Fix two issues in the delete_depth_small() function.
> >> 
> >> 1> The control is not strict in this function.
> >> 
> >> In the following structure,
> >> struct rte_lpm_tbl24_entry {
> >>        union {
> >>                uint8_t next_hop;
> >>                uint8_t tbl8_gindex;
> >>        };
> >>     uint8_t ext_entry :1;
> >> }
> >> 
> >> When ext_entry = 0, use next_hop.only to process rte_lpm_tbl24_entry.
> >> 
> >> When ext_entry = 1, use tbl8_gindex to process the rte_lpm_tbl8_entry.
> >> 
> >> When using LPM24 + 8 algorithm, it will use ext_entry to decide to process rte_lpm_tbl24_entry structure or rte_lpm_tbl8_entry structure. 
> >> If a route is deleted, the prefix of previous route is used to override the deleted route. when (lpm->tbl24[i].ext_entry == 0 && lpm->tbl24[i].depth > depth) 
> >> it should be ignored, but due to the incorrect logic, the next_hop is used as tbl8_gindex and will process the rte_lpm_tbl8_entry.
> >> 
> >> 2> Initialization of rte_lpm_tbl8_entry is incorrect in this function 
> >> 
> >> In this function, use new rte_lpm_tbl8_entry we call A to replace the old rte_lpm_tbl8_entry. But the valid_group do not set VALID, so it will be INVALID.
> >> Then when adding a new route which depth is > 24,the tbl8_alloc() function will search the rte_lpm_tbl8_entrys to find INVALID valid_group, 
> >> and it will return the A to the add_depth_big function, so A's data is overridden.
> >> 
> >> Signed-off-by: NaNa <nana.nn@alibaba-inc.com>
> >> 
> > 
> > Hi NaNa, Jijiang,
> > 
> > since this patch contains two separate fixes, it would be better split into
> > two separate patches, one for each fix. Also, please add a "Fixes" line to
> > the commit log.
> > 
> > Are there still plans for a unit test to demonstrate the bug(s) and make it easy
> > for us to verify the fix?
> > 
> > Regards,
> > /Bruce
>
  

HI Bruce:    How about this, I send you the test program , then you can demonstrate the bugs .    If you demonstrate the bugs ,then you can get the lpm bug's fix into the release first.        Further ,we explicitly discuss the details about the unit test and make it a patch of dpdk.    Thank you very much!
Regards
Na Na



    
------------------------------------------------------------------发件人：Bruce Richardson <bruce.richardson@intel.com>发送时间：2015年10月29日(星期四) 18:18收件人：那娜(恒月) <nana.nn@alibaba-inc.com>抄　送：Jijiang Liu <jijiang.liu@intel.com>，dev <dev@dpdk.org>主　题：Re: [dpdk-dev] [PATCH] lib/lpm:fix two issues in the delete_depth_small()
On Thu, Oct 29, 2015 at 10:41:45AM +0800, nana.nn wrote:
> Hi Bruce:
>  Should I send the test unit as a DPDK patch, or just the program for you to demonstrate the bugs? 
>         
>  
>  Thank you very much!
> 
> 
> Regards
> 
> Na Na
> 

A patch to add a unit test for the bug would be best.
/Bruce

> 
> 
> 
> On Oct 28, 2015, at 10:40 PM, Bruce Richardson <bruce.richardson@intel.com> wrote:
> 
> > On Wed, Oct 28, 2015 at 11:44:15AM +0800, Jijiang Liu wrote:
> >> Fix two issues in the delete_depth_small() function.
> >> 
> >> 1> The control is not strict in this function.
> >> 
> >> In the following structure,
> >> struct rte_lpm_tbl24_entry {
> >>        union {
> >>                uint8_t next_hop;
> >>                uint8_t tbl8_gindex;
> >>        };
> >>     uint8_t ext_entry :1;
> >> }
> >> 
> >> When ext_entry = 0, use next_hop.only to process rte_lpm_tbl24_entry.
> >> 
> >> When ext_entry = 1, use tbl8_gindex to process the rte_lpm_tbl8_entry.
> >> 
> >> When using LPM24 + 8 algorithm, it will use ext_entry to decide to process rte_lpm_tbl24_entry structure or rte_lpm_tbl8_entry structure. 
> >> If a route is deleted, the prefix of previous route is used to override the deleted route. when (lpm->tbl24[i].ext_entry == 0 && lpm->tbl24[i].depth > depth) 
> >> it should be ignored, but due to the incorrect logic, the next_hop is used as tbl8_gindex and will process the rte_lpm_tbl8_entry.
> >> 
> >> 2> Initialization of rte_lpm_tbl8_entry is incorrect in this function 
> >> 
> >> In this function, use new rte_lpm_tbl8_entry we call A to replace the old rte_lpm_tbl8_entry. But the valid_group do not set VALID, so it will be INVALID.
> >> Then when adding a new route which depth is > 24,the tbl8_alloc() function will search the rte_lpm_tbl8_entrys to find INVALID valid_group, 
> >> and it will return the A to the add_depth_big function, so A's data is overridden.
> >> 
> >> Signed-off-by: NaNa <nana.nn@alibaba-inc.com>
> >> 
> > 
> > Hi NaNa, Jijiang,
> > 
> > since this patch contains two separate fixes, it would be better split into
> > two separate patches, one for each fix. Also, please add a "Fixes" line to
> > the commit log.
> > 
> > Are there still plans for a unit test to demonstrate the bug(s) and make it easy
> > for us to verify the fix?
> > 
> > Regards,
> > /Bruce
>