| Message ID | 20181101070058.2760251-3-songliubraving@fb.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | show more accurrate bpf program address | expand |
On 11/01/2018 08:00 AM, Song Liu wrote: > Currently, jited_ksyms in bpf_prog_info shows page addresses of jited > bpf program. This is not ideal for detailed profiling (find hot > instructions from stack traces). This patch replaces the page address > with real prog start address. > > Signed-off-by: Song Liu <songliubraving@fb.com> > --- > kernel/bpf/syscall.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > index ccb93277aae2..34a9eef5992c 100644 > --- a/kernel/bpf/syscall.c > +++ b/kernel/bpf/syscall.c > @@ -2172,7 +2172,6 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog, > user_ksyms = u64_to_user_ptr(info.jited_ksyms); > for (i = 0; i < ulen; i++) { > ksym_addr = (ulong) prog->aux->func[i]->bpf_func; > - ksym_addr &= PAGE_MASK; Note that the masking was done on purpose here and in patch 1/3 in order to not expose randomized start address to kallsyms at least. I suppose it's okay to change it here and for kallsyms given bpf_prog_get_info_by_fd() dump is for root only, and in each of the two cases we additionally apply kallsyms_show_value() logic, so for unpriv this is zeroed out plus only root loaded programs are added under kallsyms (capable(CAP_SYS_ADMIN)) anyway. > if (put_user((u64) ksym_addr, &user_ksyms[i])) > return -EFAULT; > } >
On 11/02/2018 11:09 AM, Daniel Borkmann wrote: > On 11/01/2018 08:00 AM, Song Liu wrote: >> Currently, jited_ksyms in bpf_prog_info shows page addresses of jited >> bpf program. This is not ideal for detailed profiling (find hot >> instructions from stack traces). This patch replaces the page address >> with real prog start address. >> >> Signed-off-by: Song Liu <songliubraving@fb.com> >> --- >> kernel/bpf/syscall.c | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c >> index ccb93277aae2..34a9eef5992c 100644 >> --- a/kernel/bpf/syscall.c >> +++ b/kernel/bpf/syscall.c >> @@ -2172,7 +2172,6 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog, >> user_ksyms = u64_to_user_ptr(info.jited_ksyms); >> for (i = 0; i < ulen; i++) { >> ksym_addr = (ulong) prog->aux->func[i]->bpf_func; >> - ksym_addr &= PAGE_MASK; > > Note that the masking was done on purpose here and in patch 1/3 in order to > not expose randomized start address to kallsyms at least. I suppose it's > okay to change it here and for kallsyms given bpf_prog_get_info_by_fd() dump > is for root only, and in each of the two cases we additionally apply > kallsyms_show_value() logic, so for unpriv this is zeroed out plus only root > loaded programs are added under kallsyms (capable(CAP_SYS_ADMIN)) anyway. (Btw, something like above should have been in changelog to provide some more historical context of why we used to do it like that and explaining why it is okay to change it this way.) >> if (put_user((u64) ksym_addr, &user_ksyms[i])) >> return -EFAULT; >> } >> >
> On Nov 2, 2018, at 3:19 AM, Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 11/02/2018 11:09 AM, Daniel Borkmann wrote: >> On 11/01/2018 08:00 AM, Song Liu wrote: >>> Currently, jited_ksyms in bpf_prog_info shows page addresses of jited >>> bpf program. This is not ideal for detailed profiling (find hot >>> instructions from stack traces). This patch replaces the page address >>> with real prog start address. >>> >>> Signed-off-by: Song Liu <songliubraving@fb.com> >>> --- >>> kernel/bpf/syscall.c | 1 - >>> 1 file changed, 1 deletion(-) >>> >>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c >>> index ccb93277aae2..34a9eef5992c 100644 >>> --- a/kernel/bpf/syscall.c >>> +++ b/kernel/bpf/syscall.c >>> @@ -2172,7 +2172,6 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog, >>> user_ksyms = u64_to_user_ptr(info.jited_ksyms); >>> for (i = 0; i < ulen; i++) { >>> ksym_addr = (ulong) prog->aux->func[i]->bpf_func; >>> - ksym_addr &= PAGE_MASK; >> >> Note that the masking was done on purpose here and in patch 1/3 in order to >> not expose randomized start address to kallsyms at least. I suppose it's >> okay to change it here and for kallsyms given bpf_prog_get_info_by_fd() dump >> is for root only, and in each of the two cases we additionally apply >> kallsyms_show_value() logic, so for unpriv this is zeroed out plus only root >> loaded programs are added under kallsyms (capable(CAP_SYS_ADMIN)) anyway. > > (Btw, something like above should have been in changelog to provide some more > historical context of why we used to do it like that and explaining why it is > okay to change it this way.) Thanks Daniel! I will send v2 with these fixes. Song
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index ccb93277aae2..34a9eef5992c 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -2172,7 +2172,6 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog, user_ksyms = u64_to_user_ptr(info.jited_ksyms); for (i = 0; i < ulen; i++) { ksym_addr = (ulong) prog->aux->func[i]->bpf_func; - ksym_addr &= PAGE_MASK; if (put_user((u64) ksym_addr, &user_ksyms[i])) return -EFAULT; }
Currently, jited_ksyms in bpf_prog_info shows page addresses of jited bpf program. This is not ideal for detailed profiling (find hot instructions from stack traces). This patch replaces the page address with real prog start address. Signed-off-by: Song Liu <songliubraving@fb.com> --- kernel/bpf/syscall.c | 1 - 1 file changed, 1 deletion(-)