linux-kernel - Re: [PATCH] bpf: allow BPF programs access skb_shared_info->gso

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] [day] [month] [year] [list]

Message-ID: <20180716223003.fvh6edj3zlfegy52@ast-mbp.dhcp.thefacebook.com>
Date:   Mon, 16 Jul 2018 15:30:04 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Eddie Hao <eddieh@...gle.com>
Cc:     ast@...nel.org, daniel@...earbox.net, davem@...emloft.net,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] bpf: allow BPF programs access skb_shared_info->gso_segs
 field

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