netdev - Re: [PATCH net-next 13/17] net: atlantic: MACSec ingress offload HW bindings

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:   Mon, 23 Mar 2020 10:32:50 -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 13/17] net: atlantic: MACSec ingress offload HW
 bindings

On Mon, 2020-03-23 at 16:13 +0300, Igor Russkikh wrote:
> From: Mark Starovoytov <mstarovoitov@...vell.com>
[]
> +static int
> +set_ingress_prectlf_record(struct aq_hw_s *hw,
> +			   const struct aq_mss_ingress_prectlf_record *rec,
> +			   u16 table_index)
> +{
> +	u16 packed_record[6];
> +
> +	if (table_index >= NUMROWS_INGRESSPRECTLFRECORD)
> +		return -EINVAL;
> +
> +	memset(packed_record, 0, sizeof(u16) * 6);
> +
> +	packed_record[0] = (packed_record[0] & 0x0000) |
> +			   (((rec->sa_da[0] >> 0) & 0xFFFF) << 0);
> +	packed_record[1] = (packed_record[1] & 0x0000) |
> +			   (((rec->sa_da[0] >> 16) & 0xFFFF) << 0);
> +	packed_record[2] = (packed_record[2] & 0x0000) |
> +			   (((rec->sa_da[1] >> 0) & 0xFFFF) << 0);
> +	packed_record[3] = (packed_record[3] & 0x0000) |
> +			   (((rec->eth_type >> 0) & 0xFFFF) << 0);
> +	packed_record[4] = (packed_record[4] & 0x0000) |
> +			   (((rec->match_mask >> 0) & 0xFFFF) << 0);
> +	packed_record[5] = (packed_record[5] & 0xFFF0) |
> +			   (((rec->match_type >> 0) & 0xF) << 0);
> +	packed_record[5] =
> +		(packed_record[5] & 0xFFEF) | (((rec->action >> 0) & 0x1) << 4);

This sort of code is not very readable.

Using val & 0x0000 is silly.

Using >> 0 and << 0 is pretty useless.

Masking a u16 with 0xffff is also pretty useless.

It seems a lot of this patch does this unnecessarily.