| Message ID | 20210416202404.3443623-11-andrii@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | BPF static linker: support externs | expand |
On 4/16/21 1:23 PM, Andrii Nakryiko wrote: > It should never fail, but if it does, it's better to know about this rather > than end up with nonsensical type IDs. So this is defensive programming. Maybe do another round of audit of the callers and if you didn't find any issue, you do not need to check not-happening condition here? > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > --- > tools/lib/bpf/linker.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c > index 283249df9831..d5dc1d401f57 100644 > --- a/tools/lib/bpf/linker.c > +++ b/tools/lib/bpf/linker.c > @@ -1423,6 +1423,15 @@ static int linker_fixup_btf(struct src_obj *obj) > static int remap_type_id(__u32 *type_id, void *ctx) > { > int *id_map = ctx; > + int new_id = id_map[*type_id]; > + > + if (*type_id == 0) > + return 0; > + > + if (new_id == 0) { > + pr_warn("failed to find new ID mapping for original BTF type ID %u\n", *type_id); > + return -EINVAL; > + } > > *type_id = id_map[*type_id]; > >
On Thu, Apr 22, 2021 at 9:50 AM Yonghong Song <yhs@fb.com> wrote: > > > > On 4/16/21 1:23 PM, Andrii Nakryiko wrote: > > It should never fail, but if it does, it's better to know about this rather > > than end up with nonsensical type IDs. > > So this is defensive programming. Maybe do another round of > audit of the callers and if you didn't find any issue, you > do not need to check not-happening condition here? It's far from obvious that this will never happen, because we do a decently complicated BTF processing (we skip some types altogether believing that they are not used, for example) and it will only get more complicated with time. Just as there are "verifier bug" checks in kernel, this prevents things from going wild if non-trivial bugs will inevitably happen. > > > > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > > --- > > tools/lib/bpf/linker.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c > > index 283249df9831..d5dc1d401f57 100644 > > --- a/tools/lib/bpf/linker.c > > +++ b/tools/lib/bpf/linker.c > > @@ -1423,6 +1423,15 @@ static int linker_fixup_btf(struct src_obj *obj) > > static int remap_type_id(__u32 *type_id, void *ctx) > > { > > int *id_map = ctx; > > + int new_id = id_map[*type_id]; > > + > > + if (*type_id == 0) > > + return 0; > > + > > + if (new_id == 0) { > > + pr_warn("failed to find new ID mapping for original BTF type ID %u\n", *type_id); > > + return -EINVAL; > > + } > > > > *type_id = id_map[*type_id]; > > > >
On Thu, Apr 22, 2021 at 11:24 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Thu, Apr 22, 2021 at 9:50 AM Yonghong Song <yhs@fb.com> wrote: > > > > > > > > On 4/16/21 1:23 PM, Andrii Nakryiko wrote: > > > It should never fail, but if it does, it's better to know about this rather > > > than end up with nonsensical type IDs. > > > > So this is defensive programming. Maybe do another round of > > audit of the callers and if you didn't find any issue, you > > do not need to check not-happening condition here? > > It's far from obvious that this will never happen, because we do a > decently complicated BTF processing (we skip some types altogether > believing that they are not used, for example) and it will only get > more complicated with time. Just as there are "verifier bug" checks in > kernel, this prevents things from going wild if non-trivial bugs will > inevitably happen. I agree with Yonghong. This doesn't look right. The callback will be called for all non-void types, right? so *type_id == 0 shouldn't never happen. If it does there is a bug somewhere that should be investigated instead of ignored. The if (new_id == 0) pr_warn bit makes sense. My reading that it will abort the whole linking process and this linker bug will be reported back to us. So it's good.
On Thu, Apr 22, 2021 at 7:54 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Thu, Apr 22, 2021 at 11:24 AM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Thu, Apr 22, 2021 at 9:50 AM Yonghong Song <yhs@fb.com> wrote: > > > > > > > > > > > > On 4/16/21 1:23 PM, Andrii Nakryiko wrote: > > > > It should never fail, but if it does, it's better to know about this rather > > > > than end up with nonsensical type IDs. > > > > > > So this is defensive programming. Maybe do another round of > > > audit of the callers and if you didn't find any issue, you > > > do not need to check not-happening condition here? > > > > It's far from obvious that this will never happen, because we do a > > decently complicated BTF processing (we skip some types altogether > > believing that they are not used, for example) and it will only get > > more complicated with time. Just as there are "verifier bug" checks in > > kernel, this prevents things from going wild if non-trivial bugs will > > inevitably happen. > > I agree with Yonghong. This doesn't look right. I read it as Yonghong was asking about the entire patch. You seem to be concerned with one particular check, right? > The callback will be called for all non-void types, right? > so *type_id == 0 shouldn't never happen. > If it does there is a bug somewhere that should be investigated > instead of ignored. See btf_type_visit_type_ids() and btf_ext_visit_type_ids(), they call callback for every field that contains type ID, even if it points to VOID. So this can happen and is expected. > The > if (new_id == 0) pr_warn > bit makes sense. Right, and this is the point of this patch. id_map[] will have zeroes for any unmapped type, so I just need to make sure I'm not false erroring on id_map[0] (== 0, which is valid, but never used). > My reading that it will abort the whole linking process and > this linker bug will be reported back to us. > So it's good. Right, it will be propagated all the way up.
On Thu, Apr 22, 2021 at 9:25 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Thu, Apr 22, 2021 at 7:54 PM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Thu, Apr 22, 2021 at 11:24 AM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > > > > > > On Thu, Apr 22, 2021 at 9:50 AM Yonghong Song <yhs@fb.com> wrote: > > > > > > > > > > > > > > > > On 4/16/21 1:23 PM, Andrii Nakryiko wrote: > > > > > It should never fail, but if it does, it's better to know about this rather > > > > > than end up with nonsensical type IDs. > > > > > > > > So this is defensive programming. Maybe do another round of > > > > audit of the callers and if you didn't find any issue, you > > > > do not need to check not-happening condition here? > > > > > > It's far from obvious that this will never happen, because we do a > > > decently complicated BTF processing (we skip some types altogether > > > believing that they are not used, for example) and it will only get > > > more complicated with time. Just as there are "verifier bug" checks in > > > kernel, this prevents things from going wild if non-trivial bugs will > > > inevitably happen. > > > > I agree with Yonghong. This doesn't look right. > > I read it as Yonghong was asking about the entire patch. You seem to > be concerned with one particular check, right? > > > The callback will be called for all non-void types, right? > > so *type_id == 0 shouldn't never happen. > > If it does there is a bug somewhere that should be investigated > > instead of ignored. > > See btf_type_visit_type_ids() and btf_ext_visit_type_ids(), they call > callback for every field that contains type ID, even if it points to > VOID. So this can happen and is expected. I see. So something like 'extern cosnt void foo __ksym' would point to void type? But then why is it not a part of the id_map[] and has to be handled explicitly? > > The > > if (new_id == 0) pr_warn > > bit makes sense. > > Right, and this is the point of this patch. id_map[] will have zeroes > for any unmapped type, so I just need to make sure I'm not false > erroring on id_map[0] (== 0, which is valid, but never used). Right, id_map[0] should be 0. I'm still missing something in this combination of 'if's. May be do it as: if (new_id == 0 && *type_id != 0) { pr_warn ? That was the idea?
On Thu, Apr 22, 2021 at 10:11 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Thu, Apr 22, 2021 at 9:25 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Thu, Apr 22, 2021 at 7:54 PM Alexei Starovoitov > > <alexei.starovoitov@gmail.com> wrote: > > > > > > On Thu, Apr 22, 2021 at 11:24 AM Andrii Nakryiko > > > <andrii.nakryiko@gmail.com> wrote: > > > > > > > > On Thu, Apr 22, 2021 at 9:50 AM Yonghong Song <yhs@fb.com> wrote: > > > > > > > > > > > > > > > > > > > > On 4/16/21 1:23 PM, Andrii Nakryiko wrote: > > > > > > It should never fail, but if it does, it's better to know about this rather > > > > > > than end up with nonsensical type IDs. > > > > > > > > > > So this is defensive programming. Maybe do another round of > > > > > audit of the callers and if you didn't find any issue, you > > > > > do not need to check not-happening condition here? > > > > > > > > It's far from obvious that this will never happen, because we do a > > > > decently complicated BTF processing (we skip some types altogether > > > > believing that they are not used, for example) and it will only get > > > > more complicated with time. Just as there are "verifier bug" checks in > > > > kernel, this prevents things from going wild if non-trivial bugs will > > > > inevitably happen. > > > > > > I agree with Yonghong. This doesn't look right. > > > > I read it as Yonghong was asking about the entire patch. You seem to > > be concerned with one particular check, right? > > > > > The callback will be called for all non-void types, right? > > > so *type_id == 0 shouldn't never happen. > > > If it does there is a bug somewhere that should be investigated > > > instead of ignored. > > > > See btf_type_visit_type_ids() and btf_ext_visit_type_ids(), they call > > callback for every field that contains type ID, even if it points to > > VOID. So this can happen and is expected. > > I see. So something like 'extern cosnt void foo __ksym' would > point to void type? > But then why is it not a part of the id_map[] and has > to be handled explicitly? const void foo will be VAR -> CONST -> VOID. But any `void *` anywhere will be PTR -> VOID. Any void bla(int x) would have return type VOID (0), and so on. There are a lot of cases when we use VOID as type_id. VOID always is handled specially, because it stays zero despite any transformation: during BTF concatenation, BTF dedup, BTF generation, etc. > > > > The > > > if (new_id == 0) pr_warn > > > bit makes sense. > > > > Right, and this is the point of this patch. id_map[] will have zeroes > > for any unmapped type, so I just need to make sure I'm not false > > erroring on id_map[0] (== 0, which is valid, but never used). > > Right, id_map[0] should be 0. > I'm still missing something in this combination of 'if's. > May be do it as: > if (new_id == 0 && *type_id != 0) { pr_warn > ? > That was the idea? That's the idea, there is just no need to do VOID -> VOID transformation, but I'll rewrite it to a combined if if it makes it easier to follow. Here's full source of remap_type_id with few comments to added: static int remap_type_id(__u32 *type_id, void *ctx) { int *id_map = ctx; int new_id = id_map[*type_id]; /* Here VOID stays VOID, that's all */ if (*type_id == 0) return 0; /* This means whatever type we are trying to remap didn't get a new ID assigned in linker->btf and that's an error */ if (new_id == 0) { pr_warn("failed to find new ID mapping for original BTF type ID %u\n", *type_id); return -EINVAL; } *type_id = id_map[*type_id]; return 0; }
On Fri, Apr 23, 2021 at 9:09 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Thu, Apr 22, 2021 at 10:11 PM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Thu, Apr 22, 2021 at 9:25 PM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > > > > > > On Thu, Apr 22, 2021 at 7:54 PM Alexei Starovoitov > > > <alexei.starovoitov@gmail.com> wrote: > > > > > > > > On Thu, Apr 22, 2021 at 11:24 AM Andrii Nakryiko > > > > <andrii.nakryiko@gmail.com> wrote: > > > > > > > > > > On Thu, Apr 22, 2021 at 9:50 AM Yonghong Song <yhs@fb.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > > On 4/16/21 1:23 PM, Andrii Nakryiko wrote: > > > > > > > It should never fail, but if it does, it's better to know about this rather > > > > > > > than end up with nonsensical type IDs. > > > > > > > > > > > > So this is defensive programming. Maybe do another round of > > > > > > audit of the callers and if you didn't find any issue, you > > > > > > do not need to check not-happening condition here? > > > > > > > > > > It's far from obvious that this will never happen, because we do a > > > > > decently complicated BTF processing (we skip some types altogether > > > > > believing that they are not used, for example) and it will only get > > > > > more complicated with time. Just as there are "verifier bug" checks in > > > > > kernel, this prevents things from going wild if non-trivial bugs will > > > > > inevitably happen. > > > > > > > > I agree with Yonghong. This doesn't look right. > > > > > > I read it as Yonghong was asking about the entire patch. You seem to > > > be concerned with one particular check, right? > > > > > > > The callback will be called for all non-void types, right? > > > > so *type_id == 0 shouldn't never happen. > > > > If it does there is a bug somewhere that should be investigated > > > > instead of ignored. > > > > > > See btf_type_visit_type_ids() and btf_ext_visit_type_ids(), they call > > > callback for every field that contains type ID, even if it points to > > > VOID. So this can happen and is expected. > > > > I see. So something like 'extern cosnt void foo __ksym' would > > point to void type? > > But then why is it not a part of the id_map[] and has > > to be handled explicitly? > > const void foo will be VAR -> CONST -> VOID. But any `void *` anywhere > will be PTR -> VOID. Any void bla(int x) would have return type VOID > (0), and so on. There are a lot of cases when we use VOID as type_id. > VOID always is handled specially, because it stays zero despite any > transformation: during BTF concatenation, BTF dedup, BTF generation, > etc. > > > > > > > The > > > > if (new_id == 0) pr_warn > > > > bit makes sense. > > > > > > Right, and this is the point of this patch. id_map[] will have zeroes > > > for any unmapped type, so I just need to make sure I'm not false > > > erroring on id_map[0] (== 0, which is valid, but never used). > > > > Right, id_map[0] should be 0. > > I'm still missing something in this combination of 'if's. > > May be do it as: > > if (new_id == 0 && *type_id != 0) { pr_warn > > ? > > That was the idea? > > That's the idea, there is just no need to do VOID -> VOID > transformation, but I'll rewrite it to a combined if if it makes it > easier to follow. Here's full source of remap_type_id with few > comments to added: > > static int remap_type_id(__u32 *type_id, void *ctx) > { > int *id_map = ctx; > int new_id = id_map[*type_id]; > > > /* Here VOID stays VOID, that's all */ > > if (*type_id == 0) > return 0; Does it mean that id_map[0] is a garbage value? and all other code that might be doing id_map[idx] might be reading garbage if it doesn't have a check for idx == 0 ? > /* This means whatever type we are trying to remap didn't get a new ID > assigned in linker->btf and that's an error */ > if (new_id == 0) { > pr_warn("failed to find new ID mapping for original > BTF type ID %u\n", *type_id); > return -EINVAL; > } > > *type_id = id_map[*type_id]; > > return 0; > }
On Fri, Apr 23, 2021 at 9:18 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Fri, Apr 23, 2021 at 9:09 AM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Thu, Apr 22, 2021 at 10:11 PM Alexei Starovoitov > > <alexei.starovoitov@gmail.com> wrote: > > > > > > On Thu, Apr 22, 2021 at 9:25 PM Andrii Nakryiko > > > <andrii.nakryiko@gmail.com> wrote: > > > > > > > > On Thu, Apr 22, 2021 at 7:54 PM Alexei Starovoitov > > > > <alexei.starovoitov@gmail.com> wrote: > > > > > > > > > > On Thu, Apr 22, 2021 at 11:24 AM Andrii Nakryiko > > > > > <andrii.nakryiko@gmail.com> wrote: > > > > > > > > > > > > On Thu, Apr 22, 2021 at 9:50 AM Yonghong Song <yhs@fb.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 4/16/21 1:23 PM, Andrii Nakryiko wrote: > > > > > > > > It should never fail, but if it does, it's better to know about this rather > > > > > > > > than end up with nonsensical type IDs. > > > > > > > > > > > > > > So this is defensive programming. Maybe do another round of > > > > > > > audit of the callers and if you didn't find any issue, you > > > > > > > do not need to check not-happening condition here? > > > > > > > > > > > > It's far from obvious that this will never happen, because we do a > > > > > > decently complicated BTF processing (we skip some types altogether > > > > > > believing that they are not used, for example) and it will only get > > > > > > more complicated with time. Just as there are "verifier bug" checks in > > > > > > kernel, this prevents things from going wild if non-trivial bugs will > > > > > > inevitably happen. > > > > > > > > > > I agree with Yonghong. This doesn't look right. > > > > > > > > I read it as Yonghong was asking about the entire patch. You seem to > > > > be concerned with one particular check, right? > > > > > > > > > The callback will be called for all non-void types, right? > > > > > so *type_id == 0 shouldn't never happen. > > > > > If it does there is a bug somewhere that should be investigated > > > > > instead of ignored. > > > > > > > > See btf_type_visit_type_ids() and btf_ext_visit_type_ids(), they call > > > > callback for every field that contains type ID, even if it points to > > > > VOID. So this can happen and is expected. > > > > > > I see. So something like 'extern cosnt void foo __ksym' would > > > point to void type? > > > But then why is it not a part of the id_map[] and has > > > to be handled explicitly? > > > > const void foo will be VAR -> CONST -> VOID. But any `void *` anywhere > > will be PTR -> VOID. Any void bla(int x) would have return type VOID > > (0), and so on. There are a lot of cases when we use VOID as type_id. > > VOID always is handled specially, because it stays zero despite any > > transformation: during BTF concatenation, BTF dedup, BTF generation, > > etc. > > > > > > > > > > The > > > > > if (new_id == 0) pr_warn > > > > > bit makes sense. > > > > > > > > Right, and this is the point of this patch. id_map[] will have zeroes > > > > for any unmapped type, so I just need to make sure I'm not false > > > > erroring on id_map[0] (== 0, which is valid, but never used). > > > > > > Right, id_map[0] should be 0. > > > I'm still missing something in this combination of 'if's. > > > May be do it as: > > > if (new_id == 0 && *type_id != 0) { pr_warn > > > ? > > > That was the idea? > > > > That's the idea, there is just no need to do VOID -> VOID > > transformation, but I'll rewrite it to a combined if if it makes it > > easier to follow. Here's full source of remap_type_id with few > > comments to added: > > > > static int remap_type_id(__u32 *type_id, void *ctx) > > { > > int *id_map = ctx; > > int new_id = id_map[*type_id]; > > > > > > /* Here VOID stays VOID, that's all */ > > > > if (*type_id == 0) > > return 0; > > Does it mean that id_map[0] is a garbage value? > and all other code that might be doing id_map[idx] might be reading > garbage if it doesn't have a check for idx == 0 ? No, id_map[0] == 0 by construction (id_map is obj->btf_type_map and is calloc()'ed) and can be used as id_map[idx]. > > > /* This means whatever type we are trying to remap didn't get a new ID > > assigned in linker->btf and that's an error */ > > if (new_id == 0) { > > pr_warn("failed to find new ID mapping for original > > BTF type ID %u\n", *type_id); > > return -EINVAL; > > } > > > > *type_id = id_map[*type_id]; > > > > return 0; > > }
On Fri, Apr 23, 2021 at 9:31 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > > > > > static int remap_type_id(__u32 *type_id, void *ctx) > > > { > > > int *id_map = ctx; > > > int new_id = id_map[*type_id]; > > > > > > > > > /* Here VOID stays VOID, that's all */ > > > > > > if (*type_id == 0) > > > return 0; > > > > Does it mean that id_map[0] is a garbage value? > > and all other code that might be doing id_map[idx] might be reading > > garbage if it doesn't have a check for idx == 0 ? > > No, id_map[0] == 0 by construction (id_map is obj->btf_type_map and is > calloc()'ed) and can be used as id_map[idx]. Ok. Then why are you insisting on this micro optimization to return 0 directly? That's the confusing part for me. If it was: "if (new_id == 0 && *type_id != 0) { pr_warn" Then it would be clear what error condition is about. But 'return 0' messing things up in my mind, because it's far from obvious that first check is really a combination with the 2nd check and by itself it's a micro optimization to avoid reading id_map[0].
On Fri, Apr 23, 2021 at 9:34 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Fri, Apr 23, 2021 at 9:31 AM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > > > > > static int remap_type_id(__u32 *type_id, void *ctx) > > > > { > > > > int *id_map = ctx; > > > > int new_id = id_map[*type_id]; > > > > > > > > > > > > /* Here VOID stays VOID, that's all */ > > > > > > > > if (*type_id == 0) > > > > return 0; > > > > > > Does it mean that id_map[0] is a garbage value? > > > and all other code that might be doing id_map[idx] might be reading > > > garbage if it doesn't have a check for idx == 0 ? > > > > No, id_map[0] == 0 by construction (id_map is obj->btf_type_map and is > > calloc()'ed) and can be used as id_map[idx]. > > Ok. Then why are you insisting on this micro optimization to return 0 > directly? > That's the confusing part for me. I'm not insisting: > but I'll rewrite it to a combined if if it makes it easier to follow So I'm confused why you are confused. > > If it was: > "if (new_id == 0 && *type_id != 0) { pr_warn" > Then it would be clear what error condition is about. > But 'return 0' messing things up in my mind, > because it's far from obvious that first check is really a combination > with the 2nd check and by itself it's a micro optimization to avoid > reading id_map[0]. I didn't try to micro optimize, that's how I naturally think about the problem. I'll rewrite the if, don't know why we are spending emails on this still.
diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c index 283249df9831..d5dc1d401f57 100644 --- a/tools/lib/bpf/linker.c +++ b/tools/lib/bpf/linker.c @@ -1423,6 +1423,15 @@ static int linker_fixup_btf(struct src_obj *obj) static int remap_type_id(__u32 *type_id, void *ctx) { int *id_map = ctx; + int new_id = id_map[*type_id]; + + if (*type_id == 0) + return 0; + + if (new_id == 0) { + pr_warn("failed to find new ID mapping for original BTF type ID %u\n", *type_id); + return -EINVAL; + } *type_id = id_map[*type_id];
It should never fail, but if it does, it's better to know about this rather than end up with nonsensical type IDs. Signed-off-by: Andrii Nakryiko <andrii@kernel.org> --- tools/lib/bpf/linker.c | 9 +++++++++ 1 file changed, 9 insertions(+)