[PATCH net] macsec: Abort MACSec Rx offload datapath when skb is not marked with MACSec metadata

[PATCH net] macsec: Abort MACSec Rx offload datapath when skb is not marked with MACSec metadata

[Rahul Rameshbabu]

When MACSec is configured on an outer netdev, traffic received directly
through the underlying netdev should not be processed by the MACSec Rx
datapath. When using MACSec offload on an outer netdev, traffic with no
metadata indicator in the skb is mistakenly considered as MACSec traffic
and incorrectly handled in the handle_not_macsec function. Treat skbs with
no metadata type as non-MACSec packets rather than assuming they are MACSec
packets.

Fixes: 860ead89b851 ("net/macsec: Add MACsec skb_metadata_dst Rx Data path support")
Cc: Sabrina Dubroca <sd@queasysnail.net>
Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
Reviewed-by: Saeed Mahameed <saeed@kernel.org>
---
 drivers/net/macsec.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 9663050a852d..102200ce87d3 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -996,10 +996,12 @@ static enum rx_handler_result handle_not_macsec(struct sk_buff *skb)
 	struct metadata_dst *md_dst;
 	struct macsec_rxh_data *rxd;
 	struct macsec_dev *macsec;
+	bool is_macsec_md_skb;
 
 	rcu_read_lock();
 	rxd = macsec_data_rcu(skb->dev);
 	md_dst = skb_metadata_dst(skb);
+	is_macsec_md_skb = !md_dst || md_dst->type != METADATA_MACSEC;
 
 	list_for_each_entry_rcu(macsec, &rxd->secys, secys) {
 		struct sk_buff *nskb;
@@ -1012,10 +1014,11 @@ 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)
-				rx_sc = find_rx_sc(&macsec->secy, md_dst->u.macsec_info.sci);
+			if (is_macsec_md_skb)
+				continue;
 
-			if (md_dst && md_dst->type == METADATA_MACSEC && !rx_sc)
+			rx_sc = find_rx_sc(&macsec->secy, md_dst->u.macsec_info.sci);
+			if (!rx_sc)
 				continue;
 
 			if (ether_addr_equal_64bits(hdr->h_dest,
-- 
2.40.1

Re: [PATCH net] macsec: Abort MACSec Rx offload datapath when skb is not marked with MACSec metadata

[Sabrina Dubroca]

2023-11-01, 13:02:17 -0700, Rahul Rameshbabu wrote:
> When MACSec is configured on an outer netdev, traffic received directly
> through the underlying netdev should not be processed by the MACSec Rx
> datapath. When using MACSec offload on an outer netdev, traffic with no
> metadata indicator in the skb is mistakenly considered as MACSec traffic
> and incorrectly handled in the handle_not_macsec function. Treat skbs with
> no metadata type as non-MACSec packets rather than assuming they are MACSec
> packets.

What about the other drivers? mlx5 is the only driver that sets md_dst
on its macsec skbs, so their offloaded packets just get dropped now?

-- 
Sabrina

Re: [PATCH net] macsec: Abort MACSec Rx offload datapath when skb is not marked with MACSec metadata

[Rahul Rameshbabu]

On Wed, 01 Nov, 2023 23:31:33 +0100 Sabrina Dubroca <sd@queasysnail.net> wrote:
> 2023-11-01, 13:02:17 -0700, Rahul Rameshbabu wrote:
>> When MACSec is configured on an outer netdev, traffic received directly
>> through the underlying netdev should not be processed by the MACSec Rx
>> datapath. When using MACSec offload on an outer netdev, traffic with no
>> metadata indicator in the skb is mistakenly considered as MACSec traffic
>> and incorrectly handled in the handle_not_macsec function. Treat skbs with
>> no metadata type as non-MACSec packets rather than assuming they are MACSec
>> packets.
>
> What about the other drivers? mlx5 is the only driver that sets md_dst
> on its macsec skbs, so their offloaded packets just get dropped now?

After taking a deeper look throughout the tree, I realize there are
macsec offloading drivers that do not set md_dst. In this event, we fail
to correctly handle the skb and deliver to the port as you mentioned
previously. Sorry about this miss on my end.

However, I believe that all macsec offload supporting devices run into
the following problem today (including mlx5 devices).

When I configure macsec offload on a device and then vlan on top of the
macsec interface, I become unable to send traffic through the underlying
device.

(can replace mlx5_1 with any ifname of a device that supports macsec offload)

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

I am thinking the solution is a combination of annotating which macsec
devices support md_dst and this patch. However, I am not sure this fix
would be helpful for devices that support macsec offload without
utilizing md_dst information (would still be problematic).

Would be glad to hear any suggestions you may have before sending out a
v2.

Sorry for the late reply. Was away from a trip and wanted to validate
some things before posting back.

--
Thanks,

Rahul Rameshbabu

Re: [PATCH net] macsec: Abort MACSec Rx offload datapath when skb is not marked with MACSec metadata

[Sabrina Dubroca]

2023-11-06, 14:15:11 -0800, Rahul Rameshbabu wrote:
> However, I believe that all macsec offload supporting devices run into
> the following problem today (including mlx5 devices).

If that's the case, we have to fix all of them.

> When I configure macsec offload on a device and then vlan on top of the
> macsec interface, I become unable to send traffic through the underlying
> device.
[...]
>   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

Which packet gets dropped and why? Where? I don't understand how the
vlan makes a difference in a packet targeting the lower device.

> I am thinking the solution is a combination of annotating which macsec
> devices support md_dst and this patch.

Yes, if we know that the offloading device sets md_dst on all its
offloaded packets, we can just look up the rx_sc based on the sci and
be done, or pass the packet directly to the real device if md_dst
wasn't provided. No need to go through the MAC address matching at
all.

> However, I am not sure this fix
> would be helpful for devices that support macsec offload without
> utilizing md_dst information (would still be problematic).

Yeah, anything relying on md_dst is not going to help the rest of the
drivers.

-- 
Sabrina

Re: [PATCH net] macsec: Abort MACSec Rx offload datapath when skb is not marked with MACSec metadata

[Rahul Rameshbabu]

On Wed, 15 Nov, 2023 16:19:27 +0100 Sabrina Dubroca <sd@queasysnail.net> wrote:
> 2023-11-06, 14:15:11 -0800, Rahul Rameshbabu wrote:
>> However, I believe that all macsec offload supporting devices run into
>> the following problem today (including mlx5 devices).
>
> If that's the case, we have to fix all of them.

I have an RFC patch series undergoing internal review for supporting
devices that advertise md_dst for handling non-MACsec multicast traffic
sent to the port. In the cover letter, I describe why I do not believe
the same case can be trivially handled for devices that do not support
setting md_dst since there is no way of knowing whether the traffic
received on the port was MACsec or not.

>
>> When I configure macsec offload on a device and then vlan on top of the
>> macsec interface, I become unable to send traffic through the underlying
>> device.
> [...]
>>   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
>
> Which packet gets dropped and why? Where? I don't understand how the
> vlan makes a difference in a packet targeting the lower device.
>
>> I am thinking the solution is a combination of annotating which macsec
>> devices support md_dst and this patch.
>
> Yes, if we know that the offloading device sets md_dst on all its
> offloaded packets, we can just look up the rx_sc based on the sci and
> be done, or pass the packet directly to the real device if md_dst
> wasn't provided. No need to go through the MAC address matching at
> all.
>

This is exactly what the RFC patch series I am proposing looks like.

>> However, I am not sure this fix
>> would be helpful for devices that support macsec offload without
>> utilizing md_dst information (would still be problematic).
>
> Yeah, anything relying on md_dst is not going to help the rest of the
> drivers.

Right, I have an example in the cover letter that elaborates on why I do
not think it's possible to support this on devices that do not set
md_dst. I think the existing handling for multicast messages and
promiscuous mode in handle_not_macsec is the most appropriate for
drivers unable to set md_dst to indicate whether traffic received on the
port is MACsec traffic that will be handled by the NIC's offloading
functionality. Let me try to get that RFC out on the list soon for your
review.

--
Thanks,

Rahul Rameshbabu