I don't see any problems after applying this patch, however I'm wondering
if this is the correct fix to the problem because of this comment:
_odp_int_queue_pool_t _odp_queue_pool_create(uint32_t max_num_queues,
uint32_t max_queued_pkts)
{
queue_pool_t *pool;
uint32_t idx, initial_free_list_size, malloc_len, first_queue_blk_idx;
int rc;
pool = malloc(sizeof(queue_pool_t));
memset(pool, 0, sizeof(queue_pool_t));
/* Initialize the queue_blk_tbl_sizes array based upon the
* max_queued_pkts.
*/
max_queued_pkts = MAX(max_queued_pkts, 64 * 1024);
queue_region_desc_init(pool, 0, max_queued_pkts / 4);
queue_region_desc_init(pool, 1, max_queued_pkts / 64);
queue_region_desc_init(pool, 2, max_queued_pkts / 64);
queue_region_desc_init(pool, 3, max_queued_pkts / 64);
queue_region_desc_init(pool, 4, max_queued_pkts / 64);
for (idx = 5; idx < 16; idx++)
queue_region_desc_init(pool, idx, max_queued_pkts / 16);
/* Now allocate the first queue_blk_tbl and add its blks to the
free
* list. Replenish the queue_blk_t free list.
*/
initial_free_list_size = MIN(64 * 1024, max_queued_pkts / 4);
rc = pkt_queue_free_list_add(pool, initial_free_list_size);
if (rc < 0)
return _ODP_INT_QUEUE_POOL_INVALID;
/* Discard the first queue blk with idx 0 */ <== Note!
queue_blk_alloc(pool, &first_queue_blk_idx);
pool->max_queue_num = max_num_queues;
pool->max_queued_pkts = max_queued_pkts;
pool->next_queue_num = 1;
....
}
It appears the code understands that blocks have a 0 origin but it's
intentionally not using block 0. A switch to 0-indexed addressing would
mean that block 0 would be used even though it's been discarded via a
previous queue_blk_alloc().
So it seems given that block 0 is being discarded the correct way to
avoid the overruns is to check at the other end of the array and change
those if (pool->max_queue_num < queue_num) tests into if
(pool->max_queue_num <= queue_num) tests so that the maximum index that
will be dereferenced is max_queue_num - 1.
Barry: Any opinions on this one?
On Thu, Jul 28, 2016 at 1:28 PM, Oriol Arcas <[email protected]>
wrote:
Signed-off-by: Oriol Arcas <[email protected]>
---
platform/linux-generic/odp_pkt_queue.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/platform/linux-generic/odp_pkt_queue.c
b/platform/linux-generic/odp_pkt_queue.c
index 7734ee9..be638a3 100644
--- a/platform/linux-generic/odp_pkt_queue.c
+++ b/platform/linux-generic/odp_pkt_queue.c
@@ -263,13 +263,13 @@ int _odp_pkt_queue_append(_odp_int_queue_pool_t
queue_pool,
return -3;
pool->total_pkt_appends++;
- first_blk_idx = pool->queue_num_tbl[queue_num];
+ first_blk_idx = pool->queue_num_tbl[queue_num - 1];
if (first_blk_idx == 0) {
first_blk = queue_blk_alloc(pool, &first_blk_idx);
if (!first_blk)
return -1;
- pool->queue_num_tbl[queue_num] = first_blk_idx;
+ pool->queue_num_tbl[queue_num - 1] = first_blk_idx;
init_queue_blk(first_blk);
first_blk->pkts[0] = pkt;
return 0;
@@ -316,7 +316,7 @@ int _odp_pkt_queue_remove(_odp_int_queue_pool_t
queue_pool,
if ((queue_num == 0) || (pool->max_queue_num < queue_num))
return -2;
- first_blk_idx = pool->queue_num_tbl[queue_num];
+ first_blk_idx = pool->queue_num_tbl[queue_num - 1];
if (first_blk_idx == 0)
return 0; /* pkt queue is empty. */
@@ -344,7 +344,8 @@ int _odp_pkt_queue_remove(_odp_int_queue_pool_t
queue_pool,
first_blk->tail_queue_blk_idx;
}
- pool->queue_num_tbl[queue_num] =
next_blk_idx;
+ pool->queue_num_tbl[queue_num - 1] =
+ next_blk_idx;
queue_blk_free(pool, first_blk,
first_blk_idx);
}
--
1.9.1
