[PATCH net-next v3 00/3] Introduce MACsec offload SKB extension

[PATCH net-next v3 00/3] Introduce MACsec offload SKB extension

[Lior Nahmanson]

This patchset introduces MACsec SKB extension 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 are
sharing the same MAC address of their parent interface by default.
Therefore, HW can't distinguish if a packet was sent from MACsec
interface and need to be offloaded or not.
Also, it can't distinguish from which MACsec interface it was sent in
case there are multiple MACsec interface with the same MAC address.

Used SKB extension, so SW can mark if a packet is needed to be offloaded
and use the SCI, which is unique value for each MACsec interface,
to notify the HW from which MACsec interface the packet is sent.

For Rx:
Like in the Tx changes, packet that don't have SecTAG
header aren't necessary been offloaded by the HW.
Therefore, the MACsec driver needs to distinguish if the packet
was offloaded or not and handle accordingly.
Moreover, if there are more than one MACsec device with the same MAC
address as in the packet's destination MAC, the packet will forward only
to this device and only to the desired one.

Used SKB extension and marking it by the HW if the packet was offloaded
and to which MACsec offload device it belongs according to the packet's
SCI.

1) patch 0001-0002, Add support to SKB extension in MACsec code:
net/macsec: Add MACsec skb extension Tx Data path support
net/macsec: Add MACsec skb extension 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/Kconfig    |  1 +
 drivers/net/macsec.c   | 45 ++++++++++++++++--------------------------
 include/linux/skbuff.h |  3 +++
 include/net/macsec.h   | 27 +++++++++++++++++++++++++
 net/core/gro.c         | 16 +++++++++++++++
 net/core/skbuff.c      |  7 +++++++
 6 files changed, 71 insertions(+), 28 deletions(-)

-- 
2.25.4

[PATCH net-next v3 1/3] net/macsec: Add MACsec skb extension Tx Data path support

[Lior Nahmanson]

In the current MACsec offload implementation, MACsec interfaces are
sharing the same MAC address of their parent interface by default.
Therefore, HW can't distinguish if a packet was sent from MACsec
interface and need to be offloaded or not.
Also, it can't distinguish from which MACsec interface it was sent in
case there are multiple MACsec interface with the same MAC address.

Used SKB extension, so SW can mark if a packet is needed to be offloaded
and use the SCI, which is unique value for each MACsec interface,
to notify the HW from which MACsec interface the packet is sent.

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>
---
v1->v2:
- removed offloaded field from struct macsec_ext
v2->v3:
- removed Issue and Change-Id from commit message
---
 drivers/net/Kconfig    | 1 +
 drivers/net/macsec.c   | 4 ++++
 include/linux/skbuff.h | 3 +++
 include/net/macsec.h   | 5 +++++
 net/core/skbuff.c      | 7 +++++++
 5 files changed, 20 insertions(+)

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index b2a4f998c180..6c9a950b7010 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -313,6 +313,7 @@ config MACSEC
 	select CRYPTO_AES
 	select CRYPTO_GCM
 	select GRO_CELLS
+	select SKB_EXTENSIONS
 	help
 	   MACsec is an encryption standard for Ethernet.
 
diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index c881e1bf6f6e..9be0606d70da 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -3379,6 +3379,10 @@ static netdev_tx_t macsec_start_xmit(struct sk_buff *skb,
 	int ret, len;
 
 	if (macsec_is_offloaded(netdev_priv(dev))) {
+		struct macsec_ext *secext = skb_ext_add(skb, SKB_EXT_MACSEC);
+
+		secext->sci = secy->sci;
+
 		skb->dev = macsec->real_dev;
 		return dev_queue_xmit(skb);
 	}
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 82edf0359ab3..350693a787ca 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -4495,6 +4495,9 @@ enum skb_ext_id {
 #endif
 #if IS_ENABLED(CONFIG_MCTP_FLOWS)
 	SKB_EXT_MCTP,
+#endif
+#if IS_ENABLED(CONFIG_MACSEC)
+	SKB_EXT_MACSEC,
 #endif
 	SKB_EXT_NUM, /* must be last */
 };
diff --git a/include/net/macsec.h b/include/net/macsec.h
index d6fa6b97f6ef..6de49d9c98bc 100644
--- a/include/net/macsec.h
+++ b/include/net/macsec.h
@@ -20,6 +20,11 @@
 typedef u64 __bitwise sci_t;
 typedef u32 __bitwise ssci_t;
 
+/* MACsec sk_buff extension data */
+struct macsec_ext {
+	sci_t sci;
+};
+
 typedef union salt {
 	struct {
 		u32 ssci;
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index fec75f8bf1f4..640823b5bd2f 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -72,6 +72,7 @@
 #include <net/mptcp.h>
 #include <net/mctp.h>
 #include <net/page_pool.h>
+#include <net/macsec.h>
 
 #include <linux/uaccess.h>
 #include <trace/events/skb.h>
@@ -4346,6 +4347,9 @@ static const u8 skb_ext_type_len[] = {
 #if IS_ENABLED(CONFIG_MCTP_FLOWS)
 	[SKB_EXT_MCTP] = SKB_EXT_CHUNKSIZEOF(struct mctp_flow),
 #endif
+#if IS_ENABLED(CONFIG_MACSEC)
+	[SKB_EXT_MACSEC] = SKB_EXT_CHUNKSIZEOF(struct macsec_ext),
+#endif
 };
 
 static __always_inline unsigned int skb_ext_total_length(void)
@@ -4365,6 +4369,9 @@ static __always_inline unsigned int skb_ext_total_length(void)
 #endif
 #if IS_ENABLED(CONFIG_MCTP_FLOWS)
 		skb_ext_type_len[SKB_EXT_MCTP] +
+#endif
+#if IS_ENABLED(CONFIG_MACSEC)
+		skb_ext_type_len[SKB_EXT_MACSEC] +
 #endif
 		0;
 }
-- 
2.25.4

[PATCH net-next v3 2/3] net/macsec: Add MACsec skb extension Rx Data path support

[Lior Nahmanson]

Like in the Tx changes, packet that don't have SecTAG
header aren't necessary been offloaded by the HW.
Therefore, the MACsec driver needs to distinguish if the packet
was offloaded or not and handle accordingly.
Moreover, if there are more than one MACsec device with the same MAC
address as in the packet's destination MAC, the packet will forward only
to this device and only to the desired one.

Used SKB extension and marking it by the HW if the packet was offloaded
and to which MACsec offload device it belongs according to the packet's
SCI.

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>
---
v1->v2:
- added GRO support
- added offloaded field to struct macsec_ext
v2->v3:
- removed Issue and Change-Id from commit message
---
 drivers/net/macsec.c |  8 +++++++-
 include/net/macsec.h |  1 +
 net/core/gro.c       | 16 ++++++++++++++++
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 9be0606d70da..7b7baf3dd596 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -999,11 +999,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 macsec_ext *macsec_ext;
 	struct macsec_rxh_data *rxd;
 	struct macsec_dev *macsec;
 
 	rcu_read_lock();
 	rxd = macsec_data_rcu(skb->dev);
+	macsec_ext = skb_ext_find(skb, SKB_EXT_MACSEC);
 
 	list_for_each_entry_rcu(macsec, &rxd->secys, secys) {
 		struct sk_buff *nskb;
@@ -1013,7 +1015,11 @@ static enum rx_handler_result handle_not_macsec(struct sk_buff *skb)
 		/* If h/w offloading is enabled, HW decodes frames and strips
 		 * the SecTAG, so we have to deduce which port to deliver to.
 		 */
-		if (macsec_is_offloaded(macsec) && netif_running(ndev)) {
+		if (macsec_is_offloaded(macsec) && netif_running(ndev) &&
+		    (!macsec_ext || macsec_ext->offloaded)) {
+			if ((macsec_ext) && (!find_rx_sc(&macsec->secy, macsec_ext->sci)))
+				continue;
+
 			if (ether_addr_equal_64bits(hdr->h_dest,
 						    ndev->dev_addr)) {
 				/* exact match, divert skb to this port */
diff --git a/include/net/macsec.h b/include/net/macsec.h
index 6de49d9c98bc..fcbca963c04d 100644
--- a/include/net/macsec.h
+++ b/include/net/macsec.h
@@ -23,6 +23,7 @@ typedef u32 __bitwise ssci_t;
 /* MACsec sk_buff extension data */
 struct macsec_ext {
 	sci_t sci;
+	bool offloaded;
 };
 
 typedef union salt {
diff --git a/net/core/gro.c b/net/core/gro.c
index b4190eb08467..f68e950be37f 100644
--- a/net/core/gro.c
+++ b/net/core/gro.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 #include <net/gro.h>
+#include <net/macsec.h>
 #include <net/dst_metadata.h>
 #include <net/busy_poll.h>
 #include <trace/events/net.h>
@@ -390,6 +391,10 @@ static void gro_list_prepare(const struct list_head *head,
 			struct tc_skb_ext *skb_ext;
 			struct tc_skb_ext *p_ext;
 #endif
+#if IS_ENABLED(CONFIG_SKB_EXTENSIONS) && IS_ENABLED(CONFIG_MACSEC)
+			struct macsec_ext *macsec_skb_ext;
+			struct macsec_ext *macsec_p_ext;
+#endif
 
 			diffs |= p->sk != skb->sk;
 			diffs |= skb_metadata_dst_cmp(p, skb);
@@ -402,6 +407,17 @@ static void gro_list_prepare(const struct list_head *head,
 			diffs |= (!!p_ext) ^ (!!skb_ext);
 			if (!diffs && unlikely(skb_ext))
 				diffs |= p_ext->chain ^ skb_ext->chain;
+#endif
+#if IS_ENABLED(CONFIG_SKB_EXTENSIONS) && IS_ENABLED(CONFIG_MACSEC)
+			macsec_skb_ext = skb_ext_find(skb, SKB_EXT_MACSEC);
+			macsec_p_ext = skb_ext_find(p, SKB_EXT_MACSEC);
+
+			diffs |= (!!macsec_p_ext) ^ (!!macsec_skb_ext);
+			if (!diffs && unlikely(macsec_skb_ext)) {
+				diffs |= (__force unsigned long)macsec_p_ext->sci ^
+					 (__force unsigned long)macsec_skb_ext->sci;
+				diffs |= macsec_p_ext->offloaded ^ macsec_skb_ext->offloaded;
+			}
 #endif
 		}
 
-- 
2.25.4

Re: [PATCH net-next v3 2/3] net/macsec: Add MACsec skb extension Rx Data path support

[Paolo Abeni]

On Mon, 2022-06-13 at 14:19 +0300, Lior Nahmanson wrote:
> Like in the Tx changes, packet that don't have SecTAG
> header aren't necessary been offloaded by the HW.
> Therefore, the MACsec driver needs to distinguish if the packet
> was offloaded or not and handle accordingly.
> Moreover, if there are more than one MACsec device with the same MAC
> address as in the packet's destination MAC, the packet will forward only
> to this device and only to the desired one.
> 
> Used SKB extension and marking it by the HW if the packet was offloaded
> and to which MACsec offload device it belongs according to the packet's
> SCI.
> 
> 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>
> ---
> v1->v2:
> - added GRO support
> - added offloaded field to struct macsec_ext
> v2->v3:
> - removed Issue and Change-Id from commit message
> ---
>  drivers/net/macsec.c |  8 +++++++-
>  include/net/macsec.h |  1 +
>  net/core/gro.c       | 16 ++++++++++++++++
>  3 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
> index 9be0606d70da..7b7baf3dd596 100644
> --- a/drivers/net/macsec.c
> +++ b/drivers/net/macsec.c
> @@ -999,11 +999,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 macsec_ext *macsec_ext;
>  	struct macsec_rxh_data *rxd;
>  	struct macsec_dev *macsec;
>  
>  	rcu_read_lock();
>  	rxd = macsec_data_rcu(skb->dev);
> +	macsec_ext = skb_ext_find(skb, SKB_EXT_MACSEC);
>  
>  	list_for_each_entry_rcu(macsec, &rxd->secys, secys) {
>  		struct sk_buff *nskb;
> @@ -1013,7 +1015,11 @@ static enum rx_handler_result handle_not_macsec(struct sk_buff *skb)
>  		/* If h/w offloading is enabled, HW decodes frames and strips
>  		 * the SecTAG, so we have to deduce which port to deliver to.
>  		 */
> -		if (macsec_is_offloaded(macsec) && netif_running(ndev)) {
> +		if (macsec_is_offloaded(macsec) && netif_running(ndev) &&
> +		    (!macsec_ext || macsec_ext->offloaded)) {
> +			if ((macsec_ext) && (!find_rx_sc(&macsec->secy, macsec_ext->sci)))
> +				continue;
> +
>  			if (ether_addr_equal_64bits(hdr->h_dest,
>  						    ndev->dev_addr)) {
>  				/* exact match, divert skb to this port */
> diff --git a/include/net/macsec.h b/include/net/macsec.h
> index 6de49d9c98bc..fcbca963c04d 100644
> --- a/include/net/macsec.h
> +++ b/include/net/macsec.h
> @@ -23,6 +23,7 @@ typedef u32 __bitwise ssci_t;
>  /* MACsec sk_buff extension data */
>  struct macsec_ext {
>  	sci_t sci;
> +	bool offloaded;
>  };
>  
>  typedef union salt {
> diff --git a/net/core/gro.c b/net/core/gro.c
> index b4190eb08467..f68e950be37f 100644
> --- a/net/core/gro.c
> +++ b/net/core/gro.c
> @@ -1,5 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0-or-later
>  #include <net/gro.h>
> +#include <net/macsec.h>
>  #include <net/dst_metadata.h>
>  #include <net/busy_poll.h>
>  #include <trace/events/net.h>
> @@ -390,6 +391,10 @@ static void gro_list_prepare(const struct list_head *head,
>  			struct tc_skb_ext *skb_ext;
>  			struct tc_skb_ext *p_ext;
>  #endif
> +#if IS_ENABLED(CONFIG_SKB_EXTENSIONS) && IS_ENABLED(CONFIG_MACSEC)
> +			struct macsec_ext *macsec_skb_ext;
> +			struct macsec_ext *macsec_p_ext;
> +#endif
>  
>  			diffs |= p->sk != skb->sk;
>  			diffs |= skb_metadata_dst_cmp(p, skb);
> @@ -402,6 +407,17 @@ static void gro_list_prepare(const struct list_head *head,
>  			diffs |= (!!p_ext) ^ (!!skb_ext);
>  			if (!diffs && unlikely(skb_ext))
>  				diffs |= p_ext->chain ^ skb_ext->chain;
> +#endif
> +#if IS_ENABLED(CONFIG_SKB_EXTENSIONS) && IS_ENABLED(CONFIG_MACSEC)
> +			macsec_skb_ext = skb_ext_find(skb, SKB_EXT_MACSEC);
> +			macsec_p_ext = skb_ext_find(p, SKB_EXT_MACSEC);
> +
> +			diffs |= (!!macsec_p_ext) ^ (!!macsec_skb_ext);
> +			if (!diffs && unlikely(macsec_skb_ext)) {
> +				diffs |= (__force unsigned long)macsec_p_ext->sci ^
> +					 (__force unsigned long)macsec_skb_ext->sci;
> +				diffs |= macsec_p_ext->offloaded ^ macsec_skb_ext->offloaded;
> +			}
>  #endif 		}
> 
The main reason I suggested to look for the a possible alternative to
the skb extension is that the GRO stage is becoming bigger (and slower)
with any of such addition.

The 'slow_gro' protects the common use-case from any additional
conditionals and intructions, I still have some concerns due to the
increased code size.

This is not a reject, I'm mostly looking for a 2nd opinion.

Thanks,

Paolo

Re: [PATCH net-next v3 2/3] net/macsec: Add MACsec skb extension Rx Data path support

[Jakub Kicinski]

On Tue, 14 Jun 2022 15:55:13 +0200 Paolo Abeni wrote:
> The main reason I suggested to look for the a possible alternative to
> the skb extension is that the GRO stage is becoming bigger (and slower)
> with any of such addition.
> 
> The 'slow_gro' protects the common use-case from any additional
> conditionals and intructions, I still have some concerns due to the
> increased code size.
> 
> This is not a reject, I'm mostly looking for a 2nd opinion.

Shooting from the hip a little bit, but macsec being a tightly bound L2
upper maybe metadata dst is a workable solution for carrying the sci and
offload status between upper and lower? The range of values should be
well known and limited. 

RE: [PATCH net-next v3 2/3] net/macsec: Add MACsec skb extension Rx Data path support

[Lior Nahmanson]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Tuesday, June 14, 2022 7:15 PM
> To: Paolo Abeni <pabeni@redhat.com>
> Cc: Lior Nahmanson <liorna@nvidia.com>; edumazet@google.com;
> davem@davemloft.net; netdev@vger.kernel.org; Raed Salem
> <raeds@nvidia.com>; Jiri Pirko <jiri@nvidia.com>; Ben Ben Ishay
> <benishay@nvidia.com>
> Subject: Re: [PATCH net-next v3 2/3] net/macsec: Add MACsec skb
> extension Rx Data path support
> 
> External email: Use caution opening links or attachments
> 
> 
> On Tue, 14 Jun 2022 15:55:13 +0200 Paolo Abeni wrote:
> > The main reason I suggested to look for the a possible alternative to
> > the skb extension is that the GRO stage is becoming bigger (and
> > slower) with any of such addition.
> >
> > The 'slow_gro' protects the common use-case from any additional
> > conditionals and intructions, I still have some concerns due to the
> > increased code size.
> >
> > This is not a reject, I'm mostly looking for a 2nd opinion.
> 
> Shooting from the hip a little bit, but macsec being a tightly bound L2 upper
> maybe metadata dst is a workable solution for carrying the sci and offload
> status between upper and lower? The range of values should be well known
> and limited.

Under the assumption that by skb_metadata you meant metadata_dst,
I think there are few reasons why i think is better to use skb extensions:

1. Unlike skb extension, the metadata_dst deallaction is handled directly by the allocator.
Since the sci and offloaded fields are shared between the MACsec driver and the offload driver
(in our case mlx5 driver), for Rx, the metadata_dst allocation is done in the mlx5 driver,
while the dealloction should be done in the MACsec driver.
This is undesired behavior.

2. medadata_dst is attached to the skb using skb_dst_set(), which set the slow_gro bit.
So there is no gain regarding slow_gro flow.

3. metadata_dst allocation require much more memory than needed for MACsec use case
(mainly because struct dst_entry which seems redundant for this case).

Re: [PATCH net-next v3 2/3] net/macsec: Add MACsec skb extension Rx Data path support

[Jakub Kicinski]

On Tue, 21 Jun 2022 12:39:23 +0000 Lior Nahmanson wrote:
> > Shooting from the hip a little bit, but macsec being a tightly bound L2 upper
> > maybe metadata dst is a workable solution for carrying the sci and offload
> > status between upper and lower? The range of values should be well known
> > and limited.  
> 
> Under the assumption that by skb_metadata you meant metadata_dst,

Can you show me in my email where I said skb_metadata?

> I think there are few reasons why i think is better to use skb extensions:
> 
> 1. Unlike skb extension, the metadata_dst deallaction is handled directly by the allocator.
> Since the sci and offloaded fields are shared between the MACsec driver and the offload driver
> (in our case mlx5 driver), for Rx, the metadata_dst allocation is done in the mlx5 driver,
> while the dealloction should be done in the MACsec driver.
> This is undesired behavior.

You allocate metadata skb once and then attach it to the skbs.

> 2. medadata_dst is attached to the skb using skb_dst_set(), which set the slow_gro bit.
> So there is no gain regarding slow_gro flow.
> 
> 3. metadata_dst allocation require much more memory than needed for MACsec use case
> (mainly because struct dst_entry which seems redundant for this case).

RE: [PATCH net-next v3 2/3] net/macsec: Add MACsec skb extension Rx Data path support

[Lior Nahmanson]

 > On Tue, 21 Jun 2022 12:39:23 +0000 Lior Nahmanson wrote:
> > > Shooting from the hip a little bit, but macsec being a tightly bound
> > > L2 upper maybe metadata dst is a workable solution for carrying the
> > > sci and offload status between upper and lower? The range of values
> > > should be well known and limited.
> >
> > Under the assumption that by skb_metadata you meant metadata_dst,
> 
> Can you show me in my email where I said skb_metadata?
> 
> > I think there are few reasons why i think is better to use skb extensions:
> >
> > 1. Unlike skb extension, the metadata_dst deallaction is handled directly by
> the allocator.
> > Since the sci and offloaded fields are shared between the MACsec
> > driver and the offload driver (in our case mlx5 driver), for Rx, the
> > metadata_dst allocation is done in the mlx5 driver, while the dealloction
> should be done in the MACsec driver.
> > This is undesired behavior.
> 
> You allocate metadata skb once and then attach it to the skbs.
> 
> > 2. medadata_dst is attached to the skb using skb_dst_set(), which set the
> slow_gro bit.
> > So there is no gain regarding slow_gro flow.
> >
> > 3. metadata_dst allocation require much more memory than needed for
> > MACsec use case (mainly because struct dst_entry which seems redundant
> for this case).

i considered the usage of skb_metadata_dst, however i still think
that skb_ext will fit more to MACsec offload implementation for the following reasons:
1. for Rx, each skb can have a different SCI and offloaded values which mandate allocation
    of metadata_dst for each skb which contradicts the desired usage for skb_metadata_dst where
    it's allocated once and a refcnt held whenever used.
2. skb_ext method is used in a similar IPsec offload implementation which in the future could make it easier
    to refactor this section to unify all crypto offloads skb_ext usage.

apologize for the late respond.

Re: [PATCH net-next v3 2/3] net/macsec: Add MACsec skb extension Rx Data path support

[Jakub Kicinski]

On Tue, 12 Jul 2022 06:50:52 +0000 Lior Nahmanson wrote:
> i considered the usage of skb_metadata_dst, however i still think
> that skb_ext will fit more to MACsec offload implementation for the following reasons:
> 1. for Rx, each skb can have a different SCI and offloaded values which mandate allocation
>     of metadata_dst for each skb which contradicts the desired usage for skb_metadata_dst where
>     it's allocated once and a refcnt held whenever used.

How many distinct SCIs do you expect to see?

> 2. skb_ext method is used in a similar IPsec offload implementation which in the future could make it easier
>     to refactor this section to unify all crypto offloads skb_ext usage.

MACSec is L2, IPsec has constraints we have to work around.

RE: [PATCH net-next v3 2/3] net/macsec: Add MACsec skb extension Rx Data path support

[Lior Nahmanson]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Wednesday, July 13, 2022 3:02 AM
> To: Lior Nahmanson <liorna@nvidia.com>
> Cc: Paolo Abeni <pabeni@redhat.com>; edumazet@google.com;
> davem@davemloft.net; netdev@vger.kernel.org; Raed Salem
> <raeds@nvidia.com>; Jiri Pirko <jiri@nvidia.com>; Saeed Mahameed
> <saeedm@nvidia.com>; Yossi Kuperman <yossiku@nvidia.com>
> Subject: Re: [PATCH net-next v3 2/3] net/macsec: Add MACsec skb
> extension Rx Data path support
> 
> External email: Use caution opening links or attachments
> 
> 
> On Tue, 12 Jul 2022 06:50:52 +0000 Lior Nahmanson wrote:
> > i considered the usage of skb_metadata_dst, however i still think that
> > skb_ext will fit more to MACsec offload implementation for the following
> reasons:
> > 1. for Rx, each skb can have a different SCI and offloaded values which
> mandate allocation
> >     of metadata_dst for each skb which contradicts the desired usage for
> skb_metadata_dst where
> >     it's allocated once and a refcnt held whenever used.
> 
> How many distinct SCIs do you expect to see?

For Rx there is no limitation for the number of different SCIs.
from MACsec driver code:

struct macsec_secy {
...
     struct macsec_rx_sc __rcu *rx_sc; // each rx_sc contains unique SCI
};

static int macsec_add_rxsc(struct sk_buff *skb, struct genl_info *info)
{
...
    rx_sc = create_rx_sc(dev, sci);
...
}

where create_rx_sc() adds new rx_sc node to the secy->rx_sc list.

> 
> > 2. skb_ext method is used in a similar IPsec offload implementation which
> in the future could make it easier
> >     to refactor this section to unify all crypto offloads skb_ext usage.
> 
> MACSec is L2, IPsec has constraints we have to work around.

Re: [PATCH net-next v3 2/3] net/macsec: Add MACsec skb extension Rx Data path support

[Jakub Kicinski]

On Wed, 13 Jul 2022 06:21:25 +0000 Lior Nahmanson wrote:
> For Rx there is no limitation for the number of different SCIs.
> from MACsec driver code:
> 
> struct macsec_secy {
> ...
>      struct macsec_rx_sc __rcu *rx_sc; // each rx_sc contains unique SCI
> };
> 
> static int macsec_add_rxsc(struct sk_buff *skb, struct genl_info *info)
> {
> ...
>     rx_sc = create_rx_sc(dev, sci);
> ...
> }
> 
> where create_rx_sc() adds new rx_sc node to the secy->rx_sc list.

Right, so instead of putting them on a list put them in a map (IDR?)
and pre-allocate the metadata dst here. Then the driver just does a
lookup. If lookup failed then the SCI is not configured and macsec will
not consume the packet, anyway.

Re: [PATCH net-next v3 2/3] net/macsec: Add MACsec skb extension Rx Data path support

[Saeed Mahameed]

On 13 Jul 11:34, Jakub Kicinski wrote:
>On Wed, 13 Jul 2022 06:21:25 +0000 Lior Nahmanson wrote:
>> For Rx there is no limitation for the number of different SCIs.
>> from MACsec driver code:
>>
>> struct macsec_secy {
>> ...
>>      struct macsec_rx_sc __rcu *rx_sc; // each rx_sc contains unique SCI
>> };
>>
>> static int macsec_add_rxsc(struct sk_buff *skb, struct genl_info *info)
>> {
>> ...
>>     rx_sc = create_rx_sc(dev, sci);
>> ...
>> }
>>
>> where create_rx_sc() adds new rx_sc node to the secy->rx_sc list.
>
>Right, so instead of putting them on a list put them in a map (IDR?)

it has to be rcu protected data structure and with a fast lookup, otherwise
multicore performance will tank, so a rhashtable or xarray, rhashtable might
be more efficient as the key (SCI) range is too big and keys can be too
spread out.

>and pre-allocate the metadata dst here. Then the driver just does a
>lookup. If lookup failed then the SCI is not configured and macsec will
>not consume the packet, anyway.

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

[Lior Nahmanson]

Some MACsec infrastructure like defines and functions,
which may be useful for other drivers who implemented MACsec offload,
aren't accessible since they aren't located in any header file.

Moved those values to MACsec header file will allow those drivers
to use them and avoid code duplication.

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>
---
v1->v2:
- moved MACSEC_PORT_ES from .c to .h
v2->v3:
- removed Issue and Change-Id from commit message
---
 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 7b7baf3dd596..01ff881f4540 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -24,8 +24,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
 
@@ -46,20 +44,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);	\
@@ -230,7 +218,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)
@@ -244,14 +231,6 @@ static struct macsec_cb *macsec_skb_cb(struct sk_buff *skb)
 #define DEFAULT_ENCRYPT false
 #define DEFAULT_ENCODING_SA 0
 
-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;
@@ -316,7 +295,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;
@@ -634,7 +613,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);
 
@@ -1268,7 +1247,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) {
@@ -4007,7 +3986,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;
@@ -4131,7 +4110,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;
 
@@ -4143,7 +4122,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 fcbca963c04d..e727924c5e52 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;
 
@@ -295,5 +309,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.25.4