[PATCH bpf 1/2] libbpf: Ignore non function pointer member in struct_ops

Subsystems: bpf [general] (safe dynamic programs and tools), bpf [library] (libbpf), the rest

STALE2013d

11 messages, 2 authors, 2021-02-11 · open the first message on its own page

[PATCH bpf 1/2] libbpf: Ignore non function pointer member in struct_ops

From: Martin KaFai Lau <hidden>
Date: 2021-02-09 20:37:23 

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

[PATCH bpf 2/2] bpf: selftests: Add non function pointer test to struct_ops

From: Martin KaFai Lau <hidden>
Date: 2021-02-09 20:10:42 

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

Re: [PATCH bpf 1/2] libbpf: Ignore non function pointer member in struct_ops

From: Andrii Nakryiko <hidden>
Date: 2021-02-10 20:28:05 

On Tue, Feb 9, 2021 at 12:40 PM Martin KaFai Lau [off-list ref] 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 <redacted>
---
Looks good, see nit below.

Acked-by: Andrii Nakryiko <andrii@kernel.org>
quoted hunk
 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?
quoted hunk
@@ -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

Re: [PATCH bpf 2/2] bpf: selftests: Add non function pointer test to struct_ops

From: Andrii Nakryiko <hidden>
Date: 2021-02-10 20:29:11 

On Tue, Feb 9, 2021 at 12:11 PM Martin KaFai Lau [off-list ref] 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 <redacted>
---
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.
quoted hunk
 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

Re: [PATCH bpf 2/2] bpf: selftests: Add non function pointer test to struct_ops

From: Martin KaFai Lau <hidden>
Date: 2021-02-10 21:18:42 

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 [off-list ref] wrote:
quoted
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 <redacted>
---
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.

quoted
 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





  

    
Re: [PATCH bpf 1/2] libbpf: Ignore non function pointer member in struct_ops

    

      From: Martin KaFai Lau <hidden>

      Date: 2021-02-10 21:26:40 
    

  

  
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 [off-list ref] wrote:
quoted
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 <redacted>
---
Looks good, see nit below.

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

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





  

    
Re: [PATCH bpf 2/2] bpf: selftests: Add non function pointer test to struct_ops

    

      From: Andrii Nakryiko <hidden>

      Date: 2021-02-10 22:55:35 
    

  

  
On Wed, Feb 10, 2021 at 1:17 PM Martin KaFai Lau [off-list ref] wrote:
On Wed, Feb 10, 2021 at 12:27:38PM -0800, Andrii Nakryiko wrote:
quoted
On Tue, Feb 9, 2021 at 12:11 PM Martin KaFai Lau [off-list ref] wrote:
quoted
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 <redacted>
---
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.

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





  

    
Re: [PATCH bpf 2/2] bpf: selftests: Add non function pointer test to struct_ops

    

      From: Martin KaFai Lau <hidden>

      Date: 2021-02-11 02:00:02 
    

  

  
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 [off-list ref] wrote:
quoted
On Wed, Feb 10, 2021 at 12:27:38PM -0800, Andrii Nakryiko wrote:
quoted
On Tue, Feb 9, 2021 at 12:11 PM Martin KaFai Lau [off-list ref] wrote:
quoted
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 <redacted>
---
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[]?





  

    
Re: [PATCH bpf 2/2] bpf: selftests: Add non function pointer test to struct_ops

    

      From: Andrii Nakryiko <hidden>

      Date: 2021-02-11 02:08:17 
    

  

  
On Wed, Feb 10, 2021 at 5:55 PM Martin KaFai Lau [off-list ref] wrote:
On Wed, Feb 10, 2021 at 02:54:40PM -0800, Andrii Nakryiko wrote:
quoted
On Wed, Feb 10, 2021 at 1:17 PM Martin KaFai Lau [off-list ref] wrote:
quoted
On Wed, Feb 10, 2021 at 12:27:38PM -0800, Andrii Nakryiko wrote:
quoted
On Tue, Feb 9, 2021 at 12:11 PM Martin KaFai Lau [off-list ref] wrote:
quoted
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 <redacted>
---
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.





  

    
Re: [PATCH bpf 2/2] bpf: selftests: Add non function pointer test to struct_ops

    

      From: Martin KaFai Lau <hidden>

      Date: 2021-02-11 02:43:31 
    

  

  
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 [off-list ref] wrote:
quoted
On Wed, Feb 10, 2021 at 02:54:40PM -0800, Andrii Nakryiko wrote:
quoted
On Wed, Feb 10, 2021 at 1:17 PM Martin KaFai Lau [off-list ref] wrote:
quoted
On Wed, Feb 10, 2021 at 12:27:38PM -0800, Andrii Nakryiko wrote:
quoted
On Tue, Feb 9, 2021 at 12:11 PM Martin KaFai Lau [off-list ref] wrote:
quoted
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 <redacted>
---
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?





  

    
Re: [PATCH bpf 2/2] bpf: selftests: Add non function pointer test to struct_ops

    

      From: Andrii Nakryiko <hidden>

      Date: 2021-02-11 19:32:34 
    

  

  
On Wed, Feb 10, 2021 at 6:42 PM Martin KaFai Lau [off-list ref] wrote:
On Wed, Feb 10, 2021 at 06:07:04PM -0800, Andrii Nakryiko wrote:
quoted
On Wed, Feb 10, 2021 at 5:55 PM Martin KaFai Lau [off-list ref] wrote:
quoted
On Wed, Feb 10, 2021 at 02:54:40PM -0800, Andrii Nakryiko wrote:
quoted
On Wed, Feb 10, 2021 at 1:17 PM Martin KaFai Lau [off-list ref] wrote:
quoted
On Wed, Feb 10, 2021 at 12:27:38PM -0800, Andrii Nakryiko wrote:
quoted
On Tue, Feb 9, 2021 at 12:11 PM Martin KaFai Lau [off-list ref] wrote:
quoted
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 <redacted>
---
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.

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




    
  

  
    

      

        Keyboard shortcuts
      

      

        
          
hback out one level

          
jnext message in thread

          
kprevious message in thread

          
ldrill in

          
Escclose help / fold thread tree

          
?toggle this help