| Message ID | 20231116182900.46052-3-rrameshbabu@nvidia.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Take advantage of certain device drivers during MACsec offload | expand |
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?
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
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.
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,
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(-)