| Message ID | 20230215235931.380197-2-iii@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Support 64-bit pointers to kfuncs | expand |
On Wed, Feb 15, 2023 at 3:59 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote: > > Make the code more readable by introducing a symbolic constant > instead of using 0. > > Suggested-by: Stanislav Fomichev <sdf@google.com> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > --- > include/uapi/linux/bpf.h | 4 ++++ > kernel/bpf/disasm.c | 2 +- > kernel/bpf/verifier.c | 12 +++++++----- > tools/include/linux/filter.h | 2 +- > tools/include/uapi/linux/bpf.h | 4 ++++ > 5 files changed, 17 insertions(+), 7 deletions(-) > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > index 1503f61336b6..37f7588d5b2f 100644 > --- a/include/uapi/linux/bpf.h > +++ b/include/uapi/linux/bpf.h > @@ -1211,6 +1211,10 @@ enum bpf_link_type { > */ > #define BPF_PSEUDO_FUNC 4 > > +/* when bpf_call->src_reg == BPF_HELPER_CALL, bpf_call->imm == index of a bpf > + * helper function (see ___BPF_FUNC_MAPPER below for a full list) > + */ > +#define BPF_HELPER_CALL 0 I don't like this "cleanup". The code reads fine as-is.
On 02/16, Alexei Starovoitov wrote: > On Wed, Feb 15, 2023 at 3:59 PM Ilya Leoshkevich <iii@linux.ibm.com> > wrote: > > > > Make the code more readable by introducing a symbolic constant > > instead of using 0. > > > > Suggested-by: Stanislav Fomichev <sdf@google.com> > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > --- > > include/uapi/linux/bpf.h | 4 ++++ > > kernel/bpf/disasm.c | 2 +- > > kernel/bpf/verifier.c | 12 +++++++----- > > tools/include/linux/filter.h | 2 +- > > tools/include/uapi/linux/bpf.h | 4 ++++ > > 5 files changed, 17 insertions(+), 7 deletions(-) > > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > index 1503f61336b6..37f7588d5b2f 100644 > > --- a/include/uapi/linux/bpf.h > > +++ b/include/uapi/linux/bpf.h > > @@ -1211,6 +1211,10 @@ enum bpf_link_type { > > */ > > #define BPF_PSEUDO_FUNC 4 > > > > +/* when bpf_call->src_reg == BPF_HELPER_CALL, bpf_call->imm == index > of a bpf > > + * helper function (see ___BPF_FUNC_MAPPER below for a full list) > > + */ > > +#define BPF_HELPER_CALL 0 > I don't like this "cleanup". > The code reads fine as-is. Even in the context of patch 4? There would be the following switch without BPF_HELPER_CALL: switch (insn->src_reg) { case 0: ... break; case BPF_PSEUDO_CALL: ... break; case BPF_PSEUDO_KFUNC_CALL: ... break; } That 'case 0' feels like it deserves a name. But up to you, I'm fine either way.
On Thu, Feb 16, 2023 at 9:25 AM Stanislav Fomichev <sdf@google.com> wrote: > > On 02/16, Alexei Starovoitov wrote: > > On Wed, Feb 15, 2023 at 3:59 PM Ilya Leoshkevich <iii@linux.ibm.com> > > wrote: > > > > > > Make the code more readable by introducing a symbolic constant > > > instead of using 0. > > > > > > Suggested-by: Stanislav Fomichev <sdf@google.com> > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > > --- > > > include/uapi/linux/bpf.h | 4 ++++ > > > kernel/bpf/disasm.c | 2 +- > > > kernel/bpf/verifier.c | 12 +++++++----- > > > tools/include/linux/filter.h | 2 +- > > > tools/include/uapi/linux/bpf.h | 4 ++++ > > > 5 files changed, 17 insertions(+), 7 deletions(-) > > > > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > > index 1503f61336b6..37f7588d5b2f 100644 > > > --- a/include/uapi/linux/bpf.h > > > +++ b/include/uapi/linux/bpf.h > > > @@ -1211,6 +1211,10 @@ enum bpf_link_type { > > > */ > > > #define BPF_PSEUDO_FUNC 4 > > > > > > +/* when bpf_call->src_reg == BPF_HELPER_CALL, bpf_call->imm == index > > of a bpf > > > + * helper function (see ___BPF_FUNC_MAPPER below for a full list) > > > + */ > > > +#define BPF_HELPER_CALL 0 > > > I don't like this "cleanup". > > The code reads fine as-is. > > Even in the context of patch 4? There would be the following switch > without BPF_HELPER_CALL: > > switch (insn->src_reg) { > case 0: > ... > break; > > case BPF_PSEUDO_CALL: > ... > break; > > case BPF_PSEUDO_KFUNC_CALL: > ... > break; > } > > That 'case 0' feels like it deserves a name. But up to you, I'm fine > either way. It's philosophical. Some people insist on if (ptr == NULL). I insist on if (!ptr). That's why canonical bpf progs are written as: val = bpf_map_lookup(); if (!val) ... zero is zero. It doesn't need #define.
On 02/16, Alexei Starovoitov wrote: > On Thu, Feb 16, 2023 at 9:25 AM Stanislav Fomichev <sdf@google.com> wrote: > > > > On 02/16, Alexei Starovoitov wrote: > > > On Wed, Feb 15, 2023 at 3:59 PM Ilya Leoshkevich <iii@linux.ibm.com> > > > wrote: > > > > > > > > Make the code more readable by introducing a symbolic constant > > > > instead of using 0. > > > > > > > > Suggested-by: Stanislav Fomichev <sdf@google.com> > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > > > --- > > > > include/uapi/linux/bpf.h | 4 ++++ > > > > kernel/bpf/disasm.c | 2 +- > > > > kernel/bpf/verifier.c | 12 +++++++----- > > > > tools/include/linux/filter.h | 2 +- > > > > tools/include/uapi/linux/bpf.h | 4 ++++ > > > > 5 files changed, 17 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > > > index 1503f61336b6..37f7588d5b2f 100644 > > > > --- a/include/uapi/linux/bpf.h > > > > +++ b/include/uapi/linux/bpf.h > > > > @@ -1211,6 +1211,10 @@ enum bpf_link_type { > > > > */ > > > > #define BPF_PSEUDO_FUNC 4 > > > > > > > > +/* when bpf_call->src_reg == BPF_HELPER_CALL, bpf_call->imm == > index > > > of a bpf > > > > + * helper function (see ___BPF_FUNC_MAPPER below for a full list) > > > > + */ > > > > +#define BPF_HELPER_CALL 0 > > > > > I don't like this "cleanup". > > > The code reads fine as-is. > > > > Even in the context of patch 4? There would be the following switch > > without BPF_HELPER_CALL: > > > > switch (insn->src_reg) { > > case 0: > > ... > > break; > > > > case BPF_PSEUDO_CALL: > > ... > > break; > > > > case BPF_PSEUDO_KFUNC_CALL: > > ... > > break; > > } > > > > That 'case 0' feels like it deserves a name. But up to you, I'm fine > > either way. > It's philosophical. > Some people insist on if (ptr == NULL). I insist on if (!ptr). > That's why canonical bpf progs are written as: > val = bpf_map_lookup(); > if (!val) ... > zero is zero. It doesn't need #define. Are you sure we still want to apply the same logic here for src_reg? I agree that doing src_reg vs !src_reg made sense when we had a "helper" vs "non-helper" (bpf2bpf) situation. However now this src_reg feels more like an enum. And since we have an enum value for 1 and 2, it feels natural to have another one for 0? That second patch from the series ([0]) might be a good example on why we actually need it. I'm assuming at some point we've had: #define BPF_PSEUDO_CALL 1 So we ended up writing `src_reg != BPF_PSEUDO_CALL` instead of actually doing `src_reg == BPF_HELPER_CALL` (aka `src_reg == 0`). Afterwards, we've added BPF_PSEUDO_KFUNC_CALL=2 which broke our previous src_reg vs !src_reg assumptions... [0]: https://lore.kernel.org/bpf/20230215235931.380197-1-iii@linux.ibm.com/T/#mf87a26ef48a909b62ce950639acfdf5b296b487b
On Thu, 2023-02-16 at 10:03 -0800, Stanislav Fomichev wrote: > On 02/16, Alexei Starovoitov wrote: > > On Thu, Feb 16, 2023 at 9:25 AM Stanislav Fomichev <sdf@google.com> > > wrote: > > > > > > On 02/16, Alexei Starovoitov wrote: > > > > On Wed, Feb 15, 2023 at 3:59 PM Ilya Leoshkevich > > > > <iii@linux.ibm.com> > > > > wrote: > > > > > > > > > > Make the code more readable by introducing a symbolic > > > > > constant > > > > > instead of using 0. > > > > > > > > > > Suggested-by: Stanislav Fomichev <sdf@google.com> > > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > > > > --- > > > > > include/uapi/linux/bpf.h | 4 ++++ > > > > > kernel/bpf/disasm.c | 2 +- > > > > > kernel/bpf/verifier.c | 12 +++++++----- > > > > > tools/include/linux/filter.h | 2 +- > > > > > tools/include/uapi/linux/bpf.h | 4 ++++ > > > > > 5 files changed, 17 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/include/uapi/linux/bpf.h > > > > > b/include/uapi/linux/bpf.h > > > > > index 1503f61336b6..37f7588d5b2f 100644 > > > > > --- a/include/uapi/linux/bpf.h > > > > > +++ b/include/uapi/linux/bpf.h > > > > > @@ -1211,6 +1211,10 @@ enum bpf_link_type { > > > > > */ > > > > > #define BPF_PSEUDO_FUNC 4 > > > > > > > > > > +/* when bpf_call->src_reg == BPF_HELPER_CALL, bpf_call->imm > > > > > == > > index > > > > of a bpf > > > > > + * helper function (see ___BPF_FUNC_MAPPER below for a full > > > > > list) > > > > > + */ > > > > > +#define BPF_HELPER_CALL 0 > > > > > > > I don't like this "cleanup". > > > > The code reads fine as-is. > > > > > > Even in the context of patch 4? There would be the following > > > switch > > > without BPF_HELPER_CALL: > > > > > > switch (insn->src_reg) { > > > case 0: > > > ... > > > break; > > > > > > case BPF_PSEUDO_CALL: > > > ... > > > break; > > > > > > case BPF_PSEUDO_KFUNC_CALL: > > > ... > > > break; > > > } > > > > > > That 'case 0' feels like it deserves a name. But up to you, I'm > > > fine > > > either way. > > > It's philosophical. > > Some people insist on if (ptr == NULL). I insist on if (!ptr). > > That's why canonical bpf progs are written as: > > val = bpf_map_lookup(); > > if (!val) ... > > zero is zero. It doesn't need #define. > > Are you sure we still want to apply the same logic here for src_reg? > I > agree that doing src_reg vs !src_reg made sense when we had a > "helper" > vs "non-helper" (bpf2bpf) situation. However now this src_reg feels > more > like an enum. And since we have an enum value for 1 and 2, it feels > natural to have another one for 0? > > That second patch from the series ([0]) might be a good example on > why > we actually need it. I'm assuming at some point we've had: > #define BPF_PSEUDO_CALL 1 > > So we ended up writing `src_reg != BPF_PSEUDO_CALL` instead of > actually > doing `src_reg == BPF_HELPER_CALL` (aka `src_reg == 0`). > Afterwards, we've added BPF_PSEUDO_KFUNC_CALL=2 which broke our > previous > src_reg vs !src_reg assumptions... > > [0]: > https://lore.kernel.org/bpf/20230215235931.380197-1-iii@linux.ibm.com/T/#mf87a26ef48a909b62ce950639acfdf5b296b487b FWIW the helper checks before this series had inconsistent style: - !insn->src_reg - insn->src_reg == 0 - insn->src_reg != BPF_REG_0 - insn[i].src_reg != BPF_PSEUDO_CALL Now at least it's the same style everywhere, and also it's easy to grep for "where do we check for helper calls".
On Fri, Feb 17, 2023 at 2:57 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote: > > On Thu, 2023-02-16 at 10:03 -0800, Stanislav Fomichev wrote: > > On 02/16, Alexei Starovoitov wrote: > > > On Thu, Feb 16, 2023 at 9:25 AM Stanislav Fomichev <sdf@google.com> > > > wrote: > > > > > > > > On 02/16, Alexei Starovoitov wrote: > > > > > On Wed, Feb 15, 2023 at 3:59 PM Ilya Leoshkevich > > > > > <iii@linux.ibm.com> > > > > > wrote: > > > > > > > > > > > > Make the code more readable by introducing a symbolic > > > > > > constant > > > > > > instead of using 0. > > > > > > > > > > > > Suggested-by: Stanislav Fomichev <sdf@google.com> > > > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > > > > > --- > > > > > > include/uapi/linux/bpf.h | 4 ++++ > > > > > > kernel/bpf/disasm.c | 2 +- > > > > > > kernel/bpf/verifier.c | 12 +++++++----- > > > > > > tools/include/linux/filter.h | 2 +- > > > > > > tools/include/uapi/linux/bpf.h | 4 ++++ > > > > > > 5 files changed, 17 insertions(+), 7 deletions(-) > > > > > > > > > > > > diff --git a/include/uapi/linux/bpf.h > > > > > > b/include/uapi/linux/bpf.h > > > > > > index 1503f61336b6..37f7588d5b2f 100644 > > > > > > --- a/include/uapi/linux/bpf.h > > > > > > +++ b/include/uapi/linux/bpf.h > > > > > > @@ -1211,6 +1211,10 @@ enum bpf_link_type { > > > > > > */ > > > > > > #define BPF_PSEUDO_FUNC 4 > > > > > > > > > > > > +/* when bpf_call->src_reg == BPF_HELPER_CALL, bpf_call->imm > > > > > > == > > > index > > > > > of a bpf > > > > > > + * helper function (see ___BPF_FUNC_MAPPER below for a full > > > > > > list) > > > > > > + */ > > > > > > +#define BPF_HELPER_CALL 0 > > > > > > > > > I don't like this "cleanup". > > > > > The code reads fine as-is. > > > > > > > > Even in the context of patch 4? There would be the following > > > > switch > > > > without BPF_HELPER_CALL: > > > > > > > > switch (insn->src_reg) { > > > > case 0: > > > > ... > > > > break; > > > > > > > > case BPF_PSEUDO_CALL: > > > > ... > > > > break; > > > > > > > > case BPF_PSEUDO_KFUNC_CALL: > > > > ... > > > > break; > > > > } > > > > > > > > That 'case 0' feels like it deserves a name. But up to you, I'm > > > > fine > > > > either way. > > > > > It's philosophical. > > > Some people insist on if (ptr == NULL). I insist on if (!ptr). > > > That's why canonical bpf progs are written as: > > > val = bpf_map_lookup(); > > > if (!val) ... > > > zero is zero. It doesn't need #define. > > > > Are you sure we still want to apply the same logic here for src_reg? > > I > > agree that doing src_reg vs !src_reg made sense when we had a > > "helper" > > vs "non-helper" (bpf2bpf) situation. However now this src_reg feels > > more > > like an enum. And since we have an enum value for 1 and 2, it feels > > natural to have another one for 0? > > > > That second patch from the series ([0]) might be a good example on > > why > > we actually need it. I'm assuming at some point we've had: > > #define BPF_PSEUDO_CALL 1 > > > > So we ended up writing `src_reg != BPF_PSEUDO_CALL` instead of > > actually > > doing `src_reg == BPF_HELPER_CALL` (aka `src_reg == 0`). > > Afterwards, we've added BPF_PSEUDO_KFUNC_CALL=2 which broke our > > previous > > src_reg vs !src_reg assumptions... > > > > [0]: > > https://lore.kernel.org/bpf/20230215235931.380197-1-iii@linux.ibm.com/T/#mf87a26ef48a909b62ce950639acfdf5b296b487b > > FWIW the helper checks before this series had inconsistent style: > > - !insn->src_reg > - insn->src_reg == 0 > - insn->src_reg != BPF_REG_0 > - insn[i].src_reg != BPF_PSEUDO_CALL > > Now at least it's the same style everywhere, and also it's easy to > grep for "where do we check for helper calls". The above checks are not equivalent. Comparing src_reg with BPF_REG_0 makes sense in one context and doesn't in the other. It's never ok to add stuff to uapi when it works as-is. I also don't buy theoretical arguments about future additions and how something will be cleaner in the future because we predicted it so well today.
On Fri, Feb 17, 2023 at 8:19 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Fri, Feb 17, 2023 at 2:57 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote: > > > > On Thu, 2023-02-16 at 10:03 -0800, Stanislav Fomichev wrote: > > > On 02/16, Alexei Starovoitov wrote: > > > > On Thu, Feb 16, 2023 at 9:25 AM Stanislav Fomichev <sdf@google.com> > > > > wrote: > > > > > > > > > > On 02/16, Alexei Starovoitov wrote: > > > > > > On Wed, Feb 15, 2023 at 3:59 PM Ilya Leoshkevich > > > > > > <iii@linux.ibm.com> > > > > > > wrote: > > > > > > > > > > > > > > Make the code more readable by introducing a symbolic > > > > > > > constant > > > > > > > instead of using 0. > > > > > > > > > > > > > > Suggested-by: Stanislav Fomichev <sdf@google.com> > > > > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > > > > > > --- > > > > > > > include/uapi/linux/bpf.h | 4 ++++ > > > > > > > kernel/bpf/disasm.c | 2 +- > > > > > > > kernel/bpf/verifier.c | 12 +++++++----- > > > > > > > tools/include/linux/filter.h | 2 +- > > > > > > > tools/include/uapi/linux/bpf.h | 4 ++++ > > > > > > > 5 files changed, 17 insertions(+), 7 deletions(-) > > > > > > > > > > > > > > diff --git a/include/uapi/linux/bpf.h > > > > > > > b/include/uapi/linux/bpf.h > > > > > > > index 1503f61336b6..37f7588d5b2f 100644 > > > > > > > --- a/include/uapi/linux/bpf.h > > > > > > > +++ b/include/uapi/linux/bpf.h > > > > > > > @@ -1211,6 +1211,10 @@ enum bpf_link_type { > > > > > > > */ > > > > > > > #define BPF_PSEUDO_FUNC 4 > > > > > > > > > > > > > > +/* when bpf_call->src_reg == BPF_HELPER_CALL, bpf_call->imm > > > > > > > == > > > > index > > > > > > of a bpf > > > > > > > + * helper function (see ___BPF_FUNC_MAPPER below for a full > > > > > > > list) > > > > > > > + */ > > > > > > > +#define BPF_HELPER_CALL 0 > > > > > > > > > > > I don't like this "cleanup". > > > > > > The code reads fine as-is. > > > > > > > > > > Even in the context of patch 4? There would be the following > > > > > switch > > > > > without BPF_HELPER_CALL: > > > > > > > > > > switch (insn->src_reg) { > > > > > case 0: > > > > > ... > > > > > break; > > > > > > > > > > case BPF_PSEUDO_CALL: > > > > > ... > > > > > break; > > > > > > > > > > case BPF_PSEUDO_KFUNC_CALL: > > > > > ... > > > > > break; > > > > > } > > > > > > > > > > That 'case 0' feels like it deserves a name. But up to you, I'm > > > > > fine > > > > > either way. > > > > > > > It's philosophical. > > > > Some people insist on if (ptr == NULL). I insist on if (!ptr). > > > > That's why canonical bpf progs are written as: > > > > val = bpf_map_lookup(); > > > > if (!val) ... > > > > zero is zero. It doesn't need #define. > > > > > > Are you sure we still want to apply the same logic here for src_reg? > > > I > > > agree that doing src_reg vs !src_reg made sense when we had a > > > "helper" > > > vs "non-helper" (bpf2bpf) situation. However now this src_reg feels > > > more > > > like an enum. And since we have an enum value for 1 and 2, it feels > > > natural to have another one for 0? > > > > > > That second patch from the series ([0]) might be a good example on > > > why > > > we actually need it. I'm assuming at some point we've had: > > > #define BPF_PSEUDO_CALL 1 > > > > > > So we ended up writing `src_reg != BPF_PSEUDO_CALL` instead of > > > actually > > > doing `src_reg == BPF_HELPER_CALL` (aka `src_reg == 0`). > > > Afterwards, we've added BPF_PSEUDO_KFUNC_CALL=2 which broke our > > > previous > > > src_reg vs !src_reg assumptions... > > > > > > [0]: > > > https://lore.kernel.org/bpf/20230215235931.380197-1-iii@linux.ibm.com/T/#mf87a26ef48a909b62ce950639acfdf5b296b487b > > > > FWIW the helper checks before this series had inconsistent style: > > > > - !insn->src_reg > > - insn->src_reg == 0 > > - insn->src_reg != BPF_REG_0 > > - insn[i].src_reg != BPF_PSEUDO_CALL > > > > Now at least it's the same style everywhere, and also it's easy to > > grep for "where do we check for helper calls". > > The above checks are not equivalent. > Comparing src_reg with BPF_REG_0 makes sense in one context > and doesn't in the other. > It's never ok to add stuff to uapi when it works as-is. > I also don't buy theoretical arguments about future additions > and how something will be cleaner in the future because > we predicted it so well today. SG! Then let's maybe respin without this part? I might have derailed the conversation too much from the actual issue :-[
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index 1503f61336b6..37f7588d5b2f 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -1211,6 +1211,10 @@ enum bpf_link_type { */ #define BPF_PSEUDO_FUNC 4 +/* when bpf_call->src_reg == BPF_HELPER_CALL, bpf_call->imm == index of a bpf + * helper function (see ___BPF_FUNC_MAPPER below for a full list) + */ +#define BPF_HELPER_CALL 0 /* when bpf_call->src_reg == BPF_PSEUDO_CALL, bpf_call->imm == pc-relative * offset to another bpf function */ diff --git a/kernel/bpf/disasm.c b/kernel/bpf/disasm.c index 7b4afb7d96db..c11d9b5a45a9 100644 --- a/kernel/bpf/disasm.c +++ b/kernel/bpf/disasm.c @@ -19,7 +19,7 @@ static const char *__func_get_name(const struct bpf_insn_cbs *cbs, { BUILD_BUG_ON(ARRAY_SIZE(func_id_str) != __BPF_FUNC_MAX_ID); - if (!insn->src_reg && + if (insn->src_reg == BPF_HELPER_CALL && insn->imm >= 0 && insn->imm < __BPF_FUNC_MAX_ID && func_id_str[insn->imm]) return func_id_str[insn->imm]; diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 272563a0b770..427525fc3791 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -2947,7 +2947,8 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, /* BPF helpers that invoke callback subprogs are * equivalent to BPF_PSEUDO_CALL above */ - if (insn->src_reg == 0 && is_callback_calling_function(insn->imm)) + if (insn->src_reg == BPF_HELPER_CALL && + is_callback_calling_function(insn->imm)) return -ENOTSUPP; /* kfunc with imm==0 is invalid and fixup_kfunc_call will * catch this error later. Make backtracking conservative @@ -7522,7 +7523,7 @@ static int __check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn } if (insn->code == (BPF_JMP | BPF_CALL) && - insn->src_reg == 0 && + insn->src_reg == BPF_HELPER_CALL && insn->imm == BPF_FUNC_timer_set_callback) { struct bpf_verifier_state *async_cb; @@ -14730,7 +14731,7 @@ static int do_check(struct bpf_verifier_env *env) if (BPF_SRC(insn->code) != BPF_K || (insn->src_reg != BPF_PSEUDO_KFUNC_CALL && insn->off != 0) || - (insn->src_reg != BPF_REG_0 && + (insn->src_reg != BPF_HELPER_CALL && insn->src_reg != BPF_PSEUDO_CALL && insn->src_reg != BPF_PSEUDO_KFUNC_CALL) || insn->dst_reg != BPF_REG_0 || @@ -14740,7 +14741,8 @@ static int do_check(struct bpf_verifier_env *env) } if (env->cur_state->active_lock.ptr) { - if ((insn->src_reg == BPF_REG_0 && insn->imm != BPF_FUNC_spin_unlock) || + if ((insn->src_reg == BPF_HELPER_CALL && + insn->imm != BPF_FUNC_spin_unlock) || (insn->src_reg == BPF_PSEUDO_CALL) || (insn->src_reg == BPF_PSEUDO_KFUNC_CALL && (insn->off != 0 || !is_bpf_graph_api_kfunc(insn->imm)))) { @@ -16933,7 +16935,7 @@ static struct bpf_prog *inline_bpf_loop(struct bpf_verifier_env *env, static bool is_bpf_loop_call(struct bpf_insn *insn) { return insn->code == (BPF_JMP | BPF_CALL) && - insn->src_reg == 0 && + insn->src_reg == BPF_HELPER_CALL && insn->imm == BPF_FUNC_loop; } diff --git a/tools/include/linux/filter.h b/tools/include/linux/filter.h index 736bdeccdfe4..78dc208c8d88 100644 --- a/tools/include/linux/filter.h +++ b/tools/include/linux/filter.h @@ -261,7 +261,7 @@ ((struct bpf_insn) { \ .code = BPF_JMP | BPF_CALL, \ .dst_reg = 0, \ - .src_reg = 0, \ + .src_reg = BPF_HELPER_CALL, \ .off = 0, \ .imm = ((FUNC) - BPF_FUNC_unspec) }) diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index 1503f61336b6..37f7588d5b2f 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -1211,6 +1211,10 @@ enum bpf_link_type { */ #define BPF_PSEUDO_FUNC 4 +/* when bpf_call->src_reg == BPF_HELPER_CALL, bpf_call->imm == index of a bpf + * helper function (see ___BPF_FUNC_MAPPER below for a full list) + */ +#define BPF_HELPER_CALL 0 /* when bpf_call->src_reg == BPF_PSEUDO_CALL, bpf_call->imm == pc-relative * offset to another bpf function */
Make the code more readable by introducing a symbolic constant instead of using 0. Suggested-by: Stanislav Fomichev <sdf@google.com> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> --- include/uapi/linux/bpf.h | 4 ++++ kernel/bpf/disasm.c | 2 +- kernel/bpf/verifier.c | 12 +++++++----- tools/include/linux/filter.h | 2 +- tools/include/uapi/linux/bpf.h | 4 ++++ 5 files changed, 17 insertions(+), 7 deletions(-)