[PATCH RFC net-next v1 0/3] Take advantage of certain device drivers during MACsec offload

[PATCH RFC net-next v1 0/3] Take advantage of certain device drivers during MACsec offload

[Rahul Rameshbabu]

Some device drivers support devices that enable them to annotate whether a
Rx skb refers to a packet that was processed by the MACsec offloading
functionality of the device. Logic in the Rx handling for MACsec offload
does not utilize this information to preemptively avoid forwarding to the
macsec netdev currently. Because of this, things like multicast messages
such as ARP requests are forwarded to the macsec netdev whether the message
received was MACsec encrypted or not. The goal of this patch series is to
improve the Rx handling for MACsec offload for devices capable of
annotating skbs received that were decrypted by the NIC offload for MACsec.

Shown below is an example use case where plaintext traffic sent to a
physical port of a NIC configured for MACsec offload is unable to be
handled correctly by the software stack when the NIC provides awareness to
the kernel about whether the received packet is MACsec traffic or not. In
this specific example, plaintext ARP requests are being responded with
MACsec encrypted ARP replies (which leads to routing information being
unable to be built for the requester).

    Side 1

        ip link del macsec0
        ip address flush mlx5_1
        ip address add 1.1.1.1/24 dev mlx5_1
        ip link set dev mlx5_1 up
        ip link add link mlx5_1 macsec0 type macsec sci 1 encrypt on
        ip link set dev macsec0 address 00:11:22:33:44:66
        ip macsec offload macsec0 mac
        ip macsec add macsec0 tx sa 0 pn 1 on key 00 dffafc8d7b9a43d5b9a3dfbbf6a30c16
        ip macsec add macsec0 rx sci 2 on
        ip macsec add macsec0 rx sci 2 sa 0 pn 1 on key 00 ead3664f508eb06c40ac7104cdae4ce5
        ip address flush macsec0
        ip address add 2.2.2.1/24 dev macsec0
        ip link set dev macsec0 up
        ip link add link macsec0 name macsec_vlan type vlan id 1
        ip link set dev macsec_vlan address 00:11:22:33:44:88
        ip address flush macsec_vlan
        ip address add 3.3.3.1/24 dev macsec_vlan
        ip link set dev macsec_vlan up

    Side 2

        ip link del macsec0
        ip address flush mlx5_1
        ip address add 1.1.1.2/24 dev mlx5_1
        ip link set dev mlx5_1 up
        ip link add link mlx5_1 macsec0 type macsec sci 2 encrypt on
        ip link set dev macsec0 address 00:11:22:33:44:77
        ip macsec offload macsec0 mac
        ip macsec add macsec0 tx sa 0 pn 1 on key 00 ead3664f508eb06c40ac7104cdae4ce5
        ip macsec add macsec0 rx sci 1 on
        ip macsec add macsec0 rx sci 1 sa 0 pn 1 on key 00 dffafc8d7b9a43d5b9a3dfbbf6a30c16
        ip address flush macsec0
        ip address add 2.2.2.2/24 dev macsec0
        ip link set dev macsec0 up
        ip link add link macsec0 name macsec_vlan type vlan id 1
        ip link set dev macsec_vlan address 00:11:22:33:44:99
        ip address flush macsec_vlan
        ip address add 3.3.3.2/24 dev macsec_vlan
        ip link set dev macsec_vlan up

    Side 1

        ping -I mlx5_1 1.1.1.2
        PING 1.1.1.2 (1.1.1.2) from 1.1.1.1 mlx5_1: 56(84) bytes of data.
        From 1.1.1.1 icmp_seq=1 Destination Host Unreachable
        ping: sendmsg: No route to host
        From 1.1.1.1 icmp_seq=2 Destination Host Unreachable
        From 1.1.1.1 icmp_seq=3 Destination Host Unreachable

Link: https://lore.kernel.org/netdev/87r0l25y1c.fsf@nvidia.com/
Cc: Sabrina Dubroca <sd@queasysnail.net>
Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>

Rahul Rameshbabu (3):
  macsec: Enable devices to advertise whether they update sk_buff md_dst
    during offloads
  macsec: Detect if Rx skb is macsec-related for offloading devices that
    update md_dst
  net/mlx5e: Advertise mlx5 ethernet driver updates sk_buff md_dst for
    MACsec

 .../net/ethernet/mellanox/mlx5/core/en_accel/macsec.c |  8 ++++++++
 drivers/net/macsec.c                                  | 11 +++++++++--
 include/net/macsec.h                                  |  1 +
 3 files changed, 18 insertions(+), 2 deletions(-)

-- 
2.40.1

[PATCH RFC net-next v1 1/3] macsec: Enable devices to advertise whether they update sk_buff md_dst during offloads

[Rahul Rameshbabu]

Cannot know whether a Rx skb missing md_dst is intended for MACsec or not
without knowing whether the device is able to update this field during an
offload. Assume that an offload to a MACsec device cannot support updating
md_dst by default. Capable devices can advertise that they do indicate that
an skb is related to a MACsec offloaded packet using the md_dst.

Cc: Sabrina Dubroca <sd@queasysnail.net>
Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
---
 drivers/net/macsec.c | 3 +++
 include/net/macsec.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 9663050a852d..8c0b12490e89 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -93,6 +93,7 @@ struct pcpu_secy_stats {
  * @secys: linked list of SecY's on the underlying device
  * @gro_cells: pointer to the Generic Receive Offload cell
  * @offload: status of offloading on the MACsec device
+ * @offload_md_dst: whether MACsec device offload supports sk_buff md_dst
  */
 struct macsec_dev {
 	struct macsec_secy secy;
@@ -102,6 +103,7 @@ struct macsec_dev {
 	struct list_head secys;
 	struct gro_cells gro_cells;
 	enum macsec_offload offload;
+	bool offload_md_dst;
 };
 
 /**
@@ -3525,6 +3527,7 @@ static int macsec_dev_open(struct net_device *dev)
 		}
 
 		ctx.secy = &macsec->secy;
+		ctx.offload_md_dst = &macsec->offload_md_dst;
 		err = macsec_offload(ops->mdo_dev_open, &ctx);
 		if (err)
 			goto clear_allmulti;
diff --git a/include/net/macsec.h b/include/net/macsec.h
index ebf9bc54036a..09ca118d2df6 100644
--- a/include/net/macsec.h
+++ b/include/net/macsec.h
@@ -255,6 +255,7 @@ struct macsec_context {
 	};
 	enum macsec_offload offload;
 
+	bool *offload_md_dst;
 	struct macsec_secy *secy;
 	struct macsec_rx_sc *rx_sc;
 	struct {
-- 
2.40.1

[PATCH RFC net-next v1 2/3] macsec: Detect if Rx skb is macsec-related for offloading devices that update md_dst

[Rahul Rameshbabu]

This detection capability will enable drivers that update md_dst to be able
to receive and handle both non-MACSec and MACsec traffic received and the
same physical port when offload is enabled.

This detection is not possible without device drivers that update md_dst. A
fallback pattern should be used for supporting such device drivers. This
fallback mode causes multicast messages to be cloned to both the non-macsec
and macsec ports, independent of whether the multicast message received was
encrypted over MACsec or not. Other non-macsec traffic may also fail to be
handled correctly for devices in promiscuous mode.

Link: https://lore.kernel.org/netdev/ZULRxX9eIbFiVi7v@hog/
Cc: Sabrina Dubroca <sd@queasysnail.net>
Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
---
 drivers/net/macsec.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 8c0b12490e89..e14f2ad2e253 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -1002,6 +1002,7 @@ static enum rx_handler_result handle_not_macsec(struct sk_buff *skb)
 	rcu_read_lock();
 	rxd = macsec_data_rcu(skb->dev);
 	md_dst = skb_metadata_dst(skb);
+	bool is_macsec_md_dst = md_dst && md_dst->type == METADATA_MACSEC;
 
 	list_for_each_entry_rcu(macsec, &rxd->secys, secys) {
 		struct sk_buff *nskb;
@@ -1014,10 +1015,13 @@ static enum rx_handler_result handle_not_macsec(struct sk_buff *skb)
 		if (macsec_is_offloaded(macsec) && netif_running(ndev)) {
 			struct macsec_rx_sc *rx_sc = NULL;
 
-			if (md_dst && md_dst->type == METADATA_MACSEC)
+			if (macsec->offload_md_dst && !is_macsec_md_dst)
+				continue;
+
+			if (is_macsec_md_dst)
 				rx_sc = find_rx_sc(&macsec->secy, md_dst->u.macsec_info.sci);
 
-			if (md_dst && md_dst->type == METADATA_MACSEC && !rx_sc)
+			if (is_macsec_md_dst && !rx_sc)
 				continue;
 
 			if (ether_addr_equal_64bits(hdr->h_dest,
-- 
2.40.1

[PATCH RFC net-next v1 3/3] net/mlx5e: Advertise mlx5 ethernet driver updates sk_buff md_dst for MACsec

[Rahul Rameshbabu]

mlx5 Rx flow steering and CQE handling enable the driver to be able to
update an skb's md_dst attribute as MACsec when MACsec traffic arrives when
a device is configured for offloading. Advertise this to the core stack to
take advantage of this capability.

Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c | 8 ++++++++
 1 file changed, 8 insertions(+)

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 d4ebd8743114..0e5efe0d2c92 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
@@ -475,6 +475,13 @@ static void update_macsec_epn(struct mlx5e_macsec_sa *sa, const struct macsec_ke
 	epn_state->overlap = next_pn_halves->lower < MLX5_MACSEC_EPN_SCOPE_MID ? 0 : 1;
 }
 
+static int mlx5e_macsec_dev_open(struct macsec_context *ctx)
+{
+	*ctx->offload_md_dst = true;
+
+	return 0;
+}
+
 static int mlx5e_macsec_add_txsa(struct macsec_context *ctx)
 {
 	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
@@ -1608,6 +1615,7 @@ static void mlx5e_macsec_aso_cleanup(struct mlx5e_macsec_aso *aso, struct mlx5_c
 }
 
 static const struct macsec_ops macsec_offload_ops = {
+	.mdo_dev_open = mlx5e_macsec_dev_open,
 	.mdo_add_txsa = mlx5e_macsec_add_txsa,
 	.mdo_upd_txsa = mlx5e_macsec_upd_txsa,
 	.mdo_del_txsa = mlx5e_macsec_del_txsa,
-- 
2.40.1

Re: [PATCH RFC net-next v1 2/3] macsec: Detect if Rx skb is macsec-related for offloading devices that update md_dst

[Sabrina Dubroca]

2023-11-16, 10:28:59 -0800, Rahul Rameshbabu wrote:
> This detection capability will enable drivers that update md_dst to be able
> to receive and handle both non-MACSec and MACsec traffic received and the
> same physical port when offload is enabled.
> 
> This detection is not possible without device drivers that update md_dst. A
> fallback pattern should be used for supporting such device drivers. This
> fallback mode causes multicast messages to be cloned to both the non-macsec
> and macsec ports, independent of whether the multicast message received was
> encrypted over MACsec or not. Other non-macsec traffic may also fail to be
> handled correctly for devices in promiscuous mode.
> 
> Link: https://lore.kernel.org/netdev/ZULRxX9eIbFiVi7v@hog/
> Cc: Sabrina Dubroca <sd@queasysnail.net>
> Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
> ---
>  drivers/net/macsec.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
> index 8c0b12490e89..e14f2ad2e253 100644
> --- a/drivers/net/macsec.c
> +++ b/drivers/net/macsec.c
> @@ -1002,6 +1002,7 @@ static enum rx_handler_result handle_not_macsec(struct sk_buff *skb)
>  	rcu_read_lock();
>  	rxd = macsec_data_rcu(skb->dev);
>  	md_dst = skb_metadata_dst(skb);
> +	bool is_macsec_md_dst = md_dst && md_dst->type == METADATA_MACSEC;
>  
>  	list_for_each_entry_rcu(macsec, &rxd->secys, secys) {
>  		struct sk_buff *nskb;
> @@ -1014,10 +1015,13 @@ static enum rx_handler_result handle_not_macsec(struct sk_buff *skb)
>  		if (macsec_is_offloaded(macsec) && netif_running(ndev)) {
>  			struct macsec_rx_sc *rx_sc = NULL;
>  
> -			if (md_dst && md_dst->type == METADATA_MACSEC)
> +			if (macsec->offload_md_dst && !is_macsec_md_dst)
> +				continue;
> +
> +			if (is_macsec_md_dst)
>  				rx_sc = find_rx_sc(&macsec->secy, md_dst->u.macsec_info.sci);
>  
> -			if (md_dst && md_dst->type == METADATA_MACSEC && !rx_sc)
> +			if (is_macsec_md_dst && !rx_sc)
>  				continue;
>  
>  			if (ether_addr_equal_64bits(hdr->h_dest,

Why not skip the MAC address matching if you found the rx_sc? The way
you're implementing it, it will still distribute broadcast received
over the macsec port to other macsec ports on the same device, right?

If the device provided md_dst, either we find the corresponding rx_sc,
then we receive on this macsec device only, or we don't and try the
other macsec devices.

Something like this (completely untested):

	if (macsec_is_offloaded(macsec) && netif_running(ndev)) {
		struct macsec_rx_sc *rx_sc = NULL;
		bool exact = false;

		if (macsec->offload_md_dst && !is_macsec_md_dst)
			continue;

		if (is_macsec_md_dst) {
			DEBUG_NET_WARN_ON_ONCE(!macsec->offload_md_dst);
			rx_sc = find_rx_sc(&macsec->secy, md_dst->u.macsec_info.sci);
			if (!rx_sc)
				continue;
			exact = true;
		}

		if (exact ||
		    ether_addr_equal_64bits(hdr->h_dest, ndev->dev_addr)) {
			/* exact match, divert skb to this port */
	[keep the existing code after this]


Am I missing something?

-- 
Sabrina

Re: [PATCH RFC net-next v1 2/3] macsec: Detect if Rx skb is macsec-related for offloading devices that update md_dst

[Rahul Rameshbabu]

On Thu, 23 Nov, 2023 15:38:04 +0100 Sabrina Dubroca <sd@queasysnail.net> wrote:
> 2023-11-16, 10:28:59 -0800, Rahul Rameshbabu wrote:
>> This detection capability will enable drivers that update md_dst to be able
>> to receive and handle both non-MACSec and MACsec traffic received and the
>> same physical port when offload is enabled.
>> 
>> This detection is not possible without device drivers that update md_dst. A
>> fallback pattern should be used for supporting such device drivers. This
>> fallback mode causes multicast messages to be cloned to both the non-macsec
>> and macsec ports, independent of whether the multicast message received was
>> encrypted over MACsec or not. Other non-macsec traffic may also fail to be
>> handled correctly for devices in promiscuous mode.
>> 
>> Link: https://lore.kernel.org/netdev/ZULRxX9eIbFiVi7v@hog/
>> Cc: Sabrina Dubroca <sd@queasysnail.net>
>> Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
>> ---
>>  drivers/net/macsec.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
>> index 8c0b12490e89..e14f2ad2e253 100644
>> --- a/drivers/net/macsec.c
>> +++ b/drivers/net/macsec.c
>> @@ -1002,6 +1002,7 @@ static enum rx_handler_result handle_not_macsec(struct sk_buff *skb)
>>  	rcu_read_lock();
>>  	rxd = macsec_data_rcu(skb->dev);
>>  	md_dst = skb_metadata_dst(skb);
>> +	bool is_macsec_md_dst = md_dst && md_dst->type == METADATA_MACSEC;
>>  
>>  	list_for_each_entry_rcu(macsec, &rxd->secys, secys) {
>>  		struct sk_buff *nskb;
>> @@ -1014,10 +1015,13 @@ static enum rx_handler_result handle_not_macsec(struct sk_buff *skb)
>>  		if (macsec_is_offloaded(macsec) && netif_running(ndev)) {
>>  			struct macsec_rx_sc *rx_sc = NULL;
>>  
>> -			if (md_dst && md_dst->type == METADATA_MACSEC)
>> +			if (macsec->offload_md_dst && !is_macsec_md_dst)
>> +				continue;
>> +
>> +			if (is_macsec_md_dst)
>>  				rx_sc = find_rx_sc(&macsec->secy, md_dst->u.macsec_info.sci);
>>  
>> -			if (md_dst && md_dst->type == METADATA_MACSEC && !rx_sc)
>> +			if (is_macsec_md_dst && !rx_sc)
>>  				continue;
>>  
>>  			if (ether_addr_equal_64bits(hdr->h_dest,
>
> Why not skip the MAC address matching if you found the rx_sc? The way
> you're implementing it, it will still distribute broadcast received
> over the macsec port to other macsec ports on the same device, right?

That's true. Once the rx_sc is found, the skb can be diverted to the
macsec port.

>
> If the device provided md_dst, either we find the corresponding rx_sc,
> then we receive on this macsec device only, or we don't and try the
> other macsec devices.
>
> Something like this (completely untested):
>
> 	if (macsec_is_offloaded(macsec) && netif_running(ndev)) {
> 		struct macsec_rx_sc *rx_sc = NULL;
> 		bool exact = false;
>
> 		if (macsec->offload_md_dst && !is_macsec_md_dst)
> 			continue;
>
> 		if (is_macsec_md_dst) {
> 			DEBUG_NET_WARN_ON_ONCE(!macsec->offload_md_dst);
> 			rx_sc = find_rx_sc(&macsec->secy, md_dst->u.macsec_info.sci);
> 			if (!rx_sc)
> 				continue;
> 			exact = true;
> 		}
>
> 		if (exact ||
> 		    ether_addr_equal_64bits(hdr->h_dest, ndev->dev_addr)) {
> 			/* exact match, divert skb to this port */
> 	[keep the existing code after this]
>
>
> Am I missing something?

I just have one question with regards to this (will be testing this out
too). For the exact match case, if the receiving traffic was macsec
encrypted multicast, would the pkt_type be PACKET_HOST or
PACKET_BROADCAST/PACKET_MULTICAST? My intuition is screaming to me that
'[keep the existing code after this]' is not 100% true because we would
want to update the skb pkt_type to PACKET_BROADCAST/PACKET_MULTICAST
even if we are able to identify the incoming multicast frame was macsec
encrypted and specifically intended for this device. Does that sound
right?

--
Thanks,

Rahul Rameshbabu

Re: [PATCH RFC net-next v1 2/3] macsec: Detect if Rx skb is macsec-related for offloading devices that update md_dst

[Sabrina Dubroca]

2023-11-27, 11:10:19 -0800, Rahul Rameshbabu wrote:
> On Thu, 23 Nov, 2023 15:38:04 +0100 Sabrina Dubroca <sd@queasysnail.net> wrote:
> > If the device provided md_dst, either we find the corresponding rx_sc,
> > then we receive on this macsec device only, or we don't and try the
> > other macsec devices.
> >
> > Something like this (completely untested):
> >
> > 	if (macsec_is_offloaded(macsec) && netif_running(ndev)) {
> > 		struct macsec_rx_sc *rx_sc = NULL;
> > 		bool exact = false;
> >
> > 		if (macsec->offload_md_dst && !is_macsec_md_dst)
> > 			continue;
> >
> > 		if (is_macsec_md_dst) {
> > 			DEBUG_NET_WARN_ON_ONCE(!macsec->offload_md_dst);
> > 			rx_sc = find_rx_sc(&macsec->secy, md_dst->u.macsec_info.sci);
> > 			if (!rx_sc)
> > 				continue;
> > 			exact = true;
> > 		}
> >
> > 		if (exact ||
> > 		    ether_addr_equal_64bits(hdr->h_dest, ndev->dev_addr)) {
> > 			/* exact match, divert skb to this port */
> > 	[keep the existing code after this]
> >
> >
> > Am I missing something?
> 
> I just have one question with regards to this (will be testing this out
> too). For the exact match case, if the receiving traffic was macsec
> encrypted multicast, would the pkt_type be PACKET_HOST or
> PACKET_BROADCAST/PACKET_MULTICAST? My intuition is screaming to me that
> '[keep the existing code after this]' is not 100% true because we would
> want to update the skb pkt_type to PACKET_BROADCAST/PACKET_MULTICAST
> even if we are able to identify the incoming multicast frame was macsec
> encrypted and specifically intended for this device. Does that sound
> right?

Yes, I guess. SW decrypt path calls eth_type_trans, but that does a
lot more than we need here.

-- 
Sabrina