On 12/19/2017 07:21 AM, Lawrence Brakmo wrote:
> This patch adds a macro, SOCK_OPS_SET_FIELD, for writing to
> struct tcp_sock or struct sock fields. This required adding a new
> field "temp" to struct bpf_sock_ops_kern for temporary storage that
> is used by sock_ops_convert_ctx_access. It is used to store and recover
> the contents of a register, so the register can be used to store the
> address of the sk. Since we cannot overwrite the dst_reg because it
> contains the pointer to ctx, nor the src_reg since it contains the value
> we want to store, we need an extra register to contain the address
> of the sk.
>
> Also adds the macro SOCK_OPS_GET_OR_SET_FIELD that calls one of the
> GET or SET macros depending on the value of the TYPE field.
>
> Signed-off-by: Lawrence Brakmo <bra...@fb.com>
> ---
> include/linux/filter.h | 3 +++
> include/net/tcp.h | 2 +-
> net/core/filter.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 5feb441..8929162 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -987,6 +987,9 @@ struct bpf_sock_ops_kern {
> u32 replylong[4];
> };
> u32 is_fullsock;
> + u64 temp; /* Used by sock_ops_convert_ctx_access
> + * as temporary storaage of a register
> + */
> };
>
> #endif /* __LINUX_FILTER_H__ */
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 6cc205c..e0213f1 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -2011,7 +2011,7 @@ static inline int tcp_call_bpf(struct sock *sk, int op)
> struct bpf_sock_ops_kern sock_ops;
> int ret;
>
> - memset(&sock_ops, 0, sizeof(sock_ops));
> + memset(&sock_ops, 0, offsetof(struct bpf_sock_ops_kern, is_fullsock));
I don't think this is correct. sock_ops is on stack, so above you only
zero up to the offset of is_fullsock, but not including it, so when
you have !sk_fullsock(sk), then your BPF prog will still act as if the
sock_ops.is_fullsock was set in case prior stack garbage said so.
> if (sk_fullsock(sk)) {
> sock_ops.is_fullsock = 1;
> sock_owned_by_me(sk);
Thanks,
Daniel
