* [PATCH bpf 1/2] bpf: enforce that struct_ops programs be GPL-only
@ 2021-03-25 15:40 Toke Høiland-Jørgensen
2021-03-25 15:40 ` [PATCH bpf 2/2] bpf/selftests: test that kernel rejects a TCP CC with an invalid license Toke Høiland-Jørgensen
2021-03-25 18:15 ` [PATCH bpf 1/2] bpf: enforce that struct_ops programs be GPL-only Martin KaFai Lau
0 siblings, 2 replies; 6+ messages in thread
From: Toke Høiland-Jørgensen @ 2021-03-25 15:40 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann
Cc: Toke Høiland-Jørgensen, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, David S. Miller,
Jesper Dangaard Brouer, Andrea Arcangeli, Clark Williams, bpf,
netdev
With the introduction of the struct_ops program type, it became possible to
implement kernel functionality in BPF, making it viable to use BPF in place
of a regular kernel module for these particular operations.
Thus far, the only user of this mechanism is for implementing TCP
congestion control algorithms. These are clearly marked as GPL-only when
implemented as modules (as seen by the use of EXPORT_SYMBOL_GPL for
tcp_register_congestion_control()), so it seems like an oversight that this
was not carried over to BPF implementations. And sine this is the only user
of the struct_ops mechanism, just enforcing GPL-only for the struct_ops
program type seems like the simplest way to fix this.
Fixes: 0baf26b0fcd7 ("bpf: tcp: Support tcp_congestion_ops in bpf")
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
kernel/bpf/verifier.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 44e4ec1640f1..48dd0c0f087c 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -12166,6 +12166,11 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
return -ENOTSUPP;
}
+ if (!prog->gpl_compatible) {
+ verbose(env, "struct ops programs must have a GPL compatible license\n");
+ return -EINVAL;
+ }
+
t = st_ops->type;
member_idx = prog->expected_attach_type;
if (member_idx >= btf_type_vlen(t)) {
--
2.31.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH bpf 2/2] bpf/selftests: test that kernel rejects a TCP CC with an invalid license
2021-03-25 15:40 [PATCH bpf 1/2] bpf: enforce that struct_ops programs be GPL-only Toke Høiland-Jørgensen
@ 2021-03-25 15:40 ` Toke Høiland-Jørgensen
2021-03-25 18:32 ` Martin KaFai Lau
2021-03-25 18:15 ` [PATCH bpf 1/2] bpf: enforce that struct_ops programs be GPL-only Martin KaFai Lau
1 sibling, 1 reply; 6+ messages in thread
From: Toke Høiland-Jørgensen @ 2021-03-25 15:40 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann
Cc: Toke Høiland-Jørgensen, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, David S. Miller,
Jesper Dangaard Brouer, Andrea Arcangeli, Clark Williams, bpf,
netdev
This adds a selftest to check that the verifier rejects a TCP CC struct_ops
with a non-GPL license. To save having to add a whole new BPF object just
for this, reuse the dctcp CC, but rewrite the license field before loading.
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
.../selftests/bpf/prog_tests/bpf_tcp_ca.c | 31 +++++++++++++++++++
1 file changed, 31 insertions(+)
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
index 37c5494a0381..613cf8a00b22 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
@@ -227,10 +227,41 @@ static void test_dctcp(void)
bpf_dctcp__destroy(dctcp_skel);
}
+static void test_invalid_license(void)
+{
+ /* We want to check that the verifier refuses to load a non-GPL TCP CC.
+ * Rather than create a whole new file+skeleton, just reuse an existing
+ * object and rewrite the license in memory after loading. Sine libbpf
+ * doesn't expose this, we define a struct that includes the first couple
+ * of internal fields for struct bpf_object so we can overwrite the right
+ * bits. Yes, this is a bit of a hack, but it makes the test a lot simpler.
+ */
+ struct bpf_object_fragment {
+ char name[BPF_OBJ_NAME_LEN];
+ char license[64];
+ } *obj;
+ struct bpf_dctcp *skel;
+ int err;
+
+ skel = bpf_dctcp__open();
+ if (CHECK(!skel, "bpf_dctcp__open", "failed\n"))
+ return;
+
+ obj = (void *)skel->obj;
+ obj->license[0] = 'X'; // turns 'GPL' into 'XPL' which will fail the check
+
+ err = bpf_dctcp__load(skel);
+ CHECK(err != -LIBBPF_ERRNO__VERIFY, "bpf_dctcp__load", "err:%d\n", err);
+
+ bpf_dctcp__destroy(skel);
+}
+
void test_bpf_tcp_ca(void)
{
if (test__start_subtest("dctcp"))
test_dctcp();
if (test__start_subtest("cubic"))
test_cubic();
+ if (test__start_subtest("invalid_license"))
+ test_invalid_license();
}
--
2.31.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH bpf 2/2] bpf/selftests: test that kernel rejects a TCP CC with an invalid license
2021-03-25 15:40 ` [PATCH bpf 2/2] bpf/selftests: test that kernel rejects a TCP CC with an invalid license Toke Høiland-Jørgensen
@ 2021-03-25 18:32 ` Martin KaFai Lau
2021-03-25 20:38 ` Toke Høiland-Jørgensen
0 siblings, 1 reply; 6+ messages in thread
From: Martin KaFai Lau @ 2021-03-25 18:32 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
David S. Miller, Jesper Dangaard Brouer, Andrea Arcangeli,
Clark Williams, bpf, netdev
On Thu, Mar 25, 2021 at 04:40:34PM +0100, Toke Høiland-Jørgensen wrote:
> This adds a selftest to check that the verifier rejects a TCP CC struct_ops
> with a non-GPL license. To save having to add a whole new BPF object just
> for this, reuse the dctcp CC, but rewrite the license field before loading.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
> .../selftests/bpf/prog_tests/bpf_tcp_ca.c | 31 +++++++++++++++++++
> 1 file changed, 31 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
> index 37c5494a0381..613cf8a00b22 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
> @@ -227,10 +227,41 @@ static void test_dctcp(void)
> bpf_dctcp__destroy(dctcp_skel);
> }
>
> +static void test_invalid_license(void)
> +{
> + /* We want to check that the verifier refuses to load a non-GPL TCP CC.
> + * Rather than create a whole new file+skeleton, just reuse an existing
> + * object and rewrite the license in memory after loading. Sine libbpf
> + * doesn't expose this, we define a struct that includes the first couple
> + * of internal fields for struct bpf_object so we can overwrite the right
> + * bits. Yes, this is a bit of a hack, but it makes the test a lot simpler.
> + */
> + struct bpf_object_fragment {
> + char name[BPF_OBJ_NAME_LEN];
> + char license[64];
> + } *obj;
It is fragile. A new bpf_nogpltcp.c should be created and it does
not have to be a full tcp-cc. A very minimal implementation with
only .init. Something like this (uncompiled code):
char _license[] SEC("license") = "X";
void BPF_STRUCT_OPS(nogpltcp_init, struct sock *sk)
{
}
SEC(".struct_ops")
struct tcp_congestion_ops bpf_nogpltcp = {
.init = (void *)nogpltcp_init,
.name = "bpf_nogpltcp",
};
libbpf_set_print() can also be used to look for the
the verifier log "struct ops programs must have a GPL compatible license".
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH bpf 2/2] bpf/selftests: test that kernel rejects a TCP CC with an invalid license
2021-03-25 18:32 ` Martin KaFai Lau
@ 2021-03-25 20:38 ` Toke Høiland-Jørgensen
0 siblings, 0 replies; 6+ messages in thread
From: Toke Høiland-Jørgensen @ 2021-03-25 20:38 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
David S. Miller, Jesper Dangaard Brouer, Andrea Arcangeli,
Clark Williams, bpf, netdev
Martin KaFai Lau <kafai@fb.com> writes:
> On Thu, Mar 25, 2021 at 04:40:34PM +0100, Toke Høiland-Jørgensen wrote:
>> This adds a selftest to check that the verifier rejects a TCP CC struct_ops
>> with a non-GPL license. To save having to add a whole new BPF object just
>> for this, reuse the dctcp CC, but rewrite the license field before loading.
>>
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> ---
>> .../selftests/bpf/prog_tests/bpf_tcp_ca.c | 31 +++++++++++++++++++
>> 1 file changed, 31 insertions(+)
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
>> index 37c5494a0381..613cf8a00b22 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
>> @@ -227,10 +227,41 @@ static void test_dctcp(void)
>> bpf_dctcp__destroy(dctcp_skel);
>> }
>>
>> +static void test_invalid_license(void)
>> +{
>> + /* We want to check that the verifier refuses to load a non-GPL TCP CC.
>> + * Rather than create a whole new file+skeleton, just reuse an existing
>> + * object and rewrite the license in memory after loading. Sine libbpf
>> + * doesn't expose this, we define a struct that includes the first couple
>> + * of internal fields for struct bpf_object so we can overwrite the right
>> + * bits. Yes, this is a bit of a hack, but it makes the test a lot simpler.
>> + */
>> + struct bpf_object_fragment {
>> + char name[BPF_OBJ_NAME_LEN];
>> + char license[64];
>> + } *obj;
> It is fragile. A new bpf_nogpltcp.c should be created and it does
> not have to be a full tcp-cc. A very minimal implementation with
> only .init. Something like this (uncompiled code):
>
> char _license[] SEC("license") = "X";
>
> void BPF_STRUCT_OPS(nogpltcp_init, struct sock *sk)
> {
> }
>
> SEC(".struct_ops")
> struct tcp_congestion_ops bpf_nogpltcp = {
> .init = (void *)nogpltcp_init,
> .name = "bpf_nogpltcp",
> };
>
> libbpf_set_print() can also be used to look for the
> the verifier log "struct ops programs must have a GPL compatible license".
Ah, thanks for the pointers! Will fix this up as well...
-Toke
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf 1/2] bpf: enforce that struct_ops programs be GPL-only
2021-03-25 15:40 [PATCH bpf 1/2] bpf: enforce that struct_ops programs be GPL-only Toke Høiland-Jørgensen
2021-03-25 15:40 ` [PATCH bpf 2/2] bpf/selftests: test that kernel rejects a TCP CC with an invalid license Toke Høiland-Jørgensen
@ 2021-03-25 18:15 ` Martin KaFai Lau
2021-03-25 20:37 ` Toke Høiland-Jørgensen
1 sibling, 1 reply; 6+ messages in thread
From: Martin KaFai Lau @ 2021-03-25 18:15 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
David S. Miller, Jesper Dangaard Brouer, Andrea Arcangeli,
Clark Williams, bpf, netdev
On Thu, Mar 25, 2021 at 04:40:33PM +0100, Toke Høiland-Jørgensen wrote:
> With the introduction of the struct_ops program type, it became possible to
> implement kernel functionality in BPF, making it viable to use BPF in place
> of a regular kernel module for these particular operations.
>
> Thus far, the only user of this mechanism is for implementing TCP
> congestion control algorithms. These are clearly marked as GPL-only when
> implemented as modules (as seen by the use of EXPORT_SYMBOL_GPL for
> tcp_register_congestion_control()), so it seems like an oversight that this
> was not carried over to BPF implementations. And sine this is the only user
> of the struct_ops mechanism, just enforcing GPL-only for the struct_ops
> program type seems like the simplest way to fix this.
>
> Fixes: 0baf26b0fcd7 ("bpf: tcp: Support tcp_congestion_ops in bpf")
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
> kernel/bpf/verifier.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 44e4ec1640f1..48dd0c0f087c 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -12166,6 +12166,11 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
> return -ENOTSUPP;
> }
>
> + if (!prog->gpl_compatible) {
> + verbose(env, "struct ops programs must have a GPL compatible license\n");
> + return -EINVAL;
> + }
> +
Thanks for the patch.
A nit. Instead of sitting in between of the attach_btf_id check
and expected_attach_type check, how about moving it to the beginning
of this function. Checking attach_btf_id and expected_attach_type
would make more sense to be done next to each other as in the current
code.
Acked-by: Martin KaFai Lau <kafai@fb.com>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH bpf 1/2] bpf: enforce that struct_ops programs be GPL-only
2021-03-25 18:15 ` [PATCH bpf 1/2] bpf: enforce that struct_ops programs be GPL-only Martin KaFai Lau
@ 2021-03-25 20:37 ` Toke Høiland-Jørgensen
0 siblings, 0 replies; 6+ messages in thread
From: Toke Høiland-Jørgensen @ 2021-03-25 20:37 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
David S. Miller, Jesper Dangaard Brouer, Andrea Arcangeli,
Clark Williams, bpf, netdev
Martin KaFai Lau <kafai@fb.com> writes:
> On Thu, Mar 25, 2021 at 04:40:33PM +0100, Toke Høiland-Jørgensen wrote:
>> With the introduction of the struct_ops program type, it became possible to
>> implement kernel functionality in BPF, making it viable to use BPF in place
>> of a regular kernel module for these particular operations.
>>
>> Thus far, the only user of this mechanism is for implementing TCP
>> congestion control algorithms. These are clearly marked as GPL-only when
>> implemented as modules (as seen by the use of EXPORT_SYMBOL_GPL for
>> tcp_register_congestion_control()), so it seems like an oversight that this
>> was not carried over to BPF implementations. And sine this is the only user
>> of the struct_ops mechanism, just enforcing GPL-only for the struct_ops
>> program type seems like the simplest way to fix this.
>>
>> Fixes: 0baf26b0fcd7 ("bpf: tcp: Support tcp_congestion_ops in bpf")
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> ---
>> kernel/bpf/verifier.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 44e4ec1640f1..48dd0c0f087c 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -12166,6 +12166,11 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
>> return -ENOTSUPP;
>> }
>>
>> + if (!prog->gpl_compatible) {
>> + verbose(env, "struct ops programs must have a GPL compatible license\n");
>> + return -EINVAL;
>> + }
>> +
> Thanks for the patch.
>
> A nit. Instead of sitting in between of the attach_btf_id check
> and expected_attach_type check, how about moving it to the beginning
> of this function. Checking attach_btf_id and expected_attach_type
> would make more sense to be done next to each other as in the current
> code.
Yeah, good point. Not sure what I was thinking stuffing it in the middle
there; will fix and send a v2!
> Acked-by: Martin KaFai Lau <kafai@fb.com>
Thanks! :)
-Toke
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-03-25 20:38 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-03-25 15:40 [PATCH bpf 1/2] bpf: enforce that struct_ops programs be GPL-only Toke Høiland-Jørgensen
2021-03-25 15:40 ` [PATCH bpf 2/2] bpf/selftests: test that kernel rejects a TCP CC with an invalid license Toke Høiland-Jørgensen
2021-03-25 18:32 ` Martin KaFai Lau
2021-03-25 20:38 ` Toke Høiland-Jørgensen
2021-03-25 18:15 ` [PATCH bpf 1/2] bpf: enforce that struct_ops programs be GPL-only Martin KaFai Lau
2021-03-25 20:37 ` Toke Høiland-Jørgensen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).