linux-kernel - Re: next-20250627: IOMMU DMA warning during NVMe I/O completion after 06cae0e3f61c

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 for Android: free password hash cracker in your pocket

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

Message-ID: <aGRMilWhgF4z0WOf@kbusch-mbp>
Date: Tue, 1 Jul 2025 17:00:58 -0400
From: Keith Busch <kbusch@...nel.org>
To: Christoph Hellwig <hch@....de>
Cc: Ben Copeland <ben.copeland@...aro.org>, linux-kernel@...r.kernel.org,
	lkft-triage@...ts.linaro.org, regressions@...ts.linux.dev,
	linux-nvme@...ts.infradead.org,
	Dan Carpenter <dan.carpenter@...aro.org>, axboe@...nel.dk,
	sagi@...mberg.me, iommu@...ts.linux.dev,
	Leon Romanovsky <leonro@...dia.com>
Subject: Re: next-20250627: IOMMU DMA warning during NVMe I/O completion
 after 06cae0e3f61c

On Tue, Jul 01, 2025 at 03:29:36PM +0200, Christoph Hellwig wrote:
> Yes, that's broken, and I remember fixing it before.  A little digging
> shows that my fixes disappeared between the oct 30 version of Leon's
> dma-split branch and the latest one somewhere.  Below is what should
> restore it, but at least when forcing my Intel IOMMU down this path it
> still has issues with VPTEs already set.  So maybe Bob should not try
> it quite yet.  I'll try to get to it, but my availability today and
> tomorrow is a bit limited.

Let's say we're using ARM64 SMMU configured with 64k granularity like I
showed earlier.

Now let's send a read command with 128k transfer, and let's assume the
payload is two 64k aligned physical segments, so we have 2 bvecs.

Since nvme's virtual boundary is smaller than the iommu's granule, we
won't attempt to coalesce. We instead iommu map each bvec segment
individually.

And let's say each segment just so happens to get consecutive IOVA's.
The mapping side had done each segment individually, but your proposal
here will assume the contiguous dma_addr ranges were done as a single
larger mapping. Is that okay?

> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 38be1505dbd9..02bb5cf5db1a 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -678,40 +678,55 @@ static void nvme_free_prps(struct request *req)
>  	enum dma_data_direction dir = rq_dma_dir(req);
>  	int length = iod->total_len;
>  	dma_addr_t dma_addr;
> -	int i, desc;
> +	int prp_len, i, desc;
>  	__le64 *prp_list;
> +	dma_addr_t dma_start;
>  	u32 dma_len;
>  
>  	dma_addr = le64_to_cpu(iod->cmd.common.dptr.prp1);
> -	dma_len = min_t(u32, length,
> -		NVME_CTRL_PAGE_SIZE - (dma_addr & (NVME_CTRL_PAGE_SIZE - 1)));
> -	length -= dma_len;
> +	prp_len = NVME_CTRL_PAGE_SIZE - (dma_addr & (NVME_CTRL_PAGE_SIZE - 1));
> +	prp_len = min(length, prp_len);
> +	length -= prp_len;
>  	if (!length) {
> -		dma_unmap_page(dma_dev, dma_addr, dma_len, dir);
> +		dma_unmap_page(dma_dev, dma_addr, prp_len, dir);
>  		return;
>  	}
>  
> +	dma_start = dma_addr;
> +	dma_len = prp_len;
> +	dma_addr = le64_to_cpu(iod->cmd.common.dptr.prp2);
> +
>  	if (length <= NVME_CTRL_PAGE_SIZE) {
> -		dma_unmap_page(dma_dev, dma_addr, dma_len, dir);
> -		dma_addr = le64_to_cpu(iod->cmd.common.dptr.prp2);
> -		dma_unmap_page(dma_dev, dma_addr, length, dir);
> -		return;
> +		if (dma_addr != dma_start + dma_len) {
> +			dma_unmap_page(dma_dev, dma_start, dma_len, dir);
> +			dma_start = dma_addr;
> +			dma_len = 0;
> +		}
> +		dma_len += length;
> +		goto done;
>  	}
>  
>  	i = 0;
>  	desc = 0;
>  	prp_list = iod->descriptors[desc];
>  	do {
> -		dma_unmap_page(dma_dev, dma_addr, dma_len, dir);
>  		if (i == NVME_CTRL_PAGE_SIZE >> 3) {
>  			prp_list = iod->descriptors[++desc];
>  			i = 0;
>  		}
>  
>  		dma_addr = le64_to_cpu(prp_list[i++]);
> -		dma_len = min(length, NVME_CTRL_PAGE_SIZE);
> -		length -= dma_len;
> +		if (dma_addr != dma_start + dma_len) {
> +			dma_unmap_page(dma_dev, dma_start, dma_len, dir);
> +			dma_start = dma_addr;
> +			dma_len = 0;
> +		}
> +		prp_len = min(length, NVME_CTRL_PAGE_SIZE);
> +		dma_len += prp_len;
> +		length -= prp_len;
>  	} while (length);
> +done:
> +	dma_unmap_page(dma_dev, dma_start, dma_len, dir);
>  }
>  
>  static void nvme_free_sgls(struct request *req)