netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf 1/2] libbpf: Ignore non function pointer member in struct_ops
@ 2021-02-09 19:31 Martin KaFai Lau
  2021-02-09 19:31 ` [PATCH bpf 2/2] bpf: selftests: Add non function pointer test to struct_ops Martin KaFai Lau
  2021-02-10 20:26 ` [PATCH bpf 1/2] libbpf: Ignore non function pointer member in struct_ops Andrii Nakryiko
  0 siblings, 2 replies; 11+ messages in thread
From: Martin KaFai Lau @ 2021-02-09 19:31 UTC (permalink / raw)
  To: bpf; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, netdev

When libbpf initializes the kernel's struct_ops in
"bpf_map__init_kern_struct_ops()", it enforces all
pointer types must be a function pointer and rejects
others.  It turns out to be too strict.  For example,
when directly using "struct tcp_congestion_ops" from vmlinux.h,
it has a "struct module *owner" member and it is set to NULL
in a bpf_tcp_cc.o.

Instead, it only needs to ensure the member is a function
pointer if it has been set (relocated) to a bpf-prog.
This patch moves the "btf_is_func_proto(kern_mtype)" check
after the existing "if (!prog) { continue; }".

The "btf_is_func_proto(mtype)" has already been guaranteed
in "bpf_object__collect_st_ops_relos()" which has been run
before "bpf_map__init_kern_struct_ops()".  Thus, this check
is removed.

Fixes: 590a00888250 ("bpf: libbpf: Add STRUCT_OPS support")
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
 tools/lib/bpf/libbpf.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 6ae748f6ea11..b483608ea72a 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -887,12 +887,6 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map,
 			kern_mtype = skip_mods_and_typedefs(kern_btf,
 							    kern_mtype->type,
 							    &kern_mtype_id);
-			if (!btf_is_func_proto(mtype) ||
-			    !btf_is_func_proto(kern_mtype)) {
-				pr_warn("struct_ops init_kern %s: non func ptr %s is not supported\n",
-					map->name, mname);
-				return -ENOTSUP;
-			}
 
 			prog = st_ops->progs[i];
 			if (!prog) {
@@ -901,6 +895,12 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map,
 				continue;
 			}
 
+			if (!btf_is_func_proto(kern_mtype)) {
+				pr_warn("struct_ops init_kern %s: kernel member %s is not a func ptr\n",
+					map->name, mname);
+				return -ENOTSUP;
+			}
+
 			prog->attach_btf_id = kern_type_id;
 			prog->expected_attach_type = kern_member_idx;
 
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH bpf 2/2] bpf: selftests: Add non function pointer test to struct_ops
  2021-02-09 19:31 [PATCH bpf 1/2] libbpf: Ignore non function pointer member in struct_ops Martin KaFai Lau
@ 2021-02-09 19:31 ` Martin KaFai Lau
  2021-02-10 20:27   ` Andrii Nakryiko
  2021-02-10 20:26 ` [PATCH bpf 1/2] libbpf: Ignore non function pointer member in struct_ops Andrii Nakryiko
  1 sibling, 1 reply; 11+ messages in thread
From: Martin KaFai Lau @ 2021-02-09 19:31 UTC (permalink / raw)
  To: bpf; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, netdev

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(+)

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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH bpf 1/2] libbpf: Ignore non function pointer member in struct_ops
  2021-02-09 19:31 [PATCH bpf 1/2] libbpf: Ignore non function pointer member in struct_ops Martin KaFai Lau
  2021-02-09 19:31 ` [PATCH bpf 2/2] bpf: selftests: Add non function pointer test to struct_ops Martin KaFai Lau
@ 2021-02-10 20:26 ` Andrii Nakryiko
  2021-02-10 21:23   ` Martin KaFai Lau
  1 sibling, 1 reply; 11+ messages in thread
From: Andrii Nakryiko @ 2021-02-10 20:26 UTC (permalink / raw)
  To: Martin KaFai Lau
  Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team, Networking

On Tue, Feb 9, 2021 at 12:40 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> When libbpf initializes the kernel's struct_ops in
> "bpf_map__init_kern_struct_ops()", it enforces all
> pointer types must be a function pointer and rejects
> others.  It turns out to be too strict.  For example,
> when directly using "struct tcp_congestion_ops" from vmlinux.h,
> it has a "struct module *owner" member and it is set to NULL
> in a bpf_tcp_cc.o.
>
> Instead, it only needs to ensure the member is a function
> pointer if it has been set (relocated) to a bpf-prog.
> This patch moves the "btf_is_func_proto(kern_mtype)" check
> after the existing "if (!prog) { continue; }".
>
> The "btf_is_func_proto(mtype)" has already been guaranteed
> in "bpf_object__collect_st_ops_relos()" which has been run
> before "bpf_map__init_kern_struct_ops()".  Thus, this check
> is removed.
>
> Fixes: 590a00888250 ("bpf: libbpf: Add STRUCT_OPS support")
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> ---

Looks good, see nit below.

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>  tools/lib/bpf/libbpf.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 6ae748f6ea11..b483608ea72a 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -887,12 +887,6 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map,
>                         kern_mtype = skip_mods_and_typedefs(kern_btf,
>                                                             kern_mtype->type,
>                                                             &kern_mtype_id);
> -                       if (!btf_is_func_proto(mtype) ||
> -                           !btf_is_func_proto(kern_mtype)) {
> -                               pr_warn("struct_ops init_kern %s: non func ptr %s is not supported\n",
> -                                       map->name, mname);
> -                               return -ENOTSUP;
> -                       }
>
>                         prog = st_ops->progs[i];
>                         if (!prog) {

debug message below this line is a bit misleading, it talks about
"func ptr is not set", but it actually could be any kind of field. So
it would be nice to just talk about "members" or "fields" there, no?

> @@ -901,6 +895,12 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map,
>                                 continue;
>                         }
>
> +                       if (!btf_is_func_proto(kern_mtype)) {
> +                               pr_warn("struct_ops init_kern %s: kernel member %s is not a func ptr\n",
> +                                       map->name, mname);
> +                               return -ENOTSUP;
> +                       }
> +
>                         prog->attach_btf_id = kern_type_id;
>                         prog->expected_attach_type = kern_member_idx;
>
> --
> 2.24.1
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH bpf 2/2] bpf: selftests: Add non function pointer test to struct_ops
  2021-02-09 19:31 ` [PATCH bpf 2/2] bpf: selftests: Add non function pointer test to struct_ops Martin KaFai Lau
@ 2021-02-10 20:27   ` Andrii Nakryiko
  2021-02-10 21:17     ` Martin KaFai Lau
  0 siblings, 1 reply; 11+ messages in thread
From: Andrii Nakryiko @ 2021-02-10 20:27 UTC (permalink / raw)
  To: Martin KaFai Lau
  Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team, Networking

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
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH bpf 2/2] bpf: selftests: Add non function pointer test to struct_ops
  2021-02-10 20:27   ` Andrii Nakryiko
@ 2021-02-10 21:17     ` Martin KaFai Lau
  2021-02-10 22:54       ` Andrii Nakryiko
  0 siblings, 1 reply; 11+ messages in thread
From: Martin KaFai Lau @ 2021-02-10 21:17 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team, Networking

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
> >

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH bpf 1/2] libbpf: Ignore non function pointer member in struct_ops
  2021-02-10 20:26 ` [PATCH bpf 1/2] libbpf: Ignore non function pointer member in struct_ops Andrii Nakryiko
@ 2021-02-10 21:23   ` Martin KaFai Lau
  0 siblings, 0 replies; 11+ messages in thread
From: Martin KaFai Lau @ 2021-02-10 21:23 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team, Networking

On Wed, Feb 10, 2021 at 12:26:20PM -0800, Andrii Nakryiko wrote:
> On Tue, Feb 9, 2021 at 12:40 PM Martin KaFai Lau <kafai@fb.com> wrote:
> >
> > When libbpf initializes the kernel's struct_ops in
> > "bpf_map__init_kern_struct_ops()", it enforces all
> > pointer types must be a function pointer and rejects
> > others.  It turns out to be too strict.  For example,
> > when directly using "struct tcp_congestion_ops" from vmlinux.h,
> > it has a "struct module *owner" member and it is set to NULL
> > in a bpf_tcp_cc.o.
> >
> > Instead, it only needs to ensure the member is a function
> > pointer if it has been set (relocated) to a bpf-prog.
> > This patch moves the "btf_is_func_proto(kern_mtype)" check
> > after the existing "if (!prog) { continue; }".
> >
> > The "btf_is_func_proto(mtype)" has already been guaranteed
> > in "bpf_object__collect_st_ops_relos()" which has been run
> > before "bpf_map__init_kern_struct_ops()".  Thus, this check
> > is removed.
> >
> > Fixes: 590a00888250 ("bpf: libbpf: Add STRUCT_OPS support")
> > Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> > ---
> 
> Looks good, see nit below.
> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 
> >  tools/lib/bpf/libbpf.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 6ae748f6ea11..b483608ea72a 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -887,12 +887,6 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map,
> >                         kern_mtype = skip_mods_and_typedefs(kern_btf,
> >                                                             kern_mtype->type,
> >                                                             &kern_mtype_id);
> > -                       if (!btf_is_func_proto(mtype) ||
> > -                           !btf_is_func_proto(kern_mtype)) {
> > -                               pr_warn("struct_ops init_kern %s: non func ptr %s is not supported\n",
> > -                                       map->name, mname);
> > -                               return -ENOTSUP;
> > -                       }
> >
> >                         prog = st_ops->progs[i];
> >                         if (!prog) {
> 
> debug message below this line is a bit misleading, it talks about
> "func ptr is not set", but it actually could be any kind of field. So
> it would be nice to just talk about "members" or "fields" there, no?
Good catch.  The debug message needs to change.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH bpf 2/2] bpf: selftests: Add non function pointer test to struct_ops
  2021-02-10 21:17     ` Martin KaFai Lau
@ 2021-02-10 22:54       ` Andrii Nakryiko
  2021-02-11  1:55         ` Martin KaFai Lau
  0 siblings, 1 reply; 11+ messages in thread
From: Andrii Nakryiko @ 2021-02-10 22:54 UTC (permalink / raw)
  To: Martin KaFai Lau
  Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team, Networking

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
> > >

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH bpf 2/2] bpf: selftests: Add non function pointer test to struct_ops
  2021-02-10 22:54       ` Andrii Nakryiko
@ 2021-02-11  1:55         ` Martin KaFai Lau
  2021-02-11  2:07           ` Andrii Nakryiko
  0 siblings, 1 reply; 11+ messages in thread
From: Martin KaFai Lau @ 2021-02-11  1:55 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team, Networking

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[]?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH bpf 2/2] bpf: selftests: Add non function pointer test to struct_ops
  2021-02-11  1:55         ` Martin KaFai Lau
@ 2021-02-11  2:07           ` Andrii Nakryiko
  2021-02-11  2:42             ` Martin KaFai Lau
  0 siblings, 1 reply; 11+ messages in thread
From: Andrii Nakryiko @ 2021-02-11  2:07 UTC (permalink / raw)
  To: Martin KaFai Lau
  Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team, Networking

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH bpf 2/2] bpf: selftests: Add non function pointer test to struct_ops
  2021-02-11  2:07           ` Andrii Nakryiko
@ 2021-02-11  2:42             ` Martin KaFai Lau
  2021-02-11 19:31               ` Andrii Nakryiko
  0 siblings, 1 reply; 11+ messages in thread
From: Martin KaFai Lau @ 2021-02-11  2:42 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team, Networking

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?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH bpf 2/2] bpf: selftests: Add non function pointer test to struct_ops
  2021-02-11  2:42             ` Martin KaFai Lau
@ 2021-02-11 19:31               ` Andrii Nakryiko
  0 siblings, 0 replies; 11+ messages in thread
From: Andrii Nakryiko @ 2021-02-11 19:31 UTC (permalink / raw)
  To: Martin KaFai Lau
  Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team, Networking

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-02-11 19:32 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-09 19:31 [PATCH bpf 1/2] libbpf: Ignore non function pointer member in struct_ops Martin KaFai Lau
2021-02-09 19:31 ` [PATCH bpf 2/2] bpf: selftests: Add non function pointer test to struct_ops Martin KaFai Lau
2021-02-10 20:27   ` Andrii Nakryiko
2021-02-10 21:17     ` Martin KaFai Lau
2021-02-10 22:54       ` Andrii Nakryiko
2021-02-11  1:55         ` Martin KaFai Lau
2021-02-11  2:07           ` Andrii Nakryiko
2021-02-11  2:42             ` Martin KaFai Lau
2021-02-11 19:31               ` Andrii Nakryiko
2021-02-10 20:26 ` [PATCH bpf 1/2] libbpf: Ignore non function pointer member in struct_ops Andrii Nakryiko
2021-02-10 21:23   ` Martin KaFai Lau

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).