netdev - RE: [PATCH net-next v1 01/03] net/macsec: Add MACsec skb extension Tx Data path support

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] [day] [month] [year] [list]

Message-ID: <PH0PR12MB5449D3A9884E92733EF70E0ABFD09@PH0PR12MB5449.namprd12.prod.outlook.com>
Date:   Thu, 19 May 2022 06:26:55 +0000
From:   Lior Nahmanson <liorna@...dia.com>
To:     Paolo Abeni <pabeni@...hat.com>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Raed Salem <raeds@...dia.com>, Jiri Pirko <jiri@...dia.com>,
        Ben Ben Ishay <benishay@...dia.com>
Subject: RE: [PATCH net-next v1 01/03] net/macsec: Add MACsec skb extension Tx
 Data path support

> -----Original Message-----
> From: Paolo Abeni <pabeni@...hat.com>
> Sent: Tuesday, May 10, 2022 12:23 PM
> To: Lior Nahmanson <liorna@...dia.com>; edumazet@...gle.com;
> kuba@...nel.org
> Cc: davem@...emloft.net; netdev@...r.kernel.org; Raed Salem
> <raeds@...dia.com>; Jiri Pirko <jiri@...dia.com>; Ben Ben Ishay
> <benishay@...dia.com>
> Subject: Re: [PATCH net-next v1 01/03] net/macsec: Add MACsec skb
> extension Tx Data path support
> 
> External email: Use caution opening links or attachments
> 
> 
> On Sun, 2022-05-08 at 12:09 +0300, Lior Nahmanson wrote:
> > In the current MACsec offload implementation, MACsec interfaces are
> > sharing the same MAC address of their parent interface by default.
> > Therefore, HW can't distinguish if a packet was sent from MACsec
> > interface and need to be offloaded or not.
> > Also, it can't distinguish from which MACsec interface it was sent in
> > case there are multiple MACsec interface with the same MAC address.
> >
> > Used SKB extension, so SW can mark if a packet is needed to be
> > offloaded and use the SCI, which is unique value for each MACsec
> > interface, to notify the HW from which MACsec interface the packet is
> sent.
> >
> > Signed-off-by: Lior Nahmanson <liorna@...dia.com>
> > Reviewed-by: Raed Salem <raeds@...dia.com>
> > Reviewed-by: Jiri Pirko <jiri@...dia.com>
> > Reviewed-by: Ben Ben-Ishay <benishay@...dia.com>
> > ---
> >  drivers/net/Kconfig    | 1 +
> >  drivers/net/macsec.c   | 5 +++++
> >  include/linux/skbuff.h | 3 +++
> >  include/net/macsec.h   | 6 ++++++
> >  net/core/skbuff.c      | 7 +++++++
> >  5 files changed, 22 insertions(+)
> >
> > diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig index
> > b2a4f998c180..6c9a950b7010 100644
> > --- a/drivers/net/Kconfig
> > +++ b/drivers/net/Kconfig
> > @@ -313,6 +313,7 @@ config MACSEC
> >       select CRYPTO_AES
> >       select CRYPTO_GCM
> >       select GRO_CELLS
> > +     select SKB_EXTENSIONS
> >       help
> >          MACsec is an encryption standard for Ethernet.
> >
> > diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c index
> > 832f09ac075e..0960339e2442 100644
> > --- a/drivers/net/macsec.c
> > +++ b/drivers/net/macsec.c
> > @@ -3377,6 +3377,11 @@ static netdev_tx_t macsec_start_xmit(struct
> sk_buff *skb,
> >       int ret, len;
> >
> >       if (macsec_is_offloaded(netdev_priv(dev))) {
> > +             struct macsec_ext *secext = skb_ext_add(skb,
> > + SKB_EXT_MACSEC);
> > +
> > +             secext->sci = secy->sci;
> > +             secext->offloaded = true;
> > +
> >               skb->dev = macsec->real_dev;
> >               return dev_queue_xmit(skb);
> >       }
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index
> > 84d78df60453..4ee71c7848bf 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -4552,6 +4552,9 @@ enum skb_ext_id {  #endif  #if
> > IS_ENABLED(CONFIG_MCTP_FLOWS)
> >       SKB_EXT_MCTP,
> > +#endif
> > +#if IS_ENABLED(CONFIG_MACSEC)
> > +     SKB_EXT_MACSEC,
> >  #endif
> >       SKB_EXT_NUM, /* must be last */
> >  };
> > diff --git a/include/net/macsec.h b/include/net/macsec.h index
> > d6fa6b97f6ef..fcbca963c04d 100644
> > --- a/include/net/macsec.h
> > +++ b/include/net/macsec.h
> > @@ -20,6 +20,12 @@
> >  typedef u64 __bitwise sci_t;
> >  typedef u32 __bitwise ssci_t;
> >
> > +/* MACsec sk_buff extension data */
> > +struct macsec_ext {
> > +     sci_t sci;
> > +     bool offloaded;
> 
> It looks like the bool is not used/it's always true when the extension is
> attached? If so it's better to drop it and use the extension presence as the
> flag.

It's true for Tx but not for Rx.
The bool of the extension is false for any un-MACsec packet that is received by the NIC when the MACsec offload is on,
the MACsec layer should use this data and ignore those packets by forwarding them to the parent device.

> 
> BTW have you considered other options other then the skb extensions?
> e.g. could you use skb_metadata() here?

Other security offload like IPsec offload implemented in the same way,
so it looks like using SKB extension is the preferable approach for such use-cases.

> 
> Otherwise I think you need explicitly to take care of this extension at the
> GRO layer, see commit 8550ff8d8c75

Ack, will add it.

> 
> Thanks!
> 
> Paolo

thanks for the review.
apologize for the late respond.