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