netdev - Re: [PATCH net 0/5] macsec: offload-related fixes

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] [day] [month] [year] [list]

Date:   Tue, 25 Oct 2022 09:55:23 +0300
From:   Leon Romanovsky <leon@...nel.org>
To:     Sabrina Dubroca <sd@...asysnail.net>
Cc:     Antoine Tenart <atenart@...nel.org>, netdev@...r.kernel.org,
        Mark Starovoytov <mstarovoitov@...vell.com>,
        Igor Russkikh <irusskikh@...vell.com>
Subject: Re: [PATCH net 0/5] macsec: offload-related fixes

On Tue, Oct 25, 2022 at 12:05:02AM +0200, Sabrina Dubroca wrote:
> 2022-10-24, 11:43:26 +0300, Leon Romanovsky wrote:
> > On Mon, Oct 24, 2022 at 10:24:28AM +0200, Sabrina Dubroca wrote:
> > > 2022-10-23, 10:52:56 +0300, Leon Romanovsky wrote:
> > > > On Thu, Oct 20, 2022 at 03:54:28PM +0200, Sabrina Dubroca wrote:
> > > > > 2022-10-18, 09:28:08 +0300, Leon Romanovsky wrote:
> > > > > > On Fri, Oct 14, 2022 at 04:03:56PM +0200, Antoine Tenart wrote:
> > > > > > > Quoting Leon Romanovsky (2022-10-14 13:03:57)
> > > > > > > > On Fri, Oct 14, 2022 at 09:43:45AM +0200, Sabrina Dubroca wrote:
> > > > > > > > > 2022-10-14, 09:13:39 +0300, Leon Romanovsky wrote:
> > > > > > > > > > On Thu, Oct 13, 2022 at 04:15:38PM +0200, Sabrina Dubroca wrote:
> > > > 
> > > > <...>
> > > > 
> > > > > > > - With the revert: IPsec and MACsec can be offloaded to the lower dev.
> > > > > > >   Some features might not propagate to the MACsec dev, which won't allow
> > > > > > >   some performance optimizations in the MACsec data path.
> > > > > > 
> > > > > > My concern is related to this sentence: "it's not possible to offload macsec
> > > > > > to lower devices that also support ipsec offload", because our devices support
> > > > > > both macsec and IPsec offloads at the same time.
> > > > > > 
> > > > > > I don't want to see anything (even in commit messages) that assumes that IPsec
> > > > > > offload doesn't exist.
> > > > > 
> > > > > I don't understand what you're saying here. Patch #1 from this series
> > > > > is exactly about the macsec device acknowledging that ipsec offload
> > > > > exists. The rest of the patches is strictly macsec stuff and says
> > > > > nothing about ipsec. Can you point out where, in this series, I'm
> > > > > claiming that ipsec offload doesn't exist?
> > > > 
> > > > All this conversation is about one sentence, which I cited above - "it's not possible
> > > > to offload macsec to lower devices that also support ipsec offload". From the comments,
> > > > I think that you wanted to say "macsec offload is not working due to performance
> > > > optimization, where IPsec offload feature flag was exposed from lower device." Did I get
> > > > it correctly, now?
> > > 
> > > Yes. "In the current state" (that I wrote in front of the sentence you
> > > quoted) refers to the changes introduced by commit c850240b6c41. The
> > > details are present in the commit message for patch 1.
> > > 
> > > Do you object to the revert, if I rephrase the justification, and then
> > > re-add the features that make sense in net-next?
> > 
> > I don't have any objections.
> 
> Would this be ok for the cover letter?
> 
>     ----
>     The first patch is a revert of commit c850240b6c41 ("net: macsec:
>     report real_dev features when HW offloading is enabled"). That
>     commit tried to improve the performance of macsec offload by
>     taking advantage of some of the NIC's features, but in doing so,
>     broke macsec offload when the lower device supports both macsec
>     and ipsec offload, as the ipsec offload feature flags were copied
>     from the real device. Since the macsec device doesn't provide
>     xdo_* ops, the XFRM core rejects the registration of the new
>     macsec device in xfrm_api_check.
> 
>     I'm working on re-adding those feature flags when offload is
>     available, but I haven't fully solved that yet. I think it would
>     be safer to do that second part in net-next considering how
>     complex feature interactions tend to be.
>     ----
> 
> Do you want something added to the commit message of the revert as
> well?

It will be great to see this sentence "commit tried to improve ...
device in xfrm_api_check." in that revert patch. It makes everything
clearer.

Thanks