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

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

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@cumulusnetworks.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.