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

[Leon Romanovsky <leon@kernel.org>]

On Mon, Sep 02, 2024 at 09:44:24AM +0200, Steffen Klassert wrote:
> Sorry for the delay. I'm on vacation, so responses will take a bit
> longer during the next two weeks.
> 
> On Sat, Aug 31, 2024 at 08:39:34PM +0300, Leon Romanovsky wrote:
> > On Wed, Aug 28, 2024 at 07:32:47AM +0200, Steffen Klassert wrote:
> > > On Thu, Aug 22, 2024 at 01:02:52PM -0700, Feng Wang wrote:
> > > > From: wangfe <wangfe@google.com>
> > > > 
> > > > In packet offload mode, append Security Association (SA) information
> > > > to each packet, replicating the crypto offload implementation.
> > > > The XFRM_XMIT flag is set to enable packet to be returned immediately
> > > > from the validate_xmit_xfrm function, thus aligning with the existing
> > > > code path for packet offload mode.
> > > > 
> > > > This SA info helps HW offload match packets to their correct security
> > > > policies. The XFRM interface ID is included, which is crucial in setups
> > > > with multiple XFRM interfaces where source/destination addresses alone
> > > > can't pinpoint the right policy.
> > > > 
> > > > Signed-off-by: wangfe <wangfe@google.com>
> > > 
> > > Applied to ipsec-next, thanks Feng!
> > 
> > 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.

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.

> 
> In case the patch has issues, we should fix it.

Yes, this patch doesn't have in-kernel users.

Thanks