netdev - Re: [PATCH net-next 14/17] net: atlantic: MACSec ingress offload implementation

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: <4150e0930415966768ca2fa4621194e475c25235.camel@perches.com>
Date:   Mon, 23 Mar 2020 11:17:34 -0700
From:   Joe Perches <joe@...ches.com>
To:     Igor Russkikh <irusskikh@...vell.com>, netdev@...r.kernel.org
Cc:     Mark Starovoytov <mstarovoitov@...vell.com>,
        Sabrina Dubroca <sd@...asysnail.net>,
        Antoine Tenart <antoine.tenart@...tlin.com>
Subject: Re: [PATCH net-next 14/17] net: atlantic: MACSec ingress offload
 implementation

On Mon, 2020-03-23 at 16:13 +0300, Igor Russkikh wrote:
> This patch adds support for MACSec ingress HW offloading on Atlantic
> network cards.

Just random notes, I haven't looked at much of the patch set.

> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_macsec.c b/drivers/net/ethernet/aquantia/atlantic/aq_macsec.c
[]
> +static int aq_rxsc_validate_frames(const enum macsec_validation_type validate)
> +{
> +	switch (validate) {
> +	case MACSEC_VALIDATE_DISABLED:
> +		return 2;
> +	case MACSEC_VALIDATE_CHECK:
> +		return 1;
> +	case MACSEC_VALIDATE_STRICT:
> +		return 0;
> +	default:
> +		break;
> +	}
> +
> +	/* should never be here */
> +	WARN_ON(true);

If this is ever reached at all, this should likely
be a WARN_ONCE to avoid log spam.

> +	return 0;
> +}
> +
> +static int aq_set_rxsc(struct aq_nic_s *nic, const u32 rxsc_idx)
> +{
> +	const struct aq_macsec_rxsc *aq_rxsc =
> +		&nic->macsec_cfg->aq_rxsc[rxsc_idx];
> +	struct aq_mss_ingress_preclass_record pre_class_record;
> +	const struct macsec_rx_sc *rx_sc = aq_rxsc->sw_rxsc;
> +	const struct macsec_secy *secy = aq_rxsc->sw_secy;
> +	const u32 hw_sc_idx = aq_rxsc->hw_sc_idx;
> +	struct aq_mss_ingress_sc_record sc_record;
> +	struct aq_hw_s *hw = nic->aq_hw;
> +	__be64 nsci;
> +	int ret = 0;
> +
> +	netdev_dbg(nic->ndev,
> +		   "set rx_sc: rxsc_idx=%d, sci %#llx, hw_sc_idx=%d\n",

Could use __func__ or just remove as ftrace works.

> +		   rxsc_idx, rx_sc->sci, hw_sc_idx);
> +
> +	memset(&pre_class_record, 0, sizeof(pre_class_record));
> +	nsci = cpu_to_be64((__force u64)rx_sc->sci);
> +	memcpy(pre_class_record.sci, &nsci, sizeof(nsci));

put_unaligned_be64

> +	pre_class_record.sci_mask = 0xff;
> +	/* match all MACSEC ethertype packets */
> +	pre_class_record.eth_type = ETH_P_MACSEC;
> +	pre_class_record.eth_type_mask = 0x3;
> +
> +	aq_ether_addr_to_mac(pre_class_record.mac_sa, (char *)&rx_sc->sci);
> +	pre_class_record.sa_mask = 0x3f;
> +
> +	pre_class_record.an_mask = nic->macsec_cfg->sc_sa;
> +	pre_class_record.sc_idx = hw_sc_idx;
> +	/* strip SecTAG & forward for decryption */
> +	pre_class_record.action = 0x0;
> +	pre_class_record.valid = 1;
> +
> +	ret = aq_mss_set_ingress_preclass_record(hw, &pre_class_record,
> +						 2 * rxsc_idx + 1);
> +	if (ret) {
> +		netdev_err(nic->ndev,
> +			"aq_mss_set_ingress_preclass_record failed with %d\n",
> +			ret);

Every return of this function emits netdev_err on error.

Why not put the err in the function itself and likely
use a ratelimit on it too?