| Message ID | 20191011162124.52982-1-sdf@google.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next,1/3] bpf: preserve command of the process that loaded the program | expand |
On Fri, Oct 11, 2019 at 9:21 AM Stanislav Fomichev <sdf@google.com> wrote: > > Even though we have the pointer to user_struct and can recover > uid of the user who has created the program, it usually contains > 0 (root) which is not very informative. Let's store the comm of the > calling process and export it via bpf_prog_info. This should help > answer the question "which process loaded this particular program". > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > --- > include/linux/bpf.h | 1 + > include/uapi/linux/bpf.h | 2 ++ > kernel/bpf/syscall.c | 4 ++++ > 3 files changed, 7 insertions(+) > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index 5b9d22338606..b03ea396afe5 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -421,6 +421,7 @@ struct bpf_prog_aux { > struct work_struct work; > struct rcu_head rcu; > }; > + char created_by_comm[BPF_CREATED_COMM_LEN]; > }; > > struct bpf_array { > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > index a65c3b0c6935..4e883ecbba1e 100644 > --- a/include/uapi/linux/bpf.h > +++ b/include/uapi/linux/bpf.h > @@ -326,6 +326,7 @@ enum bpf_attach_type { > #define BPF_F_NUMA_NODE (1U << 2) > > #define BPF_OBJ_NAME_LEN 16U > +#define BPF_CREATED_COMM_LEN 16U Nack. 16 bytes is going to be useless. We found it the hard way with prog_name. If you want to embed additional debug information please use BTF for that.
On 10/11, Alexei Starovoitov wrote: > On Fri, Oct 11, 2019 at 9:21 AM Stanislav Fomichev <sdf@google.com> wrote: > > > > Even though we have the pointer to user_struct and can recover > > uid of the user who has created the program, it usually contains > > 0 (root) which is not very informative. Let's store the comm of the > > calling process and export it via bpf_prog_info. This should help > > answer the question "which process loaded this particular program". > > > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > --- > > include/linux/bpf.h | 1 + > > include/uapi/linux/bpf.h | 2 ++ > > kernel/bpf/syscall.c | 4 ++++ > > 3 files changed, 7 insertions(+) > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > index 5b9d22338606..b03ea396afe5 100644 > > --- a/include/linux/bpf.h > > +++ b/include/linux/bpf.h > > @@ -421,6 +421,7 @@ struct bpf_prog_aux { > > struct work_struct work; > > struct rcu_head rcu; > > }; > > + char created_by_comm[BPF_CREATED_COMM_LEN]; > > }; > > > > struct bpf_array { > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > index a65c3b0c6935..4e883ecbba1e 100644 > > --- a/include/uapi/linux/bpf.h > > +++ b/include/uapi/linux/bpf.h > > @@ -326,6 +326,7 @@ enum bpf_attach_type { > > #define BPF_F_NUMA_NODE (1U << 2) > > > > #define BPF_OBJ_NAME_LEN 16U > > +#define BPF_CREATED_COMM_LEN 16U > > Nack. > 16 bytes is going to be useless. > We found it the hard way with prog_name. > If you want to embed additional debug information > please use BTF for that. BTF was my natural choice initially, but then I saw created_by_uid and thought created_by_comm might have a chance :-) To clarify, by BTF you mean creating some unused global variable and use its name as the debugging info? Or there is some better way?
On Fri, Oct 11, 2019 at 5:38 PM Stanislav Fomichev <sdf@fomichev.me> wrote: > > On 10/11, Alexei Starovoitov wrote: > > On Fri, Oct 11, 2019 at 9:21 AM Stanislav Fomichev <sdf@google.com> wrote: > > > > > > Even though we have the pointer to user_struct and can recover > > > uid of the user who has created the program, it usually contains > > > 0 (root) which is not very informative. Let's store the comm of the > > > calling process and export it via bpf_prog_info. This should help > > > answer the question "which process loaded this particular program". > > > > > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > > --- > > > include/linux/bpf.h | 1 + > > > include/uapi/linux/bpf.h | 2 ++ > > > kernel/bpf/syscall.c | 4 ++++ > > > 3 files changed, 7 insertions(+) > > > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > > index 5b9d22338606..b03ea396afe5 100644 > > > --- a/include/linux/bpf.h > > > +++ b/include/linux/bpf.h > > > @@ -421,6 +421,7 @@ struct bpf_prog_aux { > > > struct work_struct work; > > > struct rcu_head rcu; > > > }; > > > + char created_by_comm[BPF_CREATED_COMM_LEN]; > > > }; > > > > > > struct bpf_array { > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > > index a65c3b0c6935..4e883ecbba1e 100644 > > > --- a/include/uapi/linux/bpf.h > > > +++ b/include/uapi/linux/bpf.h > > > @@ -326,6 +326,7 @@ enum bpf_attach_type { > > > #define BPF_F_NUMA_NODE (1U << 2) > > > > > > #define BPF_OBJ_NAME_LEN 16U > > > +#define BPF_CREATED_COMM_LEN 16U > > > > Nack. > > 16 bytes is going to be useless. > > We found it the hard way with prog_name. > > If you want to embed additional debug information > > please use BTF for that. > BTF was my natural choice initially, but then I saw created_by_uid and > thought created_by_comm might have a chance :-) > > To clarify, by BTF you mean creating some unused global variable > and use its name as the debugging info? Or there is some better way? I was thinking about adding new section to .btf.ext with this extra data, but global variable is a better idea indeed. We'd need to standardize such variables names, so that bpftool can parse and print it while doing 'bpftool prog show'. We see more and more cases where services use more than one program in single .c file to accomplish their goals. Tying such debug info (like 'created_by_comm') to each program individually isn't quite right. In that sense global variables are better, since they cover the whole .c file. Beyond 'created_by_comm' there are others things that people will likely want to know. Like which version of llvm was used to compile this .o file. Which unix user name compiled it. The name of service/daemon that will be using this .o and so on. May be some standard prefix to such global variables will do? Like "bpftool prog show" can scan global data for "__annotate_#name" and print both name and string contents ? For folks who regularly ssh into servers to debug bpf progs that will help a lot. May be some annotations llvm can automatically add to .o. Thoughts?
On Tue, Oct 15, 2019 at 2:22 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Fri, Oct 11, 2019 at 5:38 PM Stanislav Fomichev <sdf@fomichev.me> wrote: > > > > On 10/11, Alexei Starovoitov wrote: > > > On Fri, Oct 11, 2019 at 9:21 AM Stanislav Fomichev <sdf@google.com> wrote: > > > > > > > > Even though we have the pointer to user_struct and can recover > > > > uid of the user who has created the program, it usually contains > > > > 0 (root) which is not very informative. Let's store the comm of the > > > > calling process and export it via bpf_prog_info. This should help > > > > answer the question "which process loaded this particular program". > > > > > > > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > > > --- > > > > include/linux/bpf.h | 1 + > > > > include/uapi/linux/bpf.h | 2 ++ > > > > kernel/bpf/syscall.c | 4 ++++ > > > > 3 files changed, 7 insertions(+) > > > > > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > > > index 5b9d22338606..b03ea396afe5 100644 > > > > --- a/include/linux/bpf.h > > > > +++ b/include/linux/bpf.h > > > > @@ -421,6 +421,7 @@ struct bpf_prog_aux { > > > > struct work_struct work; > > > > struct rcu_head rcu; > > > > }; > > > > + char created_by_comm[BPF_CREATED_COMM_LEN]; > > > > }; > > > > > > > > struct bpf_array { > > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > > > index a65c3b0c6935..4e883ecbba1e 100644 > > > > --- a/include/uapi/linux/bpf.h > > > > +++ b/include/uapi/linux/bpf.h > > > > @@ -326,6 +326,7 @@ enum bpf_attach_type { > > > > #define BPF_F_NUMA_NODE (1U << 2) > > > > > > > > #define BPF_OBJ_NAME_LEN 16U > > > > +#define BPF_CREATED_COMM_LEN 16U > > > > > > Nack. > > > 16 bytes is going to be useless. > > > We found it the hard way with prog_name. > > > If you want to embed additional debug information > > > please use BTF for that. > > BTF was my natural choice initially, but then I saw created_by_uid and > > thought created_by_comm might have a chance :-) > > > > To clarify, by BTF you mean creating some unused global variable > > and use its name as the debugging info? Or there is some better way? > > I was thinking about adding new section to .btf.ext with this extra data, > but global variable is a better idea indeed. > We'd need to standardize such variables names, so that > bpftool can parse and print it while doing 'bpftool prog show'. > We see more and more cases where services use more than > one program in single .c file to accomplish their goals. > Tying such debug info (like 'created_by_comm') to each program > individually isn't quite right. > In that sense global variables are better, since they cover the > whole .c file. > Beyond 'created_by_comm' there are others things that people > will likely want to know. > Like which version of llvm was used to compile this .o file. > Which unix user name compiled it. > The name of service/daemon that will be using this .o > and so on. > May be some standard prefix to such global variables will do? > Like "bpftool prog show" can scan global data for > "__annotate_#name" and print both name and string contents ? > For folks who regularly ssh into servers to debug bpf progs > that will help a lot. > May be some annotations llvm can automatically add to .o. > Thoughts? We can dedicate separate ELF section for such variables, similar to license and version today, so that libbpf will know that those variables are not real variables and shouldn't be used from BPF program itself. But we can have many of them in single section, unlike version and license. :) With that, we'll have metadata and list of variables in BTF (DATASEC + VARs). The only downside - you'll need ELF itself to get the value of that variable, no? Is that acceptable? Do we always know where original ELF is? Alternatively we can have extra .BTF.ext section and re-use BTF's string section for values. Which one is more acceptable?
On Tue, Oct 15, 2019 at 3:14 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, Oct 15, 2019 at 2:22 PM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Fri, Oct 11, 2019 at 5:38 PM Stanislav Fomichev <sdf@fomichev.me> wrote: > > > > > > On 10/11, Alexei Starovoitov wrote: > > > > On Fri, Oct 11, 2019 at 9:21 AM Stanislav Fomichev <sdf@google.com> wrote: > > > > > > > > > > Even though we have the pointer to user_struct and can recover > > > > > uid of the user who has created the program, it usually contains > > > > > 0 (root) which is not very informative. Let's store the comm of the > > > > > calling process and export it via bpf_prog_info. This should help > > > > > answer the question "which process loaded this particular program". > > > > > > > > > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > > > > --- > > > > > include/linux/bpf.h | 1 + > > > > > include/uapi/linux/bpf.h | 2 ++ > > > > > kernel/bpf/syscall.c | 4 ++++ > > > > > 3 files changed, 7 insertions(+) > > > > > > > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > > > > index 5b9d22338606..b03ea396afe5 100644 > > > > > --- a/include/linux/bpf.h > > > > > +++ b/include/linux/bpf.h > > > > > @@ -421,6 +421,7 @@ struct bpf_prog_aux { > > > > > struct work_struct work; > > > > > struct rcu_head rcu; > > > > > }; > > > > > + char created_by_comm[BPF_CREATED_COMM_LEN]; > > > > > }; > > > > > > > > > > struct bpf_array { > > > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > > > > index a65c3b0c6935..4e883ecbba1e 100644 > > > > > --- a/include/uapi/linux/bpf.h > > > > > +++ b/include/uapi/linux/bpf.h > > > > > @@ -326,6 +326,7 @@ enum bpf_attach_type { > > > > > #define BPF_F_NUMA_NODE (1U << 2) > > > > > > > > > > #define BPF_OBJ_NAME_LEN 16U > > > > > +#define BPF_CREATED_COMM_LEN 16U > > > > > > > > Nack. > > > > 16 bytes is going to be useless. > > > > We found it the hard way with prog_name. > > > > If you want to embed additional debug information > > > > please use BTF for that. > > > BTF was my natural choice initially, but then I saw created_by_uid and > > > thought created_by_comm might have a chance :-) > > > > > > To clarify, by BTF you mean creating some unused global variable > > > and use its name as the debugging info? Or there is some better way? > > > > I was thinking about adding new section to .btf.ext with this extra data, > > but global variable is a better idea indeed. > > We'd need to standardize such variables names, so that > > bpftool can parse and print it while doing 'bpftool prog show'. > > We see more and more cases where services use more than > > one program in single .c file to accomplish their goals. > > Tying such debug info (like 'created_by_comm') to each program > > individually isn't quite right. > > In that sense global variables are better, since they cover the > > whole .c file. > > Beyond 'created_by_comm' there are others things that people > > will likely want to know. > > Like which version of llvm was used to compile this .o file. > > Which unix user name compiled it. > > The name of service/daemon that will be using this .o > > and so on. > > May be some standard prefix to such global variables will do? > > Like "bpftool prog show" can scan global data for > > "__annotate_#name" and print both name and string contents ? > > For folks who regularly ssh into servers to debug bpf progs > > that will help a lot. > > May be some annotations llvm can automatically add to .o. > > Thoughts? > > We can dedicate separate ELF section for such variables, similar to > license and version today, so that libbpf will know that those > variables are not real variables and shouldn't be used from BPF > program itself. But we can have many of them in single section, unlike > version and license. :) With that, we'll have metadata and list of > variables in BTF (DATASEC + VARs). The only downside - you'll need ELF > itself to get the value of that variable, no? Is that acceptable? Do > we always know where original ELF is? Having .o around is not acceptable. That was already tried and didn't work with bcc. I was proposing to have these special vars to be loaded into the kernel as part of normal btf loading. Not sure what special section gives.
On 10/15, Alexei Starovoitov wrote: > On Fri, Oct 11, 2019 at 5:38 PM Stanislav Fomichev <sdf@fomichev.me> wrote: > > > > On 10/11, Alexei Starovoitov wrote: > > > On Fri, Oct 11, 2019 at 9:21 AM Stanislav Fomichev <sdf@google.com> wrote: > > > > > > > > Even though we have the pointer to user_struct and can recover > > > > uid of the user who has created the program, it usually contains > > > > 0 (root) which is not very informative. Let's store the comm of the > > > > calling process and export it via bpf_prog_info. This should help > > > > answer the question "which process loaded this particular program". > > > > > > > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > > > --- > > > > include/linux/bpf.h | 1 + > > > > include/uapi/linux/bpf.h | 2 ++ > > > > kernel/bpf/syscall.c | 4 ++++ > > > > 3 files changed, 7 insertions(+) > > > > > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > > > index 5b9d22338606..b03ea396afe5 100644 > > > > --- a/include/linux/bpf.h > > > > +++ b/include/linux/bpf.h > > > > @@ -421,6 +421,7 @@ struct bpf_prog_aux { > > > > struct work_struct work; > > > > struct rcu_head rcu; > > > > }; > > > > + char created_by_comm[BPF_CREATED_COMM_LEN]; > > > > }; > > > > > > > > struct bpf_array { > > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > > > index a65c3b0c6935..4e883ecbba1e 100644 > > > > --- a/include/uapi/linux/bpf.h > > > > +++ b/include/uapi/linux/bpf.h > > > > @@ -326,6 +326,7 @@ enum bpf_attach_type { > > > > #define BPF_F_NUMA_NODE (1U << 2) > > > > > > > > #define BPF_OBJ_NAME_LEN 16U > > > > +#define BPF_CREATED_COMM_LEN 16U > > > > > > Nack. > > > 16 bytes is going to be useless. > > > We found it the hard way with prog_name. > > > If you want to embed additional debug information > > > please use BTF for that. > > BTF was my natural choice initially, but then I saw created_by_uid and > > thought created_by_comm might have a chance :-) > > > > To clarify, by BTF you mean creating some unused global variable > > and use its name as the debugging info? Or there is some better way? > > I was thinking about adding new section to .btf.ext with this extra data, > but global variable is a better idea indeed. > We'd need to standardize such variables names, so that > bpftool can parse and print it while doing 'bpftool prog show'. > We see more and more cases where services use more than > one program in single .c file to accomplish their goals. > Tying such debug info (like 'created_by_comm') to each program > individually isn't quite right. > In that sense global variables are better, since they cover the > whole .c file. > Beyond 'created_by_comm' there are others things that people > will likely want to know. > Like which version of llvm was used to compile this .o file. > Which unix user name compiled it. > The name of service/daemon that will be using this .o > and so on. > May be some standard prefix to such global variables will do? > Like "bpftool prog show" can scan global data for > "__annotate_#name" and print both name and string contents ? > For folks who regularly ssh into servers to debug bpf progs > that will help a lot. > May be some annotations llvm can automatically add to .o. > Thoughts? We started some proof-of-concept prototyping yesterday; the idea, roughly: * build system generates build_info.h header which contains: char __attribute__((section("aux_timestamp"))) *__aux_<build timestamp> = ""; char __attribute__((section("aux_version"))) *__aux_<version> = ""; ... * clang has -include flag which includes this auto-generated file, so we don't rely on users including it * 'bpftool show btf | grep aux_' can be used for low-level debugging It's not pretty, but it gets the job done. I agree that having some sort of convention is nice to make it more usable. If we can agree on a pre-defined section (aux in my case) so that bpftool can take "variable" from aux_<variable> section name and print "value" from __aux_<value>, that would be nice. One thing I still have no idea how to implement with this scheme is some alternative to created_by_comm. There is no easy way to add some BTF at runtime (load) time. Ideas? > "__annotate_#name" and print both name and string contents ? As Andrii just pointed out, this requires knowing where to look for the obj files to print the contents of the vars :-(
On Tue, Oct 15, 2019 at 3:24 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Tue, Oct 15, 2019 at 3:14 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Tue, Oct 15, 2019 at 2:22 PM Alexei Starovoitov > > <alexei.starovoitov@gmail.com> wrote: > > > > > > On Fri, Oct 11, 2019 at 5:38 PM Stanislav Fomichev <sdf@fomichev.me> wrote: > > > > > > > > On 10/11, Alexei Starovoitov wrote: > > > > > On Fri, Oct 11, 2019 at 9:21 AM Stanislav Fomichev <sdf@google.com> wrote: > > > > > > > > > > > > Even though we have the pointer to user_struct and can recover > > > > > > uid of the user who has created the program, it usually contains > > > > > > 0 (root) which is not very informative. Let's store the comm of the > > > > > > calling process and export it via bpf_prog_info. This should help > > > > > > answer the question "which process loaded this particular program". > > > > > > > > > > > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > > > > > --- > > > > > > include/linux/bpf.h | 1 + > > > > > > include/uapi/linux/bpf.h | 2 ++ > > > > > > kernel/bpf/syscall.c | 4 ++++ > > > > > > 3 files changed, 7 insertions(+) > > > > > > > > > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > > > > > index 5b9d22338606..b03ea396afe5 100644 > > > > > > --- a/include/linux/bpf.h > > > > > > +++ b/include/linux/bpf.h > > > > > > @@ -421,6 +421,7 @@ struct bpf_prog_aux { > > > > > > struct work_struct work; > > > > > > struct rcu_head rcu; > > > > > > }; > > > > > > + char created_by_comm[BPF_CREATED_COMM_LEN]; > > > > > > }; > > > > > > > > > > > > struct bpf_array { > > > > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > > > > > index a65c3b0c6935..4e883ecbba1e 100644 > > > > > > --- a/include/uapi/linux/bpf.h > > > > > > +++ b/include/uapi/linux/bpf.h > > > > > > @@ -326,6 +326,7 @@ enum bpf_attach_type { > > > > > > #define BPF_F_NUMA_NODE (1U << 2) > > > > > > > > > > > > #define BPF_OBJ_NAME_LEN 16U > > > > > > +#define BPF_CREATED_COMM_LEN 16U > > > > > > > > > > Nack. > > > > > 16 bytes is going to be useless. > > > > > We found it the hard way with prog_name. > > > > > If you want to embed additional debug information > > > > > please use BTF for that. > > > > BTF was my natural choice initially, but then I saw created_by_uid and > > > > thought created_by_comm might have a chance :-) > > > > > > > > To clarify, by BTF you mean creating some unused global variable > > > > and use its name as the debugging info? Or there is some better way? > > > > > > I was thinking about adding new section to .btf.ext with this extra data, > > > but global variable is a better idea indeed. > > > We'd need to standardize such variables names, so that > > > bpftool can parse and print it while doing 'bpftool prog show'. > > > We see more and more cases where services use more than > > > one program in single .c file to accomplish their goals. > > > Tying such debug info (like 'created_by_comm') to each program > > > individually isn't quite right. > > > In that sense global variables are better, since they cover the > > > whole .c file. > > > Beyond 'created_by_comm' there are others things that people > > > will likely want to know. > > > Like which version of llvm was used to compile this .o file. > > > Which unix user name compiled it. > > > The name of service/daemon that will be using this .o > > > and so on. > > > May be some standard prefix to such global variables will do? > > > Like "bpftool prog show" can scan global data for > > > "__annotate_#name" and print both name and string contents ? > > > For folks who regularly ssh into servers to debug bpf progs > > > that will help a lot. > > > May be some annotations llvm can automatically add to .o. > > > Thoughts? > > > > We can dedicate separate ELF section for such variables, similar to > > license and version today, so that libbpf will know that those > > variables are not real variables and shouldn't be used from BPF > > program itself. But we can have many of them in single section, unlike > > version and license. :) With that, we'll have metadata and list of > > variables in BTF (DATASEC + VARs). The only downside - you'll need ELF > > itself to get the value of that variable, no? Is that acceptable? Do > > we always know where original ELF is? > > Having .o around is not acceptable. > That was already tried and didn't work with bcc. > I was proposing to have these special vars to be loaded into the kernel > as part of normal btf loading. BTF is just metadata for variables. We'll know name and type information about variable, but we need a string contents. That is stored in ELF, so without .o file we won't be able to extract it. Unless you have something else in mind? > Not sure what special section gives. It's a marker that libbpf doesn't have to allocate memory and create internal map for that section. We don't want those annotation variables to be backed by BPF map, do we?
On Tue, Oct 15, 2019 at 3:34 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, Oct 15, 2019 at 3:24 PM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Tue, Oct 15, 2019 at 3:14 PM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > > > > > > On Tue, Oct 15, 2019 at 2:22 PM Alexei Starovoitov > > > <alexei.starovoitov@gmail.com> wrote: > > > > > > > > On Fri, Oct 11, 2019 at 5:38 PM Stanislav Fomichev <sdf@fomichev.me> wrote: > > > > > > > > > > On 10/11, Alexei Starovoitov wrote: > > > > > > On Fri, Oct 11, 2019 at 9:21 AM Stanislav Fomichev <sdf@google.com> wrote: > > > > > > > > > > > > > > Even though we have the pointer to user_struct and can recover > > > > > > > uid of the user who has created the program, it usually contains > > > > > > > 0 (root) which is not very informative. Let's store the comm of the > > > > > > > calling process and export it via bpf_prog_info. This should help > > > > > > > answer the question "which process loaded this particular program". > > > > > > > > > > > > > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > > > > > > --- > > > > > > > include/linux/bpf.h | 1 + > > > > > > > include/uapi/linux/bpf.h | 2 ++ > > > > > > > kernel/bpf/syscall.c | 4 ++++ > > > > > > > 3 files changed, 7 insertions(+) > > > > > > > > > > > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > > > > > > index 5b9d22338606..b03ea396afe5 100644 > > > > > > > --- a/include/linux/bpf.h > > > > > > > +++ b/include/linux/bpf.h > > > > > > > @@ -421,6 +421,7 @@ struct bpf_prog_aux { > > > > > > > struct work_struct work; > > > > > > > struct rcu_head rcu; > > > > > > > }; > > > > > > > + char created_by_comm[BPF_CREATED_COMM_LEN]; > > > > > > > }; > > > > > > > > > > > > > > struct bpf_array { > > > > > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > > > > > > index a65c3b0c6935..4e883ecbba1e 100644 > > > > > > > --- a/include/uapi/linux/bpf.h > > > > > > > +++ b/include/uapi/linux/bpf.h > > > > > > > @@ -326,6 +326,7 @@ enum bpf_attach_type { > > > > > > > #define BPF_F_NUMA_NODE (1U << 2) > > > > > > > > > > > > > > #define BPF_OBJ_NAME_LEN 16U > > > > > > > +#define BPF_CREATED_COMM_LEN 16U > > > > > > > > > > > > Nack. > > > > > > 16 bytes is going to be useless. > > > > > > We found it the hard way with prog_name. > > > > > > If you want to embed additional debug information > > > > > > please use BTF for that. > > > > > BTF was my natural choice initially, but then I saw created_by_uid and > > > > > thought created_by_comm might have a chance :-) > > > > > > > > > > To clarify, by BTF you mean creating some unused global variable > > > > > and use its name as the debugging info? Or there is some better way? > > > > > > > > I was thinking about adding new section to .btf.ext with this extra data, > > > > but global variable is a better idea indeed. > > > > We'd need to standardize such variables names, so that > > > > bpftool can parse and print it while doing 'bpftool prog show'. > > > > We see more and more cases where services use more than > > > > one program in single .c file to accomplish their goals. > > > > Tying such debug info (like 'created_by_comm') to each program > > > > individually isn't quite right. > > > > In that sense global variables are better, since they cover the > > > > whole .c file. > > > > Beyond 'created_by_comm' there are others things that people > > > > will likely want to know. > > > > Like which version of llvm was used to compile this .o file. > > > > Which unix user name compiled it. > > > > The name of service/daemon that will be using this .o > > > > and so on. > > > > May be some standard prefix to such global variables will do? > > > > Like "bpftool prog show" can scan global data for > > > > "__annotate_#name" and print both name and string contents ? > > > > For folks who regularly ssh into servers to debug bpf progs > > > > that will help a lot. > > > > May be some annotations llvm can automatically add to .o. > > > > Thoughts? > > > > > > We can dedicate separate ELF section for such variables, similar to > > > license and version today, so that libbpf will know that those > > > variables are not real variables and shouldn't be used from BPF > > > program itself. But we can have many of them in single section, unlike > > > version and license. :) With that, we'll have metadata and list of > > > variables in BTF (DATASEC + VARs). The only downside - you'll need ELF > > > itself to get the value of that variable, no? Is that acceptable? Do > > > we always know where original ELF is? > > > > Having .o around is not acceptable. > > That was already tried and didn't work with bcc. > > I was proposing to have these special vars to be loaded into the kernel > > as part of normal btf loading. > > BTF is just metadata for variables. We'll know name and type > information about variable, but we need a string contents. That is > stored in ELF, so without .o file we won't be able to extract it. > Unless you have something else in mind? > > > Not sure what special section gives. > > It's a marker that libbpf doesn't have to allocate memory and create > internal map for that section. We don't want those annotation > variables to be backed by BPF map, do we? I'm proposing that these special variables to be part of normal global data. Which will be loaded into the kernel and .o is not necessary.
On Tue, Oct 15, 2019 at 02:21:50PM -0700, Alexei Starovoitov wrote: > On Fri, Oct 11, 2019 at 5:38 PM Stanislav Fomichev <sdf@fomichev.me> wrote: > > On 10/11, Alexei Starovoitov wrote: > > > On Fri, Oct 11, 2019 at 9:21 AM Stanislav Fomichev <sdf@google.com> wrote: > > > > > > > > Even though we have the pointer to user_struct and can recover > > > > uid of the user who has created the program, it usually contains > > > > 0 (root) which is not very informative. Let's store the comm of the > > > > calling process and export it via bpf_prog_info. This should help > > > > answer the question "which process loaded this particular program". > > > > > > > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > > > --- > > > > include/linux/bpf.h | 1 + > > > > include/uapi/linux/bpf.h | 2 ++ > > > > kernel/bpf/syscall.c | 4 ++++ > > > > 3 files changed, 7 insertions(+) > > > > > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > > > index 5b9d22338606..b03ea396afe5 100644 > > > > --- a/include/linux/bpf.h > > > > +++ b/include/linux/bpf.h > > > > @@ -421,6 +421,7 @@ struct bpf_prog_aux { > > > > struct work_struct work; > > > > struct rcu_head rcu; > > > > }; > > > > + char created_by_comm[BPF_CREATED_COMM_LEN]; > > > > }; > > > > > > > > struct bpf_array { > > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > > > index a65c3b0c6935..4e883ecbba1e 100644 > > > > --- a/include/uapi/linux/bpf.h > > > > +++ b/include/uapi/linux/bpf.h > > > > @@ -326,6 +326,7 @@ enum bpf_attach_type { > > > > #define BPF_F_NUMA_NODE (1U << 2) > > > > > > > > #define BPF_OBJ_NAME_LEN 16U > > > > +#define BPF_CREATED_COMM_LEN 16U > > > > > > Nack. > > > 16 bytes is going to be useless. > > > We found it the hard way with prog_name. > > > If you want to embed additional debug information > > > please use BTF for that. > > BTF was my natural choice initially, but then I saw created_by_uid and > > thought created_by_comm might have a chance :-) > > > > To clarify, by BTF you mean creating some unused global variable > > and use its name as the debugging info? Or there is some better way? > > I was thinking about adding new section to .btf.ext with this extra data, > but global variable is a better idea indeed. > We'd need to standardize such variables names, so that > bpftool can parse and print it while doing 'bpftool prog show'. +1, much better indeed. > We see more and more cases where services use more than > one program in single .c file to accomplish their goals. > Tying such debug info (like 'created_by_comm') to each program > individually isn't quite right. > In that sense global variables are better, since they cover the > whole .c file. > Beyond 'created_by_comm' there are others things that people > will likely want to know. > Like which version of llvm was used to compile this .o file. > Which unix user name compiled it. > The name of service/daemon that will be using this .o > and so on. Also latest git sha of the source repo, for example. > May be some standard prefix to such global variables will do? > Like "bpftool prog show" can scan global data for > "__annotate_#name" and print both name and string contents ? > For folks who regularly ssh into servers to debug bpf progs > that will help a lot. > May be some annotations llvm can automatically add to .o. > Thoughts? One thing that might be less clear is how information such as comm or comm args would be stuffed into BTF here, but perhaps these two wouldn't necessarily need to be part of it since these can be retrieved today (as in: "which program is currently holding a reference via fd to a certain prog/map"). For that bpftool could simply walk procfs once and correlate via fdinfo on unique prog/map id, so we could list comms in the dump which should be trivial to add: # ls -la /proc/30651/fd/10 lrwx------ 1 root root 64 Oct 16 15:53 /proc/30651/fd/10 -> anon_inode:bpf-map # cat /proc/30651/fdinfo/10 pos: 0 flags: 02000002 mnt_id: 15 map_type: 1 key_size: 24 value_size: 12 max_entries: 65536 map_flags: 0x0 memlock: 6819840 map_id: 384 <--- frozen: 0 # cat /proc/30651/comm cilium-agent # cat /proc/30651/cmdline ./daemon/cilium-agent--ipv4-range10.11.0.0/16[...]--enable-node-port=true ... and similar for progs. Getting the cmdline from kernel side seems rather annoying from looking into what detour procfs needs to perform. But aside from these, such annotations via BTF would be really useful. Thanks, Daniel
On 10/16, Daniel Borkmann wrote: > On Tue, Oct 15, 2019 at 02:21:50PM -0700, Alexei Starovoitov wrote: > > On Fri, Oct 11, 2019 at 5:38 PM Stanislav Fomichev <sdf@fomichev.me> wrote: > > > On 10/11, Alexei Starovoitov wrote: > > > > On Fri, Oct 11, 2019 at 9:21 AM Stanislav Fomichev <sdf@google.com> wrote: > > > > > > > > > > Even though we have the pointer to user_struct and can recover > > > > > uid of the user who has created the program, it usually contains > > > > > 0 (root) which is not very informative. Let's store the comm of the > > > > > calling process and export it via bpf_prog_info. This should help > > > > > answer the question "which process loaded this particular program". > > > > > > > > > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > > > > --- > > > > > include/linux/bpf.h | 1 + > > > > > include/uapi/linux/bpf.h | 2 ++ > > > > > kernel/bpf/syscall.c | 4 ++++ > > > > > 3 files changed, 7 insertions(+) > > > > > > > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > > > > index 5b9d22338606..b03ea396afe5 100644 > > > > > --- a/include/linux/bpf.h > > > > > +++ b/include/linux/bpf.h > > > > > @@ -421,6 +421,7 @@ struct bpf_prog_aux { > > > > > struct work_struct work; > > > > > struct rcu_head rcu; > > > > > }; > > > > > + char created_by_comm[BPF_CREATED_COMM_LEN]; > > > > > }; > > > > > > > > > > struct bpf_array { > > > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > > > > index a65c3b0c6935..4e883ecbba1e 100644 > > > > > --- a/include/uapi/linux/bpf.h > > > > > +++ b/include/uapi/linux/bpf.h > > > > > @@ -326,6 +326,7 @@ enum bpf_attach_type { > > > > > #define BPF_F_NUMA_NODE (1U << 2) > > > > > > > > > > #define BPF_OBJ_NAME_LEN 16U > > > > > +#define BPF_CREATED_COMM_LEN 16U > > > > > > > > Nack. > > > > 16 bytes is going to be useless. > > > > We found it the hard way with prog_name. > > > > If you want to embed additional debug information > > > > please use BTF for that. > > > BTF was my natural choice initially, but then I saw created_by_uid and > > > thought created_by_comm might have a chance :-) > > > > > > To clarify, by BTF you mean creating some unused global variable > > > and use its name as the debugging info? Or there is some better way? > > > > I was thinking about adding new section to .btf.ext with this extra data, > > but global variable is a better idea indeed. > > We'd need to standardize such variables names, so that > > bpftool can parse and print it while doing 'bpftool prog show'. > > +1, much better indeed. > > > We see more and more cases where services use more than > > one program in single .c file to accomplish their goals. > > Tying such debug info (like 'created_by_comm') to each program > > individually isn't quite right. > > In that sense global variables are better, since they cover the > > whole .c file. > > Beyond 'created_by_comm' there are others things that people > > will likely want to know. > > Like which version of llvm was used to compile this .o file. > > Which unix user name compiled it. > > The name of service/daemon that will be using this .o > > and so on. > > Also latest git sha of the source repo, for example. > > > May be some standard prefix to such global variables will do? > > Like "bpftool prog show" can scan global data for > > "__annotate_#name" and print both name and string contents ? > > For folks who regularly ssh into servers to debug bpf progs > > that will help a lot. > > May be some annotations llvm can automatically add to .o. > > Thoughts? > > One thing that might be less clear is how information such as comm > or comm args would be stuffed into BTF here, but perhaps these two > wouldn't necessarily need to be part of it since these can be retrieved > today (as in: "which program is currently holding a reference via fd > to a certain prog/map"). For that bpftool could simply walk procfs > once and correlate via fdinfo on unique prog/map id, so we could list > comms in the dump which should be trivial to add: > > # ls -la /proc/30651/fd/10 > lrwx------ 1 root root 64 Oct 16 15:53 /proc/30651/fd/10 -> anon_inode:bpf-map > # cat /proc/30651/fdinfo/10 > pos: 0 > flags: 02000002 > mnt_id: 15 > map_type: 1 > key_size: 24 > value_size: 12 > max_entries: 65536 > map_flags: 0x0 > memlock: 6819840 > map_id: 384 <--- > frozen: 0 > # cat /proc/30651/comm > cilium-agent > # cat /proc/30651/cmdline > ./daemon/cilium-agent--ipv4-range10.11.0.0/16[...]--enable-node-port=true > > ... and similar for progs. Getting the cmdline from kernel side seems > rather annoying from looking into what detour procfs needs to perform. > > But aside from these, such annotations via BTF would be really useful. Tried to do the following: 1. Add: static volatile const char __annotate_source1[] = __FILE__; to test_rdonly_maps.c and I think it got optimized away :-/ At least I don't see it in the 'bpftool btf dump' output. 2. Add: char __annotate_source2[] SEC(".meta") = __FILE__; to test_rdonly_maps.c and do all the required plumbing in libbpf to treat .meta like .rodata. I think it works, but the map disappears after bpftool exits because this data is not referenced in the prog and the refcount drops to zero :-( Am I missing something?
On Thu, Oct 17, 2019 at 9:28 AM Stanislav Fomichev <sdf@fomichev.me> wrote: > Tried to do the following: > > 1. Add: static volatile const char __annotate_source1[] = __FILE__; > to test_rdonly_maps.c and I think it got optimized away :-/ > At least I don't see it in the 'bpftool btf dump' output. > > 2. Add: char __annotate_source2[] SEC(".meta") = __FILE__; > to test_rdonly_maps.c and do all the required plumbing in libbpf > to treat .meta like .rodata. I think it works, but the map > disappears after bpftool exits because this data is not referenced > in the prog and the refcount drops to zero :-( > > Am I missing something? "Some assembly required". I think first variant should work at the end. I don't think extra section should be a requirement. Sounds like with 2 it's pretty close. Just need to make sure that prog side sees the map as referenced. And more importantly bpftool can find that map from prog later.
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 5b9d22338606..b03ea396afe5 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -421,6 +421,7 @@ struct bpf_prog_aux { struct work_struct work; struct rcu_head rcu; }; + char created_by_comm[BPF_CREATED_COMM_LEN]; }; struct bpf_array { diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index a65c3b0c6935..4e883ecbba1e 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -326,6 +326,7 @@ enum bpf_attach_type { #define BPF_F_NUMA_NODE (1U << 2) #define BPF_OBJ_NAME_LEN 16U +#define BPF_CREATED_COMM_LEN 16U /* Flags for accessing BPF object from syscall side. */ #define BPF_F_RDONLY (1U << 3) @@ -3252,6 +3253,7 @@ struct bpf_prog_info { __aligned_u64 prog_tags; __u64 run_time_ns; __u64 run_cnt; + char created_by_comm[BPF_CREATED_COMM_LEN]; } __attribute__((aligned(8))); struct bpf_map_info { diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 82eabd4e38ad..51c125292eaf 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -1735,6 +1735,8 @@ static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr) bpf_prog_kallsyms_add(prog); perf_event_bpf_event(prog, PERF_BPF_EVENT_PROG_LOAD, 0); + get_task_comm(prog->aux->created_by_comm, current); + err = bpf_prog_new_fd(prog); if (err < 0) bpf_prog_put(prog); @@ -2337,6 +2339,8 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog, memcpy(info.tag, prog->tag, sizeof(prog->tag)); memcpy(info.name, prog->aux->name, sizeof(prog->aux->name)); + memcpy(info.created_by_comm, prog->aux->created_by_comm, + sizeof(prog->aux->created_by_comm)); ulen = info.nr_map_ids; info.nr_map_ids = prog->aux->used_map_cnt;
Even though we have the pointer to user_struct and can recover uid of the user who has created the program, it usually contains 0 (root) which is not very informative. Let's store the comm of the calling process and export it via bpf_prog_info. This should help answer the question "which process loaded this particular program". Signed-off-by: Stanislav Fomichev <sdf@google.com> --- include/linux/bpf.h | 1 + include/uapi/linux/bpf.h | 2 ++ kernel/bpf/syscall.c | 4 ++++ 3 files changed, 7 insertions(+)