[PATCH net-next 00/3] Introduce MACsec skb_metadata_dst

[PATCH net-next 00/3] Introduce MACsec skb_metadata_dst

[Lior Nahmanson]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="y", Size: 2326 bytes --]

This patchset introduces MACsec skb_metadata_dst to lay the ground
for MACsec HW offload.

MACsec is an IEEE standard (IEEE 802.1AE) for MAC security.
It defines a way to establish a protocol independent connection
between two hosts with data confidentiality, authenticity and/or
integrity, using GCM-AES. MACsec operates on the Ethernet layer and
as such is a layer 2 protocol, which means it’s designed to secure
traffic within a layer 2 network, including DHCP or ARP requests.

Linux has a software implementation of the MACsec standard and
HW offloading support.
The offloading is re-using the logic, netlink API and data
structures of the existing MACsec software implementation.

For Tx:
In the current MACsec offload implementation, MACsec interfaces shares
the same MAC address by default.
Therefore, HW can't distinguish from which MACsec interface the traffic
originated from.

MACsec stack will use skb_metadata_dst to store the SCI value, which is
unique per MACsec interface, skb_metadat_dst will be used later by the
offloading device driver to associate the SKB with the corresponding
offloaded interface (SCI) to facilitate HW MACsec offload.

For Rx:
Like in the Tx changes, if there are more than one MACsec device with
the same MAC address as in the packet's destination MAC, the packet will
be forward only to one of the devices and not neccessarly to the desired one.

Offloading device driver sets the MACsec skb_metadata_dst sci
field with the appropriaate Rx SCI for each SKB so the MACsec rx handler
will know to which port to divert those skbs, instead of wrongly solely
relaying on dst MAC address comparison.

1) patch 0001-0002, Add support to skb_metadata_dst in MACsec code:
net/macsec: Add MACsec skb_metadata_dst Tx Data path support 
net/macsec: Add MACsec skb_metadata_dst Rx Data path support

2) patch 0003, Move some MACsec driver code for sharing with various
drivers that implements offload:
net/macsec: Move some code for sharing with various drivers that
implements offload

Follow-up patchset for Nvidia MACsec HW offload will be submitted
later on.

 drivers/net/macsec.c       | 54 +++++++++++++++++++-------------------
 include/net/dst_metadata.h | 10 +++++++
 include/net/macsec.h       | 24 +++++++++++++++++
 3 files changed, 61 insertions(+), 27 deletions(-)

-- 
2.21.3

[PATCH 1/3] net/macsec: Add MACsec skb_metadata_dst Tx Data path support

[Lior Nahmanson]

In the current MACsec offload implementation, MACsec interfaces shares
the same MAC address by default.
Therefore, HW can't distinguish from which MACsec interface the traffic
originated from.

MACsec stack will use skb_metadata_dst to store the SCI value, which is
unique per MACsec interface, skb_metadat_dst will be used later by the
offloading device driver to associate the SKB with the corresponding
offloaded interface (SCI) to facilitate HW MACsec offload.

Signed-off-by: Lior Nahmanson <liorna@nvidia.com>
Reviewed-by: Raed Salem <raeds@nvidia.com>
Reviewed-by: Saeed Mahameed <saeedm@nvidia.com>
---
 drivers/net/macsec.c       | 15 +++++++++++++++
 include/net/dst_metadata.h | 10 ++++++++++
 include/net/macsec.h       |  3 +++
 3 files changed, 28 insertions(+)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index f1683ce6b561..4bf7f9870b91 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -18,6 +18,7 @@
 #include <net/sock.h>
 #include <net/gro_cells.h>
 #include <net/macsec.h>
+#include <net/dst_metadata.h>
 #include <linux/phy.h>
 #include <linux/byteorder/generic.h>
 #include <linux/if_arp.h>
@@ -3381,6 +3382,11 @@ static netdev_tx_t macsec_start_xmit(struct sk_buff *skb,
 	int ret, len;
 
 	if (macsec_is_offloaded(netdev_priv(dev))) {
+		struct metadata_dst *md_dst = secy->tx_sc.md_dst;
+
+		skb_dst_drop(skb);
+		dst_hold(&md_dst->dst);
+		skb_dst_set(skb, &md_dst->dst);
 		skb->dev = macsec->real_dev;
 		return dev_queue_xmit(skb);
 	}
@@ -3708,6 +3714,7 @@ static void macsec_free_netdev(struct net_device *dev)
 {
 	struct macsec_dev *macsec = macsec_priv(dev);
 
+	metadata_dst_free(macsec->secy.tx_sc.md_dst);
 	free_percpu(macsec->stats);
 	free_percpu(macsec->secy.tx_sc.stats);
 
@@ -3975,6 +3982,13 @@ static int macsec_add_dev(struct net_device *dev, sci_t sci, u8 icv_len)
 		return -ENOMEM;
 	}
 
+	secy->tx_sc.md_dst = metadata_dst_alloc(0, METADATA_MACSEC, GFP_KERNEL);
+	if (!secy->tx_sc.md_dst) {
+		free_percpu(secy->tx_sc.stats);
+		free_percpu(macsec->stats);
+		return -ENOMEM;
+	}
+
 	if (sci == MACSEC_UNDEF_SCI)
 		sci = dev_to_sci(dev, MACSEC_PORT_ES);
 
@@ -3988,6 +4002,7 @@ static int macsec_add_dev(struct net_device *dev, sci_t sci, u8 icv_len)
 	secy->xpn = DEFAULT_XPN;
 
 	secy->sci = sci;
+	secy->tx_sc.md_dst->u.macsec_info.sci = sci;
 	secy->tx_sc.active = true;
 	secy->tx_sc.encoding_sa = DEFAULT_ENCODING_SA;
 	secy->tx_sc.encrypt = DEFAULT_ENCRYPT;
diff --git a/include/net/dst_metadata.h b/include/net/dst_metadata.h
index adab27ba1ecb..22a6924bf6da 100644
--- a/include/net/dst_metadata.h
+++ b/include/net/dst_metadata.h
@@ -4,11 +4,13 @@
 
 #include <linux/skbuff.h>
 #include <net/ip_tunnels.h>
+#include <net/macsec.h>
 #include <net/dst.h>
 
 enum metadata_type {
 	METADATA_IP_TUNNEL,
 	METADATA_HW_PORT_MUX,
+	METADATA_MACSEC,
 };
 
 struct hw_port_info {
@@ -16,12 +18,17 @@ struct hw_port_info {
 	u32 port_id;
 };
 
+struct macsec_info {
+	sci_t sci;
+};
+
 struct metadata_dst {
 	struct dst_entry		dst;
 	enum metadata_type		type;
 	union {
 		struct ip_tunnel_info	tun_info;
 		struct hw_port_info	port_info;
+		struct macsec_info	macsec_info;
 	} u;
 };
 
@@ -82,6 +89,9 @@ static inline int skb_metadata_dst_cmp(const struct sk_buff *skb_a,
 		return memcmp(&a->u.tun_info, &b->u.tun_info,
 			      sizeof(a->u.tun_info) +
 					 a->u.tun_info.options_len);
+	case METADATA_MACSEC:
+		return memcmp(&a->u.macsec_info, &b->u.macsec_info,
+			      sizeof(a->u.macsec_info));
 	default:
 		return 1;
 	}
diff --git a/include/net/macsec.h b/include/net/macsec.h
index d6fa6b97f6ef..aae6c510df05 100644
--- a/include/net/macsec.h
+++ b/include/net/macsec.h
@@ -20,6 +20,8 @@
 typedef u64 __bitwise sci_t;
 typedef u32 __bitwise ssci_t;
 
+struct metadata_dst;
+
 typedef union salt {
 	struct {
 		u32 ssci;
@@ -193,6 +195,7 @@ struct macsec_tx_sc {
 	bool scb;
 	struct macsec_tx_sa __rcu *sa[MACSEC_NUM_AN];
 	struct pcpu_tx_sc_stats __percpu *stats;
+	struct metadata_dst *md_dst;
 };
 
 /**
-- 
2.21.3

[PATCH 2/3] net/macsec: Add MACsec skb_metadata_dst Rx Data path support

[Lior Nahmanson]

Like in the Tx changes, if there are more than one MACsec device with
the same MAC address as in the packet's destination MAC, the packet will
be forward only to one of the devices and not neccessarly to the desired one.

Offloading device driver sets the MACsec skb_metadata_dst sci
field with the appropriaate Rx SCI for each SKB so the MACsec rx handler
will know to which port to divert those skbs, instead of wrongly solely
relaying on dst MAC address comparison.

Signed-off-by: Lior Nahmanson <liorna@nvidia.com>
Reviewed-by: Raed Salem <raeds@nvidia.com>
Reviewed-by: Saeed Mahameed <saeedm@nvidia.com>
---
 drivers/net/macsec.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 4bf7f9870b91..534459dbc956 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -1001,11 +1001,13 @@ static enum rx_handler_result handle_not_macsec(struct sk_buff *skb)
 	/* Deliver to the uncontrolled port by default */
 	enum rx_handler_result ret = RX_HANDLER_PASS;
 	struct ethhdr *hdr = eth_hdr(skb);
+	struct metadata_dst *md_dst;
 	struct macsec_rxh_data *rxd;
 	struct macsec_dev *macsec;
 
 	rcu_read_lock();
 	rxd = macsec_data_rcu(skb->dev);
+	md_dst = skb_metadata_dst(skb);
 
 	list_for_each_entry_rcu(macsec, &rxd->secys, secys) {
 		struct sk_buff *nskb;
@@ -1016,6 +1018,10 @@ 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)))
+				continue;
+
 			if (ether_addr_equal_64bits(hdr->h_dest,
 						    ndev->dev_addr)) {
 				/* exact match, divert skb to this port */
-- 
2.21.3

[PATCH 3/3] net/macsec: Move some code for sharing with various drivers that implements offload

[Lior Nahmanson]

Move some MACsec infrastructure like defines and functions,
in order to avoid code duplication for future drivers which
implements MACsec offload.

Signed-off-by: Lior Nahmanson <liorna@nvidia.com>
Reviewed-by: Raed Salem <raeds@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
Reviewed-by: Ben Ben-Ishay <benishay@nvidia.com>
Reviewed-by: Saeed Mahameed <saeedm@nvidia.com>
---
 drivers/net/macsec.c | 33 ++++++---------------------------
 include/net/macsec.h | 21 +++++++++++++++++++++
 2 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 534459dbc956..0b898469fc18 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -25,8 +25,6 @@
 
 #include <uapi/linux/if_macsec.h>
 
-#define MACSEC_SCI_LEN 8
-
 /* SecTAG length = macsec_eth_header without the optional SCI */
 #define MACSEC_TAG_LEN 6
 
@@ -47,20 +45,10 @@ struct macsec_eth_header {
 	u8 secure_channel_id[8]; /* optional */
 } __packed;
 
-#define MACSEC_TCI_VERSION 0x80
-#define MACSEC_TCI_ES      0x40 /* end station */
-#define MACSEC_TCI_SC      0x20 /* SCI present */
-#define MACSEC_TCI_SCB     0x10 /* epon */
-#define MACSEC_TCI_E       0x08 /* encryption */
-#define MACSEC_TCI_C       0x04 /* changed text */
-#define MACSEC_AN_MASK     0x03 /* association number */
-#define MACSEC_TCI_CONFID  (MACSEC_TCI_E | MACSEC_TCI_C)
-
 /* minimum secure data length deemed "not short", see IEEE 802.1AE-2006 9.7 */
 #define MIN_NON_SHORT_LEN 48
 
 #define GCM_AES_IV_LEN 12
-#define DEFAULT_ICV_LEN 16
 
 #define for_each_rxsc(secy, sc)				\
 	for (sc = rcu_dereference_bh(secy->rx_sc);	\
@@ -231,7 +219,6 @@ static struct macsec_cb *macsec_skb_cb(struct sk_buff *skb)
 	return (struct macsec_cb *)skb->cb;
 }
 
-#define MACSEC_PORT_ES (htons(0x0001))
 #define MACSEC_PORT_SCB (0x0000)
 #define MACSEC_UNDEF_SCI ((__force sci_t)0xffffffffffffffffULL)
 #define MACSEC_UNDEF_SSCI ((__force ssci_t)0xffffffff)
@@ -246,14 +233,6 @@ static struct macsec_cb *macsec_skb_cb(struct sk_buff *skb)
 #define DEFAULT_ENCODING_SA 0
 #define MACSEC_XPN_MAX_REPLAY_WINDOW (((1 << 30) - 1))
 
-static bool send_sci(const struct macsec_secy *secy)
-{
-	const struct macsec_tx_sc *tx_sc = &secy->tx_sc;
-
-	return tx_sc->send_sci ||
-		(secy->n_rx_sc > 1 && !tx_sc->end_station && !tx_sc->scb);
-}
-
 static sci_t make_sci(const u8 *addr, __be16 port)
 {
 	sci_t sci;
@@ -318,7 +297,7 @@ static void macsec_fill_sectag(struct macsec_eth_header *h,
 	/* with GCM, C/E clear for !encrypt, both set for encrypt */
 	if (tx_sc->encrypt)
 		h->tci_an |= MACSEC_TCI_CONFID;
-	else if (secy->icv_len != DEFAULT_ICV_LEN)
+	else if (secy->icv_len != MACSEC_DEFAULT_ICV_LEN)
 		h->tci_an |= MACSEC_TCI_C;
 
 	h->tci_an |= tx_sc->encoding_sa;
@@ -636,7 +615,7 @@ static struct sk_buff *macsec_encrypt(struct sk_buff *skb,
 
 	unprotected_len = skb->len;
 	eth = eth_hdr(skb);
-	sci_present = send_sci(secy);
+	sci_present = macsec_send_sci(secy);
 	hh = skb_push(skb, macsec_extra_len(sci_present));
 	memmove(hh, eth, 2 * ETH_ALEN);
 
@@ -1270,7 +1249,7 @@ static rx_handler_result_t macsec_handle_frame(struct sk_buff **pskb)
 	/* 10.6.1 if the SC is not found */
 	cbit = !!(hdr->tci_an & MACSEC_TCI_C);
 	if (!cbit)
-		macsec_finalize_skb(skb, DEFAULT_ICV_LEN,
+		macsec_finalize_skb(skb, MACSEC_DEFAULT_ICV_LEN,
 				    macsec_extra_len(macsec_skb_cb(skb)->has_sci));
 
 	list_for_each_entry_rcu(macsec, &rxd->secys, secys) {
@@ -4027,7 +4006,7 @@ static int macsec_newlink(struct net *net, struct net_device *dev,
 {
 	struct macsec_dev *macsec = macsec_priv(dev);
 	rx_handler_func_t *rx_handler;
-	u8 icv_len = DEFAULT_ICV_LEN;
+	u8 icv_len = MACSEC_DEFAULT_ICV_LEN;
 	struct net_device *real_dev;
 	int err, mtu;
 	sci_t sci;
@@ -4151,7 +4130,7 @@ static int macsec_validate_attr(struct nlattr *tb[], struct nlattr *data[],
 				struct netlink_ext_ack *extack)
 {
 	u64 csid = MACSEC_DEFAULT_CIPHER_ID;
-	u8 icv_len = DEFAULT_ICV_LEN;
+	u8 icv_len = MACSEC_DEFAULT_ICV_LEN;
 	int flag;
 	bool es, scb, sci;
 
@@ -4163,7 +4142,7 @@ static int macsec_validate_attr(struct nlattr *tb[], struct nlattr *data[],
 
 	if (data[IFLA_MACSEC_ICV_LEN]) {
 		icv_len = nla_get_u8(data[IFLA_MACSEC_ICV_LEN]);
-		if (icv_len != DEFAULT_ICV_LEN) {
+		if (icv_len != MACSEC_DEFAULT_ICV_LEN) {
 			char dummy_key[DEFAULT_SAK_LEN] = { 0 };
 			struct crypto_aead *dummy_tfm;
 
diff --git a/include/net/macsec.h b/include/net/macsec.h
index aae6c510df05..752374efab83 100644
--- a/include/net/macsec.h
+++ b/include/net/macsec.h
@@ -17,6 +17,20 @@
 #define MACSEC_SALT_LEN 12
 #define MACSEC_NUM_AN 4 /* 2 bits for the association number */
 
+#define MACSEC_SCI_LEN 8
+#define MACSEC_PORT_ES (htons(0x0001))
+
+#define MACSEC_TCI_VERSION 0x80
+#define MACSEC_TCI_ES      0x40 /* end station */
+#define MACSEC_TCI_SC      0x20 /* SCI present */
+#define MACSEC_TCI_SCB     0x10 /* epon */
+#define MACSEC_TCI_E       0x08 /* encryption */
+#define MACSEC_TCI_C       0x04 /* changed text */
+#define MACSEC_AN_MASK     0x03 /* association number */
+#define MACSEC_TCI_CONFID  (MACSEC_TCI_E | MACSEC_TCI_C)
+
+#define MACSEC_DEFAULT_ICV_LEN 16
+
 typedef u64 __bitwise sci_t;
 typedef u32 __bitwise ssci_t;
 
@@ -292,5 +306,12 @@ struct macsec_ops {
 };
 
 void macsec_pn_wrapped(struct macsec_secy *secy, struct macsec_tx_sa *tx_sa);
+static inline bool macsec_send_sci(const struct macsec_secy *secy)
+{
+	const struct macsec_tx_sc *tx_sc = &secy->tx_sc;
+
+	return tx_sc->send_sci ||
+		(secy->n_rx_sc > 1 && !tx_sc->end_station && !tx_sc->scb);
+}
 
 #endif /* _NET_MACSEC_H_ */
-- 
2.21.3

Re: [PATCH 1/3] net/macsec: Add MACsec skb_metadata_dst Tx Data path support

[Jakub Kicinski]

On Thu, 18 Aug 2022 16:24:09 +0300 Lior Nahmanson wrote:
> In the current MACsec offload implementation, MACsec interfaces shares
> the same MAC address by default.
> Therefore, HW can't distinguish from which MACsec interface the traffic
> originated from.
> 
> MACsec stack will use skb_metadata_dst to store the SCI value, which is
> unique per MACsec interface, skb_metadat_dst will be used later by the
> offloading device driver to associate the SKB with the corresponding
> offloaded interface (SCI) to facilitate HW MACsec offload.

struct macsec_tx_sc has a kdoc so you need to document the new field (md_dst).

On a quick (sorry we're behind on patches this week) look I don't see
the driver integration - is it coming later? Or there's already somehow
a driver in the tree using this infra? Normally the infra should be in
the same patchset as the in-tree user.

Last thing - please CC some of the folks who worked on MACsec in the
past, so we can get expert reviews, Antoine and Sabrina come to mind,
look thru the git history please.

RE: [PATCH 1/3] net/macsec: Add MACsec skb_metadata_dst Tx Data path support

[Raed Salem]

>-----Original Message-----
>From: Jakub Kicinski <kuba@kernel.org>
>Sent: Friday, August 19, 2022 7:09 AM
>To: Lior Nahmanson <liorna@nvidia.com>
>Cc: edumazet@google.com; pabeni@redhat.com; davem@davemloft.net;
>netdev@vger.kernel.org; Raed Salem <raeds@nvidia.com>; Saeed Mahameed
><saeedm@nvidia.com>
>Subject: Re: [PATCH 1/3] net/macsec: Add MACsec skb_metadata_dst Tx Data
>path support
>
>External email: Use caution opening links or attachments
>
>
>On Thu, 18 Aug 2022 16:24:09 +0300 Lior Nahmanson wrote:
>> In the current MACsec offload implementation, MACsec interfaces shares
>> the same MAC address by default.
>> Therefore, HW can't distinguish from which MACsec interface the
>> traffic originated from.
>>
>> MACsec stack will use skb_metadata_dst to store the SCI value, which
>> is unique per MACsec interface, skb_metadat_dst will be used later by
>> the offloading device driver to associate the SKB with the
>> corresponding offloaded interface (SCI) to facilitate HW MACsec offload.
>
>struct macsec_tx_sc has a kdoc so you need to document the new field
>(md_dst).
Ack, will do as part of V2
>
>On a quick (sorry we're behind on patches this week) look I don't see the
>driver integration - is it coming later? Or there's already somehow a driver in
>the tree using this infra? Normally the infra should be in the same patchset as
>the in-tree user.
Driver integration series will be submitted later on
>
>Last thing - please CC some of the folks who worked on MACsec in the past,
>so we can get expert reviews, Antoine and Sabrina come to mind, look thru
>the git history please.
Ack, will be added starting form V2

Thanks for the review

Re: [PATCH 1/3] net/macsec: Add MACsec skb_metadata_dst Tx Data path support

[Jakub Kicinski]

On Sun, 21 Aug 2022 11:12:00 +0000 Raed Salem wrote:
> >On a quick (sorry we're behind on patches this week) look I don't see the
> >driver integration - is it coming later? Or there's already somehow a driver in
> >the tree using this infra? Normally the infra should be in the same patchset as
> >the in-tree user.  
> Driver integration series will be submitted later on

This is a requirement, perhaps it'd be good for you to connect with
netdev folks @nvidia to talk thru your plan? Saeed, Gal, Tariq etc.

Re: [PATCH 1/3] net/macsec: Add MACsec skb_metadata_dst Tx Data path support

[Saeed Mahameed]

On 22 Aug 11:10, Jakub Kicinski wrote:
>On Sun, 21 Aug 2022 11:12:00 +0000 Raed Salem wrote:
>> >On a quick (sorry we're behind on patches this week) look I don't see the
>> >driver integration - is it coming later? Or there's already somehow a driver in
>> >the tree using this infra? Normally the infra should be in the same patchset as
>> >the in-tree user.
>> Driver integration series will be submitted later on
>
>This is a requirement, perhaps it'd be good for you to connect with
>netdev folks @nvidia to talk thru your plan? Saeed, Gal, Tariq etc.

driver part is still WIP in terms of maintainer review, but we are very
close, we will submit everything at once in next version.

Thanks.