| Message ID | 20210209193112.1752976-1-kafai@fb.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
On Tue, Feb 9, 2021 at 12:11 PM Martin KaFai Lau <kafai@fb.com> wrote: > > This patch adds a "void *owner" member. The existing > bpf_tcp_ca test will ensure the bpf_cubic.o and bpf_dctcp.o > can be loaded. > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > --- Acked-by: Andrii Nakryiko <andrii@kernel.org> What will happen if BPF code initializes such non-func ptr member? Will libbpf complain or just ignore those values? Ignoring initialized members isn't great. > tools/testing/selftests/bpf/bpf_tcp_helpers.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/tools/testing/selftests/bpf/bpf_tcp_helpers.h b/tools/testing/selftests/bpf/bpf_tcp_helpers.h > index 6a9053162cf2..91f0fac632f4 100644 > --- a/tools/testing/selftests/bpf/bpf_tcp_helpers.h > +++ b/tools/testing/selftests/bpf/bpf_tcp_helpers.h > @@ -177,6 +177,7 @@ struct tcp_congestion_ops { > * after all the ca_state processing. (optional) > */ > void (*cong_control)(struct sock *sk, const struct rate_sample *rs); > + void *owner; > }; > > #define min(a, b) ((a) < (b) ? (a) : (b)) > -- > 2.24.1 >
On Wed, Feb 10, 2021 at 12:27:38PM -0800, Andrii Nakryiko wrote: > On Tue, Feb 9, 2021 at 12:11 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > This patch adds a "void *owner" member. The existing > > bpf_tcp_ca test will ensure the bpf_cubic.o and bpf_dctcp.o > > can be loaded. > > > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > > --- > > Acked-by: Andrii Nakryiko <andrii@kernel.org> > > What will happen if BPF code initializes such non-func ptr member? > Will libbpf complain or just ignore those values? Ignoring initialized > members isn't great. The latter. libbpf will ignore non-func ptr member. The non-func ptr member stays zero when it is passed to the kernel. libbpf can be changed to copy this non-func ptr value. The kernel will decide what to do with it. It will then be consistent with int/array member like ".name" and ".flags" where the kernel will verify the value. I can spin v2 to do that. > > > tools/testing/selftests/bpf/bpf_tcp_helpers.h | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/tools/testing/selftests/bpf/bpf_tcp_helpers.h b/tools/testing/selftests/bpf/bpf_tcp_helpers.h > > index 6a9053162cf2..91f0fac632f4 100644 > > --- a/tools/testing/selftests/bpf/bpf_tcp_helpers.h > > +++ b/tools/testing/selftests/bpf/bpf_tcp_helpers.h > > @@ -177,6 +177,7 @@ struct tcp_congestion_ops { > > * after all the ca_state processing. (optional) > > */ > > void (*cong_control)(struct sock *sk, const struct rate_sample *rs); > > + void *owner; > > }; > > > > #define min(a, b) ((a) < (b) ? (a) : (b)) > > -- > > 2.24.1 > >
On Wed, Feb 10, 2021 at 1:17 PM Martin KaFai Lau <kafai@fb.com> wrote: > > On Wed, Feb 10, 2021 at 12:27:38PM -0800, Andrii Nakryiko wrote: > > On Tue, Feb 9, 2021 at 12:11 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > This patch adds a "void *owner" member. The existing > > > bpf_tcp_ca test will ensure the bpf_cubic.o and bpf_dctcp.o > > > can be loaded. > > > > > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > > > --- > > > > Acked-by: Andrii Nakryiko <andrii@kernel.org> > > > > What will happen if BPF code initializes such non-func ptr member? > > Will libbpf complain or just ignore those values? Ignoring initialized > > members isn't great. > The latter. libbpf will ignore non-func ptr member. The non-func ptr > member stays zero when it is passed to the kernel. > > libbpf can be changed to copy this non-func ptr value. > The kernel will decide what to do with it. It will > then be consistent with int/array member like ".name" > and ".flags" where the kernel will verify the value. > I can spin v2 to do that. I was thinking about erroring out on non-zero fields, but if you think it's useful to pass through values, it could be done, but will require more and careful code, probably. So, basically, don't feel obligated to do this in this patch set. > > > > > > tools/testing/selftests/bpf/bpf_tcp_helpers.h | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/tools/testing/selftests/bpf/bpf_tcp_helpers.h b/tools/testing/selftests/bpf/bpf_tcp_helpers.h > > > index 6a9053162cf2..91f0fac632f4 100644 > > > --- a/tools/testing/selftests/bpf/bpf_tcp_helpers.h > > > +++ b/tools/testing/selftests/bpf/bpf_tcp_helpers.h > > > @@ -177,6 +177,7 @@ struct tcp_congestion_ops { > > > * after all the ca_state processing. (optional) > > > */ > > > void (*cong_control)(struct sock *sk, const struct rate_sample *rs); > > > + void *owner; > > > }; > > > > > > #define min(a, b) ((a) < (b) ? (a) : (b)) > > > -- > > > 2.24.1 > > >
On Wed, Feb 10, 2021 at 02:54:40PM -0800, Andrii Nakryiko wrote: > On Wed, Feb 10, 2021 at 1:17 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > On Wed, Feb 10, 2021 at 12:27:38PM -0800, Andrii Nakryiko wrote: > > > On Tue, Feb 9, 2021 at 12:11 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > > > This patch adds a "void *owner" member. The existing > > > > bpf_tcp_ca test will ensure the bpf_cubic.o and bpf_dctcp.o > > > > can be loaded. > > > > > > > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > > > > --- > > > > > > Acked-by: Andrii Nakryiko <andrii@kernel.org> > > > > > > What will happen if BPF code initializes such non-func ptr member? > > > Will libbpf complain or just ignore those values? Ignoring initialized > > > members isn't great. > > The latter. libbpf will ignore non-func ptr member. The non-func ptr > > member stays zero when it is passed to the kernel. > > > > libbpf can be changed to copy this non-func ptr value. > > The kernel will decide what to do with it. It will > > then be consistent with int/array member like ".name" > > and ".flags" where the kernel will verify the value. > > I can spin v2 to do that. > > I was thinking about erroring out on non-zero fields, but if you think > it's useful to pass through values, it could be done, but will require > more and careful code, probably. So, basically, don't feel obligated > to do this in this patch set. You meant it needs different handling in copying ptr value than copying int/char[]?
On Wed, Feb 10, 2021 at 5:55 PM Martin KaFai Lau <kafai@fb.com> wrote: > > On Wed, Feb 10, 2021 at 02:54:40PM -0800, Andrii Nakryiko wrote: > > On Wed, Feb 10, 2021 at 1:17 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > On Wed, Feb 10, 2021 at 12:27:38PM -0800, Andrii Nakryiko wrote: > > > > On Tue, Feb 9, 2021 at 12:11 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > > > > > This patch adds a "void *owner" member. The existing > > > > > bpf_tcp_ca test will ensure the bpf_cubic.o and bpf_dctcp.o > > > > > can be loaded. > > > > > > > > > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > > > > > --- > > > > > > > > Acked-by: Andrii Nakryiko <andrii@kernel.org> > > > > > > > > What will happen if BPF code initializes such non-func ptr member? > > > > Will libbpf complain or just ignore those values? Ignoring initialized > > > > members isn't great. > > > The latter. libbpf will ignore non-func ptr member. The non-func ptr > > > member stays zero when it is passed to the kernel. > > > > > > libbpf can be changed to copy this non-func ptr value. > > > The kernel will decide what to do with it. It will > > > then be consistent with int/array member like ".name" > > > and ".flags" where the kernel will verify the value. > > > I can spin v2 to do that. > > > > I was thinking about erroring out on non-zero fields, but if you think > > it's useful to pass through values, it could be done, but will require > > more and careful code, probably. So, basically, don't feel obligated > > to do this in this patch set. > You meant it needs different handling in copying ptr value > than copying int/char[]? Hm.. If we are talking about copying pointer values, then I don't see how you can provide a valid kernel pointer from the BPF program?... But if we are talking about copying field values in general, then you'll need to handle enums, struct/union, etc, no? If int/char[] is supported (I probably missed that it is), that might be the only things you'd need to support. So for non function pointers, I'd just enforce zeroes.
On Wed, Feb 10, 2021 at 06:07:04PM -0800, Andrii Nakryiko wrote: > On Wed, Feb 10, 2021 at 5:55 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > On Wed, Feb 10, 2021 at 02:54:40PM -0800, Andrii Nakryiko wrote: > > > On Wed, Feb 10, 2021 at 1:17 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > > > On Wed, Feb 10, 2021 at 12:27:38PM -0800, Andrii Nakryiko wrote: > > > > > On Tue, Feb 9, 2021 at 12:11 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > > > > > > > This patch adds a "void *owner" member. The existing > > > > > > bpf_tcp_ca test will ensure the bpf_cubic.o and bpf_dctcp.o > > > > > > can be loaded. > > > > > > > > > > > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > > > > > > --- > > > > > > > > > > Acked-by: Andrii Nakryiko <andrii@kernel.org> > > > > > > > > > > What will happen if BPF code initializes such non-func ptr member? > > > > > Will libbpf complain or just ignore those values? Ignoring initialized > > > > > members isn't great. > > > > The latter. libbpf will ignore non-func ptr member. The non-func ptr > > > > member stays zero when it is passed to the kernel. > > > > > > > > libbpf can be changed to copy this non-func ptr value. > > > > The kernel will decide what to do with it. It will > > > > then be consistent with int/array member like ".name" > > > > and ".flags" where the kernel will verify the value. > > > > I can spin v2 to do that. > > > > > > I was thinking about erroring out on non-zero fields, but if you think > > > it's useful to pass through values, it could be done, but will require > > > more and careful code, probably. So, basically, don't feel obligated > > > to do this in this patch set. > > You meant it needs different handling in copying ptr value > > than copying int/char[]? > > Hm.. If we are talking about copying pointer values, then I don't see > how you can provide a valid kernel pointer from the BPF program?... I am thinking the kernel is already rejecting members that is supposed to be zero (e.g. non func ptr here), so there is no need to add codes to libbpf to do this again. > But if we are talking about copying field values in general, then > you'll need to handle enums, struct/union, etc, no? If int/char[] is > supported (I probably missed that it is), that might be the only > things you'd need to support. So for non function pointers, I'd just > enforce zeroes. Sure, we can reject everything else for non zero in libbpf. I think we can use a different patch set for that?
On Wed, Feb 10, 2021 at 6:42 PM Martin KaFai Lau <kafai@fb.com> wrote: > > On Wed, Feb 10, 2021 at 06:07:04PM -0800, Andrii Nakryiko wrote: > > On Wed, Feb 10, 2021 at 5:55 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > On Wed, Feb 10, 2021 at 02:54:40PM -0800, Andrii Nakryiko wrote: > > > > On Wed, Feb 10, 2021 at 1:17 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > > > > > On Wed, Feb 10, 2021 at 12:27:38PM -0800, Andrii Nakryiko wrote: > > > > > > On Tue, Feb 9, 2021 at 12:11 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > > > > > > > > > This patch adds a "void *owner" member. The existing > > > > > > > bpf_tcp_ca test will ensure the bpf_cubic.o and bpf_dctcp.o > > > > > > > can be loaded. > > > > > > > > > > > > > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > > > > > > > --- > > > > > > > > > > > > Acked-by: Andrii Nakryiko <andrii@kernel.org> > > > > > > > > > > > > What will happen if BPF code initializes such non-func ptr member? > > > > > > Will libbpf complain or just ignore those values? Ignoring initialized > > > > > > members isn't great. > > > > > The latter. libbpf will ignore non-func ptr member. The non-func ptr > > > > > member stays zero when it is passed to the kernel. > > > > > > > > > > libbpf can be changed to copy this non-func ptr value. > > > > > The kernel will decide what to do with it. It will > > > > > then be consistent with int/array member like ".name" > > > > > and ".flags" where the kernel will verify the value. > > > > > I can spin v2 to do that. > > > > > > > > I was thinking about erroring out on non-zero fields, but if you think > > > > it's useful to pass through values, it could be done, but will require > > > > more and careful code, probably. So, basically, don't feel obligated > > > > to do this in this patch set. > > > You meant it needs different handling in copying ptr value > > > than copying int/char[]? > > > > Hm.. If we are talking about copying pointer values, then I don't see > > how you can provide a valid kernel pointer from the BPF program?... > I am thinking the kernel is already rejecting members that is supposed > to be zero (e.g. non func ptr here), so there is no need to add codes > to libbpf to do this again. > > > But if we are talking about copying field values in general, then > > you'll need to handle enums, struct/union, etc, no? If int/char[] is > > supported (I probably missed that it is), that might be the only > > things you'd need to support. So for non function pointers, I'd just > > enforce zeroes. > Sure, we can reject everything else for non zero in libbpf. > I think we can use a different patch set for that? Sure. My original point was that if someone initialized, say, owner field with some meaningless number, it would be nice for libbpf to reject this with error instead of ignoring. It's unlikely, though, so no big deal.
diff --git a/tools/testing/selftests/bpf/bpf_tcp_helpers.h b/tools/testing/selftests/bpf/bpf_tcp_helpers.h index 6a9053162cf2..91f0fac632f4 100644 --- a/tools/testing/selftests/bpf/bpf_tcp_helpers.h +++ b/tools/testing/selftests/bpf/bpf_tcp_helpers.h @@ -177,6 +177,7 @@ struct tcp_congestion_ops { * after all the ca_state processing. (optional) */ void (*cong_control)(struct sock *sk, const struct rate_sample *rs); + void *owner; }; #define min(a, b) ((a) < (b) ? (a) : (b))
This patch adds a "void *owner" member. The existing bpf_tcp_ca test will ensure the bpf_cubic.o and bpf_dctcp.o can be loaded. Signed-off-by: Martin KaFai Lau <kafai@fb.com> --- tools/testing/selftests/bpf/bpf_tcp_helpers.h | 1 + 1 file changed, 1 insertion(+)