| Message ID | 20241101000017.3856204-1-surenb@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] alloc_tag: fix empty codetag module section handling | expand |
On Thu, Oct 31, 2024 at 5:00 PM Suren Baghdasaryan <surenb@google.com> wrote: > > When a module does not have any allocations, it's allocation tag section > is empty and codetag_alloc_module_section() returns NULL. However this > condition should never happen because codetag_needs_module_section() will > detect an empty section and avoid calling codetag_alloc_module_section(). > Change codetag_alloc_module_section() to never return NULL, which should > prevent static checker warnings. Add a WARN_ON() and a proper error > reporting in case codetag_alloc_module_section() returns NULL, to prevent > future codetag type implementations from returning NULL from their > cttype->desc.alloc_section_mem() operation. > > Fixes: 61c9e58f3a10 ("alloc_tag: load module tags into separate contiguous memory") > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > Closes: https://lore.kernel.org/all/50f12fa1-17c1-4940-a6bf-beaf61f6b17a@stanley.mountain/ > Signed-off-by: Suren Baghdasaryan <surenb@google.com> Andrew, I was going to respin v5 of my patchset and include all these small fixes in it but it's a bit tricky because I would have to revert another unrelated patch [1] from mm-unstable which refactors relevant code. So far the fixes are rather small, so I think you should not have much trouble folding them into the original patchset when the time comes, but if that becomes a problem I can prepare a new version. [1] d0d77a256a75 ("mm/codetag: uninline and move pgalloc_tag_copy and pgalloc_tag_split") > --- > kernel/module/main.c | 4 ++++ > lib/alloc_tag.c | 2 +- > lib/codetag.c | 2 +- > 3 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/kernel/module/main.c b/kernel/module/main.c > index 129c98e6380d..00c16f5c5568 100644 > --- a/kernel/module/main.c > +++ b/kernel/module/main.c > @@ -2316,6 +2316,10 @@ static int move_module(struct module *mod, struct load_info *info) > if (codetag_needs_module_section(mod, sname, shdr->sh_size)) { > dest = codetag_alloc_module_section(mod, sname, shdr->sh_size, > arch_mod_section_prepend(mod, i), shdr->sh_addralign); > + if (WARN_ON(!dest)) { > + ret = -EINVAL; > + goto out_err; > + } > if (IS_ERR(dest)) { > ret = PTR_ERR(dest); > goto out_err; > diff --git a/lib/alloc_tag.c b/lib/alloc_tag.c > index 1c74942e6dfd..00ab18ea452a 100644 > --- a/lib/alloc_tag.c > +++ b/lib/alloc_tag.c > @@ -437,7 +437,7 @@ static void *reserve_module_tags(struct module *mod, unsigned long size, > > /* If no tags return NULL */ > if (size < sizeof(struct alloc_tag)) > - return NULL; > + return ERR_PTR(-EINVAL); > > /* > * align is always power of 2, so we can use IS_ALIGNED and ALIGN. > diff --git a/lib/codetag.c b/lib/codetag.c > index 4949511b4933..42aadd6c1454 100644 > --- a/lib/codetag.c > +++ b/lib/codetag.c > @@ -244,7 +244,7 @@ void *codetag_alloc_module_section(struct module *mod, const char *name, > { > const char *type_name = name + strlen(CODETAG_SECTION_PREFIX); > struct codetag_type *cttype; > - void *ret = NULL; > + void *ret = ERR_PTR(-EINVAL); > > mutex_lock(&codetag_lock); > list_for_each_entry(cttype, &codetag_types, link) { > > base-commit: 758db9ca0107bc6c00f0ed4808974d66c8dc2fea > -- > 2.47.0.163.g1226f6d8fa-goog >
On Thu, 31 Oct 2024 17:13:58 -0700 Suren Baghdasaryan <surenb@google.com> wrote: > On Thu, Oct 31, 2024 at 5:00 PM Suren Baghdasaryan <surenb@google.com> wrote: > > > > When a module does not have any allocations, it's allocation tag section > > is empty and codetag_alloc_module_section() returns NULL. However this > > condition should never happen because codetag_needs_module_section() will > > detect an empty section and avoid calling codetag_alloc_module_section(). > > Change codetag_alloc_module_section() to never return NULL, which should > > prevent static checker warnings. Add a WARN_ON() and a proper error > > reporting in case codetag_alloc_module_section() returns NULL, to prevent > > future codetag type implementations from returning NULL from their > > cttype->desc.alloc_section_mem() operation. > > > > Fixes: 61c9e58f3a10 ("alloc_tag: load module tags into separate contiguous memory") > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > > Closes: https://lore.kernel.org/all/50f12fa1-17c1-4940-a6bf-beaf61f6b17a@stanley.mountain/ > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > > Andrew, I was going to respin v5 of my patchset and include all these > small fixes in it but it's a bit tricky because I would have to revert > another unrelated patch [1] from mm-unstable which refactors relevant > code. So far the fixes are rather small, so I think you should not > have much trouble folding them into the original patchset when the > time comes, but if that becomes a problem I can prepare a new version. > No probs, thanks, I figured it out. The descriptions of where-it-fits-and-why are helpful. Of course we could just pile everything onto mm-unstable HEAD and live with a messier commit history, but I think things are manageable at present.
On Thu, Oct 31, 2024 at 05:00:17PM -0700, Suren Baghdasaryan wrote: > diff --git a/lib/alloc_tag.c b/lib/alloc_tag.c > index 1c74942e6dfd..00ab18ea452a 100644 > --- a/lib/alloc_tag.c > +++ b/lib/alloc_tag.c > @@ -437,7 +437,7 @@ static void *reserve_module_tags(struct module *mod, unsigned long size, > > /* If no tags return NULL */ ^^^^^^^^^^^ > if (size < sizeof(struct alloc_tag)) > - return NULL; > + return ERR_PTR(-EINVAL); > Thanks for this. We'd want to update the comment as well. regards, dan carpenter
diff --git a/kernel/module/main.c b/kernel/module/main.c index 129c98e6380d..00c16f5c5568 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -2316,6 +2316,10 @@ static int move_module(struct module *mod, struct load_info *info) if (codetag_needs_module_section(mod, sname, shdr->sh_size)) { dest = codetag_alloc_module_section(mod, sname, shdr->sh_size, arch_mod_section_prepend(mod, i), shdr->sh_addralign); + if (WARN_ON(!dest)) { + ret = -EINVAL; + goto out_err; + } if (IS_ERR(dest)) { ret = PTR_ERR(dest); goto out_err; diff --git a/lib/alloc_tag.c b/lib/alloc_tag.c index 1c74942e6dfd..00ab18ea452a 100644 --- a/lib/alloc_tag.c +++ b/lib/alloc_tag.c @@ -437,7 +437,7 @@ static void *reserve_module_tags(struct module *mod, unsigned long size, /* If no tags return NULL */ if (size < sizeof(struct alloc_tag)) - return NULL; + return ERR_PTR(-EINVAL); /* * align is always power of 2, so we can use IS_ALIGNED and ALIGN. diff --git a/lib/codetag.c b/lib/codetag.c index 4949511b4933..42aadd6c1454 100644 --- a/lib/codetag.c +++ b/lib/codetag.c @@ -244,7 +244,7 @@ void *codetag_alloc_module_section(struct module *mod, const char *name, { const char *type_name = name + strlen(CODETAG_SECTION_PREFIX); struct codetag_type *cttype; - void *ret = NULL; + void *ret = ERR_PTR(-EINVAL); mutex_lock(&codetag_lock); list_for_each_entry(cttype, &codetag_types, link) {
When a module does not have any allocations, it's allocation tag section is empty and codetag_alloc_module_section() returns NULL. However this condition should never happen because codetag_needs_module_section() will detect an empty section and avoid calling codetag_alloc_module_section(). Change codetag_alloc_module_section() to never return NULL, which should prevent static checker warnings. Add a WARN_ON() and a proper error reporting in case codetag_alloc_module_section() returns NULL, to prevent future codetag type implementations from returning NULL from their cttype->desc.alloc_section_mem() operation. Fixes: 61c9e58f3a10 ("alloc_tag: load module tags into separate contiguous memory") Reported-by: Dan Carpenter <dan.carpenter@linaro.org> Closes: https://lore.kernel.org/all/50f12fa1-17c1-4940-a6bf-beaf61f6b17a@stanley.mountain/ Signed-off-by: Suren Baghdasaryan <surenb@google.com> --- kernel/module/main.c | 4 ++++ lib/alloc_tag.c | 2 +- lib/codetag.c | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) base-commit: 758db9ca0107bc6c00f0ed4808974d66c8dc2fea