Hi,
Enclosed are a pair of patches for an oops that can occur if an exception is
generated while a bpf subprogram is running. One of the bpf_prog_aux entries
for the subprograms are missing an extable. This can lead to an exception that
would otherwise be handled turning into a NULL pointer bug.
The bulk of the change here is simply adding a pair of programs for the
selftest. The proposed fix in this iteration is a 1-line change.
These changes were tested via the verifier and progs selftests and no
regressions were observed.
Changes from v1:
- Add a selftest (Feedback From Alexei Starovoitov)
- Move to a 1-line verifier change instead of searching multiple extables
Krister Johansen (2):
Add a selftest for subprogram extables
bpf: ensure main program has an extable
kernel/bpf/verifier.c | 1 +
.../bpf/prog_tests/subprogs_extable.c | 35 +++++++++
.../bpf/progs/test_subprogs_extable.c | 71 +++++++++++++++++++
3 files changed, 107 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/subprogs_extable.c
create mode 100644 tools/testing/selftests/bpf/progs/test_subprogs_extable.c
--
2.25.1
When bpf subprograms are in use, the main program is not jit'd after the
subprograms because jit_subprogs sets a value for prog->bpf_func upon
success. Subsequent calls to the JIT are bypassed when this value is
non-NULL. This leads to a situation where the main program and its
func[0] counterpart are both in the bpf kallsyms tree, but only func[0]
has an extable. Extables are only created during JIT. Now there are
two nearly identical program ksym entries in the tree, but only one has
an extable. Depending upon how the entries are placed, there's a chance
that a fault will call search_extable on the aux with the NULL entry.
Since jit_subprogs already copies state from func[0] to the main
program, include the extable pointer in this state duplication. The
alternative is to skip adding the main program to the bpf_kallsyms
table, but that would mean adding a check for subprograms into the
middle of bpf_prog_load.
Cc: [email protected]
Fixes: 1c2a088a6626 ("bpf: x64: add JIT support for multi-function programs")
Signed-off-by: Krister Johansen <[email protected]>
---
kernel/bpf/verifier.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 5871aa78d01a..d6939db9fbf9 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -17242,6 +17242,7 @@ static int jit_subprogs(struct bpf_verifier_env *env)
prog->jited = 1;
prog->bpf_func = func[0]->bpf_func;
prog->jited_len = func[0]->jited_len;
+ prog->aux->extable = func[0]->aux->extable;
prog->aux->func = func;
prog->aux->func_cnt = env->subprog_cnt;
bpf_prog_jit_attempt_done(prog);
--
2.25.1
On 6/7/23 2:04 PM, Krister Johansen wrote:
> When bpf subprograms are in use, the main program is not jit'd after the
> subprograms because jit_subprogs sets a value for prog->bpf_func upon
> success. Subsequent calls to the JIT are bypassed when this value is
> non-NULL. This leads to a situation where the main program and its
> func[0] counterpart are both in the bpf kallsyms tree, but only func[0]
> has an extable. Extables are only created during JIT. Now there are
> two nearly identical program ksym entries in the tree, but only one has
> an extable. Depending upon how the entries are placed, there's a chance
> that a fault will call search_extable on the aux with the NULL entry.
>
> Since jit_subprogs already copies state from func[0] to the main
> program, include the extable pointer in this state duplication. The
> alternative is to skip adding the main program to the bpf_kallsyms
> table, but that would mean adding a check for subprograms into the
> middle of bpf_prog_load.
I think having two early identical program ksym entries is bad.
When people 'cat /proc/kallsyms | grep <their program name>',
they will find two programs with identical kernel address but different
hash value. This is just very confusing. I think removing the
duplicate in kallsyms is better from user's perspective.
>
> Cc: [email protected]
> Fixes: 1c2a088a6626 ("bpf: x64: add JIT support for multi-function programs")
> Signed-off-by: Krister Johansen <[email protected]>
> ---
> kernel/bpf/verifier.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 5871aa78d01a..d6939db9fbf9 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -17242,6 +17242,7 @@ static int jit_subprogs(struct bpf_verifier_env *env)
> prog->jited = 1;
> prog->bpf_func = func[0]->bpf_func;
> prog->jited_len = func[0]->jited_len;
> + prog->aux->extable = func[0]->aux->extable;
> prog->aux->func = func;
> prog->aux->func_cnt = env->subprog_cnt;
> bpf_prog_jit_attempt_done(prog);
On Wed, Jun 7, 2023 at 2:04 PM Krister Johansen <[email protected]> wrote:
>
> When bpf subprograms are in use, the main program is not jit'd after the
> subprograms because jit_subprogs sets a value for prog->bpf_func upon
> success. Subsequent calls to the JIT are bypassed when this value is
> non-NULL. This leads to a situation where the main program and its
> func[0] counterpart are both in the bpf kallsyms tree, but only func[0]
> has an extable. Extables are only created during JIT. Now there are
> two nearly identical program ksym entries in the tree, but only one has
> an extable. Depending upon how the entries are placed, there's a chance
> that a fault will call search_extable on the aux with the NULL entry.
>
> Since jit_subprogs already copies state from func[0] to the main
> program, include the extable pointer in this state duplication. The
> alternative is to skip adding the main program to the bpf_kallsyms
> table, but that would mean adding a check for subprograms into the
> middle of bpf_prog_load.
adding a check to bpf_prog_load() isn't great. that's true, but...
> Cc: [email protected]
> Fixes: 1c2a088a6626 ("bpf: x64: add JIT support for multi-function programs")
> Signed-off-by: Krister Johansen <[email protected]>
> ---
> kernel/bpf/verifier.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 5871aa78d01a..d6939db9fbf9 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -17242,6 +17242,7 @@ static int jit_subprogs(struct bpf_verifier_env *env)
> prog->jited = 1;
> prog->bpf_func = func[0]->bpf_func;
> prog->jited_len = func[0]->jited_len;
> + prog->aux->extable = func[0]->aux->extable;
Why not to do this hunk and what I suggested earlier: start from func=1 ?
That will address double ksym insertion that Yonghong mentioned.
On Thu, Jun 08, 2023 at 10:38:12AM -0700, Yonghong Song wrote:
>
>
> On 6/7/23 2:04 PM, Krister Johansen wrote:
> > When bpf subprograms are in use, the main program is not jit'd after the
> > subprograms because jit_subprogs sets a value for prog->bpf_func upon
> > success. Subsequent calls to the JIT are bypassed when this value is
> > non-NULL. This leads to a situation where the main program and its
> > func[0] counterpart are both in the bpf kallsyms tree, but only func[0]
> > has an extable. Extables are only created during JIT. Now there are
> > two nearly identical program ksym entries in the tree, but only one has
> > an extable. Depending upon how the entries are placed, there's a chance
> > that a fault will call search_extable on the aux with the NULL entry.
> >
> > Since jit_subprogs already copies state from func[0] to the main
> > program, include the extable pointer in this state duplication. The
> > alternative is to skip adding the main program to the bpf_kallsyms
> > table, but that would mean adding a check for subprograms into the
> > middle of bpf_prog_load.
>
> I think having two early identical program ksym entries is bad.
> When people 'cat /proc/kallsyms | grep <their program name>',
> they will find two programs with identical kernel address but different
> hash value. This is just very confusing. I think removing the
> duplicate in kallsyms is better from user's perspective.
Thanks for all the feedback.
In terms of resolving this confusion my inclination is to use the main
program. That way users see in kallsyms the same tag that is reported by
bpftool. On the other hand, the tag in kallsyms won't match the sha1 of
that actual chunk of code. Is anything relying on the hash in the tag
and the digest of the code agreeing?
-K
On Thu, Jun 08, 2023 at 03:01:36PM -0700, Alexei Starovoitov wrote:
> On Wed, Jun 7, 2023 at 2:04 PM Krister Johansen <[email protected]> wrote:
> > Cc: [email protected]
> > Fixes: 1c2a088a6626 ("bpf: x64: add JIT support for multi-function programs")
> > Signed-off-by: Krister Johansen <[email protected]>
> > ---
> > kernel/bpf/verifier.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 5871aa78d01a..d6939db9fbf9 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -17242,6 +17242,7 @@ static int jit_subprogs(struct bpf_verifier_env *env)
> > prog->jited = 1;
> > prog->bpf_func = func[0]->bpf_func;
> > prog->jited_len = func[0]->jited_len;
> > + prog->aux->extable = func[0]->aux->extable;
>
> Why not to do this hunk and what I suggested earlier: start from func=1 ?
> That will address double ksym insertion that Yonghong mentioned.
Sure thing. Yonghong and you have convinced me.
I'll send out a v3 with all changes requested so far.
-K