[PATCH net-next v2 0/2] Two Helper function about socket information

[PATCH net-next v2 0/2] Two Helper function about socket information

[Chenbo Feng]

From: Chenbo Feng <fengc@google.com>

Introduce two eBpf helper function to get the socket cookie and
socket uid for each packet. The helper function is useful when
the *sk field inside sk_buff is not empty.

Chenbo Feng (2):
  Add a helper function to get socket cookie in eBPF
  Add a eBPF helper function to retrieve socket uid

 include/linux/sock_diag.h |  1 +
 include/uapi/linux/bpf.h  | 16 +++++++++++++++-
 net/core/filter.c         | 32 ++++++++++++++++++++++++++++++++
 net/core/sock_diag.c      |  2 +-
 4 files changed, 49 insertions(+), 2 deletions(-)

-- 
2.7.4

[PATCH net-next v2 1/2] Add a helper function to get socket cookie in eBPF

[Chenbo Feng]

From: Chenbo Feng <fengc@google.com>

Retrieve the socket cookie generated by sock_gen_cookie() from a sk_buff
with a known socket. Generates a new cookie if one was not yet set.If
the socket pointer inside sk_buff is NULL, 0 is returned. The helper
function coud be useful in monitoring per socket networking traffic
statistics and provide a unique socket identifier per namespace.

Signed-off-by: Chenbo Feng <chenbofeng.kernel@gmail.com>
---
 include/linux/sock_diag.h |  1 +
 include/uapi/linux/bpf.h  |  9 ++++++++-
 net/core/filter.c         | 15 +++++++++++++++
 net/core/sock_diag.c      |  2 +-
 4 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/include/linux/sock_diag.h b/include/linux/sock_diag.h
index a0596ca0..a2f8109 100644
--- a/include/linux/sock_diag.h
+++ b/include/linux/sock_diag.h
@@ -24,6 +24,7 @@ void sock_diag_unregister(const struct sock_diag_handler *h);
 void sock_diag_register_inet_compat(int (*fn)(struct sk_buff *skb, struct nlmsghdr *nlh));
 void sock_diag_unregister_inet_compat(int (*fn)(struct sk_buff *skb, struct nlmsghdr *nlh));
 
+u64 sock_gen_cookie(struct sock *sk);
 int sock_diag_check_cookie(struct sock *sk, const __u32 *cookie);
 void sock_diag_save_cookie(struct sock *sk, __u32 *cookie);
 
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index e07fd5a..6923d21 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -449,6 +449,12 @@ union bpf_attr {
  *     Return:
  *       > 0 length of the string including the trailing NUL on success
  *       < 0 error
+ *
+ * u64 bpf_bpf_get_socket_cookie(skb)
+ *     Get the cookie for the socket stored inside sk_buff.
+ *     @skb: pointer to skb
+ *     Return: 8 Bytes non-decreasing number on success or 0 if the socket
+ *     field is missing inside sk_buff
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -496,7 +502,8 @@ union bpf_attr {
 	FN(get_numa_node_id),		\
 	FN(skb_change_head),		\
 	FN(xdp_adjust_head),		\
-	FN(probe_read_str),
+	FN(probe_read_str),		\
+	FN(get_socket_cookie),
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
  * function eBPF program intends to call
diff --git a/net/core/filter.c b/net/core/filter.c
index 0b753cb..632fb91 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -26,6 +26,7 @@
 #include <linux/mm.h>
 #include <linux/fcntl.h>
 #include <linux/socket.h>
+#include <linux/sock_diag.h>
 #include <linux/in.h>
 #include <linux/inet.h>
 #include <linux/netdevice.h>
@@ -2599,6 +2600,18 @@ static const struct bpf_func_proto bpf_xdp_event_output_proto = {
 	.arg5_type	= ARG_CONST_SIZE,
 };
 
+BPF_CALL_1(bpf_get_socket_cookie, struct sk_buff *, skb)
+{
+	return skb->sk ? sock_gen_cookie(skb->sk) : 0;
+}
+
+static const struct bpf_func_proto bpf_get_socket_cookie_proto = {
+	.func           = bpf_get_socket_cookie,
+	.gpl_only       = false,
+	.ret_type       = RET_INTEGER,
+	.arg1_type      = ARG_PTR_TO_CTX,
+};
+
 static const struct bpf_func_proto *
 bpf_base_func_proto(enum bpf_func_id func_id)
 {
@@ -2622,6 +2635,8 @@ bpf_base_func_proto(enum bpf_func_id func_id)
 	case BPF_FUNC_trace_printk:
 		if (capable(CAP_SYS_ADMIN))
 			return bpf_get_trace_printk_proto();
+	case BPF_FUNC_get_socket_cookie:
+		return &bpf_get_socket_cookie_proto;
 	default:
 		return NULL;
 	}
diff --git a/net/core/sock_diag.c b/net/core/sock_diag.c
index 6b10573..acd2a6c 100644
--- a/net/core/sock_diag.c
+++ b/net/core/sock_diag.c
@@ -19,7 +19,7 @@ static int (*inet_rcv_compat)(struct sk_buff *skb, struct nlmsghdr *nlh);
 static DEFINE_MUTEX(sock_diag_table_mutex);
 static struct workqueue_struct *broadcast_wq;
 
-static u64 sock_gen_cookie(struct sock *sk)
+u64 sock_gen_cookie(struct sock *sk)
 {
 	while (1) {
 		u64 res = atomic64_read(&sk->sk_cookie);
-- 
2.7.4

[PATCH net-next v2 2/2] Add a eBPF helper function to retrieve socket uid

[Chenbo Feng]

From: Chenbo Feng <fengc@google.com>

Returns the owner uid of the socket inside a sk_buff. This is useful to
perform per-UID accounting of network traffic or per-UID packet
filtering.

Signed-off-by: Chenbo Feng <chenbofeng.kernel@gmail.com>
---
 include/uapi/linux/bpf.h |  9 ++++++++-
 net/core/filter.c        | 17 +++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 6923d21..4854027 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -455,6 +455,12 @@ union bpf_attr {
  *     @skb: pointer to skb
  *     Return: 8 Bytes non-decreasing number on success or 0 if the socket
  *     field is missing inside sk_buff
+ *
+ * u32 bpf_get_socket_uid(skb)
+ *     Get the owner uid of the socket stored inside sk_buff.
+ *     @skb: pointer to skb
+ *     Return: uid of the socket owner on success or 0 if the socket pointer
+ *     inside sk_buff is NULL
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -503,7 +509,8 @@ union bpf_attr {
 	FN(skb_change_head),		\
 	FN(xdp_adjust_head),		\
 	FN(probe_read_str),		\
-	FN(get_socket_cookie),
+	FN(get_socket_cookie),		\
+	FN(get_socket_uid),
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
  * function eBPF program intends to call
diff --git a/net/core/filter.c b/net/core/filter.c
index 632fb91..523ed08 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2612,6 +2612,21 @@ static const struct bpf_func_proto bpf_get_socket_cookie_proto = {
 	.arg1_type      = ARG_PTR_TO_CTX,
 };
 
+BPF_CALL_1(bpf_get_socket_uid, struct sk_buff *, skb)
+{
+	struct sock *sk = skb->sk;
+	kuid_t kuid = sock_net_uid(net, sk && sk_fullsock(sk) ?
+				   sk : NULL);
+	return (u32)kuid.val;
+}
+
+static const struct bpf_func_proto bpf_get_socket_uid_proto = {
+	.func           = bpf_get_socket_uid,
+	.gpl_only       = false,
+	.ret_type       = RET_INTEGER,
+	.arg1_type      = ARG_PTR_TO_CTX,
+};
+
 static const struct bpf_func_proto *
 bpf_base_func_proto(enum bpf_func_id func_id)
 {
@@ -2637,6 +2652,8 @@ bpf_base_func_proto(enum bpf_func_id func_id)
 			return bpf_get_trace_printk_proto();
 	case BPF_FUNC_get_socket_cookie:
 		return &bpf_get_socket_cookie_proto;
+	case BPF_FUNC_get_socket_uid:
+		return &bpf_get_socket_uid_proto;
 	default:
 		return NULL;
 	}
-- 
2.7.4

Re: [PATCH net-next v2 2/2] Add a eBPF helper function to retrieve socket uid

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1267 bytes --]

Hi Chenbo,

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Chenbo-Feng/Two-Helper-function-about-socket-information/20170206-102835
config: x86_64-randconfig-x014-201706 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   net/core/filter.c: In function '____bpf_get_socket_uid':
>> net/core/filter.c:2618:29: error: 'net' undeclared (first use in this function)
     kuid_t kuid = sock_net_uid(net, sk && sk_fullsock(sk) ?
                                ^~~
   net/core/filter.c:2618:29: note: each undeclared identifier is reported only once for each function it appears in

vim +/net +2618 net/core/filter.c

  2612		.arg1_type      = ARG_PTR_TO_CTX,
  2613	};
  2614	
  2615	BPF_CALL_1(bpf_get_socket_uid, struct sk_buff *, skb)
  2616	{
  2617		struct sock *sk = skb->sk;
> 2618		kuid_t kuid = sock_net_uid(net, sk && sk_fullsock(sk) ?
  2619					   sk : NULL);
  2620		return (u32)kuid.val;
  2621	}

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 21667 bytes --]

Re: [PATCH net-next v2 1/2] Add a helper function to get socket cookie in eBPF

[Eric Dumazet]

On Sun, 2017-02-05 at 18:17 -0800, Chenbo Feng wrote:
> From: Chenbo Feng <fengc@google.com>
> 
> Retrieve the socket cookie generated by sock_gen_cookie() from a sk_buff
> with a known socket. Generates a new cookie if one was not yet set.If
> the socket pointer inside sk_buff is NULL, 0 is returned. The helper
> function coud be useful in monitoring per socket networking traffic
> statistics and provide a unique socket identifier per namespace.
> 
> Signed-off-by: Chenbo Feng <chenbofeng.kernel@gmail.com>
> ---

Acked-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net-next v2 1/2] Add a helper function to get socket cookie in eBPF

[Lorenzo Colitti]

On Mon, Feb 6, 2017 at 11:17 AM, Chenbo Feng
<chenbofeng.kernel@gmail.com> wrote:
> +BPF_CALL_1(bpf_get_socket_cookie, struct sk_buff *, skb)
> +{
> +       return skb->sk ? sock_gen_cookie(skb->sk) : 0;
> +}
> +

Does this code need to increment the socket refcount, or call
ACCESS_ONCE to get skb->sk? The socket filter codepath should be safe,
but if this function is called in xt_ebpf, could it race with
something that sets skb->sk to null?

Re: [PATCH net-next v2 2/2] Add a eBPF helper function to retrieve socket uid

[Eric Dumazet]

On Sun, 2017-02-05 at 18:17 -0800, Chenbo Feng wrote:
> From: Chenbo Feng <fengc@google.com>
> 
> Returns the owner uid of the socket inside a sk_buff. This is useful to
> perform per-UID accounting of network traffic or per-UID packet
> filtering.
> 
> Signed-off-by: Chenbo Feng <chenbofeng.kernel@gmail.com>
> ---
> +BPF_CALL_1(bpf_get_socket_uid, struct sk_buff *, skb)
> +{
> +	struct sock *sk = skb->sk;
> +	kuid_t kuid = sock_net_uid(net, sk && sk_fullsock(sk) ?
> +				   sk : NULL);
> +	return (u32)kuid.val;
> +}
> +

Have you considered to use sk_to_full_sk() ?

struct sock *sk = sk_to_full_sk(skb->sk);
kuid_t kuid = sock_net_uid(net, sk);

Re: [PATCH net-next v2 1/2] Add a helper function to get socket cookie in eBPF

[Eric Dumazet]

On Mon, 2017-02-06 at 12:01 +0900, Lorenzo Colitti wrote:
> On Mon, Feb 6, 2017 at 11:17 AM, Chenbo Feng
> <chenbofeng.kernel@gmail.com> wrote:
> > +BPF_CALL_1(bpf_get_socket_cookie, struct sk_buff *, skb)
> > +{
> > +       return skb->sk ? sock_gen_cookie(skb->sk) : 0;
> > +}
> > +
> 
> Does this code need to increment the socket refcount, or call
> ACCESS_ONCE to get skb->sk? The socket filter codepath should be safe,
> but if this function is called in xt_ebpf, could it race with
> something that sets skb->sk to null?

I do not see how this could possibly happen.

READ_ONCE() would not prevent the 'old' sk from disappearing anyway.

Re: [PATCH net-next v2 1/2] Add a helper function to get socket cookie in eBPF

[Daniel Borkmann]

On 02/06/2017 03:17 AM, Chenbo Feng wrote:
> From: Chenbo Feng <fengc@google.com>
>
> Retrieve the socket cookie generated by sock_gen_cookie() from a sk_buff
> with a known socket. Generates a new cookie if one was not yet set.If
> the socket pointer inside sk_buff is NULL, 0 is returned. The helper
> function coud be useful in monitoring per socket networking traffic
> statistics and provide a unique socket identifier per namespace.
>
> Signed-off-by: Chenbo Feng <chenbofeng.kernel@gmail.com>
[...]
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 0b753cb..632fb91 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -26,6 +26,7 @@
>   #include <linux/mm.h>
>   #include <linux/fcntl.h>
>   #include <linux/socket.h>
> +#include <linux/sock_diag.h>
>   #include <linux/in.h>
>   #include <linux/inet.h>
>   #include <linux/netdevice.h>
> @@ -2599,6 +2600,18 @@ static const struct bpf_func_proto bpf_xdp_event_output_proto = {
>   	.arg5_type	= ARG_CONST_SIZE,
>   };
>
> +BPF_CALL_1(bpf_get_socket_cookie, struct sk_buff *, skb)
> +{
> +	return skb->sk ? sock_gen_cookie(skb->sk) : 0;
> +}
> +
> +static const struct bpf_func_proto bpf_get_socket_cookie_proto = {
> +	.func           = bpf_get_socket_cookie,
> +	.gpl_only       = false,
> +	.ret_type       = RET_INTEGER,
> +	.arg1_type      = ARG_PTR_TO_CTX,
> +};
> +
>   static const struct bpf_func_proto *
>   bpf_base_func_proto(enum bpf_func_id func_id)
>   {
> @@ -2622,6 +2635,8 @@ bpf_base_func_proto(enum bpf_func_id func_id)
>   	case BPF_FUNC_trace_printk:
>   		if (capable(CAP_SYS_ADMIN))
>   			return bpf_get_trace_printk_proto();
> +	case BPF_FUNC_get_socket_cookie:
> +		return &bpf_get_socket_cookie_proto;
>   	default:
>   		return NULL;
>   	}

This still has one issue that would need to be addressed, otherwise
looks good and ready to me.

Issue is that it cannot be added to bpf_base_func_proto(), because
that is also used by cg_sock_ops, which has struct sock as input
instead of struct sk_buff.

I suggest, we initially add this to both, sk_filter_func_proto() and
tc_cls_act_func_proto() and, if needed, we could follow-up with other
prog types at some later point in time. Otherwise, looks fine.

> diff --git a/net/core/sock_diag.c b/net/core/sock_diag.c
> index 6b10573..acd2a6c 100644
> --- a/net/core/sock_diag.c
> +++ b/net/core/sock_diag.c
> @@ -19,7 +19,7 @@ static int (*inet_rcv_compat)(struct sk_buff *skb, struct nlmsghdr *nlh);
>   static DEFINE_MUTEX(sock_diag_table_mutex);
>   static struct workqueue_struct *broadcast_wq;
>
> -static u64 sock_gen_cookie(struct sock *sk)
> +u64 sock_gen_cookie(struct sock *sk)
>   {
>   	while (1) {
>   		u64 res = atomic64_read(&sk->sk_cookie);
>