netdev - Re: [PATCH net-next v6 4/6] bpf: Add support for reading socket family, type, protocol

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: <20161201055314.GA42375@ast-mbp.thefacebook.com>
Date:   Wed, 30 Nov 2016 21:53:17 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     David Ahern <dsa@...ulusnetworks.com>
Cc:     netdev@...r.kernel.org, daniel@...que.org, ast@...com,
        daniel@...earbox.net, maheshb@...gle.com, tgraf@...g.ch
Subject: Re: [PATCH net-next v6 4/6] bpf: Add support for reading socket
 family, type, protocol

On Wed, Nov 30, 2016 at 10:16:48AM -0800, David Ahern wrote:
> Add socket family, type and protocol to bpf_sock allowing bpf programs
> read-only access.
> 
> Signed-off-by: David Ahern <dsa@...ulusnetworks.com>
...
> +static u32 convert_sock_access(int sock_field, int dst_reg, int src_reg,
> +			       struct bpf_insn *insn_buf)
> +{
> +	struct bpf_insn *insn = insn_buf;
> +
> +	switch (sock_field) {
> +	case SOCKF_AD_TYPE:
> +		*insn++ = BPF_LDX_MEM(BPF_W, dst_reg, src_reg,
> +				      offsetof(struct sock, __sk_flags_offset));
> +		*insn++ = BPF_ALU32_IMM(BPF_AND, dst_reg, SK_FL_TYPE_MASK);
> +		*insn++ = BPF_ALU32_IMM(BPF_RSH, dst_reg, SK_FL_TYPE_SHIFT);
> +		break;

Wasn't that complex after all? ;)
This part looks good.

> +	case offsetof(struct bpf_sock, type):
> +		return convert_sock_access(SOCKF_AD_TYPE, dst_reg, src_reg,
> +					   insn_buf);

my only request-for-change is to place the above 3 lines of *insn++ here
instead of adding new helper function for it.
In case of skb it was a helper, because it was called from different places.
Here this is the only place where it ever be needed, so extra helper
only confuses the reader.

The rest of the patches look good to me.
I'm not Ack-ing them yet, since you didn't carry over my Ack
from the first patch and I had to re-review it again this time.

Thanks.