Re: [PATCH] bpf: allow BPF programs access skb_shared_info->gso_segs field

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

On Sun, Jul 15, 2018 at 01:45:24AM -0700, Eddie Hao wrote:
> This adds the ability to read gso_segs from a BPF program. This is useful
> for user space monitoring systems to get gso_segs statistics for each
> arbitrary group of flows defined by the BPF program.
> 
> Signed-off-by: Eddie Hao <eddieh@google.com>
> ---
>  include/uapi/linux/bpf.h | 1 +
>  net/core/filter.c        | 9 +++++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index b7db3261c62d..282085445616 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -2266,6 +2266,7 @@ struct __sk_buff {
>  	/* ... here. */
>  
>  	__u32 data_meta;
> +	unsigned short gso_segs;

'short' will make it harder to extend due to struct hole.
please use __u32 instead.

>  };
>  
>  struct bpf_tunnel_key {
> diff --git a/net/core/filter.c b/net/core/filter.c
> index b9ec916f4e3a..f17e249f3fbb 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -5820,6 +5820,15 @@ static u32 bpf_convert_ctx_access(enum bpf_access_type type,
>  				      bpf_target_off(struct sock_common,
>  						     skc_num, 2, target_size));
>  		break;
> +
> +	case offsetof(struct __sk_buff, gso_segs):
> +		BUILD_BUG_ON(FIELD_SIZEOF(struct skb_shared_info, gso_segs) !=
> +					  sizeof(unsigned short));
> +		off = offsetof(struct sk_buff, end);
> +		off += offsetof(struct skb_shared_info, gso_segs);

did you test this patch?
Above math doesn't look correct.
NET_SKBUFF_DATA_USES_OFFSET is not handled. At least one more LDX is needed.

> +		*insn++ = BPF_LDX_MEM(BPF_SIZEOF(unsigned short), si->dst_reg,
> +						 si->src_reg, off);

I think BUILD_BUG_ON is a bit of overkill when BPF_LDX_MEM
can use BPF_FIELD_SIZEOF(struct skb_shared_info, gso_segs) instead.