[PATCH v2 net-next 0/8] bpf: Add option to set mark and priority in cgroup sock programs

[PATCH v2 net-next 0/8] bpf: Add option to set mark and priority in cgroup sock programs

[David Ahern]

Add option to set mark and priority in addition to bound device for newly
created sockets. Also, allow the bpf programs to use the get_current_uid_gid
helper meaning socket marks, priority and device can be set base on the
uid/gid of the running process.

For flexbility in deploying these programs, option is added to allow cgroups
to be walked from current to root running any program attached. This allows
one cgroup level to control the device a socket is bound to (e.g, a VRF) while
cgroups can be used to set socket marks and priority.

Sample programs are updated to demonstrate the new options.

v2
- added flag to control recursive behavior as requested by Alexei
- added comment to sock_filter_func_proto regarding use of
  get_current_uid_gid helper
- updated test programs for recursive option

David Ahern (8):
  bpf: Add support for recursively running cgroup sock filters
  bpf: Add mark and priority to sock options that can be set
  bpf: Allow cgroup sock filters to use get_current_uid_gid helper
  samples/bpf: Update sock test to allow setting mark and priority
  samples/bpf: Add detach option to test_cgrp2_sock
  samples/bpf: Add option to dump socket settings
  samples/bpf: Add test case for nested socket options
  samples/bpf: Update cgroup socket examples to use uid gid helper

 include/linux/bpf-cgroup.h      |  10 +-
 include/uapi/linux/bpf.h        |  11 ++
 kernel/bpf/cgroup.c             |  29 +++--
 kernel/bpf/syscall.c            |   6 +-
 kernel/cgroup/cgroup.c          |  25 +++-
 net/core/filter.c               |  42 ++++++-
 samples/bpf/sock_flags_kern.c   |   5 +
 samples/bpf/test_cgrp2_sock.c   | 258 ++++++++++++++++++++++++++++++++++++----
 samples/bpf/test_cgrp2_sock.sh  |   2 +-
 samples/bpf/test_cgrp2_sock3.sh | 162 +++++++++++++++++++++++++
 10 files changed, 506 insertions(+), 44 deletions(-)
 create mode 100755 samples/bpf/test_cgrp2_sock3.sh

-- 
2.1.4

[PATCH v2 net-next 1/8] bpf: Add support for recursively running cgroup sock filters

[David Ahern]

Add support for recursively applying sock filters attached to a cgroup.
For now, start with the inner cgroup attached to the socket and work back
to the root or first cgroup without the recursive flag set. Once the
recursive flag is set for a cgroup all descendant group's must have the
flag as well.

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 include/linux/bpf-cgroup.h | 10 ++++++----
 include/uapi/linux/bpf.h   |  9 +++++++++
 kernel/bpf/cgroup.c        | 29 ++++++++++++++++++++++-------
 kernel/bpf/syscall.c       |  6 +++---
 kernel/cgroup/cgroup.c     | 25 +++++++++++++++++++++++--
 5 files changed, 63 insertions(+), 16 deletions(-)

diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
index d41d40ac3efd..2d02187f242f 100644
--- a/include/linux/bpf-cgroup.h
+++ b/include/linux/bpf-cgroup.h
@@ -23,6 +23,7 @@ struct cgroup_bpf {
 	struct bpf_prog *prog[MAX_BPF_ATTACH_TYPE];
 	struct bpf_prog __rcu *effective[MAX_BPF_ATTACH_TYPE];
 	bool disallow_override[MAX_BPF_ATTACH_TYPE];
+	bool is_recursive[MAX_BPF_ATTACH_TYPE];
 };
 
 void cgroup_bpf_put(struct cgroup *cgrp);
@@ -30,18 +31,19 @@ void cgroup_bpf_inherit(struct cgroup *cgrp, struct cgroup *parent);
 
 int __cgroup_bpf_update(struct cgroup *cgrp, struct cgroup *parent,
 			struct bpf_prog *prog, enum bpf_attach_type type,
-			bool overridable);
+			u32 flags);
 
 /* Wrapper for __cgroup_bpf_update() protected by cgroup_mutex */
 int cgroup_bpf_update(struct cgroup *cgrp, struct bpf_prog *prog,
-		      enum bpf_attach_type type, bool overridable);
+		      enum bpf_attach_type type, u32 flags);
 
 int __cgroup_bpf_run_filter_skb(struct sock *sk,
 				struct sk_buff *skb,
 				enum bpf_attach_type type);
 
-int __cgroup_bpf_run_filter_sk(struct sock *sk,
+int __cgroup_bpf_run_filter_sk(struct cgroup *cgrp, struct sock *sk,
 			       enum bpf_attach_type type);
+int cgroup_bpf_run_filter_sk(struct sock *sk, enum bpf_attach_type type);
 
 int __cgroup_bpf_run_filter_sock_ops(struct sock *sk,
 				     struct bpf_sock_ops_kern *sock_ops,
@@ -74,7 +76,7 @@ int __cgroup_bpf_run_filter_sock_ops(struct sock *sk,
 ({									       \
 	int __ret = 0;							       \
 	if (cgroup_bpf_enabled && sk) {					       \
-		__ret = __cgroup_bpf_run_filter_sk(sk,			       \
+		__ret = cgroup_bpf_run_filter_sk(sk,			       \
 						 BPF_CGROUP_INET_SOCK_CREATE); \
 	}								       \
 	__ret;								       \
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index f71f5e07d82d..595e31b30f23 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -151,6 +151,15 @@ enum bpf_attach_type {
  */
 #define BPF_F_ALLOW_OVERRIDE	(1U << 0)
 
+/* If BPF_F_RECURSIVE flag is used in BPF_PROG_ATTACH command
+ * cgroups are walked recursively back to the root cgroup or the
+ * first cgroup without the flag set running any program attached.
+ * Once the flag is set, it MUST be set for all descendant cgroups.
+ */
+#define BPF_F_RECURSIVE		(1U << 1)
+
+#define BPF_F_ALL_ATTACH_FLAGS  (BPF_F_ALLOW_OVERRIDE | BPF_F_RECURSIVE)
+
 /* If BPF_F_STRICT_ALIGNMENT is used in BPF_PROG_LOAD command, the
  * verifier will perform strict alignment checking as if the kernel
  * has been built with CONFIG_EFFICIENT_UNALIGNED_ACCESS not set,
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index 546113430049..eb1f436c18fb 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -47,10 +47,16 @@ void cgroup_bpf_inherit(struct cgroup *cgrp, struct cgroup *parent)
 	unsigned int type;
 
 	for (type = 0; type < ARRAY_SIZE(cgrp->bpf.effective); type++) {
-		struct bpf_prog *e;
+		struct bpf_prog *e = NULL;
+
+		/* do not need to set effective program if cgroups are
+		 * walked recursively
+		 */
+		cgrp->bpf.is_recursive[type] = parent->bpf.is_recursive[type];
+		if (!cgrp->bpf.is_recursive[type])
+			e = rcu_dereference_protected(parent->bpf.effective[type],
+						      lockdep_is_held(&cgroup_mutex));
 
-		e = rcu_dereference_protected(parent->bpf.effective[type],
-					      lockdep_is_held(&cgroup_mutex));
 		rcu_assign_pointer(cgrp->bpf.effective[type], e);
 		cgrp->bpf.disallow_override[type] = parent->bpf.disallow_override[type];
 	}
@@ -85,8 +91,12 @@ void cgroup_bpf_inherit(struct cgroup *cgrp, struct cgroup *parent)
  */
 int __cgroup_bpf_update(struct cgroup *cgrp, struct cgroup *parent,
 			struct bpf_prog *prog, enum bpf_attach_type type,
-			bool new_overridable)
+			u32 flags)
 {
+	bool new_overridable = flags & BPF_F_ALLOW_OVERRIDE;
+	/* initial state inherited from parent */
+	bool curr_recursive = cgrp->bpf.is_recursive[type];
+	bool new_recursive = flags & BPF_F_RECURSIVE;
 	struct bpf_prog *old_prog, *effective = NULL;
 	struct cgroup_subsys_state *pos;
 	bool overridable = true;
@@ -109,6 +119,12 @@ int __cgroup_bpf_update(struct cgroup *cgrp, struct cgroup *parent,
 		 */
 		return -EPERM;
 
+	if (prog && curr_recursive && !new_recursive)
+		/* if a parent has recursive prog attached, only
+		 * allow recursive programs in descendent cgroup
+		 */
+		return -EINVAL;
+
 	old_prog = cgrp->bpf.prog[type];
 
 	if (prog) {
@@ -139,6 +155,7 @@ int __cgroup_bpf_update(struct cgroup *cgrp, struct cgroup *parent,
 			rcu_assign_pointer(desc->bpf.effective[type],
 					   effective);
 			desc->bpf.disallow_override[type] = !overridable;
+			desc->bpf.is_recursive[type] = new_recursive;
 		}
 	}
 
@@ -217,14 +234,12 @@ EXPORT_SYMBOL(__cgroup_bpf_run_filter_skb);
  * This function will return %-EPERM if any if an attached program was found
  * and if it returned != 1 during execution. In all other cases, 0 is returned.
  */
-int __cgroup_bpf_run_filter_sk(struct sock *sk,
+int __cgroup_bpf_run_filter_sk(struct cgroup *cgrp, struct sock *sk,
 			       enum bpf_attach_type type)
 {
-	struct cgroup *cgrp = sock_cgroup_ptr(&sk->sk_cgrp_data);
 	struct bpf_prog *prog;
 	int ret = 0;
 
-
 	rcu_read_lock();
 
 	prog = rcu_dereference(cgrp->bpf.effective[type]);
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index d5774a6851f1..a1ab5dbaae89 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1187,7 +1187,7 @@ static int bpf_prog_attach(const union bpf_attr *attr)
 	if (CHECK_ATTR(BPF_PROG_ATTACH))
 		return -EINVAL;
 
-	if (attr->attach_flags & ~BPF_F_ALLOW_OVERRIDE)
+	if (attr->attach_flags & ~BPF_F_ALL_ATTACH_FLAGS)
 		return -EINVAL;
 
 	switch (attr->attach_type) {
@@ -1222,7 +1222,7 @@ static int bpf_prog_attach(const union bpf_attr *attr)
 	}
 
 	ret = cgroup_bpf_update(cgrp, prog, attr->attach_type,
-				attr->attach_flags & BPF_F_ALLOW_OVERRIDE);
+				attr->attach_flags);
 	if (ret)
 		bpf_prog_put(prog);
 	cgroup_put(cgrp);
@@ -1252,7 +1252,7 @@ static int bpf_prog_detach(const union bpf_attr *attr)
 		if (IS_ERR(cgrp))
 			return PTR_ERR(cgrp);
 
-		ret = cgroup_bpf_update(cgrp, NULL, attr->attach_type, false);
+		ret = cgroup_bpf_update(cgrp, NULL, attr->attach_type, 0);
 		cgroup_put(cgrp);
 		break;
 
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index df2e0f14a95d..27a4f14435a3 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -5176,14 +5176,35 @@ void cgroup_sk_free(struct sock_cgroup_data *skcd)
 
 #ifdef CONFIG_CGROUP_BPF
 int cgroup_bpf_update(struct cgroup *cgrp, struct bpf_prog *prog,
-		      enum bpf_attach_type type, bool overridable)
+		      enum bpf_attach_type type, u32 flags)
 {
 	struct cgroup *parent = cgroup_parent(cgrp);
 	int ret;
 
 	mutex_lock(&cgroup_mutex);
-	ret = __cgroup_bpf_update(cgrp, parent, prog, type, overridable);
+	ret = __cgroup_bpf_update(cgrp, parent, prog, type, flags);
 	mutex_unlock(&cgroup_mutex);
 	return ret;
 }
+
+int cgroup_bpf_run_filter_sk(struct sock *sk,
+			     enum bpf_attach_type type)
+{
+	struct cgroup *cgrp = sock_cgroup_ptr(&sk->sk_cgrp_data);
+	int ret = 0;
+
+	while (cgrp) {
+		ret = __cgroup_bpf_run_filter_sk(cgrp, sk, type);
+		if (ret)
+			break;
+
+		if (!cgrp->bpf.is_recursive[type])
+			break;
+
+		cgrp = cgroup_parent(cgrp);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(cgroup_bpf_run_filter_sk);
 #endif /* CONFIG_CGROUP_BPF */
-- 
2.1.4

Re: [PATCH v2 net-next 1/8] bpf: Add support for recursively running cgroup sock filters

[Daniel Borkmann]

On 08/25/2017 09:05 PM, David Ahern wrote:
> Add support for recursively applying sock filters attached to a cgroup.
> For now, start with the inner cgroup attached to the socket and work back
> to the root or first cgroup without the recursive flag set. Once the
> recursive flag is set for a cgroup all descendant group's must have the
> flag as well.
>
> Signed-off-by: David Ahern <dsahern@gmail.com>
[...]
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index f71f5e07d82d..595e31b30f23 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -151,6 +151,15 @@ enum bpf_attach_type {
>    */
>   #define BPF_F_ALLOW_OVERRIDE	(1U << 0)
>
> +/* If BPF_F_RECURSIVE flag is used in BPF_PROG_ATTACH command
> + * cgroups are walked recursively back to the root cgroup or the
> + * first cgroup without the flag set running any program attached.
> + * Once the flag is set, it MUST be set for all descendant cgroups.
> + */
> +#define BPF_F_RECURSIVE		(1U << 1)
> +
> +#define BPF_F_ALL_ATTACH_FLAGS  (BPF_F_ALLOW_OVERRIDE | BPF_F_RECURSIVE)
> +
>   /* If BPF_F_STRICT_ALIGNMENT is used in BPF_PROG_LOAD command, the
>    * verifier will perform strict alignment checking as if the kernel
>    * has been built with CONFIG_EFFICIENT_UNALIGNED_ACCESS not set,
> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index 546113430049..eb1f436c18fb 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -47,10 +47,16 @@ void cgroup_bpf_inherit(struct cgroup *cgrp, struct cgroup *parent)
>   	unsigned int type;
>
>   	for (type = 0; type < ARRAY_SIZE(cgrp->bpf.effective); type++) {
> -		struct bpf_prog *e;
> +		struct bpf_prog *e = NULL;
> +
> +		/* do not need to set effective program if cgroups are
> +		 * walked recursively
> +		 */
> +		cgrp->bpf.is_recursive[type] = parent->bpf.is_recursive[type];
> +		if (!cgrp->bpf.is_recursive[type])
> +			e = rcu_dereference_protected(parent->bpf.effective[type],
> +						      lockdep_is_held(&cgroup_mutex));

[...]

> -		e = rcu_dereference_protected(parent->bpf.effective[type],
> -					      lockdep_is_held(&cgroup_mutex));
>   		rcu_assign_pointer(cgrp->bpf.effective[type], e);
>   		cgrp->bpf.disallow_override[type] = parent->bpf.disallow_override[type];
>   	}
[...]
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index d5774a6851f1..a1ab5dbaae89 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -1187,7 +1187,7 @@ static int bpf_prog_attach(const union bpf_attr *attr)
>   	if (CHECK_ATTR(BPF_PROG_ATTACH))
>   		return -EINVAL;
>
> -	if (attr->attach_flags & ~BPF_F_ALLOW_OVERRIDE)
> +	if (attr->attach_flags & ~BPF_F_ALL_ATTACH_FLAGS)
>   		return -EINVAL;
>
>   	switch (attr->attach_type) {
> @@ -1222,7 +1222,7 @@ static int bpf_prog_attach(const union bpf_attr *attr)
>   	}
>
>   	ret = cgroup_bpf_update(cgrp, prog, attr->attach_type,
> -				attr->attach_flags & BPF_F_ALLOW_OVERRIDE);
> +				attr->attach_flags);
>   	if (ret)
>   		bpf_prog_put(prog);
>   	cgroup_put(cgrp);
> @@ -1252,7 +1252,7 @@ static int bpf_prog_detach(const union bpf_attr *attr)
>   		if (IS_ERR(cgrp))
>   			return PTR_ERR(cgrp);
>
> -		ret = cgroup_bpf_update(cgrp, NULL, attr->attach_type, false);
> +		ret = cgroup_bpf_update(cgrp, NULL, attr->attach_type, 0);
>   		cgroup_put(cgrp);
>   		break;

Can you elaborate on the semantical changes for the programs
setting the new flag which are not using below cgroup_bpf_run_filter_sk()
helper to walk back to root?

> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> index df2e0f14a95d..27a4f14435a3 100644
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -5176,14 +5176,35 @@ void cgroup_sk_free(struct sock_cgroup_data *skcd)
>
>   #ifdef CONFIG_CGROUP_BPF
>   int cgroup_bpf_update(struct cgroup *cgrp, struct bpf_prog *prog,
> -		      enum bpf_attach_type type, bool overridable)
> +		      enum bpf_attach_type type, u32 flags)
>   {
>   	struct cgroup *parent = cgroup_parent(cgrp);
>   	int ret;
>
>   	mutex_lock(&cgroup_mutex);
> -	ret = __cgroup_bpf_update(cgrp, parent, prog, type, overridable);
> +	ret = __cgroup_bpf_update(cgrp, parent, prog, type, flags);
>   	mutex_unlock(&cgroup_mutex);
>   	return ret;
>   }
> +
> +int cgroup_bpf_run_filter_sk(struct sock *sk,
> +			     enum bpf_attach_type type)
> +{
> +	struct cgroup *cgrp = sock_cgroup_ptr(&sk->sk_cgrp_data);
> +	int ret = 0;
> +
> +	while (cgrp) {
> +		ret = __cgroup_bpf_run_filter_sk(cgrp, sk, type);
> +		if (ret)
> +			break;
> +
> +		if (!cgrp->bpf.is_recursive[type])
> +			break;
> +
> +		cgrp = cgroup_parent(cgrp);
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(cgroup_bpf_run_filter_sk);
>   #endif /* CONFIG_CGROUP_BPF */
>

Re: [PATCH v2 net-next 1/8] bpf: Add support for recursively running cgroup sock filters

[David Ahern]

On 8/25/17 8:00 PM, Daniel Borkmann wrote:
> Can you elaborate on the semantical changes for the programs
> setting the new flag which are not using below cgroup_bpf_run_filter_sk()
> helper to walk back to root?

You mean other cgroup based programs -- BPF_CGROUP_* ? If so, any reason
not to allow the recursion model on those too?

Re: [PATCH v2 net-next 1/8] bpf: Add support for recursively running cgroup sock filters

[Alexei Starovoitov]

On Fri, Aug 25, 2017 at 12:05:34PM -0700, David Ahern wrote:
> Add support for recursively applying sock filters attached to a cgroup.
> For now, start with the inner cgroup attached to the socket and work back
> to the root or first cgroup without the recursive flag set. Once the
> recursive flag is set for a cgroup all descendant group's must have the
> flag as well.
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>
> ---
>  include/linux/bpf-cgroup.h | 10 ++++++----
>  include/uapi/linux/bpf.h   |  9 +++++++++
>  kernel/bpf/cgroup.c        | 29 ++++++++++++++++++++++-------
>  kernel/bpf/syscall.c       |  6 +++---
>  kernel/cgroup/cgroup.c     | 25 +++++++++++++++++++++++--
>  5 files changed, 63 insertions(+), 16 deletions(-)
> 
> diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
> index d41d40ac3efd..2d02187f242f 100644
> --- a/include/linux/bpf-cgroup.h
> +++ b/include/linux/bpf-cgroup.h
> @@ -23,6 +23,7 @@ struct cgroup_bpf {
>  	struct bpf_prog *prog[MAX_BPF_ATTACH_TYPE];
>  	struct bpf_prog __rcu *effective[MAX_BPF_ATTACH_TYPE];
>  	bool disallow_override[MAX_BPF_ATTACH_TYPE];
> +	bool is_recursive[MAX_BPF_ATTACH_TYPE];
>  };
>  
>  void cgroup_bpf_put(struct cgroup *cgrp);
> @@ -30,18 +31,19 @@ void cgroup_bpf_inherit(struct cgroup *cgrp, struct cgroup *parent);
>  
>  int __cgroup_bpf_update(struct cgroup *cgrp, struct cgroup *parent,
>  			struct bpf_prog *prog, enum bpf_attach_type type,
> -			bool overridable);
> +			u32 flags);
>  
>  /* Wrapper for __cgroup_bpf_update() protected by cgroup_mutex */
>  int cgroup_bpf_update(struct cgroup *cgrp, struct bpf_prog *prog,
> -		      enum bpf_attach_type type, bool overridable);
> +		      enum bpf_attach_type type, u32 flags);
>  
>  int __cgroup_bpf_run_filter_skb(struct sock *sk,
>  				struct sk_buff *skb,
>  				enum bpf_attach_type type);
>  
> -int __cgroup_bpf_run_filter_sk(struct sock *sk,
> +int __cgroup_bpf_run_filter_sk(struct cgroup *cgrp, struct sock *sk,
>  			       enum bpf_attach_type type);
> +int cgroup_bpf_run_filter_sk(struct sock *sk, enum bpf_attach_type type);
>  
>  int __cgroup_bpf_run_filter_sock_ops(struct sock *sk,
>  				     struct bpf_sock_ops_kern *sock_ops,
> @@ -74,7 +76,7 @@ int __cgroup_bpf_run_filter_sock_ops(struct sock *sk,
>  ({									       \
>  	int __ret = 0;							       \
>  	if (cgroup_bpf_enabled && sk) {					       \
> -		__ret = __cgroup_bpf_run_filter_sk(sk,			       \
> +		__ret = cgroup_bpf_run_filter_sk(sk,			       \
>  						 BPF_CGROUP_INET_SOCK_CREATE); \
>  	}								       \
>  	__ret;								       \
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index f71f5e07d82d..595e31b30f23 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -151,6 +151,15 @@ enum bpf_attach_type {
>   */
>  #define BPF_F_ALLOW_OVERRIDE	(1U << 0)
>  
> +/* If BPF_F_RECURSIVE flag is used in BPF_PROG_ATTACH command
> + * cgroups are walked recursively back to the root cgroup or the
> + * first cgroup without the flag set running any program attached.
> + * Once the flag is set, it MUST be set for all descendant cgroups.
> + */
> +#define BPF_F_RECURSIVE		(1U << 1)

above logic makes sense, but ...

> +	if (prog && curr_recursive && !new_recursive)
> +		/* if a parent has recursive prog attached, only
> +		 * allow recursive programs in descendent cgroup
> +		 */
> +		return -EINVAL;
> +
>  	old_prog = cgrp->bpf.prog[type];

... I'm struggling to completely understand how it interacts
with BPF_F_ALLOW_OVERRIDE.
By default we shouldn't allow overriding, so if default prog attached
to a root, what happens if we try to attach F_RECURSIVE to a descendent?
If I'm reading the code correctly it will not succeed, which is good.
Could you add such scenario as test to test_cgrp2_attach2.c ?

Now say we attach overridable and !recursive to a root, another
recursive prog will not be attached to a descedent, which is correct.

But if we attach !overridable + recursive to a root we cannot attach
anything to a descendent right? Then why allow such combination at all?
So only overridable + recursive combination makes sense, right?

I think all these combinations must be documented and tests must be
added. Sooner or later people will build security sensitive environment
with it and we have to meticulous now.

Do you think it would make sense to split this patch out and
push patches 2 and 3 with few tests in parallel, while we're review
this change?

Tejun needs to take a deep look into this patch as well.

Re: [PATCH v2 net-next 1/8] bpf: Add support for recursively running cgroup sock filters

[David Ahern]

On 8/25/17 8:49 PM, Alexei Starovoitov wrote:
> 
>> +	if (prog && curr_recursive && !new_recursive)
>> +		/* if a parent has recursive prog attached, only
>> +		 * allow recursive programs in descendent cgroup
>> +		 */
>> +		return -EINVAL;
>> +
>>  	old_prog = cgrp->bpf.prog[type];
> 
> ... I'm struggling to completely understand how it interacts
> with BPF_F_ALLOW_OVERRIDE.

The 2 flags are completely independent. The existing override logic is
unchanged. If a program can not be overridden, then the new recursive
flag is irrelevant.

> By default we shouldn't allow overriding, so if default prog attached
> to a root, what happens if we try to attach F_RECURSIVE to a descendent?
> If I'm reading the code correctly it will not succeed, which is good.
> Could you add such scenario as test to test_cgrp2_attach2.c ?

Patch 7 adds test cases to cover scenarios. I will add more tests per
comments below and rename to convey it tests the recursive flag.

> 
> Now say we attach overridable and !recursive to a root, another
> recursive prog will not be attached to a descedent, which is correct.

yes

> 
> But if we attach !overridable + recursive to a root we cannot attach
> anything to a descendent right? Then why allow such combination at all?

Sure, we can not allow that combination to prevent the inefficiency of
recursively running through cgroups to run the base program.

> So only overridable + recursive combination makes sense, right?
> 
> I think all these combinations must be documented and tests must be
> added. Sooner or later people will build security sensitive environment
> with it and we have to meticulous now.

Intentions below. I'll add more test cases to verify intentions agree
with code.

> 
> Do you think it would make sense to split this patch out and
> push patches 2 and 3 with few tests in parallel, while we're review
> this change?

I thought about that but decided no. The 'ip vrf exec' use case would
break right of the gate if the other settings were used.

> 
> Tejun needs to take a deep look into this patch as well.
> 

This is the intended behavior:

The override flag is independent of the recursive flag. If the override
flag does not allow an override, the attempt to add a new program fails.
The recursive flag brings an additional constraint: once a cgroup has a
program with the recursive flag set it is inherited by all descendant
groups. Attempts to insert a program that changes that flag fails EINVAL.

Start with the root group at $MNT. No program is attached. By default
override is allowed and recursive is not set.

1. Group $MNT/a is created.

i. Default settings from $MNT are inherited; 'a' has override enabled
and recursive disabled.

ii. Program is attached. Override flag is set, recursive flag is not set.

iii. Process in 'a' opens a socket, program attached to 'a' is run.


2. $MNT/a/b is created

i. 'b' inherits the program and settings of 'a' (override enabled,
recursive disabled).

ii. Process in 'b' opens a socket. Program inherited from 'a' is run.

iii. Non-interesting case for this patch set: attaching a non-recursive
program to 'b' overrides the inherited one. process opens a socket only
the 'b' program is run.

iv. Program is attached to 'b', override flag set, recursive flag set.

v. Process in 'b' opens a socket. Program attached to 'b' is run and
then program from 'a' is run. Recursion stops here since 'a' does not
have the recursion flag set.


3. $MNT/a/b/c is created

i. 'c' inherits the settings of 'b' (override is allowed, recursive flag
is set)

ii. Process in 'c' opens a socket. No program from 'c' exists, so
nothing is run. Recursion flag is set, so program from 'b' is run, then
program from 'a' is run. Stop (recursive flag not set on 'a').

iii. Attaching a non-recursive program to 'c' fails because it inherited
the recursive flag from 'b' and that can not be reset by a descendant.

iv. Recursive program is attached to 'c'

v. Process in 'c' opens a socket. Program attached to 'c' is run, then
the program from 'b' and the program from 'a'. Stop.

etc.

To consider what happens on doubling back and changing programs in the
hierarchy, start with $MNT/a/b/c from 3 above (non-recursive on 'a',
recursive on 'b' and recursive on 'c') for each of the following cases:

1. Program attached to 'b' is detached, recursive flag is reset in the
request. Attempt fails EINVAL because the recursion flag has to be set.


2. Program attached to 'b' is detached, recursive flag is set. Allowed.

Process in 'b' opens a socket. No program attached to 'b' so no program
is run. Recursive flag is set to program from 'a' is run. Stop.

We should allow the recursive flag to be reset if the parent is not
recursive allowing an unwind of settings applied. I'll add that change.

Re: [PATCH v2 net-next 1/8] bpf: Add support for recursively running cgroup sock filters

[Alexei Starovoitov]

On Sun, Aug 27, 2017 at 08:49:23AM -0600, David Ahern wrote:
> 
> The override flag is independent of the recursive flag. If the override
> flag does not allow an override, the attempt to add a new program fails.
> The recursive flag brings an additional constraint: once a cgroup has a
> program with the recursive flag set it is inherited by all descendant
> groups. Attempts to insert a program that changes that flag fails EINVAL.
> 
> Start with the root group at $MNT. No program is attached. By default
> override is allowed and recursive is not set.

The above explanation is the reason we need tests for this logic.
The default is the opposite! By default override is _not_ allowed.

> 1. Group $MNT/a is created.
> 
> i. Default settings from $MNT are inherited; 'a' has override enabled
> and recursive disabled.

not true, but say the user attached a prog wih override on...

> ii. Program is attached. Override flag is set, recursive flag is not set.
> 
> iii. Process in 'a' opens a socket, program attached to 'a' is run.
> 
> 
> 2. $MNT/a/b is created
> 
> i. 'b' inherits the program and settings of 'a' (override enabled,
> recursive disabled).
> 
> ii. Process in 'b' opens a socket. Program inherited from 'a' is run.
> 
> iii. Non-interesting case for this patch set: attaching a non-recursive
> program to 'b' overrides the inherited one. process opens a socket only
> the 'b' program is run.
>
> iv. Program is attached to 'b', override flag set, recursive flag set.
> 
> v. Process in 'b' opens a socket. Program attached to 'b' is run and
> then program from 'a' is run. Recursion stops here since 'a' does not
> have the recursion flag set.

isn't this the problem? Override+non_recurse was set on 'a'.
Now we attached override+recurse on 'b' and suddenly 'a'
will be run like it was 'recursive'?
imo that is counter intuitive to the owner of 'a'.
I think there can be two options:
- if recurse is not set on 'a', all of it descendents should not be allowed
  to use recurse flag
- if recurse is not set on 'a', it should not be run

imo the former is cleaner and avoids issues with detach in the middle

> 3. $MNT/a/b/c is created
> 
> i. 'c' inherits the settings of 'b' (override is allowed, recursive flag
> is set)
> 
> ii. Process in 'c' opens a socket. No program from 'c' exists, so
> nothing is run. Recursion flag is set, so program from 'b' is run, then
> program from 'a' is run. Stop (recursive flag not set on 'a').

also doesn't make sense to me. Both 'b' and 'c' were attached as
override+recurse while 'a' as non-recurse why would it run?
The owner of 'a' attached it as override in the first place,
so it assumed that if descendent wants to override it it can
and the prog 'a' won't be running.

> iii. Attaching a non-recursive program to 'c' fails because it inherited
> the recursive flag from 'b' and that can not be reset by a descendant.

that part makes sense

> iv. Recursive program is attached to 'c'
> 
> v. Process in 'c' opens a socket. Program attached to 'c' is run, then
> the program from 'b' and the program from 'a'. Stop.
> 
> etc.
> 
> To consider what happens on doubling back and changing programs in the
> hierarchy, start with $MNT/a/b/c from 3 above (non-recursive on 'a',
> recursive on 'b' and recursive on 'c') for each of the following cases:
> 
> 1. Program attached to 'b' is detached, recursive flag is reset in the
> request. Attempt fails EINVAL because the recursion flag has to be set.

didn't get this point. you mean 'detach' will fail?

> 2. Program attached to 'b' is detached, recursive flag is set. Allowed.

meaing that detach from 'b' has to pass recurse flag to be detached?
That's also odd.
imo detach should always succeed and the process doing detach
shouldn't need to know what flags were used in attach.

> Process in 'b' opens a socket. No program attached to 'b' so no program
> is run. Recursive flag is set to program from 'a' is run. Stop.
> 
> We should allow the recursive flag to be reset if the parent is not
> recursive allowing an unwind of settings applied. I'll add that change.

I don't get this part.
Anyway looking forward to the next patch set with tests and comments like above.

Also adding Andy to cc. I'd really like both Andy and Tejun to review this logic.

Re: [PATCH v2 net-next 1/8] bpf: Add support for recursively running cgroup sock filters

[David Ahern]

On 8/28/17 5:56 PM, Alexei Starovoitov wrote:
> On Sun, Aug 27, 2017 at 08:49:23AM -0600, David Ahern wrote:
>>
>> The override flag is independent of the recursive flag. If the override
>> flag does not allow an override, the attempt to add a new program fails.
>> The recursive flag brings an additional constraint: once a cgroup has a
>> program with the recursive flag set it is inherited by all descendant
>> groups. Attempts to insert a program that changes that flag fails EINVAL.
>>
>> Start with the root group at $MNT. No program is attached. By default
>> override is allowed and recursive is not set.
> 
> The above explanation is the reason we need tests for this logic.
> The default is the opposite! By default override is _not_ allowed.

yes, I got that backwards. It's how my programs work since I always add
the OVERRIDE flag ATM.

> 
>> 1. Group $MNT/a is created.
>>
>> i. Default settings from $MNT are inherited; 'a' has override enabled
>> and recursive disabled.
> 
> not true, but say the user attached a prog wih override on...

exactly.

> 
>> ii. Program is attached. Override flag is set, recursive flag is not set.
>>
>> iii. Process in 'a' opens a socket, program attached to 'a' is run.
>>
>>
>> 2. $MNT/a/b is created
>>
>> i. 'b' inherits the program and settings of 'a' (override enabled,
>> recursive disabled).
>>
>> ii. Process in 'b' opens a socket. Program inherited from 'a' is run.
>>
>> iii. Non-interesting case for this patch set: attaching a non-recursive
>> program to 'b' overrides the inherited one. process opens a socket only
>> the 'b' program is run.
>>
>> iv. Program is attached to 'b', override flag set, recursive flag set.
>>
>> v. Process in 'b' opens a socket. Program attached to 'b' is run and
>> then program from 'a' is run. Recursion stops here since 'a' does not
>> have the recursion flag set.
> 
> isn't this the problem? Override+non_recurse was set on 'a'.
> Now we attached override+recurse on 'b' and suddenly 'a'
> will be run like it was 'recursive'?

Without 'b', 'a' is run. With 'b' and the recurse flag I am suggesting
the user is saying I want 'b' and then 'a'. ie., the first parent group
without the flag is where we stop.

> imo that is counter intuitive to the owner of 'a'.
> I think there can be two options:
> - if recurse is not set on 'a', all of it descendents should not be allowed
>   to use recurse flag
> - if recurse is not set on 'a', it should not be run

If 'a' can be overridden then I don't agree with the first suggestion.
Which means you are suggesting we stop at the last group with the
recursive flag set. That is fine too.

> 
> imo the former is cleaner and avoids issues with detach in the middle
> 
>> 3. $MNT/a/b/c is created
>>
>> i. 'c' inherits the settings of 'b' (override is allowed, recursive flag
>> is set)
>>
>> ii. Process in 'c' opens a socket. No program from 'c' exists, so
>> nothing is run. Recursion flag is set, so program from 'b' is run, then
>> program from 'a' is run. Stop (recursive flag not set on 'a').
> 
> also doesn't make sense to me. Both 'b' and 'c' were attached as
> override+recurse while 'a' as non-recurse why would it run?
> The owner of 'a' attached it as override in the first place,
> so it assumed that if descendent wants to override it it can
> and the prog 'a' won't be running.

same argument as above - the question is where do we stop. My suggestion
and implementation is stopping at the first group without the flag.

As stated all along, descendant groups inherit programs of the parent
marked recurse - not by copying them into the group but by recursion.


> 
>> iii. Attaching a non-recursive program to 'c' fails because it inherited
>> the recursive flag from 'b' and that can not be reset by a descendant.
> 
> that part makes sense
> 
>> iv. Recursive program is attached to 'c'
>>
>> v. Process in 'c' opens a socket. Program attached to 'c' is run, then
>> the program from 'b' and the program from 'a'. Stop.
>>
>> etc.
>>
>> To consider what happens on doubling back and changing programs in the
>> hierarchy, start with $MNT/a/b/c from 3 above (non-recursive on 'a',
>> recursive on 'b' and recursive on 'c') for each of the following cases:
>>
>> 1. Program attached to 'b' is detached, recursive flag is reset in the
>> request. Attempt fails EINVAL because the recursion flag has to be set.
> 
> didn't get this point. you mean 'detach' will fail?

Yes, because it tried to reset the flag in the process.

This is something we can make user friendly - the detach succeeds, but
the recurse flag is ignored and the recurse flag in the group is not
reset unless it is the base group with the recurse flag (i.e., the
parent is marked non-recurse).


> 
>> 2. Program attached to 'b' is detached, recursive flag is set. Allowed.
> 
> meaing that detach from 'b' has to pass recurse flag to be detached?
> That's also odd.
> imo detach should always succeed and the process doing detach
> shouldn't need to know what flags were used in attach.

Then, we agree to make it user friendly and handle resetting the recurse
flag automatically.

> 
>> Process in 'b' opens a socket. No program attached to 'b' so no program
>> is run. Recursive flag is set to program from 'a' is run. Stop.
>>
>> We should allow the recursive flag to be reset if the parent is not
>> recursive allowing an unwind of settings applied. I'll add that change.
> 
> I don't get this part.
> Anyway looking forward to the next patch set with tests and comments like above.

Per above discussion, you don't want 'a' run since it is not marked
recurse. My last sentence is the user friendly part in resetting the
flag in the cgroup.

Re: [PATCH v2 net-next 1/8] bpf: Add support for recursively running cgroup sock filters

[Alexei Starovoitov]

On Mon, Aug 28, 2017 at 06:43:42PM -0600, David Ahern wrote:
> On 8/28/17 5:56 PM, Alexei Starovoitov wrote:
> > On Sun, Aug 27, 2017 at 08:49:23AM -0600, David Ahern wrote:
> >>
> >> The override flag is independent of the recursive flag. If the override
> >> flag does not allow an override, the attempt to add a new program fails.
> >> The recursive flag brings an additional constraint: once a cgroup has a
> >> program with the recursive flag set it is inherited by all descendant
> >> groups. Attempts to insert a program that changes that flag fails EINVAL.
> >>
> >> Start with the root group at $MNT. No program is attached. By default
> >> override is allowed and recursive is not set.
> > 
> > The above explanation is the reason we need tests for this logic.
> > The default is the opposite! By default override is _not_ allowed.
> 
> yes, I got that backwards. It's how my programs work since I always add
> the OVERRIDE flag ATM.
> 
> > 
> >> 1. Group $MNT/a is created.
> >>
> >> i. Default settings from $MNT are inherited; 'a' has override enabled
> >> and recursive disabled.
> > 
> > not true, but say the user attached a prog wih override on...
> 
> exactly.
> 
> > 
> >> ii. Program is attached. Override flag is set, recursive flag is not set.
> >>
> >> iii. Process in 'a' opens a socket, program attached to 'a' is run.
> >>
> >>
> >> 2. $MNT/a/b is created
> >>
> >> i. 'b' inherits the program and settings of 'a' (override enabled,
> >> recursive disabled).
> >>
> >> ii. Process in 'b' opens a socket. Program inherited from 'a' is run.
> >>
> >> iii. Non-interesting case for this patch set: attaching a non-recursive
> >> program to 'b' overrides the inherited one. process opens a socket only
> >> the 'b' program is run.
> >>
> >> iv. Program is attached to 'b', override flag set, recursive flag set.
> >>
> >> v. Process in 'b' opens a socket. Program attached to 'b' is run and
> >> then program from 'a' is run. Recursion stops here since 'a' does not
> >> have the recursion flag set.
> > 
> > isn't this the problem? Override+non_recurse was set on 'a'.
> > Now we attached override+recurse on 'b' and suddenly 'a'
> > will be run like it was 'recursive'?
> 
> Without 'b', 'a' is run. With 'b' and the recurse flag I am suggesting
> the user is saying I want 'b' and then 'a'. ie., the first parent group
> without the flag is where we stop.
> 
> > imo that is counter intuitive to the owner of 'a'.
> > I think there can be two options:
> > - if recurse is not set on 'a', all of it descendents should not be allowed
> >   to use recurse flag
> > - if recurse is not set on 'a', it should not be run
> 
> If 'a' can be overridden then I don't agree with the first suggestion.
> Which means you are suggesting we stop at the last group with the
> recursive flag set. That is fine too.
> 
> > 
> > imo the former is cleaner and avoids issues with detach in the middle
> > 
> >> 3. $MNT/a/b/c is created
> >>
> >> i. 'c' inherits the settings of 'b' (override is allowed, recursive flag
> >> is set)
> >>
> >> ii. Process in 'c' opens a socket. No program from 'c' exists, so
> >> nothing is run. Recursion flag is set, so program from 'b' is run, then
> >> program from 'a' is run. Stop (recursive flag not set on 'a').
> > 
> > also doesn't make sense to me. Both 'b' and 'c' were attached as
> > override+recurse while 'a' as non-recurse why would it run?
> > The owner of 'a' attached it as override in the first place,
> > so it assumed that if descendent wants to override it it can
> > and the prog 'a' won't be running.
> 
> same argument as above - the question is where do we stop. My suggestion
> and implementation is stopping at the first group without the flag.
> 
> As stated all along, descendant groups inherit programs of the parent
> marked recurse - not by copying them into the group but by recursion.
> 
> 
> > 
> >> iii. Attaching a non-recursive program to 'c' fails because it inherited
> >> the recursive flag from 'b' and that can not be reset by a descendant.
> > 
> > that part makes sense
> > 
> >> iv. Recursive program is attached to 'c'
> >>
> >> v. Process in 'c' opens a socket. Program attached to 'c' is run, then
> >> the program from 'b' and the program from 'a'. Stop.
> >>
> >> etc.
> >>
> >> To consider what happens on doubling back and changing programs in the
> >> hierarchy, start with $MNT/a/b/c from 3 above (non-recursive on 'a',
> >> recursive on 'b' and recursive on 'c') for each of the following cases:
> >>
> >> 1. Program attached to 'b' is detached, recursive flag is reset in the
> >> request. Attempt fails EINVAL because the recursion flag has to be set.
> > 
> > didn't get this point. you mean 'detach' will fail?
> 
> Yes, because it tried to reset the flag in the process.
> 
> This is something we can make user friendly - the detach succeeds, but
> the recurse flag is ignored and the recurse flag in the group is not
> reset unless it is the base group with the recurse flag (i.e., the
> parent is marked non-recurse).

if we don't reset group flags to default it will be even more difficult
for users to use, since attach with recursive flag + immediate detach sets
some internal flag on the cgroup and user space has no way of
either querying this flag or deleting it.

> > 
> >> 2. Program attached to 'b' is detached, recursive flag is set. Allowed.
> > 
> > meaing that detach from 'b' has to pass recurse flag to be detached?
> > That's also odd.
> > imo detach should always succeed and the process doing detach
> > shouldn't need to know what flags were used in attach.
> 
> Then, we agree to make it user friendly and handle resetting the recurse
> flag automatically.

in that sense yes attach/delete pair should be side-effect free.

> >> Process in 'b' opens a socket. No program attached to 'b' so no program
> >> is run. Recursive flag is set to program from 'a' is run. Stop.
> >>
> >> We should allow the recursive flag to be reset if the parent is not
> >> recursive allowing an unwind of settings applied. I'll add that change.
> > 
> > I don't get this part.
> > Anyway looking forward to the next patch set with tests and comments like above.
> 
> Per above discussion, you don't want 'a' run since it is not marked
> recurse. My last sentence is the user friendly part in resetting the
> flag in the cgroup.

I'm still not grasping fully the semantics of what you're proposing.
You keep saying that override and recurse flags are indepedent, but
the more we talk the more it's clear that there is a complicated
relationship between them. Like no_override overrules everything, etc.
I'm looking for the simplest to use logic. Not implementation.
Implementation can be complex, but uapi should be as simple to
explain and as simple to understand as possible.
So how about allowing recurse+overide combination only?
All descendents must be recurse+override too and
no program allowed to be set on parent unless it's recurse+override
as well. Then detach anywhere is simple, since all programs in
such chain are always recurse+override.

Re: [PATCH v2 net-next 1/8] bpf: Add support for recursively running cgroup sock filters

[David Ahern]

On 8/28/17 7:12 PM, Alexei Starovoitov wrote:
>>>> To consider what happens on doubling back and changing programs in the
>>>> hierarchy, start with $MNT/a/b/c from 3 above (non-recursive on 'a',
>>>> recursive on 'b' and recursive on 'c') for each of the following cases:
>>>>
>>>> 1. Program attached to 'b' is detached, recursive flag is reset in the
>>>> request. Attempt fails EINVAL because the recursion flag has to be set.
>>>
>>> didn't get this point. you mean 'detach' will fail?
>>
>> Yes, because it tried to reset the flag in the process.
>>
>> This is something we can make user friendly - the detach succeeds, but
>> the recurse flag is ignored and the recurse flag in the group is not
>> reset unless it is the base group with the recurse flag (i.e., the
>> parent is marked non-recurse).
> 
> if we don't reset group flags to default it will be even more difficult
> for users to use, since attach with recursive flag + immediate detach sets
> some internal flag on the cgroup and user space has no way of
> either querying this flag or deleting it.

We have discussed this before -- the need to know which cgroup has a
program and now what is the status of flags. That need is a different
problem than this patch set.

I'll address the reset of the flags below to keep that discussion together.

> 
>>>
>>>> 2. Program attached to 'b' is detached, recursive flag is set. Allowed.
>>>
>>> meaing that detach from 'b' has to pass recurse flag to be detached?
>>> That's also odd.
>>> imo detach should always succeed and the process doing detach
>>> shouldn't need to know what flags were used in attach.
>>
>> Then, we agree to make it user friendly and handle resetting the recurse
>> flag automatically.
> 
> in that sense yes attach/delete pair should be side-effect free.
> 
>>>> Process in 'b' opens a socket. No program attached to 'b' so no program
>>>> is run. Recursive flag is set to program from 'a' is run. Stop.
>>>>
>>>> We should allow the recursive flag to be reset if the parent is not
>>>> recursive allowing an unwind of settings applied. I'll add that change.
>>>
>>> I don't get this part.
>>> Anyway looking forward to the next patch set with tests and comments like above.
>>
>> Per above discussion, you don't want 'a' run since it is not marked
>> recurse. My last sentence is the user friendly part in resetting the
>> flag in the cgroup.
> 
> I'm still not grasping fully the semantics of what you're proposing.
> You keep saying that override and recurse flags are indepedent, but
> the more we talk the more it's clear that there is a complicated
> relationship between them. Like no_override overrules everything, etc.

yes, I have said that a few times. Override should block everything in
terms of installing programs. If it is not enabled, the status of the
recurse flag is not relevant at attach / detach time as the call should
fail. So installing a program with the recurse flag only works if
override is allowed.


> I'm looking for the simplest to use logic. Not implementation.
> Implementation can be complex, but uapi should be as simple to
> explain and as simple to understand as possible.
> So how about allowing recurse+overide combination only?
> All descendents must be recurse+override too and
> no program allowed to be set on parent unless it's recurse+override
> as well. Then detach anywhere is simple, since all programs in
> such chain are always recurse+override.


Let's walk through examples based on the new ground rule - recursion
stops at last cgroup with flag set.

Assuming override is allowed ...

${MNT}/a/b/c/d

- 'a' has no program

- 'b' has a program, override allowed, recurse set

- 'c' and 'd' inherit the program from 'b' by recursion, not inheritance
(ie., bpf.effective is not updated with the program from 'b', but the
recurse flag is set on 'c' and 'd').

At this point 'c' and 'd' can ONLY take programs that are recursive.

- 'c' gets a program installed
- 'd' gets a program installed.


Process in 'd' has programs run in this order: 'd', 'c', 'b'


Now, program 'c' is detached. It is in the middle of the recursive set.
It MUST keep the recurse flag set as it inherited the restriction from
'b'. The recurse flag on 'c' can ONLY be reset when the program is
detached from 'b' as it is the start of the recursive chain.

I'll stop here to make sure we agree on the above. Considering all
permutations is a maze.

###

Also, let's agree on this intention. Based on the new ground rule, I
want to point out this example:

If 'a' gets a program installed with no recurse flag set, ONLY processes
in 'a' have the 'a' program run. Processes in groups 'b', 'c' and 'd'
all stop at cgroup 'b' program.

To me this is counterintuitive and why I said programs are run up to the
first parent without the recurse flag. They are all part of the same tree.

Re: [PATCH v2 net-next 1/8] bpf: Add support for recursively running cgroup sock filters

[Alexei Starovoitov]

On Mon, Aug 28, 2017 at 08:22:31PM -0600, David Ahern wrote:
> On 8/28/17 7:12 PM, Alexei Starovoitov wrote:
> >>>> To consider what happens on doubling back and changing programs in the
> >>>> hierarchy, start with $MNT/a/b/c from 3 above (non-recursive on 'a',
> >>>> recursive on 'b' and recursive on 'c') for each of the following cases:
> >>>>
> >>>> 1. Program attached to 'b' is detached, recursive flag is reset in the
> >>>> request. Attempt fails EINVAL because the recursion flag has to be set.
> >>>
> >>> didn't get this point. you mean 'detach' will fail?
> >>
> >> Yes, because it tried to reset the flag in the process.
> >>
> >> This is something we can make user friendly - the detach succeeds, but
> >> the recurse flag is ignored and the recurse flag in the group is not
> >> reset unless it is the base group with the recurse flag (i.e., the
> >> parent is marked non-recurse).
> > 
> > if we don't reset group flags to default it will be even more difficult
> > for users to use, since attach with recursive flag + immediate detach sets
> > some internal flag on the cgroup and user space has no way of
> > either querying this flag or deleting it.
> 
> We have discussed this before -- the need to know which cgroup has a
> program and now what is the status of flags. That need is a different
> problem than this patch set.
> 
> I'll address the reset of the flags below to keep that discussion together.
> 
> > 
> >>>
> >>>> 2. Program attached to 'b' is detached, recursive flag is set. Allowed.
> >>>
> >>> meaing that detach from 'b' has to pass recurse flag to be detached?
> >>> That's also odd.
> >>> imo detach should always succeed and the process doing detach
> >>> shouldn't need to know what flags were used in attach.
> >>
> >> Then, we agree to make it user friendly and handle resetting the recurse
> >> flag automatically.
> > 
> > in that sense yes attach/delete pair should be side-effect free.
> > 
> >>>> Process in 'b' opens a socket. No program attached to 'b' so no program
> >>>> is run. Recursive flag is set to program from 'a' is run. Stop.
> >>>>
> >>>> We should allow the recursive flag to be reset if the parent is not
> >>>> recursive allowing an unwind of settings applied. I'll add that change.
> >>>
> >>> I don't get this part.
> >>> Anyway looking forward to the next patch set with tests and comments like above.
> >>
> >> Per above discussion, you don't want 'a' run since it is not marked
> >> recurse. My last sentence is the user friendly part in resetting the
> >> flag in the cgroup.
> > 
> > I'm still not grasping fully the semantics of what you're proposing.
> > You keep saying that override and recurse flags are indepedent, but
> > the more we talk the more it's clear that there is a complicated
> > relationship between them. Like no_override overrules everything, etc.
> 
> yes, I have said that a few times. Override should block everything in
> terms of installing programs. If it is not enabled, the status of the
> recurse flag is not relevant at attach / detach time as the call should
> fail. So installing a program with the recurse flag only works if
> override is allowed.
> 
> 
> > I'm looking for the simplest to use logic. Not implementation.
> > Implementation can be complex, but uapi should be as simple to
> > explain and as simple to understand as possible.
> > So how about allowing recurse+overide combination only?
> > All descendents must be recurse+override too and
> > no program allowed to be set on parent unless it's recurse+override
> > as well. Then detach anywhere is simple, since all programs in
> > such chain are always recurse+override.
> 
> 
> Let's walk through examples based on the new ground rule - recursion
> stops at last cgroup with flag set.
> 
> Assuming override is allowed ...
> 
> ${MNT}/a/b/c/d
> 
> - 'a' has no program
> 
> - 'b' has a program, override allowed, recurse set
> 
> - 'c' and 'd' inherit the program from 'b' by recursion, not inheritance
> (ie., bpf.effective is not updated with the program from 'b', but the
> recurse flag is set on 'c' and 'd').
> 
> At this point 'c' and 'd' can ONLY take programs that are recursive.
> 
> - 'c' gets a program installed
> - 'd' gets a program installed.
> 
> 
> Process in 'd' has programs run in this order: 'd', 'c', 'b'
> 
> 
> Now, program 'c' is detached. It is in the middle of the recursive set.
> It MUST keep the recurse flag set as it inherited the restriction from
> 'b'. The recurse flag on 'c' can ONLY be reset when the program is
> detached from 'b' as it is the start of the recursive chain.
> 
> I'll stop here to make sure we agree on the above. Considering all
> permutations is a maze.

Agree on the above, but you're mixing semantics of the new recurse
flag and implementation of it. Ex: we don't have to copy this flag
from prog->attr into cgroup. So this reset or non-reset discussion
only makes sense in the context of your current implementation.
We can implement the logic differently. Like don't copy that flag
at all and at attach time walk parent->parent->parent and see
what programs are attached. All of them should have prog->attr & recurse_bit set
In such implementation detach from 'b' is a nop from reset/non-reset
point of view. When socket creation in 'c' is invoked the program
'c' is called first then the code keeps walking parents until root
invoking 'a' along the way.
I'm not saying it will be an efficient implementation. The point
is to discuss UAPI independent of implementation.

> ###
> 
> Also, let's agree on this intention. Based on the new ground rule, I
> want to point out this example:
> 
> If 'a' gets a program installed with no recurse flag set, ONLY processes
> in 'a' have the 'a' program run. Processes in groups 'b', 'c' and 'd'
> all stop at cgroup 'b' program.

I'm proposing that such situation should not be allowed to happen.
In a->b->c->d cgroup scenario if override+recurse prog attached to 'b'
then only the same override+recurse can be attached to c, d, a.
So at detach time there can be gaps (like only 'b' and 'd' have
override+recurse progs), but walking up until root from any point
will guarantee that only override+recurse programs are seen.

Re: [PATCH v2 net-next 1/8] bpf: Add support for recursively running cgroup sock filters

[David Ahern]

On 8/28/17 10:11 PM, Alexei Starovoitov wrote:
> 
> Agree on the above, but you're mixing semantics of the new recurse
> flag and implementation of it. Ex: we don't have to copy this flag
> from prog->attr into cgroup. So this reset or non-reset discussion
> only makes sense in the context of your current implementation.
> We can implement the logic differently. Like don't copy that flag
> at all and at attach time walk parent->parent->parent and see
> what programs are attached. All of them should have prog->attr & recurse_bit set
> In such implementation detach from 'b' is a nop from reset/non-reset
> point of view. When socket creation in 'c' is invoked the program
> 'c' is called first then the code keeps walking parents until root
> invoking 'a' along the way.

So you are suggesting there is no recursive flag per cgroup? How do you
know you need to walk cgroups? How do you know when to stop running
programs?

> I'm not saying it will be an efficient implementation. The point
> is to discuss UAPI independent of implementation.
> 
>> ###
>>
>> Also, let's agree on this intention. Based on the new ground rule, I
>> want to point out this example:
>>
>> If 'a' gets a program installed with no recurse flag set, ONLY processes
>> in 'a' have the 'a' program run. Processes in groups 'b', 'c' and 'd'
>> all stop at cgroup 'b' program.
> 
> I'm proposing that such situation should not be allowed to happen.
> In a->b->c->d cgroup scenario if override+recurse prog attached to 'b'
> then only the same override+recurse can be attached to c, d, a.
> So at detach time there can be gaps (like only 'b' and 'd' have
> override+recurse progs), but walking up until root from any point
> will guarantee that only override+recurse programs are seen.
> 

That seems very limiting to me. Seems like you are suggesting the entire
cgroup tree is recursive or non-recursive, but never a mix.

Re: [PATCH v2 net-next 1/8] bpf: Add support for recursively running cgroup sock filters

[Alexei Starovoitov]

On Tue, Aug 29, 2017 at 07:03:43PM -0600, David Ahern wrote:
> On 8/28/17 10:11 PM, Alexei Starovoitov wrote:
> > 
> > Agree on the above, but you're mixing semantics of the new recurse
> > flag and implementation of it. Ex: we don't have to copy this flag
> > from prog->attr into cgroup. So this reset or non-reset discussion
> > only makes sense in the context of your current implementation.
> > We can implement the logic differently. Like don't copy that flag
> > at all and at attach time walk parent->parent->parent and see
> > what programs are attached. All of them should have prog->attr & recurse_bit set
> > In such implementation detach from 'b' is a nop from reset/non-reset
> > point of view. When socket creation in 'c' is invoked the program
> > 'c' is called first then the code keeps walking parents until root
> > invoking 'a' along the way.
> 
> So you are suggesting there is no recursive flag per cgroup? How do you
> know you need to walk cgroups? How do you know when to stop running
> programs?

you're talking about implementation, right?
My 'proposed' implemenation of walking from cgroup all the way to the root
is just an example. It's not efficient. More below...

> > I'm not saying it will be an efficient implementation. The point
> > is to discuss UAPI independent of implementation.
> > 
> >> ###
> >>
> >> Also, let's agree on this intention. Based on the new ground rule, I
> >> want to point out this example:
> >>
> >> If 'a' gets a program installed with no recurse flag set, ONLY processes
> >> in 'a' have the 'a' program run. Processes in groups 'b', 'c' and 'd'
> >> all stop at cgroup 'b' program.
> > 
> > I'm proposing that such situation should not be allowed to happen.
> > In a->b->c->d cgroup scenario if override+recurse prog attached to 'b'
> > then only the same override+recurse can be attached to c, d, a.
> > So at detach time there can be gaps (like only 'b' and 'd' have
> > override+recurse progs), but walking up until root from any point
> > will guarantee that only override+recurse programs are seen.
> > 
> 
> That seems very limiting to me. Seems like you are suggesting the entire
> cgroup tree is recursive or non-recursive, but never a mix.

Entire cgroup subtree. Yes. It's the simplest uapi I could think of.
Easy to understand and argue about and I think it's solving your use case.
It's also easily extendable. New combination and features won't break
the users. It feels you're in rush to get this stuff for this merge
window, therefore I want to agree on something that is simple,
non-controversial and extensible.
If you're not in rush (I'm not), we can come up with more flexible uapi.
For example: another way of thinking about your 'recursive' requirement
is to think that all 'program to be run' should be present as a link list
in a given cgroup. So no walking a chain of parents.
Instead of 'recursive' let's call this new flag 'multiprog'.
Now in a->b->c->d scenario. We can install 'multiprog' prog in 'b'.
The kernel will automatically propage it (like it does right now
with css_for_each_descendant_pre() loop) to 'c' and to 'd'.
Now we allow users to attach another 'multiprog' program to 'c'.
The kernel will maintain a link list of programs in every cgroup,
so there will be a link list of two programs in 'c' and 'd'
and invocation of the programs will be faster than walking
cgroup->parent->parent and checking some flags at every step,
since there will be less pointer dereferences and no flags to check.
Just invoke all programs in the current cgroup. Kernel took care
of ordering at the time of attach/detach.
I believe Andy proposed something like this back in Jan/Feb.

Re: [PATCH v2 net-next 1/8] bpf: Add support for recursively running cgroup sock filters

[David Ahern]

On 8/29/17 8:58 PM, Alexei Starovoitov wrote:
> On Tue, Aug 29, 2017 at 07:03:43PM -0600, David Ahern wrote:
>> On 8/28/17 10:11 PM, Alexei Starovoitov wrote:
>>>
>>> Agree on the above, but you're mixing semantics of the new recurse
>>> flag and implementation of it. Ex: we don't have to copy this flag
>>> from prog->attr into cgroup. So this reset or non-reset discussion
>>> only makes sense in the context of your current implementation.
>>> We can implement the logic differently. Like don't copy that flag
>>> at all and at attach time walk parent->parent->parent and see
>>> what programs are attached. All of them should have prog->attr & recurse_bit set
>>> In such implementation detach from 'b' is a nop from reset/non-reset
>>> point of view. When socket creation in 'c' is invoked the program
>>> 'c' is called first then the code keeps walking parents until root
>>> invoking 'a' along the way.
>>
>> So you are suggesting there is no recursive flag per cgroup? How do you
>> know you need to walk cgroups? How do you know when to stop running
>> programs?
> 
> you're talking about implementation, right?
> My 'proposed' implemenation of walking from cgroup all the way to the root
> is just an example. It's not efficient. More below...
> 
>>> I'm not saying it will be an efficient implementation. The point
>>> is to discuss UAPI independent of implementation.
>>>
>>>> ###
>>>>
>>>> Also, let's agree on this intention. Based on the new ground rule, I
>>>> want to point out this example:
>>>>
>>>> If 'a' gets a program installed with no recurse flag set, ONLY processes
>>>> in 'a' have the 'a' program run. Processes in groups 'b', 'c' and 'd'
>>>> all stop at cgroup 'b' program.
>>>
>>> I'm proposing that such situation should not be allowed to happen.
>>> In a->b->c->d cgroup scenario if override+recurse prog attached to 'b'
>>> then only the same override+recurse can be attached to c, d, a.
>>> So at detach time there can be gaps (like only 'b' and 'd' have
>>> override+recurse progs), but walking up until root from any point
>>> will guarantee that only override+recurse programs are seen.
>>>
>>
>> That seems very limiting to me. Seems like you are suggesting the entire
>> cgroup tree is recursive or non-recursive, but never a mix.
> 
> Entire cgroup subtree. Yes. It's the simplest uapi I could think of.

So 10 email exchanges later you agree on the UAPI I implemented in this
patch: user opts in to recursive behavior via a new flag at attach time,
and once a recursive program is installed at some point in the cgroup
tree it applies to all descendant cgroups.

So all of these exchanges weren't about the UAPI, but your disagreement
in my implementation. The only user visible change here is only programs
marked recursive are run versus going back to the first cgroup marked
non-recursive.

> Easy to understand and argue about and I think it's solving your use case.
> It's also easily extendable. New combination and features won't break
> the users. It feels you're in rush to get this stuff for this merge
> window, therefore I want to agree on something that is simple,
> non-controversial and extensible.

I am in no-rush, but this does not to fall by the wayside like the net
namespace specification.

Given the pending release of 4.13 net-next will close which gives a 2+
week window to work on v3 before the next merge window. Plenty of time
for me to work it into the many other things on my plate.

Re: [PATCH v2 net-next 1/8] bpf: Add support for recursively running cgroup sock filters

[Alexei Starovoitov]

On Tue, Aug 29, 2017 at 09:38:16PM -0600, David Ahern wrote:
> On 8/29/17 8:58 PM, Alexei Starovoitov wrote:
> > On Tue, Aug 29, 2017 at 07:03:43PM -0600, David Ahern wrote:
> >> On 8/28/17 10:11 PM, Alexei Starovoitov wrote:
> >>>
> >>> Agree on the above, but you're mixing semantics of the new recurse
> >>> flag and implementation of it. Ex: we don't have to copy this flag
> >>> from prog->attr into cgroup. So this reset or non-reset discussion
> >>> only makes sense in the context of your current implementation.
> >>> We can implement the logic differently. Like don't copy that flag
> >>> at all and at attach time walk parent->parent->parent and see
> >>> what programs are attached. All of them should have prog->attr & recurse_bit set
> >>> In such implementation detach from 'b' is a nop from reset/non-reset
> >>> point of view. When socket creation in 'c' is invoked the program
> >>> 'c' is called first then the code keeps walking parents until root
> >>> invoking 'a' along the way.
> >>
> >> So you are suggesting there is no recursive flag per cgroup? How do you
> >> know you need to walk cgroups? How do you know when to stop running
> >> programs?
> > 
> > you're talking about implementation, right?
> > My 'proposed' implemenation of walking from cgroup all the way to the root
> > is just an example. It's not efficient. More below...
> > 
> >>> I'm not saying it will be an efficient implementation. The point
> >>> is to discuss UAPI independent of implementation.
> >>>
> >>>> ###
> >>>>
> >>>> Also, let's agree on this intention. Based on the new ground rule, I
> >>>> want to point out this example:
> >>>>
> >>>> If 'a' gets a program installed with no recurse flag set, ONLY processes
> >>>> in 'a' have the 'a' program run. Processes in groups 'b', 'c' and 'd'
> >>>> all stop at cgroup 'b' program.
> >>>
> >>> I'm proposing that such situation should not be allowed to happen.
> >>> In a->b->c->d cgroup scenario if override+recurse prog attached to 'b'
> >>> then only the same override+recurse can be attached to c, d, a.
> >>> So at detach time there can be gaps (like only 'b' and 'd' have
> >>> override+recurse progs), but walking up until root from any point
> >>> will guarantee that only override+recurse programs are seen.
> >>>
> >>
> >> That seems very limiting to me. Seems like you are suggesting the entire
> >> cgroup tree is recursive or non-recursive, but never a mix.
> > 
> > Entire cgroup subtree. Yes. It's the simplest uapi I could think of.
> 
> So 10 email exchanges later you agree on the UAPI I implemented in this
> patch: user opts in to recursive behavior via a new flag at attach time,
> and once a recursive program is installed at some point in the cgroup
> tree it applies to all descendant cgroups.
> 
> So all of these exchanges weren't about the UAPI, but your disagreement
> in my implementation. The only user visible change here is only programs
> marked recursive are run versus going back to the first cgroup marked
> non-recursive.

you cannot be serious. Your implemention is not at all what i'm proposing
above as a simplest uapi. Should we all go back and re-read from the beginning?

> > Easy to understand and argue about and I think it's solving your use case.
> > It's also easily extendable. New combination and features won't break
> > the users. It feels you're in rush to get this stuff for this merge
> > window, therefore I want to agree on something that is simple,
> > non-controversial and extensible.
> 
> I am in no-rush, but this does not to fall by the wayside like the net
> namespace specification.

It's more than that! I think you only looking at it from vrf perspective
whereas cgroup-bpf became a corner stone feature and enabler for tcp-bpf
which in turn became a stepping stone for bpf_sk_redirect.
So no, I'm not going to let something half baked that touches
the core idea of cgroup-bpf slide in.
Tejun and Andy also need to take a look, so yes it will take long
time for everyone to agree on this core uapi.

> Given the pending release of 4.13 net-next will close which gives a 2+
> week window to work on v3 before the next merge window. Plenty of time
> for me to work it into the many other things on my plate.

As I proposed several emails ago, please repost patches 2 and 3 that
I already acked and we can continue discussing this patch without
the agitation.

Re: [PATCH v2 net-next 1/8] bpf: Add support for recursively running cgroup sock filters

[Tejun Heo]

Hello, David, Alexei.

Sorry about late reply.

On Sun, Aug 27, 2017 at 08:49:23AM -0600, David Ahern wrote:
> On 8/25/17 8:49 PM, Alexei Starovoitov wrote:
> > 
> >> +	if (prog && curr_recursive && !new_recursive)
> >> +		/* if a parent has recursive prog attached, only
> >> +		 * allow recursive programs in descendent cgroup
> >> +		 */
> >> +		return -EINVAL;
> >> +
> >>  	old_prog = cgrp->bpf.prog[type];
> > 
> > ... I'm struggling to completely understand how it interacts
> > with BPF_F_ALLOW_OVERRIDE.
> 
> The 2 flags are completely independent. The existing override logic is
> unchanged. If a program can not be overridden, then the new recursive
> flag is irrelevant.

I'm not sure all four combo of the two flags makes sense.  Can't we
have something simpler like the following?

1. None: No further bpf programs allowed in the subtree.

2. Overridable: If a sub-cgroup installs the same bpf program, this
   one yields to that one.

3. Recursive: If a sub-cgroup installs the same bpf program, that
   cgroup program gets run in addition to this one.

Note that we can have combinations of overridables and recursives -
both allow further programs in the sub-hierarchy and the only
distinction is whether that specific program behaves when that
happens.

Thanks.

-- 
tejun

Re: [PATCH v2 net-next 1/8] bpf: Add support for recursively running cgroup sock filters

[David Ahern]

On 8/31/17 8:22 AM, Tejun Heo wrote:
> On Sun, Aug 27, 2017 at 08:49:23AM -0600, David Ahern wrote:
>> On 8/25/17 8:49 PM, Alexei Starovoitov wrote:
>>>
>>>> +	if (prog && curr_recursive && !new_recursive)
>>>> +		/* if a parent has recursive prog attached, only
>>>> +		 * allow recursive programs in descendent cgroup
>>>> +		 */
>>>> +		return -EINVAL;
>>>> +
>>>>  	old_prog = cgrp->bpf.prog[type];
>>>
>>> ... I'm struggling to completely understand how it interacts
>>> with BPF_F_ALLOW_OVERRIDE.
>>
>> The 2 flags are completely independent. The existing override logic is
>> unchanged. If a program can not be overridden, then the new recursive
>> flag is irrelevant.
> 
> I'm not sure all four combo of the two flags makes sense.  Can't we
> have something simpler like the following?
> 
> 1. None: No further bpf programs allowed in the subtree.
> 
> 2. Overridable: If a sub-cgroup installs the same bpf program, this
>    one yields to that one.
> 
> 3. Recursive: If a sub-cgroup installs the same bpf program, that
>    cgroup program gets run in addition to this one.
> 
> Note that we can have combinations of overridables and recursives -
> both allow further programs in the sub-hierarchy and the only
> distinction is whether that specific program behaves when that
> happens.
> 

I am going to send v3 for patches 2-6 and 8 - the uncontested patches.

Alexei and I will meet in L.A. the week of Sept 11-15 to discuss the
recursive implementation (Patch 1 and its testing, patch 7).

Re: [PATCH v2 net-next 1/8] bpf: Add support for recursively running cgroup sock filters

[Alexei Starovoitov]

On Thu, Aug 31, 2017 at 07:22:01AM -0700, Tejun Heo wrote:
> Hello, David, Alexei.
> 
> Sorry about late reply.
> 
> On Sun, Aug 27, 2017 at 08:49:23AM -0600, David Ahern wrote:
> > On 8/25/17 8:49 PM, Alexei Starovoitov wrote:
> > > 
> > >> +	if (prog && curr_recursive && !new_recursive)
> > >> +		/* if a parent has recursive prog attached, only
> > >> +		 * allow recursive programs in descendent cgroup
> > >> +		 */
> > >> +		return -EINVAL;
> > >> +
> > >>  	old_prog = cgrp->bpf.prog[type];
> > > 
> > > ... I'm struggling to completely understand how it interacts
> > > with BPF_F_ALLOW_OVERRIDE.
> > 
> > The 2 flags are completely independent. The existing override logic is
> > unchanged. If a program can not be overridden, then the new recursive
> > flag is irrelevant.
> 
> I'm not sure all four combo of the two flags makes sense.  Can't we
> have something simpler like the following?
> 
> 1. None: No further bpf programs allowed in the subtree.
> 
> 2. Overridable: If a sub-cgroup installs the same bpf program, this
>    one yields to that one.
> 
> 3. Recursive: If a sub-cgroup installs the same bpf program, that
>    cgroup program gets run in addition to this one.
> 
> Note that we can have combinations of overridables and recursives -
> both allow further programs in the sub-hierarchy and the only
> distinction is whether that specific program behaves when that
> happens.

If I understand the proposal correctly in case of:
A (with recurs) -> B (with override) -> C (with recurse) -> D (with override)
when something happens in D, you propose to run D,C,A  ?

With the order of execution from children to parent?
That's a bit a different then what I was proposing with 'multi-prog' flag,
but the more I think about it the more I like it.
In your case detach is sort of transparent to everything around.
And you would also allow to say 'None' to one of the substrees too, right?
So something like:
A (with recurs) -> B (with override) -> C (with recurse) -> D (None) -> E
would mean that E cannot attach anything and events in E will
call D->C->A, right?
I will work on a patch for the above and see how it looks.

Re: [PATCH v2 net-next 1/8] bpf: Add support for recursively running cgroup sock filters

[Tejun Heo]

Hello, Alexei.

On Thu, Aug 31, 2017 at 08:27:56PM -0700, Alexei Starovoitov wrote:
> > > The 2 flags are completely independent. The existing override logic is
> > > unchanged. If a program can not be overridden, then the new recursive
> > > flag is irrelevant.
> > 
> > I'm not sure all four combo of the two flags makes sense.  Can't we
> > have something simpler like the following?
> > 
> > 1. None: No further bpf programs allowed in the subtree.
> > 
> > 2. Overridable: If a sub-cgroup installs the same bpf program, this
> >    one yields to that one.
> > 
> > 3. Recursive: If a sub-cgroup installs the same bpf program, that
> >    cgroup program gets run in addition to this one.
> > 
> > Note that we can have combinations of overridables and recursives -
> > both allow further programs in the sub-hierarchy and the only
> > distinction is whether that specific program behaves when that
> > happens.
> 
> If I understand the proposal correctly in case of:
> A (with recurs) -> B (with override) -> C (with recurse) -> D (with override)
> when something happens in D, you propose to run D,C,A  ?

Yes, B gets overridden by C, so the effective progarms are A, C and D.

> With the order of execution from children to parent?

Hmm... I'm not sure about the execution ordering.  How these programs
chain would be dependent on the type of the program, right?  Would we
be able to use the same chaining order for all types of programs?

> That's a bit a different then what I was proposing with 'multi-prog' flag,
> but the more I think about it the more I like it.

Great.

> In your case detach is sort of transparent to everything around.
> And you would also allow to say 'None' to one of the substrees too, right?
> So something like:
> A (with recurs) -> B (with override) -> C (with recurse) -> D (None) -> E
> would mean that E cannot attach anything and events in E will
> call D->C->A, right?

Yeap.

Thanks.

-- 
tejun

[PATCH v2 net-next 2/8] bpf: Add mark and priority to sock options that can be set

[David Ahern]

Add socket mark and priority to fields that can be set by
ebpf program when a socket is created.

Signed-off-by: David Ahern <dsahern@gmail.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
 include/uapi/linux/bpf.h |  2 ++
 net/core/filter.c        | 26 ++++++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 595e31b30f23..f72b957580cd 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -773,6 +773,8 @@ struct bpf_sock {
 	__u32 family;
 	__u32 type;
 	__u32 protocol;
+	__u32 mark;
+	__u32 priority;
 };
 
 #define XDP_PACKET_HEADROOM 256
diff --git a/net/core/filter.c b/net/core/filter.c
index 4bcd6baa80c9..d582d1b1e533 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3444,6 +3444,10 @@ static bool sock_filter_is_valid_access(int off, int size,
 		switch (off) {
 		case offsetof(struct bpf_sock, bound_dev_if):
 			break;
+		case offsetof(struct bpf_sock, mark):
+			break;
+		case offsetof(struct bpf_sock, priority):
+			break;
 		default:
 			return false;
 		}
@@ -3947,6 +3951,28 @@ static u32 sock_filter_convert_ctx_access(enum bpf_access_type type,
 				      offsetof(struct sock, sk_bound_dev_if));
 		break;
 
+	case offsetof(struct bpf_sock, mark):
+		BUILD_BUG_ON(FIELD_SIZEOF(struct sock, sk_mark) != 4);
+
+		if (type == BPF_WRITE)
+			*insn++ = BPF_STX_MEM(BPF_W, si->dst_reg, si->src_reg,
+					offsetof(struct sock, sk_mark));
+		else
+			*insn++ = BPF_LDX_MEM(BPF_W, si->dst_reg, si->src_reg,
+				      offsetof(struct sock, sk_mark));
+		break;
+
+	case offsetof(struct bpf_sock, priority):
+		BUILD_BUG_ON(FIELD_SIZEOF(struct sock, sk_priority) != 4);
+
+		if (type == BPF_WRITE)
+			*insn++ = BPF_STX_MEM(BPF_W, si->dst_reg, si->src_reg,
+					offsetof(struct sock, sk_priority));
+		else
+			*insn++ = BPF_LDX_MEM(BPF_W, si->dst_reg, si->src_reg,
+				      offsetof(struct sock, sk_priority));
+		break;
+
 	case offsetof(struct bpf_sock, family):
 		BUILD_BUG_ON(FIELD_SIZEOF(struct sock, sk_family) != 2);
 
-- 
2.1.4

[PATCH v2 net-next 3/8] bpf: Allow cgroup sock filters to use get_current_uid_gid helper

[David Ahern]

Allow BPF programs run on sock create to use the get_current_uid_gid
helper. IPv4 and IPv6 sockets are created in a process context so
there is always a valid uid/gid

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 net/core/filter.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index d582d1b1e533..eb505842a77e 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3139,6 +3139,20 @@ bpf_base_func_proto(enum bpf_func_id func_id)
 }
 
 static const struct bpf_func_proto *
+sock_filter_func_proto(enum bpf_func_id func_id)
+{
+	switch (func_id) {
+	/* inet and inet6 sockets are created in a process
+	 * context so there is always a valid uid/gid
+	 */
+	case BPF_FUNC_get_current_uid_gid:
+		return &bpf_get_current_uid_gid_proto;
+	default:
+		return bpf_base_func_proto(func_id);
+	}
+}
+
+static const struct bpf_func_proto *
 sk_filter_func_proto(enum bpf_func_id func_id)
 {
 	switch (func_id) {
@@ -4222,7 +4236,7 @@ const struct bpf_verifier_ops lwt_xmit_prog_ops = {
 };
 
 const struct bpf_verifier_ops cg_sock_prog_ops = {
-	.get_func_proto		= bpf_base_func_proto,
+	.get_func_proto		= sock_filter_func_proto,
 	.is_valid_access	= sock_filter_is_valid_access,
 	.convert_ctx_access	= sock_filter_convert_ctx_access,
 };
-- 
2.1.4

Re: [PATCH v2 net-next 3/8] bpf: Allow cgroup sock filters to use get_current_uid_gid helper

[Alexei Starovoitov]

On Fri, Aug 25, 2017 at 12:05:36PM -0700, David Ahern wrote:
> Allow BPF programs run on sock create to use the get_current_uid_gid
> helper. IPv4 and IPv6 sockets are created in a process context so
> there is always a valid uid/gid
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>

Acked-by: Alexei Starovoitov <ast@kernel.org>

[PATCH v2 net-next 4/8] samples/bpf: Update sock test to allow setting mark and priority

[David Ahern]

Update sock test to set mark and priority on socket create.

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 samples/bpf/test_cgrp2_sock.c  | 139 ++++++++++++++++++++++++++++++++++++-----
 samples/bpf/test_cgrp2_sock.sh |   2 +-
 2 files changed, 123 insertions(+), 18 deletions(-)

diff --git a/samples/bpf/test_cgrp2_sock.c b/samples/bpf/test_cgrp2_sock.c
index c3cfb23e23b5..b018bf948933 100644
--- a/samples/bpf/test_cgrp2_sock.c
+++ b/samples/bpf/test_cgrp2_sock.c
@@ -19,63 +19,168 @@
 #include <errno.h>
 #include <fcntl.h>
 #include <net/if.h>
+#include <inttypes.h>
 #include <linux/bpf.h>
 
 #include "libbpf.h"
 
 char bpf_log_buf[BPF_LOG_BUF_SIZE];
 
-static int prog_load(int idx)
+static int prog_load(__u32 idx, __u32 mark, __u32 prio)
 {
-	struct bpf_insn prog[] = {
+	/* save pointer to context */
+	struct bpf_insn prog_start[] = {
 		BPF_MOV64_REG(BPF_REG_6, BPF_REG_1),
+	};
+	struct bpf_insn prog_end[] = {
+		BPF_MOV64_IMM(BPF_REG_0, 1), /* r0 = verdict */
+		BPF_EXIT_INSN(),
+	};
+
+	/* set sk_bound_dev_if on socket */
+	struct bpf_insn prog_dev[] = {
 		BPF_MOV64_IMM(BPF_REG_3, idx),
 		BPF_MOV64_IMM(BPF_REG_2, offsetof(struct bpf_sock, bound_dev_if)),
 		BPF_STX_MEM(BPF_W, BPF_REG_1, BPF_REG_3, offsetof(struct bpf_sock, bound_dev_if)),
-		BPF_MOV64_IMM(BPF_REG_0, 1), /* r0 = verdict */
-		BPF_EXIT_INSN(),
 	};
-	size_t insns_cnt = sizeof(prog) / sizeof(struct bpf_insn);
 
-	return bpf_load_program(BPF_PROG_TYPE_CGROUP_SOCK, prog, insns_cnt,
+	/* set mark on socket */
+	struct bpf_insn prog_mark[] = {
+		BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+		BPF_MOV64_IMM(BPF_REG_3, mark),
+		BPF_MOV64_IMM(BPF_REG_2, offsetof(struct bpf_sock, mark)),
+		BPF_STX_MEM(BPF_W, BPF_REG_1, BPF_REG_3, offsetof(struct bpf_sock, mark)),
+	};
+
+	/* set priority on socket */
+	struct bpf_insn prog_prio[] = {
+		BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+		BPF_MOV64_IMM(BPF_REG_3, prio),
+		BPF_MOV64_IMM(BPF_REG_2, offsetof(struct bpf_sock, priority)),
+		BPF_STX_MEM(BPF_W, BPF_REG_1, BPF_REG_3, offsetof(struct bpf_sock, priority)),
+	};
+
+	struct bpf_insn *prog;
+	size_t insns_cnt;
+	void *p;
+	int ret;
+
+	insns_cnt = sizeof(prog_start) + sizeof(prog_end);
+	if (idx)
+		insns_cnt += sizeof(prog_dev);
+
+	if (mark)
+		insns_cnt += sizeof(prog_mark);
+
+	if (prio)
+		insns_cnt += sizeof(prog_prio);
+
+	p = prog = malloc(insns_cnt);
+	if (!prog) {
+		fprintf(stderr, "Failed to allocate memory for instructions\n");
+		return EXIT_FAILURE;
+	}
+
+	memcpy(p, prog_start, sizeof(prog_start));
+	p += sizeof(prog_start);
+
+	if (idx) {
+		memcpy(p, prog_dev, sizeof(prog_dev));
+		p += sizeof(prog_dev);
+	}
+
+	if (mark) {
+		memcpy(p, prog_mark, sizeof(prog_mark));
+		p += sizeof(prog_mark);
+	}
+
+	if (prio) {
+		memcpy(p, prog_prio, sizeof(prog_prio));
+		p += sizeof(prog_prio);
+	}
+
+	memcpy(p, prog_end, sizeof(prog_end));
+	p += sizeof(prog_end);
+
+	insns_cnt /= sizeof(struct bpf_insn);
+
+	ret = bpf_load_program(BPF_PROG_TYPE_CGROUP_SOCK, prog, insns_cnt,
 				"GPL", 0, bpf_log_buf, BPF_LOG_BUF_SIZE);
+
+	free(prog);
+
+	return ret;
 }
 
 static int usage(const char *argv0)
 {
-	printf("Usage: %s cg-path device-index\n", argv0);
+	printf("Usage: %s -b bind-to-dev -m mark -p prio -r cg-path\n", argv0);
 	return EXIT_FAILURE;
 }
 
 int main(int argc, char **argv)
 {
+	__u32 attach_flags = BPF_F_ALLOW_OVERRIDE;
+	__u32 idx = 0, mark = 0, prio = 0;
+	const char *cgrp_path = NULL;
 	int cg_fd, prog_fd, ret;
-	unsigned int idx;
+	int rc;
+
+	while ((rc = getopt(argc, argv, "b:m:p:r")) != -1) {
+		switch (rc) {
+		case 'b':
+			idx = if_nametoindex(optarg);
+			if (!idx) {
+				idx = strtoumax(optarg, NULL, 0);
+				if (!idx) {
+					printf("Invalid device name\n");
+					return EXIT_FAILURE;
+				}
+			}
+			break;
+		case 'm':
+			mark = strtoumax(optarg, NULL, 0);
+			break;
+		case 'p':
+			prio = strtoumax(optarg, NULL, 0);
+			break;
+		case 'r':
+			attach_flags |= BPF_F_RECURSIVE;
+			break;
+		default:
+			return usage(argv[0]);
+		}
+	}
 
-	if (argc < 2)
+	if (optind == argc)
 		return usage(argv[0]);
 
-	idx = if_nametoindex(argv[2]);
-	if (!idx) {
-		printf("Invalid device name\n");
+	cgrp_path = argv[optind];
+	if (!cgrp_path) {
+		fprintf(stderr, "cgroup path not given\n");
 		return EXIT_FAILURE;
 	}
 
-	cg_fd = open(argv[1], O_DIRECTORY | O_RDONLY);
+	if (!idx && !mark && !prio) {
+		fprintf(stderr, "One of device, mark or priority must be given\n");
+		return EXIT_FAILURE;
+	}
+
+	cg_fd = open(cgrp_path, O_DIRECTORY | O_RDONLY);
 	if (cg_fd < 0) {
 		printf("Failed to open cgroup path: '%s'\n", strerror(errno));
 		return EXIT_FAILURE;
 	}
 
-	prog_fd = prog_load(idx);
-	printf("Output from kernel verifier:\n%s\n-------\n", bpf_log_buf);
-
+	prog_fd = prog_load(idx, mark, prio);
 	if (prog_fd < 0) {
 		printf("Failed to load prog: '%s'\n", strerror(errno));
+		printf("Output from kernel verifier:\n%s\n-------\n", bpf_log_buf);
 		return EXIT_FAILURE;
 	}
 
-	ret = bpf_prog_attach(prog_fd, cg_fd, BPF_CGROUP_INET_SOCK_CREATE, 0);
+	ret = bpf_prog_attach(prog_fd, cg_fd, BPF_CGROUP_INET_SOCK_CREATE,
+			      attach_flags);
 	if (ret < 0) {
 		printf("Failed to attach prog to cgroup: '%s'\n",
 		       strerror(errno));
diff --git a/samples/bpf/test_cgrp2_sock.sh b/samples/bpf/test_cgrp2_sock.sh
index 925fd467c7cc..1153c33e8964 100755
--- a/samples/bpf/test_cgrp2_sock.sh
+++ b/samples/bpf/test_cgrp2_sock.sh
@@ -20,7 +20,7 @@ function attach_bpf {
 	mkdir -p /tmp/cgroupv2
 	mount -t cgroup2 none /tmp/cgroupv2
 	mkdir -p /tmp/cgroupv2/foo
-	test_cgrp2_sock /tmp/cgroupv2/foo foo
+	test_cgrp2_sock -b foo /tmp/cgroupv2/foo
 	echo $$ >> /tmp/cgroupv2/foo/cgroup.procs
 }
 
-- 
2.1.4

[PATCH v2 net-next 5/8] samples/bpf: Add detach option to test_cgrp2_sock

[David Ahern]

Add option to detach programs from a cgroup.

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 samples/bpf/test_cgrp2_sock.c | 50 ++++++++++++++++++++++++++++++-------------
 1 file changed, 35 insertions(+), 15 deletions(-)

diff --git a/samples/bpf/test_cgrp2_sock.c b/samples/bpf/test_cgrp2_sock.c
index b018bf948933..a1ef7b8bd3f9 100644
--- a/samples/bpf/test_cgrp2_sock.c
+++ b/samples/bpf/test_cgrp2_sock.c
@@ -114,7 +114,12 @@ static int prog_load(__u32 idx, __u32 mark, __u32 prio)
 
 static int usage(const char *argv0)
 {
-	printf("Usage: %s -b bind-to-dev -m mark -p prio -r cg-path\n", argv0);
+	printf("Usage:\n");
+	printf("  Attach a program\n");
+	printf("  %s -b bind-to-dev -m mark -p prio -r cg-path\n", argv0);
+	printf("\n");
+	printf("  Detach a program\n");
+	printf("  %s -d cg-path\n", argv0);
 	return EXIT_FAILURE;
 }
 
@@ -124,10 +129,14 @@ int main(int argc, char **argv)
 	__u32 idx = 0, mark = 0, prio = 0;
 	const char *cgrp_path = NULL;
 	int cg_fd, prog_fd, ret;
+	int do_attach = 1;
 	int rc;
 
-	while ((rc = getopt(argc, argv, "b:m:p:r")) != -1) {
+	while ((rc = getopt(argc, argv, "db:m:p:r")) != -1) {
 		switch (rc) {
+		case 'd':
+			do_attach = 0;
+			break;
 		case 'b':
 			idx = if_nametoindex(optarg);
 			if (!idx) {
@@ -161,7 +170,7 @@ int main(int argc, char **argv)
 		return EXIT_FAILURE;
 	}
 
-	if (!idx && !mark && !prio) {
+	if (do_attach && !idx && !mark && !prio) {
 		fprintf(stderr, "One of device, mark or priority must be given\n");
 		return EXIT_FAILURE;
 	}
@@ -172,20 +181,31 @@ int main(int argc, char **argv)
 		return EXIT_FAILURE;
 	}
 
-	prog_fd = prog_load(idx, mark, prio);
-	if (prog_fd < 0) {
-		printf("Failed to load prog: '%s'\n", strerror(errno));
-		printf("Output from kernel verifier:\n%s\n-------\n", bpf_log_buf);
-		return EXIT_FAILURE;
-	}
+	if (do_attach) {
+		prog_fd = prog_load(idx, mark, prio);
+		if (prog_fd < 0) {
+			printf("Failed to load prog: '%s'\n", strerror(errno));
+			printf("Output from kernel verifier:\n%s\n-------\n",
+			       bpf_log_buf);
+			return EXIT_FAILURE;
+		}
 
-	ret = bpf_prog_attach(prog_fd, cg_fd, BPF_CGROUP_INET_SOCK_CREATE,
-			      attach_flags);
-	if (ret < 0) {
-		printf("Failed to attach prog to cgroup: '%s'\n",
-		       strerror(errno));
-		return EXIT_FAILURE;
+		ret = bpf_prog_attach(prog_fd, cg_fd, BPF_CGROUP_INET_SOCK_CREATE,
+				      attach_flags);
+		if (ret < 0) {
+			printf("Failed to attach prog to cgroup: '%s'\n",
+			       strerror(errno));
+			return EXIT_FAILURE;
+		}
+	} else {
+		ret = bpf_prog_detach(cg_fd, BPF_CGROUP_INET_SOCK_CREATE);
+		if (ret < 0) {
+			printf("Failed to detach prog from cgroup: '%s'\n",
+			       strerror(errno));
+			return EXIT_FAILURE;
+		}
 	}
 
+	close(cg_fd);
 	return EXIT_SUCCESS;
 }
-- 
2.1.4

[PATCH v2 net-next 6/8] samples/bpf: Add option to dump socket settings

[David Ahern]

Add option to dump socket settings. Will be used in the next patch
to verify bpf programs are correctly setting mark, priority and
device based on the cgroup attachment for the program run.

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 samples/bpf/test_cgrp2_sock.c | 75 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 73 insertions(+), 2 deletions(-)

diff --git a/samples/bpf/test_cgrp2_sock.c b/samples/bpf/test_cgrp2_sock.c
index a1ef7b8bd3f9..eabf530a5223 100644
--- a/samples/bpf/test_cgrp2_sock.c
+++ b/samples/bpf/test_cgrp2_sock.c
@@ -112,6 +112,70 @@ static int prog_load(__u32 idx, __u32 mark, __u32 prio)
 	return ret;
 }
 
+static int get_bind_to_device(int sd, char *name, size_t len)
+{
+	socklen_t optlen = len;
+	int rc;
+
+	name[0] = '\0';
+	rc = getsockopt(sd, SOL_SOCKET, SO_BINDTODEVICE, name, &optlen);
+	if (rc < 0)
+		perror("setsockopt(SO_BINDTODEVICE)");
+
+	return rc;
+}
+
+static unsigned int get_somark(int sd)
+{
+	unsigned int mark = 0;
+	socklen_t optlen = sizeof(mark);
+	int rc;
+
+	rc = getsockopt(sd, SOL_SOCKET, SO_MARK, &mark, &optlen);
+	if (rc < 0)
+		perror("getsockopt(SO_MARK)");
+
+	return mark;
+}
+
+static unsigned int get_priority(int sd)
+{
+	unsigned int prio = 0;
+	socklen_t optlen = sizeof(prio);
+	int rc;
+
+	rc = getsockopt(sd, SOL_SOCKET, SO_PRIORITY, &prio, &optlen);
+	if (rc < 0)
+		perror("getsockopt(SO_PRIORITY)");
+
+	return prio;
+}
+
+static int show_sockopts(int family)
+{
+	unsigned int mark, prio;
+	char name[16];
+	int sd;
+
+	sd = socket(family, SOCK_DGRAM, 17);
+	if (sd < 0) {
+		perror("socket");
+		return 1;
+	}
+
+	if (get_bind_to_device(sd, name, sizeof(name)) < 0)
+		return 1;
+
+	mark = get_somark(sd);
+	prio = get_priority(sd);
+
+	close(sd);
+
+	printf("sd %d: dev %s, mark %u, priority %u\n", sd, name, mark, prio);
+
+	return 0;
+}
+
 static int usage(const char *argv0)
 {
 	printf("Usage:\n");
@@ -120,6 +184,9 @@ static int usage(const char *argv0)
 	printf("\n");
 	printf("  Detach a program\n");
 	printf("  %s -d cg-path\n", argv0);
+	printf("\n");
+	printf("  Show inherited socket settings (mark, priority, and device)\n");
+	printf("  %s [-6]\n", argv0);
 	return EXIT_FAILURE;
 }
 
@@ -129,10 +196,11 @@ int main(int argc, char **argv)
 	__u32 idx = 0, mark = 0, prio = 0;
 	const char *cgrp_path = NULL;
 	int cg_fd, prog_fd, ret;
+	int family = PF_INET;
 	int do_attach = 1;
 	int rc;
 
-	while ((rc = getopt(argc, argv, "db:m:p:r")) != -1) {
+	while ((rc = getopt(argc, argv, "db:m:p:r6")) != -1) {
 		switch (rc) {
 		case 'd':
 			do_attach = 0;
@@ -156,13 +224,16 @@ int main(int argc, char **argv)
 		case 'r':
 			attach_flags |= BPF_F_RECURSIVE;
 			break;
+		case '6':
+			family = PF_INET6;
+			break;
 		default:
 			return usage(argv[0]);
 		}
 	}
 
 	if (optind == argc)
-		return usage(argv[0]);
+		return show_sockopts(family);
 
 	cgrp_path = argv[optind];
 	if (!cgrp_path) {
-- 
2.1.4

[PATCH v2 net-next 7/8] samples/bpf: Add test case for nested socket options

[David Ahern]

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 samples/bpf/test_cgrp2_sock3.sh | 162 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 162 insertions(+)
 create mode 100755 samples/bpf/test_cgrp2_sock3.sh

diff --git a/samples/bpf/test_cgrp2_sock3.sh b/samples/bpf/test_cgrp2_sock3.sh
new file mode 100755
index 000000000000..9bfed035963f
--- /dev/null
+++ b/samples/bpf/test_cgrp2_sock3.sh
@@ -0,0 +1,162 @@
+#!/bin/sh
+
+# Verify socket options inherited by bpf programs attached
+# to a cgroup.
+
+CGRP_MNT="/tmp/cgroupv2-test_cgrp2_sock"
+
+################################################################################
+#
+print_result()
+{
+	printf "%-50s    [%4s]\n" "$1" "$2"
+}
+
+check_sock()
+{
+	out=$(test_cgrp2_sock)
+	echo $out | grep -q "$1"
+	if [ $? -ne 0 ]; then
+		print_result "IPv4: $2" "FAIL"
+		echo "    expected: $1"
+		echo "        have: $out"
+		rc=1
+	else
+		print_result "IPv4: $2" " OK "
+	fi
+}
+
+check_sock6()
+{
+	out=$(test_cgrp2_sock -6)
+	echo $out | grep -q "$1"
+	if [ $? -ne 0 ]; then
+		print_result "IPv6: $2" "FAIL"
+		echo "    expected: $1"
+		echo "        have: $out"
+		rc=1
+	else
+		print_result "IPv6: $2" " OK "
+	fi
+}
+
+################################################################################
+#
+setup()
+{
+	cleanup 2>/dev/null
+
+	mkdir -p ${CGRP_MNT}/cgrp_sock_test/prio/mark/dev
+	[ $? -ne 0 ] && cleanup_and_exit 1 "Failed to create cgroup hierarchy"
+
+	test_cgrp2_sock -p 123 ${CGRP_MNT}/cgrp_sock_test/prio
+	[ $? -ne 0 ] && cleanup_and_exit 1 "Failed to install program to set priority"
+
+	test_cgrp2_sock -m 666 -r ${CGRP_MNT}/cgrp_sock_test/prio/mark
+	[ $? -ne 0 ] && cleanup_and_exit 1 "Failed to install program to set mark"
+
+	test_cgrp2_sock -b cgrp2_sock -r ${CGRP_MNT}/cgrp_sock_test/prio/mark/dev
+	[ $? -ne 0 ] && cleanup_and_exit 1 "Failed to install program to set device"
+}
+
+cleanup()
+{
+	echo $$ >> ${CGRP_MNT}/cgroup.procs
+	rmdir ${CGRP_MNT}/cgrp_sock_test/prio/mark/dev
+	rmdir ${CGRP_MNT}/cgrp_sock_test/prio/mark
+	rmdir ${CGRP_MNT}/cgrp_sock_test/prio
+	rmdir ${CGRP_MNT}/cgrp_sock_test
+}
+
+cleanup_and_exit()
+{
+	local rc=$1
+	local msg="$2"
+
+	[ -n "$msg" ] && echo "ERROR: $msg"
+
+	ip li del cgrp2_sock
+	umount ${CGRP_MNT}
+
+	exit $rc
+}
+
+################################################################################
+#
+
+run_tests()
+{
+	# set pid into first cgroup. socket should show it
+	# has a priority but not a mark or device bind
+	echo $$ > ${CGRP_MNT}/cgrp_sock_test/prio/cgroup.procs
+	check_sock "dev , mark 0, priority 123" "Priority only"
+
+	# set pid into second group. socket should show it
+	# has a priority and mark but not a device bind
+	echo $$ > ${CGRP_MNT}/cgrp_sock_test/prio/mark/cgroup.procs
+	check_sock "dev , mark 666, priority 123" "Priority + mark"
+
+	# set pid into inner group. socket should show it
+	# has a priority, mark and a device bind
+	echo $$ > ${CGRP_MNT}/cgrp_sock_test/prio/mark/dev/cgroup.procs
+	check_sock "dev cgrp2_sock, mark 666, priority 123" "Priority + mark + dev"
+
+	echo
+
+	# set pid into first cgroup. socket should show it
+	# has a priority but not a mark or device bind
+	echo $$ > ${CGRP_MNT}/cgrp_sock_test/prio/cgroup.procs
+	check_sock6 "dev , mark 0, priority 123" "Priority only"
+
+	# set pid into second group. socket should show it
+	# has a priority and mark but not a device bind
+	echo $$ > ${CGRP_MNT}/cgrp_sock_test/prio/mark/cgroup.procs
+	check_sock6 "dev , mark 666, priority 123" "Priority + mark"
+
+	# set pid into inner group. socket should show it
+	# has a priority, mark and a device bind
+	echo $$ > ${CGRP_MNT}/cgrp_sock_test/prio/mark/dev/cgroup.procs
+	check_sock6 "dev cgrp2_sock, mark 666, priority 123" "Priority + mark + dev"
+}
+
+################################################################################
+# verify expected invalid setups are invalid
+
+invalid_setup()
+{
+	echo
+
+	mkdir -p ${CGRP_MNT}/cgrp_sock_test/prio/mark/dev
+	[ $? -ne 0 ] && cleanup_and_exit 1 "Failed to create cgroup hierarchy"
+
+	test_cgrp2_sock -p 123 -r ${CGRP_MNT}/cgrp_sock_test/prio
+	[ $? -ne 0 ] && cleanup_and_exit 1 "Failed to install program to set priority"
+
+	# recursive - followed by non-recursive is not allowed
+	test_cgrp2_sock -m 666 ${CGRP_MNT}/cgrp_sock_test/prio/mark >/dev/null 2>&1
+	if [ $? -eq 0 ]; then
+		print_result "recursive setting followed by non-recursive" "FAIL"
+	else
+		print_result "recursive setting followed by non-recursive" " OK "
+	fi
+}
+
+################################################################################
+# main
+
+rc=0
+
+ip li add cgrp2_sock type dummy 2>/dev/null
+
+set -e
+mkdir -p ${CGRP_MNT}
+mount -t cgroup2 none ${CGRP_MNT}
+set +e
+
+setup
+run_tests
+cleanup
+
+invalid_setup
+
+cleanup_and_exit $rc
-- 
2.1.4

[PATCH v2 net-next 8/8] samples/bpf: Update cgroup socket examples to use uid gid helper

[David Ahern]

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 samples/bpf/sock_flags_kern.c |  5 +++++
 samples/bpf/test_cgrp2_sock.c | 12 +++++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/samples/bpf/sock_flags_kern.c b/samples/bpf/sock_flags_kern.c
index 533dd11a6baa..05dcdf8a4baa 100644
--- a/samples/bpf/sock_flags_kern.c
+++ b/samples/bpf/sock_flags_kern.c
@@ -9,8 +9,13 @@ SEC("cgroup/sock1")
 int bpf_prog1(struct bpf_sock *sk)
 {
 	char fmt[] = "socket: family %d type %d protocol %d\n";
+	char fmt2[] = "socket: uid %u gid %u\n";
+	__u64 gid_uid = bpf_get_current_uid_gid();
+	__u32 uid = gid_uid & 0xffffffff;
+	__u32 gid = gid_uid >> 32;
 
 	bpf_trace_printk(fmt, sizeof(fmt), sk->family, sk->type, sk->protocol);
+	bpf_trace_printk(fmt2, sizeof(fmt2), uid, gid);
 
 	/* block PF_INET6, SOCK_RAW, IPPROTO_ICMPV6 sockets
 	 * ie., make ping6 fail
diff --git a/samples/bpf/test_cgrp2_sock.c b/samples/bpf/test_cgrp2_sock.c
index eabf530a5223..e9eeaaf52219 100644
--- a/samples/bpf/test_cgrp2_sock.c
+++ b/samples/bpf/test_cgrp2_sock.c
@@ -46,8 +46,18 @@ static int prog_load(__u32 idx, __u32 mark, __u32 prio)
 
 	/* set mark on socket */
 	struct bpf_insn prog_mark[] = {
-		BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+		/* get uid of process */
+		BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
+			     BPF_FUNC_get_current_uid_gid),
+		BPF_ALU64_IMM(BPF_AND, BPF_REG_0, 0xffffffff),
+
+		/* if uid is 0, use given mark, else use the uid as the mark */
+		BPF_MOV64_REG(BPF_REG_3, BPF_REG_0),
+		BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
 		BPF_MOV64_IMM(BPF_REG_3, mark),
+
+		/* set the mark on the new socket */
+		BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
 		BPF_MOV64_IMM(BPF_REG_2, offsetof(struct bpf_sock, mark)),
 		BPF_STX_MEM(BPF_W, BPF_REG_1, BPF_REG_3, offsetof(struct bpf_sock, mark)),
 	};
-- 
2.1.4

Re: [PATCH v2 net-next 0/8] bpf: Add option to set mark and priority in cgroup sock programs

[David Miller]

From: David Ahern <dsahern@gmail.com>
Date: Fri, 25 Aug 2017 12:05:33 -0700

> Add option to set mark and priority in addition to bound device for newly
> created sockets. Also, allow the bpf programs to use the get_current_uid_gid
> helper meaning socket marks, priority and device can be set base on the
> uid/gid of the running process.
> 
> For flexbility in deploying these programs, option is added to allow cgroups
> to be walked from current to root running any program attached. This allows
> one cgroup level to control the device a socket is bound to (e.g, a VRF) while
> cgroups can be used to set socket marks and priority.
> 
> Sample programs are updated to demonstrate the new options.
> 
> v2
> - added flag to control recursive behavior as requested by Alexei
> - added comment to sock_filter_func_proto regarding use of
>   get_current_uid_gid helper
> - updated test programs for recursive option

I'm marking this patch series as "deferred" while the semantic issues
keep getting discussed.

Thanks.