netdev - Re: [PATCH net-next v3 2/3] net/macsec: Add MACsec skb extension Rx 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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 14 Jun 2022 15:55:13 +0200
From:   Paolo Abeni <pabeni@...hat.com>
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 v3 2/3] net/macsec: Add MACsec skb extension Rx
 Data path support

On Mon, 2022-06-13 at 14:19 +0300, Lior Nahmanson wrote:
> Like in the Tx changes, packet that don't have SecTAG
> header aren't necessary been offloaded by the HW.
> Therefore, the MACsec driver needs to distinguish if the packet
> was offloaded or not and handle accordingly.
> Moreover, if there are more than one MACsec device with the same MAC
> address as in the packet's destination MAC, the packet will forward only
> to this device and only to the desired one.
> 
> Used SKB extension and marking it by the HW if the packet was offloaded
> and to which MACsec offload device it belongs according to the packet's
> SCI.
> 
> 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>
> ---
> v1->v2:
> - added GRO support
> - added offloaded field to struct macsec_ext
> v2->v3:
> - removed Issue and Change-Id from commit message
> ---
>  drivers/net/macsec.c |  8 +++++++-
>  include/net/macsec.h |  1 +
>  net/core/gro.c       | 16 ++++++++++++++++
>  3 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
> index 9be0606d70da..7b7baf3dd596 100644
> --- a/drivers/net/macsec.c
> +++ b/drivers/net/macsec.c
> @@ -999,11 +999,13 @@ static enum rx_handler_result handle_not_macsec(struct sk_buff *skb)
>  	/* Deliver to the uncontrolled port by default */
>  	enum rx_handler_result ret = RX_HANDLER_PASS;
>  	struct ethhdr *hdr = eth_hdr(skb);
> +	struct macsec_ext *macsec_ext;
>  	struct macsec_rxh_data *rxd;
>  	struct macsec_dev *macsec;
>  
>  	rcu_read_lock();
>  	rxd = macsec_data_rcu(skb->dev);
> +	macsec_ext = skb_ext_find(skb, SKB_EXT_MACSEC);
>  
>  	list_for_each_entry_rcu(macsec, &rxd->secys, secys) {
>  		struct sk_buff *nskb;
> @@ -1013,7 +1015,11 @@ static enum rx_handler_result handle_not_macsec(struct sk_buff *skb)
>  		/* 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 (macsec_is_offloaded(macsec) && netif_running(ndev) &&
> +		    (!macsec_ext || macsec_ext->offloaded)) {
> +			if ((macsec_ext) && (!find_rx_sc(&macsec->secy, macsec_ext->sci)))
> +				continue;
> +
>  			if (ether_addr_equal_64bits(hdr->h_dest,
>  						    ndev->dev_addr)) {
>  				/* exact match, divert skb to this port */
> diff --git a/include/net/macsec.h b/include/net/macsec.h
> index 6de49d9c98bc..fcbca963c04d 100644
> --- a/include/net/macsec.h
> +++ b/include/net/macsec.h
> @@ -23,6 +23,7 @@ typedef u32 __bitwise ssci_t;
>  /* MACsec sk_buff extension data */
>  struct macsec_ext {
>  	sci_t sci;
> +	bool offloaded;
>  };
>  
>  typedef union salt {
> diff --git a/net/core/gro.c b/net/core/gro.c
> index b4190eb08467..f68e950be37f 100644
> --- a/net/core/gro.c
> +++ b/net/core/gro.c
> @@ -1,5 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0-or-later
>  #include <net/gro.h>
> +#include <net/macsec.h>
>  #include <net/dst_metadata.h>
>  #include <net/busy_poll.h>
>  #include <trace/events/net.h>
> @@ -390,6 +391,10 @@ static void gro_list_prepare(const struct list_head *head,
>  			struct tc_skb_ext *skb_ext;
>  			struct tc_skb_ext *p_ext;
>  #endif
> +#if IS_ENABLED(CONFIG_SKB_EXTENSIONS) && IS_ENABLED(CONFIG_MACSEC)
> +			struct macsec_ext *macsec_skb_ext;
> +			struct macsec_ext *macsec_p_ext;
> +#endif
>  
>  			diffs |= p->sk != skb->sk;
>  			diffs |= skb_metadata_dst_cmp(p, skb);
> @@ -402,6 +407,17 @@ static void gro_list_prepare(const struct list_head *head,
>  			diffs |= (!!p_ext) ^ (!!skb_ext);
>  			if (!diffs && unlikely(skb_ext))
>  				diffs |= p_ext->chain ^ skb_ext->chain;
> +#endif
> +#if IS_ENABLED(CONFIG_SKB_EXTENSIONS) && IS_ENABLED(CONFIG_MACSEC)
> +			macsec_skb_ext = skb_ext_find(skb, SKB_EXT_MACSEC);
> +			macsec_p_ext = skb_ext_find(p, SKB_EXT_MACSEC);
> +
> +			diffs |= (!!macsec_p_ext) ^ (!!macsec_skb_ext);
> +			if (!diffs && unlikely(macsec_skb_ext)) {
> +				diffs |= (__force unsigned long)macsec_p_ext->sci ^
> +					 (__force unsigned long)macsec_skb_ext->sci;
> +				diffs |= macsec_p_ext->offloaded ^ macsec_skb_ext->offloaded;
> +			}
>  #endif 		}
> 
The main reason I suggested to look for the a possible alternative to
the skb extension is that the GRO stage is becoming bigger (and slower)
with any of such addition.

The 'slow_gro' protects the common use-case from any additional
conditionals and intructions, I still have some concerns due to the
increased code size.

This is not a reject, I'm mostly looking for a 2nd opinion.

Thanks,

Paolo