[PATCH net-next v5 0/5] Support MACsec VLAN

[PATCH net-next v5 0/5] Support MACsec VLAN

[Emeel Hakim]

Dear maintainers,

This patch series introduces support for hardware (HW) offload MACsec
devices with VLAN configuration. The patches address both scenarios
where the VLAN header is both the inner and outer header for MACsec.

The changes include:

1. Adding MACsec offload operation for VLAN.
2. Considering VLAN when accessing MACsec net device.
3. Currently offloading MACsec when it's configured over VLAN with
current MACsec TX steering rules would wrongly insert the MACsec sec tag
after inserting the VLAN header. This resulted in an ETHERNET | SECTAG |
VLAN packet when ETHERNET | VLAN | SECTAG is configured. The patche
handles this issue when configuring steering rules.
4. Adding MACsec rx_handler change support in case of a marked skb and a
mismatch on the dst MAC address.

Please review these changes and let me know if you have any feedback or
concerns.

Updates since v1:
- Consult vlan_features when adding NETIF_F_HW_MACSEC.
- Allow grep for the functions.
- Add helper function to get the macsec operation to allow the compiler
  to make some choice.

Updates since v2:
- Don't use macros to allow direct navigattion from mdo functions to its
  implementation.
- Make the vlan_get_macsec_ops argument a const.
- Check if the specific mdo function is available before calling it.
- Enable NETIF_F_HW_MACSEC by default when the lower device has it enabled
  and in case the lower device currently has NETIF_F_HW_MACSEC but disabled
  let the new vlan device also have it disabled.

Updates since v3:
- Split patch ("vlan: Add MACsec offload operations for VLAN interface")
  to prevent mixing generic vlan code changes with driver changes.
- Add mdo_open, stop and stats to support drivers which have those.
- Don't fail if macsec offload operations are available but a specific
  function is not, to support drivers which does not implement all
  macsec offload operations.
- Don't call find_rx_sc twice in the same loop, instead save the result
  in a parameter and re-use it.
- Completely remove _BUILD_VLAN_MACSEC_MDO macro, to prevent returning
  from a macro.
- Reorder the functions inside struct macsec_ops to match the struct
  decleration.
  
 Updates since v4:
 - Change subject line of ("macsec: Add MACsec rx_handler change support") and adapt commit message.
 - Don't separate the new check in patch ("macsec: Add MACsec rx_handler change support")
   from the previous if/else if.
 - Drop"_found" from the parameter naming "rx_sc_found" and move the definition to
   the relevant block.
 - Remove "{}" since not needed around a single line.

Emeel Hakim (5):
  vlan: Add MACsec offload operations for VLAN interface
  net/mlx5: Enable MACsec offload feature for VLAN interface
  net/mlx5: Support MACsec over VLAN
  net/mlx5: Consider VLAN interface in MACsec TX steering rules
  macsec: Don't rely solely on the dst MAC address to identify
    destination MACsec device

 .../mellanox/mlx5/core/en_accel/macsec.c      |  42 +--
 .../mellanox/mlx5/core/en_accel/macsec_fs.c   |   7 +
 .../net/ethernet/mellanox/mlx5/core/en_main.c |   1 +
 drivers/net/macsec.c                          |  13 +-
 net/8021q/vlan_dev.c                          | 242 ++++++++++++++++++
 5 files changed, 287 insertions(+), 18 deletions(-)

-- 
2.21.3

[PATCH net-next v5 1/5] vlan: Add MACsec offload operations for VLAN interface

[Emeel Hakim]

Add support for MACsec offload operations for VLAN driver
to allow offloading MACsec when VLAN's real device supports
Macsec offload by forwarding the offload request to it.

Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
---
 net/8021q/vlan_dev.c | 242 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 242 insertions(+)

diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 5920544e93e8..870e4935d6e6 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -26,6 +26,7 @@
 #include <linux/ethtool.h>
 #include <linux/phy.h>
 #include <net/arp.h>
+#include <net/macsec.h>
 
 #include "vlan.h"
 #include "vlanproc.h"
@@ -572,6 +573,9 @@ static int vlan_dev_init(struct net_device *dev)
 			   NETIF_F_HIGHDMA | NETIF_F_SCTP_CRC |
 			   NETIF_F_ALL_FCOE;
 
+	if (real_dev->vlan_features & NETIF_F_HW_MACSEC)
+		dev->hw_features |= NETIF_F_HW_MACSEC;
+
 	dev->features |= dev->hw_features | NETIF_F_LLTX;
 	netif_inherit_tso_max(dev, real_dev);
 	if (dev->features & NETIF_F_VLAN_FEATURES)
@@ -803,6 +807,241 @@ static int vlan_dev_fill_forward_path(struct net_device_path_ctx *ctx,
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_MACSEC)
+
+static const struct macsec_ops *vlan_get_macsec_ops(const struct macsec_context *ctx)
+{
+	return vlan_dev_priv(ctx->netdev)->real_dev->macsec_ops;
+}
+
+static int vlan_macsec_offload(int (* const func)(struct macsec_context *),
+			       struct macsec_context *ctx)
+{
+	if (unlikely(!func))
+		return 0;
+
+	return (*func)(ctx);
+}
+
+static int vlan_macsec_dev_open(struct macsec_context *ctx)
+{
+	const struct macsec_ops *ops = vlan_get_macsec_ops(ctx);
+
+	if (!ops)
+		return -EOPNOTSUPP;
+
+	return vlan_macsec_offload(ops->mdo_dev_open, ctx);
+}
+
+static int vlan_macsec_dev_stop(struct macsec_context *ctx)
+{
+	const struct macsec_ops *ops = vlan_get_macsec_ops(ctx);
+
+	if (!ops)
+		return -EOPNOTSUPP;
+
+	return vlan_macsec_offload(ops->mdo_dev_stop, ctx);
+}
+
+static int vlan_macsec_add_secy(struct macsec_context *ctx)
+{
+	const struct macsec_ops *ops = vlan_get_macsec_ops(ctx);
+
+	if (!ops)
+		return -EOPNOTSUPP;
+
+	return vlan_macsec_offload(ops->mdo_add_secy, ctx);
+}
+
+static int vlan_macsec_upd_secy(struct macsec_context *ctx)
+{
+	const struct macsec_ops *ops = vlan_get_macsec_ops(ctx);
+
+	if (!ops)
+		return -EOPNOTSUPP;
+
+	return vlan_macsec_offload(ops->mdo_upd_secy, ctx);
+}
+
+static int vlan_macsec_del_secy(struct macsec_context *ctx)
+{
+	const struct macsec_ops *ops = vlan_get_macsec_ops(ctx);
+
+	if (!ops)
+		return -EOPNOTSUPP;
+
+	return vlan_macsec_offload(ops->mdo_del_secy, ctx);
+}
+
+static int vlan_macsec_add_rxsc(struct macsec_context *ctx)
+{
+	const struct macsec_ops *ops = vlan_get_macsec_ops(ctx);
+
+	if (!ops)
+		return -EOPNOTSUPP;
+
+	return vlan_macsec_offload(ops->mdo_add_rxsc, ctx);
+}
+
+static int vlan_macsec_upd_rxsc(struct macsec_context *ctx)
+{
+	const struct macsec_ops *ops = vlan_get_macsec_ops(ctx);
+
+	if (!ops)
+		return -EOPNOTSUPP;
+
+	return vlan_macsec_offload(ops->mdo_upd_rxsc, ctx);
+}
+
+static int vlan_macsec_del_rxsc(struct macsec_context *ctx)
+{
+	const struct macsec_ops *ops = vlan_get_macsec_ops(ctx);
+
+	if (!ops)
+		return -EOPNOTSUPP;
+
+	return vlan_macsec_offload(ops->mdo_del_rxsc, ctx);
+}
+
+static int vlan_macsec_add_rxsa(struct macsec_context *ctx)
+{
+	const struct macsec_ops *ops = vlan_get_macsec_ops(ctx);
+
+	if (!ops)
+		return -EOPNOTSUPP;
+
+	return vlan_macsec_offload(ops->mdo_add_rxsa, ctx);
+}
+
+static int vlan_macsec_upd_rxsa(struct macsec_context *ctx)
+{
+	const struct macsec_ops *ops = vlan_get_macsec_ops(ctx);
+
+	if (!ops)
+		return -EOPNOTSUPP;
+
+	return vlan_macsec_offload(ops->mdo_upd_rxsa, ctx);
+}
+
+static int vlan_macsec_del_rxsa(struct macsec_context *ctx)
+{
+	const struct macsec_ops *ops = vlan_get_macsec_ops(ctx);
+
+	if (!ops)
+		return -EOPNOTSUPP;
+
+	return vlan_macsec_offload(ops->mdo_del_rxsa, ctx);
+}
+
+static int vlan_macsec_add_txsa(struct macsec_context *ctx)
+{
+	const struct macsec_ops *ops = vlan_get_macsec_ops(ctx);
+
+	if (!ops)
+		return -EOPNOTSUPP;
+
+	return vlan_macsec_offload(ops->mdo_add_txsa, ctx);
+}
+
+static int vlan_macsec_upd_txsa(struct macsec_context *ctx)
+{
+	const struct macsec_ops *ops = vlan_get_macsec_ops(ctx);
+
+	if (!ops)
+		return -EOPNOTSUPP;
+
+	return vlan_macsec_offload(ops->mdo_upd_txsa, ctx);
+}
+
+static int vlan_macsec_del_txsa(struct macsec_context *ctx)
+{
+	const struct macsec_ops *ops = vlan_get_macsec_ops(ctx);
+
+	if (!ops)
+		return -EOPNOTSUPP;
+
+	return vlan_macsec_offload(ops->mdo_del_txsa, ctx);
+}
+
+static int vlan_macsec_get_dev_stats(struct macsec_context *ctx)
+{
+	const struct macsec_ops *ops = vlan_get_macsec_ops(ctx);
+
+	if (!ops)
+		return -EOPNOTSUPP;
+
+	return vlan_macsec_offload(ops->mdo_get_dev_stats, ctx);
+}
+
+static int vlan_macsec_get_tx_sc_stats(struct macsec_context *ctx)
+{
+	const struct macsec_ops *ops = vlan_get_macsec_ops(ctx);
+
+	if (!ops)
+		return -EOPNOTSUPP;
+
+	return vlan_macsec_offload(ops->mdo_get_tx_sc_stats, ctx);
+}
+
+static int vlan_macsec_get_tx_sa_stats(struct macsec_context *ctx)
+{
+	const struct macsec_ops *ops = vlan_get_macsec_ops(ctx);
+
+	if (!ops)
+		return -EOPNOTSUPP;
+
+	return vlan_macsec_offload(ops->mdo_get_tx_sa_stats, ctx);
+}
+
+static int vlan_macsec_get_rx_sc_stats(struct macsec_context *ctx)
+{
+	const struct macsec_ops *ops = vlan_get_macsec_ops(ctx);
+
+	if (!ops)
+		return -EOPNOTSUPP;
+
+	return vlan_macsec_offload(ops->mdo_get_rx_sc_stats, ctx);
+}
+
+static int vlan_macsec_get_rx_sa_stats(struct macsec_context *ctx)
+{
+	const struct macsec_ops *ops = vlan_get_macsec_ops(ctx);
+
+	if (!ops)
+		return -EOPNOTSUPP;
+
+	return vlan_macsec_offload(ops->mdo_get_rx_sa_stats, ctx);
+}
+
+static const struct macsec_ops macsec_offload_ops = {
+	/* Device wide */
+	.mdo_dev_open = vlan_macsec_dev_open,
+	.mdo_dev_stop = vlan_macsec_dev_stop,
+	/* SecY */
+	.mdo_add_secy = vlan_macsec_add_secy,
+	.mdo_upd_secy = vlan_macsec_upd_secy,
+	.mdo_del_secy = vlan_macsec_del_secy,
+	/* Security channels */
+	.mdo_add_rxsc = vlan_macsec_add_rxsc,
+	.mdo_upd_rxsc = vlan_macsec_upd_rxsc,
+	.mdo_del_rxsc = vlan_macsec_del_rxsc,
+	/* Security associations */
+	.mdo_add_rxsa = vlan_macsec_add_rxsa,
+	.mdo_upd_rxsa = vlan_macsec_upd_rxsa,
+	.mdo_del_rxsa = vlan_macsec_del_rxsa,
+	.mdo_add_txsa = vlan_macsec_add_txsa,
+	.mdo_upd_txsa = vlan_macsec_upd_txsa,
+	.mdo_del_txsa = vlan_macsec_del_txsa,
+	/* Statistics */
+	.mdo_get_dev_stats = vlan_macsec_get_dev_stats,
+	.mdo_get_tx_sc_stats = vlan_macsec_get_tx_sc_stats,
+	.mdo_get_tx_sa_stats = vlan_macsec_get_tx_sa_stats,
+	.mdo_get_rx_sc_stats = vlan_macsec_get_rx_sc_stats,
+	.mdo_get_rx_sa_stats = vlan_macsec_get_rx_sa_stats,
+};
+
+#endif
+
 static const struct ethtool_ops vlan_ethtool_ops = {
 	.get_link_ksettings	= vlan_ethtool_get_link_ksettings,
 	.get_drvinfo	        = vlan_ethtool_get_drvinfo,
@@ -869,6 +1108,9 @@ void vlan_setup(struct net_device *dev)
 	dev->priv_destructor	= vlan_dev_free;
 	dev->ethtool_ops	= &vlan_ethtool_ops;
 
+#if IS_ENABLED(CONFIG_MACSEC)
+	dev->macsec_ops		= &macsec_offload_ops;
+#endif
 	dev->min_mtu		= 0;
 	dev->max_mtu		= ETH_MAX_MTU;
 
-- 
2.21.3

[PATCH net-next v5 2/5] net/mlx5: Enable MACsec offload feature for VLAN interface

[Emeel Hakim]

Enable MACsec offload feature over VLAN by adding NETIF_F_HW_MACSEC
to the device vlan_features.

Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index ec72743b64e2..1b4b4afa9dc7 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -5125,6 +5125,7 @@ static void mlx5e_build_nic_netdev(struct net_device *netdev)
 
 	netdev->vlan_features    |= NETIF_F_SG;
 	netdev->vlan_features    |= NETIF_F_HW_CSUM;
+	netdev->vlan_features    |= NETIF_F_HW_MACSEC;
 	netdev->vlan_features    |= NETIF_F_GRO;
 	netdev->vlan_features    |= NETIF_F_TSO;
 	netdev->vlan_features    |= NETIF_F_TSO6;
-- 
2.21.3

[PATCH net-next v5 3/5] net/mlx5: Support MACsec over VLAN

[Emeel Hakim]

MACsec device may have a VLAN device on top of it.
Detect MACsec state correctly under this condition,
and return the correct net device accordingly.

Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
---
 .../mellanox/mlx5/core/en_accel/macsec.c      | 42 ++++++++++++-------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
index 33b3620ea45c..79b523ded87b 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
@@ -4,6 +4,7 @@
 #include <linux/mlx5/device.h>
 #include <linux/mlx5/mlx5_ifc.h>
 #include <linux/xarray.h>
+#include <linux/if_vlan.h>
 
 #include "en.h"
 #include "lib/aso.h"
@@ -348,12 +349,21 @@ static void mlx5e_macsec_cleanup_sa(struct mlx5e_macsec *macsec,
 	sa->macsec_rule = NULL;
 }
 
+static inline struct mlx5e_priv *macsec_netdev_priv(const struct net_device *dev)
+{
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
+	if (is_vlan_dev(dev))
+		return netdev_priv(vlan_dev_priv(dev)->real_dev);
+#endif
+	return netdev_priv(dev);
+}
+
 static int mlx5e_macsec_init_sa(struct macsec_context *ctx,
 				struct mlx5e_macsec_sa *sa,
 				bool encrypt,
 				bool is_tx)
 {
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	struct mlx5e_macsec *macsec = priv->macsec;
 	struct mlx5_macsec_rule_attrs rule_attrs;
 	struct mlx5_core_dev *mdev = priv->mdev;
@@ -427,7 +437,7 @@ static int macsec_rx_sa_active_update(struct macsec_context *ctx,
 				      struct mlx5e_macsec_sa *rx_sa,
 				      bool active)
 {
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	struct mlx5e_macsec *macsec = priv->macsec;
 	int err = 0;
 
@@ -508,9 +518,9 @@ static void update_macsec_epn(struct mlx5e_macsec_sa *sa, const struct macsec_ke
 
 static int mlx5e_macsec_add_txsa(struct macsec_context *ctx)
 {
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	const struct macsec_tx_sc *tx_sc = &ctx->secy->tx_sc;
 	const struct macsec_tx_sa *ctx_tx_sa = ctx->sa.tx_sa;
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
 	const struct macsec_secy *secy = ctx->secy;
 	struct mlx5e_macsec_device *macsec_device;
 	struct mlx5_core_dev *mdev = priv->mdev;
@@ -583,9 +593,9 @@ static int mlx5e_macsec_add_txsa(struct macsec_context *ctx)
 
 static int mlx5e_macsec_upd_txsa(struct macsec_context *ctx)
 {
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	const struct macsec_tx_sc *tx_sc = &ctx->secy->tx_sc;
 	const struct macsec_tx_sa *ctx_tx_sa = ctx->sa.tx_sa;
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
 	struct mlx5e_macsec_device *macsec_device;
 	u8 assoc_num = ctx->sa.assoc_num;
 	struct mlx5e_macsec_sa *tx_sa;
@@ -645,7 +655,7 @@ static int mlx5e_macsec_upd_txsa(struct macsec_context *ctx)
 
 static int mlx5e_macsec_del_txsa(struct macsec_context *ctx)
 {
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	struct mlx5e_macsec_device *macsec_device;
 	u8 assoc_num = ctx->sa.assoc_num;
 	struct mlx5e_macsec_sa *tx_sa;
@@ -696,7 +706,7 @@ static u32 mlx5e_macsec_get_sa_from_hashtable(struct rhashtable *sci_hash, sci_t
 static int mlx5e_macsec_add_rxsc(struct macsec_context *ctx)
 {
 	struct mlx5e_macsec_rx_sc_xarray_element *sc_xarray_element;
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	const struct macsec_rx_sc *ctx_rx_sc = ctx->rx_sc;
 	struct mlx5e_macsec_device *macsec_device;
 	struct mlx5e_macsec_rx_sc *rx_sc;
@@ -776,7 +786,7 @@ static int mlx5e_macsec_add_rxsc(struct macsec_context *ctx)
 
 static int mlx5e_macsec_upd_rxsc(struct macsec_context *ctx)
 {
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	const struct macsec_rx_sc *ctx_rx_sc = ctx->rx_sc;
 	struct mlx5e_macsec_device *macsec_device;
 	struct mlx5e_macsec_rx_sc *rx_sc;
@@ -854,7 +864,7 @@ static void macsec_del_rxsc_ctx(struct mlx5e_macsec *macsec, struct mlx5e_macsec
 
 static int mlx5e_macsec_del_rxsc(struct macsec_context *ctx)
 {
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	struct mlx5e_macsec_device *macsec_device;
 	struct mlx5e_macsec_rx_sc *rx_sc;
 	struct mlx5e_macsec *macsec;
@@ -890,8 +900,8 @@ static int mlx5e_macsec_del_rxsc(struct macsec_context *ctx)
 
 static int mlx5e_macsec_add_rxsa(struct macsec_context *ctx)
 {
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	const struct macsec_rx_sa *ctx_rx_sa = ctx->sa.rx_sa;
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
 	struct mlx5e_macsec_device *macsec_device;
 	struct mlx5_core_dev *mdev = priv->mdev;
 	u8 assoc_num = ctx->sa.assoc_num;
@@ -976,8 +986,8 @@ static int mlx5e_macsec_add_rxsa(struct macsec_context *ctx)
 
 static int mlx5e_macsec_upd_rxsa(struct macsec_context *ctx)
 {
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	const struct macsec_rx_sa *ctx_rx_sa = ctx->sa.rx_sa;
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
 	struct mlx5e_macsec_device *macsec_device;
 	u8 assoc_num = ctx->sa.assoc_num;
 	struct mlx5e_macsec_rx_sc *rx_sc;
@@ -1033,7 +1043,7 @@ static int mlx5e_macsec_upd_rxsa(struct macsec_context *ctx)
 
 static int mlx5e_macsec_del_rxsa(struct macsec_context *ctx)
 {
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	struct mlx5e_macsec_device *macsec_device;
 	sci_t sci = ctx->sa.rx_sa->sc->sci;
 	struct mlx5e_macsec_rx_sc *rx_sc;
@@ -1085,7 +1095,7 @@ static int mlx5e_macsec_del_rxsa(struct macsec_context *ctx)
 
 static int mlx5e_macsec_add_secy(struct macsec_context *ctx)
 {
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	const struct net_device *dev = ctx->secy->netdev;
 	const struct net_device *netdev = ctx->netdev;
 	struct mlx5e_macsec_device *macsec_device;
@@ -1137,7 +1147,7 @@ static int mlx5e_macsec_add_secy(struct macsec_context *ctx)
 static int macsec_upd_secy_hw_address(struct macsec_context *ctx,
 				      struct mlx5e_macsec_device *macsec_device)
 {
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	const struct net_device *dev = ctx->secy->netdev;
 	struct mlx5e_macsec *macsec = priv->macsec;
 	struct mlx5e_macsec_rx_sc *rx_sc, *tmp;
@@ -1184,8 +1194,8 @@ static int macsec_upd_secy_hw_address(struct macsec_context *ctx,
  */
 static int mlx5e_macsec_upd_secy(struct macsec_context *ctx)
 {
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	const struct macsec_tx_sc *tx_sc = &ctx->secy->tx_sc;
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
 	const struct net_device *dev = ctx->secy->netdev;
 	struct mlx5e_macsec_device *macsec_device;
 	struct mlx5e_macsec_sa *tx_sa;
@@ -1240,7 +1250,7 @@ static int mlx5e_macsec_upd_secy(struct macsec_context *ctx)
 
 static int mlx5e_macsec_del_secy(struct macsec_context *ctx)
 {
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	struct mlx5e_macsec_device *macsec_device;
 	struct mlx5e_macsec_rx_sc *rx_sc, *tmp;
 	struct mlx5e_macsec_sa *tx_sa;
@@ -1741,7 +1751,7 @@ void mlx5e_macsec_offload_handle_rx_skb(struct net_device *netdev,
 {
 	struct mlx5e_macsec_rx_sc_xarray_element *sc_xarray_element;
 	u32 macsec_meta_data = be32_to_cpu(cqe->ft_metadata);
-	struct mlx5e_priv *priv = netdev_priv(netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(netdev);
 	struct mlx5e_macsec_rx_sc *rx_sc;
 	struct mlx5e_macsec *macsec;
 	u32  fs_id;
-- 
2.21.3

[PATCH net-next v5 4/5] net/mlx5: Consider VLAN interface in MACsec TX steering rules

[Emeel Hakim]

Offloading MACsec when its configured over VLAN with current MACsec
TX steering rules will wrongly insert MACsec sec tag after inserting
the VLAN header leading to a ETHERNET | SECTAG | VLAN packet when
ETHERNET | VLAN | SECTAG is configured.

The above issue is due to adding the SECTAG by HW which is a later
stage compared to the VLAN header insertion stage.

Detect such a case and adjust TX steering rules to insert the
SECTAG in the correct place by using reformat_param_0 field in
the packet reformat to indicate the offset of SECTAG from end of
the MAC header to account for VLANs in granularity of 4Bytes.

Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
---
 .../net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c   | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
index 9173b67becef..7fc901a6ec5f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
@@ -4,6 +4,7 @@
 #include <net/macsec.h>
 #include <linux/netdevice.h>
 #include <linux/mlx5/qp.h>
+#include <linux/if_vlan.h>
 #include "fs_core.h"
 #include "en/fs.h"
 #include "en_accel/macsec_fs.h"
@@ -508,6 +509,8 @@ static void macsec_fs_tx_del_rule(struct mlx5e_macsec_fs *macsec_fs,
 	macsec_fs_tx_ft_put(macsec_fs);
 }
 
+#define MLX5_REFORMAT_PARAM_ADD_MACSEC_OFFSET_4_BYTES 1
+
 static union mlx5e_macsec_rule *
 macsec_fs_tx_add_rule(struct mlx5e_macsec_fs *macsec_fs,
 		      const struct macsec_context *macsec_ctx,
@@ -553,6 +556,10 @@ macsec_fs_tx_add_rule(struct mlx5e_macsec_fs *macsec_fs,
 	reformat_params.type = MLX5_REFORMAT_TYPE_ADD_MACSEC;
 	reformat_params.size = reformat_size;
 	reformat_params.data = reformatbf;
+
+	if (is_vlan_dev(macsec_ctx->netdev))
+		reformat_params.param_0 = MLX5_REFORMAT_PARAM_ADD_MACSEC_OFFSET_4_BYTES;
+
 	flow_act.pkt_reformat = mlx5_packet_reformat_alloc(macsec_fs->mdev,
 							   &reformat_params,
 							   MLX5_FLOW_NAMESPACE_EGRESS_MACSEC);
-- 
2.21.3

[PATCH net-next v5 5/5] macsec: Don't rely solely on the dst MAC address to identify destination MACsec device

[Emeel Hakim]

Offloading device drivers will mark offloaded MACsec SKBs with the
corresponding SCI in the skb_metadata_dst so the macsec rx handler will
know to which interface to divert those skbs, in case of a marked skb
and a mismatch on the dst MAC address, divert the skb to the macsec
net_device where the macsec rx_handler will be called to consider cases
where relying solely on the dst MAC address is insufficient.

One such instance is when using MACsec with a VLAN as an inner
header, where the packet structure is ETHERNET | SECTAG | VLAN.
In such a scenario, the dst MAC address in the ethernet header
will correspond to the VLAN MAC address, resulting in a mismatch.

Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
---
 drivers/net/macsec.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 25616247d7a5..c19a45dc6977 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -1021,8 +1021,11 @@ static enum rx_handler_result handle_not_macsec(struct sk_buff *skb)
 		 * the SecTAG, so we have to deduce which port to deliver to.
 		 */
 		if (macsec_is_offloaded(macsec) && netif_running(ndev)) {
-			if (md_dst && md_dst->type == METADATA_MACSEC &&
-			    (!find_rx_sc(&macsec->secy, md_dst->u.macsec_info.sci)))
+			struct macsec_rx_sc *rx_sc = (md_dst && md_dst->type == METADATA_MACSEC) ?
+						     find_rx_sc(&macsec->secy,
+								md_dst->u.macsec_info.sci) : NULL;
+
+			if (md_dst && md_dst->type == METADATA_MACSEC && !rx_sc)
 				continue;
 
 			if (ether_addr_equal_64bits(hdr->h_dest,
@@ -1047,7 +1050,13 @@ static enum rx_handler_result handle_not_macsec(struct sk_buff *skb)
 					nskb->pkt_type = PACKET_MULTICAST;
 
 				__netif_rx(nskb);
+			} else if (rx_sc) {
+				skb->dev = ndev;
+				skb->pkt_type = PACKET_HOST;
+				ret = RX_HANDLER_ANOTHER;
+				goto out;
 			}
+
 			continue;
 		}
 
-- 
2.21.3

Re: [PATCH net-next v5 0/5] Support MACsec VLAN

[Jacob Keller]

On 4/13/2023 3:56 AM, Emeel Hakim wrote:
> Dear maintainers,
> 
> This patch series introduces support for hardware (HW) offload MACsec
> devices with VLAN configuration. The patches address both scenarios
> where the VLAN header is both the inner and outer header for MACsec.
> 
> The changes include:
> 
> 1. Adding MACsec offload operation for VLAN.
> 2. Considering VLAN when accessing MACsec net device.
> 3. Currently offloading MACsec when it's configured over VLAN with
> current MACsec TX steering rules would wrongly insert the MACsec sec tag
> after inserting the VLAN header. This resulted in an ETHERNET | SECTAG |
> VLAN packet when ETHERNET | VLAN | SECTAG is configured. The patche
> handles this issue when configuring steering rules.
> 4. Adding MACsec rx_handler change support in case of a marked skb and a
> mismatch on the dst MAC address.
> 
> Please review these changes and let me know if you have any feedback or
> concerns.
> 
> Updates since v1:
> - Consult vlan_features when adding NETIF_F_HW_MACSEC.
> - Allow grep for the functions.
> - Add helper function to get the macsec operation to allow the compiler
>   to make some choice.
> 
> Updates since v2:
> - Don't use macros to allow direct navigattion from mdo functions to its
>   implementation.
> - Make the vlan_get_macsec_ops argument a const.
> - Check if the specific mdo function is available before calling it.
> - Enable NETIF_F_HW_MACSEC by default when the lower device has it enabled
>   and in case the lower device currently has NETIF_F_HW_MACSEC but disabled
>   let the new vlan device also have it disabled.
> 
> Updates since v3:
> - Split patch ("vlan: Add MACsec offload operations for VLAN interface")
>   to prevent mixing generic vlan code changes with driver changes.
> - Add mdo_open, stop and stats to support drivers which have those.
> - Don't fail if macsec offload operations are available but a specific
>   function is not, to support drivers which does not implement all
>   macsec offload operations.
> - Don't call find_rx_sc twice in the same loop, instead save the result
>   in a parameter and re-use it.
> - Completely remove _BUILD_VLAN_MACSEC_MDO macro, to prevent returning
>   from a macro.
> - Reorder the functions inside struct macsec_ops to match the struct
>   decleration.
>   
>  Updates since v4:
>  - Change subject line of ("macsec: Add MACsec rx_handler change support") and adapt commit message.
>  - Don't separate the new check in patch ("macsec: Add MACsec rx_handler change support")
>    from the previous if/else if.
>  - Drop"_found" from the parameter naming "rx_sc_found" and move the definition to
>    the relevant block.
>  - Remove "{}" since not needed around a single line.
> 
> Emeel Hakim (5):
>   vlan: Add MACsec offload operations for VLAN interface
>   net/mlx5: Enable MACsec offload feature for VLAN interface
>   net/mlx5: Support MACsec over VLAN
>   net/mlx5: Consider VLAN interface in MACsec TX steering rules
>   macsec: Don't rely solely on the dst MAC address to identify
>     destination MACsec device
> 

For the whole series:

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

>  .../mellanox/mlx5/core/en_accel/macsec.c      |  42 +--
>  .../mellanox/mlx5/core/en_accel/macsec_fs.c   |   7 +
>  .../net/ethernet/mellanox/mlx5/core/en_main.c |   1 +
>  drivers/net/macsec.c                          |  13 +-
>  net/8021q/vlan_dev.c                          | 242 ++++++++++++++++++
>  5 files changed, 287 insertions(+), 18 deletions(-)
> 

RE: [PATCH net-next v5 5/5] macsec: Don't rely solely on the dst MAC address to identify destination MACsec device

[Subbaraya Sundeep Bhatta]

Hi,

>-----Original Message-----
>From: Emeel Hakim <ehakim@nvidia.com> <ehakim@nvidia.com>
>Sent: Thursday, April 13, 2023 4:26 PM
>To: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
>edumazet@google.com; sd@queasysnail.net
>Cc: netdev@vger.kernel.org; leon@kernel.org; Emeel Hakim
><ehakim@nvidia.com>
>Subject: [PATCH net-next v5 5/5] macsec: Don't rely solely on the dst MAC
>address to identify destination MACsec device
>
>Offloading device drivers will mark offloaded MACsec SKBs with the
>corresponding SCI in the skb_metadata_dst so the macsec rx handler will know to
>which interface to divert those skbs, in case of a marked skb and a mismatch on
>the dst MAC address, divert the skb to the macsec net_device where the macsec
>rx_handler will be called to consider cases where relying solely on the dst MAC
>address is insufficient.
>
>One such instance is when using MACsec with a VLAN as an inner header, where
>the packet structure is ETHERNET | SECTAG | VLAN.
>In such a scenario, the dst MAC address in the ethernet header will correspond to
>the VLAN MAC address, resulting in a mismatch.
>

I did below commands:
ifconfig eth2 up
ip link add link eth2 macsec0 type macsec sci cacbcd4142430002
ifconfig macsec0 hw ether ca:cb:cd:41:42:43
ip macsec offload macsec0 mac
ifconfig macsec0 up
ip macsec add macsec0 tx sa 0 on pn 5 key 02 22222222222222222222222222222222
ip macsec add macsec0 rx sci cacbcd2122230001
ip macsec add macsec0 rx sci cacbcd2122230001 sa 0 pn 5 on key 01 11111111111111111111111111111111
ip link add link macsec0 vlan0 type vlan id 2

ifconfig vlan0 hw ether ca:cb:cd:21:22:23
ifconfig vlan0 up
[ 7106.072451] device macsec0 entered promiscuous mode
[ 7106.077330] device eth2 entered promiscuous mode

macsec0 entered promisc mode when upper_dev mac address is not equal to its mac.
I think we should check if macsec device is in promisc mode instead of omitting mac address compare.
Also all drivers/hardware do not support md_dst->type == METADATA_MACSEC hence if macsec is
offloaded and in promisc mode then go for another round.
Correct me if I am wrong.

Thanks,
Sundeep

>Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
>---
> drivers/net/macsec.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c index
>25616247d7a5..c19a45dc6977 100644
>--- a/drivers/net/macsec.c
>+++ b/drivers/net/macsec.c
>@@ -1021,8 +1021,11 @@ static enum rx_handler_result
>handle_not_macsec(struct sk_buff *skb)
> 		 * the SecTAG, so we have to deduce which port to deliver to.
> 		 */
> 		if (macsec_is_offloaded(macsec) && netif_running(ndev)) {
>-			if (md_dst && md_dst->type == METADATA_MACSEC &&
>-			    (!find_rx_sc(&macsec->secy, md_dst-
>>u.macsec_info.sci)))
>+			struct macsec_rx_sc *rx_sc = (md_dst && md_dst->type
>== METADATA_MACSEC) ?
>+						     find_rx_sc(&macsec->secy,
>+								md_dst-
>>u.macsec_info.sci) : NULL;
>+
>+			if (md_dst && md_dst->type == METADATA_MACSEC &&
>!rx_sc)
> 				continue;
>
> 			if (ether_addr_equal_64bits(hdr->h_dest,
>@@ -1047,7 +1050,13 @@ static enum rx_handler_result
>handle_not_macsec(struct sk_buff *skb)
> 					nskb->pkt_type = PACKET_MULTICAST;
>
> 				__netif_rx(nskb);
>+			} else if (rx_sc) {
>+				skb->dev = ndev;
>+				skb->pkt_type = PACKET_HOST;
>+				ret = RX_HANDLER_ANOTHER;
>+				goto out;
> 			}
>+
> 			continue;
> 		}
>
>--
>2.21.3
>

Re: [PATCH net-next v5 5/5] macsec: Don't rely solely on the dst MAC address to identify destination MACsec device

[Jakub Kicinski]

On Thu, 13 Apr 2023 13:56:22 +0300 Emeel Hakim wrote:
> +			struct macsec_rx_sc *rx_sc = (md_dst && md_dst->type == METADATA_MACSEC) ?
> +						     find_rx_sc(&macsec->secy,
> +								md_dst->u.macsec_info.sci) : NULL;

Just a coding nit, in addition to Subbaraya's question:
why use a ternary operator if the entire expression ends up being 
3 lines of code? :| And well above 80 char.

RE: [PATCH net-next v5 5/5] macsec: Don't rely solely on the dst MAC address to identify destination MACsec device

[Emeel Hakim]

> -----Original Message-----
> From: Subbaraya Sundeep Bhatta <sbhatta@marvell.com>
> Sent: Friday, 14 April 2023 9:32
> To: Emeel Hakim <ehakim@nvidia.com>; davem@davemloft.net;
> kuba@kernel.org; pabeni@redhat.com; edumazet@google.com;
> sd@queasysnail.net
> Cc: netdev@vger.kernel.org; leon@kernel.org; Sunil Kovvuri Goutham
> <sgoutham@marvell.com>; Naveen Mamindlapalli <naveenm@marvell.com>
> Subject: RE: [PATCH net-next v5 5/5] macsec: Don't rely solely on the dst MAC
> address to identify destination MACsec device
> 
> External email: Use caution opening links or attachments
> 
> 
> Hi,
> 
> >-----Original Message-----
> >From: Emeel Hakim <ehakim@nvidia.com> <ehakim@nvidia.com>
> >Sent: Thursday, April 13, 2023 4:26 PM
> >To: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
> >edumazet@google.com; sd@queasysnail.net
> >Cc: netdev@vger.kernel.org; leon@kernel.org; Emeel Hakim
> ><ehakim@nvidia.com>
> >Subject: [PATCH net-next v5 5/5] macsec: Don't rely solely on the dst
> >MAC address to identify destination MACsec device
> >
> >Offloading device drivers will mark offloaded MACsec SKBs with the
> >corresponding SCI in the skb_metadata_dst so the macsec rx handler will
> >know to which interface to divert those skbs, in case of a marked skb
> >and a mismatch on the dst MAC address, divert the skb to the macsec
> >net_device where the macsec rx_handler will be called to consider cases
> >where relying solely on the dst MAC address is insufficient.
> >
> >One such instance is when using MACsec with a VLAN as an inner header,
> >where the packet structure is ETHERNET | SECTAG | VLAN.
> >In such a scenario, the dst MAC address in the ethernet header will
> >correspond to the VLAN MAC address, resulting in a mismatch.
> >
> 
> I did below commands:
> ifconfig eth2 up
> ip link add link eth2 macsec0 type macsec sci cacbcd4142430002 ifconfig macsec0
> hw ether ca:cb:cd:41:42:43 ip macsec offload macsec0 mac ifconfig macsec0 up ip
> macsec add macsec0 tx sa 0 on pn 5 key 02 22222222222222222222222222222222
> ip macsec add macsec0 rx sci cacbcd2122230001 ip macsec add macsec0 rx sci
> cacbcd2122230001 sa 0 pn 5 on key 01 11111111111111111111111111111111 ip link
> add link macsec0 vlan0 type vlan id 2
> 
> ifconfig vlan0 hw ether ca:cb:cd:21:22:23 ifconfig vlan0 up [ 7106.072451] device
> macsec0 entered promiscuous mode [ 7106.077330] device eth2 entered
> promiscuous mode
> 
> macsec0 entered promisc mode when upper_dev mac address is not equal to its
> mac.
> I think we should check if macsec device is in promisc mode instead of omitting
> mac address compare.
> Also all drivers/hardware do not support md_dst->type == METADATA_MACSEC
> hence if macsec is offloaded and in promisc mode then go for another round.
> Correct me if I am wrong.
> 
> Thanks,
> Sundeep

Hi thanks for bringing this up, 

I'm new to this region so correct me if I'm wrong,  IIUC we are entering promisc mode since the upper_dev mac address is not equal to its
mac hence we won't have a match, and in the current code (of this patch) we are covered  in case of existing metadata, however in case of
a driver/hardware which does not support metadata this approach does not cover the promisc mode case.

IMHO I think we should stick to the metadata approach since in the future we might use it to improve macsec offload
(such as allowing to have same mac address for both the physical and macsec device and still to be able to send traffic from the physical device this requires depending solely on metadata)
however this does not prevent supporting drivers/hardwares which does not support metadata.
so If I understood you correctly the following approach should cover both cases:

@@ -1047,7 +1050,13 @@ static enum rx_handler_result
handle_not_macsec(struct sk_buff *skb)

                           ...

                               __netif_rx(nskb);
+                      } else if (rx_sc || ndev->flags & IFF_PROMISC) {
+                              skb->dev = ndev;
+                              skb->pkt_type = PACKET_HOST;
+                              ret = RX_HANDLER_ANOTHER;
+                              goto out;
                       }
+
                       continue;
               }

if this is not what you meant can you please clarify what did you mean with "go for another round" in " if macsec is offloaded and in promisc mode then go for another round."

Thanks,
Emeel

> >Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
> >---
> > drivers/net/macsec.c | 13 +++++++++++--
> > 1 file changed, 11 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c index
> >25616247d7a5..c19a45dc6977 100644
> >--- a/drivers/net/macsec.c
> >+++ b/drivers/net/macsec.c
> >@@ -1021,8 +1021,11 @@ static enum rx_handler_result
> >handle_not_macsec(struct sk_buff *skb)
> >                * the SecTAG, so we have to deduce which port to deliver to.
> >                */
> >               if (macsec_is_offloaded(macsec) && netif_running(ndev)) {
> >-                      if (md_dst && md_dst->type == METADATA_MACSEC &&
> >-                          (!find_rx_sc(&macsec->secy, md_dst-
> >>u.macsec_info.sci)))
> >+                      struct macsec_rx_sc *rx_sc = (md_dst &&
> >+ md_dst->type
> >== METADATA_MACSEC) ?
> >+                                                   find_rx_sc(&macsec->secy,
> >+                                                              md_dst-
> >>u.macsec_info.sci) : NULL;
> >+
> >+                      if (md_dst && md_dst->type == METADATA_MACSEC &&
> >!rx_sc)
> >                               continue;
> >
> >                       if (ether_addr_equal_64bits(hdr->h_dest,
> >@@ -1047,7 +1050,13 @@ static enum rx_handler_result
> >handle_not_macsec(struct sk_buff *skb)
> >                                       nskb->pkt_type =
> >PACKET_MULTICAST;
> >
> >                               __netif_rx(nskb);
> >+                      } else if (rx_sc) {
> >+                              skb->dev = ndev;
> >+                              skb->pkt_type = PACKET_HOST;
> >+                              ret = RX_HANDLER_ANOTHER;
> >+                              goto out;
> >                       }
> >+
> >                       continue;
> >               }
> >
> >--
> >2.21.3
> >

RE: [PATCH net-next v5 5/5] macsec: Don't rely solely on the dst MAC address to identify destination MACsec device

[Emeel Hakim]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Friday, 14 April 2023 18:01
> To: Emeel Hakim <ehakim@nvidia.com>
> Cc: davem@davemloft.net; pabeni@redhat.com; edumazet@google.com;
> sd@queasysnail.net; netdev@vger.kernel.org; leon@kernel.org
> Subject: Re: [PATCH net-next v5 5/5] macsec: Don't rely solely on the dst MAC
> address to identify destination MACsec device
> 
> External email: Use caution opening links or attachments
> 
> 
> On Thu, 13 Apr 2023 13:56:22 +0300 Emeel Hakim wrote:
> > +                     struct macsec_rx_sc *rx_sc = (md_dst && md_dst->type ==
> METADATA_MACSEC) ?
> > +                                                  find_rx_sc(&macsec->secy,
> > +                                                             md_dst->u.macsec_info.sci) : NULL;
> 
> Just a coding nit, in addition to Subbaraya's question:
> why use a ternary operator if the entire expression ends up being
> 3 lines of code? :| And well above 80 char.

right, maybe this is a better approach?

 struct macsec_rx_sc *rx_sc = NULL;

if (md_dst && md_dst->type == METADATA_MACSEC) 
        rx_sc  = find_rx_sc(&macsec->secy, md_dst->u.macsec_info.sci);

what do you think?

RE: [PATCH net-next v5 5/5] macsec: Don't rely solely on the dst MAC address to identify destination MACsec device

[Subbaraya Sundeep Bhatta]

Hi,

>-----Original Message-----
>From: Emeel Hakim <ehakim@nvidia.com>
>Sent: Sunday, April 16, 2023 5:45 PM
>To: Subbaraya Sundeep Bhatta <sbhatta@marvell.com>;
>davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
>edumazet@google.com; sd@queasysnail.net
>Cc: netdev@vger.kernel.org; leon@kernel.org; Sunil Kovvuri Goutham
><sgoutham@marvell.com>; Naveen Mamindlapalli <naveenm@marvell.com>
>Subject: [EXT] RE: [PATCH net-next v5 5/5] macsec: Don't rely solely on the dst
>MAC address to identify destination MACsec device
>
>
>
>> -----Original Message-----
>> From: Subbaraya Sundeep Bhatta <sbhatta@marvell.com>
>> Sent: Friday, 14 April 2023 9:32
>> To: Emeel Hakim <ehakim@nvidia.com>; davem@davemloft.net;
>> kuba@kernel.org; pabeni@redhat.com; edumazet@google.com;
>> sd@queasysnail.net
>> Cc: netdev@vger.kernel.org; leon@kernel.org; Sunil Kovvuri Goutham
>> <sgoutham@marvell.com>; Naveen Mamindlapalli
><naveenm@marvell.com>
>> Subject: RE: [PATCH net-next v5 5/5] macsec: Don't rely solely on the
>> dst MAC address to identify destination MACsec device
>>
>> External email: Use caution opening links or attachments
>>
>>
>> Hi,
>>
>> >-----Original Message-----
>> >From: Emeel Hakim <ehakim@nvidia.com> <ehakim@nvidia.com>
>> >Sent: Thursday, April 13, 2023 4:26 PM
>> >To: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
>> >edumazet@google.com; sd@queasysnail.net
>> >Cc: netdev@vger.kernel.org; leon@kernel.org; Emeel Hakim
>> ><ehakim@nvidia.com>
>> >Subject: [PATCH net-next v5 5/5] macsec: Don't rely solely on the dst
>> >MAC address to identify destination MACsec device
>> >
>> >Offloading device drivers will mark offloaded MACsec SKBs with the
>> >corresponding SCI in the skb_metadata_dst so the macsec rx handler
>> >will know to which interface to divert those skbs, in case of a
>> >marked skb and a mismatch on the dst MAC address, divert the skb to
>> >the macsec net_device where the macsec rx_handler will be called to
>> >consider cases where relying solely on the dst MAC address is insufficient.
>> >
>> >One such instance is when using MACsec with a VLAN as an inner
>> >header, where the packet structure is ETHERNET | SECTAG | VLAN.
>> >In such a scenario, the dst MAC address in the ethernet header will
>> >correspond to the VLAN MAC address, resulting in a mismatch.
>> >
>>
>> I did below commands:
>> ifconfig eth2 up
>> ip link add link eth2 macsec0 type macsec sci cacbcd4142430002
>> ifconfig macsec0 hw ether ca:cb:cd:41:42:43 ip macsec offload macsec0
>> mac ifconfig macsec0 up ip macsec add macsec0 tx sa 0 on pn 5 key 02
>> 22222222222222222222222222222222 ip macsec add macsec0 rx sci
>> cacbcd2122230001 ip macsec add macsec0 rx sci
>> cacbcd2122230001 sa 0 pn 5 on key 01
>11111111111111111111111111111111
>> ip link add link macsec0 vlan0 type vlan id 2
>>
>> ifconfig vlan0 hw ether ca:cb:cd:21:22:23 ifconfig vlan0 up [
>> 7106.072451] device
>> macsec0 entered promiscuous mode [ 7106.077330] device eth2 entered
>> promiscuous mode
>>
>> macsec0 entered promisc mode when upper_dev mac address is not equal
>> to its mac.
>> I think we should check if macsec device is in promisc mode instead of
>> omitting mac address compare.
>> Also all drivers/hardware do not support md_dst->type ==
>> METADATA_MACSEC hence if macsec is offloaded and in promisc mode
>then go for another round.
>> Correct me if I am wrong.
>>
>> Thanks,
>> Sundeep
>
>Hi thanks for bringing this up,
>
>I'm new to this region so correct me if I'm wrong,  IIUC we are entering
>promisc mode since the upper_dev mac address is not equal to its mac hence
>we won't have a match, and in the current code (of this patch) we are covered
>in case of existing metadata, however in case of a driver/hardware which
>does not support metadata this approach does not cover the promisc mode
>case.
>
>IMHO I think we should stick to the metadata approach since in the future we
>might use it to improve macsec offload (such as allowing to have same mac
>address for both the physical and macsec device and still to be able to send
>traffic from the physical device this requires depending solely on metadata)

Aren't you doing this already in mlx5 driver? I mean, using metadata you can have
same mac for macsec and physical device. Also you can have multiple macsec interfaces
on top of physical interface.

>however this does not prevent supporting drivers/hardwares which does not
>support metadata.
>so If I understood you correctly the following approach should cover both
>cases:
>
>@@ -1047,7 +1050,13 @@ static enum rx_handler_result
>handle_not_macsec(struct sk_buff *skb)
>
>                           ...
>
>                               __netif_rx(nskb);
>+                      } else if (rx_sc || ndev->flags & IFF_PROMISC) {
>+                              skb->dev = ndev;
>+                              skb->pkt_type = PACKET_HOST;
>+                              ret = RX_HANDLER_ANOTHER;
>+                              goto out;
>                       }
>+
>                       continue;
>               }
>
>if this is not what you meant can you please clarify what did you mean with
>"go for another round" in " if macsec is offloaded and in promisc mode then
>go for another round."
>

Yes I meant this. Above changes look good to me. I was just saying that returning RX_HANDLER_ANOTHER from
macsec rx handler will make the code calling it (__netif_receive_skb_core) to do another round of processing.


Thanks,
Sundeep

>Thanks,
>Emeel
>
>> >Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
>> >---
>> > drivers/net/macsec.c | 13 +++++++++++--
>> > 1 file changed, 11 insertions(+), 2 deletions(-)
>> >
>> >diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c index
>> >25616247d7a5..c19a45dc6977 100644
>> >--- a/drivers/net/macsec.c
>> >+++ b/drivers/net/macsec.c
>> >@@ -1021,8 +1021,11 @@ static enum rx_handler_result
>> >handle_not_macsec(struct sk_buff *skb)
>> >                * the SecTAG, so we have to deduce which port to deliver to.
>> >                */
>> >               if (macsec_is_offloaded(macsec) && netif_running(ndev)) {
>> >-                      if (md_dst && md_dst->type == METADATA_MACSEC &&
>> >-                          (!find_rx_sc(&macsec->secy, md_dst-
>> >>u.macsec_info.sci)))
>> >+                      struct macsec_rx_sc *rx_sc = (md_dst &&
>> >+ md_dst->type
>> >== METADATA_MACSEC) ?
>> >+                                                   find_rx_sc(&macsec->secy,
>> >+
>> >+ md_dst-
>> >>u.macsec_info.sci) : NULL;
>> >+
>> >+                      if (md_dst && md_dst->type == METADATA_MACSEC
>> >+ &&
>> >!rx_sc)
>> >                               continue;
>> >
>> >                       if (ether_addr_equal_64bits(hdr->h_dest,
>> >@@ -1047,7 +1050,13 @@ static enum rx_handler_result
>> >handle_not_macsec(struct sk_buff *skb)
>> >                                       nskb->pkt_type =
>> >PACKET_MULTICAST;
>> >
>> >                               __netif_rx(nskb);
>> >+                      } else if (rx_sc) {
>> >+                              skb->dev = ndev;
>> >+                              skb->pkt_type = PACKET_HOST;
>> >+                              ret = RX_HANDLER_ANOTHER;
>> >+                              goto out;
>> >                       }
>> >+
>> >                       continue;
>> >               }
>> >
>> >--
>> >2.21.3
>> >

Re: [PATCH net-next v5 5/5] macsec: Don't rely solely on the dst MAC address to identify destination MACsec device

[Jakub Kicinski]

On Sun, 16 Apr 2023 12:18:05 +0000 Emeel Hakim wrote:
>  struct macsec_rx_sc *rx_sc = NULL;
> 
> if (md_dst && md_dst->type == METADATA_MACSEC) 
>         rx_sc  = find_rx_sc(&macsec->secy, md_dst->u.macsec_info.sci);
> 
> what do you think?

Yes, definitely.

RE: [PATCH net-next v5 5/5] macsec: Don't rely solely on the dst MAC address to identify destination MACsec device

[Emeel Hakim]

> -----Original Message-----
> From: Subbaraya Sundeep Bhatta <sbhatta@marvell.com>
> Sent: Monday, 17 April 2023 20:37
> To: Emeel Hakim <ehakim@nvidia.com>; davem@davemloft.net;
> kuba@kernel.org; pabeni@redhat.com; edumazet@google.com;
> sd@queasysnail.net
> Cc: netdev@vger.kernel.org; leon@kernel.org; Sunil Kovvuri Goutham
> <sgoutham@marvell.com>; Naveen Mamindlapalli <naveenm@marvell.com>
> Subject: RE: [PATCH net-next v5 5/5] macsec: Don't rely solely on the dst MAC
> address to identify destination MACsec device
> 
> External email: Use caution opening links or attachments
> 
> 
> Hi,
> 
> >-----Original Message-----
> >From: Emeel Hakim <ehakim@nvidia.com>
> >Sent: Sunday, April 16, 2023 5:45 PM
> >To: Subbaraya Sundeep Bhatta <sbhatta@marvell.com>;
> >davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
> >edumazet@google.com; sd@queasysnail.net
> >Cc: netdev@vger.kernel.org; leon@kernel.org; Sunil Kovvuri Goutham
> ><sgoutham@marvell.com>; Naveen Mamindlapalli <naveenm@marvell.com>
> >Subject: [EXT] RE: [PATCH net-next v5 5/5] macsec: Don't rely solely on
> >the dst MAC address to identify destination MACsec device
> >
> >
> >
> >> -----Original Message-----
> >> From: Subbaraya Sundeep Bhatta <sbhatta@marvell.com>
> >> Sent: Friday, 14 April 2023 9:32
> >> To: Emeel Hakim <ehakim@nvidia.com>; davem@davemloft.net;
> >> kuba@kernel.org; pabeni@redhat.com; edumazet@google.com;
> >> sd@queasysnail.net
> >> Cc: netdev@vger.kernel.org; leon@kernel.org; Sunil Kovvuri Goutham
> >> <sgoutham@marvell.com>; Naveen Mamindlapalli
> ><naveenm@marvell.com>
> >> Subject: RE: [PATCH net-next v5 5/5] macsec: Don't rely solely on the
> >> dst MAC address to identify destination MACsec device
> >>
> >> External email: Use caution opening links or attachments
> >>
> >>
> >> Hi,
> >>
> >> >-----Original Message-----
> >> >From: Emeel Hakim <ehakim@nvidia.com> <ehakim@nvidia.com>
> >> >Sent: Thursday, April 13, 2023 4:26 PM
> >> >To: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
> >> >edumazet@google.com; sd@queasysnail.net
> >> >Cc: netdev@vger.kernel.org; leon@kernel.org; Emeel Hakim
> >> ><ehakim@nvidia.com>
> >> >Subject: [PATCH net-next v5 5/5] macsec: Don't rely solely on the
> >> >dst MAC address to identify destination MACsec device
> >> >
> >> >Offloading device drivers will mark offloaded MACsec SKBs with the
> >> >corresponding SCI in the skb_metadata_dst so the macsec rx handler
> >> >will know to which interface to divert those skbs, in case of a
> >> >marked skb and a mismatch on the dst MAC address, divert the skb to
> >> >the macsec net_device where the macsec rx_handler will be called to
> >> >consider cases where relying solely on the dst MAC address is insufficient.
> >> >
> >> >One such instance is when using MACsec with a VLAN as an inner
> >> >header, where the packet structure is ETHERNET | SECTAG | VLAN.
> >> >In such a scenario, the dst MAC address in the ethernet header will
> >> >correspond to the VLAN MAC address, resulting in a mismatch.
> >> >
> >>
> >> I did below commands:
> >> ifconfig eth2 up
> >> ip link add link eth2 macsec0 type macsec sci cacbcd4142430002
> >> ifconfig macsec0 hw ether ca:cb:cd:41:42:43 ip macsec offload macsec0
> >> mac ifconfig macsec0 up ip macsec add macsec0 tx sa 0 on pn 5 key 02
> >> 22222222222222222222222222222222 ip macsec add macsec0 rx sci
> >> cacbcd2122230001 ip macsec add macsec0 rx sci
> >> cacbcd2122230001 sa 0 pn 5 on key 01
> >11111111111111111111111111111111
> >> ip link add link macsec0 vlan0 type vlan id 2
> >>
> >> ifconfig vlan0 hw ether ca:cb:cd:21:22:23 ifconfig vlan0 up [
> >> 7106.072451] device
> >> macsec0 entered promiscuous mode [ 7106.077330] device eth2 entered
> >> promiscuous mode
> >>
> >> macsec0 entered promisc mode when upper_dev mac address is not equal
> >> to its mac.
> >> I think we should check if macsec device is in promisc mode instead
> >> of omitting mac address compare.
> >> Also all drivers/hardware do not support md_dst->type ==
> >> METADATA_MACSEC hence if macsec is offloaded and in promisc mode
> >then go for another round.
> >> Correct me if I am wrong.
> >>
> >> Thanks,
> >> Sundeep
> >
> >Hi thanks for bringing this up,
> >
> >I'm new to this region so correct me if I'm wrong,  IIUC we are
> >entering promisc mode since the upper_dev mac address is not equal to
> >its mac hence we won't have a match, and in the current code (of this
> >patch) we are covered in case of existing metadata, however in case of
> >a driver/hardware which does not support metadata this approach does
> >not cover the promisc mode case.
> >
> >IMHO I think we should stick to the metadata approach since in the
> >future we might use it to improve macsec offload (such as allowing to
> >have same mac address for both the physical and macsec device and still
> >to be able to send traffic from the physical device this requires
> >depending solely on metadata)
> 
> Aren't you doing this already in mlx5 driver? I mean, using metadata you can have
> same mac for macsec and physical device. Also you can have multiple macsec
> interfaces on top of physical interface.
> 
> >however this does not prevent supporting drivers/hardwares which does
> >not support metadata.
> >so If I understood you correctly the following approach should cover
> >both
> >cases:
> >
> >@@ -1047,7 +1050,13 @@ static enum rx_handler_result
> >handle_not_macsec(struct sk_buff *skb)
> >
> >                           ...
> >
> >                               __netif_rx(nskb);
> >+                      } else if (rx_sc || ndev->flags & IFF_PROMISC) {
> >+                              skb->dev = ndev;
> >+                              skb->pkt_type = PACKET_HOST;
> >+                              ret = RX_HANDLER_ANOTHER;
> >+                              goto out;
> >                       }
> >+
> >                       continue;
> >               }
> >
> >if this is not what you meant can you please clarify what did you mean
> >with "go for another round" in " if macsec is offloaded and in promisc
> >mode then go for another round."
> >
> 
> Yes I meant this. Above changes look good to me. I was just saying that returning
> RX_HANDLER_ANOTHER from macsec rx handler will make the code calling it
> (__netif_receive_skb_core) to do another round of processing.

ack , I will send a new version with the above changes.

> 
> Thanks,
> Sundeep
> 
> >Thanks,
> >Emeel
> >
> >> >Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
> >> >---
> >> > drivers/net/macsec.c | 13 +++++++++++--
> >> > 1 file changed, 11 insertions(+), 2 deletions(-)
> >> >
> >> >diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c index
> >> >25616247d7a5..c19a45dc6977 100644
> >> >--- a/drivers/net/macsec.c
> >> >+++ b/drivers/net/macsec.c
> >> >@@ -1021,8 +1021,11 @@ static enum rx_handler_result
> >> >handle_not_macsec(struct sk_buff *skb)
> >> >                * the SecTAG, so we have to deduce which port to deliver to.
> >> >                */
> >> >               if (macsec_is_offloaded(macsec) && netif_running(ndev)) {
> >> >-                      if (md_dst && md_dst->type == METADATA_MACSEC &&
> >> >-                          (!find_rx_sc(&macsec->secy, md_dst-
> >> >>u.macsec_info.sci)))
> >> >+                      struct macsec_rx_sc *rx_sc = (md_dst &&
> >> >+ md_dst->type
> >> >== METADATA_MACSEC) ?
> >> >+
> >> >+ find_rx_sc(&macsec->secy,
> >> >+
> >> >+ md_dst-
> >> >>u.macsec_info.sci) : NULL;
> >> >+
> >> >+                      if (md_dst && md_dst->type == METADATA_MACSEC
> >> >+ &&
> >> >!rx_sc)
> >> >                               continue;
> >> >
> >> >                       if (ether_addr_equal_64bits(hdr->h_dest,
> >> >@@ -1047,7 +1050,13 @@ static enum rx_handler_result
> >> >handle_not_macsec(struct sk_buff *skb)
> >> >                                       nskb->pkt_type =
> >> >PACKET_MULTICAST;
> >> >
> >> >                               __netif_rx(nskb);
> >> >+                      } else if (rx_sc) {
> >> >+                              skb->dev = ndev;
> >> >+                              skb->pkt_type = PACKET_HOST;
> >> >+                              ret = RX_HANDLER_ANOTHER;
> >> >+                              goto out;
> >> >                       }
> >> >+
> >> >                       continue;
> >> >               }
> >> >
> >> >--
> >> >2.21.3
> >> >

Re: [PATCH net-next v5 5/5] macsec: Don't rely solely on the dst MAC address to identify destination MACsec device

[Sabrina Dubroca]

2023-04-14, 06:32:28 +0000, Subbaraya Sundeep Bhatta wrote:
> Hi,
> 
> >-----Original Message-----
> >From: Emeel Hakim <ehakim@nvidia.com> <ehakim@nvidia.com>
> >Sent: Thursday, April 13, 2023 4:26 PM
> >To: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
> >edumazet@google.com; sd@queasysnail.net
> >Cc: netdev@vger.kernel.org; leon@kernel.org; Emeel Hakim
> ><ehakim@nvidia.com>
> >Subject: [PATCH net-next v5 5/5] macsec: Don't rely solely on the dst MAC
> >address to identify destination MACsec device
> >
> >Offloading device drivers will mark offloaded MACsec SKBs with the
> >corresponding SCI in the skb_metadata_dst so the macsec rx handler will know to
> >which interface to divert those skbs, in case of a marked skb and a mismatch on
> >the dst MAC address, divert the skb to the macsec net_device where the macsec
> >rx_handler will be called to consider cases where relying solely on the dst MAC
> >address is insufficient.
> >
> >One such instance is when using MACsec with a VLAN as an inner header, where
> >the packet structure is ETHERNET | SECTAG | VLAN.
> >In such a scenario, the dst MAC address in the ethernet header will correspond to
> >the VLAN MAC address, resulting in a mismatch.
> >
> 
> I did below commands:
> ifconfig eth2 up
> ip link add link eth2 macsec0 type macsec sci cacbcd4142430002
> ifconfig macsec0 hw ether ca:cb:cd:41:42:43
> ip macsec offload macsec0 mac
> ifconfig macsec0 up
> ip macsec add macsec0 tx sa 0 on pn 5 key 02 22222222222222222222222222222222
> ip macsec add macsec0 rx sci cacbcd2122230001
> ip macsec add macsec0 rx sci cacbcd2122230001 sa 0 pn 5 on key 01 11111111111111111111111111111111
> ip link add link macsec0 vlan0 type vlan id 2
> 
> ifconfig vlan0 hw ether ca:cb:cd:21:22:23
> ifconfig vlan0 up
> [ 7106.072451] device macsec0 entered promiscuous mode
> [ 7106.077330] device eth2 entered promiscuous mode
> 
> macsec0 entered promisc mode when upper_dev mac address is not equal to its mac.
> I think we should check if macsec device is in promisc mode instead of omitting mac address compare.
> Also all drivers/hardware do not support md_dst->type == METADATA_MACSEC 

Is there a good reason to not make all drivers use metadata? It seems
to me it would be cleaner than trying to guess where a packet belongs
once it reaches the macsec core.

-- 
Sabrina

Re: [PATCH net-next v5 5/5] macsec: Don't rely solely on the dst MAC address to identify destination MACsec device

[Subbaraya Sundeep Bhatta]

Hi,

>-----Original Message-----
>From: Sabrina Dubroca <sd@queasysnail.net>
>Sent: Tuesday, April 18, 2023 4:18 PM
>To: Subbaraya Sundeep Bhatta <sbhatta@marvell.com>
>Cc: ehakim@nvidia.com; davem@davemloft.net; kuba@kernel.org;
>pabeni@redhat.com; edumazet@google.com; netdev@vger.kernel.org;
>leon@kernel.org; Sunil Kovvuri Goutham <sgoutham@marvell.com>; Naveen
>Mamindlapalli <naveenm@marvell.com>; atenart@kernel.org
>Subject: Re: [PATCH net-next v5 5/5] macsec: Don't rely solely on the dst
>MAC address to identify destination MACsec device
>
>2023-04-14, 06:32:28 +0000, Subbaraya Sundeep Bhatta wrote:
>> Hi,
>>
>> >-----Original Message-----
>> >From: Emeel Hakim <ehakim@nvidia.com> <ehakim@nvidia.com>
>> >Sent: Thursday, April 13, 2023 4:26 PM
>> >To: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
>> >edumazet@google.com; sd@queasysnail.net
>> >Cc: netdev@vger.kernel.org; leon@kernel.org; Emeel Hakim
>> ><ehakim@nvidia.com>
>> >Subject: [PATCH net-next v5 5/5] macsec: Don't rely solely on the dst
>> >MAC address to identify destination MACsec device
>> >
>> >Offloading device drivers will mark offloaded MACsec SKBs with the
>> >corresponding SCI in the skb_metadata_dst so the macsec rx handler
>> >will know to which interface to divert those skbs, in case of a
>> >marked skb and a mismatch on the dst MAC address, divert the skb to
>> >the macsec net_device where the macsec rx_handler will be called to
>> >consider cases where relying solely on the dst MAC address is insufficient.
>> >
>> >One such instance is when using MACsec with a VLAN as an inner
>> >header, where the packet structure is ETHERNET | SECTAG | VLAN.
>> >In such a scenario, the dst MAC address in the ethernet header will
>> >correspond to the VLAN MAC address, resulting in a mismatch.
>> >
>>
>> I did below commands:
>> ifconfig eth2 up
>> ip link add link eth2 macsec0 type macsec sci cacbcd4142430002
>> ifconfig macsec0 hw ether ca:cb:cd:41:42:43 ip macsec offload macsec0
>> mac ifconfig macsec0 up ip macsec add macsec0 tx sa 0 on pn 5 key 02
>> 22222222222222222222222222222222 ip macsec add macsec0 rx sci
>> cacbcd2122230001 ip macsec add macsec0 rx sci cacbcd2122230001 sa 0 pn
>> 5 on key 01 11111111111111111111111111111111 ip link add link macsec0
>> vlan0 type vlan id 2
>>
>> ifconfig vlan0 hw ether ca:cb:cd:21:22:23 ifconfig vlan0 up [
>> 7106.072451] device macsec0 entered promiscuous mode [ 7106.077330]
>> device eth2 entered promiscuous mode
>>
>> macsec0 entered promisc mode when upper_dev mac address is not equal
>to its mac.
>> I think we should check if macsec device is in promisc mode instead of
>omitting mac address compare.
>> Also all drivers/hardware do not support md_dst->type ==
>> METADATA_MACSEC
>
>Is there a good reason to not make all drivers use metadata? It seems to me it
>would be cleaner than trying to guess where a packet belongs once it reaches
>the macsec core.
>
We need the support from hardware to use skb metadata.
On ingress side:
Hardware has to send metadata(SCI) as a side band signal to software to detect that packet has
been processed by macsec hardware block.
On egress side:
There has to be a provision for software to instruct hardware to use particular flow id/secy policy.

For our hardware I can support ingress side metadata by preserving sectag(SCI) in macsec hardware and
stripping sectag in driver (retrieve SCI) before giving to macsec core.
But for egress we only have TCAM. A TCAM flow is hit with match criteria of L2 header and action is to choose
corresponding secy policy (SCs, SA keys etc). 

I am checking with our hardware folks for solution wrt egress.

Thanks,
Sundeep

>--
>Sabrina