linux-kernel - Re: [PATCH V3 1/4] misc: vop: change the way of allocating vring and device page

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]

Message-ID: <20201023092524.GA29066@infradead.org>
Date:   Fri, 23 Oct 2020 10:25:24 +0100
From:   Christoph Hellwig <hch@...radead.org>
To:     Sherry Sun <sherry.sun@....com>
Cc:     hch@...radead.org, sudeep.dutt@...el.com, ashutosh.dixit@...el.com,
        arnd@...db.de, gregkh@...uxfoundation.org, kishon@...com,
        lorenzo.pieralisi@....com, linux-kernel@...r.kernel.org,
        linux-pci@...r.kernel.org, linux-imx@....com
Subject: Re: [PATCH V3 1/4] misc: vop: change the way of allocating vring and
 device page

>  static int mic_dp_init(struct mic_device *mdev)
>  {
> -	mdev->dp = kzalloc(MIC_DP_SIZE, GFP_KERNEL);
> +	mdev->dp = dma_alloc_coherent(&mdev->pdev->dev, MIC_DP_SIZE,
> +				      &mdev->dp_dma_addr, GFP_KERNEL);
>  	if (!mdev->dp)
>  		return -ENOMEM;
>  
> -	mdev->dp_dma_addr = mic_map_single(mdev,
> -		mdev->dp, MIC_DP_SIZE);
> -	if (mic_map_error(mdev->dp_dma_addr)) {
> -		kfree(mdev->dp);
> -		dev_err(&mdev->pdev->dev, "%s %d err %d\n",
> -			__func__, __LINE__, -ENOMEM);
> -		return -ENOMEM;
> -	}
>  	mdev->ops->write_spad(mdev, MIC_DPLO_SPAD, mdev->dp_dma_addr);
>  	mdev->ops->write_spad(mdev, MIC_DPHI_SPAD, mdev->dp_dma_addr >> 32);
>  	return 0;
> @@ -68,8 +62,7 @@ static int mic_dp_init(struct mic_device *mdev)
>  /* Uninitialize the device page */
>  static void mic_dp_uninit(struct mic_device *mdev)
>  {
> -	mic_unmap_single(mdev, mdev->dp_dma_addr, MIC_DP_SIZE);
> -	kfree(mdev->dp);
> +	dma_free_coherent(&mdev->pdev->dev, MIC_DP_SIZE, mdev->dp, mdev->dp_dma_addr);
>  }

This adds an over 80 char line.  Also please just kill mic_dp_init and
mic_dp_uninit and inline those into the callers.

> +		vvr->buf = dma_alloc_coherent(vop_dev(vdev), VOP_INT_DMA_BUF_SIZE,
> +					      &vvr->buf_da, GFP_KERNEL);

Another overly long line.

> @@ -1068,7 +1049,7 @@ vop_query_offset(struct vop_vdev *vdev, unsigned long offset,
>  		struct vop_vringh *vvr = &vdev->vvr[i];
>  
>  		if (offset == start) {
> -			*pa = virt_to_phys(vvr->vring.va);
> +			*pa = vqconfig[i].address;

vqconfig[i].address is a __le64, so this needs an endian swap.

But more importantly the caller of vop_query_offset, vop_mmap, uses
remap_pfn_range and pa.  You cannot mix remap_pfn_range with DMA
coherent allocations, it can only be mmaped using dma_mmap_coherent.