Re: [PATCH] xfrm: add SA information to the offloaded packet

[Leon Romanovsky <leon@kernel.org>]

On Tue, Sep 24, 2024 at 12:34:32PM +0200, Steffen Klassert wrote:
> On Wed, Sep 11, 2024 at 01:40:40PM +0300, Leon Romanovsky wrote:
> > On Mon, Sep 09, 2024 at 11:09:05AM +0200, Steffen Klassert wrote:
> > > On Mon, Sep 02, 2024 at 12:44:52PM +0300, Leon Romanovsky wrote:
> > > > On Mon, Sep 02, 2024 at 09:44:24AM +0200, Steffen Klassert wrote:
> > > > > > 
> > > > > > Steffen,
> > > > > > 
> > > > > > What is your position on this patch?
> > > > > > It is the same patch (logically) as the one that was rejected before?
> > > > > > https://lore.kernel.org/all/ZfpnCIv+8eYd7CpO@gauss3.secunet.de/
> > > > > 
> > > > > This is an infrastructure patch to support routing based IPsec
> > > > > with xfrm interfaces. I just did not notice it because it was not
> > > > > mentioned in the commit message of the first patchset. This should have
> > > > > been included into the packet offload API patchset, but I overlooked
> > > > > that xfrm interfaces can't work with packet offload mode. The stack
> > > > > infrastructure should be complete, so that drivers can implement
> > > > > that without the need to fix the stack before.
> > > > 
> > > > Core implementation that is not used by any upstream code is rarely
> > > > right thing to do. It is not tested, complicates the code and mostly
> > > > overlooked when patches are reviewed. The better way will be to extend
> > > > the stack when this feature will be actually used and needed.
> > > 
> > > This is our tradeoff, an API should be fully designed from the
> > > beginning, everything else is bad design and will likely result
> > > in band aids (as it happens here). The API can be connected to
> > > netdevsim to test it.
> > > 
> > > Currently the combination of xfrm interfaces and packet offload
> > > is just broken. 
> > 
> > I don't think that it is broken.
> 
> I don't see anything that prevents you from offloading a SA
> with an xfrm interface ID. The binding to the interface is
> just ignored in that case.
> 
> > It is just not implemented. XFRM
> > interfaces are optional field, which is not really popular in the
> > field.
> 
> It is very popular, I know of more than a billion devices that
> are using xfrm interfaces.

We see different parts of "the field". In my case it it enterprise/cloud
world, and I can say same sentence as you with "are NOT using ..."
words instead. This is why so important to see google's driver (which is Android)
to understand the real need from this feature.

> 
> > 
> > > Unfortunalely this patch does not fix it.
> > > 
> > > I think we need to do three things:
> > > 
> > > - Fix xfrm interfaces + packet offload combination
> > > 
> > > - Extend netdevsim to support packet offload
> > > 
> > > - Extend the API for xfrm interfaces (and everything
> > >   else we forgot).
> > 
> > This is the most challenging part. It is not clear what should
> > we extend if customers are not asking for it and they are extremely
> > happy with the current IPsec packet offload state.
> 
> We just need to push the information down to the driver,
> and reject the offload if not supported.

Yes and in addition to that it will be beneficial do not add this information
to SKB if it won't be used.

> 
> > 
> > BTW, I'm aware of one gap, which is not clear how to handle, and
> > it is combination of policy sockets and offload.
> 
> Socket policies are a bit special as they are configured by
> the application that uses the socket. I don't think that
> we can even configure offload for a socket policy.

One of the idea is to iterate over all devices and check if they
support offload, if yes, then offload, if not, then fallback
to software for that device. This is just rough idea with many
caveats.

Thanks