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

[Sabrina Dubroca <sd@queasysnail.net>]

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:
> > > > > > I'm working on a dummy offload for macsec on netdevsim. It just has a
> > > > > > small SecY and RXSC table so I can trigger failures easily on the
> > > > > > ndo_* side. It has exposed a couple of issues.
> > > > > > 
> > > > > > The first patch will cause some performance degradation, but in the
> > > > > > current state it's not possible to offload macsec to lower devices
> > > > > > that also support ipsec offload. 
> > > > > 
> > > > > Please don't, IPsec offload is available and undergoing review.
> > > > > https://lore.kernel.org/netdev/cover.1662295929.git.leonro@nvidia.com/
> > > > > 
> > > > > This is whole series (XFRM + driver) for IPsec full offload.
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/leon/linux-rdma.git/log/?h=xfrm-next
> > > 
> > > > That patchset is also doing nothing to address the issue I'm refering
> > > > to here, where xfrm_api_check rejects the macsec device because it has
> > > > the NETIF_F_HW_ESP flag (passed from the lower device) and no xfrmdev_ops.
> > > 
> > > Of course, why do you think that IPsec series should address MACsec bugs?
> > 
> > I was looking at this and the series LGTM. I don't get the above
> > concern, can you clarify?
> > 
> > If a lower device has both IPsec & MACsec offload capabilities:
> > 
> > - Without the revert: IPsec can be offloaded to the lower dev, MACsec
> >   can't. That's a bug.
> 
> And how does it possible that mlx5 macsec offload work?

Well, I don't know. AFAICT, xfrm_api_check will be called for every
new net_device created in the system, so a new macsec device with
NETIF_F_HW_ESP will be rejected. Am I missing something?

I don't have access to mlx5 NICs with macsec offload so I can't check
how that would work. My guess is that for some reason, the mlx5 driver
didn't expose the NETIF_F_HW_ESP feature (CONFIG_MLX5_EN_IPSEC=n?).
The only other possibility I see is CONFIG_XFRM=n.

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

-- 
Sabrina