Re: BPF: failed module verification on linux-next
From: Andrii Nakryiko <hidden>
Date: 2021-05-25 19:27:04
Also in:
bpf, lkml
On Tue, May 25, 2021 at 6:51 AM Mel Gorman [off-list ref] wrote:
On Mon, May 24, 2021 at 03:58:29PM -0700, Andrii Nakryiko wrote:quoted
quoted
It took me a while to reliably bisect this, but it clearly points to this commit: e481fac7d80b ("mm/page_alloc: convert per-cpu list protection to local_lock") One commit before it, 676535512684 ("mm/page_alloc: split per cpu page lists and zone stats -fix"), works just fine. I'll have to spend more time debugging what exactly is happening, but the immediate problem is two different definitions of numa_node per-cpu variable. They both are at the same offset within .data..percpu ELF section, they both have the same name, but one of them is marked as static and another as global. And one is int variable, while another is struct pagesets. I'll look some more tomorrow, but adding Jiri and Arnaldo for visibility. [110907] DATASEC '.data..percpu' size=178904 vlen=303 ... type_id=27753 offset=163976 size=4 (VAR 'numa_node') type_id=27754 offset=163976 size=4 (VAR 'numa_node') [27753] VAR 'numa_node' type_id=27556, linkage=static [27754] VAR 'numa_node' type_id=20, linkage=global [20] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED [27556] STRUCT 'pagesets' size=0 vlen=1 'lock' type_id=507 bits_offset=0 [506] STRUCT '(anon)' size=0 vlen=0 [507] TYPEDEF 'local_lock_t' type_id=506 So also something weird about those zero-sized struct pagesets and local_lock_t inside it.Ok, so nothing weird about them. local_lock_t is designed to be zero-sized unless CONFIG_DEBUG_LOCK_ALLOC is defined. But such zero-sized per-CPU variables are confusing pahole during BTF generation, as now two different variables "occupy" the same address. Given this seems to be the first zero-sized per-CPU variable, I wonder if it would be ok to make sure it's never zero-sized, while pahole gets fixed and it's latest version gets widely packaged and distributed. Mel, what do you think about something like below? Or maybe you can advise some better solution?Ouch, something like that may never go away. How about just this?
Yeah, that would work just fine, thanks! Would you like me to send a formal patch or you'd like to do it?
quoted hunk ↗ jump to hunk
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index 58426acf5983..dce2df33d823 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug@@ -338,6 +338,9 @@ config DEBUG_INFO_BTF config PAHOLE_HAS_SPLIT_BTF def_bool $(success, test `$(PAHOLE) --version | sed -E 's/v([0-9]+)\.([0-9]+)/\1\2/'` -ge "119") +config PAHOLE_HAS_ZEROSIZE_PERCPU_SUPPORT + def_bool $(success, test `$(PAHOLE) --version | sed -E 's/v([0-9]+)\.([0-9]+)/\1\2/'` -ge "122") + config DEBUG_INFO_BTF_MODULES def_bool y depends on DEBUG_INFO_BTF && MODULES && PAHOLE_HAS_SPLIT_BTFdiff --git a/mm/page_alloc.c b/mm/page_alloc.c index 1599985e0ee1..cb1f84848c99 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c@@ -124,6 +124,17 @@ static DEFINE_MUTEX(pcp_batch_high_lock); struct pagesets { local_lock_t lock; +#if defined(CONFIG_DEBUG_INFO_BTF) && \ + !defined(CONFIG_DEBUG_LOCK_ALLOC) && \ + !defined(CONFIG_PAHOLE_HAS_ZEROSIZE_PERCPU_SUPPORT) + /* + * pahole 1.21 and earlier gets confused by zero-sized per-CPU + * variables and produces invalid BTF. Ensure that + * sizeof(struct pagesets) != 0 for older versions of pahole. + */ + char __pahole_hack; + #warning "pahole too old to support zero-sized struct pagesets" +#endif }; static DEFINE_PER_CPU(struct pagesets, pagesets) = { .lock = INIT_LOCAL_LOCK(lock),diff --git a/scripts/rust-version.sh b/scripts/rust-version.sh old mode 100644 new mode 100755
Probably didn't intend to include this?
-- Mel Gorman SUSE Labs