netdev - RE: [PATCH net-next v4 5/5] macsec: Add MACsec rx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <IA1PR12MB6353A4C01FE89E6256C89E94AB989@IA1PR12MB6353.namprd12.prod.outlook.com>
Date:   Thu, 13 Apr 2023 06:38:12 +0000
From:   Emeel Hakim <ehakim@...dia.com>
To:     Sabrina Dubroca <sd@...asysnail.net>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "leon@...nel.org" <leon@...nel.org>
Subject: RE: [PATCH net-next v4 5/5] macsec: Add MACsec rx_handler change
 support



> -----Original Message-----
> From: Sabrina Dubroca <sd@...asysnail.net>
> Sent: Wednesday, 12 April 2023 17:59
> To: Emeel Hakim <ehakim@...dia.com>
> Cc: davem@...emloft.net; kuba@...nel.org; pabeni@...hat.com;
> edumazet@...gle.com; netdev@...r.kernel.org; leon@...nel.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@...dia.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