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