netdev - Re: [PATCH net-next 2/6] net/bpf: split VLAN_PRESENT bit handling from VLAN

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: <c420b7a8-73df-3e9c-962a-a295ad0a6139@iogearbox.net>
Date:   Mon, 19 Nov 2018 12:26:46 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Michał Mirosław <mirq-linux@...e.qmqm.pl>,
        netdev@...r.kernel.org
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        "David S. Miller" <davem@...emloft.net>,
        James Hogan <jhogan@...nel.org>, linux-mips@...ux-mips.org,
        linuxppc-dev@...ts.ozlabs.org,
        Michael Ellerman <mpe@...erman.id.au>,
        Paul Burton <paul.burton@...s.com>,
        Paul Mackerras <paulus@...ba.org>,
        Ralf Baechle <ralf@...ux-mips.org>, sparclinux@...r.kernel.org
Subject: Re: [PATCH net-next 2/6] net/bpf: split VLAN_PRESENT bit handling
 from VLAN_TCI

On 11/10/2018 07:58 PM, Michał Mirosław wrote:
> Signed-off-by: Michał Mirosław <mirq-linux@...e.qmqm.pl>

Why you have empty commit messages for non-trivial changes like this in
4 out of 6 of your patches ...

How was it tested on the JITs you were changing? Did you test on both,
big and little endian machines?

> ---
>  net/core/filter.c | 40 +++++++++++++++++++++-------------------
>  1 file changed, 21 insertions(+), 19 deletions(-)
> 
> diff --git a/net/core/filter.c b/net/core/filter.c
> index e521c5ebc7d1..c151b906df53 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -296,22 +296,21 @@ static u32 convert_skb_access(int skb_field, int dst_reg, int src_reg,
>  		break;
>  
>  	case SKF_AD_VLAN_TAG:
> -	case SKF_AD_VLAN_TAG_PRESENT:
>  		BUILD_BUG_ON(FIELD_SIZEOF(struct sk_buff, vlan_tci) != 2);
> -		BUILD_BUG_ON(VLAN_TAG_PRESENT != 0x1000);
>  
>  		/* dst_reg = *(u16 *) (src_reg + offsetof(vlan_tci)) */
>  		*insn++ = BPF_LDX_MEM(BPF_H, dst_reg, src_reg,
>  				      offsetof(struct sk_buff, vlan_tci));
> -		if (skb_field == SKF_AD_VLAN_TAG) {
> -			*insn++ = BPF_ALU32_IMM(BPF_AND, dst_reg,
> -						~VLAN_TAG_PRESENT);
> -		} else {
> -			/* dst_reg >>= 12 */
> -			*insn++ = BPF_ALU32_IMM(BPF_RSH, dst_reg, 12);
> -			/* dst_reg &= 1 */
> +#ifdef VLAN_TAG_PRESENT
> +		*insn++ = BPF_ALU32_IMM(BPF_AND, dst_reg, ~VLAN_TAG_PRESENT);
> +#endif
> +		break;
> +	case SKF_AD_VLAN_TAG_PRESENT:
> +		*insn++ = BPF_LDX_MEM(BPF_B, dst_reg, src_reg, PKT_VLAN_PRESENT_OFFSET());
> +		if (PKT_VLAN_PRESENT_BIT)
> +			*insn++ = BPF_ALU32_IMM(BPF_RSH, dst_reg, PKT_VLAN_PRESENT_BIT);
> +		if (PKT_VLAN_PRESENT_BIT < 7)
>  			*insn++ = BPF_ALU32_IMM(BPF_AND, dst_reg, 1);
> -		}
>  		break;
>  	}
>  
> @@ -6140,19 +6139,22 @@ static u32 bpf_convert_ctx_access(enum bpf_access_type type,
>  		break;
>  
>  	case offsetof(struct __sk_buff, vlan_present):
> +		*target_size = 1;
> +		*insn++ = BPF_LDX_MEM(BPF_B, si->dst_reg, si->src_reg,
> +				      PKT_VLAN_PRESENT_OFFSET());
> +		if (PKT_VLAN_PRESENT_BIT)
> +			*insn++ = BPF_ALU32_IMM(BPF_RSH, si->dst_reg, PKT_VLAN_PRESENT_BIT);
> +		if (PKT_VLAN_PRESENT_BIT < 7)
> +			*insn++ = BPF_ALU32_IMM(BPF_AND, si->dst_reg, 1);
> +		break;
> +
>  	case offsetof(struct __sk_buff, vlan_tci):
> -		BUILD_BUG_ON(VLAN_TAG_PRESENT != 0x1000);
> -
>  		*insn++ = BPF_LDX_MEM(BPF_H, si->dst_reg, si->src_reg,
>  				      bpf_target_off(struct sk_buff, vlan_tci, 2,
>  						     target_size));
> -		if (si->off == offsetof(struct __sk_buff, vlan_tci)) {
> -			*insn++ = BPF_ALU32_IMM(BPF_AND, si->dst_reg,
> -						~VLAN_TAG_PRESENT);
> -		} else {
> -			*insn++ = BPF_ALU32_IMM(BPF_RSH, si->dst_reg, 12);
> -			*insn++ = BPF_ALU32_IMM(BPF_AND, si->dst_reg, 1);
> -		}
> +#ifdef VLAN_TAG_PRESENT
> +		*insn++ = BPF_ALU32_IMM(BPF_AND, si->dst_reg, ~VLAN_TAG_PRESENT);
> +#endif
>  		break;
>  
>  	case offsetof(struct __sk_buff, cb[0]) ...
>