Re: [PATCH v9 3/4] module: introduce SH_ENTSIZE_STANDALONE for separately allocated sections

On 9/9/26 3:08 PM, Hao Ge wrote:

On 9/9/26 20:47, Hao Ge wrote:

quoted

On 9/9/26 19:30, Petr Pavlu wrote:

quoted

On 9/8/26 11:24 AM, Hao Ge wrote:

quoted

SHF_ALLOC means, per the ELF spec, that a section occupies memory
during process execution. Some module sections occupy memory
outside the regular module layout, for example the percpu section
with its per-CPU allocations. The loader currently excludes such
a section from the layout by clearing its SHF_ALLOC, which
overloads the flag with a loader-internal meaning.
apply_relocations() needs a special case for the section, and
find_sec(".data..percpu") returns different results before and
after layout_and_allocate().

Introduce SH_ENTSIZE_STANDALONE to mark sections with a separate
allocation. The percpu section is its first user. layout_sections()
and move_module() skip marked sections, and apply_relocations() goes
back to testing only SHF_ALLOC. Based on a patch by Petr Pavlu [1].

.data..percpu keeps SHF_ALLOC, so it would now show up under
/sys/module/*/sections/. The section has one instance per CPU and no
single address to report, and the entry never existed before, so
skip it in add_sect_attrs(). add_notes_attrs() indexes its attrs[]
array and skips it too. No functional change otherwise.

Fixes: 4835f747d3ed ("alloc_tag: support for page allocation tag compression")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/all/499bb60c-c6e3-43a3-bd92-95a0567ece5e@suse.com/ (local) [1]
Suggested-by: Petr Pavlu <petr.pavlu@suse.com>
Cc: stable@vger.kernel.org
Signed-off-by: Hao Ge <hao.ge@linux.dev>
---
[...]

diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c
index 0fc11e45df9b..49190deae61e 100644
--- a/kernel/module/kallsyms.c
+++ b/kernel/module/kallsyms.c

@@ -76,7 +76,7 @@ static char elf_type(const Elf_Sym *sym, const struct load_info *info)
  }
    static bool is_core_symbol(const Elf_Sym *src, const Elf_Shdr *sechdrs,
-               unsigned int shnum, unsigned int pcpundx)
+               unsigned int shnum)
  {
      const Elf_Shdr *sec;
      enum mod_mem_type type;

@@ -86,11 +86,6 @@ static bool is_core_symbol(const Elf_Sym *src, const Elf_Shdr *sechdrs,
          !src->st_name)
          return false;
  -#ifdef CONFIG_KALLSYMS_ALL
-    if (src->st_shndx == pcpundx)
-        return true;
-#endif
-
      sec = sechdrs + src->st_shndx;
      type = sec->sh_entsize >> SH_ENTSIZE_TYPE_SHIFT;
      if (!(sec->sh_flags & SHF_ALLOC)

@@ -131,8 +126,7 @@ void layout_symtab(struct module *mod, struct load_info *info)
      /* Compute total space required for the core symbols' strtab. */
      for (ndst = i = 0; i < nsrc; i++) {
          if (i == 0 || is_livepatch_module(mod) ||
-            is_core_symbol(src + i, info->sechdrs, info->hdr->e_shnum,
-                   info->index.pcpu)) {
+            is_core_symbol(src + i, info->sechdrs, info->hdr->e_shnum)) {
              strtab_size += strlen(&info->strtab[src[i].st_name]) + 1;
              ndst++;
          }

@@ -199,8 +193,7 @@ void add_kallsyms(struct module *mod, const struct load_info *info)
      for (ndst = i = 0; i < kallsyms->num_symtab; i++) {
          kallsyms->typetab[i] = elf_type(src + i, info);
          if (i == 0 || is_livepatch_module(mod) ||
-            is_core_symbol(src + i, info->sechdrs, info->hdr->e_shnum,
-                   info->index.pcpu)) {
+            is_core_symbol(src + i, info->sechdrs, info->hdr->e_shnum)) {
              ssize_t ret;
                mod->core_kallsyms.typetab[ndst] =

FTR These changes in kernel/module/kallsyms.c have a conflict with the
series "Ignore local labels and mapping symbols during module load" [1],
which is currently queued on modules-next, but it should be
straightforward to resolve.

quoted

diff --git a/kernel/module/sysfs.c b/kernel/module/sysfs.c
index 01c65d608873..f64170344e69 100644
--- a/kernel/module/sysfs.c
+++ b/kernel/module/sysfs.c

@@ -62,6 +62,15 @@ static void free_sect_attrs(struct module_sect_attrs *sect_attrs)
      kfree(sect_attrs);
  }
  +/*
+ * .data..percpu has a separate allocation per CPU and no single
+ * address to report.
+ */
+static bool sect_visible(const struct load_info *info, unsigned int i)
+{
+    return !sect_empty(&info->sechdrs[i]) && i != info->index.pcpu;
+}
+
  static int add_sect_attrs(struct module *mod, const struct load_info *info)
  {
      struct module_sect_attrs *sect_attrs;

@@ -72,7 +81,7 @@ static int add_sect_attrs(struct module *mod, const struct load_info *info)
        /* Count loaded sections and allocate structures */
      for (i = 0; i < info->hdr->e_shnum; i++)
-        if (!sect_empty(&info->sechdrs[i]))
+        if (sect_visible(info, i))
              nloaded++;
      sect_attrs = kzalloc_flex(*sect_attrs, attrs, nloaded);
      if (!sect_attrs)

@@ -92,7 +101,7 @@ static int add_sect_attrs(struct module *mod, const struct load_info *info)
      for (i = 0; i < info->hdr->e_shnum; i++) {
          Elf_Shdr *sec = &info->sechdrs[i];
  -        if (sect_empty(sec))
+        if (!sect_visible(info, i))
              continue;
          sysfs_bin_attr_init(sattr);
          sattr->attr.name =

@@ -181,7 +190,7 @@ static int add_notes_attrs(struct module *mod, const struct load_info *info)
        nattr = &notes_attrs->attrs[0];
      for (loaded = i = 0; i < info->hdr->e_shnum; ++i) {
-        if (sect_empty(&info->sechdrs[i]))
+        if (!sect_visible(info, i))
              continue;
          if (info->sechdrs[i].sh_type == SHT_NOTE) {
              sysfs_bin_attr_init(nattr);

add_notes_attrs() has two sect_empty() calls. Both should be changed to
sect_visible().

I kept this part unmodified to preserve the loop's original intent.

This loop counts SHT_NOTE sections.

SHT_NOTE refers to ELF note sections, which hold non-executable

metadata such as build ID and ABI info.

I wonder if we could keep the current implementation.

As noted in the comment above, the top part counts SHT_NOTE sections

and allocates structures, while the lower logic handles control of node attributes.


WDYT?

Sorry, I've reconsidered this. I think changing it to sect_visible would be better.

sect_visible stands for the count of externally visible note attributes, so the code

above and below can align with each other.


Sorry for the noise.

No worries.

One problem with continuing to use sect_empty() in the first loop is
that it could overallocate the number of required entries if a module
contains an SHT_NOTE section named .data..percpu. That shouldn't happen,
but we can trivially avoid it by using sect_visible() there as well.

Using the same condition in both loops also makes the code generally
simpler to understand.

-- 
Cheers,
Petr