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