| 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
| ||
|
Message-ID: <CAEGmHFGDLvySk83Y8n_NdPEqAvNSVCnjt++eWQ5sMFRuzJ19dA@mail.gmail.com>
Date: Wed, 21 Oct 2020 14:46:20 -0700
From: Furquan Shaikh <furquan@...gle.com>
To: Ard Biesheuvel <ardb@...nel.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Prashant Malani <pmalani@...omium.org>,
Arthur Heymans <arthur@...ymans.xyz>,
Patrick Rudolph <patrick.rudolph@...ements.com>,
Duncan Laurie <dlaurie@...gle.com>
Subject: Re: [PATCH] firmware: gsmi: Drop the use of dma_pool_* API functions
On Wed, Oct 21, 2020 at 2:36 AM Ard Biesheuvel <ardb@...nel.org> wrote:
>
> On Wed, 21 Oct 2020 at 10:51, Greg Kroah-Hartman
> <gregkh@...uxfoundation.org> wrote:
> >
> > On Wed, Oct 21, 2020 at 12:37:52AM -0700, Furquan Shaikh wrote:
> > > On Tue, Oct 20, 2020 at 11:37 PM Ard Biesheuvel <ardb@...nel.org> wrote:
> > > >
> > > > On Wed, 21 Oct 2020 at 07:18, Greg Kroah-Hartman
> > > > <gregkh@...uxfoundation.org> wrote:
> > > > >
> > > > > On Tue, Oct 20, 2020 at 10:01:41PM -0700, Furquan Shaikh wrote:
> > > > > > GSMI driver uses dma_pool_* API functions for buffer allocation
> > > > > > because it requires that the SMI buffers are allocated within 32-bit
> > > > > > physical address space. However, this does not work well with IOMMU
> > > > > > since there is no real device and hence no domain associated with the
> > > > > > device.
> > > > > >
> > > > > > Since this is not a real device, it does not require any device
> > > > > > address(IOVA) for the buffer allocations. The only requirement is to
> > > > > > ensure that the physical address allocated to the buffer is within
> > > > > > 32-bit physical address space. This change allocates a page using
> > > > > > `get_zeroed_page()` and passes in GFP_DMA32 flag to ensure that the
> > > > > > page allocation is done in the DMA32 zone. All the buffer allocation
> > > > > > requests for gsmi_buf are then satisfed using this pre-allocated page
> > > > > > for the device.
> > > > >
> > > > > Are you sure that "GFP_DMA32" really does what you think it does? A
> > > > > "normal" call with GFP_KERNEL" will give you memory that is properly
> > > > > dma-able.
> > > > >
> > > > > We should not be adding new GFP_DMA* users in the kernel in these days,
> > > > > just call dma_alloc*() and you should be fine.
> > > > >
> > > >
> > > > The point seems to be that this is not about DMA at all, and so there
> > > > is no device associated with the DMA allocation.
> > > >
> > > > The other 'master' is the CPU running firmware in an execution mode
> > > > where it can only access the bottom 4 GB of memory, and GFP_DMA32
> > > > happens to allocate from a zone which is guaranteed to be accessible
> > > > to the firmware.
> > >
> > > Ard captured the context and requirement perfectly. GFP_DMA32
> > > satisfies the requirement for allocating memory from a zone which is
> > > accessible to the firmware in SMI mode. This seems to be one of the
> > > common ways how other drivers and common code in the kernel currently
> > > allocate physical memory below the 4G boundary. Hence, I used the same
> > > mechanism in GSMI driver.
> >
> > Then can you please document this a bit better in the changelog,
> > explaining why this is ok to use this obsolete api, and also in the code
> > itself so that no one tries to clean it up in the future?
I will do that and send out a newer revision of the patch. Thanks!
>
> >
>
> Wouldn't it be simpler to switch to a SLAB cache created with SLAB_CACHE_DMA32?
I considered doing that, but there is some weirdness around use of
GFP_DMA32 in `kmem_cache_zalloc` i.e. you can allocate slab cache
using SLAB_CACHE_DMA32, however you cannot pass in GFP_DMA32 flag when
using `kmem_cache_zalloc`, else it hits GFP_SLAB_BUG_MASK. If the
GFP_DMA32 flag is skipped, then `kmem_cache_zalloc` is happy and
provides an allocation under the 4G boundary.
Since this driver really just needs to allocate space for 3 buffers, I
went with allocating a page and handing out space for the buffers from
the page. If the SLAB_CACHE_DMA32 seems like a cleaner way to do this,
I can update the patch to use that.
Thanks,
Furquan
>
>
>
> I.e., something like the below (whitespace mangling courtesy of gmail)
>
> diff --git a/drivers/firmware/google/gsmi.c b/drivers/firmware/google/gsmi.c
> index 7d9367b22010..d932284f970c 100644
> --- a/drivers/firmware/google/gsmi.c
> +++ b/drivers/firmware/google/gsmi.c
> @@ -85,7 +85,6 @@
> struct gsmi_buf {
> u8 *start; /* start of buffer */
> size_t length; /* length of buffer */
> - dma_addr_t handle; /* dma allocation handle */
> u32 address; /* physical address of buffer */
> };
>
> @@ -97,7 +96,7 @@ static struct gsmi_device {
> spinlock_t lock; /* serialize access to SMIs */
> u16 smi_cmd; /* SMI command port */
> int handshake_type; /* firmware handler interlock type */
> - struct dma_pool *dma_pool; /* DMA buffer pool */
> + struct kmem_cache *mem_pool; /* buffer pool */
> } gsmi_dev;
>
> /* Packed structures for communicating with the firmware */
> @@ -157,8 +156,7 @@ static struct gsmi_buf *gsmi_buf_alloc(void)
> }
>
> /* allocate buffer in 32bit address space */
> - smibuf->start = dma_pool_alloc(gsmi_dev.dma_pool, GFP_KERNEL,
> - &smibuf->handle);
> + smibuf->start = kmem_cache_zalloc(gsmi_dev.mem_pool, GFP_KERNEL);
> if (!smibuf->start) {
> printk(KERN_ERR "gsmi: failed to allocate name buffer\n");
> kfree(smibuf);
> @@ -176,8 +174,7 @@ static void gsmi_buf_free(struct gsmi_buf *smibuf)
> {
> if (smibuf) {
> if (smibuf->start)
> - dma_pool_free(gsmi_dev.dma_pool, smibuf->start,
> - smibuf->handle);
> + kmem_cache_free(gsmi_dev.mem_pool, smibuf->start);
> kfree(smibuf);
> }
> }
> @@ -914,9 +911,10 @@ static __init int gsmi_init(void)
> spin_lock_init(&gsmi_dev.lock);
>
> ret = -ENOMEM;
> - gsmi_dev.dma_pool = dma_pool_create("gsmi", &gsmi_dev.pdev->dev,
> - GSMI_BUF_SIZE, GSMI_BUF_ALIGN, 0);
> - if (!gsmi_dev.dma_pool)
> + gsmi_dev.mem_pool = kmem_cache_create("gsmi", GSMI_BUF_SIZE,
> + GSMI_BUF_ALIGN, SLAB_CACHE_DMA32,
> + NULL);
> + if (!gsmi_dev.mem_pool)
> goto out_err;
>
> /*
> @@ -1032,7 +1030,7 @@ static __init int gsmi_init(void)
> gsmi_buf_free(gsmi_dev.param_buf);
> gsmi_buf_free(gsmi_dev.data_buf);
> gsmi_buf_free(gsmi_dev.name_buf);
> - dma_pool_destroy(gsmi_dev.dma_pool);
> + kmem_cache_destroy(gsmi_dev.mem_pool);
> platform_device_unregister(gsmi_dev.pdev);
> pr_info("gsmi: failed to load: %d\n", ret);
> #ifdef CONFIG_PM
> @@ -1057,7 +1055,7 @@ static void __exit gsmi_exit(void)
> gsmi_buf_free(gsmi_dev.param_buf);
> gsmi_buf_free(gsmi_dev.data_buf);
> gsmi_buf_free(gsmi_dev.name_buf);
> - dma_pool_destroy(gsmi_dev.dma_pool);
> + kmem_cache_destroy(gsmi_dev.mem_pool);
> platform_device_unregister(gsmi_dev.pdev);
> #ifdef CONFIG_PM
> platform_driver_unregister(&gsmi_driver_info);
Powered by blists - more mailing lists