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

[Leon Romanovsky <leon@kernel.org>]

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. It is just not implemented. XFRM
interfaces are optional field, which is not really popular in the
field.

> 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.

BTW, I'm aware of one gap, which is not clear how to handle, and
it is combination of policy sockets and offload.

> 
> > IMHO, attempt to enrich core code without showing users of this new flow
> > is comparable to premature optimization.
> > 
> > And Feng more than once said that this code is for some out-of-tree
> > driver.
> 
> It is an API, so everyone can use it.

Of course, as long as in-kernel user exists.

Thanks