Message-ID: <e2dd566b-eed3-41e0-fbbd-53d3dbfc9f40@zonque.org>
Date:   Sun, 12 Feb 2017 09:01:31 +0100
From:   Daniel Mack <daniel@...que.org>
To:     Alexei Starovoitov <ast@...com>,
        "David S . Miller" <davem@...emloft.net>
Cc:     Daniel Borkmann <daniel@...earbox.net>,
        David Ahern <dsa@...ulusnetworks.com>,
        Tejun Heo <tj@...nel.org>,
        Andy Lutomirski <luto@...capital.net>, netdev@...r.kernel.org
Subject: Re: [PATCH v2 net] bpf: introduce BPF_F_ALLOW_OVERRIDE flag

On 02/11/2017 05:28 AM, Alexei Starovoitov wrote:
> If BPF_F_ALLOW_OVERRIDE flag is used in BPF_PROG_ATTACH command
> to the given cgroup the descendent cgroup will be able to override
> effective bpf program that was inherited from this cgroup.
> By default it's not passed, therefore override is disallowed.
> 
> Examples:
> 1.
> prog X attached to /A with default
> prog Y fails to attach to /A/B and /A/B/C
> Everything under /A runs prog X
> 
> 2.
> prog X attached to /A with allow_override.
> prog Y fails to attach to /A/B with default (non-override)
> prog M attached to /A/B with allow_override.
> Everything under /A/B runs prog M only.
> 
> 3.
> prog X attached to /A with allow_override.
> prog Y fails to attach to /A with default.
> The user has to detach first to switch the mode.
> 
> In the future this behavior may be extended with a chain of
> non-overridable programs.
> 
> Also fix the bug where detach from cgroup where nothing is attached
> was not throwing error. Return ENOENT in such case.
> 
> Add several testcases and adjust libbpf.
> 
> Fixes: 3007098494be ("cgroup: add support for eBPF programs")
> Signed-off-by: Alexei Starovoitov <ast@...nel.org>

Looks good to me.

Acked-by: Daniel Mack <daniel@...que.org>

Let's get this into 4.10!


Thanks,
Daniel



> ---
> v1->v2: disallowed overridable->non_override transition as suggested by Andy
> added tests and fixed double detach bug
> 
> Andy, Daniel,
> please review and ack quickly, so it can land into 4.10.
> ---
>  include/linux/bpf-cgroup.h       | 13 ++++----
>  include/uapi/linux/bpf.h         |  7 +++++
>  kernel/bpf/cgroup.c              | 59 +++++++++++++++++++++++++++-------
>  kernel/bpf/syscall.c             | 20 ++++++++----
>  kernel/cgroup.c                  |  9 +++---
>  samples/bpf/test_cgrp2_attach.c  |  2 +-
>  samples/bpf/test_cgrp2_attach2.c | 68 +++++++++++++++++++++++++++++++++++++---
>  samples/bpf/test_cgrp2_sock.c    |  2 +-
>  samples/bpf/test_cgrp2_sock2.c   |  2 +-
>  tools/lib/bpf/bpf.c              |  4 ++-
>  tools/lib/bpf/bpf.h              |  3 +-
>  11 files changed, 151 insertions(+), 38 deletions(-)
> 
> diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
> index 92bc89ae7e20..c970a25d2a49 100644
> --- a/include/linux/bpf-cgroup.h
> +++ b/include/linux/bpf-cgroup.h
> @@ -21,20 +21,19 @@ 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];
>  };
>  
>  void cgroup_bpf_put(struct cgroup *cgrp);
>  void cgroup_bpf_inherit(struct cgroup *cgrp, struct cgroup *parent);
>  
> -void __cgroup_bpf_update(struct cgroup *cgrp,
> -			 struct cgroup *parent,
> -			 struct bpf_prog *prog,
> -			 enum bpf_attach_type type);
> +int __cgroup_bpf_update(struct cgroup *cgrp, struct cgroup *parent,
> +			struct bpf_prog *prog, enum bpf_attach_type type,
> +			bool overridable);
>  
>  /* Wrapper for __cgroup_bpf_update() protected by cgroup_mutex */
> -void cgroup_bpf_update(struct cgroup *cgrp,
> -		       struct bpf_prog *prog,
> -		       enum bpf_attach_type type);
> +int cgroup_bpf_update(struct cgroup *cgrp, struct bpf_prog *prog,
> +		      enum bpf_attach_type type, bool overridable);
>  
>  int __cgroup_bpf_run_filter_skb(struct sock *sk,
>  				struct sk_buff *skb,
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index e5b8cf16cbaf..69f65b710b10 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -116,6 +116,12 @@ enum bpf_attach_type {
>  
>  #define MAX_BPF_ATTACH_TYPE __MAX_BPF_ATTACH_TYPE
>  
> +/* If BPF_F_ALLOW_OVERRIDE flag is used in BPF_PROG_ATTACH command
> + * to the given target_fd cgroup the descendent cgroup will be able to
> + * override effective bpf program that was inherited from this cgroup
> + */
> +#define BPF_F_ALLOW_OVERRIDE	(1U << 0)
> +
>  #define BPF_PSEUDO_MAP_FD	1
>  
>  /* flags for BPF_MAP_UPDATE_ELEM command */
> @@ -171,6 +177,7 @@ union bpf_attr {
>  		__u32		target_fd;	/* container object to attach to */
>  		__u32		attach_bpf_fd;	/* eBPF program to attach */
>  		__u32		attach_type;
> +		__u32		attach_flags;
>  	};
>  } __attribute__((aligned(8)));
>  
> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index a515f7b007c6..da0f53690295 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -52,6 +52,7 @@ void cgroup_bpf_inherit(struct cgroup *cgrp, struct cgroup *parent)
>  		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];
>  	}
>  }
>  
> @@ -82,30 +83,63 @@ void cgroup_bpf_inherit(struct cgroup *cgrp, struct cgroup *parent)
>   *
>   * Must be called with cgroup_mutex held.
>   */
> -void __cgroup_bpf_update(struct cgroup *cgrp,
> -			 struct cgroup *parent,
> -			 struct bpf_prog *prog,
> -			 enum bpf_attach_type type)
> +int __cgroup_bpf_update(struct cgroup *cgrp, struct cgroup *parent,
> +			struct bpf_prog *prog, enum bpf_attach_type type,
> +			bool new_overridable)
>  {
> -	struct bpf_prog *old_prog, *effective;
> +	struct bpf_prog *old_prog, *effective = NULL;
>  	struct cgroup_subsys_state *pos;
> +	bool overridable = true;
>  
> -	old_prog = xchg(cgrp->bpf.prog + type, prog);
> +	if (parent) {
> +		overridable = !parent->bpf.disallow_override[type];
> +		effective = rcu_dereference_protected(parent->bpf.effective[type],
> +						      lockdep_is_held(&cgroup_mutex));
> +	}
> +
> +	if (prog && effective && !overridable)
> +		/* if parent has non-overridable prog attached, disallow
> +		 * attaching new programs to descendent cgroup
> +		 */
> +		return -EPERM;
> +
> +	if (prog && effective && overridable != new_overridable)
> +		/* if parent has overridable prog attached, only
> +		 * allow overridable programs in descendent cgroup
> +		 */
> +		return -EPERM;
>  
> -	effective = (!prog && parent) ?
> -		rcu_dereference_protected(parent->bpf.effective[type],
> -					  lockdep_is_held(&cgroup_mutex)) :
> -		prog;
> +	old_prog = cgrp->bpf.prog[type];
> +
> +	if (prog) {
> +		overridable = new_overridable;
> +		effective = prog;
> +		if (old_prog &&
> +		    cgrp->bpf.disallow_override[type] == new_overridable)
> +			/* disallow attaching non-overridable on top
> +			 * of existing overridable in this cgroup
> +			 * and vice versa
> +			 */
> +			return -EPERM;
> +	}
> +
> +	if (!prog && !old_prog)
> +		/* report error when trying to detach and nothing is attached */
> +		return -ENOENT;
> +
> +	cgrp->bpf.prog[type] = prog;
>  
>  	css_for_each_descendant_pre(pos, &cgrp->self) {
>  		struct cgroup *desc = container_of(pos, struct cgroup, self);
>  
>  		/* skip the subtree if the descendant has its own program */
> -		if (desc->bpf.prog[type] && desc != cgrp)
> +		if (desc->bpf.prog[type] && desc != cgrp) {
>  			pos = css_rightmost_descendant(pos);
> -		else
> +		} else {
>  			rcu_assign_pointer(desc->bpf.effective[type],
>  					   effective);
> +			desc->bpf.disallow_override[type] = !overridable;
> +		}
>  	}
>  
>  	if (prog)
> @@ -115,6 +149,7 @@ void __cgroup_bpf_update(struct cgroup *cgrp,
>  		bpf_prog_put(old_prog);
>  		static_branch_dec(&cgroup_bpf_enabled_key);
>  	}
> +	return 0;
>  }
>  
>  /**
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 19b6129eab23..bbb016adbaeb 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -920,13 +920,14 @@ static int bpf_obj_get(const union bpf_attr *attr)
>  
>  #ifdef CONFIG_CGROUP_BPF
>  
> -#define BPF_PROG_ATTACH_LAST_FIELD attach_type
> +#define BPF_PROG_ATTACH_LAST_FIELD attach_flags
>  
>  static int bpf_prog_attach(const union bpf_attr *attr)
>  {
> +	enum bpf_prog_type ptype;
>  	struct bpf_prog *prog;
>  	struct cgroup *cgrp;
> -	enum bpf_prog_type ptype;
> +	int ret;
>  
>  	if (!capable(CAP_NET_ADMIN))
>  		return -EPERM;
> @@ -934,6 +935,9 @@ 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)
> +		return -EINVAL;
> +
>  	switch (attr->attach_type) {
>  	case BPF_CGROUP_INET_INGRESS:
>  	case BPF_CGROUP_INET_EGRESS:
> @@ -956,10 +960,13 @@ static int bpf_prog_attach(const union bpf_attr *attr)
>  		return PTR_ERR(cgrp);
>  	}
>  
> -	cgroup_bpf_update(cgrp, prog, attr->attach_type);
> +	ret = cgroup_bpf_update(cgrp, prog, attr->attach_type,
> +				attr->attach_flags & BPF_F_ALLOW_OVERRIDE);
> +	if (ret)
> +		bpf_prog_put(prog);
>  	cgroup_put(cgrp);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  #define BPF_PROG_DETACH_LAST_FIELD attach_type
> @@ -967,6 +974,7 @@ static int bpf_prog_attach(const union bpf_attr *attr)
>  static int bpf_prog_detach(const union bpf_attr *attr)
>  {
>  	struct cgroup *cgrp;
> +	int ret;
>  
>  	if (!capable(CAP_NET_ADMIN))
>  		return -EPERM;
> @@ -982,7 +990,7 @@ static int bpf_prog_detach(const union bpf_attr *attr)
>  		if (IS_ERR(cgrp))
>  			return PTR_ERR(cgrp);
>  
> -		cgroup_bpf_update(cgrp, NULL, attr->attach_type);
> +		ret = cgroup_bpf_update(cgrp, NULL, attr->attach_type, false);
>  		cgroup_put(cgrp);
>  		break;
>  
> @@ -990,7 +998,7 @@ static int bpf_prog_detach(const union bpf_attr *attr)
>  		return -EINVAL;
>  	}
>  
> -	return 0;
> +	return ret;
>  }
>  #endif /* CONFIG_CGROUP_BPF */
>  
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 688dd02af985..53bbca7c4859 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -6498,15 +6498,16 @@ static __init int cgroup_namespaces_init(void)
>  subsys_initcall(cgroup_namespaces_init);
>  
>  #ifdef CONFIG_CGROUP_BPF
> -void cgroup_bpf_update(struct cgroup *cgrp,
> -		       struct bpf_prog *prog,
> -		       enum bpf_attach_type type)
> +int cgroup_bpf_update(struct cgroup *cgrp, struct bpf_prog *prog,
> +		      enum bpf_attach_type type, bool overridable)
>  {
>  	struct cgroup *parent = cgroup_parent(cgrp);
> +	int ret;
>  
>  	mutex_lock(&cgroup_mutex);
> -	__cgroup_bpf_update(cgrp, parent, prog, type);
> +	ret = __cgroup_bpf_update(cgrp, parent, prog, type, overridable);
>  	mutex_unlock(&cgroup_mutex);
> +	return ret;
>  }
>  #endif /* CONFIG_CGROUP_BPF */
>  
> diff --git a/samples/bpf/test_cgrp2_attach.c b/samples/bpf/test_cgrp2_attach.c
> index 504058631ffc..4bfcaf93fcf3 100644
> --- a/samples/bpf/test_cgrp2_attach.c
> +++ b/samples/bpf/test_cgrp2_attach.c
> @@ -104,7 +104,7 @@ static int attach_filter(int cg_fd, int type, int verdict)
>  		return EXIT_FAILURE;
>  	}
>  
> -	ret = bpf_prog_attach(prog_fd, cg_fd, type);
> +	ret = bpf_prog_attach(prog_fd, cg_fd, type, 0);
>  	if (ret < 0) {
>  		printf("Failed to attach prog to cgroup: '%s'\n",
>  		       strerror(errno));
> diff --git a/samples/bpf/test_cgrp2_attach2.c b/samples/bpf/test_cgrp2_attach2.c
> index 6e69be37f87f..3049b1f26267 100644
> --- a/samples/bpf/test_cgrp2_attach2.c
> +++ b/samples/bpf/test_cgrp2_attach2.c
> @@ -79,11 +79,12 @@ int main(int argc, char **argv)
>  	if (join_cgroup(FOO))
>  		goto err;
>  
> -	if (bpf_prog_attach(drop_prog, foo, BPF_CGROUP_INET_EGRESS)) {
> +	if (bpf_prog_attach(drop_prog, foo, BPF_CGROUP_INET_EGRESS, 1)) {
>  		log_err("Attaching prog to /foo");
>  		goto err;
>  	}
>  
> +	printf("Attached DROP prog. This ping in cgroup /foo should fail...\n");
>  	assert(system(PING_CMD) != 0);
>  
>  	/* Create cgroup /foo/bar, get fd, and join it */
> @@ -94,24 +95,27 @@ int main(int argc, char **argv)
>  	if (join_cgroup(BAR))
>  		goto err;
>  
> +	printf("Attached DROP prog. This ping in cgroup /foo/bar should fail...\n");
>  	assert(system(PING_CMD) != 0);
>  
> -	if (bpf_prog_attach(allow_prog, bar, BPF_CGROUP_INET_EGRESS)) {
> +	if (bpf_prog_attach(allow_prog, bar, BPF_CGROUP_INET_EGRESS, 1)) {
>  		log_err("Attaching prog to /foo/bar");
>  		goto err;
>  	}
>  
> +	printf("Attached PASS prog. This ping in cgroup /foo/bar should pass...\n");
>  	assert(system(PING_CMD) == 0);
>  
> -
>  	if (bpf_prog_detach(bar, BPF_CGROUP_INET_EGRESS)) {
>  		log_err("Detaching program from /foo/bar");
>  		goto err;
>  	}
>  
> +	printf("Detached PASS from /foo/bar while DROP is attached to /foo.\n"
> +	       "This ping in cgroup /foo/bar should fail...\n");
>  	assert(system(PING_CMD) != 0);
>  
> -	if (bpf_prog_attach(allow_prog, bar, BPF_CGROUP_INET_EGRESS)) {
> +	if (bpf_prog_attach(allow_prog, bar, BPF_CGROUP_INET_EGRESS, 1)) {
>  		log_err("Attaching prog to /foo/bar");
>  		goto err;
>  	}
> @@ -121,8 +125,60 @@ int main(int argc, char **argv)
>  		goto err;
>  	}
>  
> +	printf("Attached PASS from /foo/bar and detached DROP from /foo.\n"
> +	       "This ping in cgroup /foo/bar should pass...\n");
>  	assert(system(PING_CMD) == 0);
>  
> +	if (bpf_prog_attach(allow_prog, bar, BPF_CGROUP_INET_EGRESS, 1)) {
> +		log_err("Attaching prog to /foo/bar");
> +		goto err;
> +	}
> +
> +	if (!bpf_prog_attach(allow_prog, bar, BPF_CGROUP_INET_EGRESS, 0)) {
> +		errno = 0;
> +		log_err("Unexpected success attaching prog to /foo/bar");
> +		goto err;
> +	}
> +
> +	if (bpf_prog_detach(bar, BPF_CGROUP_INET_EGRESS)) {
> +		log_err("Detaching program from /foo/bar");
> +		goto err;
> +	}
> +
> +	if (!bpf_prog_detach(foo, BPF_CGROUP_INET_EGRESS)) {
> +		errno = 0;
> +		log_err("Unexpected success in double detach from /foo");
> +		goto err;
> +	}
> +
> +	if (bpf_prog_attach(allow_prog, foo, BPF_CGROUP_INET_EGRESS, 0)) {
> +		log_err("Attaching non-overridable prog to /foo");
> +		goto err;
> +	}
> +
> +	if (!bpf_prog_attach(allow_prog, bar, BPF_CGROUP_INET_EGRESS, 0)) {
> +		errno = 0;
> +		log_err("Unexpected success attaching non-overridable prog to /foo/bar");
> +		goto err;
> +	}
> +
> +	if (!bpf_prog_attach(allow_prog, bar, BPF_CGROUP_INET_EGRESS, 1)) {
> +		errno = 0;
> +		log_err("Unexpected success attaching overridable prog to /foo/bar");
> +		goto err;
> +	}
> +
> +	if (!bpf_prog_attach(allow_prog, foo, BPF_CGROUP_INET_EGRESS, 1)) {
> +		errno = 0;
> +		log_err("Unexpected success attaching overridable prog to /foo");
> +		goto err;
> +	}
> +
> +	if (bpf_prog_attach(drop_prog, foo, BPF_CGROUP_INET_EGRESS, 0)) {
> +		log_err("Attaching different non-overridable prog to /foo");
> +		goto err;
> +	}
> +
>  	goto out;
>  
>  err:
> @@ -132,5 +188,9 @@ int main(int argc, char **argv)
>  	close(foo);
>  	close(bar);
>  	cleanup_cgroup_environment();
> +	if (!rc)
> +		printf("PASS\n");
> +	else
> +		printf("FAIL\n");
>  	return rc;
>  }
> diff --git a/samples/bpf/test_cgrp2_sock.c b/samples/bpf/test_cgrp2_sock.c
> index 0791b949cbe4..c3cfb23e23b5 100644
> --- a/samples/bpf/test_cgrp2_sock.c
> +++ b/samples/bpf/test_cgrp2_sock.c
> @@ -75,7 +75,7 @@ int main(int argc, char **argv)
>  		return EXIT_FAILURE;
>  	}
>  
> -	ret = bpf_prog_attach(prog_fd, cg_fd, BPF_CGROUP_INET_SOCK_CREATE);
> +	ret = bpf_prog_attach(prog_fd, cg_fd, BPF_CGROUP_INET_SOCK_CREATE, 0);
>  	if (ret < 0) {
>  		printf("Failed to attach prog to cgroup: '%s'\n",
>  		       strerror(errno));
> diff --git a/samples/bpf/test_cgrp2_sock2.c b/samples/bpf/test_cgrp2_sock2.c
> index 455ef0d06e93..db036077b644 100644
> --- a/samples/bpf/test_cgrp2_sock2.c
> +++ b/samples/bpf/test_cgrp2_sock2.c
> @@ -55,7 +55,7 @@ int main(int argc, char **argv)
>  	}
>  
>  	ret = bpf_prog_attach(prog_fd[filter_id], cg_fd,
> -			      BPF_CGROUP_INET_SOCK_CREATE);
> +			      BPF_CGROUP_INET_SOCK_CREATE, 0);
>  	if (ret < 0) {
>  		printf("Failed to attach prog to cgroup: '%s'\n",
>  		       strerror(errno));
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index 3ddb58a36d3c..ae752fa4eaa7 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -168,7 +168,8 @@ int bpf_obj_get(const char *pathname)
>  	return sys_bpf(BPF_OBJ_GET, &attr, sizeof(attr));
>  }
>  
> -int bpf_prog_attach(int prog_fd, int target_fd, enum bpf_attach_type type)
> +int bpf_prog_attach(int prog_fd, int target_fd, enum bpf_attach_type type,
> +		    unsigned int flags)
>  {
>  	union bpf_attr attr;
>  
> @@ -176,6 +177,7 @@ int bpf_prog_attach(int prog_fd, int target_fd, enum bpf_attach_type type)
>  	attr.target_fd	   = target_fd;
>  	attr.attach_bpf_fd = prog_fd;
>  	attr.attach_type   = type;
> +	attr.attach_flags  = flags;
>  
>  	return sys_bpf(BPF_PROG_ATTACH, &attr, sizeof(attr));
>  }
> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> index a2f9853dd882..4ac6c4b84100 100644
> --- a/tools/lib/bpf/bpf.h
> +++ b/tools/lib/bpf/bpf.h
> @@ -41,7 +41,8 @@ int bpf_map_delete_elem(int fd, void *key);
>  int bpf_map_get_next_key(int fd, void *key, void *next_key);
>  int bpf_obj_pin(int fd, const char *pathname);
>  int bpf_obj_get(const char *pathname);
> -int bpf_prog_attach(int prog_fd, int attachable_fd, enum bpf_attach_type type);
> +int bpf_prog_attach(int prog_fd, int attachable_fd, enum bpf_attach_type type,
> +		    unsigned int flags);
>  int bpf_prog_detach(int attachable_fd, enum bpf_attach_type type);
>  
>  
> 

Powered by blists - more mailing lists