On Mon, Apr 08, 2019 at 09:04:30AM -0700, Stanislav Fomichev wrote:
> Add new set of arguments to bpf_attr for BPF_PROG_TEST_RUN:
> * ctx_in/ctx_size_in - input context
> * ctx_out/ctx_size_out - output context
>
> The intended use case is to pass some meta data to the test runs that
> operate on skb (this has being brought up on recent LPC).
>
> For programs that use bpf_prog_test_run_skb, support __sk_buff input and
> output. Initially, from input __sk_buff, copy _only_ cb and priority into
> skb, all other non-zero fields are prohibited (with EINVAL).
> If the user has set ctx_out/ctx_size_out, copy the potentially modified
> __sk_buff back to the userspace.
>
> We require all fields of input __sk_buff except the ones we explicitly
> support to be set to zero. The expectation is that in the future we might
> add support for more fields and we want to fail explicitly if the user
> runs the program on the kernel where we don't yet support them.
>
> The API is intentionally vague (i.e. we don't explicitly add __sk_buff
> to bpf_attr, but ctx_in) to potentially let other test_run types use
> this interface in the future (this can be xdp_md for xdp types for
> example).
>
> v2:
> * Addressed comments from Martin Lau
>
> Cc: Martin Lau <kafai@xxxxxx>
> Signed-off-by: Stanislav Fomichev <sdf@xxxxxxxxxx>
> ---
> include/uapi/linux/bpf.h | 7 ++
> kernel/bpf/syscall.c | 10 ++-
> net/bpf/test_run.c | 135 ++++++++++++++++++++++++++++++++++++---
> 3 files changed, 143 insertions(+), 9 deletions(-)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 837024512baf..8e96f99cebf8 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -396,6 +396,13 @@ union bpf_attr {
> __aligned_u64 data_out;
> __u32 repeat;
> __u32 duration;
> + __u32 ctx_size_in; /* input: len of ctx_in */
> + __u32 ctx_size_out; /* input/output: len of ctx_out
> + * returns ENOSPC if ctx_out
> + * is too small.
> + */
> + __aligned_u64 ctx_in;
> + __aligned_u64 ctx_out;
> } test;
>
> struct { /* anonymous struct used by BPF_*_GET_*_ID */
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 1d65e56594db..5bb963e8f9b0 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -1949,7 +1949,7 @@ static int bpf_prog_query(const union bpf_attr *attr,
> return cgroup_bpf_prog_query(attr, uattr);
> }
>
> -#define BPF_PROG_TEST_RUN_LAST_FIELD test.duration
> +#define BPF_PROG_TEST_RUN_LAST_FIELD test.ctx_out
>
> static int bpf_prog_test_run(const union bpf_attr *attr,
> union bpf_attr __user *uattr)
> @@ -1962,6 +1962,14 @@ static int bpf_prog_test_run(const union bpf_attr *attr,
> if (CHECK_ATTR(BPF_PROG_TEST_RUN))
> return -EINVAL;
>
> + if ((attr->test.ctx_size_in && !attr->test.ctx_in) ||
> + (!attr->test.ctx_size_in && attr->test.ctx_in))
> + return -EINVAL;
> +
> + if ((attr->test.ctx_size_out && !attr->test.ctx_out) ||
> + (!attr->test.ctx_size_out && attr->test.ctx_out))
> + return -EINVAL;
> +
> prog = bpf_prog_get(attr->test.prog_fd);
> if (IS_ERR(prog))
> return PTR_ERR(prog);
> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> index fab142b796ef..3c6e3855c886 100644
> --- a/net/bpf/test_run.c
> +++ b/net/bpf/test_run.c
> @@ -123,12 +123,118 @@ static void *bpf_test_init(const union bpf_attr *kattr, u32 size,
> return data;
> }
>
> +static void *bpf_ctx_init(const union bpf_attr *kattr, u32 max_size)
> +{
> + void __user *data_in = u64_to_user_ptr(kattr->test.ctx_in);
> + u32 size = kattr->test.ctx_size_in;
> + void *data;
> +
> + if (!size)
> + return NULL;
A note for a later comment in bpf_prog_test_run_skb().
This function may return NULL.
> +
> + if (size > max_size)
If should be fine if the kernel unsupported bytes are zeros
(i.e. tailing zeros in data_in).
Please refer to the existing bpf_check_uarg_tail_zero() usages
or also CHECK_ATTR() in syscall.c. Something like this if
bpf_check_uarg_tail_zero() is used,
err = bpf_check_uarg_tail_zero(data_in, max_size, size));
if (err)
return ERR_PTR(err);
> + return ERR_PTR(-E2BIG);
> +
> + data = kzalloc(max_size, GFP_USER);
> + if (!data)
> + return ERR_PTR(-ENOMEM);
> +
> + if (copy_from_user(data, data_in, size)) {
> + kfree(data);
> + return ERR_PTR(-EFAULT);
> + }
> + return data;
> +}
> +
> +static int bpf_ctx_finish(const union bpf_attr *kattr,
> + union bpf_attr __user *uattr, const void *data,
> + u32 size)
> +{
> + void __user *data_out = u64_to_user_ptr(kattr->test.ctx_out);
> + int err = -EFAULT;
> + u32 copy_size = size;
> +
data is not checked for NULL here.
> + if (!kattr->test.ctx_size_out)
> + return 0;
> +
> + if (copy_size > kattr->test.ctx_size_out) {
> + copy_size = kattr->test.ctx_size_out;
> + err = -ENOSPC;
> + }
> +
> + if (copy_to_user(data_out, data, copy_size))
> + goto out;
> + if (copy_to_user(&uattr->test.ctx_size_out, &size, sizeof(size)))
> + goto out;
> + if (err != -ENOSPC)
> + err = 0;
> +out:
> + return err;
> +}
[ ... ]
> int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
> union bpf_attr __user *uattr)
> {
> bool is_l2 = false, is_direct_pkt_access = false;
> u32 size = kattr->test.data_size_in;
> u32 repeat = kattr->test.repeat;
> + struct __sk_buff *ctx = NULL;
> u32 retval, duration;
> int hh_len = ETH_HLEN;
> struct sk_buff *skb;
> @@ -141,6 +247,12 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
> if (IS_ERR(data))
> return PTR_ERR(data);
>
> + ctx = bpf_ctx_init(kattr, sizeof(struct __sk_buff));
> + if (IS_ERR(ctx)) {
> + kfree(data);
> + return PTR_ERR(ctx);
> + }
> +
> switch (prog->type) {
> case BPF_PROG_TYPE_SCHED_CLS:
> case BPF_PROG_TYPE_SCHED_ACT:
> @@ -158,6 +270,7 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
> sk = kzalloc(sizeof(struct sock), GFP_USER);
> if (!sk) {
> kfree(data);
> + kfree(ctx);
> return -ENOMEM;
> }
> sock_net_set(sk, current->nsproxy->net_ns);
> @@ -166,6 +279,7 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
> skb = build_skb(data, 0);
> if (!skb) {
> kfree(data);
> + kfree(ctx);
> kfree(sk);
> return -ENOMEM;
> }
> @@ -180,32 +294,37 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
> __skb_push(skb, hh_len);
> if (is_direct_pkt_access)
> bpf_compute_data_pointers(skb);
> + ret = convert___skb_to_skb(skb, ctx);
> + if (ret)
> + goto out;
> ret = bpf_test_run(prog, skb, repeat, &retval, &duration);
> - if (ret) {
> - kfree_skb(skb);
> - kfree(sk);
> - return ret;
> - }
> + if (ret)
> + goto out;
> if (!is_l2) {
> if (skb_headroom(skb) < hh_len) {
> int nhead = HH_DATA_ALIGN(hh_len - skb_headroom(skb));
>
> if (pskb_expand_head(skb, nhead, 0, GFP_USER)) {
> - kfree_skb(skb);
> - kfree(sk);
> - return -ENOMEM;
> + ret = -ENOMEM;
> + goto out;
> }
> }
> memset(__skb_push(skb, hh_len), 0, hh_len);
> }
> + convert_skb_to___skb(skb, ctx);
>
> size = skb->len;
> /* bpf program can never convert linear skb to non-linear */
> if (WARN_ON_ONCE(skb_is_nonlinear(skb)))
> size = skb_headlen(skb);
> ret = bpf_test_finish(kattr, uattr, skb->data, size, retval, duration);
> + if (!ret)
> + ret = bpf_ctx_finish(kattr, uattr, ctx,
ctx is not checked for NULL in here either.
> + sizeof(struct __sk_buff));
> +out:
> kfree_skb(skb);
> kfree(sk);
> + kfree(ctx);
> return ret;
> }
>
> --
> 2.21.0.392.gf8f6787159e-goog
>
