diff mbox series

[v2,bpf-next,1/2] bpf: add cg_skb_is_valid_access for BPF_PROG_TYPE_CGROUP_SKB

Message ID 20181017233616.3130909-2-songliubraving@fb.com
State Changes Requested, archived
Delegated to: BPF Maintainers
Headers show
Series bpf: add cg_skb_is_valid_access | expand

Commit Message

Song Liu Oct. 17, 2018, 11:36 p.m. UTC
BPF programs of BPF_PROG_TYPE_CGROUP_SKB need to access headers in the
skb. This patch enables direct access of skb for these programs.

In __cgroup_bpf_run_filter_skb(), bpf_compute_data_pointers() is called
to compute proper data_end for the BPF program.

Signed-off-by: Song Liu <songliubraving@fb.com>
---
 kernel/bpf/cgroup.c |  4 ++++
 net/core/filter.c   | 36 +++++++++++++++++++++++++++++++++++-
 2 files changed, 39 insertions(+), 1 deletion(-)

Comments

Alexei Starovoitov Oct. 18, 2018, 4:44 a.m. UTC | #1
On Wed, Oct 17, 2018 at 04:36:15PM -0700, Song Liu wrote:
> BPF programs of BPF_PROG_TYPE_CGROUP_SKB need to access headers in the
> skb. This patch enables direct access of skb for these programs.
> 
> In __cgroup_bpf_run_filter_skb(), bpf_compute_data_pointers() is called
> to compute proper data_end for the BPF program.
> 
> Signed-off-by: Song Liu <songliubraving@fb.com>
> ---
>  kernel/bpf/cgroup.c |  4 ++++
>  net/core/filter.c   | 36 +++++++++++++++++++++++++++++++++++-
>  2 files changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index 00f6ed2e4f9a..340d496f35bd 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -566,6 +566,10 @@ int __cgroup_bpf_run_filter_skb(struct sock *sk,
>  	save_sk = skb->sk;
>  	skb->sk = sk;
>  	__skb_push(skb, offset);
> +
> +	/* compute pointers for the bpf prog */
> +	bpf_compute_data_pointers(skb);

cg_skb_is_valid_access() below looks good to me now,
but I just realized that above change is not safe for all sockets.
After sk_filter_trim_cap() is called in udp_queue_rcv_skb()
it needs to see valid UDP_SKB_CB.
But sizeof(struct udp_skb_cb)==28, so bpf_compute_data_pointers()
would mangle the end of it.
So we have to save/restore data_end/data_meta pointers as well.

I'm thinking that new helper like:
  bpf_compute_and_save_data_pointers(skb, &buffer_of_16_bytes);
  BPF_PROG_RUN_ARRAY();
  bpf_restore_data_pointers(skb, &buffer_of_16_bytes);
would be decent interface.
Song Liu Oct. 18, 2018, 5:06 a.m. UTC | #2
> On Oct 17, 2018, at 9:44 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> 
> On Wed, Oct 17, 2018 at 04:36:15PM -0700, Song Liu wrote:
>> BPF programs of BPF_PROG_TYPE_CGROUP_SKB need to access headers in the
>> skb. This patch enables direct access of skb for these programs.
>> 
>> In __cgroup_bpf_run_filter_skb(), bpf_compute_data_pointers() is called
>> to compute proper data_end for the BPF program.
>> 
>> Signed-off-by: Song Liu <songliubraving@fb.com>
>> ---
>> kernel/bpf/cgroup.c |  4 ++++
>> net/core/filter.c   | 36 +++++++++++++++++++++++++++++++++++-
>> 2 files changed, 39 insertions(+), 1 deletion(-)
>> 
>> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
>> index 00f6ed2e4f9a..340d496f35bd 100644
>> --- a/kernel/bpf/cgroup.c
>> +++ b/kernel/bpf/cgroup.c
>> @@ -566,6 +566,10 @@ int __cgroup_bpf_run_filter_skb(struct sock *sk,
>> 	save_sk = skb->sk;
>> 	skb->sk = sk;
>> 	__skb_push(skb, offset);
>> +
>> +	/* compute pointers for the bpf prog */
>> +	bpf_compute_data_pointers(skb);
> 
> cg_skb_is_valid_access() below looks good to me now,
> but I just realized that above change is not safe for all sockets.
> After sk_filter_trim_cap() is called in udp_queue_rcv_skb()
> it needs to see valid UDP_SKB_CB.
> But sizeof(struct udp_skb_cb)==28, so bpf_compute_data_pointers()
> would mangle the end of it.
> So we have to save/restore data_end/data_meta pointers as well.
> 
> I'm thinking that new helper like:
>  bpf_compute_and_save_data_pointers(skb, &buffer_of_16_bytes);
>  BPF_PROG_RUN_ARRAY();
>  bpf_restore_data_pointers(skb, &buffer_of_16_bytes);
> would be decent interface.

Thanks Alexei!

Will send v3 shortly.

Song
diff mbox series

Patch

diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index 00f6ed2e4f9a..340d496f35bd 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -566,6 +566,10 @@  int __cgroup_bpf_run_filter_skb(struct sock *sk,
 	save_sk = skb->sk;
 	skb->sk = sk;
 	__skb_push(skb, offset);
+
+	/* compute pointers for the bpf prog */
+	bpf_compute_data_pointers(skb);
+
 	ret = BPF_PROG_RUN_ARRAY(cgrp->bpf.effective[type], skb,
 				 bpf_prog_run_save_cb);
 	__skb_pull(skb, offset);
diff --git a/net/core/filter.c b/net/core/filter.c
index 1a3ac6c46873..e3ca30bd6840 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5346,6 +5346,40 @@  static bool sk_filter_is_valid_access(int off, int size,
 	return bpf_skb_is_valid_access(off, size, type, prog, info);
 }
 
+static bool cg_skb_is_valid_access(int off, int size,
+				   enum bpf_access_type type,
+				   const struct bpf_prog *prog,
+				   struct bpf_insn_access_aux *info)
+{
+	switch (off) {
+	case bpf_ctx_range(struct __sk_buff, tc_classid):
+	case bpf_ctx_range(struct __sk_buff, data_meta):
+	case bpf_ctx_range(struct __sk_buff, flow_keys):
+		return false;
+	}
+	if (type == BPF_WRITE) {
+		switch (off) {
+		case bpf_ctx_range(struct __sk_buff, mark):
+		case bpf_ctx_range(struct __sk_buff, priority):
+		case bpf_ctx_range_till(struct __sk_buff, cb[0], cb[4]):
+			break;
+		default:
+			return false;
+		}
+	}
+
+	switch (off) {
+	case bpf_ctx_range(struct __sk_buff, data):
+		info->reg_type = PTR_TO_PACKET;
+		break;
+	case bpf_ctx_range(struct __sk_buff, data_end):
+		info->reg_type = PTR_TO_PACKET_END;
+		break;
+	}
+
+	return bpf_skb_is_valid_access(off, size, type, prog, info);
+}
+
 static bool lwt_is_valid_access(int off, int size,
 				enum bpf_access_type type,
 				const struct bpf_prog *prog,
@@ -7038,7 +7072,7 @@  const struct bpf_prog_ops xdp_prog_ops = {
 
 const struct bpf_verifier_ops cg_skb_verifier_ops = {
 	.get_func_proto		= cg_skb_func_proto,
-	.is_valid_access	= sk_filter_is_valid_access,
+	.is_valid_access	= cg_skb_is_valid_access,
 	.convert_ctx_access	= bpf_convert_ctx_access,
 };