Re: [PATCH bpf 03/11] bpf: Add write access to tcp_sock and sock fields

[Alexei Starovoitov]

On 12/18/17 10:21 PM, Lawrence Brakmo wrote:

+#define SOCK_OPS_SET_FIELD(FIELD_NAME, OBJ)  \
+   do {  \
+   int reg = BPF_REG_9;  \
+   BUILD_BUG_ON(FIELD_SIZEOF(OBJ, FIELD_NAME) >   \
+FIELD_SIZEOF(struct bpf_sock_ops, FIELD_NAME));  \
+   while (si->dst_reg == reg || si->src_reg == reg)\
+   reg--;\
+   *insn++ = BPF_STX_MEM(BPF_DW, si->dst_reg, reg, 
   \
+ offsetof(struct bpf_sock_ops_kern,  \
+  temp));\
+   *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(   \
+   struct bpf_sock_ops_kern, \
+   is_fullsock), \
+ reg, si->dst_reg, 
   \
+ offsetof(struct bpf_sock_ops_kern,  \
+  is_fullsock)); \
+   *insn++ = BPF_JMP_IMM(BPF_JEQ, reg, 0, 2);\
+   *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(   \
+   struct bpf_sock_ops_kern, sk),\
+ reg, si->dst_reg, 
   \
+ offsetof(struct bpf_sock_ops_kern, sk));\
+   *insn++ = BPF_STX_MEM(BPF_FIELD_SIZEOF(OBJ, FIELD_NAME),  \
+ reg, si->src_reg, 
   \
+ offsetof(OBJ, FIELD_NAME)); \
+   *insn++ = BPF_LDX_MEM(BPF_DW, reg, si->dst_reg, 
   \
+ offsetof(struct bpf_sock_ops_kern,  \
+  temp));\
+   } while (0)


that's neat. I like it.
I guess the prog can check is_fullsock on its own to see whether writes
will fail or not, so JEQ above is ok.
Only while() loop looks a bit scary.
May be replace with two 'if' ?
if (si->dst_reg == reg || si->src_reg == reg)
  reg --;
if (si->dst_reg == reg || si->src_reg == reg)
  reg --;
so it's clear that tmp reg will be reg_7, 8 or 9.

Re: [PATCH bpf 03/11] bpf: Add write access to tcp_sock and sock fields

[Daniel Borkmann]

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 
> ---
>  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(_ops, 0, sizeof(sock_ops));
> + memset(_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

[PATCH bpf 03/11] bpf: Add write access to tcp_sock and sock fields

[Lawrence Brakmo]

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 
---
 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(_ops, 0, sizeof(sock_ops));
+   memset(_ops, 0, offsetof(struct bpf_sock_ops_kern, is_fullsock));
if (sk_fullsock(sk)) {
sock_ops.is_fullsock = 1;
sock_owned_by_me(sk);
diff --git a/net/core/filter.c b/net/core/filter.c
index f808269..97e65df 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -4469,6 +4469,52 @@ static u32 sock_ops_convert_ctx_access(enum 
bpf_access_type type,
  offsetof(OBJ, FIELD_NAME)); \
} while (0)
 
+/* Helper macro for adding write access to tcp_sock or sock fields.
+ * The macro is called with two registers, dst_reg which contains a pointer
+ * to ctx (context) and src_reg which contains the value that should be
+ * stored. However, we need an aditional register since we cannot overwrite
+ * dst_reg because it may be used later in the program.
+ * Instead we "borrow" one of the other register. We first save its value
+ * into a new (temp) field in bpf_sock_ops_kern, use it, and then restore
+ * it at the end of the macro.
+ */
+#define SOCK_OPS_SET_FIELD(FIELD_NAME, OBJ)  \
+   do {  \
+   int reg = BPF_REG_9;  \
+   BUILD_BUG_ON(FIELD_SIZEOF(OBJ, FIELD_NAME) >  \
+FIELD_SIZEOF(struct bpf_sock_ops, FIELD_NAME));  \
+   while (si->dst_reg == reg || si->src_reg == reg)  \
+   reg--;\
+   *insn++ = BPF_STX_MEM(BPF_DW, si->dst_reg, reg,   \
+ offsetof(struct bpf_sock_ops_kern,  \
+  temp));\
+   *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(   \
+   struct bpf_sock_ops_kern, \
+   is_fullsock), \
+ reg, si->dst_reg,   \
+ offsetof(struct bpf_sock_ops_kern,  \
+  is_fullsock)); \
+   *insn++ = BPF_JMP_IMM(BPF_JEQ, reg, 0, 2);\
+   *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(   \
+   struct bpf_sock_ops_kern, sk),\
+ reg, si->dst_reg,   \
+ offsetof(struct bpf_sock_ops_kern, sk));\
+   *insn++ = BPF_STX_MEM(BPF_FIELD_SIZEOF(OBJ, FIELD_NAME),  \
+ reg, si->src_reg,   \
+ offsetof(OBJ, FIELD_NAME)); \
+   *insn++ = BPF_LDX_MEM(BPF_DW, reg, si->dst_reg,   \
+ offsetof(struct bpf_sock_ops_kern,  \
+  temp));\
+   } while (0)
+
+#define