> -----Original Message----- > From: Jason Gunthorpe [mailto:jgg@xxxxxxxxxx] > Sent: Friday, August 26, 2022 6:45 PM > To: Kashyap Desai <kashyap.desai@xxxxxxxxxxxx> > Cc: linux-rdma@xxxxxxxxxxxxxxx; leonro@xxxxxxxxxx; Selvin Xavier > <selvin.xavier@xxxxxxxxxxxx>; Andrew Gospodarek > <andrew.gospodarek@xxxxxxxxxxxx> > Subject: Re: [PATCH rdma-rc v1] RDMA/core: fix sg_to_page mapping for > boundary condition > > On Mon, Aug 22, 2022 at 07:51:22PM +0530, Kashyap Desai wrote: > > > Now, we will enter into below loop with dma_addr = page_addr = > > 0xffffffffffffe000 and "end_dma_addr = dma_addr + dma_len" is ZERO. > > eval 0xffffffffffffe000 + 8192 > > hexadecimal: 0 > > This is called overflow. Is this not DMAable for 64bit DMA mask device ? It is DMAable. So not sure why you call it as overflow. ? My understanding is - There is a roll over (overflow) issue, If we get DMA address = 0xffffffffffffe000 and length = 16K. This is a last page of the possible DMA range. Similar discussion I found @https://marc.info/?l=git-commits-head&m=149437079023021&w=2 We need to handle size vs end_address gracefully. > > Anything doing maths on the sgl's is likely to become broken by this - which is > why I think it is unnecessarily dangerous for the iommu code to general dma > addresses like this. It just shouldn't. > > > > It should not create mappings that are so dangerous. There is really > > > no > > reason to > > > use the last page of IOVA space that includes -1. I agree that such mapping is obviously dangerous, but it is not illegal as well. Same sgl mapping works if it is direct attached Storage, so there will be a logical question why IB stack is not handling this. > > > > That is correct, but if API which deals with mapping they handle this > > kind of request gracefully is needed. Right ? > > Ideally, but that is a game of wack a mole across the kernel, and redoing > algorithms to avoid overflowing addition is tricky stuff. > > > I thought about better approach without creating regression and I > > found having loop using sg_dma_len can avoid such issues gracefully. > > How about original patch. ? > > It overflows too. > > You need to write the code so you never create the situation where > A+B=0 - don't try to fix things up after that happens. In proposed patch, A + B = 0 is possible, but it will be considered as end of the loop. So let's say it was supposed to setup 8 sgl entries, A + B = 0 will be detected only after 8th entry is setup. Current code detect A + B = 0 much early and that is what I am trying to fix in this patch (This patch will not fix any roll over issue). At least it will serve the purpose of creating correct sgl entries in low level driver's Memory region through set_page() callback. I am fine with your call since this is a concern case and it is going to change core function. We have two choice - If function ib_sg_to_pages() can't handle such case, would you like to detect such mapping error and at least return -EINVAL ? OR Just parse whatever mapping is received in sgl to low level driver and don't really care about overflow case. (May be this patch can help.) ? Kashyap > > Usually that means transforming the algorithm so that it works on a "last byte" > so we never need to compute an address that is +1 to the last byte, which would > be the overflown 0. > > > Above revert is part of my test. In my setup "iommu_dma_forcedac = $2 > > = false". > > So you opt into this behavior, OK > > Jason
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
