linux-kernel - Re: [RFC PATCH v2 0/3] prerequisites for device reserved local mem rework

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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 16 May 2019 09:35:27 +0000
From:   Laurentiu Tudor <laurentiu.tudor@....com>
To:     Fredrik Noring <noring@...rew.org>
CC:     Robin Murphy <robin.murphy@....com>, "hch@....de" <hch@....de>,
        "stern@...land.harvard.edu" <stern@...land.harvard.edu>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "marex@...x.de" <marex@...x.de>,
        "JuergenUrban@....de" <JuergenUrban@....de>,
        Leo Li <leoyang.li@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH v2 0/3] prerequisites for device reserved local mem
 rework

Hi Fredrik,

Thanks very much for taking the time to look into this. Please see 
comments inline.

On 15.05.2019 19:28, Fredrik Noring wrote:
> Hi Lauretniu,
> 
>> I think that any recent kernel will do, so I'd say your current branch
>> should be fine.
> 
> The kernel oopses with "unable to handle kernel paging request at virtual
> address 000aba0b" in hcd_alloc_coherent via usb_hcd_map_urb_for_dma. 

By any chance, does this address looks like the dma_addr that the device 
should target?

> This relates to patch 2/3 that I didn't quite understand, where
> 
> -	retval = dma_declare_coherent_memory(dev, mem->start,
> -					 mem->start - mem->parent->start,
> -					 resource_size(mem));
> 
> becomes
> 
> +	retval = gen_pool_add_virt(hcd->localmem_pool,
> +				   (unsigned long)mem->start -
> +					mem->parent->start,
> +				   mem->start, resource_size(mem),
> 
> so that arguments two and three switch places. Given
> 
> int dma_declare_coherent_memory(struct device *dev, phys_addr_t phys_addr,
> 				dma_addr_t device_addr, size_t size);
> 
> and
> 
> int gen_pool_add_virt(struct gen_pool *pool, unsigned long virt, phys_addr_t phys
> 		 size_t size, int nid)
> 
> it seems that the device address (dma_addr_t device_addr) now becomes a
> virtual address (unsigned long virt). Is that intended?

Actually, I think I'm misusing genalloc and also it appears that i need 
to add a mapping on the phys address. So my plan is to change the 
"unsigned long virt" to be the void * returned by the mapping operation 
and the phys_addr_t be the dma_addr_t. I'll return with a patch.

Regarding the usage of unsigned long in genalloc, yeah seems a bit 
strange but by looking at other users in the kernel it appears that 
that's the design.

---
Best Regards, Laurentiu

> My corresponding patch is below for reference. I applied your 1/3 patch
> to test it.
> 
> Fredrik
> 
> diff --git a/drivers/usb/host/ohci-ps2.c b/drivers/usb/host/ohci-ps2.c
> --- a/drivers/usb/host/ohci-ps2.c
> +++ b/drivers/usb/host/ohci-ps2.c
> @@ -7,6 +7,7 @@
>    */
>   
>   #include <linux/dma-mapping.h>
> +#include <linux/genalloc.h>
>   #include <linux/module.h>
>   #include <linux/platform_device.h>
>   #include <linux/usb.h>
> @@ -92,12 +93,12 @@ static irqreturn_t ohci_ps2_irq(struct usb_hcd *hcd)
>   	return ohci_irq(hcd); /* Call normal IRQ handler. */
>   }
>   
> -static int iopheap_alloc_coherent(struct platform_device *pdev,
> -	size_t size, int flags)
> +static int iopheap_alloc_coherent(struct platform_device *pdev, size_t size)
>   {
>   	struct device *dev = &pdev->dev;
>   	struct usb_hcd *hcd = platform_get_drvdata(pdev);
>   	struct ps2_hcd *ps2priv = hcd_to_priv(hcd);
> +	int err;
>   
>   	if (ps2priv->iop_dma_addr != 0)
>   		return 0;
> @@ -112,28 +113,37 @@ static int iopheap_alloc_coherent(struct platform_device *pdev,
>   		return -ENOMEM;
>   	}
>   
> -	if (dma_declare_coherent_memory(dev,
> -			iop_bus_to_phys(ps2priv->iop_dma_addr),
> -			ps2priv->iop_dma_addr, size, flags)) {
> -		dev_err(dev, "dma_declare_coherent_memory failed\n");
> -		iop_free(ps2priv->iop_dma_addr);
> -		ps2priv->iop_dma_addr = 0;
> -		return -ENOMEM;
> +	hcd->localmem_pool = devm_gen_pool_create(dev,
> +		PAGE_SHIFT, dev_to_node(dev), DRV_NAME);
> +	if (IS_ERR(hcd->localmem_pool)) {
> +		err = PTR_ERR(hcd->localmem_pool);
> +		goto out_err;
> +	}
> +
> +	err = gen_pool_add_virt(hcd->localmem_pool, ps2priv->iop_dma_addr,
> +		iop_bus_to_phys(ps2priv->iop_dma_addr), size, dev_to_node(dev));
> +	if (err < 0) {
> +		dev_err(dev, "gen_pool_add_virt failed with %d\n", err);
> +		goto out_err;
>   	}
>   
>   	return 0;
> +
> +out_err:
> +	iop_free(ps2priv->iop_dma_addr);
> +	ps2priv->iop_dma_addr = 0;
> +
> +	return err;
>   }
>   
>   static void iopheap_free_coherent(struct platform_device *pdev)
>   {
> -	struct device *dev = &pdev->dev;
>   	struct usb_hcd *hcd = platform_get_drvdata(pdev);
>   	struct ps2_hcd *ps2priv = hcd_to_priv(hcd);
>   
>   	if (ps2priv->iop_dma_addr == 0)
>   		return;
>   
> -	dma_release_declared_memory(dev);
>   	iop_free(ps2priv->iop_dma_addr);
>   	ps2priv->iop_dma_addr = 0;
>   }
> @@ -177,8 +187,7 @@ static int ohci_hcd_ps2_probe(struct platform_device *pdev)
>   		goto err;
>   	}
>   
> -	ret = iopheap_alloc_coherent(pdev,
> -		DMA_BUFFER_SIZE, DMA_MEMORY_EXCLUSIVE);
> +	ret = iopheap_alloc_coherent(pdev, DMA_BUFFER_SIZE);
>   	if (ret != 0)
>   		goto err_alloc_coherent;
>   
>