Re: [net-next,v6 1/8] octeontx2-pf: map skb data as device writeable

Bharat Bhushan
2024-08-24T13:59:58Z

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