diff mbox

[dpdk-dev] lib/librte_table: Fix table array lookup

Message ID 20141213010638.28002.63966.stgit@localhost.localdomain (mailing list archive)
State Rejected, archived
Headers

Commit Message

Mark Wunderlich Dec. 13, 2014, 1:06 a.m. UTC 
  The existing lookup function was returning an unmodified
pkts_mask bitmask into lookup_hit_mask.  This effectively
assumes that all packets would index correctly into one
of the array table entries.

Also, there was no check that the metadata provided index
value was within range of the table max entries.  By using
using table index bitmask on the metadata provided index
the resulting entry position may falsely indicate a hit
for index values provided that happen to be greter than
the number of table entries.

Like other table type lookup functions it would seem that
the possibility exists that some of the packets provided
to the function would not result in a hit.  It is assumed
that the metadata provided should be a direct index into
the array table.  So, code was added to build and return
a bitmask for only those packets that correctly index
directly into the table array.

If the original intent for this table type was to accept
any 32-bit value, then by applying the table index bitmask
as a modulo index for distribution across table entries,
then this patch would be invalid and should be rejected.

Signed-off-by: Mark Wunderlich <mark.w.wunderlich@intel.com>
---
 lib/librte_table/rte_table_array.c |   25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

Comments

Thomas Monjalon Dec. 16, 2014, 5:59 p.m. UTC | #1 
Cristian, this patch is about packet framework.
Could you review it please?

2014-12-12 17:06, Mark Wunderlich:
> The existing lookup function was returning an unmodified
> pkts_mask bitmask into lookup_hit_mask.  This effectively
> assumes that all packets would index correctly into one
> of the array table entries.
> 
> Also, there was no check that the metadata provided index
> value was within range of the table max entries.  By using
> using table index bitmask on the metadata provided index
> the resulting entry position may falsely indicate a hit
> for index values provided that happen to be greter than
> the number of table entries.
> 
> Like other table type lookup functions it would seem that
> the possibility exists that some of the packets provided
> to the function would not result in a hit.  It is assumed
> that the metadata provided should be a direct index into
> the array table.  So, code was added to build and return
> a bitmask for only those packets that correctly index
> directly into the table array.
> 
> If the original intent for this table type was to accept
> any 32-bit value, then by applying the table index bitmask
> as a modulo index for distribution across table entries,
> then this patch would be invalid and should be rejected.
> 
> Signed-off-by: Mark Wunderlich <mark.w.wunderlich@intel.com>
> ---
>  lib/librte_table/rte_table_array.c |   25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
diff mbox

Patch

diff --git a/lib/librte_table/rte_table_array.c b/lib/librte_table/rte_table_array.c
index c031070..0164d18 100644
--- a/lib/librte_table/rte_table_array.c
+++ b/lib/librte_table/rte_table_array.c
@@ -164,8 +164,7 @@  rte_table_array_lookup(
 	void **entries)
 {
 	struct rte_table_array *t = (struct rte_table_array *) table;
-
-	*lookup_hit_mask = pkts_mask;
+	uint64_t pkts_out_mask = 0;
 
 	if ((pkts_mask & (pkts_mask + 1)) == 0) {
 		uint64_t n_pkts = __builtin_popcountll(pkts_mask);
@@ -173,26 +172,32 @@  rte_table_array_lookup(
 
 		for (i = 0; i < n_pkts; i++) {
 			struct rte_mbuf *pkt = pkts[i];
-			uint32_t entry_pos = RTE_MBUF_METADATA_UINT32(pkt,
-				t->offset) & t->entry_pos_mask;
+			uint32_t entry_pos = RTE_MBUF_METADATA_UINT32(pkt,t->offset);
 
-			entries[i] = (void *) &t->array[entry_pos *
-				t->entry_size];
+			if (entry_pos < t->n_entries) {
+				entries[i] = (void *) &t->array[entry_pos *
+						t->entry_size];
+				pkts_out_mask |= (1LLU << i);
+			}
 		}
 	} else {
 		for ( ; pkts_mask; ) {
 			uint32_t pkt_index = __builtin_ctzll(pkts_mask);
 			uint64_t pkt_mask = 1LLU << pkt_index;
 			struct rte_mbuf *pkt = pkts[pkt_index];
-			uint32_t entry_pos = RTE_MBUF_METADATA_UINT32(pkt,
-				t->offset) & t->entry_pos_mask;
+			uint32_t entry_pos = RTE_MBUF_METADATA_UINT32(pkt,t->offset);
 
-			entries[pkt_index] = (void *) &t->array[entry_pos *
-				t->entry_size];
+			if (entry_pos < t->n_entries) {
+				entries[pkt_index] = (void *) &t->array[entry_pos *
+							t->entry_size];
+				pkts_out_mask |= pkt_mask;
+			}
 			pkts_mask &= ~pkt_mask;
 		}
 	}
 
+	*lookup_hit_mask = pkts_out_mask;
+
 	return 0;
 }