| Message ID | 20230408105735.22935-6-ehakim@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Support MACsec VLAN | expand |
2023-04-08, 13:57:35 +0300, Emeel Hakim wrote: > Offloading device drivers will mark offloaded MACsec SKBs with the > corresponding SCI in the skb_metadata_dst so the macsec rx handler will > know to which interface to divert those skbs, in case of a marked skb > and a mismatch on the dst MAC address, divert the skb to the macsec > net_device where the macsec rx_handler will be called. Quoting my reply to v2: ======== Sorry, I don't understand what you're trying to say here and in the subject line. To me, "Add MACsec rx_handler change support" sounds like you're changing what function is used as ->rx_handler, which is not what this patch is doing. ======== > Example of such a case is having a MACsec with VLAN as an inner header > ETHERNET | SECTAG | VLAN packet. > > Signed-off-by: Emeel Hakim <ehakim@nvidia.com> > --- > drivers/net/macsec.c | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c > index 25616247d7a5..4e58d2b4f0e1 100644 > --- a/drivers/net/macsec.c > +++ b/drivers/net/macsec.c > @@ -1016,14 +1016,18 @@ static enum rx_handler_result handle_not_macsec(struct sk_buff *skb) > struct sk_buff *nskb; > struct pcpu_secy_stats *secy_stats = this_cpu_ptr(macsec->stats); > struct net_device *ndev = macsec->secy.netdev; > + struct macsec_rx_sc *rx_sc_found = NULL; I don't think "_found" is adding any information. "rx_sc" is enough. And since it's only used in the if block below, it could be defined down there. And btw I don't think we even need to check "&& rx_sc_found" in the code you're adding, but maybe I need more coffee. Anyway, I'd be fine with saving the result of find_rx_sc and reusing it. if (A && !rx_sc) continue; [...] if (A) // here we know rx_sc can't be NULL, otherwise we would have hit the continue earlier packet_host etc > /* 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 (md_dst && md_dst->type == METADATA_MACSEC && > - (!find_rx_sc(&macsec->secy, md_dst->u.macsec_info.sci))) > + rx_sc_found = (md_dst && md_dst->type == METADATA_MACSEC) ? > + find_rx_sc(&macsec->secy, md_dst->u.macsec_info.sci) : NULL; > + > + if (md_dst && md_dst->type == METADATA_MACSEC && !rx_sc_found) { > continue; > + } {} not needed around a single line. > > if (ether_addr_equal_64bits(hdr->h_dest, > ndev->dev_addr)) { > @@ -1048,6 +1052,14 @@ static enum rx_handler_result handle_not_macsec(struct sk_buff *skb) > > __netif_rx(nskb); > } > + > + if (md_dst && md_dst->type == METADATA_MACSEC && rx_sc_found) { > + skb->dev = ndev; > + skb->pkt_type = PACKET_HOST; > + ret = RX_HANDLER_ANOTHER; > + goto out; > + } > + > continue; > } > > -- > 2.21.3 >
> -----Original Message----- > From: Sabrina Dubroca <sd@queasysnail.net> > Sent: Wednesday, 12 April 2023 17:59 > To: Emeel Hakim <ehakim@nvidia.com> > Cc: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com; > edumazet@google.com; netdev@vger.kernel.org; leon@kernel.org > Subject: Re: [PATCH net-next v4 5/5] macsec: Add MACsec rx_handler change > support > > External email: Use caution opening links or attachments > > > 2023-04-08, 13:57:35 +0300, Emeel Hakim wrote: > > Offloading device drivers will mark offloaded MACsec SKBs with the > > corresponding SCI in the skb_metadata_dst so the macsec rx handler > > will know to which interface to divert those skbs, in case of a marked > > skb and a mismatch on the dst MAC address, divert the skb to the > > macsec net_device where the macsec rx_handler will be called. > > Quoting my reply to v2: > > ======== > > Sorry, I don't understand what you're trying to say here and in the subject line. > > To me, "Add MACsec rx_handler change support" sounds like you're changing > what function is used as ->rx_handler, which is not what this patch is doing. > > ======== Sorry that I missed it. what do you think of "Don't rely solely on the dst MAC address for skb diversion upon MACsec rx_handler change" is it good enough? > > Example of such a case is having a MACsec with VLAN as an inner header > > ETHERNET | SECTAG | VLAN packet. > > > > Signed-off-by: Emeel Hakim <ehakim@nvidia.com> > > --- > > drivers/net/macsec.c | 16 ++++++++++++++-- > > 1 file changed, 14 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c index > > 25616247d7a5..4e58d2b4f0e1 100644 > > --- a/drivers/net/macsec.c > > +++ b/drivers/net/macsec.c > > @@ -1016,14 +1016,18 @@ static enum rx_handler_result > handle_not_macsec(struct sk_buff *skb) > > struct sk_buff *nskb; > > struct pcpu_secy_stats *secy_stats = this_cpu_ptr(macsec->stats); > > struct net_device *ndev = macsec->secy.netdev; > > + struct macsec_rx_sc *rx_sc_found = NULL; > > I don't think "_found" is adding any information. "rx_sc" is enough. And since it's > only used in the if block below, it could be defined down there. Agree, will drop the "_found". > And btw I don't think we even need to check "&& rx_sc_found" in the code > you're adding, but maybe I need more coffee. Anyway, I'd be fine with saving the > result of find_rx_sc and reusing it. > > if (A && !rx_sc) > continue; > > [...] > > if (A) // here we know rx_sc can't be NULL, otherwise we would have hit the > continue earlier > packet_host etc Agree, will do that. > > /* 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 (md_dst && md_dst->type == METADATA_MACSEC && > > - (!find_rx_sc(&macsec->secy, md_dst->u.macsec_info.sci))) > > + rx_sc_found = (md_dst && md_dst->type == METADATA_MACSEC) > ? > > + find_rx_sc(&macsec->secy, > > + md_dst->u.macsec_info.sci) : NULL; > > + > > + if (md_dst && md_dst->type == METADATA_MACSEC && > > + !rx_sc_found) { > > continue; > > + } > > {} not needed around a single line. ACK , will remove them. > > > > if (ether_addr_equal_64bits(hdr->h_dest, > > ndev->dev_addr)) { > > @@ -1048,6 +1052,14 @@ static enum rx_handler_result > > handle_not_macsec(struct sk_buff *skb) > > > > __netif_rx(nskb); > > } > > + > > + if (md_dst && md_dst->type == METADATA_MACSEC && > rx_sc_found) { > > + skb->dev = ndev; > > + skb->pkt_type = PACKET_HOST; > > + ret = RX_HANDLER_ANOTHER; > > + goto out; > > + } > > + > > continue; > > } > > > > -- > > 2.21.3 > > > > -- > Sabrina
2023-04-13, 06:38:12 +0000, Emeel Hakim wrote: > > > > -----Original Message----- > > From: Sabrina Dubroca <sd@queasysnail.net> > > Sent: Wednesday, 12 April 2023 17:59 > > To: Emeel Hakim <ehakim@nvidia.com> > > Cc: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com; > > edumazet@google.com; netdev@vger.kernel.org; leon@kernel.org > > Subject: Re: [PATCH net-next v4 5/5] macsec: Add MACsec rx_handler change > > support > > > > External email: Use caution opening links or attachments > > > > > > 2023-04-08, 13:57:35 +0300, Emeel Hakim wrote: > > > Offloading device drivers will mark offloaded MACsec SKBs with the > > > corresponding SCI in the skb_metadata_dst so the macsec rx handler > > > will know to which interface to divert those skbs, in case of a marked > > > skb and a mismatch on the dst MAC address, divert the skb to the > > > macsec net_device where the macsec rx_handler will be called. > > > > Quoting my reply to v2: > > > > ======== > > > > Sorry, I don't understand what you're trying to say here and in the subject line. > > > > To me, "Add MACsec rx_handler change support" sounds like you're changing > > what function is used as ->rx_handler, which is not what this patch is doing. > > > > ======== > > Sorry that I missed it. > what do you think of "Don't rely solely on the dst MAC address for skb diversion upon MACsec rx_handler change" > is it good enough? But there's no "change of rx_handler". You're just receiving the packet on the macsec device. I don't understand what you're trying to say with "change of rx_handler", but to me that's not describing this patch at all. "change of rx_handler" would describe a patch that modifies dev->rx_handler. "Don't rely solely on the dst MAC address to identify destination MACsec device" looks ok, and should be followed by an explanation: - why the dst MAC address may not be enough - why it's not needed when we have metadata > > > @@ -1048,6 +1052,14 @@ static enum rx_handler_result > > > handle_not_macsec(struct sk_buff *skb) > > > > > > __netif_rx(nskb); > > > } > > > + > > > + if (md_dst && md_dst->type == METADATA_MACSEC && > > rx_sc_found) { BTW, why did you choose to separate that from the previous if/else if? > > > + skb->dev = ndev; > > > + skb->pkt_type = PACKET_HOST; > > > + ret = RX_HANDLER_ANOTHER; > > > + goto out; > > > + } > > > + > > > continue; > > > }
> -----Original Message----- > From: Sabrina Dubroca <sd@queasysnail.net> > Sent: Thursday, 13 April 2023 11:36 > To: Emeel Hakim <ehakim@nvidia.com> > Cc: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com; > edumazet@google.com; netdev@vger.kernel.org; leon@kernel.org > Subject: Re: [PATCH net-next v4 5/5] macsec: Add MACsec rx_handler change > support > > External email: Use caution opening links or attachments > > > 2023-04-13, 06:38:12 +0000, Emeel Hakim wrote: > > > > > > > -----Original Message----- > > > From: Sabrina Dubroca <sd@queasysnail.net> > > > Sent: Wednesday, 12 April 2023 17:59 > > > To: Emeel Hakim <ehakim@nvidia.com> > > > Cc: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com; > > > edumazet@google.com; netdev@vger.kernel.org; leon@kernel.org > > > Subject: Re: [PATCH net-next v4 5/5] macsec: Add MACsec rx_handler > > > change support > > > > > > External email: Use caution opening links or attachments > > > > > > > > > 2023-04-08, 13:57:35 +0300, Emeel Hakim wrote: > > > > Offloading device drivers will mark offloaded MACsec SKBs with the > > > > corresponding SCI in the skb_metadata_dst so the macsec rx handler > > > > will know to which interface to divert those skbs, in case of a > > > > marked skb and a mismatch on the dst MAC address, divert the skb > > > > to the macsec net_device where the macsec rx_handler will be called. > > > > > > Quoting my reply to v2: > > > > > > ======== > > > > > > Sorry, I don't understand what you're trying to say here and in the subject > line. > > > > > > To me, "Add MACsec rx_handler change support" sounds like you're > > > changing what function is used as ->rx_handler, which is not what this patch is > doing. > > > > > > ======== > > > > Sorry that I missed it. > > what do you think of "Don't rely solely on the dst MAC address for skb diversion > upon MACsec rx_handler change" > > is it good enough? > > But there's no "change of rx_handler". You're just receiving the packet on the > macsec device. I don't understand what you're trying to say with "change of > rx_handler", but to me that's not describing this patch at all. "change of > rx_handler" would describe a patch that modifies dev->rx_handler. > > "Don't rely solely on the dst MAC address to identify destination MACsec device" > looks ok, and should be followed by an explanation: > - why the dst MAC address may not be enough > - why it's not needed when we have metadata ACK , I will fix it and send a new version. > > > > @@ -1048,6 +1052,14 @@ static enum rx_handler_result > > > > handle_not_macsec(struct sk_buff *skb) > > > > > > > > __netif_rx(nskb); > > > > } > > > > + > > > > + if (md_dst && md_dst->type == > > > > + METADATA_MACSEC && > > > rx_sc_found) { > > BTW, why did you choose to separate that from the previous if/else if? I felt that the if/else if is handling the "relying on dst mac" case so I separated it, but I see no harm in adding it as an else if. Will address this both with the commit message and send a new version. Thanks Sabrina. > > > > + skb->dev = ndev; > > > > + skb->pkt_type = PACKET_HOST; > > > > + ret = RX_HANDLER_ANOTHER; > > > > + goto out; > > > > + } > > > > + > > > > continue; > > > > } > > -- > Sabrina
diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c index 25616247d7a5..4e58d2b4f0e1 100644 --- a/drivers/net/macsec.c +++ b/drivers/net/macsec.c @@ -1016,14 +1016,18 @@ static enum rx_handler_result handle_not_macsec(struct sk_buff *skb) struct sk_buff *nskb; struct pcpu_secy_stats *secy_stats = this_cpu_ptr(macsec->stats); struct net_device *ndev = macsec->secy.netdev; + struct macsec_rx_sc *rx_sc_found = NULL; /* 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 (md_dst && md_dst->type == METADATA_MACSEC && - (!find_rx_sc(&macsec->secy, md_dst->u.macsec_info.sci))) + rx_sc_found = (md_dst && md_dst->type == METADATA_MACSEC) ? + find_rx_sc(&macsec->secy, md_dst->u.macsec_info.sci) : NULL; + + if (md_dst && md_dst->type == METADATA_MACSEC && !rx_sc_found) { continue; + } if (ether_addr_equal_64bits(hdr->h_dest, ndev->dev_addr)) { @@ -1048,6 +1052,14 @@ static enum rx_handler_result handle_not_macsec(struct sk_buff *skb) __netif_rx(nskb); } + + if (md_dst && md_dst->type == METADATA_MACSEC && rx_sc_found) { + skb->dev = ndev; + skb->pkt_type = PACKET_HOST; + ret = RX_HANDLER_ANOTHER; + goto out; + } + continue; }
Offloading device drivers will mark offloaded MACsec SKBs with the corresponding SCI in the skb_metadata_dst so the macsec rx handler will know to which interface to divert those skbs, in case of a marked skb and a mismatch on the dst MAC address, divert the skb to the macsec net_device where the macsec rx_handler will be called. Example of such a case is having a MACsec with VLAN as an inner header ETHERNET | SECTAG | VLAN packet. Signed-off-by: Emeel Hakim <ehakim@nvidia.com> --- drivers/net/macsec.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)