| Message ID | 20211109021624.1140446-1-haoluo@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | bpf: Clean up _OR_NULL arg types | expand |
On Mon, Nov 08, 2021 at 06:16:15PM -0800, Hao Luo wrote: > This is a pure cleanup patchset that tries to use flag to mark whether > an arg may be null. It replaces enum bpf_arg_type with a struct. Doing > so allows us to embed the properties of arguments in the struct, which > is a more scalable solution than introducing a new enum. This patchset > performs this transformation only on arg_type. If it looks good, > follow-up patches will do the same on reg_type and ret_type. > > The first patch replaces 'enum bpf_arg_type' with 'struct bpf_arg_type' > and each of the rest patches transforms one type of ARG_XXX_OR_NULLs. Nice. Thank you for working on it! The enum->struct conversion works for bpf_arg_type, but applying the same technique to bpf_reg_type could be problematic. Since it's part of bpf_reg_state which in turn is multiplied by a large factor. Growing enum from 4 bytes to 8 byte struct will consume quite a lot of extra memory. > 19 files changed, 932 insertions(+), 780 deletions(-) Just bpf_arg_type refactoring adds a lot of churn which could make backports of future fixes not automatic anymore. Similar converstion for bpf_reg_type and bpf_return_type will be even more churn. Have you considered using upper bits to represent flags? Instead of diff: - .arg1_type = ARG_CONST_MAP_PTR, - .arg2_type = ARG_PTR_TO_FUNC, - .arg3_type = ARG_PTR_TO_STACK_OR_NULL, - .arg4_type = ARG_ANYTHING, + .arg1 = { .type = ARG_CONST_MAP_PTR }, + .arg2 = { .type = ARG_PTR_TO_FUNC }, + .arg3 = { .type = ARG_PTR_TO_STACK_OR_NULL }, + .arg4 = { .type = ARG_ANYTHING }, can we make it look like: .arg1_type = ARG_CONST_MAP_PTR, .arg2_type = ARG_PTR_TO_FUNC, - .arg3_type = ARG_PTR_TO_STACK_OR_NULL, + .arg3_type = ARG_PTR_TO_STACK | MAYBE_NULL, .arg4_type = ARG_ANYTHING, Ideally all three (bpf_reg_type, bpf_return_type, and bpf_arg_type) would share the same flag bit: MAYBE_NULL. Then static bool arg_type_may_be_null() will be comparing only single bit ? While if (arg_type == ARG_PTR_TO_MAP_VALUE || arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE || arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL) { will become: arg_type &= FLAG_MASK; if (arg_type == ARG_PTR_TO_MAP_VALUE || arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE) { Most of the time I would prefer explicit .type and .flag structure, but saving memory is important for bpf_reg_type, so explicit bit operations are probably justified.
On Tue, Nov 9, 2021 at 10:21 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Mon, Nov 08, 2021 at 06:16:15PM -0800, Hao Luo wrote: > > This is a pure cleanup patchset that tries to use flag to mark whether > > an arg may be null. It replaces enum bpf_arg_type with a struct. Doing > > so allows us to embed the properties of arguments in the struct, which > > is a more scalable solution than introducing a new enum. This patchset > > performs this transformation only on arg_type. If it looks good, > > follow-up patches will do the same on reg_type and ret_type. > > > > The first patch replaces 'enum bpf_arg_type' with 'struct bpf_arg_type' > > and each of the rest patches transforms one type of ARG_XXX_OR_NULLs. > > Nice. Thank you for working on it! No problem. :) > > The enum->struct conversion works for bpf_arg_type, but applying > the same technique to bpf_reg_type could be problematic. > Since it's part of bpf_reg_state which in turn is multiplied by a large factor. > Growing enum from 4 bytes to 8 byte struct will consume quite > a lot of extra memory. > > > 19 files changed, 932 insertions(+), 780 deletions(-) > > Just bpf_arg_type refactoring adds a lot of churn which could make > backports of future fixes not automatic anymore. > Similar converstion for bpf_reg_type and bpf_return_type will > be even more churn. Acknowledged. > Have you considered using upper bits to represent flags? Yes, I thought about that. Some of my thoughts are: - I wasn't sure how many bits should be reserved. Maybe 16 bits is good enough? - What if we run out of flag bits in future? - We could fold btf_id in the structure in this patchset. And new fields could be easily added if needed. So with these questions, I didn't pursue that approach in the first place. But I admit that it does look better by writing + .arg3_type = ARG_PTR_TO_STACK | MAYBE_NULL, Instead of + .arg3 = { + .type = ARG_PTR_TO_MAP_VALUE, + .flag = ARG_FLAG_MAYBE_NULL, + }, Let's see if there is any further comment. I can go take a look and prepare for that approach in the next revision. > > Instead of diff: > - .arg1_type = ARG_CONST_MAP_PTR, > - .arg2_type = ARG_PTR_TO_FUNC, > - .arg3_type = ARG_PTR_TO_STACK_OR_NULL, > - .arg4_type = ARG_ANYTHING, > + .arg1 = { .type = ARG_CONST_MAP_PTR }, > + .arg2 = { .type = ARG_PTR_TO_FUNC }, > + .arg3 = { .type = ARG_PTR_TO_STACK_OR_NULL }, > + .arg4 = { .type = ARG_ANYTHING }, > > can we make it look like: > .arg1_type = ARG_CONST_MAP_PTR, > .arg2_type = ARG_PTR_TO_FUNC, > - .arg3_type = ARG_PTR_TO_STACK_OR_NULL, > + .arg3_type = ARG_PTR_TO_STACK | MAYBE_NULL, > .arg4_type = ARG_ANYTHING, > > Ideally all three (bpf_reg_type, bpf_return_type, and bpf_arg_type) > would share the same flag bit: MAYBE_NULL. > Then static bool arg_type_may_be_null() will be comparing only single bit ? > > While > if (arg_type == ARG_PTR_TO_MAP_VALUE || > arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE || > arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL) { > will become: > arg_type &= FLAG_MASK; > if (arg_type == ARG_PTR_TO_MAP_VALUE || > arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE) { > > Most of the time I would prefer explicit .type and .flag structure, > but saving memory is important for bpf_reg_type, so explicit bit > operations are probably justified.
On Tue, Nov 9, 2021 at 11:42 AM Hao Luo <haoluo@google.com> wrote: > > On Tue, Nov 9, 2021 at 10:21 AM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Mon, Nov 08, 2021 at 06:16:15PM -0800, Hao Luo wrote: > > > This is a pure cleanup patchset that tries to use flag to mark whether > > > an arg may be null. It replaces enum bpf_arg_type with a struct. Doing > > > so allows us to embed the properties of arguments in the struct, which > > > is a more scalable solution than introducing a new enum. This patchset > > > performs this transformation only on arg_type. If it looks good, > > > follow-up patches will do the same on reg_type and ret_type. > > > > > > The first patch replaces 'enum bpf_arg_type' with 'struct bpf_arg_type' > > > and each of the rest patches transforms one type of ARG_XXX_OR_NULLs. > > > > Nice. Thank you for working on it! > > No problem. :) > > > > > The enum->struct conversion works for bpf_arg_type, but applying > > the same technique to bpf_reg_type could be problematic. > > Since it's part of bpf_reg_state which in turn is multiplied by a large factor. > > Growing enum from 4 bytes to 8 byte struct will consume quite > > a lot of extra memory. > > > > > 19 files changed, 932 insertions(+), 780 deletions(-) > > > > Just bpf_arg_type refactoring adds a lot of churn which could make > > backports of future fixes not automatic anymore. > > Similar converstion for bpf_reg_type and bpf_return_type will > > be even more churn. > > Acknowledged. > > > Have you considered using upper bits to represent flags? > > Yes, I thought about that. Some of my thoughts are: > > - I wasn't sure how many bits should be reserved. Maybe 16 bits is good enough? > - What if we run out of flag bits in future? > - We could fold btf_id in the structure in this patchset. And new > fields could be easily added if needed. > > So with these questions, I didn't pursue that approach in the first > place. But I admit that it does look better by writing > > + .arg3_type = ARG_PTR_TO_STACK | MAYBE_NULL, > > Instead of > > + .arg3 = { > + .type = ARG_PTR_TO_MAP_VALUE, > + .flag = ARG_FLAG_MAYBE_NULL, > + }, > > Let's see if there is any further comment. I can go take a look and > prepare for that approach in the next revision. > +1 on staying within a single enum and using upper bits > > > > > > Instead of diff: > > - .arg1_type = ARG_CONST_MAP_PTR, > > - .arg2_type = ARG_PTR_TO_FUNC, > > - .arg3_type = ARG_PTR_TO_STACK_OR_NULL, > > - .arg4_type = ARG_ANYTHING, > > + .arg1 = { .type = ARG_CONST_MAP_PTR }, > > + .arg2 = { .type = ARG_PTR_TO_FUNC }, > > + .arg3 = { .type = ARG_PTR_TO_STACK_OR_NULL }, > > + .arg4 = { .type = ARG_ANYTHING }, > > > > can we make it look like: > > .arg1_type = ARG_CONST_MAP_PTR, > > .arg2_type = ARG_PTR_TO_FUNC, > > - .arg3_type = ARG_PTR_TO_STACK_OR_NULL, > > + .arg3_type = ARG_PTR_TO_STACK | MAYBE_NULL, > > .arg4_type = ARG_ANYTHING, > > > > Ideally all three (bpf_reg_type, bpf_return_type, and bpf_arg_type) > > would share the same flag bit: MAYBE_NULL. I support using the same bit value, but should we use the exact same enum name for three different enums? Like MAYBE_NULL, which enum is it defined in? Wouldn't RET_MAYBE_NULL and RET_MAYBE_NULL, in addition to REG_MAYBE_NULL be more explicit about what they apply to? BTW (see my comment on another patch), _OR_NULL and _OR_ZERO are not the same thing, are they? Is the plan to use two different bits for them or pretend that CONST_OR_ZERO "may be null"? > > Then static bool arg_type_may_be_null() will be comparing only single bit ? > > > > While > > if (arg_type == ARG_PTR_TO_MAP_VALUE || > > arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE || > > arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL) { > > will become: > > arg_type &= FLAG_MASK; > > if (arg_type == ARG_PTR_TO_MAP_VALUE || > > arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE) { > > > > Most of the time I would prefer explicit .type and .flag structure, > > but saving memory is important for bpf_reg_type, so explicit bit > > operations are probably justified.
On Tue, Nov 9, 2021 at 8:34 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, Nov 9, 2021 at 11:42 AM Hao Luo <haoluo@google.com> wrote: > > > > On Tue, Nov 9, 2021 at 10:21 AM Alexei Starovoitov > > <alexei.starovoitov@gmail.com> wrote: > > > > > > On Mon, Nov 08, 2021 at 06:16:15PM -0800, Hao Luo wrote: > > > > This is a pure cleanup patchset that tries to use flag to mark whether > > > > an arg may be null. It replaces enum bpf_arg_type with a struct. Doing > > > > so allows us to embed the properties of arguments in the struct, which > > > > is a more scalable solution than introducing a new enum. This patchset > > > > performs this transformation only on arg_type. If it looks good, > > > > follow-up patches will do the same on reg_type and ret_type. > > > > > > > > The first patch replaces 'enum bpf_arg_type' with 'struct bpf_arg_type' > > > > and each of the rest patches transforms one type of ARG_XXX_OR_NULLs. > > > > > > Nice. Thank you for working on it! > > > > No problem. :) > > > > > > > > The enum->struct conversion works for bpf_arg_type, but applying > > > the same technique to bpf_reg_type could be problematic. > > > Since it's part of bpf_reg_state which in turn is multiplied by a large factor. > > > Growing enum from 4 bytes to 8 byte struct will consume quite > > > a lot of extra memory. > > > > > > > 19 files changed, 932 insertions(+), 780 deletions(-) > > > > > > Just bpf_arg_type refactoring adds a lot of churn which could make > > > backports of future fixes not automatic anymore. > > > Similar converstion for bpf_reg_type and bpf_return_type will > > > be even more churn. > > > > Acknowledged. > > > > > Have you considered using upper bits to represent flags? > > > > Yes, I thought about that. Some of my thoughts are: > > > > - I wasn't sure how many bits should be reserved. Maybe 16 bits is good enough? > > - What if we run out of flag bits in future? > > - We could fold btf_id in the structure in this patchset. And new > > fields could be easily added if needed. > > > > So with these questions, I didn't pursue that approach in the first > > place. But I admit that it does look better by writing > > > > + .arg3_type = ARG_PTR_TO_STACK | MAYBE_NULL, > > > > Instead of > > > > + .arg3 = { > > + .type = ARG_PTR_TO_MAP_VALUE, > > + .flag = ARG_FLAG_MAYBE_NULL, > > + }, > > > > Let's see if there is any further comment. I can go take a look and > > prepare for that approach in the next revision. > > > > +1 on staying within a single enum and using upper bits > > > > > > > > > > > Instead of diff: > > > - .arg1_type = ARG_CONST_MAP_PTR, > > > - .arg2_type = ARG_PTR_TO_FUNC, > > > - .arg3_type = ARG_PTR_TO_STACK_OR_NULL, > > > - .arg4_type = ARG_ANYTHING, > > > + .arg1 = { .type = ARG_CONST_MAP_PTR }, > > > + .arg2 = { .type = ARG_PTR_TO_FUNC }, > > > + .arg3 = { .type = ARG_PTR_TO_STACK_OR_NULL }, > > > + .arg4 = { .type = ARG_ANYTHING }, > > > > > > can we make it look like: > > > .arg1_type = ARG_CONST_MAP_PTR, > > > .arg2_type = ARG_PTR_TO_FUNC, > > > - .arg3_type = ARG_PTR_TO_STACK_OR_NULL, > > > + .arg3_type = ARG_PTR_TO_STACK | MAYBE_NULL, > > > .arg4_type = ARG_ANYTHING, > > > > > > Ideally all three (bpf_reg_type, bpf_return_type, and bpf_arg_type) > > > would share the same flag bit: MAYBE_NULL. > > I support using the same bit value, but should we use the exact same > enum name for three different enums? Like MAYBE_NULL, which enum is it > defined in? Wouldn't RET_MAYBE_NULL and RET_MAYBE_NULL, in addition to > REG_MAYBE_NULL be more explicit about what they apply to? argh, I meant to write "RET_MAYBE_NULL and ARG_MAYBE_NULL, in addition to REG_MAYBE_NULL". > > BTW (see my comment on another patch), _OR_NULL and _OR_ZERO are not > the same thing, are they? Is the plan to use two different bits for > them or pretend that CONST_OR_ZERO "may be null"? > > > > Then static bool arg_type_may_be_null() will be comparing only single bit ? > > > > > > While > > > if (arg_type == ARG_PTR_TO_MAP_VALUE || > > > arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE || > > > arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL) { > > > will become: > > > arg_type &= FLAG_MASK; > > > if (arg_type == ARG_PTR_TO_MAP_VALUE || > > > arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE) { > > > > > > Most of the time I would prefer explicit .type and .flag structure, > > > but saving memory is important for bpf_reg_type, so explicit bit > > > operations are probably justified.
On Tue, Nov 9, 2021 at 8:36 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, Nov 9, 2021 at 8:34 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Tue, Nov 9, 2021 at 11:42 AM Hao Luo <haoluo@google.com> wrote: > > > > > > On Tue, Nov 9, 2021 at 10:21 AM Alexei Starovoitov > > > <alexei.starovoitov@gmail.com> wrote: > > > > > > > > On Mon, Nov 08, 2021 at 06:16:15PM -0800, Hao Luo wrote: > > > > > This is a pure cleanup patchset that tries to use flag to mark whether > > > > > an arg may be null. It replaces enum bpf_arg_type with a struct. Doing > > > > > so allows us to embed the properties of arguments in the struct, which > > > > > is a more scalable solution than introducing a new enum. This patchset > > > > > performs this transformation only on arg_type. If it looks good, > > > > > follow-up patches will do the same on reg_type and ret_type. > > > > > > > > > > The first patch replaces 'enum bpf_arg_type' with 'struct bpf_arg_type' > > > > > and each of the rest patches transforms one type of ARG_XXX_OR_NULLs. > > > > > > > > Nice. Thank you for working on it! > > > > > > No problem. :) > > > > > > > > > > > The enum->struct conversion works for bpf_arg_type, but applying > > > > the same technique to bpf_reg_type could be problematic. > > > > Since it's part of bpf_reg_state which in turn is multiplied by a large factor. > > > > Growing enum from 4 bytes to 8 byte struct will consume quite > > > > a lot of extra memory. > > > > > > > > > 19 files changed, 932 insertions(+), 780 deletions(-) > > > > > > > > Just bpf_arg_type refactoring adds a lot of churn which could make > > > > backports of future fixes not automatic anymore. > > > > Similar converstion for bpf_reg_type and bpf_return_type will > > > > be even more churn. > > > > > > Acknowledged. > > > > > > > Have you considered using upper bits to represent flags? > > > > > > Yes, I thought about that. Some of my thoughts are: > > > > > > - I wasn't sure how many bits should be reserved. Maybe 16 bits is good enough? > > > - What if we run out of flag bits in future? > > > - We could fold btf_id in the structure in this patchset. And new > > > fields could be easily added if needed. > > > > > > So with these questions, I didn't pursue that approach in the first > > > place. But I admit that it does look better by writing > > > > > > + .arg3_type = ARG_PTR_TO_STACK | MAYBE_NULL, > > > > > > Instead of > > > > > > + .arg3 = { > > > + .type = ARG_PTR_TO_MAP_VALUE, > > > + .flag = ARG_FLAG_MAYBE_NULL, > > > + }, > > > > > > Let's see if there is any further comment. I can go take a look and > > > prepare for that approach in the next revision. > > > > > > > +1 on staying within a single enum and using upper bits > > > > > > > > > > > > > > > > Instead of diff: > > > > - .arg1_type = ARG_CONST_MAP_PTR, > > > > - .arg2_type = ARG_PTR_TO_FUNC, > > > > - .arg3_type = ARG_PTR_TO_STACK_OR_NULL, > > > > - .arg4_type = ARG_ANYTHING, > > > > + .arg1 = { .type = ARG_CONST_MAP_PTR }, > > > > + .arg2 = { .type = ARG_PTR_TO_FUNC }, > > > > + .arg3 = { .type = ARG_PTR_TO_STACK_OR_NULL }, > > > > + .arg4 = { .type = ARG_ANYTHING }, > > > > > > > > can we make it look like: > > > > .arg1_type = ARG_CONST_MAP_PTR, > > > > .arg2_type = ARG_PTR_TO_FUNC, > > > > - .arg3_type = ARG_PTR_TO_STACK_OR_NULL, > > > > + .arg3_type = ARG_PTR_TO_STACK | MAYBE_NULL, > > > > .arg4_type = ARG_ANYTHING, > > > > > > > > Ideally all three (bpf_reg_type, bpf_return_type, and bpf_arg_type) > > > > would share the same flag bit: MAYBE_NULL. > > > > I support using the same bit value, but should we use the exact same > > enum name for three different enums? Like MAYBE_NULL, which enum is it > > defined in? Wouldn't RET_MAYBE_NULL and RET_MAYBE_NULL, in addition to > > REG_MAYBE_NULL be more explicit about what they apply to? > > argh, I meant to write "RET_MAYBE_NULL and ARG_MAYBE_NULL, in addition > to REG_MAYBE_NULL". Why differentiate? What difference do you see? The flag looks the same to me in return, reg and arg. > > > > BTW (see my comment on another patch), _OR_NULL and _OR_ZERO are not > > the same thing, are they? Is the plan to use two different bits for > > them or pretend that CONST_OR_ZERO "may be null"? What's the difference ? I think single MAYBE_NULL (or call it MAYBE_ZERO) can apply correctly to both ARG_CONST_SIZE, ARG_CONST_ALLOC_SIZE, and [ARG_]PTR_TO_MEM.
On Tue, Nov 9, 2021 at 8:41 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Tue, Nov 9, 2021 at 8:36 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Tue, Nov 9, 2021 at 8:34 PM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > > > > > > On Tue, Nov 9, 2021 at 11:42 AM Hao Luo <haoluo@google.com> wrote: > > > > > > > > On Tue, Nov 9, 2021 at 10:21 AM Alexei Starovoitov > > > > <alexei.starovoitov@gmail.com> wrote: > > > > > > > > > > On Mon, Nov 08, 2021 at 06:16:15PM -0800, Hao Luo wrote: > > > > > > This is a pure cleanup patchset that tries to use flag to mark whether > > > > > > an arg may be null. It replaces enum bpf_arg_type with a struct. Doing > > > > > > so allows us to embed the properties of arguments in the struct, which > > > > > > is a more scalable solution than introducing a new enum. This patchset > > > > > > performs this transformation only on arg_type. If it looks good, > > > > > > follow-up patches will do the same on reg_type and ret_type. > > > > > > > > > > > > The first patch replaces 'enum bpf_arg_type' with 'struct bpf_arg_type' > > > > > > and each of the rest patches transforms one type of ARG_XXX_OR_NULLs. > > > > > > > > > > Nice. Thank you for working on it! > > > > > > > > No problem. :) > > > > > > > > > > > > > > The enum->struct conversion works for bpf_arg_type, but applying > > > > > the same technique to bpf_reg_type could be problematic. > > > > > Since it's part of bpf_reg_state which in turn is multiplied by a large factor. > > > > > Growing enum from 4 bytes to 8 byte struct will consume quite > > > > > a lot of extra memory. > > > > > > > > > > > 19 files changed, 932 insertions(+), 780 deletions(-) > > > > > > > > > > Just bpf_arg_type refactoring adds a lot of churn which could make > > > > > backports of future fixes not automatic anymore. > > > > > Similar converstion for bpf_reg_type and bpf_return_type will > > > > > be even more churn. > > > > > > > > Acknowledged. > > > > > > > > > Have you considered using upper bits to represent flags? > > > > > > > > Yes, I thought about that. Some of my thoughts are: > > > > > > > > - I wasn't sure how many bits should be reserved. Maybe 16 bits is good enough? > > > > - What if we run out of flag bits in future? > > > > - We could fold btf_id in the structure in this patchset. And new > > > > fields could be easily added if needed. > > > > > > > > So with these questions, I didn't pursue that approach in the first > > > > place. But I admit that it does look better by writing > > > > > > > > + .arg3_type = ARG_PTR_TO_STACK | MAYBE_NULL, > > > > > > > > Instead of > > > > > > > > + .arg3 = { > > > > + .type = ARG_PTR_TO_MAP_VALUE, > > > > + .flag = ARG_FLAG_MAYBE_NULL, > > > > + }, > > > > > > > > Let's see if there is any further comment. I can go take a look and > > > > prepare for that approach in the next revision. > > > > > > > > > > +1 on staying within a single enum and using upper bits > > > > > > > > > > > > > > > > > > > > > Instead of diff: > > > > > - .arg1_type = ARG_CONST_MAP_PTR, > > > > > - .arg2_type = ARG_PTR_TO_FUNC, > > > > > - .arg3_type = ARG_PTR_TO_STACK_OR_NULL, > > > > > - .arg4_type = ARG_ANYTHING, > > > > > + .arg1 = { .type = ARG_CONST_MAP_PTR }, > > > > > + .arg2 = { .type = ARG_PTR_TO_FUNC }, > > > > > + .arg3 = { .type = ARG_PTR_TO_STACK_OR_NULL }, > > > > > + .arg4 = { .type = ARG_ANYTHING }, > > > > > > > > > > can we make it look like: > > > > > .arg1_type = ARG_CONST_MAP_PTR, > > > > > .arg2_type = ARG_PTR_TO_FUNC, > > > > > - .arg3_type = ARG_PTR_TO_STACK_OR_NULL, > > > > > + .arg3_type = ARG_PTR_TO_STACK | MAYBE_NULL, > > > > > .arg4_type = ARG_ANYTHING, > > > > > > > > > > Ideally all three (bpf_reg_type, bpf_return_type, and bpf_arg_type) > > > > > would share the same flag bit: MAYBE_NULL. > > > > > > I support using the same bit value, but should we use the exact same > > > enum name for three different enums? Like MAYBE_NULL, which enum is it > > > defined in? Wouldn't RET_MAYBE_NULL and RET_MAYBE_NULL, in addition to > > > REG_MAYBE_NULL be more explicit about what they apply to? > > > > argh, I meant to write "RET_MAYBE_NULL and ARG_MAYBE_NULL, in addition > > to REG_MAYBE_NULL". > > Why differentiate? What difference do you see? > The flag looks the same to me in return, reg and arg. We have three different enums which will be combined with some constants defined outside of some of those enums. Just have a gut feeling that this will bite us at some point. Nothing more. > > > > > > > BTW (see my comment on another patch), _OR_NULL and _OR_ZERO are not > > > the same thing, are they? Is the plan to use two different bits for > > > them or pretend that CONST_OR_ZERO "may be null"? > > What's the difference ? > I think single MAYBE_NULL (or call it MAYBE_ZERO) can apply correctly > to both ARG_CONST_SIZE, ARG_CONST_ALLOC_SIZE, and [ARG_]PTR_TO_MEM. Again, just gut feeling that this will go wrong. But also one specific example from kernel/bpf/verifier.c: if (register_is_null(reg) && arg_type_may_be_null(arg_type)) goto skip_type_check; Currently arg_type_may_be_null(arg_type) returns false for ARG_CONST_SIZE_OR_ZERO. If we are not careful and blindly check the MAYBE_NULL flag (which the current patch set seems to be doing), we'll start returning true for it and some other _OR_ZERO arg types. It might be benign in this particular case, I haven't traced if ARG_CONST_SIZE_OR_ZERO can be passed in that particular code path, but it was hardly intended this way, no?
On Tue, Nov 09, 2021 at 09:00:25PM -0800, Andrii Nakryiko wrote: > > But also one specific example from kernel/bpf/verifier.c: > > if (register_is_null(reg) && arg_type_may_be_null(arg_type)) > goto skip_type_check; > > Currently arg_type_may_be_null(arg_type) returns false for > ARG_CONST_SIZE_OR_ZERO. If we are not careful and blindly check the > MAYBE_NULL flag (which the current patch set seems to be doing), we'll > start returning true for it and some other _OR_ZERO arg types. It > might be benign in this particular case, I haven't traced if > ARG_CONST_SIZE_OR_ZERO can be passed in that particular code path, but > it was hardly intended this way, no? I think it's an example where uniform handling of MAYBE_ZERO flag would have helped. The case of arg_type_may_be_null() missing in ARG_CONST_SIZE_OR_ZERO doesn't hurt. The subsequent check_reg_type() is just doing unnecessary work (it's checking that reg is scalar, but register_is_null already did that). I think such application of flags would have positive result. Doesn't mean that we should apply it blindly. There could be a case where it would be incorrect, but as this example demonstrated it's more likely to find cases where it's safe to do. We just forgot to include all of OR_NULL flavors here and could be in other places too. I think your main point that gotta be very careful about introducing the flags. That's for sure. As anything that touches the verifier.