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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CADsK2K_ip7YUipHa3ZYW7tmvgU0_vEsDTkeACrgi7oN5VLTqpQ@mail.gmail.com>
Date: Wed, 11 Sep 2024 16:43:55 -0700
From: Feng Wang <wangfe@...gle.com>
To: Leon Romanovsky <leon@...nel.org>
Cc: Steffen Klassert <steffen.klassert@...unet.com>, netdev@...r.kernel.org, 
	antony.antony@...unet.com
Subject: Re: [PATCH] xfrm: add SA information to the offloaded packet

Hi Steffen,

Can you reconsider the revert of the CL? Based on our discussion, I
believe we've reached a consensus that the xfrm id is necessary. If
some customers prefer not to utilize it, we can extend the offload
flag (something like XFRM_DEV_OFFLOAD_FLAG_ACQ) to manage this
behavior. The default setting would remain consistent with the current
implementation.

Thank you!

Feng

On Wed, Sep 11, 2024 at 3:40 AM Leon Romanovsky <leon@...nel.org> 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. 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