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

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

[Sabrina Dubroca]

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

Sabrina Dubroca (5):
  Revert "net: macsec: report real_dev features when HW offloading is
    enabled"
  macsec: delete new rxsc when offload fails
  macsec: fix secy->n_rx_sc accounting
  macsec: fix detection of RXSCs when toggling offloading
  macsec: clear encryption keys from the stack after setting up offload

 drivers/net/macsec.c | 51 ++++++++++++++++----------------------------
 1 file changed, 18 insertions(+), 33 deletions(-)

-- 
2.38.0

[PATCH net 1/5] Revert "net: macsec: report real_dev features when HW offloading is enabled"

[Sabrina Dubroca]

This reverts commit c850240b6c4132574a00f2da439277ab94265b66.

Copying the features of the real device doesn't work when it also
supports IPsec offload, because then we'll have a device with
NETIF_F_HW_ESP but no netdev->xfrmdev_ops.

Example perf trace when running
  ip link add link eni1np1 type macsec port 4 offload mac

    ip   737 [003]   795.477676: probe:xfrm_dev_event__REGISTER      name="macsec0" features=0x1c000080014869
              xfrm_dev_event+0x3a
              notifier_call_chain+0x47
              register_netdevice+0x846
              macsec_newlink+0x25a

    ip   737 [003]   795.477687:   probe:xfrm_dev_event__return      ret=0x8002 (NOTIFY_BAD)
             notifier_call_chain+0x47
             register_netdevice+0x846
             macsec_newlink+0x25a

dev->features includes NETIF_F_HW_ESP (0x04000000000000), so
xfrm_api_check returns NOTIFY_BAD because we don't have
dev->xfrmdev_ops on the macsec device.

We could probably propagate GSO and a few other features from the
lower device, similar to macvlan. This will be done in a future patch.

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/macsec.c | 27 ++++-----------------------
 1 file changed, 4 insertions(+), 23 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index c891b60937a7..b3f76e8071f2 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -2654,11 +2654,6 @@ static int macsec_upd_offload(struct sk_buff *skb, struct genl_info *info)
 	if (ret)
 		goto rollback;
 
-	/* Force features update, since they are different for SW MACSec and
-	 * HW offloading cases.
-	 */
-	netdev_update_features(dev);
-
 	rtnl_unlock();
 	return 0;
 
@@ -3432,16 +3427,9 @@ static netdev_tx_t macsec_start_xmit(struct sk_buff *skb,
 	return ret;
 }
 
-#define SW_MACSEC_FEATURES \
+#define MACSEC_FEATURES \
 	(NETIF_F_SG | NETIF_F_HIGHDMA | NETIF_F_FRAGLIST)
 
-/* If h/w offloading is enabled, use real device features save for
- *   VLAN_FEATURES - they require additional ops
- *   HW_MACSEC - no reason to report it
- */
-#define REAL_DEV_FEATURES(dev) \
-	((dev)->features & ~(NETIF_F_VLAN_FEATURES | NETIF_F_HW_MACSEC))
-
 static int macsec_dev_init(struct net_device *dev)
 {
 	struct macsec_dev *macsec = macsec_priv(dev);
@@ -3458,12 +3446,8 @@ static int macsec_dev_init(struct net_device *dev)
 		return err;
 	}
 
-	if (macsec_is_offloaded(macsec)) {
-		dev->features = REAL_DEV_FEATURES(real_dev);
-	} else {
-		dev->features = real_dev->features & SW_MACSEC_FEATURES;
-		dev->features |= NETIF_F_LLTX | NETIF_F_GSO_SOFTWARE;
-	}
+	dev->features = real_dev->features & MACSEC_FEATURES;
+	dev->features |= NETIF_F_LLTX | NETIF_F_GSO_SOFTWARE;
 
 	dev->needed_headroom = real_dev->needed_headroom +
 			       MACSEC_NEEDED_HEADROOM;
@@ -3495,10 +3479,7 @@ static netdev_features_t macsec_fix_features(struct net_device *dev,
 	struct macsec_dev *macsec = macsec_priv(dev);
 	struct net_device *real_dev = macsec->real_dev;
 
-	if (macsec_is_offloaded(macsec))
-		return REAL_DEV_FEATURES(real_dev);
-
-	features &= (real_dev->features & SW_MACSEC_FEATURES) |
+	features &= (real_dev->features & MACSEC_FEATURES) |
 		    NETIF_F_GSO_SOFTWARE | NETIF_F_SOFT_FEATURES;
 	features |= NETIF_F_LLTX;
 
-- 
2.38.0

[PATCH net 2/5] macsec: delete new rxsc when offload fails

[Sabrina Dubroca]

Currently we get an inconsistent state:
 - netlink returns the error to userspace
 - the RXSC is installed but not offloaded

Then the device could get confused when we try to add an RXSA, because
the RXSC isn't supposed to exist.

Fixes: 3cf3227a21d1 ("net: macsec: hardware offloading infrastructure")
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/macsec.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index b3f76e8071f2..0d6fe34b91ae 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -1876,7 +1876,6 @@ static int macsec_add_rxsc(struct sk_buff *skb, struct genl_info *info)
 	struct macsec_rx_sc *rx_sc;
 	struct nlattr *tb_rxsc[MACSEC_RXSC_ATTR_MAX + 1];
 	struct macsec_secy *secy;
-	bool was_active;
 	int ret;
 
 	if (!attrs[MACSEC_ATTR_IFINDEX])
@@ -1904,7 +1903,6 @@ static int macsec_add_rxsc(struct sk_buff *skb, struct genl_info *info)
 		return PTR_ERR(rx_sc);
 	}
 
-	was_active = rx_sc->active;
 	if (tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE])
 		rx_sc->active = !!nla_get_u8(tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE]);
 
@@ -1931,7 +1929,8 @@ static int macsec_add_rxsc(struct sk_buff *skb, struct genl_info *info)
 	return 0;
 
 cleanup:
-	rx_sc->active = was_active;
+	del_rx_sc(secy, sci);
+	free_rx_sc(rx_sc);
 	rtnl_unlock();
 	return ret;
 }
-- 
2.38.0

[PATCH net 3/5] macsec: fix secy->n_rx_sc accounting

[Sabrina Dubroca]

secy->n_rx_sc is supposed to be the number of _active_ rxsc's within a
secy. This is then used by macsec_send_sci to help decide if we should
add the SCI to the header or not.

This logic is currently broken when we create a new RXSC and turn it
off at creation, as create_rx_sc always sets ->active to true (and
immediately uses that to increment n_rx_sc), and only later
macsec_add_rxsc sets rx_sc->active.

Fixes: c09440f7dcb3 ("macsec: introduce IEEE 802.1AE driver")
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/macsec.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 0d6fe34b91ae..cdee342e42cd 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -1413,7 +1413,8 @@ static struct macsec_rx_sc *del_rx_sc(struct macsec_secy *secy, sci_t sci)
 	return NULL;
 }
 
-static struct macsec_rx_sc *create_rx_sc(struct net_device *dev, sci_t sci)
+static struct macsec_rx_sc *create_rx_sc(struct net_device *dev, sci_t sci,
+					 bool active)
 {
 	struct macsec_rx_sc *rx_sc;
 	struct macsec_dev *macsec;
@@ -1437,7 +1438,7 @@ static struct macsec_rx_sc *create_rx_sc(struct net_device *dev, sci_t sci)
 	}
 
 	rx_sc->sci = sci;
-	rx_sc->active = true;
+	rx_sc->active = active;
 	refcount_set(&rx_sc->refcnt, 1);
 
 	secy = &macsec_priv(dev)->secy;
@@ -1876,6 +1877,7 @@ static int macsec_add_rxsc(struct sk_buff *skb, struct genl_info *info)
 	struct macsec_rx_sc *rx_sc;
 	struct nlattr *tb_rxsc[MACSEC_RXSC_ATTR_MAX + 1];
 	struct macsec_secy *secy;
+	bool active = true;
 	int ret;
 
 	if (!attrs[MACSEC_ATTR_IFINDEX])
@@ -1897,15 +1899,16 @@ static int macsec_add_rxsc(struct sk_buff *skb, struct genl_info *info)
 	secy = &macsec_priv(dev)->secy;
 	sci = nla_get_sci(tb_rxsc[MACSEC_RXSC_ATTR_SCI]);
 
-	rx_sc = create_rx_sc(dev, sci);
+
+	if (tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE])
+		active = !!nla_get_u8(tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE]);
+
+	rx_sc = create_rx_sc(dev, sci, active);
 	if (IS_ERR(rx_sc)) {
 		rtnl_unlock();
 		return PTR_ERR(rx_sc);
 	}
 
-	if (tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE])
-		rx_sc->active = !!nla_get_u8(tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE]);
-
 	if (macsec_is_offloaded(netdev_priv(dev))) {
 		const struct macsec_ops *ops;
 		struct macsec_context ctx;
-- 
2.38.0

[PATCH net 4/5] macsec: fix detection of RXSCs when toggling offloading

[Sabrina Dubroca]

macsec_is_configured incorrectly uses secy->n_rx_sc to check if some
RXSCs exist. secy->n_rx_sc only counts the number of active RXSCs, but
there can also be inactive SCs as well, which may be stored in the
driver (in case we're disabling offloading), or would have to be
pushed to the device (in case we're trying to enable offloading).

As long as RXSCs active on creation and never turned off, the issue is
not visible.

Fixes: dcb780fb2795 ("net: macsec: add nla support for changing the offloading selection")
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/macsec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index cdee342e42cd..0ad6eba0a807 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -2572,7 +2572,7 @@ static bool macsec_is_configured(struct macsec_dev *macsec)
 	struct macsec_tx_sc *tx_sc = &secy->tx_sc;
 	int i;
 
-	if (secy->n_rx_sc > 0)
+	if (secy->rx_sc)
 		return true;
 
 	for (i = 0; i < MACSEC_NUM_AN; i++)
-- 
2.38.0

[PATCH net 5/5] macsec: clear encryption keys from the stack after setting up offload

[Sabrina Dubroca]

macsec_add_rxsa and macsec_add_txsa copy the key to an on-stack
offloading context to pass it to the drivers, but leaves it there when
it's done. Clear it with memzero_explicit as soon as it's not needed
anymore.

Fixes: 3cf3227a21d1 ("net: macsec: hardware offloading infrastructure")
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/macsec.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 0ad6eba0a807..333e47b68e02 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -1839,6 +1839,7 @@ static int macsec_add_rxsa(struct sk_buff *skb, struct genl_info *info)
 		       secy->key_len);
 
 		err = macsec_offload(ops->mdo_add_rxsa, &ctx);
+		memzero_explicit(ctx.sa.key, secy->key_len);
 		if (err)
 			goto cleanup;
 	}
@@ -2082,6 +2083,7 @@ static int macsec_add_txsa(struct sk_buff *skb, struct genl_info *info)
 		       secy->key_len);
 
 		err = macsec_offload(ops->mdo_add_txsa, &ctx);
+		memzero_explicit(ctx.sa.key, secy->key_len);
 		if (err)
 			goto cleanup;
 	}
-- 
2.38.0

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

[Leon Romanovsky]

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

> I'm working on re-adding those feature flags when offload is available,
> but I haven't fully solved that yet.

So let's revert when you are ready.

Thanks

Re: [PATCH net 3/5] macsec: fix secy->n_rx_sc accounting

[Leon Romanovsky]

On Thu, Oct 13, 2022 at 04:15:41PM +0200, Sabrina Dubroca wrote:
> secy->n_rx_sc is supposed to be the number of _active_ rxsc's within a
> secy. This is then used by macsec_send_sci to help decide if we should
> add the SCI to the header or not.
> 
> This logic is currently broken when we create a new RXSC and turn it
> off at creation, as create_rx_sc always sets ->active to true (and
> immediately uses that to increment n_rx_sc), and only later
> macsec_add_rxsc sets rx_sc->active.
> 
> Fixes: c09440f7dcb3 ("macsec: introduce IEEE 802.1AE driver")
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> ---
>  drivers/net/macsec.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
> index 0d6fe34b91ae..cdee342e42cd 100644
> --- a/drivers/net/macsec.c
> +++ b/drivers/net/macsec.c
> @@ -1413,7 +1413,8 @@ static struct macsec_rx_sc *del_rx_sc(struct macsec_secy *secy, sci_t sci)
>  	return NULL;
>  }
>  
> -static struct macsec_rx_sc *create_rx_sc(struct net_device *dev, sci_t sci)
> +static struct macsec_rx_sc *create_rx_sc(struct net_device *dev, sci_t sci,
> +					 bool active)
>  {
>  	struct macsec_rx_sc *rx_sc;
>  	struct macsec_dev *macsec;
> @@ -1437,7 +1438,7 @@ static struct macsec_rx_sc *create_rx_sc(struct net_device *dev, sci_t sci)
>  	}
>  
>  	rx_sc->sci = sci;
> -	rx_sc->active = true;
> +	rx_sc->active = active;
>  	refcount_set(&rx_sc->refcnt, 1);
>  
>  	secy = &macsec_priv(dev)->secy;
> @@ -1876,6 +1877,7 @@ static int macsec_add_rxsc(struct sk_buff *skb, struct genl_info *info)
>  	struct macsec_rx_sc *rx_sc;
>  	struct nlattr *tb_rxsc[MACSEC_RXSC_ATTR_MAX + 1];
>  	struct macsec_secy *secy;
> +	bool active = true;
>  	int ret;
>  
>  	if (!attrs[MACSEC_ATTR_IFINDEX])
> @@ -1897,15 +1899,16 @@ static int macsec_add_rxsc(struct sk_buff *skb, struct genl_info *info)
>  	secy = &macsec_priv(dev)->secy;
>  	sci = nla_get_sci(tb_rxsc[MACSEC_RXSC_ATTR_SCI]);
>  
> -	rx_sc = create_rx_sc(dev, sci);
> +
> +	if (tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE])
> +		active = !!nla_get_u8(tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE]);

You don't need !! to assign to bool variables and can safely omit them.

thanks

> +
> +	rx_sc = create_rx_sc(dev, sci, active);
>  	if (IS_ERR(rx_sc)) {
>  		rtnl_unlock();
>  		return PTR_ERR(rx_sc);
>  	}
>  
> -	if (tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE])
> -		rx_sc->active = !!nla_get_u8(tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE]);
> -
>  	if (macsec_is_offloaded(netdev_priv(dev))) {
>  		const struct macsec_ops *ops;
>  		struct macsec_context ctx;
> -- 
> 2.38.0
> 

Re: [PATCH net 3/5] macsec: fix secy->n_rx_sc accounting

[Sabrina Dubroca]

2022-10-14, 09:16:56 +0300, Leon Romanovsky wrote:
[...]
> > @@ -1897,15 +1899,16 @@ static int macsec_add_rxsc(struct sk_buff *skb, struct genl_info *info)
> >  	secy = &macsec_priv(dev)->secy;
> >  	sci = nla_get_sci(tb_rxsc[MACSEC_RXSC_ATTR_SCI]);
> >  
> > -	rx_sc = create_rx_sc(dev, sci);
> > +
> > +	if (tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE])
> > +		active = !!nla_get_u8(tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE]);
> 
> You don't need !! to assign to bool variables and can safely omit them.

Yeah, but I'm just moving existing code, see below.

(please trim your replies, nobody wants to scroll through endless
quoting just to find one comment hiding in the middle)

> 
> thanks
> 
> > +
> > +	rx_sc = create_rx_sc(dev, sci, active);
> >  	if (IS_ERR(rx_sc)) {
> >  		rtnl_unlock();
> >  		return PTR_ERR(rx_sc);
> >  	}
> >  
> > -	if (tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE])
> > -		rx_sc->active = !!nla_get_u8(tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE]);

-- 
Sabrina

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

[Sabrina Dubroca]

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

Yes, and that's not upstream yet. 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.

OTOH the mlx5 driver has both macsec and ipsec offload already, so I
think it should be affected by this exact issue.

There are other feature flags that almost certainly shouldn't be
copied from the lower device, such as the TLS offload flags.

-- 
Sabrina

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

[Leon Romanovsky]

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
> 
> Yes, and that's not upstream yet. 

For this conversation, you can assume what that series is merged.
It is not merged due to request to change how we store XFRM policies
in XFRM core code.

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

Thanks

Re: [PATCH net 3/5] macsec: fix secy->n_rx_sc accounting

[Leon Romanovsky]

On Fri, Oct 14, 2022 at 09:43:41AM +0200, Sabrina Dubroca wrote:
> 2022-10-14, 09:16:56 +0300, Leon Romanovsky wrote:
> [...]
> > > @@ -1897,15 +1899,16 @@ static int macsec_add_rxsc(struct sk_buff *skb, struct genl_info *info)
> > >  	secy = &macsec_priv(dev)->secy;
> > >  	sci = nla_get_sci(tb_rxsc[MACSEC_RXSC_ATTR_SCI]);
> > >  
> > > -	rx_sc = create_rx_sc(dev, sci);
> > > +
> > > +	if (tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE])
> > > +		active = !!nla_get_u8(tb_rxsc[MACSEC_RXSC_ATTR_ACTIVE]);
> > 
> > You don't need !! to assign to bool variables and can safely omit them.
> 
> Yeah, but I'm just moving existing code, see below.

Not really, original code was "rx_sc->active = ...", but you changed to
use local value. So it is perfectly fine to fix the !! too.

Thanks

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

[Antoine Tenart]

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.

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

Thanks,
Antoine

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

[Sabrina Dubroca]

2022-10-14, 14:03:57 +0300, Leon Romanovsky wrote:
> 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
> > 
> > Yes, and that's not upstream yet. 
> 
> For this conversation, you can assume what that series is merged.
> It is not merged due to request to change how we store XFRM policies
> in XFRM core code.
> 
> > 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 don't. That's why I really don't understand how the "full offload"
series is relevant for this patchset.

-- 
Sabrina

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

[Leon Romanovsky]

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?

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

Thanks

> 
> Thanks,
> Antoine

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

[Sabrina Dubroca]

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

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

[Leon Romanovsky]

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?

Thanks

> 
> -- 
> Sabrina
> 

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

[Sabrina Dubroca]

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?

-- 
Sabrina

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

[Leon Romanovsky]

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.

Thanks

> 
> -- 
> Sabrina
> 

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

[Sabrina Dubroca]

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?

Thanks.

-- 
Sabrina

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

[Leon Romanovsky]

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