It has been seen that for some network mac drivers (i.e. lan78xx) the related module for the phy is loaded dynamically depending on the current hardware. In this case, the associated phy is read using mdio bus and then the associated phy module is loaded during runtime (kernel function phy_request_driver_module). However, no software dependency is defined, so the user tools will no be able to get this dependency. For example, if dracut is used and the hardware is present, lan78xx will be included but no phy module will be added, and in the next restart the device will not work from boot because no related phy will be found during initramfs stage. In order to solve this, we could define a normal 'pre' software dependency in lan78xx module with all the possible phy modules (there may be some), but proceeding in that way, all the possible phy modules would be loaded while only one is necessary. The idea is to add a new attribute when the software dependency is defined, apart from the normal ones 'pre' and 'post', I have called it 'user', to be used only by the user tools that need to detect this situation. In that way, for example, dracut could check the 'user' attribute of the modules in order to install these software dependencies in initramfs too. That is, for the commented lan78xx module, defining the 'user' attribute to the software dependency with the possible phy modules list, only the necessary phy would be loaded on demand keeping the same behavior but all the possible phy modules would be available from initramfs. A new function 'kmod_module_get_user_softdeps' in libkmod will be added for this to avoid breaking the API and maintain backward compatibility. This general procedure could be useful for other similar cases (not only for dynamic phy loading). Signed-off-by: Jose Ignacio Tornos Martinez <jtornosm@redhat.com> --- libkmod/docs/libkmod-sections.txt | 1 + libkmod/libkmod-config.c | 66 +++++++++++++++++++++++++++---- libkmod/libkmod-internal.h | 1 + libkmod/libkmod-module.c | 50 +++++++++++++++++++++++ libkmod/libkmod.h | 2 + libkmod/libkmod.sym | 1 + 6 files changed, 114 insertions(+), 7 deletions(-) diff --git a/libkmod/docs/libkmod-sections.txt b/libkmod/docs/libkmod-sections.txt index 33d9eec..04743e4 100644 --- a/libkmod/docs/libkmod-sections.txt +++ b/libkmod/docs/libkmod-sections.txt @@ -62,6 +62,7 @@ kmod_module_remove_module kmod_module_get_module kmod_module_get_dependencies kmod_module_get_softdeps +kmod_module_get_user_softdeps kmod_module_apply_filter kmod_module_get_filtered_blacklist kmod_module_get_install_commands diff --git a/libkmod/libkmod-config.c b/libkmod/libkmod-config.c index e83621b..c0e15be 100644 --- a/libkmod/libkmod-config.c +++ b/libkmod/libkmod-config.c @@ -54,8 +54,10 @@ struct kmod_softdep { char *name; const char **pre; const char **post; + const char **user; unsigned int n_pre; unsigned int n_post; + unsigned int n_user; }; const char *kmod_blacklist_get_modname(const struct kmod_list *l) @@ -110,6 +112,12 @@ const char * const *kmod_softdep_get_post(const struct kmod_list *l, unsigned in return dep->post; } +const char * const *kmod_softdep_get_user(const struct kmod_list *l, unsigned int *count) { + const struct kmod_softdep *dep = l->data; + *count = dep->n_user; + return dep->user; +} + static int kmod_config_add_command(struct kmod_config *config, const char *modname, const char *command, @@ -263,11 +271,11 @@ static int kmod_config_add_softdep(struct kmod_config *config, struct kmod_softdep *dep; const char *s, *p; char *itr; - unsigned int n_pre = 0, n_post = 0; + unsigned int n_pre = 0, n_post = 0, n_user = 0; size_t modnamelen = strlen(modname) + 1; size_t buflen = 0; bool was_space = false; - enum { S_NONE, S_PRE, S_POST } mode = S_NONE; + enum { S_NONE, S_PRE, S_POST, S_USER } mode = S_NONE; DBG(config->ctx, "modname=%s\n", modname); @@ -298,6 +306,9 @@ static int kmod_config_add_softdep(struct kmod_config *config, else if (plen == sizeof("post:") - 1 && memcmp(p, "post:", sizeof("post:") - 1) == 0) mode = S_POST; + else if (plen == sizeof("user:") - 1 && + memcmp(p, "user:", sizeof("user:") - 1) == 0) + mode = S_USER; else if (*s != '\0' || (*s == '\0' && !was_space)) { if (mode == S_PRE) { buflen += plen + 1; @@ -305,6 +316,9 @@ static int kmod_config_add_softdep(struct kmod_config *config, } else if (mode == S_POST) { buflen += plen + 1; n_post++; + } else if (mode == S_USER) { + buflen += plen + 1; + n_user++; } } p = s + 1; @@ -312,11 +326,12 @@ static int kmod_config_add_softdep(struct kmod_config *config, break; } - DBG(config->ctx, "%u pre, %u post\n", n_pre, n_post); + DBG(config->ctx, "%u pre, %u post, %u user\n", n_pre, n_post, n_user); dep = malloc(sizeof(struct kmod_softdep) + modnamelen + n_pre * sizeof(const char *) + n_post * sizeof(const char *) + + n_user * sizeof(const char *) + buflen); if (dep == NULL) { ERR(config->ctx, "out-of-memory modname=%s\n", modname); @@ -324,9 +339,11 @@ static int kmod_config_add_softdep(struct kmod_config *config, } dep->n_pre = n_pre; dep->n_post = n_post; + dep->n_user = n_user; dep->pre = (const char **)((char *)dep + sizeof(struct kmod_softdep)); dep->post = dep->pre + n_pre; - dep->name = (char *)(dep->post + n_post); + dep->user = dep->post + n_post; + dep->name = (char *)(dep->user + n_user); memcpy(dep->name, modname, modnamelen); @@ -334,6 +351,7 @@ static int kmod_config_add_softdep(struct kmod_config *config, itr = dep->name + modnamelen; n_pre = 0; n_post = 0; + n_user = 0; mode = S_NONE; was_space = false; for (p = s = line; ; s++) { @@ -362,6 +380,9 @@ static int kmod_config_add_softdep(struct kmod_config *config, else if (plen == sizeof("post:") - 1 && memcmp(p, "post:", sizeof("post:") - 1) == 0) mode = S_POST; + else if (plen == sizeof("user:") - 1 && + memcmp(p, "user:", sizeof("user:") - 1) == 0) + mode = S_USER; else if (*s != '\0' || (*s == '\0' && !was_space)) { if (mode == S_PRE) { dep->pre[n_pre] = itr; @@ -375,6 +396,12 @@ static int kmod_config_add_softdep(struct kmod_config *config, itr[plen] = '\0'; itr += plen + 1; n_post++; + } else if (mode == S_USER) { + dep->user[n_user] = itr; + memcpy(itr, p, plen); + itr[plen] = '\0'; + itr += plen + 1; + n_user++; } } p = s + 1; @@ -395,14 +422,15 @@ static int kmod_config_add_softdep(struct kmod_config *config, static char *softdep_to_char(struct kmod_softdep *dep) { const size_t sz_preprefix = sizeof("pre: ") - 1; const size_t sz_postprefix = sizeof("post: ") - 1; + const size_t sz_userprefix = sizeof("user: ") - 1; size_t sz = 1; /* at least '\0' */ - size_t sz_pre, sz_post; + size_t sz_pre, sz_post, sz_user; const char *start, *end; char *s, *itr; /* - * Rely on the fact that dep->pre[] and dep->post[] are strv's that - * point to a contiguous buffer + * Rely on the fact that dep->pre[] dep->post[] and dep->user[] + * are strv's that point to a contiguous buffer */ if (dep->n_pre > 0) { start = dep->pre[0]; @@ -422,6 +450,15 @@ static char *softdep_to_char(struct kmod_softdep *dep) { } else sz_post = 0; + if (dep->n_user > 0) { + start = dep->user[0]; + end = dep->user[dep->n_user - 1] + + strlen(dep->user[dep->n_user - 1]); + sz_user = end - start; + sz += sz_user + sz_userprefix; + } else + sz_user = 0; + itr = s = malloc(sz); if (s == NULL) return NULL; @@ -456,6 +493,21 @@ static char *softdep_to_char(struct kmod_softdep *dep) { itr = p; } + if (sz_user) { + char *p; + + memcpy(itr, "user: ", sz_userprefix); + itr += sz_userprefix; + + /* include last '\0' */ + memcpy(itr, dep->user[0], sz_user + 1); + for (p = itr; p < itr + sz_user; p++) { + if (*p == '\0') + *p = ' '; + } + itr = p; + } + *itr = '\0'; return s; diff --git a/libkmod/libkmod-internal.h b/libkmod/libkmod-internal.h index 26a7e28..8e4f112 100644 --- a/libkmod/libkmod-internal.h +++ b/libkmod/libkmod-internal.h @@ -145,6 +145,7 @@ const char *kmod_command_get_modname(const struct kmod_list *l) __attribute__((n const char *kmod_softdep_get_name(const struct kmod_list *l) __attribute__((nonnull(1))); const char * const *kmod_softdep_get_pre(const struct kmod_list *l, unsigned int *count) __attribute__((nonnull(1, 2))); const char * const *kmod_softdep_get_post(const struct kmod_list *l, unsigned int *count); +const char * const *kmod_softdep_get_user(const struct kmod_list *l, unsigned int *count); /* libkmod-module.c */ diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c index 585da41..dbe676c 100644 --- a/libkmod/libkmod-module.c +++ b/libkmod/libkmod-module.c @@ -1664,6 +1664,56 @@ KMOD_EXPORT int kmod_module_get_softdeps(const struct kmod_module *mod, return 0; } +/** + * kmod_module_get_user_softdeps: + * @mod: kmod module + * @user: where to save the list of user soft dependencies. + * + * Get user dependencies for this kmod module. Soft dependencies come + * from configuration file and are not cached in @mod because it may include + * dependency cycles that would make we leak kmod_module. Any call + * to this function will search for this module in configuration, allocate a + * list and return the result. + * + * @user is newly created list of kmod_module and + * should be unreferenced with kmod_module_unref_list(). + * + * Returns: 0 on success or < 0 otherwise. + */ +KMOD_EXPORT int kmod_module_get_user_softdeps(const struct kmod_module *mod, + struct kmod_list **user) +{ + const struct kmod_list *l; + const struct kmod_config *config; + + if (mod == NULL || user == NULL) + return -ENOENT; + + assert(*user == NULL); + + config = kmod_get_config(mod->ctx); + + kmod_list_foreach(l, config->softdeps) { + const char *modname = kmod_softdep_get_name(l); + const char * const *array; + unsigned count; + + if (fnmatch(modname, mod->name, 0) != 0) + continue; + + array = kmod_softdep_get_user(l, &count); + *user = lookup_softdep(mod->ctx, array, count); + + /* + * find only the first command, as modprobe from + * module-init-tools does + */ + break; + } + + return 0; +} + /** * kmod_module_get_remove_commands: * @mod: kmod module diff --git a/libkmod/libkmod.h b/libkmod/libkmod.h index 7251aa7..ec6d270 100644 --- a/libkmod/libkmod.h +++ b/libkmod/libkmod.h @@ -196,6 +196,8 @@ const char *kmod_module_get_remove_commands(const struct kmod_module *mod); struct kmod_list *kmod_module_get_dependencies(const struct kmod_module *mod); int kmod_module_get_softdeps(const struct kmod_module *mod, struct kmod_list **pre, struct kmod_list **post); +int kmod_module_get_user_softdeps(const struct kmod_module *mod, + struct kmod_list **user); int kmod_module_get_filtered_blacklist(const struct kmod_ctx *ctx, const struct kmod_list *input, struct kmod_list **output) __attribute__ ((deprecated)); diff --git a/libkmod/libkmod.sym b/libkmod/libkmod.sym index 0c04fda..26c3eef 100644 --- a/libkmod/libkmod.sym +++ b/libkmod/libkmod.sym @@ -42,6 +42,7 @@ global: kmod_module_get_dependencies; kmod_module_get_softdeps; + kmod_module_get_user_softdeps; kmod_module_get_filtered_blacklist; kmod_module_get_name; -- 2.44.0
On Fri, Mar 15, 2024 at 4:52 PM Vlastimil Babka <vbabka@suse.cz> wrote: > > On 3/15/24 16:43, Suren Baghdasaryan wrote: > > On Fri, Mar 15, 2024 at 3:58 AM Vlastimil Babka <vbabka@suse.cz> wrote: > >> > >> On 3/6/24 19:24, Suren Baghdasaryan wrote: > >> > Account slab allocations using codetag reference embedded into slabobj_ext. > >> > > >> > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > >> > Co-developed-by: Kent Overstreet <kent.overstreet@linux.dev> > >> > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev> > >> > Reviewed-by: Kees Cook <keescook@chromium.org> > >> > >> Reviewed-by: Vlastimil Babka <vbabka@suse.cz> > >> > >> Nit below: > >> > >> > @@ -3833,6 +3913,7 @@ void slab_post_alloc_hook(struct kmem_cache *s, struct obj_cgroup *objcg, > >> > unsigned int orig_size) > >> > { > >> > unsigned int zero_size = s->object_size; > >> > + struct slabobj_ext *obj_exts; > >> > bool kasan_init = init; > >> > size_t i; > >> > gfp_t init_flags = flags & gfp_allowed_mask; > >> > @@ -3875,6 +3956,12 @@ void slab_post_alloc_hook(struct kmem_cache *s, struct obj_cgroup *objcg, > >> > kmemleak_alloc_recursive(p[i], s->object_size, 1, > >> > s->flags, init_flags); > >> > kmsan_slab_alloc(s, p[i], init_flags); > >> > + obj_exts = prepare_slab_obj_exts_hook(s, flags, p[i]); > >> > +#ifdef CONFIG_MEM_ALLOC_PROFILING > >> > + /* obj_exts can be allocated for other reasons */ > >> > + if (likely(obj_exts) && mem_alloc_profiling_enabled()) > > Could you at least flip these two checks then so the static key one goes first? Yes, definitely. I was thinking about removing need_slab_obj_ext() from prepare_slab_obj_exts_hook() and adding this instead of the above code: + if (need_slab_obj_ext()) { + obj_exts = prepare_slab_obj_exts_hook(s, flags, p[i]); +#ifdef CONFIG_MEM_ALLOC_PROFILING + /* + * Currently obj_exts is used only for allocation profiling. If other users appear + * then mem_alloc_profiling_enabled() check should be added here. + */ + if (likely(obj_exts)) + alloc_tag_add(&obj_exts->ref, current->alloc_tag, s->size); +#endif + } Does that look good? > >> > +#ifdef CONFIG_MEM_ALLOC_PROFILING > >> > + /* obj_exts can be allocated for other reasons */ > >> > + if (likely(obj_exts) && mem_alloc_profiling_enabled()) > > >> > + alloc_tag_add(&obj_exts->ref, current->alloc_tag, s->size); > >> > +#endif > >> > >> I think you could still do this a bit better: > >> > >> Check mem_alloc_profiling_enabled() once before the whole block calling > >> prepare_slab_obj_exts_hook() and alloc_tag_add() > >> Remove need_slab_obj_ext() check from prepare_slab_obj_exts_hook() > > > > Agree about checking mem_alloc_profiling_enabled() early and one time, > > except I would like to use need_slab_obj_ext() instead of > > mem_alloc_profiling_enabled() for that check. Currently they are > > equivalent but if there are more slab_obj_ext users in the future then > > there will be cases when we need to prepare_slab_obj_exts_hook() even > > when mem_alloc_profiling_enabled()==false. need_slab_obj_ext() will be > > easy to extend for such cases. > > I thought we don't generally future-proof internal implementation details > like this until it's actually needed. But at least what I suggested above > would help, thanks. > > > Thanks, > > Suren. > > > >> > >> > } > >> > > >> > memcg_slab_post_alloc_hook(s, objcg, flags, size, p); > >> > @@ -4353,6 +4440,7 @@ void slab_free(struct kmem_cache *s, struct slab *slab, void *object, > >> > unsigned long addr) > >> > { > >> > memcg_slab_free_hook(s, slab, &object, 1); > >> > + alloc_tagging_slab_free_hook(s, slab, &object, 1); > >> > > >> > if (likely(slab_free_hook(s, object, slab_want_init_on_free(s)))) > >> > do_slab_free(s, slab, object, object, 1, addr); > >> > @@ -4363,6 +4451,7 @@ void slab_free_bulk(struct kmem_cache *s, struct slab *slab, void *head, > >> > void *tail, void **p, int cnt, unsigned long addr) > >> > { > >> > memcg_slab_free_hook(s, slab, p, cnt); > >> > + alloc_tagging_slab_free_hook(s, slab, p, cnt); > >> > /* > >> > * With KASAN enabled slab_free_freelist_hook modifies the freelist > >> > * to remove objects, whose reuse must be delayed. > >> > > -- > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com. >
On 3/15/24 16:43, Suren Baghdasaryan wrote: > On Fri, Mar 15, 2024 at 3:58 AM Vlastimil Babka <vbabka@suse.cz> wrote: >> >> On 3/6/24 19:24, Suren Baghdasaryan wrote: >> > Account slab allocations using codetag reference embedded into slabobj_ext. >> > >> > Signed-off-by: Suren Baghdasaryan <surenb@google.com> >> > Co-developed-by: Kent Overstreet <kent.overstreet@linux.dev> >> > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev> >> > Reviewed-by: Kees Cook <keescook@chromium.org> >> >> Reviewed-by: Vlastimil Babka <vbabka@suse.cz> >> >> Nit below: >> >> > @@ -3833,6 +3913,7 @@ void slab_post_alloc_hook(struct kmem_cache *s, struct obj_cgroup *objcg, >> > unsigned int orig_size) >> > { >> > unsigned int zero_size = s->object_size; >> > + struct slabobj_ext *obj_exts; >> > bool kasan_init = init; >> > size_t i; >> > gfp_t init_flags = flags & gfp_allowed_mask; >> > @@ -3875,6 +3956,12 @@ void slab_post_alloc_hook(struct kmem_cache *s, struct obj_cgroup *objcg, >> > kmemleak_alloc_recursive(p[i], s->object_size, 1, >> > s->flags, init_flags); >> > kmsan_slab_alloc(s, p[i], init_flags); >> > + obj_exts = prepare_slab_obj_exts_hook(s, flags, p[i]); >> > +#ifdef CONFIG_MEM_ALLOC_PROFILING >> > + /* obj_exts can be allocated for other reasons */ >> > + if (likely(obj_exts) && mem_alloc_profiling_enabled()) Could you at least flip these two checks then so the static key one goes first? >> > + alloc_tag_add(&obj_exts->ref, current->alloc_tag, s->size); >> > +#endif >> >> I think you could still do this a bit better: >> >> Check mem_alloc_profiling_enabled() once before the whole block calling >> prepare_slab_obj_exts_hook() and alloc_tag_add() >> Remove need_slab_obj_ext() check from prepare_slab_obj_exts_hook() > > Agree about checking mem_alloc_profiling_enabled() early and one time, > except I would like to use need_slab_obj_ext() instead of > mem_alloc_profiling_enabled() for that check. Currently they are > equivalent but if there are more slab_obj_ext users in the future then > there will be cases when we need to prepare_slab_obj_exts_hook() even > when mem_alloc_profiling_enabled()==false. need_slab_obj_ext() will be > easy to extend for such cases. I thought we don't generally future-proof internal implementation details like this until it's actually needed. But at least what I suggested above would help, thanks. > Thanks, > Suren. > >> >> > } >> > >> > memcg_slab_post_alloc_hook(s, objcg, flags, size, p); >> > @@ -4353,6 +4440,7 @@ void slab_free(struct kmem_cache *s, struct slab *slab, void *object, >> > unsigned long addr) >> > { >> > memcg_slab_free_hook(s, slab, &object, 1); >> > + alloc_tagging_slab_free_hook(s, slab, &object, 1); >> > >> > if (likely(slab_free_hook(s, object, slab_want_init_on_free(s)))) >> > do_slab_free(s, slab, object, object, 1, addr); >> > @@ -4363,6 +4451,7 @@ void slab_free_bulk(struct kmem_cache *s, struct slab *slab, void *head, >> > void *tail, void **p, int cnt, unsigned long addr) >> > { >> > memcg_slab_free_hook(s, slab, p, cnt); >> > + alloc_tagging_slab_free_hook(s, slab, p, cnt); >> > /* >> > * With KASAN enabled slab_free_freelist_hook modifies the freelist >> > * to remove objects, whose reuse must be delayed. >>
On Fri, Mar 15, 2024 at 7:24 AM Matthew Wilcox <willy@infradead.org> wrote: > > On Wed, Mar 06, 2024 at 10:24:12AM -0800, Suren Baghdasaryan wrote: > > +static inline void pgalloc_tag_add(struct page *page, struct task_struct *task, > > + unsigned int order) > > If you make this "unsigned int nr" instead of order, (a) it won't look > completely insane (what does adding an order even mean?) and (b) you > can reuse it from the __free_pages path. Sounds good to me. > > > @@ -1101,6 +1102,7 @@ __always_inline bool free_pages_prepare(struct page *page, > > /* Do not let hwpoison pages hit pcplists/buddy */ > > reset_page_owner(page, order); > > page_table_check_free(page, order); > > + pgalloc_tag_sub(page, order); > > Obviously you'll need to make sure all the callers now pass in 1 << > order instead of just order. Ack. >
On Fri, Mar 15, 2024 at 3:58 AM Vlastimil Babka <vbabka@suse.cz> wrote: > > On 3/6/24 19:24, Suren Baghdasaryan wrote: > > Account slab allocations using codetag reference embedded into slabobj_ext. > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > > Co-developed-by: Kent Overstreet <kent.overstreet@linux.dev> > > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev> > > Reviewed-by: Kees Cook <keescook@chromium.org> > > Reviewed-by: Vlastimil Babka <vbabka@suse.cz> > > Nit below: > > > @@ -3833,6 +3913,7 @@ void slab_post_alloc_hook(struct kmem_cache *s, struct obj_cgroup *objcg, > > unsigned int orig_size) > > { > > unsigned int zero_size = s->object_size; > > + struct slabobj_ext *obj_exts; > > bool kasan_init = init; > > size_t i; > > gfp_t init_flags = flags & gfp_allowed_mask; > > @@ -3875,6 +3956,12 @@ void slab_post_alloc_hook(struct kmem_cache *s, struct obj_cgroup *objcg, > > kmemleak_alloc_recursive(p[i], s->object_size, 1, > > s->flags, init_flags); > > kmsan_slab_alloc(s, p[i], init_flags); > > + obj_exts = prepare_slab_obj_exts_hook(s, flags, p[i]); > > +#ifdef CONFIG_MEM_ALLOC_PROFILING > > + /* obj_exts can be allocated for other reasons */ > > + if (likely(obj_exts) && mem_alloc_profiling_enabled()) > > + alloc_tag_add(&obj_exts->ref, current->alloc_tag, s->size); > > +#endif > > I think you could still do this a bit better: > > Check mem_alloc_profiling_enabled() once before the whole block calling > prepare_slab_obj_exts_hook() and alloc_tag_add() > Remove need_slab_obj_ext() check from prepare_slab_obj_exts_hook() Agree about checking mem_alloc_profiling_enabled() early and one time, except I would like to use need_slab_obj_ext() instead of mem_alloc_profiling_enabled() for that check. Currently they are equivalent but if there are more slab_obj_ext users in the future then there will be cases when we need to prepare_slab_obj_exts_hook() even when mem_alloc_profiling_enabled()==false. need_slab_obj_ext() will be easy to extend for such cases. Thanks, Suren. > > > } > > > > memcg_slab_post_alloc_hook(s, objcg, flags, size, p); > > @@ -4353,6 +4440,7 @@ void slab_free(struct kmem_cache *s, struct slab *slab, void *object, > > unsigned long addr) > > { > > memcg_slab_free_hook(s, slab, &object, 1); > > + alloc_tagging_slab_free_hook(s, slab, &object, 1); > > > > if (likely(slab_free_hook(s, object, slab_want_init_on_free(s)))) > > do_slab_free(s, slab, object, object, 1, addr); > > @@ -4363,6 +4451,7 @@ void slab_free_bulk(struct kmem_cache *s, struct slab *slab, void *head, > > void *tail, void **p, int cnt, unsigned long addr) > > { > > memcg_slab_free_hook(s, slab, p, cnt); > > + alloc_tagging_slab_free_hook(s, slab, p, cnt); > > /* > > * With KASAN enabled slab_free_freelist_hook modifies the freelist > > * to remove objects, whose reuse must be delayed. >
On Wed, Mar 06, 2024 at 10:24:12AM -0800, Suren Baghdasaryan wrote: > +static inline void pgalloc_tag_add(struct page *page, struct task_struct *task, > + unsigned int order) If you make this "unsigned int nr" instead of order, (a) it won't look completely insane (what does adding an order even mean?) and (b) you can reuse it from the __free_pages path. > @@ -1101,6 +1102,7 @@ __always_inline bool free_pages_prepare(struct page *page, > /* Do not let hwpoison pages hit pcplists/buddy */ > reset_page_owner(page, order); > page_table_check_free(page, order); > + pgalloc_tag_sub(page, order); Obviously you'll need to make sure all the callers now pass in 1 << order instead of just order.
#regzbot title: SHA1 support removal breaks iwd's ability to connect to eduroam #regzbot monitor: https://lore.kernel.org/all/20240313233227.56391-1-ebiggers@kernel.org/ #regzbot monitor: https://lore.kernel.org/all/CZSHRUIJ4RKL.34T4EASV5DNJM@matfyz.cz/ #regzbot link: https://lore.kernel.org/iwd/njvxKaPo_CBxsQGaNSRHj8xOSxzk1_j_K-minIe4GCKUMB1qxJT8nPk9SGmfqg7Aepm_5dO7FEofYIYP1g15R9V5dJ0F8bN6O4VthSjzu1g=@yartys.no/ Sorry for the tracking mess...
On 3/6/24 19:24, Suren Baghdasaryan wrote: > Account slab allocations using codetag reference embedded into slabobj_ext. > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > Co-developed-by: Kent Overstreet <kent.overstreet@linux.dev> > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev> > Reviewed-by: Kees Cook <keescook@chromium.org> Reviewed-by: Vlastimil Babka <vbabka@suse.cz> Nit below: > @@ -3833,6 +3913,7 @@ void slab_post_alloc_hook(struct kmem_cache *s, struct obj_cgroup *objcg, > unsigned int orig_size) > { > unsigned int zero_size = s->object_size; > + struct slabobj_ext *obj_exts; > bool kasan_init = init; > size_t i; > gfp_t init_flags = flags & gfp_allowed_mask; > @@ -3875,6 +3956,12 @@ void slab_post_alloc_hook(struct kmem_cache *s, struct obj_cgroup *objcg, > kmemleak_alloc_recursive(p[i], s->object_size, 1, > s->flags, init_flags); > kmsan_slab_alloc(s, p[i], init_flags); > + obj_exts = prepare_slab_obj_exts_hook(s, flags, p[i]); > +#ifdef CONFIG_MEM_ALLOC_PROFILING > + /* obj_exts can be allocated for other reasons */ > + if (likely(obj_exts) && mem_alloc_profiling_enabled()) > + alloc_tag_add(&obj_exts->ref, current->alloc_tag, s->size); > +#endif I think you could still do this a bit better: Check mem_alloc_profiling_enabled() once before the whole block calling prepare_slab_obj_exts_hook() and alloc_tag_add() Remove need_slab_obj_ext() check from prepare_slab_obj_exts_hook() > } > > memcg_slab_post_alloc_hook(s, objcg, flags, size, p); > @@ -4353,6 +4440,7 @@ void slab_free(struct kmem_cache *s, struct slab *slab, void *object, > unsigned long addr) > { > memcg_slab_free_hook(s, slab, &object, 1); > + alloc_tagging_slab_free_hook(s, slab, &object, 1); > > if (likely(slab_free_hook(s, object, slab_want_init_on_free(s)))) > do_slab_free(s, slab, object, object, 1, addr); > @@ -4363,6 +4451,7 @@ void slab_free_bulk(struct kmem_cache *s, struct slab *slab, void *head, > void *tail, void **p, int cnt, unsigned long addr) > { > memcg_slab_free_hook(s, slab, p, cnt); > + alloc_tagging_slab_free_hook(s, slab, p, cnt); > /* > * With KASAN enabled slab_free_freelist_hook modifies the freelist > * to remove objects, whose reuse must be delayed.
On Thu, 14 Mar 2024 at 21:20, Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Thu, Mar 14, 2024 at 04:52:47AM -0700, James Prestwood wrote:
> > IWD uses AF_ALG/keyctl for _all_ its crypto, cipher, and checksum needs.
> > Anything that wifi requires as far as crypto goes IWD uses the kernel,
> > except ECC is the only exception. The entire list of crypto requirements
> > (for full support at least) for IWD is here:
> >
> > https://git.kernel.org/pub/scm/network/wireless/iwd.git/tree/tools/test_runner_kernel_config
>
> That's quite an extensive list, and it's not documented in the iwd README.
> Don't you get bug reports from users who are running a kernel that's missing one
> of those options?
>
> > For KEYCTL_PKEY_* specifically we use it for all asymmetric crypto
> > operations, (query), encrypt, decrypt, sign, verify.
> >
> > I'll be honest, the AF_ALG/keyctl support in ELL was mostly done by the time
> > I started working on IWD so I was not aware the documentation was so poor.
> > That is an entirely separate issue than this IMO, and I'm happy to help with
> > getting docs updated to include a proper list of supported features. In
> > addition maybe some automated testing that gets run on kernel builds which
> > actually exercises this API so it doesn't get accidentally get broken in the
> > future? Docs/tests IMO are the proper "fix" here, not telling someone to
> > stop using an API that has existed a long time.
>
> I looked into the history, and it seems the KEYCTL_PKEY_* APIs were added as a
> collaboration between the iwd developers and the kernel keyrings maintainer.
> So, as far as I can tell, it's not that the kernel had an existing API that iwd
> started using. It's that iwd got some APIs added to the kernel for themselves.
> KEYCTL_PKEY_* don't seem to have been adopted elsewhere; Debian Code Search
> doesn't return any notable results. keyctl does provide a command-line
> interface to them, but I can't find any users of the keyctl commands either.
>
> Then, everyone disappeared and it got dumped on the next generation of kernel
> developers, who often don't know that this API even exists. And since the API
> is also poorly specified and difficult to maintain (e.g., changing a seemingly
> unrelated part of the kernel can break it), the results are predictable... And
> of course the only thing that breaks is iwd, since it's the only user.
>
> It would be worth taking a step back and looking at the overall system
> architecture here. Is this the best way to ensure a reliable wireless
> experience for Linux users?
>
> Maybe it's time to admit that KEYCTL_PKEY_* was basically an experiment, and a
> different direction (e.g. using OpenSSL) should be taken...
>
> (Another issue with the kernel keyrings stuff is that provides a significant
> attack surface for the kernel to be exploited.)
>
> If you do decide to continue with the status quo, it may be necessary for the
> iwd developers to take a more active role in maintaining this API in order to
> ensure it continues working properly for you.
>
> AF_ALG is on *slightly* firmer ground since it's been around for longer, is
> properly part of the crypto subsystem, and has a few other users. Unfortunately
> it still suffers from the same issues though, just to a slightly lesser degree.
>
We dropped MD4 because there are no users in the kernel. It is not the
kernel's job to run code on behalf of user space if it does not
require any privileges and can therefore execute in user space
directly.
The fact that AF_ALG permits this is a huge oversight on the part of
the kernel community, and a major maintenance burden. The point of
AF_ALG was to expose hardware crypto accelerators (which are shared
resources that /need/ to be managed by the kernel) to user space, and
we inadvertently ended up allowing the kernel's pure-software
algorithms to be used in the same way.
The fact that we even added APIs to the kernel to accommodate iwd is
even worse. It means system call overhead (which has become worse due
to all the speculation mitigations) to execute some code that could
execute in user space just as well, which is a bad idea for other
reasons too (for instance, accelerated crypto that uses SIMD in the
kernel disables preemption on many architectures, resulting in
scheduling jitter)
Note that in the case of iwd, it is unlikely that the use of AF_ALG
could ever result in meaningful use of hardware accelerators: today's
wireless interfaces don't use software crypto for the bulk of the data
(i.e., the packets themselves) and the wireless key exchange protocols
etc are unlikely to be supported in generic crypto accelerators, and
even if they were, the latency would likely result in worse
performance overall than a software implementation.
So iwd's deliberate choice to use the kernel as a crypto library is
severely misguided. I have made the same point 4 years ago when I
replaced iwd's use of the kernel's ecb(arc4) code with a suitable
software implementation (3 files changed, 53 insertions, 40
deletions). Of course, replacing other algorithms will take more work
than that, but it is the only sensible approach. We all know the cat
is out of the bag when it comes to AF_ALG, and we simply have to
retain all those broken algorithms as executable code at the kernel's
privileged execution level, just in case some user space is still
around that relies on it. But that doesn't mean we cannot be very
clear about our preferred way forward.
On Thu, Mar 14, 2024 at 04:52:47AM -0700, James Prestwood wrote: > IWD uses AF_ALG/keyctl for _all_ its crypto, cipher, and checksum needs. > Anything that wifi requires as far as crypto goes IWD uses the kernel, > except ECC is the only exception. The entire list of crypto requirements > (for full support at least) for IWD is here: > > https://git.kernel.org/pub/scm/network/wireless/iwd.git/tree/tools/test_runner_kernel_config That's quite an extensive list, and it's not documented in the iwd README. Don't you get bug reports from users who are running a kernel that's missing one of those options? > For KEYCTL_PKEY_* specifically we use it for all asymmetric crypto > operations, (query), encrypt, decrypt, sign, verify. > > I'll be honest, the AF_ALG/keyctl support in ELL was mostly done by the time > I started working on IWD so I was not aware the documentation was so poor. > That is an entirely separate issue than this IMO, and I'm happy to help with > getting docs updated to include a proper list of supported features. In > addition maybe some automated testing that gets run on kernel builds which > actually exercises this API so it doesn't get accidentally get broken in the > future? Docs/tests IMO are the proper "fix" here, not telling someone to > stop using an API that has existed a long time. I looked into the history, and it seems the KEYCTL_PKEY_* APIs were added as a collaboration between the iwd developers and the kernel keyrings maintainer. So, as far as I can tell, it's not that the kernel had an existing API that iwd started using. It's that iwd got some APIs added to the kernel for themselves. KEYCTL_PKEY_* don't seem to have been adopted elsewhere; Debian Code Search doesn't return any notable results. keyctl does provide a command-line interface to them, but I can't find any users of the keyctl commands either. Then, everyone disappeared and it got dumped on the next generation of kernel developers, who often don't know that this API even exists. And since the API is also poorly specified and difficult to maintain (e.g., changing a seemingly unrelated part of the kernel can break it), the results are predictable... And of course the only thing that breaks is iwd, since it's the only user. It would be worth taking a step back and looking at the overall system architecture here. Is this the best way to ensure a reliable wireless experience for Linux users? Maybe it's time to admit that KEYCTL_PKEY_* was basically an experiment, and a different direction (e.g. using OpenSSL) should be taken... (Another issue with the kernel keyrings stuff is that provides a significant attack surface for the kernel to be exploited.) If you do decide to continue with the status quo, it may be necessary for the iwd developers to take a more active role in maintaining this API in order to ensure it continues working properly for you. AF_ALG is on *slightly* firmer ground since it's been around for longer, is properly part of the crypto subsystem, and has a few other users. Unfortunately it still suffers from the same issues though, just to a slightly lesser degree. > I'm also not entirely sure why this stuff continues to be removed from the > kernel. First MD4, then it got reverted, then this (now reverted, thanks). > Both cases there was not clear justification of why it was being removed. These algorithms are insecure, and it's likely that the author of these commits thought that there were no remaining users and nothing would break. Removing them is a worthy goal for code maintenance purposes and to avoid providing insecure options that could accidentally be used. The AF_ALG and KEYCTL_PKEY_* APIs are very easy to overlook and I suspect that the author of these commits did not know about them. These APIs are rarely used, not well specified, the availability of them and specific algorithms varies by kernel configuration, and userspace only uses a subset of the algorithms in the kernel's museum of crypto primitives anyway. So it's plausible that there are algorithms that no one is using or that at least there is a fallback for, so can be safely removed... - Eric
On Thu, 2024-03-14 at 04:52 -0700, James Prestwood wrote:
> I'm also not entirely sure why this stuff continues to be removed
> from the kernel. First MD4, then it got reverted, then this (now
> reverted, thanks). Both cases there was not clear justification of
> why it was being removed.
I think this is some misunderstanding of the NIST and FIPS requirements
with regards to hashes, ciphers and bits of security. The bottom line
is that neither NIST nor FIPS requires the removal of the sha1
algorithm at all. Both of them still support it for HMAC (for now).
In addition, the FIPS requirement is only that you not *issue* sha1
hashed signatures. FIPS still allows you to verify legacy signatures
with sha1 as the signing hash (for backwards compatibility reasons).
Enterprises with no legacy and no HMAC requirements *may* remove the
hash, but it's not mandated.
So *removing* sha1 from the certificate code was the wrong thing to do.
We should have configurably prevented using sha1 as the algorithm for
new signatures but kept it for signature verification.
Can we please get this sorted out before 2025, because next up is the
FIPS requirement to move to at least 128 bits of security which will
see RSA2048 deprecated in a similar way: We should refuse to issue
RSA2048 signatures, but will still be allowed to verify them for legacy
reasons.
James
Hi, On 3/13/24 4:06 PM, Eric Biggers wrote: > On Wed, Mar 13, 2024 at 03:51:10PM -0700, Jeff Johnson wrote: >> On 3/13/2024 3:10 PM, Eric Biggers wrote: >>> On Wed, Mar 13, 2024 at 02:17:29PM -0700, James Prestwood wrote: >>>> Hi, >>>> >>>> On 3/13/24 1:22 PM, Eric Biggers wrote: >>>>> On Wed, Mar 13, 2024 at 01:12:54PM -0700, James Prestwood wrote: >>>>>> Hi, >>>>>> >>>>>> On 3/13/24 12:44 PM, Eric Biggers wrote: >>>>>>> On Wed, Mar 13, 2024 at 10:26:06AM -0700, James Prestwood wrote: >>>>>>>> Hi, >>>>>>>> >>>>>>>> On 3/13/24 1:56 AM, Johannes Berg wrote: >>>>>>>>> Not sure why you're CC'ing the world, but I guess adding a few more >>>>>>>>> doesn't hurt ... >>>>>>>>> >>>>>>>>> On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote: >>>>>>>>>> and I use iwd >>>>>>>>> This is your problem, the wireless stack in the kernel doesn't use any >>>>>>>>> kernel crypto code for 802.1X. >>>>>>>> Yes, the wireless stack has zero bearing on the issue. I think that's what >>>>>>>> you meant by "problem". >>>>>>>> >>>>>>>> IWD has used the kernel crypto API forever which was abruptly broken, that >>>>>>>> is the problem. >>>>>>>> >>>>>>>> The original commit says it was to remove support for sha1 signed kernel >>>>>>>> modules, but it did more than that and broke the keyctl API. >>>>>>>> >>>>>>> Which specific API is iwd using that is relevant here? >>>>>>> I cloned https://kernel.googlesource.com/pub/scm/network/wireless/iwd >>>>>>> and grepped for keyctl and AF_ALG, but there are no matches. >>>>>> IWD uses ELL for its crypto, which uses the AF_ALG API: >>>>>> >>>>>> https://git.kernel.org/pub/scm/libs/ell/ell.git/ >>>>> Thanks for pointing out that the relevant code is really in that separate >>>>> repository. Note, it seems that keyctl() is the problem here, not AF_ALG. The >>>>> blamed commit didn't change anything for AF_ALG. >>>>> >>>>>> I believe the failure is when calling: >>>>>> >>>>>> KEYCTL_PKEY_QUERY enc="x962" hash="sha1" >>>>>> >>>>>> From logs Michael posted on the IWD list, the ELL API that fails is: >>>>>> >>>>>> l_key_get_info (ell.git/ell/key.c:416) >>>>> Okay, I guess that's what's actually causing the problem. KEYCTL_PKEY_* are a >>>>> weird set of APIs where userspace can ask the kernel to do asymmetric key >>>>> operations. It's unclear why they exist, as the same functionality is available >>>>> in userspace crypto libraries. >>>>> >>>>> I suppose that the blamed commit, or at least part of it, will need to be >>>>> reverted to keep these weird keyctls working. >>>>> >>>>> For the future, why doesn't iwd just use a userspace crypto library such as >>>>> OpenSSL? >>>> I was not around when the original decision was made, but a few reasons I >>>> know we don't use openSSL: >>>> >>>> - IWD has virtually zero dependencies. >>> Depending on something in the kernel does not eliminate a dependency; it just >>> adds that particular kernel UAPI to your list of dependencies. The reason that >>> we're having this discussion in the first place is because iwd is depending on >>> an obscure kernel UAPI that is not well defined. Historically it's been hard to >>> avoid "breaking" changes in these crypto-related UAPIs because of the poor >>> design where a huge number of algorithms are potentially supported, but the list >>> is undocumented and it varies from one system to another based on configuration. >>> Also due to their obscurity many kernel developers don't know that these UAPIs >>> even exist. (The reaction when someone finds out is usually "Why!?") >>> >>> It may be worth looking at if iwd should make a different choice for this >>> dependency. It's understandable to blame dependencies when things go wrong, but >>> at the same time the choice of dependency is very much a choice, and some >>> choices can be more technically sound and cause fewer problems than others... >>> >>>> - OpenSSL + friends are rather large libraries. >>> The Linux kernel is also large, and it's made larger by having to support >>> obsolete crypto algorithms for backwards compatibility with iwd. >>> >>>> - AF_ALG has transparent hardware acceleration (not sure if openSSL does >>>> too). >>> OpenSSL takes advantage of CPU-based hardware acceleration, e.g. AES-NI. >>> >>>> Another consideration is once you support openSSL someone wants wolfSSL, >>>> then boringSSL etc. Even if users implement support it just becomes a huge >>>> burden to carry for the project. Just look at wpa_supplicant's src/crypto/ >>>> folder, nearly 40k LOC in there, compared to ELL's crypto modules which is >>>> ~5k. You have to sort out all the nitty gritty details of each library, and >>>> provide a common driver/API for the core code, differences between openssl >>>> versions, the list goes on. >>> What is the specific functionality that you're actually relying on that you >>> think would need 40K lines of code to replace, even using OpenSSL? I see you >>> are using KEYCTL_PKEY_*, but what specifically are you using them for? What >>> operations are being performed, and with which algorithms and key formats? >>> Also, is the kernel behavior that you're relying on documented anywhere? There >>> are man pages for those keyctls, but they don't say anything about any >>> particular hash algorithm, SHA-1 or otherwise, being supported. >> <https://lore.kernel.org/all/CA+55aFxW7NMAMvYhkvz1UPbUTUJewRt6Yb51QAx5RtrWOwjebg@mail.gmail.com/> >> "And we simply do not break user space." >> -Linus Torvalds >> >> Is this no longer applicable? >> > As I said, the commit, or at least the part of it that broke iwd (it's not clear > that it's the whole commit), needs to be reverted. > > I just hope that, simultaneously, the iwd developers will consider improving the > design of iwd to avoid this type of recurring issue in the future. After all, > this may be the only real chance for such a discussion before the next time iwd > breaks. > > Also, part of the reason I'm asking about what functionality that iwd is relying > on is so that, if necessary, it can be properly documented and supported... > > If we don't know what we are supporting, it is very hard to support it. IWD uses AF_ALG/keyctl for _all_ its crypto, cipher, and checksum needs. Anything that wifi requires as far as crypto goes IWD uses the kernel, except ECC is the only exception. The entire list of crypto requirements (for full support at least) for IWD is here: https://git.kernel.org/pub/scm/network/wireless/iwd.git/tree/tools/test_runner_kernel_config For KEYCTL_PKEY_* specifically we use it for all asymmetric crypto operations, (query), encrypt, decrypt, sign, verify. I'll be honest, the AF_ALG/keyctl support in ELL was mostly done by the time I started working on IWD so I was not aware the documentation was so poor. That is an entirely separate issue than this IMO, and I'm happy to help with getting docs updated to include a proper list of supported features. In addition maybe some automated testing that gets run on kernel builds which actually exercises this API so it doesn't get accidentally get broken in the future? Docs/tests IMO are the proper "fix" here, not telling someone to stop using an API that has existed a long time. I'm also not entirely sure why this stuff continues to be removed from the kernel. First MD4, then it got reverted, then this (now reverted, thanks). Both cases there was not clear justification of why it was being removed. Thanks, James > > - Eric >
On Wed, Mar 13, 2024 at 04:06:11PM -0700, Eric Biggers wrote: > On Wed, Mar 13, 2024 at 03:51:10PM -0700, Jeff Johnson wrote: > > On 3/13/2024 3:10 PM, Eric Biggers wrote: > > > On Wed, Mar 13, 2024 at 02:17:29PM -0700, James Prestwood wrote: > > >> Hi, > > >> > > >> On 3/13/24 1:22 PM, Eric Biggers wrote: > > >>> On Wed, Mar 13, 2024 at 01:12:54PM -0700, James Prestwood wrote: > > >>>> Hi, > > >>>> > > >>>> On 3/13/24 12:44 PM, Eric Biggers wrote: > > >>>>> On Wed, Mar 13, 2024 at 10:26:06AM -0700, James Prestwood wrote: > > >>>>>> Hi, > > >>>>>> > > >>>>>> On 3/13/24 1:56 AM, Johannes Berg wrote: > > >>>>>>> Not sure why you're CC'ing the world, but I guess adding a few more > > >>>>>>> doesn't hurt ... > > >>>>>>> > > >>>>>>> On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote: > > >>>>>>>> and I use iwd > > >>>>>>> This is your problem, the wireless stack in the kernel doesn't use any > > >>>>>>> kernel crypto code for 802.1X. > > >>>>>> Yes, the wireless stack has zero bearing on the issue. I think that's what > > >>>>>> you meant by "problem". > > >>>>>> > > >>>>>> IWD has used the kernel crypto API forever which was abruptly broken, that > > >>>>>> is the problem. > > >>>>>> > > >>>>>> The original commit says it was to remove support for sha1 signed kernel > > >>>>>> modules, but it did more than that and broke the keyctl API. > > >>>>>> > > >>>>> Which specific API is iwd using that is relevant here? > > >>>>> I cloned https://kernel.googlesource.com/pub/scm/network/wireless/iwd > > >>>>> and grepped for keyctl and AF_ALG, but there are no matches. > > >>>> IWD uses ELL for its crypto, which uses the AF_ALG API: > > >>>> > > >>>> https://git.kernel.org/pub/scm/libs/ell/ell.git/ > > >>> Thanks for pointing out that the relevant code is really in that separate > > >>> repository. Note, it seems that keyctl() is the problem here, not AF_ALG. The > > >>> blamed commit didn't change anything for AF_ALG. > > >>> > > >>>> I believe the failure is when calling: > > >>>> > > >>>> KEYCTL_PKEY_QUERY enc="x962" hash="sha1" > > >>>> > > >>>> From logs Michael posted on the IWD list, the ELL API that fails is: > > >>>> > > >>>> l_key_get_info (ell.git/ell/key.c:416) > > >>> Okay, I guess that's what's actually causing the problem. KEYCTL_PKEY_* are a > > >>> weird set of APIs where userspace can ask the kernel to do asymmetric key > > >>> operations. It's unclear why they exist, as the same functionality is available > > >>> in userspace crypto libraries. > > >>> > > >>> I suppose that the blamed commit, or at least part of it, will need to be > > >>> reverted to keep these weird keyctls working. > > >>> > > >>> For the future, why doesn't iwd just use a userspace crypto library such as > > >>> OpenSSL? > > >> > > >> I was not around when the original decision was made, but a few reasons I > > >> know we don't use openSSL: > > >> > > >> - IWD has virtually zero dependencies. > > > > > > Depending on something in the kernel does not eliminate a dependency; it just > > > adds that particular kernel UAPI to your list of dependencies. The reason that > > > we're having this discussion in the first place is because iwd is depending on > > > an obscure kernel UAPI that is not well defined. Historically it's been hard to > > > avoid "breaking" changes in these crypto-related UAPIs because of the poor > > > design where a huge number of algorithms are potentially supported, but the list > > > is undocumented and it varies from one system to another based on configuration. > > > Also due to their obscurity many kernel developers don't know that these UAPIs > > > even exist. (The reaction when someone finds out is usually "Why!?") > > > > > > It may be worth looking at if iwd should make a different choice for this > > > dependency. It's understandable to blame dependencies when things go wrong, but > > > at the same time the choice of dependency is very much a choice, and some > > > choices can be more technically sound and cause fewer problems than others... > > > > > >> - OpenSSL + friends are rather large libraries. > > > > > > The Linux kernel is also large, and it's made larger by having to support > > > obsolete crypto algorithms for backwards compatibility with iwd. > > > > > >> - AF_ALG has transparent hardware acceleration (not sure if openSSL does > > >> too). > > > > > > OpenSSL takes advantage of CPU-based hardware acceleration, e.g. AES-NI. > > > > > >> Another consideration is once you support openSSL someone wants wolfSSL, > > >> then boringSSL etc. Even if users implement support it just becomes a huge > > >> burden to carry for the project. Just look at wpa_supplicant's src/crypto/ > > >> folder, nearly 40k LOC in there, compared to ELL's crypto modules which is > > >> ~5k. You have to sort out all the nitty gritty details of each library, and > > >> provide a common driver/API for the core code, differences between openssl > > >> versions, the list goes on. > > > > > > What is the specific functionality that you're actually relying on that you > > > think would need 40K lines of code to replace, even using OpenSSL? I see you > > > are using KEYCTL_PKEY_*, but what specifically are you using them for? What > > > operations are being performed, and with which algorithms and key formats? > > > Also, is the kernel behavior that you're relying on documented anywhere? There > > > are man pages for those keyctls, but they don't say anything about any > > > particular hash algorithm, SHA-1 or otherwise, being supported. > > > > <https://lore.kernel.org/all/CA+55aFxW7NMAMvYhkvz1UPbUTUJewRt6Yb51QAx5RtrWOwjebg@mail.gmail.com/> > > "And we simply do not break user space." > > -Linus Torvalds > > > > Is this no longer applicable? > > > > As I said, the commit, or at least the part of it that broke iwd (it's not clear > that it's the whole commit), needs to be reverted. > > I just hope that, simultaneously, the iwd developers will consider improving the > design of iwd to avoid this type of recurring issue in the future. After all, > this may be the only real chance for such a discussion before the next time iwd > breaks. > > Also, part of the reason I'm asking about what functionality that iwd is relying > on is so that, if necessary, it can be properly documented and supported... > > If we don't know what we are supporting, it is very hard to support it. I ended up just sending out a patch that does a full revert: https://lore.kernel.org/linux-crypto/20240313233227.56391-1-ebiggers@kernel.org Again, regardless of that, these issues with iwd have been recurring, and it is still worth discussing the best way from a technical perspective to prevent them from happening in the future... There's a fairly clear path to achieve that. - Eric
On Wed, Mar 13, 2024 at 03:51:10PM -0700, Jeff Johnson wrote:
> On 3/13/2024 3:10 PM, Eric Biggers wrote:
> > On Wed, Mar 13, 2024 at 02:17:29PM -0700, James Prestwood wrote:
> >> Hi,
> >>
> >> On 3/13/24 1:22 PM, Eric Biggers wrote:
> >>> On Wed, Mar 13, 2024 at 01:12:54PM -0700, James Prestwood wrote:
> >>>> Hi,
> >>>>
> >>>> On 3/13/24 12:44 PM, Eric Biggers wrote:
> >>>>> On Wed, Mar 13, 2024 at 10:26:06AM -0700, James Prestwood wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> On 3/13/24 1:56 AM, Johannes Berg wrote:
> >>>>>>> Not sure why you're CC'ing the world, but I guess adding a few more
> >>>>>>> doesn't hurt ...
> >>>>>>>
> >>>>>>> On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote:
> >>>>>>>> and I use iwd
> >>>>>>> This is your problem, the wireless stack in the kernel doesn't use any
> >>>>>>> kernel crypto code for 802.1X.
> >>>>>> Yes, the wireless stack has zero bearing on the issue. I think that's what
> >>>>>> you meant by "problem".
> >>>>>>
> >>>>>> IWD has used the kernel crypto API forever which was abruptly broken, that
> >>>>>> is the problem.
> >>>>>>
> >>>>>> The original commit says it was to remove support for sha1 signed kernel
> >>>>>> modules, but it did more than that and broke the keyctl API.
> >>>>>>
> >>>>> Which specific API is iwd using that is relevant here?
> >>>>> I cloned https://kernel.googlesource.com/pub/scm/network/wireless/iwd
> >>>>> and grepped for keyctl and AF_ALG, but there are no matches.
> >>>> IWD uses ELL for its crypto, which uses the AF_ALG API:
> >>>>
> >>>> https://git.kernel.org/pub/scm/libs/ell/ell.git/
> >>> Thanks for pointing out that the relevant code is really in that separate
> >>> repository. Note, it seems that keyctl() is the problem here, not AF_ALG. The
> >>> blamed commit didn't change anything for AF_ALG.
> >>>
> >>>> I believe the failure is when calling:
> >>>>
> >>>> KEYCTL_PKEY_QUERY enc="x962" hash="sha1"
> >>>>
> >>>> From logs Michael posted on the IWD list, the ELL API that fails is:
> >>>>
> >>>> l_key_get_info (ell.git/ell/key.c:416)
> >>> Okay, I guess that's what's actually causing the problem. KEYCTL_PKEY_* are a
> >>> weird set of APIs where userspace can ask the kernel to do asymmetric key
> >>> operations. It's unclear why they exist, as the same functionality is available
> >>> in userspace crypto libraries.
> >>>
> >>> I suppose that the blamed commit, or at least part of it, will need to be
> >>> reverted to keep these weird keyctls working.
> >>>
> >>> For the future, why doesn't iwd just use a userspace crypto library such as
> >>> OpenSSL?
> >>
> >> I was not around when the original decision was made, but a few reasons I
> >> know we don't use openSSL:
> >>
> >> - IWD has virtually zero dependencies.
> >
> > Depending on something in the kernel does not eliminate a dependency; it just
> > adds that particular kernel UAPI to your list of dependencies. The reason that
> > we're having this discussion in the first place is because iwd is depending on
> > an obscure kernel UAPI that is not well defined. Historically it's been hard to
> > avoid "breaking" changes in these crypto-related UAPIs because of the poor
> > design where a huge number of algorithms are potentially supported, but the list
> > is undocumented and it varies from one system to another based on configuration.
> > Also due to their obscurity many kernel developers don't know that these UAPIs
> > even exist. (The reaction when someone finds out is usually "Why!?")
> >
> > It may be worth looking at if iwd should make a different choice for this
> > dependency. It's understandable to blame dependencies when things go wrong, but
> > at the same time the choice of dependency is very much a choice, and some
> > choices can be more technically sound and cause fewer problems than others...
> >
> >> - OpenSSL + friends are rather large libraries.
> >
> > The Linux kernel is also large, and it's made larger by having to support
> > obsolete crypto algorithms for backwards compatibility with iwd.
> >
> >> - AF_ALG has transparent hardware acceleration (not sure if openSSL does
> >> too).
> >
> > OpenSSL takes advantage of CPU-based hardware acceleration, e.g. AES-NI.
> >
> >> Another consideration is once you support openSSL someone wants wolfSSL,
> >> then boringSSL etc. Even if users implement support it just becomes a huge
> >> burden to carry for the project. Just look at wpa_supplicant's src/crypto/
> >> folder, nearly 40k LOC in there, compared to ELL's crypto modules which is
> >> ~5k. You have to sort out all the nitty gritty details of each library, and
> >> provide a common driver/API for the core code, differences between openssl
> >> versions, the list goes on.
> >
> > What is the specific functionality that you're actually relying on that you
> > think would need 40K lines of code to replace, even using OpenSSL? I see you
> > are using KEYCTL_PKEY_*, but what specifically are you using them for? What
> > operations are being performed, and with which algorithms and key formats?
> > Also, is the kernel behavior that you're relying on documented anywhere? There
> > are man pages for those keyctls, but they don't say anything about any
> > particular hash algorithm, SHA-1 or otherwise, being supported.
>
> <https://lore.kernel.org/all/CA+55aFxW7NMAMvYhkvz1UPbUTUJewRt6Yb51QAx5RtrWOwjebg@mail.gmail.com/>
> "And we simply do not break user space."
> -Linus Torvalds
>
> Is this no longer applicable?
>
As I said, the commit, or at least the part of it that broke iwd (it's not clear
that it's the whole commit), needs to be reverted.
I just hope that, simultaneously, the iwd developers will consider improving the
design of iwd to avoid this type of recurring issue in the future. After all,
this may be the only real chance for such a discussion before the next time iwd
breaks.
Also, part of the reason I'm asking about what functionality that iwd is relying
on is so that, if necessary, it can be properly documented and supported...
If we don't know what we are supporting, it is very hard to support it.
- Eric
On 3/13/2024 3:10 PM, Eric Biggers wrote: > On Wed, Mar 13, 2024 at 02:17:29PM -0700, James Prestwood wrote: >> Hi, >> >> On 3/13/24 1:22 PM, Eric Biggers wrote: >>> On Wed, Mar 13, 2024 at 01:12:54PM -0700, James Prestwood wrote: >>>> Hi, >>>> >>>> On 3/13/24 12:44 PM, Eric Biggers wrote: >>>>> On Wed, Mar 13, 2024 at 10:26:06AM -0700, James Prestwood wrote: >>>>>> Hi, >>>>>> >>>>>> On 3/13/24 1:56 AM, Johannes Berg wrote: >>>>>>> Not sure why you're CC'ing the world, but I guess adding a few more >>>>>>> doesn't hurt ... >>>>>>> >>>>>>> On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote: >>>>>>>> and I use iwd >>>>>>> This is your problem, the wireless stack in the kernel doesn't use any >>>>>>> kernel crypto code for 802.1X. >>>>>> Yes, the wireless stack has zero bearing on the issue. I think that's what >>>>>> you meant by "problem". >>>>>> >>>>>> IWD has used the kernel crypto API forever which was abruptly broken, that >>>>>> is the problem. >>>>>> >>>>>> The original commit says it was to remove support for sha1 signed kernel >>>>>> modules, but it did more than that and broke the keyctl API. >>>>>> >>>>> Which specific API is iwd using that is relevant here? >>>>> I cloned https://kernel.googlesource.com/pub/scm/network/wireless/iwd >>>>> and grepped for keyctl and AF_ALG, but there are no matches. >>>> IWD uses ELL for its crypto, which uses the AF_ALG API: >>>> >>>> https://git.kernel.org/pub/scm/libs/ell/ell.git/ >>> Thanks for pointing out that the relevant code is really in that separate >>> repository. Note, it seems that keyctl() is the problem here, not AF_ALG. The >>> blamed commit didn't change anything for AF_ALG. >>> >>>> I believe the failure is when calling: >>>> >>>> KEYCTL_PKEY_QUERY enc="x962" hash="sha1" >>>> >>>> From logs Michael posted on the IWD list, the ELL API that fails is: >>>> >>>> l_key_get_info (ell.git/ell/key.c:416) >>> Okay, I guess that's what's actually causing the problem. KEYCTL_PKEY_* are a >>> weird set of APIs where userspace can ask the kernel to do asymmetric key >>> operations. It's unclear why they exist, as the same functionality is available >>> in userspace crypto libraries. >>> >>> I suppose that the blamed commit, or at least part of it, will need to be >>> reverted to keep these weird keyctls working. >>> >>> For the future, why doesn't iwd just use a userspace crypto library such as >>> OpenSSL? >> >> I was not around when the original decision was made, but a few reasons I >> know we don't use openSSL: >> >> - IWD has virtually zero dependencies. > > Depending on something in the kernel does not eliminate a dependency; it just > adds that particular kernel UAPI to your list of dependencies. The reason that > we're having this discussion in the first place is because iwd is depending on > an obscure kernel UAPI that is not well defined. Historically it's been hard to > avoid "breaking" changes in these crypto-related UAPIs because of the poor > design where a huge number of algorithms are potentially supported, but the list > is undocumented and it varies from one system to another based on configuration. > Also due to their obscurity many kernel developers don't know that these UAPIs > even exist. (The reaction when someone finds out is usually "Why!?") > > It may be worth looking at if iwd should make a different choice for this > dependency. It's understandable to blame dependencies when things go wrong, but > at the same time the choice of dependency is very much a choice, and some > choices can be more technically sound and cause fewer problems than others... > >> - OpenSSL + friends are rather large libraries. > > The Linux kernel is also large, and it's made larger by having to support > obsolete crypto algorithms for backwards compatibility with iwd. > >> - AF_ALG has transparent hardware acceleration (not sure if openSSL does >> too). > > OpenSSL takes advantage of CPU-based hardware acceleration, e.g. AES-NI. > >> Another consideration is once you support openSSL someone wants wolfSSL, >> then boringSSL etc. Even if users implement support it just becomes a huge >> burden to carry for the project. Just look at wpa_supplicant's src/crypto/ >> folder, nearly 40k LOC in there, compared to ELL's crypto modules which is >> ~5k. You have to sort out all the nitty gritty details of each library, and >> provide a common driver/API for the core code, differences between openssl >> versions, the list goes on. > > What is the specific functionality that you're actually relying on that you > think would need 40K lines of code to replace, even using OpenSSL? I see you > are using KEYCTL_PKEY_*, but what specifically are you using them for? What > operations are being performed, and with which algorithms and key formats? > Also, is the kernel behavior that you're relying on documented anywhere? There > are man pages for those keyctls, but they don't say anything about any > particular hash algorithm, SHA-1 or otherwise, being supported. <https://lore.kernel.org/all/CA+55aFxW7NMAMvYhkvz1UPbUTUJewRt6Yb51QAx5RtrWOwjebg@mail.gmail.com/> "And we simply do not break user space." -Linus Torvalds Is this no longer applicable?
On Wed, Mar 13, 2024 at 02:17:29PM -0700, James Prestwood wrote: > Hi, > > On 3/13/24 1:22 PM, Eric Biggers wrote: > > On Wed, Mar 13, 2024 at 01:12:54PM -0700, James Prestwood wrote: > > > Hi, > > > > > > On 3/13/24 12:44 PM, Eric Biggers wrote: > > > > On Wed, Mar 13, 2024 at 10:26:06AM -0700, James Prestwood wrote: > > > > > Hi, > > > > > > > > > > On 3/13/24 1:56 AM, Johannes Berg wrote: > > > > > > Not sure why you're CC'ing the world, but I guess adding a few more > > > > > > doesn't hurt ... > > > > > > > > > > > > On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote: > > > > > > > and I use iwd > > > > > > This is your problem, the wireless stack in the kernel doesn't use any > > > > > > kernel crypto code for 802.1X. > > > > > Yes, the wireless stack has zero bearing on the issue. I think that's what > > > > > you meant by "problem". > > > > > > > > > > IWD has used the kernel crypto API forever which was abruptly broken, that > > > > > is the problem. > > > > > > > > > > The original commit says it was to remove support for sha1 signed kernel > > > > > modules, but it did more than that and broke the keyctl API. > > > > > > > > > Which specific API is iwd using that is relevant here? > > > > I cloned https://kernel.googlesource.com/pub/scm/network/wireless/iwd > > > > and grepped for keyctl and AF_ALG, but there are no matches. > > > IWD uses ELL for its crypto, which uses the AF_ALG API: > > > > > > https://git.kernel.org/pub/scm/libs/ell/ell.git/ > > Thanks for pointing out that the relevant code is really in that separate > > repository. Note, it seems that keyctl() is the problem here, not AF_ALG. The > > blamed commit didn't change anything for AF_ALG. > > > > > I believe the failure is when calling: > > > > > > KEYCTL_PKEY_QUERY enc="x962" hash="sha1" > > > > > > From logs Michael posted on the IWD list, the ELL API that fails is: > > > > > > l_key_get_info (ell.git/ell/key.c:416) > > Okay, I guess that's what's actually causing the problem. KEYCTL_PKEY_* are a > > weird set of APIs where userspace can ask the kernel to do asymmetric key > > operations. It's unclear why they exist, as the same functionality is available > > in userspace crypto libraries. > > > > I suppose that the blamed commit, or at least part of it, will need to be > > reverted to keep these weird keyctls working. > > > > For the future, why doesn't iwd just use a userspace crypto library such as > > OpenSSL? > > I was not around when the original decision was made, but a few reasons I > know we don't use openSSL: > > - IWD has virtually zero dependencies. Depending on something in the kernel does not eliminate a dependency; it just adds that particular kernel UAPI to your list of dependencies. The reason that we're having this discussion in the first place is because iwd is depending on an obscure kernel UAPI that is not well defined. Historically it's been hard to avoid "breaking" changes in these crypto-related UAPIs because of the poor design where a huge number of algorithms are potentially supported, but the list is undocumented and it varies from one system to another based on configuration. Also due to their obscurity many kernel developers don't know that these UAPIs even exist. (The reaction when someone finds out is usually "Why!?") It may be worth looking at if iwd should make a different choice for this dependency. It's understandable to blame dependencies when things go wrong, but at the same time the choice of dependency is very much a choice, and some choices can be more technically sound and cause fewer problems than others... > - OpenSSL + friends are rather large libraries. The Linux kernel is also large, and it's made larger by having to support obsolete crypto algorithms for backwards compatibility with iwd. > - AF_ALG has transparent hardware acceleration (not sure if openSSL does > too). OpenSSL takes advantage of CPU-based hardware acceleration, e.g. AES-NI. > Another consideration is once you support openSSL someone wants wolfSSL, > then boringSSL etc. Even if users implement support it just becomes a huge > burden to carry for the project. Just look at wpa_supplicant's src/crypto/ > folder, nearly 40k LOC in there, compared to ELL's crypto modules which is > ~5k. You have to sort out all the nitty gritty details of each library, and > provide a common driver/API for the core code, differences between openssl > versions, the list goes on. What is the specific functionality that you're actually relying on that you think would need 40K lines of code to replace, even using OpenSSL? I see you are using KEYCTL_PKEY_*, but what specifically are you using them for? What operations are being performed, and with which algorithms and key formats? Also, is the kernel behavior that you're relying on documented anywhere? There are man pages for those keyctls, but they don't say anything about any particular hash algorithm, SHA-1 or otherwise, being supported. - Eric
The pull request you sent on Tue, 12 Mar 2024 18:38:32 -0700: > git://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/ tags/modules-6.9-rc1 has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/ce0c1c92656e3ea3840c4a5c748aa352285cae9c Thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/prtracker.html
Hi, On 3/13/24 1:22 PM, Eric Biggers wrote: > On Wed, Mar 13, 2024 at 01:12:54PM -0700, James Prestwood wrote: >> Hi, >> >> On 3/13/24 12:44 PM, Eric Biggers wrote: >>> On Wed, Mar 13, 2024 at 10:26:06AM -0700, James Prestwood wrote: >>>> Hi, >>>> >>>> On 3/13/24 1:56 AM, Johannes Berg wrote: >>>>> Not sure why you're CC'ing the world, but I guess adding a few more >>>>> doesn't hurt ... >>>>> >>>>> On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote: >>>>>> and I use iwd >>>>> This is your problem, the wireless stack in the kernel doesn't use any >>>>> kernel crypto code for 802.1X. >>>> Yes, the wireless stack has zero bearing on the issue. I think that's what >>>> you meant by "problem". >>>> >>>> IWD has used the kernel crypto API forever which was abruptly broken, that >>>> is the problem. >>>> >>>> The original commit says it was to remove support for sha1 signed kernel >>>> modules, but it did more than that and broke the keyctl API. >>>> >>> Which specific API is iwd using that is relevant here? >>> I cloned https://kernel.googlesource.com/pub/scm/network/wireless/iwd >>> and grepped for keyctl and AF_ALG, but there are no matches. >> IWD uses ELL for its crypto, which uses the AF_ALG API: >> >> https://git.kernel.org/pub/scm/libs/ell/ell.git/ > Thanks for pointing out that the relevant code is really in that separate > repository. Note, it seems that keyctl() is the problem here, not AF_ALG. The > blamed commit didn't change anything for AF_ALG. > >> I believe the failure is when calling: >> >> KEYCTL_PKEY_QUERY enc="x962" hash="sha1" >> >> From logs Michael posted on the IWD list, the ELL API that fails is: >> >> l_key_get_info (ell.git/ell/key.c:416) > Okay, I guess that's what's actually causing the problem. KEYCTL_PKEY_* are a > weird set of APIs where userspace can ask the kernel to do asymmetric key > operations. It's unclear why they exist, as the same functionality is available > in userspace crypto libraries. > > I suppose that the blamed commit, or at least part of it, will need to be > reverted to keep these weird keyctls working. > > For the future, why doesn't iwd just use a userspace crypto library such as > OpenSSL? I was not around when the original decision was made, but a few reasons I know we don't use openSSL: - IWD has virtually zero dependencies. - OpenSSL + friends are rather large libraries. - AF_ALG has transparent hardware acceleration (not sure if openSSL does too). Another consideration is once you support openSSL someone wants wolfSSL, then boringSSL etc. Even if users implement support it just becomes a huge burden to carry for the project. Just look at wpa_supplicant's src/crypto/ folder, nearly 40k LOC in there, compared to ELL's crypto modules which is ~5k. You have to sort out all the nitty gritty details of each library, and provide a common driver/API for the core code, differences between openssl versions, the list goes on. Thanks, James > > - Eric
On Wed, Mar 13, 2024 at 01:12:54PM -0700, James Prestwood wrote: > Hi, > > On 3/13/24 12:44 PM, Eric Biggers wrote: > > On Wed, Mar 13, 2024 at 10:26:06AM -0700, James Prestwood wrote: > > > Hi, > > > > > > On 3/13/24 1:56 AM, Johannes Berg wrote: > > > > Not sure why you're CC'ing the world, but I guess adding a few more > > > > doesn't hurt ... > > > > > > > > On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote: > > > > > and I use iwd > > > > This is your problem, the wireless stack in the kernel doesn't use any > > > > kernel crypto code for 802.1X. > > > Yes, the wireless stack has zero bearing on the issue. I think that's what > > > you meant by "problem". > > > > > > IWD has used the kernel crypto API forever which was abruptly broken, that > > > is the problem. > > > > > > The original commit says it was to remove support for sha1 signed kernel > > > modules, but it did more than that and broke the keyctl API. > > > > > Which specific API is iwd using that is relevant here? > > I cloned https://kernel.googlesource.com/pub/scm/network/wireless/iwd > > and grepped for keyctl and AF_ALG, but there are no matches. > > IWD uses ELL for its crypto, which uses the AF_ALG API: > > https://git.kernel.org/pub/scm/libs/ell/ell.git/ Thanks for pointing out that the relevant code is really in that separate repository. Note, it seems that keyctl() is the problem here, not AF_ALG. The blamed commit didn't change anything for AF_ALG. > I believe the failure is when calling: > > KEYCTL_PKEY_QUERY enc="x962" hash="sha1" > > From logs Michael posted on the IWD list, the ELL API that fails is: > > l_key_get_info (ell.git/ell/key.c:416) Okay, I guess that's what's actually causing the problem. KEYCTL_PKEY_* are a weird set of APIs where userspace can ask the kernel to do asymmetric key operations. It's unclear why they exist, as the same functionality is available in userspace crypto libraries. I suppose that the blamed commit, or at least part of it, will need to be reverted to keep these weird keyctls working. For the future, why doesn't iwd just use a userspace crypto library such as OpenSSL? - Eric
Hi, On 3/13/24 12:44 PM, Eric Biggers wrote: > On Wed, Mar 13, 2024 at 10:26:06AM -0700, James Prestwood wrote: >> Hi, >> >> On 3/13/24 1:56 AM, Johannes Berg wrote: >>> Not sure why you're CC'ing the world, but I guess adding a few more >>> doesn't hurt ... >>> >>> On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote: >>>> and I use iwd >>> This is your problem, the wireless stack in the kernel doesn't use any >>> kernel crypto code for 802.1X. >> Yes, the wireless stack has zero bearing on the issue. I think that's what >> you meant by "problem". >> >> IWD has used the kernel crypto API forever which was abruptly broken, that >> is the problem. >> >> The original commit says it was to remove support for sha1 signed kernel >> modules, but it did more than that and broke the keyctl API. >> > Which specific API is iwd using that is relevant here? > I cloned https://kernel.googlesource.com/pub/scm/network/wireless/iwd > and grepped for keyctl and AF_ALG, but there are no matches. IWD uses ELL for its crypto, which uses the AF_ALG API: https://git.kernel.org/pub/scm/libs/ell/ell.git/ I believe the failure is when calling: KEYCTL_PKEY_QUERY enc="x962" hash="sha1" From logs Michael posted on the IWD list, the ELL API that fails is: l_key_get_info (ell.git/ell/key.c:416) Thanks, James > > - Eric
Thank you all for your feedback so far. Since it seems that this really is a regression on the kernel side, let me add the appropriate list to Cc and tag this: #regzbot introduced: 16ab7cb5825f Best regards, K. B.
On Wed, Mar 13, 2024 at 10:26:06AM -0700, James Prestwood wrote: > Hi, > > On 3/13/24 1:56 AM, Johannes Berg wrote: > > Not sure why you're CC'ing the world, but I guess adding a few more > > doesn't hurt ... > > > > On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote: > > > and I use iwd > > This is your problem, the wireless stack in the kernel doesn't use any > > kernel crypto code for 802.1X. > > Yes, the wireless stack has zero bearing on the issue. I think that's what > you meant by "problem". > > IWD has used the kernel crypto API forever which was abruptly broken, that > is the problem. > > The original commit says it was to remove support for sha1 signed kernel > modules, but it did more than that and broke the keyctl API. > Which specific API is iwd using that is relevant here? I cloned https://kernel.googlesource.com/pub/scm/network/wireless/iwd and grepped for keyctl and AF_ALG, but there are no matches. - Eric
Hi
This came in via the iwd mailing list, and I would like to add some small a detail as I also experience this issue on my university eduroam network. I've verified that the certificate chain doesn't contain SHA-1 signed certificates, so the update breaks more than just SHA-1.
Michael
On Wednesday, March 13th, 2024 at 09:56, Johannes Berg <johannes@sipsolutions.net> wrote:
>
>
> Not sure why you're CC'ing the world, but I guess adding a few more
> doesn't hurt ...
>
> On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote:
>
> > and I use iwd
>
>
> This is your problem, the wireless stack in the kernel doesn't use any
> kernel crypto code for 802.1X.
>
> I suppose iwd wants to use the kernel infrastructure but has no
> fallbacks to other implementations.
>
> johannes
Hi, On 3/13/24 1:56 AM, Johannes Berg wrote: > Not sure why you're CC'ing the world, but I guess adding a few more > doesn't hurt ... > > On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote: >> and I use iwd > This is your problem, the wireless stack in the kernel doesn't use any > kernel crypto code for 802.1X. Yes, the wireless stack has zero bearing on the issue. I think that's what you meant by "problem". IWD has used the kernel crypto API forever which was abruptly broken, that is the problem. The original commit says it was to remove support for sha1 signed kernel modules, but it did more than that and broke the keyctl API. > > I suppose iwd wants to use the kernel infrastructure but has no > fallbacks to other implementations. > johannes >
On Wed, Mar 13, 2024 at 3:04 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Wed, Mar 06, 2024 at 10:24:18AM -0800, Suren Baghdasaryan wrote: > > When a non-compound multi-order page is freed, it is possible that a > > speculative reference keeps the page pinned. In this case we free all > > pages except for the first page, which will be freed later by the last > > put_page(). However put_page() ignores the order of the page being freed, > > treating it as a 0-order page. This creates a memory accounting imbalance > > because the pages freed in __free_pages() do not have their own alloc_tag > > and their memory was accounted to the first page. To fix this the first > > page should adjust its allocation size counter when "tail" pages are freed. > > It's not "ignored". It's not available! > > Better wording: > > However the page passed to put_page() is indisinguishable from an > order-0 page, so it cannot do the accounting, just as it cannot free > the subsequent pages. Do the accounting here, where we free the pages. > > (I'm sure further improvements are possible) > > > +static inline void pgalloc_tag_sub_bytes(struct alloc_tag *tag, unsigned int order) > > +{ > > + if (mem_alloc_profiling_enabled() && tag) > > + this_cpu_sub(tag->counters->bytes, PAGE_SIZE << order); > > +} > > This is a terribly named function. And it's not even good for what we > want to use it for. > > static inline void pgalloc_tag_sub_pages(struct alloc_tag *tag, unsigned int nr) > { > if (mem_alloc_profiling_enabled() && tag) > this_cpu_sub(tag->counters->bytes, PAGE_SIZE * nr); > } > > > +++ b/mm/page_alloc.c > > @@ -4697,12 +4697,21 @@ void __free_pages(struct page *page, unsigned int order) > > { > > /* get PageHead before we drop reference */ > > int head = PageHead(page); > > + struct alloc_tag *tag = pgalloc_tag_get(page); > > > > if (put_page_testzero(page)) > > free_the_page(page, order); > > else if (!head) > > - while (order-- > 0) > > + while (order-- > 0) { > > free_the_page(page + (1 << order), order); > > + /* > > + * non-compound multi-order page accounts all allocations > > + * to the first page (just like compound one), therefore > > + * we need to adjust the allocation size of the first > > + * page as its order is ignored when put_page() frees it. > > + */ > > + pgalloc_tag_sub_bytes(tag, order); > > - else if (!head > + else if (!head) { > + pgalloc_tag_sub_pages(1 << order - 1); > while (order-- > 0) > free_the_page(page + (1 << order), order); > + } > > It doesn't need a comment, it's obvious what you're doing. All suggestions seem fine to me. I'll adjust the next version accordingly. Thanks for reviewing and the feedback! >