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

[Bruce Richardson]

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 at 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