linux-kernel - Re: [PATCH for-next v3] RDMA/core: Avoid hmm_dma_map

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: <77ba8709-9b59-4c83-8898-6f0c699992c3@gmail.com>
Date: Sun, 25 May 2025 18:34:56 +0900
From: Daisuke Matsuda <dskmtsd@...il.com>
To: Leon Romanovsky <leon@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-rdma@...r.kernel.org, jgg@...pe.ca,
 zyjzyj2000@...il.com, hch@...radead.org
Subject: Re: [PATCH for-next v3] RDMA/core: Avoid hmm_dma_map_alloc() for
 virtual DMA devices

Hi Leon,

Thank you for amending the patch.
I've run the test and confirmed that the bug has been resolved.

We still have two build errors. Please see my reply below.

On 2025/05/25 16:08, Leon Romanovsky wrote:
> On Sat, May 24, 2025 at 02:43:28PM +0000, Daisuke Matsuda wrote:
<...>
>>   drivers/infiniband/core/device.c   | 17 +++++++++++++++++
>>   drivers/infiniband/core/umem_odp.c | 11 ++++++++---
>>   include/rdma/ib_verbs.h            |  4 ++++
>>   3 files changed, 29 insertions(+), 3 deletions(-)
> 
> Please include changelogs when you submit vX patches next time.

Oh, sorry. I will be careful from next time.

> 
> I ended with the following patch:
> 
> diff --git a/drivers/infiniband/core/umem_odp.c b/drivers/infiniband/core/umem_odp.c
> index 51d518989914e..cf16549919e02 100644
> --- a/drivers/infiniband/core/umem_odp.c
> +++ b/drivers/infiniband/core/umem_odp.c
> @@ -60,9 +60,11 @@ static int ib_init_umem_odp(struct ib_umem_odp *umem_odp,
>   {
>   	struct ib_device *dev = umem_odp->umem.ibdev;
>   	size_t page_size = 1UL << umem_odp->page_shift;
> +	struct hmm_dma_map *map;
>   	unsigned long start;
>   	unsigned long end;
> -	int ret;
> +	size_t nr_entries;
> +	int ret = 0;
>   
>   	umem_odp->umem.is_odp = 1;
>   	mutex_init(&umem_odp->umem_mutex);
> @@ -75,9 +77,20 @@ static int ib_init_umem_odp(struct ib_umem_odp *umem_odp,
>   	if (unlikely(end < page_size))
>   		return -EOVERFLOW;
>   
> -	ret = hmm_dma_map_alloc(dev->dma_device, &umem_odp->map,
> -				(end - start) >> PAGE_SHIFT,
> -				1 << umem_odp->page_shift);
> +	nr_entries = (end - start) >> PAGE_SHIFT;
> +	if (!(nr_entries * PAGE_SIZE / page_size))
> +		return -EINVAL;
> +
> +	nap = &umem_odp->map;

BUILD ERROR: 'nap' should be 'map'

> +	if (ib_uses_virt_dma(dev)) {
> +		map->pfn_list = kvcalloc(nr_entries, sizeof(*map->pfn_list),
> +					 GFP_KERNEL | __GFP_NOWARN);
> +		if (!map->pfn_list)
> +			ret = -ENOMEM;
> +	} else
> +		ret = hmm_dma_map_alloc(dev->dma_device, &umem_odp->map,

OPTIONAL: Perhaps we can just pass 'map' for the 2nd arg?

> +					(end - start) >> PAGE_SHIFT,
> +					1 << umem_odp->page_shift);
>   	if (ret)
>   		return ret;
>   
> @@ -90,7 +103,10 @@ static int ib_init_umem_odp(struct ib_umem_odp *umem_odp,
>   	return 0;
>   
>   out_free_map:
> -	hmm_dma_map_free(dev->dma_device, &umem_odp->map);
> +	if (ib_uses_virt_dma(dev))
> +		kfree(map->pfn_list);
> +	else
> +		hmm_dma_map_free(dev->dma_device, &umem_odp->map);

OPTIONAL: Here too.

>   	return ret;
>   }
>   
> @@ -259,7 +275,10 @@ static void ib_umem_odp_free(struct ib_umem_odp *umem_odp)
>   				    ib_umem_end(umem_odp));
>   	mutex_unlock(&umem_odp->umem_mutex);
>   	mmu_interval_notifier_remove(&umem_odp->notifier);
> -	hmm_dma_map_free(dev->dma_device, &umem_odp->map);
> +	if (ib_uses_virt_dma(dev))
> +		kfree(umem_odp->map->pfn_list);

BUILD ERROR:     'umem_odp->map.pfn_list' is correct.

Thanks again,
Daisuke