| Message ID | 20231209002709.535966-5-thinker.li@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Registrating struct_ops types from modules | expand |
On 12/8/23 4:26 PM, thinker.li@gmail.com wrote: > +const struct bpf_struct_ops_desc *btf_get_struct_ops(struct btf *btf, u32 *ret_cnt) > +{ > + if (!btf) > + return NULL; > + if (!btf->struct_ops_tab) *ret_cnt = 0; unless the later patch checks the return value NULL before using *ret_cnt. Anyway, better to set *ret_cnt to 0 if the btf has no struct_ops. The same should go for the "!btf" case above but I suspect the above !btf check is unnecessary also and the caller should have checked for !btf itself instead of expecting a list of struct_ops from a NULL btf. Lets continue the review on the later patches for now to confirm where the above !btf case might happen. > + return NULL; > + > + *ret_cnt = btf->struct_ops_tab->cnt; > + return (const struct bpf_struct_ops_desc *)btf->struct_ops_tab->ops; > +}
On 12/14/23 18:22, Martin KaFai Lau wrote: > On 12/8/23 4:26 PM, thinker.li@gmail.com wrote: >> +const struct bpf_struct_ops_desc *btf_get_struct_ops(struct btf *btf, >> u32 *ret_cnt) >> +{ >> + if (!btf) >> + return NULL; >> + if (!btf->struct_ops_tab) > > *ret_cnt = 0; > > unless the later patch checks the return value NULL before using *ret_cnt. > Anyway, better to set *ret_cnt to 0 if the btf has no struct_ops. > > The same should go for the "!btf" case above but I suspect the above > !btf check is unnecessary also and the caller should have checked for > !btf itself instead of expecting a list of struct_ops from a NULL btf. > Lets continue the review on the later patches for now to confirm where > the above !btf case might happen. Checking callers, I didn't find anything that make btf here NULL so far. It is safe to remove !btf check. For the same reason as assigning *ret_cnt for safe, this check should be fine here as well, right? I don't have strong opinion here. What I though is to keep the values as it is without any side-effect if the function call fails and if possible. And, the callers should not expect the callee to set some specific values when a call fails. > >> + return NULL; >> + >> + *ret_cnt = btf->struct_ops_tab->cnt; >> + return (const struct bpf_struct_ops_desc *)btf->struct_ops_tab->ops; >> +} >
On 12/15/23 1:42 PM, Kui-Feng Lee wrote: > > > On 12/14/23 18:22, Martin KaFai Lau wrote: >> On 12/8/23 4:26 PM, thinker.li@gmail.com wrote: >>> +const struct bpf_struct_ops_desc *btf_get_struct_ops(struct btf *btf, u32 >>> *ret_cnt) >>> +{ >>> + if (!btf) >>> + return NULL; >>> + if (!btf->struct_ops_tab) >> >> *ret_cnt = 0; >> >> unless the later patch checks the return value NULL before using *ret_cnt. >> Anyway, better to set *ret_cnt to 0 if the btf has no struct_ops. >> >> The same should go for the "!btf" case above but I suspect the above !btf >> check is unnecessary also and the caller should have checked for !btf itself >> instead of expecting a list of struct_ops from a NULL btf. Lets continue the >> review on the later patches for now to confirm where the above !btf case might >> happen. > > Checking callers, I didn't find anything that make btf here NULL so far. > It is safe to remove !btf check. For the same reason as assigning > *ret_cnt for safe, this check should be fine here as well, right? If for safety, why ref_cnt is not checked for NULL also? The userspace passed-in btf should have been checked for NULL long time before reaching here. There is no need to be over protective here. It would really need a BUG_ON instead if btf was NULL here (don't add a BUG_ON though). afaict, no function in btf.c is checking the btf argument for NULL also. > > I don't have strong opinion here. What I though is to keep the values > as it is without any side-effect if the function call fails and if > possible. And, the callers should not expect the callee to set some > specific values when a call fails. For *ref_cnt stay uninit, there is a bug in patch 10 which exactly assumes 0 is set in *ret_cnt when there is no struct_ops. It is a good signal on how this function will be used. I think it is arguable whether returning NULL here is failure. I would argue getting a 0 struct_ops_desc array is not a failure. It is why the !btf case confuses the return NULL case to mean a never would happen error instead of meaning there is no struct_ops. Taking out the !btf case, NULL means there is no struct_ops (instead of failure), so 0 cnt. Anyhow, either assign 0 to *ret_cnt here, or fix patch 10 to init the local cnt 0 and write a warning comment here in btf_get_struct_ops() saying ret_cnt won't be set when there is no struct_ops in the btf. When looking at it again, how about moving the bpf_struct_ops_find_*() to btf.c. Then it will avoid the need of the new btf_get_struct_ops() function. bpf_struct_ops_find_*() can directly use the btf->struct_ops_tab. > >> >>> + return NULL; >>> + >>> + *ret_cnt = btf->struct_ops_tab->cnt; >>> + return (const struct bpf_struct_ops_desc *)btf->struct_ops_tab->ops; >>> +} >>
On 12/15/23 17:19, Martin KaFai Lau wrote: > On 12/15/23 1:42 PM, Kui-Feng Lee wrote: >> >> >> On 12/14/23 18:22, Martin KaFai Lau wrote: >>> On 12/8/23 4:26 PM, thinker.li@gmail.com wrote: >>>> +const struct bpf_struct_ops_desc *btf_get_struct_ops(struct btf >>>> *btf, u32 *ret_cnt) >>>> +{ >>>> + if (!btf) >>>> + return NULL; >>>> + if (!btf->struct_ops_tab) >>> >>> *ret_cnt = 0; >>> >>> unless the later patch checks the return value NULL before using >>> *ret_cnt. >>> Anyway, better to set *ret_cnt to 0 if the btf has no struct_ops. >>> >>> The same should go for the "!btf" case above but I suspect the above >>> !btf check is unnecessary also and the caller should have checked for >>> !btf itself instead of expecting a list of struct_ops from a NULL >>> btf. Lets continue the review on the later patches for now to confirm >>> where the above !btf case might happen. >> >> Checking callers, I didn't find anything that make btf here NULL so far. > >> It is safe to remove !btf check. For the same reason as assigning >> *ret_cnt for safe, this check should be fine here as well, right? > > If for safety, why ref_cnt is not checked for NULL also? The userspace > passed-in btf should have been checked for NULL long time before > reaching here. There is no need to be over protective here. It would > really need a BUG_ON instead if btf was NULL here (don't add a BUG_ON > though). > > afaict, no function in btf.c is checking the btf argument for NULL also. > >> >> I don't have strong opinion here. What I though is to keep the values >> as it is without any side-effect if the function call fails and if >> possible. And, the callers should not expect the callee to set some >> specific values when a call fails. > > For *ref_cnt stay uninit, there is a bug in patch 10 which exactly > assumes 0 is set in *ret_cnt when there is no struct_ops. It is a good > signal on how this function will be used. > > I think it is arguable whether returning NULL here is failure. I would > argue getting a 0 struct_ops_desc array is not a failure. It is why the > !btf case confuses the return NULL case to mean a never would happen > error instead of meaning there is no struct_ops. Taking out the !btf > case, NULL means there is no struct_ops (instead of failure), so 0 cnt. > > Anyhow, either assign 0 to *ret_cnt here, or fix patch 10 to init the > local cnt 0 and write a warning comment here in btf_get_struct_ops() > saying ret_cnt won't be set when there is no struct_ops in the btf. I will fix at the patch 10 by setting local cnt 0. > > When looking at it again, how about moving the bpf_struct_ops_find_*() > to btf.c. Then it will avoid the need of the new btf_get_struct_ops() > function. bpf_struct_ops_find_*() can directly use the btf->struct_ops_tab. > I prefer to keep them in bpf_struct_ops.c if it is ok to you. Fixing the initialization issue of bpf_struct_ops_find() should be enough. > >> >>> >>>> + return NULL; >>>> + >>>> + *ret_cnt = btf->struct_ops_tab->cnt; >>>> + return (const struct bpf_struct_ops_desc >>>> *)btf->struct_ops_tab->ops; >>>> +} >>> >
On 12/15/23 9:43 PM, Kui-Feng Lee wrote: > > > On 12/15/23 17:19, Martin KaFai Lau wrote: >> On 12/15/23 1:42 PM, Kui-Feng Lee wrote: >>> >>> >>> On 12/14/23 18:22, Martin KaFai Lau wrote: >>>> On 12/8/23 4:26 PM, thinker.li@gmail.com wrote: >>>>> +const struct bpf_struct_ops_desc *btf_get_struct_ops(struct btf *btf, u32 >>>>> *ret_cnt) >>>>> +{ >>>>> + if (!btf) >>>>> + return NULL; >>>>> + if (!btf->struct_ops_tab) >>>> >>>> *ret_cnt = 0; >>>> >>>> unless the later patch checks the return value NULL before using *ret_cnt. >>>> Anyway, better to set *ret_cnt to 0 if the btf has no struct_ops. >>>> >>>> The same should go for the "!btf" case above but I suspect the above !btf >>>> check is unnecessary also and the caller should have checked for !btf itself >>>> instead of expecting a list of struct_ops from a NULL btf. Lets continue the >>>> review on the later patches for now to confirm where the above !btf case >>>> might happen. >>> >>> Checking callers, I didn't find anything that make btf here NULL so far. >> >>> It is safe to remove !btf check. For the same reason as assigning >>> *ret_cnt for safe, this check should be fine here as well, right? >> >> If for safety, why ref_cnt is not checked for NULL also? The userspace >> passed-in btf should have been checked for NULL long time before reaching >> here. There is no need to be over protective here. It would really need a >> BUG_ON instead if btf was NULL here (don't add a BUG_ON though). >> >> afaict, no function in btf.c is checking the btf argument for NULL also. >> >>> >>> I don't have strong opinion here. What I though is to keep the values >>> as it is without any side-effect if the function call fails and if >>> possible. And, the callers should not expect the callee to set some >>> specific values when a call fails. >> >> For *ref_cnt stay uninit, there is a bug in patch 10 which exactly assumes 0 >> is set in *ret_cnt when there is no struct_ops. It is a good signal on how >> this function will be used. >> >> I think it is arguable whether returning NULL here is failure. I would argue >> getting a 0 struct_ops_desc array is not a failure. It is why the !btf case >> confuses the return NULL case to mean a never would happen error instead of >> meaning there is no struct_ops. Taking out the !btf case, NULL means there is >> no struct_ops (instead of failure), so 0 cnt. >> >> Anyhow, either assign 0 to *ret_cnt here, or fix patch 10 to init the local >> cnt 0 and write a warning comment here in btf_get_struct_ops() saying ret_cnt >> won't be set when there is no struct_ops in the btf. > > > I will fix at the patch 10 by setting local cnt 0. > >> >> When looking at it again, how about moving the bpf_struct_ops_find_*() to >> btf.c. Then it will avoid the need of the new btf_get_struct_ops() function. >> bpf_struct_ops_find_*() can directly use the btf->struct_ops_tab. >> > > I prefer to keep them in bpf_struct_ops.c if it is ok to you. > Fixing the initialization issue of bpf_struct_ops_find() > should be enough. If choosing between fixing the bug in patch 10 and moving them to btf.c, I would avoid patch 10 (and future) bug to begin with by moving them to btf.c. Moving them should be cheap unless there is other dependency that I have overlooked. > >> >>> >>>> >>>>> + return NULL; >>>>> + >>>>> + *ret_cnt = btf->struct_ops_tab->cnt; >>>>> + return (const struct bpf_struct_ops_desc *)btf->struct_ops_tab->ops; >>>>> +} >>>> >>
On 12/16/23 08:48, Martin KaFai Lau wrote: > On 12/15/23 9:43 PM, Kui-Feng Lee wrote: >> >> >> On 12/15/23 17:19, Martin KaFai Lau wrote: >>> On 12/15/23 1:42 PM, Kui-Feng Lee wrote: >>>> >>>> >>>> On 12/14/23 18:22, Martin KaFai Lau wrote: >>>>> On 12/8/23 4:26 PM, thinker.li@gmail.com wrote: >>>>>> +const struct bpf_struct_ops_desc *btf_get_struct_ops(struct btf >>>>>> *btf, u32 *ret_cnt) >>>>>> +{ >>>>>> + if (!btf) >>>>>> + return NULL; >>>>>> + if (!btf->struct_ops_tab) >>>>> >>>>> *ret_cnt = 0; >>>>> >>>>> unless the later patch checks the return value NULL before using >>>>> *ret_cnt. >>>>> Anyway, better to set *ret_cnt to 0 if the btf has no struct_ops. >>>>> >>>>> The same should go for the "!btf" case above but I suspect the >>>>> above !btf check is unnecessary also and the caller should have >>>>> checked for !btf itself instead of expecting a list of struct_ops >>>>> from a NULL btf. Lets continue the review on the later patches for >>>>> now to confirm where the above !btf case might happen. >>>> >>>> Checking callers, I didn't find anything that make btf here NULL so >>>> far. >>> >>>> It is safe to remove !btf check. For the same reason as assigning >>>> *ret_cnt for safe, this check should be fine here as well, right? >>> >>> If for safety, why ref_cnt is not checked for NULL also? The >>> userspace passed-in btf should have been checked for NULL long time >>> before reaching here. There is no need to be over protective here. It >>> would really need a BUG_ON instead if btf was NULL here (don't add a >>> BUG_ON though). >>> >>> afaict, no function in btf.c is checking the btf argument for NULL also. >>> >>>> >>>> I don't have strong opinion here. What I though is to keep the values >>>> as it is without any side-effect if the function call fails and if >>>> possible. And, the callers should not expect the callee to set some >>>> specific values when a call fails. >>> >>> For *ref_cnt stay uninit, there is a bug in patch 10 which exactly >>> assumes 0 is set in *ret_cnt when there is no struct_ops. It is a >>> good signal on how this function will be used. >>> >>> I think it is arguable whether returning NULL here is failure. I >>> would argue getting a 0 struct_ops_desc array is not a failure. It is >>> why the !btf case confuses the return NULL case to mean a never would >>> happen error instead of meaning there is no struct_ops. Taking out >>> the !btf case, NULL means there is no struct_ops (instead of >>> failure), so 0 cnt. >>> >>> Anyhow, either assign 0 to *ret_cnt here, or fix patch 10 to init the >>> local cnt 0 and write a warning comment here in btf_get_struct_ops() >>> saying ret_cnt won't be set when there is no struct_ops in the btf. >> >> >> I will fix at the patch 10 by setting local cnt 0 >> >>> >>> When looking at it again, how about moving the >>> bpf_struct_ops_find_*() to btf.c. Then it will avoid the need of the >>> new btf_get_struct_ops() function. bpf_struct_ops_find_*() can >>> directly use the btf->struct_ops_tab. >>> >> >> I prefer to keep them in bpf_struct_ops.c if it is ok to you. >> Fixing the initialization issue of bpf_struct_ops_find() >> should be enough. > > If choosing between fixing the bug in patch 10 and moving them to btf.c, > I would avoid patch 10 (and future) bug to begin with by moving them to > btf.c. Moving them should be cheap unless there is other dependency that > I have overlooked. Ok! Got it! > >> >>> >>>> >>>>> >>>>>> + return NULL; >>>>>> + >>>>>> + *ret_cnt = btf->struct_ops_tab->cnt; >>>>>> + return (const struct bpf_struct_ops_desc >>>>>> *)btf->struct_ops_tab->ops; >>>>>> +} >>>>> >>> >
diff --git a/include/linux/btf.h b/include/linux/btf.h index 1d852dad7473..e2f4b85cf82a 100644 --- a/include/linux/btf.h +++ b/include/linux/btf.h @@ -584,4 +584,9 @@ static inline bool btf_type_is_struct_ptr(struct btf *btf, const struct btf_type return btf_type_is_struct(t); } +struct bpf_struct_ops_desc; + +const struct bpf_struct_ops_desc * +btf_get_struct_ops(struct btf *btf, u32 *ret_cnt); + #endif diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 6935a88ed190..edbe3cbf2dcc 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -241,6 +241,12 @@ struct btf_id_dtor_kfunc_tab { struct btf_id_dtor_kfunc dtors[]; }; +struct btf_struct_ops_tab { + u32 cnt; + u32 capacity; + struct bpf_struct_ops_desc ops[]; +}; + struct btf { void *data; struct btf_type **types; @@ -258,6 +264,7 @@ struct btf { struct btf_kfunc_set_tab *kfunc_set_tab; struct btf_id_dtor_kfunc_tab *dtor_kfunc_tab; struct btf_struct_metas *struct_meta_tab; + struct btf_struct_ops_tab *struct_ops_tab; /* split BTF support */ struct btf *base_btf; @@ -1688,11 +1695,20 @@ static void btf_free_struct_meta_tab(struct btf *btf) btf->struct_meta_tab = NULL; } +static void btf_free_struct_ops_tab(struct btf *btf) +{ + struct btf_struct_ops_tab *tab = btf->struct_ops_tab; + + kfree(tab); + btf->struct_ops_tab = NULL; +} + static void btf_free(struct btf *btf) { btf_free_struct_meta_tab(btf); btf_free_dtor_kfunc_tab(btf); btf_free_kfunc_set_tab(btf); + btf_free_struct_ops_tab(btf); kvfree(btf->types); kvfree(btf->resolved_sizes); kvfree(btf->resolved_ids); @@ -8604,3 +8620,60 @@ bool btf_type_ids_nocast_alias(struct bpf_verifier_log *log, return !strncmp(reg_name, arg_name, cmp_len); } + +static int +btf_add_struct_ops(struct btf *btf, struct bpf_struct_ops *st_ops) +{ + struct btf_struct_ops_tab *tab, *new_tab; + int i; + + if (!btf) + return -ENOENT; + + /* Assume this function is called for a module when the module is + * loading. + */ + + tab = btf->struct_ops_tab; + if (!tab) { + tab = kzalloc(offsetof(struct btf_struct_ops_tab, ops[4]), + GFP_KERNEL); + if (!tab) + return -ENOMEM; + tab->capacity = 4; + btf->struct_ops_tab = tab; + } + + for (i = 0; i < tab->cnt; i++) + if (tab->ops[i].st_ops == st_ops) + return -EEXIST; + + if (tab->cnt == tab->capacity) { + new_tab = krealloc(tab, + offsetof(struct btf_struct_ops_tab, + ops[tab->capacity * 2]), + GFP_KERNEL); + if (!new_tab) + return -ENOMEM; + tab = new_tab; + tab->capacity *= 2; + btf->struct_ops_tab = tab; + } + + tab->ops[btf->struct_ops_tab->cnt].st_ops = st_ops; + + btf->struct_ops_tab->cnt++; + + return 0; +} + +const struct bpf_struct_ops_desc *btf_get_struct_ops(struct btf *btf, u32 *ret_cnt) +{ + if (!btf) + return NULL; + if (!btf->struct_ops_tab) + return NULL; + + *ret_cnt = btf->struct_ops_tab->cnt; + return (const struct bpf_struct_ops_desc *)btf->struct_ops_tab->ops; +}