linux-kernel - Re: staging: ion: ION allocation fall back order depends on heap linkage order

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <58951af2-a84e-7d7c-e956-ece3190aa8c2@redhat.com>
Date:   Tue, 6 Feb 2018 15:48:01 -0800
From:   Laura Abbott <labbott@...hat.com>
To:     Alexey Skidanov <alexey.skidanov@...il.com>,
        devel@...verdev.osuosl.org
Cc:     linux-kernel@...r.kernel.org
Subject: Re: staging: ion: ION allocation fall back order depends on heap
 linkage order

On 01/28/2018 08:24 AM, Alexey Skidanov wrote:
> Hi,
> 
> According to my understanding, the allocation fall back order
> completely depends on heap->id that is assigned during the heap
> creation:
>     plist_for_each_entry(heap, &dev->heaps, node) {
>          /* if the caller didn't specify this heap id */
>          if (!((1 << heap->id) & heap_id_mask))
>              continue;
>          buffer = ion_buffer_create(heap, dev, len, flags);
>          if (!IS_ERR(buffer))
>              break;
>      }
> 
> On creation, each heap is added to the priority list according to the
> priority assigned:
> 
> ...
> static int heap_id;
> ...
> void ion_device_add_heap(struct ion_heap *heap)
> {
>          ...
>          heap->id = heap_id++;
>          ...
> }
> 
> 
> The order of creation is the order of linkage defined in the Makefile.
> Thus, by default, we have:
> 
> heap id 2, type ION_HEAP_TYPE_DMA
> heap id 1, type ION_HEAP_TYPE_SYSTEM
> heap id 0, type ION_HEAP_TYPE_SYSTEM_CONTIG
> 
> Changing the linkage order:
> diff --git a/drivers/staging/android/ion/Makefile
> b/drivers/staging/android/ion/Makefile
> index bb30bf8..e05052c 100644
> --- a/drivers/staging/android/ion/Makefile
> +++ b/drivers/staging/android/ion/Makefile
> @@ -1,6 +1,6 @@
>   # SPDX-License-Identifier: GPL-2.0
>   obj-$(CONFIG_ION) +=   ion.o ion-ioctl.o ion_heap.o
> -obj-$(CONFIG_ION_SYSTEM_HEAP) += ion_system_heap.o ion_page_pool.o
>   obj-$(CONFIG_ION_CARVEOUT_HEAP) += ion_carveout_heap.o
>   obj-$(CONFIG_ION_CHUNK_HEAP) += ion_chunk_heap.o
>   obj-$(CONFIG_ION_CMA_HEAP) += ion_cma_heap.o
> +obj-$(CONFIG_ION_SYSTEM_HEAP) += ion_system_heap.o ion_page_pool.o
> 
> I get the following order:
> 
> heap id 2, type ION_HEAP_TYPE_SYSTEM
> heap id 1, type ION_HEAP_TYPE_SYSTEM_CONTIG
> heap id 0, type ION_HEAP_TYPE_DMA
> 
> So, if the user specifies more than 1 heap in the heap_id_mask during
> allocation, the allocation fall back order completely depends on the
> order of linkage. Probably, it's better to let the user to define the
> fall back order (and NOT to be dependent on the linkage order at all)
> ?
> 

Yup, you've hit upon a key problem. Having fallbacks be stable
was always a problem and the recommendation these days is to
not rely on them. You can specify a heap at a time and fallback
manually if you want that behavior.

If you have a proposal to make fallbacks work reliably without
overly complicating the ABI I'm happy to review it.

Thanks,
Laura