| Message ID | 20240819122348.490445-2-bbhushan2@marvell.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | cn10k-ipsec: Add outbound inline ipsec support | expand |
On Mon, 19 Aug 2024 17:53:41 +0530 Bharat Bhushan wrote: > Crypto hardware need write permission for in-place encrypt > or decrypt operation on skb-data to support IPsec crypto > offload. So map this memory as device read-write. How do you know the fragments are not read only? (Whatever the answer is it should be part of the commit msg)
On Wed, Aug 21, 2024 at 4:06 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Mon, 19 Aug 2024 17:53:41 +0530 Bharat Bhushan wrote: > > Crypto hardware need write permission for in-place encrypt > > or decrypt operation on skb-data to support IPsec crypto > > offload. So map this memory as device read-write. > > How do you know the fragments are not read only? IOMMU permission faults will be reported if the DMA_TO_DEVICE direction flag is used in dma_map_page_attrs(). This is because iommu creates read only mapping if the DMA_TO_DEVICE direction flag is used. If the direction flag used in dma_map_pages() is DMA_BIDIRECTIONAL then iommu creates mapping with both read and write permission. > > (Whatever the answer is it should be part of the commit msg) Will update commit message to something like: Crypto hardware needs both read and write permission for in-place encryption/decryption operation. Iommu creates read only mapping if the DMA_TO_DEVICE direction flag is used in dma_map_page_attrs(). Use DMA_BIDIRECTIONAL direction flag in dma_map_page_attrs(), so that iommu mapping is created with both read and write permission. Is that okay? Thanks -Bharat
On Thu, 22 Aug 2024 09:15:43 +0530 Bharat Bhushan wrote: > On Wed, Aug 21, 2024 at 4:06 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Mon, 19 Aug 2024 17:53:41 +0530 Bharat Bhushan wrote: > > > Crypto hardware need write permission for in-place encrypt > > > or decrypt operation on skb-data to support IPsec crypto > > > offload. So map this memory as device read-write. > > > > How do you know the fragments are not read only? > > IOMMU permission faults will be reported if the DMA_TO_DEVICE direction flag > is used in dma_map_page_attrs(). This is because iommu creates read only mapping > if the DMA_TO_DEVICE direction flag is used. If the direction flag used in > dma_map_pages() is DMA_BIDIRECTIONAL then iommu creates mapping with > both read and write permission. The other way around, I understand that your code makes the pages writable for the device. What I'm concerned about is that if this code path is fed Tx skbs you will corrupt them. Are these not Tx skbs that you're mapping? Have you fully CoW'd them to make sure they are writable?
On Thu, Aug 22, 2024 at 8:18 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 22 Aug 2024 09:15:43 +0530 Bharat Bhushan wrote: > > On Wed, Aug 21, 2024 at 4:06 AM Jakub Kicinski <kuba@kernel.org> wrote: > > > On Mon, 19 Aug 2024 17:53:41 +0530 Bharat Bhushan wrote: > > > > Crypto hardware need write permission for in-place encrypt > > > > or decrypt operation on skb-data to support IPsec crypto > > > > offload. So map this memory as device read-write. > > > > > > How do you know the fragments are not read only? > > > > IOMMU permission faults will be reported if the DMA_TO_DEVICE direction flag > > is used in dma_map_page_attrs(). This is because iommu creates read only mapping > > if the DMA_TO_DEVICE direction flag is used. If the direction flag used in > > dma_map_pages() is DMA_BIDIRECTIONAL then iommu creates mapping with > > both read and write permission. > > The other way around, I understand that your code makes the pages > writable for the device. What I'm concerned about is that if this > code path is fed Tx skbs you will corrupt them. Are these not Tx > skbs that you're mapping? Have you fully CoW'd them to make sure > they are writable? This code is mapping skb data for hardware to over-write plain-text with cypher-text and update authentication data (in-place encap/auth). This patch series doesn't take care of CoWing for skb data. Actually I was not aware of that before your comment. To understand your comment better, If the device writes to shared skb data without CoWing then we have an issue. Is that correct? I do not see any other driver supporting IPsec crypto offload ensuring skb data CoWing, but there is a possibility that those devices are not doing in-place encap and auth (encap and auth data written to separate buffer). I do not have clarity about this, This skb is set for IPSEC crypto offload, Is this the driver which has to ensure that the skb is writeable or the network stack (xfrm layer) will ensure the same. If it is former then add code to call skb_unshare(). Please suggest. Thanks -Bharat
On Fri, Aug 23, 2024 at 4:55 PM Bharat Bhushan <bharatb.linux@gmail.com> wrote: > > On Thu, Aug 22, 2024 at 8:18 PM Jakub Kicinski <kuba@kernel.org> wrote: > > > > On Thu, 22 Aug 2024 09:15:43 +0530 Bharat Bhushan wrote: > > > On Wed, Aug 21, 2024 at 4:06 AM Jakub Kicinski <kuba@kernel.org> wrote: > > > > On Mon, 19 Aug 2024 17:53:41 +0530 Bharat Bhushan wrote: > > > > > Crypto hardware need write permission for in-place encrypt > > > > > or decrypt operation on skb-data to support IPsec crypto > > > > > offload. So map this memory as device read-write. > > > > > > > > How do you know the fragments are not read only? > > > > > > IOMMU permission faults will be reported if the DMA_TO_DEVICE direction flag > > > is used in dma_map_page_attrs(). This is because iommu creates read only mapping > > > if the DMA_TO_DEVICE direction flag is used. If the direction flag used in > > > dma_map_pages() is DMA_BIDIRECTIONAL then iommu creates mapping with > > > both read and write permission. > > > > The other way around, I understand that your code makes the pages > > writable for the device. What I'm concerned about is that if this > > code path is fed Tx skbs you will corrupt them. Are these not Tx > > skbs that you're mapping? Have you fully CoW'd them to make sure > > they are writable? > > This code is mapping skb data for hardware to over-write plain-text with > cypher-text and update authentication data (in-place encap/auth). > This patch series doesn't take care of CoWing for skb data. Actually I was > not aware of that before your comment. > > To understand your comment better, If the device writes to shared skb data > without CoWing then we have an issue. Is that correct? > > I do not see any other driver supporting IPsec crypto offload ensuring > skb data CoWing, > but there is a possibility that those devices are not doing in-place > encap and auth (encap > and auth data written to separate buffer). I do not have clarity about > this, This skb is set for > IPSEC crypto offload, Is this the driver which has to ensure that the > skb is writeable or the > network stack (xfrm layer) will ensure the same. If it is former then > add code to call skb_unshare(). > Please suggest. My understanding after further looking into the code is that it is the driver responsibility to ensure skb is not in a shared state. Thanks for pointing this out. Will change code in the next version. Regards -Bharat > > Thanks > -Bharat
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c index 3eb85949677a..ce1a7db745c5 100644 --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c @@ -98,7 +98,7 @@ static dma_addr_t otx2_dma_map_skb_frag(struct otx2_nic *pfvf, offset = skb_frag_off(frag); *len = skb_frag_size(frag); } - return otx2_dma_map_page(pfvf, page, offset, *len, DMA_TO_DEVICE); + return otx2_dma_map_page(pfvf, page, offset, *len, DMA_BIDIRECTIONAL); } static void otx2_dma_unmap_skb_frags(struct otx2_nic *pfvf, struct sg_list *sg) @@ -107,7 +107,7 @@ static void otx2_dma_unmap_skb_frags(struct otx2_nic *pfvf, struct sg_list *sg) for (seg = 0; seg < sg->num_segs; seg++) { otx2_dma_unmap_page(pfvf, sg->dma_addr[seg], - sg->size[seg], DMA_TO_DEVICE); + sg->size[seg], DMA_BIDIRECTIONAL); } sg->num_segs = 0; }
Crypto hardware need write permission for in-place encrypt or decrypt operation on skb-data to support IPsec crypto offload. So map this memory as device read-write. Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com> --- drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)