On 07/29/16 16:46, Oriol Arcas wrote:
After studying the code, is it possible that the patch is still correct?
The pool->queue_num_tbl[] array is different than the indexes contained in
this array: the index *of* the array (queue number) should be 0-based; the
indexes contained *in* the array, are 1-based, I think.
Oriol
shouldn't _odp_pkt_queue_create() return value before increment?
Maxim.
--
Oriol Arcas
Software Engineer
Starflow Networks
On Fri, Jul 29, 2016 at 3:31 PM, Oriol Arcas <[email protected]>
wrote:
I see. Then, if the 1-based _odp_int_pkt_queue_t is used as index to
access pool->queue_num_tbl, and the 0 block is discarded, the malloc should
allocate max + 1, right?
--
Oriol Arcas
Software Engineer
Starflow Networks
On Thu, Jul 28, 2016 at 10:34 PM, Bill Fischofer <
[email protected]> wrote:
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
