Harden and extend ELF build ID parsing logic
Subsystems (12)
Andrii Nakryiko
maintainerv4 cover Harden and extend ELF build ID parsing logic
The goal of this patch set is to extend existing ELF build ID parsing logic, currently mostly used by BPF subsystem, with support for working in sleepable mode in which memory faults are allowed and can be relied upon to fetch relevant parts of ELF file to find and fetch .note.gnu.build-id information.
This is useful and important for BPF subsystem itself, but also for PROCMAP_QUERY ioctl(), built atop of /proc/<pid>/maps functionality (see [0]), which makes use of the same build_id_parse() functionality. PROCMAP_QUERY is always called from sleepable user process context, so it doesn't have to suffer from current restrictions of build_id_parse() which are due to the NMI context assumption.
Along the way, we harden the logic to avoid TOCTOU, overflow, out-of-bounds access problems. This is the very first patch, which can be backported to older releases, if necessary.
We also lift existing limitations of only working as long as ELF program headers and build ID note section is contained strictly within the very first page of ELF file.
We achieve all of the above without duplication of logic between sleepable and non-sleepable modes through freader abstraction that manages underlying folio from page cache (on demand) and gives a simple to use direct memory access interface. With that, single page restrictions and adding sleepable mode support is rather straightforward.
We also extend existing set of BPF selftests with a few tests targeting build ID logic across sleepable and non-sleepabe contexts (we utilize sleepable and non-sleepable uprobes for that).
[0] https://lore.kernel.org/linux-mm/[email protected]/
v3->v4:
- fix few more potential overflow and out-of-bounds access issues (Andi);
- use purely folio-based implementation for freader (Matthew);v2->v3:
- remove unneeded READ_ONCE()s and force phoff to u64 for 32-bit mode (Andi);
- moved hardening fixes to the front for easier backporting (Jann);
- call freader_cleanup() from build_id_parse_buf() for consistency (Jiri);v1->v2:
- ensure MADV_PAGEOUT works reliably by paging data in first (Shakeel);
- to fix BPF CI build optionally define MADV_POPULATE_READ in selftest.
Andrii Nakryiko (10):
lib/buildid: harden build ID parsing logic
lib/buildid: add single folio-based file reader abstraction lib/buildid: take into account e_phoff when fetching program headers lib/buildid: remove single-page limit for PHDR search lib/buildid: rename build_id_parse() into build_id_parse_nofault() lib/buildid: implement sleepable build_id_parse() API lib/buildid: don't limit .note.gnu.build-id to the first page in ELF
bpf: decouple stack_map_get_build_id_offset() from
perf_callchain_entry
bpf: wire up sleepable bpf_get_stack() and bpf_get_task_stack()
helpers
selftests/bpf: add build ID tests
include/linux/bpf.h | 2 + include/linux/buildid.h | 4 +- kernel/bpf/stackmap.c | 131 ++++-- kernel/events/core.c | 2 +- kernel/trace/bpf_trace.c | 5 +- lib/buildid.c | 389 +++++++++++++----- tools/testing/selftests/bpf/Makefile | 5 +- .../selftests/bpf/prog_tests/build_id.c | 118 ++++++ .../selftests/bpf/progs/test_build_id.c | 31 ++ tools/testing/selftests/bpf/uprobe_multi.c | 41 ++ tools/testing/selftests/bpf/uprobe_multi.ld | 11 + 11 files changed, 598 insertions(+), 141 deletions(-) create mode 100644 tools/testing/selftests/bpf/prog_tests/build_id.c create mode 100644 tools/testing/selftests/bpf/progs/test_build_id.c create mode 100644 tools/testing/selftests/bpf/uprobe_multi.ld
Signature · 1 line
2.43.5
Andrii Nakryiko
maintainerPATCH v4 1/10 lib/buildid: harden build ID parsing logic
Harden build ID parsing logic, adding explicit READ_ONCE() where it's important to have a consistent value read and validated just once.
Also, as pointed out by Andi Kleen, we need to make sure that entire ELF note is within a page bounds, so move the overflow check up and add an extra note_size boundaries validation.
Fixes tag below points to the code that moved this code into lib/buildid.c, and then subsequently was used in perf subsystem, making this code exposed to perf_event_open() users in v5.12+.
--- lib/buildid.c | 60 ++++++++++++++++++++++++++++----------------------- 1 file changed, 33 insertions(+), 27 deletions(-)
lib/buildid.c +33 -27
@@ -18,30 +18,34 @@ static int parse_build_id_buf(unsigned char *build_id,const void *note_start, Elf32_Word note_size) { + const char note_name[] = "GNU"; +const size_t note_name_sz =sizeof (note_name);Elf32_Word note_offs =0 , new_offs; +u32 name_sz, desc_sz; +const char *data; -while (note_offs +sizeof (Elf32_Nhdr) < note_size) { +while (note_offs +sizeof (Elf32_Nhdr) < note_size && + note_offs +sizeof (Elf32_Nhdr) > note_offs/* overflow */ ) {Elf32_Nhdr *nhdr = (Elf32_Nhdr *)(note_start + note_offs); + name_sz =READ_ONCE (nhdr->n_namesz); + desc_sz =READ_ONCE (nhdr->n_descsz); + new_offs = note_offs +sizeof (Elf32_Nhdr) +ALIGN (name_sz,4 ) +ALIGN (desc_sz,4 ); +if (new_offs <= note_offs/* overflow */ || new_offs > note_size) +break ; +if (nhdr->n_type ==BUILD_ID && - nhdr->n_namesz ==sizeof ("GNU" ) && - !strcmp ((char *)(nhdr +1 ),"GNU" ) && - nhdr->n_descsz >0 && - nhdr->n_descsz <=BUILD_ID_SIZE_MAX ) { -memcpy (build_id, - note_start + note_offs + -ALIGN (sizeof ("GNU" ),4 ) +sizeof (Elf32_Nhdr), - nhdr->n_descsz); -memset (build_id + nhdr->n_descsz,0 , -BUILD_ID_SIZE_MAX - nhdr->n_descsz); + name_sz == note_name_sz && +strcmp ((char *)(nhdr +1 ), note_name) ==0 && + desc_sz >0 && desc_sz <=BUILD_ID_SIZE_MAX ) { + data = note_start + note_offs +ALIGN (note_name_sz,4 ); +memcpy (build_id, data, desc_sz); +memset (build_id + desc_sz,0 ,BUILD_ID_SIZE_MAX - desc_sz);if (size) - *size = nhdr->n_descsz; + *size = desc_sz;return 0 ; } - new_offs = note_offs +sizeof (Elf32_Nhdr) + -ALIGN (nhdr->n_namesz,4 ) +ALIGN (nhdr->n_descsz,4 ); -if (new_offs <= note_offs)/* overflow */ -break ; + note_offs = new_offs; }
@@ -71,7 +75,7 @@ static int get_build_id_32(const void *page_addr, unsigned char *build_id, {Elf32_Ehdr *ehdr = (Elf32_Ehdr *)page_addr;Elf32_Phdr *phdr; -int i; +__u32 i, phnum;/* * FIXME
@@ -80,9 +84,10 @@ static int get_build_id_32(const void *page_addr, unsigned char *build_id,*/ if (ehdr->e_phoff !=sizeof (Elf32_Ehdr))return -EINVAL ; + + phnum =READ_ONCE (ehdr->e_phnum);/* only supports phdr that fits in one page */ -if (ehdr->e_phnum > - (PAGE_SIZE -sizeof (Elf32_Ehdr)) /sizeof (Elf32_Phdr)) +if (phnum > (PAGE_SIZE -sizeof (Elf32_Ehdr)) /sizeof (Elf32_Phdr))return -EINVAL ; phdr = (Elf32_Phdr *)(page_addr +sizeof (Elf32_Ehdr));
@@ -90,8 +95,8 @@ static int get_build_id_32(const void *page_addr, unsigned char *build_id,for (i =0 ; i < ehdr->e_phnum; ++i) {if (phdr[i].p_type ==PT_NOTE && !parse_build_id (page_addr, build_id, size, - page_addr + phdr[i].p_offset, - phdr[i].p_filesz)) + page_addr +READ_ONCE (phdr[i].p_offset), +READ_ONCE (phdr[i].p_filesz)))return 0 ; }return -EINVAL ;
@@ -103,7 +108,7 @@ static int get_build_id_64(const void *page_addr, unsigned char *build_id, {Elf64_Ehdr *ehdr = (Elf64_Ehdr *)page_addr;Elf64_Phdr *phdr; -int i; +__u32 i, phnum;/* * FIXME
@@ -112,18 +117,19 @@ static int get_build_id_64(const void *page_addr, unsigned char *build_id,*/ if (ehdr->e_phoff !=sizeof (Elf64_Ehdr))return -EINVAL ; + + phnum =READ_ONCE (ehdr->e_phnum);/* only supports phdr that fits in one page */ -if (ehdr->e_phnum > - (PAGE_SIZE -sizeof (Elf64_Ehdr)) /sizeof (Elf64_Phdr)) +if (phnum > (PAGE_SIZE -sizeof (Elf64_Ehdr)) /sizeof (Elf64_Phdr))return -EINVAL ; phdr = (Elf64_Phdr *)(page_addr +sizeof (Elf64_Ehdr)); -for (i =0 ; i < ehdr->e_phnum; ++i) { +for (i =0 ; i < phnum; ++i) {if (phdr[i].p_type ==PT_NOTE && !parse_build_id (page_addr, build_id, size, - page_addr + phdr[i].p_offset, - phdr[i].p_filesz)) + page_addr +READ_ONCE (phdr[i].p_offset), +READ_ONCE (phdr[i].p_filesz)))return 0 ; }return -EINVAL ;
Signature · 1 line
2.43.5
Re: [PATCH v4 bpf-next 01/10] lib/buildid: harden build ID parsing logic
lib/buildid.c static int parse_build_id_buf(unsigned char *build_id, · line 33
Quoted 3 lines
lib/buildid.c +3
-0+ name_sz =READ_ONCE (nhdr->n_namesz); + desc_sz =READ_ONCE (nhdr->n_descsz); + new_offs = note_offs +sizeof (Elf32_Nhdr) +ALIGN (name_sz,4 ) +ALIGN (desc_sz,4 );
Don't you need to check the name_sz and desc_sz overflows separately?
Otherwise name_sz could be ~0 and desc_sz small (or reversed) and the check below wouldn't trigger, but still bad things could happen.
lib/buildid.c static int parse_build_id_buf(unsigned char *build_id, · line 35
Quoted 2 lines
lib/buildid.c +2
-0+if (new_offs <= note_offs/* overflow */ || new_offs > note_size) +break ;
-Andi
Re: [PATCH v4 bpf-next 01/10] lib/buildid: harden build ID parsing logic
Quoted 9 lines · Andi Kleen
lib/buildid.c static int parse_build_id_buf(unsigned char *build_id, · line 33
Quoted 4 lines
lib/buildid.c +3
-0+ name_sz =READ_ONCE (nhdr->n_namesz); + desc_sz =READ_ONCE (nhdr->n_descsz); + new_offs = note_offs +sizeof (Elf32_Nhdr) +ALIGN (name_sz,4 ) +ALIGN (desc_sz,4 );Don't you need to check the
name_szanddesc_szoverflows separately?Otherwise
name_szcould be ~0 anddesc_szsmall (or reversed) and the check below wouldn't trigger, but still bad things could happen.
Yes, both sizes are full u32, so yes, they could technically both overflow resulting in final non-overflown new_offs. I'll switch the additions to be done step by step.
Quoted 6 lines
lib/buildid.c static int parse_build_id_buf(unsigned char *build_id, · line 35
Quoted 4 lines
lib/buildid.c +2
-0+if (new_offs <= note_offs/* overflow */ || new_offs > note_size) +break ;-Andi
Andrii Nakryiko
maintainerPATCH v4 2/10 lib/buildid: add single folio-based file reader abstraction
In reply to Andrii NakryikoAdd freader abstraction that transparently manages fetching and local mapping of the underlying file page(s) and provides a simple direct data access interface.
freader_fetch() is the only and single interface necessary. It accepts file offset and desired number of bytes that should be accessed, and will return a kernel mapped pointer that caller can use to dereference data up to requested size. Requested size can't be bigger than the size of the extra buffer provided during initialization (because, worst case, all requested data has to be copied into it, so it's better to flag wrongly sized buffer unconditionally, regardless if requested data range is crossing page boundaries or not).
If folio is not paged in, or some of the conditions are not satisfied, NULL is returned and more detailed error code can be accessed through freader->err field. This approach makes the usage of freader_fetch() cleaner.
To accommodate accessing file data that crosses folio boundaries, user has to provide an extra buffer that will be used to make a local copy, if necessary. This is done to maintain a simple linear pointer data access interface.
We switch existing build ID parsing logic to it, without changing or lifting any of the existing constraints, yet. This will be done separately.
Given existing code was written with the assumption that it's always working with a single (first) page of the underlying ELF file, logic passes direct pointers around, which doesn't really work well with freader approach and would be limiting when removing the single page (folio) limitation. So we adjust all the logic to work in terms of file offsets.
There is also a memory buffer-based version (freader_init_from_mem()) for cases when desired data is already available in kernel memory. This is used for parsing vmlinux's own build ID note. In this mode assumption is that provided data starts at "file offset" zero, which works great when parsing ELF notes sections, as all the parsing logic is relative to note section's start.
--- lib/buildid.c | 260 ++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 210 insertions(+), 50 deletions(-)
lib/buildid.c +210 -50
@@ -8,37 +8,173 @@#define BUILD_ID 3 +struct freader { +void *buf; +u32 buf_sz; +int err; +union { +struct { +struct address_space *mapping; +struct folio *folio; +void *addr; +loff_t folio_off; + }; +struct { +const char *data; +u64 data_sz; + }; + }; +}; + +static void freader_init_from_file (struct freader *r,void *buf,u32 buf_sz, +struct address_space *mapping) +{ +memset (r,0 ,sizeof (*r)); + r->buf = buf; + r->buf_sz = buf_sz; + r->mapping = mapping; +} + +static void freader_init_from_mem (struct freader *r,const char *data,u64 data_sz) +{ +memset (r,0 ,sizeof (*r)); + r->data = data; + r->data_sz = data_sz; +} + +static void freader_put_folio (struct freader *r) +{ +if (!r->folio) +return ; +kunmap_local (r->addr); +folio_put (r->folio); + r->folio =NULL ; +} + +static int freader_get_folio (struct freader *r,loff_t file_off) +{ +/* check if we can just reuse current folio */ +if (r->folio && file_off >= r->folio_off && + file_off < r->folio_off +folio_size (r->folio)) +return 0 ; + +freader_put_folio (r); + + r->folio =filemap_get_folio (r->mapping, file_off >>PAGE_SHIFT ); +if (IS_ERR (r->folio) || !folio_test_uptodate (r->folio)) { +if (!IS_ERR (r->folio)) +folio_put (r->folio); + r->folio =NULL ; +return -EFAULT ; + } + + r->folio_off =folio_pos (r->folio); + r->addr =kmap_local_folio (r->folio,0 ); + +return 0 ; +} + +static const void *freader_fetch (struct freader *r,loff_t file_off,size_t sz) +{ +size_t folio_sz; + +/* provided internal temporary buffer should be sized correctly */ +if (WARN_ON (r->buf && sz > r->buf_sz)) { + r->err = -E2BIG ; +return NULL ; + } + +if (unlikely (file_off + sz < file_off)) { + r->err = -EOVERFLOW ; +return NULL ; + } + +/* working with memory buffer is much more straightforward */ +if (!r->buf) { +if (file_off + sz > r->data_sz) { + r->err = -ERANGE ; +return NULL ; + } +return r->data + file_off; + } + +/* fetch or reuse folio for given file offset */ + r->err =freader_get_folio (r, file_off); +if (r->err) +return NULL ; + +/* if requested data is crossing folio boundaries, we have to copy +* everything into our local buffer to keep a simple linear memory +* access interface +*/ + folio_sz =folio_size (r->folio); +if (file_off + sz > r->folio_off + folio_sz) { +int part_sz = r->folio_off + folio_sz - file_off; + +/* copy the part that resides in the current folio */ +memcpy (r->buf, r->addr + (file_off - r->folio_off), part_sz); + +/* fetch next folio */ + r->err =freader_get_folio (r, r->folio_off + folio_sz); +if (r->err) +return NULL ; + +/* copy the rest of requested data */ +memcpy (r->buf + part_sz, r->addr, sz - part_sz); + +return r->buf; + } + +/* if data fits in a single folio, just return direct pointer */ +return r->addr + (file_off - r->folio_off); +} + +static void freader_cleanup (struct freader *r) +{ +if (!r->buf) +return ;/* non-file-backed mode */ + +freader_put_folio (r); +} +/* * Parse build id from the note segment. This logic can be shared between * 32-bit and 64-bit system, because Elf32_Nhdr and Elf64_Nhdr are * identical. */ -static int parse_build_id_buf (unsigned char *build_id, -__u32 *size, -const void *note_start, -Elf32_Word note_size) +static int parse_build_id_buf (struct freader *r, +unsigned char *build_id,__u32 *size, +loff_t note_offs,Elf32_Word note_size) {const char note_name[] ="GNU" ;const size_t note_name_sz =sizeof (note_name); -Elf32_Word note_offs =0 , new_offs; +const Elf32_Nhdr *nhdr; +loff_t build_id_off, new_offs, note_end = note_offs + note_size;u32 name_sz, desc_sz;const char *data; -while (note_offs +sizeof (Elf32_Nhdr) < note_size && - note_offs +sizeof (Elf32_Nhdr) > note_offs/* overflow */ ) { -Elf32_Nhdr *nhdr = (Elf32_Nhdr *)(note_start + note_offs); +while (note_end - note_offs >sizeof (Elf32_Nhdr) + note_name_sz) { + nhdr =freader_fetch (r, note_offs,sizeof (Elf32_Nhdr) + note_name_sz); +if (!nhdr) +return r->err; name_sz =READ_ONCE (nhdr->n_namesz); desc_sz =READ_ONCE (nhdr->n_descsz); new_offs = note_offs +sizeof (Elf32_Nhdr) +ALIGN (name_sz,4 ) +ALIGN (desc_sz,4 ); -if (new_offs <= note_offs/* overflow */ || new_offs > note_size) +if (new_offs <= note_offs/* overflow */ || new_offs > note_end)break ;if (nhdr->n_type ==BUILD_ID && name_sz == note_name_sz &&strcmp ((char *)(nhdr +1 ), note_name) ==0 && desc_sz >0 && desc_sz <=BUILD_ID_SIZE_MAX ) { - data = note_start + note_offs +ALIGN (note_name_sz,4 ); + build_id_off = note_offs +sizeof (Elf32_Nhdr) +ALIGN (note_name_sz,4 ); + +/* freader_fetch() will invalidate nhdr pointer */ + data =freader_fetch (r, build_id_off, desc_sz); +if (!data) +return r->err; +memcpy (build_id, data, desc_sz);memset (build_id + desc_sz,0 ,BUILD_ID_SIZE_MAX - desc_sz);if (size)
@@ -52,30 +188,33 @@ static int parse_build_id_buf(unsigned char *build_id,return -EINVAL ; } -static inline int parse_build_id (const void *page_addr, +static inline int parse_build_id (struct freader *r,unsigned char *build_id,__u32 *size, -const void *note_start, +loff_t note_start_off,Elf32_Word note_size) {/* check for overflow */ -if (note_start < page_addr || note_start + note_size < note_start) +if (note_start_off + note_size < note_start_off)return -EINVAL ;/* only supports note that fits in the first page */ -if (note_start + note_size > page_addr +PAGE_SIZE ) +if (note_start_off + note_size >PAGE_SIZE )return -EINVAL ; -return parse_build_id_buf (build_id, size, note_start, note_size); +return parse_build_id_buf (r, build_id, size, note_start_off, note_size); }/* Parse build ID from 32-bit ELF */ -static int get_build_id_32 (const void *page_addr,unsigned char *build_id, -__u32 *size) +static int get_build_id_32 (struct freader *r,unsigned char *build_id,__u32 *size) { -Elf32_Ehdr *ehdr = (Elf32_Ehdr *)page_addr; -Elf32_Phdr *phdr; -__u32 i, phnum; +const Elf32_Ehdr *ehdr; +const Elf32_Phdr *phdr; +__u32 phnum, i; + + ehdr =freader_fetch (r,0 ,sizeof (Elf32_Ehdr)); +if (!ehdr) +return r->err;/* * FIXME
@@ -85,30 +224,35 @@ static int get_build_id_32(const void *page_addr, unsigned char *build_id,if (ehdr->e_phoff !=sizeof (Elf32_Ehdr))return -EINVAL ; +/* subsequent freader_fetch() calls invalidate pointers, so remember locally */ phnum =READ_ONCE (ehdr->e_phnum);/* only supports phdr that fits in one page */ if (phnum > (PAGE_SIZE -sizeof (Elf32_Ehdr)) /sizeof (Elf32_Phdr))return -EINVAL ; - phdr = (Elf32_Phdr *)(page_addr +sizeof (Elf32_Ehdr)); +for (i =0 ; i < phnum; ++i) { + phdr =freader_fetch (r, i *sizeof (Elf32_Phdr),sizeof (Elf32_Phdr)); +if (!phdr) +return r->err; -for (i =0 ; i < ehdr->e_phnum; ++i) { -if (phdr[i].p_type ==PT_NOTE && - !parse_build_id (page_addr, build_id, size, - page_addr +READ_ONCE (phdr[i].p_offset), -READ_ONCE (phdr[i].p_filesz))) +if (phdr->p_type ==PT_NOTE && + !parse_build_id (r, build_id, size,READ_ONCE (phdr->p_offset), +READ_ONCE (phdr->p_filesz)))return 0 ; }return -EINVAL ; }/* Parse build ID from 64-bit ELF */ -static int get_build_id_64 (const void *page_addr,unsigned char *build_id, -__u32 *size) +static int get_build_id_64 (struct freader *r,unsigned char *build_id,__u32 *size) { -Elf64_Ehdr *ehdr = (Elf64_Ehdr *)page_addr; -Elf64_Phdr *phdr; -__u32 i, phnum; +const Elf64_Ehdr *ehdr; +const Elf64_Phdr *phdr; +__u32 phnum, i; + + ehdr =freader_fetch (r,0 ,sizeof (Elf64_Ehdr)); +if (!ehdr) +return r->err;/* * FIXME
@@ -118,23 +262,29 @@ static int get_build_id_64(const void *page_addr, unsigned char *build_id,if (ehdr->e_phoff !=sizeof (Elf64_Ehdr))return -EINVAL ; +/* subsequent freader_fetch() calls invalidate pointers, so remember locally */ phnum =READ_ONCE (ehdr->e_phnum);/* only supports phdr that fits in one page */ if (phnum > (PAGE_SIZE -sizeof (Elf64_Ehdr)) /sizeof (Elf64_Phdr))return -EINVAL ; - phdr = (Elf64_Phdr *)(page_addr +sizeof (Elf64_Ehdr)); -for (i =0 ; i < phnum; ++i) { -if (phdr[i].p_type ==PT_NOTE && - !parse_build_id (page_addr, build_id, size, - page_addr +READ_ONCE (phdr[i].p_offset), -READ_ONCE (phdr[i].p_filesz))) + phdr =freader_fetch (r, i *sizeof (Elf64_Phdr),sizeof (Elf64_Phdr)); +if (!phdr) +return r->err; + +if (phdr->p_type ==PT_NOTE && + !parse_build_id (r, build_id, size,READ_ONCE (phdr->p_offset), +READ_ONCE (phdr->p_filesz)))return 0 ; } +return -EINVAL ; } +/* enough for Elf64_Ehdr, Elf64_Phdr, and all the smaller requests */ +#define MAX_FREADER_BUF_SZ 64 +/* * Parse build ID of ELF file mapped to vma * @vma: vma object
@@ -146,22 +296,25 @@ static int get_build_id_64(const void *page_addr, unsigned char *build_id,int build_id_parse (struct vm_area_struct *vma,unsigned char *build_id,__u32 *size) { -Elf32_Ehdr *ehdr; -struct page *page; -void *page_addr; +const Elf32_Ehdr *ehdr; +struct freader r; +char buf[MAX_FREADER_BUF_SZ ];int ret;/* only works for page backed storage */ if (!vma->vm_file)return -EINVAL ; - page =find_get_page (vma->vm_file->f_mapping,0 ); -if (!page) -return -EFAULT ;/* page not mapped */ +freader_init_from_file (&r, buf,sizeof (buf), vma->vm_file->f_mapping); + +/* fetch first 18 bytes of ELF header for checks */ + ehdr =freader_fetch (&r,0 ,offsetofend (Elf32_Ehdr, e_type)); +if (!ehdr) { + ret = r.err; + goto out; + } ret = -EINVAL ; - page_addr =kmap_local_page (page); - ehdr = (Elf32_Ehdr *)page_addr;/* compare magic x7f "ELF" */ if (memcmp (ehdr->e_ident,ELFMAG ,SELFMAG ) !=0 )
@@ -172,12 +325,11 @@ int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id, goto out;if (ehdr->e_ident[EI_CLASS ] ==ELFCLASS32 ) - ret =get_build_id_32 (page_addr, build_id, size); + ret =get_build_id_32 (&r, build_id, size);else if (ehdr->e_ident[EI_CLASS ] ==ELFCLASS64 ) - ret =get_build_id_64 (page_addr, build_id, size); + ret =get_build_id_64 (&r, build_id, size); out: -kunmap_local (page_addr); -put_page (page); +freader_cleanup (&r);return ret; }
@@ -191,7 +343,15 @@ int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,*/ int build_id_parse_buf (const void *buf,unsigned char *build_id,u32 buf_size) { -return parse_build_id_buf (build_id,NULL , buf, buf_size); +struct freader r; +int err; + +freader_init_from_mem (&r, buf, buf_size); + + err =parse_build_id (&r, build_id,NULL ,0 , buf_size); + +freader_cleanup (&r); +return err; }#if IS_ENABLED (CONFIG_STACKTRACE_BUILD_ID ) ||IS_ENABLED (CONFIG_VMCORE_INFO )
Signature · 1 line
2.43.5
Re: [PATCH v4 bpf-next 02/10] lib/buildid: add single folio-based file reader abstraction
Quoted 41 lines · Andrii Nakryiko
Add freader abstraction that transparently manages fetching and local mapping of the underlying file page(s) and provides a simple direct data access interface.
freader_fetch()is the only and single interface necessary. It accepts file offset and desired number of bytes that should be accessed, and will return a kernel mapped pointer that caller can use to dereference data up to requested size. Requested size can't be bigger than the size of the extra buffer provided during initialization (because, worst case, all requested data has to be copied into it, so it's better to flag wrongly sized buffer unconditionally, regardless if requested data range is crossing page boundaries or not).If folio is not paged in, or some of the conditions are not satisfied, NULL is returned and more detailed error code can be accessed through
freader->errfield. This approach makes the usage offreader_fetch()cleaner.To accommodate accessing file data that crosses folio boundaries, user has to provide an extra buffer that will be used to make a local copy, if necessary. This is done to maintain a simple linear pointer data access interface.
We switch existing build ID parsing logic to it, without changing or lifting any of the existing constraints, yet. This will be done separately.
Given existing code was written with the assumption that it's always working with a single (first) page of the underlying ELF file, logic passes direct pointers around, which doesn't really work well with freader approach and would be limiting when removing the single page (folio) limitation. So we adjust all the logic to work in terms of file offsets.
There is also a memory buffer-based version (
freader_init_from_mem()) for cases when desired data is already available in kernel memory. This is used for parsing vmlinux's own build ID note. In this mode assumption is that provided data starts at "file offset" zero, which works great when parsing ELF notes sections, as all the parsing logic is relative to note section's start.Signed-off-by Andrii Nakryiko <[email protected]>
[...]
lib/buildid.c line 64
Quoted 11 lines
lib/buildid.c +11
-0+static int freader_get_folio (struct freader *r,loff_t file_off) +{ +/* check if we can just reuse current folio */ +if (r->folio && file_off >= r->folio_off && + file_off < r->folio_off +folio_size (r->folio)) +return 0 ; + +freader_put_folio (r); + + r->folio =filemap_get_folio (r->mapping, file_off >>PAGE_SHIFT ); +if (IS_ERR (r->folio) || !folio_test_uptodate (r->folio)) {
Can you also fix the uptodate stuff in patch 1, or in another stable-backportable patch? (I guess alternatively we can fix it with a custom patch in stable after it's been fixed in mainline, but that feels kinda hacky to me.)
Re: [PATCH v4 bpf-next 02/10] lib/buildid: add single folio-based file reader abstraction
Quoted 44 lines · Jann Horn
Quoted 41 lines · Andrii Nakryiko
Add freader abstraction that transparently manages fetching and local mapping of the underlying file page(s) and provides a simple direct data access interface.
freader_fetch()is the only and single interface necessary. It accepts file offset and desired number of bytes that should be accessed, and will return a kernel mapped pointer that caller can use to dereference data up to requested size. Requested size can't be bigger than the size of the extra buffer provided during initialization (because, worst case, all requested data has to be copied into it, so it's better to flag wrongly sized buffer unconditionally, regardless if requested data range is crossing page boundaries or not).If folio is not paged in, or some of the conditions are not satisfied, NULL is returned and more detailed error code can be accessed through
freader->errfield. This approach makes the usage offreader_fetch()cleaner.To accommodate accessing file data that crosses folio boundaries, user has to provide an extra buffer that will be used to make a local copy, if necessary. This is done to maintain a simple linear pointer data access interface.
We switch existing build ID parsing logic to it, without changing or lifting any of the existing constraints, yet. This will be done separately.
Given existing code was written with the assumption that it's always working with a single (first) page of the underlying ELF file, logic passes direct pointers around, which doesn't really work well with freader approach and would be limiting when removing the single page (folio) limitation. So we adjust all the logic to work in terms of file offsets.
There is also a memory buffer-based version (
freader_init_from_mem()) for cases when desired data is already available in kernel memory. This is used for parsing vmlinux's own build ID note. In this mode assumption is that provided data starts at "file offset" zero, which works great when parsing ELF notes sections, as all the parsing logic is relative to note section's start.Signed-off-by Andrii Nakryiko <[email protected]>[...]
Quoted 16 lines
lib/buildid.c line 64
Quoted 11 lines
lib/buildid.c +11
-0+static int freader_get_folio (struct freader *r,loff_t file_off) +{ +/* check if we can just reuse current folio */ +if (r->folio && file_off >= r->folio_off && + file_off < r->folio_off +folio_size (r->folio)) +return 0 ; + +freader_put_folio (r); + + r->folio =filemap_get_folio (r->mapping, file_off >>PAGE_SHIFT ); +if (IS_ERR (r->folio) || !folio_test_uptodate (r->folio)) {Can you also fix the uptodate stuff in patch 1, or in another stable-backportable patch? (I guess alternatively we can fix it with a custom patch in stable after it's been fixed in mainline, but that feels kinda hacky to me.)
Ah, right, damn, forgot to do that also in patch #1... I'll fix it locally so I don't forget, will give others time to review and provide feedback or acks, so I don't spam the list too much.
Re: [PATCH v4 bpf-next 02/10] lib/buildid: add single folio-based file reader abstraction
In reply to Andrii NakryikoQuoted 33 lines · Andrii Nakryiko
Add freader abstraction that transparently manages fetching and local mapping of the underlying file page(s) and provides a simple direct data access interface.
freader_fetch()is the only and single interface necessary. It accepts file offset and desired number of bytes that should be accessed, and will return a kernel mapped pointer that caller can use to dereference data up to requested size. Requested size can't be bigger than the size of the extra buffer provided during initialization (because, worst case, all requested data has to be copied into it, so it's better to flag wrongly sized buffer unconditionally, regardless if requested data range is crossing page boundaries or not).If folio is not paged in, or some of the conditions are not satisfied, NULL is returned and more detailed error code can be accessed through
freader->errfield. This approach makes the usage offreader_fetch()cleaner.To accommodate accessing file data that crosses folio boundaries, user has to provide an extra buffer that will be used to make a local copy, if necessary. This is done to maintain a simple linear pointer data access interface.
We switch existing build ID parsing logic to it, without changing or lifting any of the existing constraints, yet. This will be done separately.
Given existing code was written with the assumption that it's always working with a single (first) page of the underlying ELF file, logic passes direct pointers around, which doesn't really work well with freader approach and would be limiting when removing the single page (folio) limitation. So we adjust all the logic to work in terms of file offsets.
Quoted 8 lines
There is also a memory buffer-based version (
freader_init_from_mem()) for cases when desired data is already available in kernel memory. This is used for parsing vmlinux's own build ID note. In this mode assumption is that provided data starts at "file offset" zero, which works great when parsing ELF notes sections, as all the parsing logic is relative to note section's start.Signed-off-by Andrii Nakryiko <[email protected]>
Andrii Nakryiko
maintainerPATCH v4 3/10 lib/buildid: take into account e_phoff when fetching program headers
In reply to Andrii NakryikoCurrent code assumption is that program (segment) headers are following ELF header immediately. This is a common case, but is not guaranteed. So take into account e_phoff field of the ELF header when accessing program headers.
--- lib/buildid.c | 35 ++++++++++++++++------------------- 1 file changed, 16 insertions(+), 19 deletions(-)
lib/buildid.c +16 -19
@@ -210,28 +210,26 @@ static int get_build_id_32(struct freader *r, unsigned char *build_id, __u32 *si {const Elf32_Ehdr *ehdr;const Elf32_Phdr *phdr; -__u32 phnum, i; +__u32 phnum, phoff, i; ehdr =freader_fetch (r,0 ,sizeof (Elf32_Ehdr));if (!ehdr)return r->err; -/* -* FIXME -* Neither ELF spec nor ELF loader require that program headers -* start immediately after ELF header. -*/ -if (ehdr->e_phoff !=sizeof (Elf32_Ehdr)) -return -EINVAL ; -/* subsequent freader_fetch() calls invalidate pointers, so remember locally */ phnum =READ_ONCE (ehdr->e_phnum); + phoff =READ_ONCE (ehdr->e_phoff); +/* only supports phdr that fits in one page */ if (phnum > (PAGE_SIZE -sizeof (Elf32_Ehdr)) /sizeof (Elf32_Phdr))return -EINVAL ; +/* check that phoff is not large enough to cause an overflow */ +if (phoff + phnum *sizeof (Elf32_Phdr) < phoff) +return -EINVAL ; +for (i =0 ; i < phnum; ++i) { - phdr =freader_fetch (r, i *sizeof (Elf32_Phdr),sizeof (Elf32_Phdr)); + phdr =freader_fetch (r, phoff + i *sizeof (Elf32_Phdr),sizeof (Elf32_Phdr));if (!phdr)return r->err;
@@ -249,27 +247,26 @@ static int get_build_id_64(struct freader *r, unsigned char *build_id, __u32 *siconst Elf64_Ehdr *ehdr;const Elf64_Phdr *phdr;__u32 phnum, i; +__u64 phoff; ehdr =freader_fetch (r,0 ,sizeof (Elf64_Ehdr));if (!ehdr)return r->err; -/* -* FIXME -* Neither ELF spec nor ELF loader require that program headers -* start immediately after ELF header. -*/ -if (ehdr->e_phoff !=sizeof (Elf64_Ehdr)) -return -EINVAL ; -/* subsequent freader_fetch() calls invalidate pointers, so remember locally */ phnum =READ_ONCE (ehdr->e_phnum); + phoff =READ_ONCE (ehdr->e_phoff); +/* only supports phdr that fits in one page */ if (phnum > (PAGE_SIZE -sizeof (Elf64_Ehdr)) /sizeof (Elf64_Phdr))return -EINVAL ; +/* check that phoff is not large enough to cause an overflow */ +if (phoff + phnum *sizeof (Elf64_Phdr) < phoff) +return -EINVAL ; +for (i =0 ; i < phnum; ++i) { - phdr =freader_fetch (r, i *sizeof (Elf64_Phdr),sizeof (Elf64_Phdr)); + phdr =freader_fetch (r, phoff + i *sizeof (Elf64_Phdr),sizeof (Elf64_Phdr));if (!phdr)return r->err;
Signature · 1 line
2.43.5
Andrii Nakryiko
maintainerPATCH v4 4/10 lib/buildid: remove single-page limit for PHDR search
In reply to Andrii NakryikoNow that freader allows to access multiple pages transparently, there is no need to limit program headers to the very first ELF file page. Remove this limitation, but still put some sane limit on amount of program headers that we are willing to iterate over (set arbitrarily to 256).
--- lib/buildid.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
lib/buildid.c +8 -6
@@ -8,6 +8,8 @@#define BUILD_ID 3 +#define MAX_PHDR_CNT 256 +struct freader {void *buf;u32 buf_sz;
@@ -220,9 +222,9 @@ static int get_build_id_32(struct freader *r, unsigned char *build_id, __u32 *si phnum =READ_ONCE (ehdr->e_phnum); phoff =READ_ONCE (ehdr->e_phoff); -/* only supports phdr that fits in one page */ -if (phnum > (PAGE_SIZE -sizeof (Elf32_Ehdr)) /sizeof (Elf32_Phdr)) -return -EINVAL ; +/* set upper bound on amount of segments (phdrs) we iterate */ +if (phnum >MAX_PHDR_CNT ) + phnum =MAX_PHDR_CNT ;/* check that phoff is not large enough to cause an overflow */ if (phoff + phnum *sizeof (Elf32_Phdr) < phoff)
@@ -257,9 +259,9 @@ static int get_build_id_64(struct freader *r, unsigned char *build_id, __u32 *si phnum =READ_ONCE (ehdr->e_phnum); phoff =READ_ONCE (ehdr->e_phoff); -/* only supports phdr that fits in one page */ -if (phnum > (PAGE_SIZE -sizeof (Elf64_Ehdr)) /sizeof (Elf64_Phdr)) -return -EINVAL ; +/* set upper bound on amount of segments (phdrs) we iterate */ +if (phnum >MAX_PHDR_CNT ) + phnum =MAX_PHDR_CNT ;/* check that phoff is not large enough to cause an overflow */ if (phoff + phnum *sizeof (Elf64_Phdr) < phoff)
Signature · 1 line
2.43.5
Andrii Nakryiko
maintainerPATCH v4 5/10 lib/buildid: rename build_id_parse() into build_id_parse_nofault()
In reply to Andrii NakryikoMake it clear that build_id_parse() assumes that it can take no page fault by renaming it and current few users to build_id_parse_nofault().
Also add build_id_parse() stub which for now falls back to non-sleepable implementation, but will be changed in subsequent patches to take advantage of sleepable context. PROCMAP_QUERY ioctl() on /proc/<pid>/maps file is using build_id_parse() and will automatically take advantage of more reliable sleepable context implementation.
--- include/linux/buildid.h | 4 ++-- kernel/bpf/stackmap.c | 2 +- kernel/events/core.c | 2 +- lib/buildid.c | 25 ++++++++++++++++++++++--- 4 files changed, 26 insertions(+), 7 deletions(-)
include/linux/buildid.h +2 -2
@@ -7,8 +7,8 @@#define BUILD_ID_SIZE_MAX 20struct vm_area_struct ; -int build_id_parse (struct vm_area_struct *vma,unsigned char *build_id, -__u32 *size); +int build_id_parse (struct vm_area_struct *vma,unsigned char *build_id,__u32 *size); +int build_id_parse_nofault (struct vm_area_struct *vma,unsigned char *build_id,__u32 *size);int build_id_parse_buf (const void *buf,unsigned char *build_id,u32 buf_size);#if IS_ENABLED (CONFIG_STACKTRACE_BUILD_ID ) ||IS_ENABLED (CONFIG_VMCORE_INFO )
kernel/bpf/stackmap.c +1 -1
@@ -156,7 +156,7 @@ static void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs, goto build_id_valid; } vma =find_vma (current->mm, ips[i]); -if (!vma ||build_id_parse (vma, id_offs[i].build_id,NULL )) { +if (!vma ||build_id_parse_nofault (vma, id_offs[i].build_id,NULL )) {/* per entry fall back to ips */ id_offs[i].status =BPF_STACK_BUILD_ID_IP ; id_offs[i].ip = ips[i];
kernel/events/core.c +1 -1
@@ -8851,7 +8851,7 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event) mmap_event->event_id.header.size =sizeof (mmap_event->event_id) + size;if (atomic_read (&nr_build_id_events)) -build_id_parse (vma, mmap_event->build_id, &mmap_event->build_id_size); +build_id_parse_nofault (vma, mmap_event->build_id, &mmap_event->build_id_size); perf_iterate_sb(perf_event_mmap_output, mmap_event,
lib/buildid.c +22 -3
@@ -290,10 +290,12 @@ static int get_build_id_64(struct freader *r, unsigned char *build_id, __u32 *si* @build_id: buffer to store build id, at least BUILD_ID_SIZE long * @size: returns actual build id size in case of success * -* Return: 0 on success, -EINVAL otherwise +* Assumes no page fault can be taken, so if relevant portions of ELF file are +* not already paged in, fetching of build ID fails. +* +* Return: 0 on success; negative error, otherwise */ -int build_id_parse (struct vm_area_struct *vma,unsigned char *build_id, -__u32 *size) +int build_id_parse_nofault (struct vm_area_struct *vma,unsigned char *build_id,__u32 *size) {const Elf32_Ehdr *ehdr;struct freader r;
@@ -332,6 +334,23 @@ int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,return ret; } +/* +* Parse build ID of ELF file mapped to VMA +* @vma: vma object +* @build_id: buffer to store build id, at least BUILD_ID_SIZE long +* @size: returns actual build id size in case of success +* +* Assumes faultable context and can cause page faults to bring in file data +* into page cache. +* +* Return: 0 on success; negative error, otherwise +*/ +int build_id_parse (struct vm_area_struct *vma,unsigned char *build_id,__u32 *size) +{ +/* fallback to non-faultable version for now */ +return build_id_parse_nofault (vma, build_id, size); +} +/** * build_id_parse_buf - Get build ID from a buffer * @buf: ELF note section(s) to parse
Signature · 1 line
2.43.5
Andrii Nakryiko
maintainerPATCH v4 6/10 lib/buildid: implement sleepable build_id_parse() API
In reply to Andrii NakryikoExtend freader with a flag specifying whether it's OK to cause page fault to fetch file data that is not already physically present in memory. With this, it's now easy to wait for data if the caller is running in sleepable (faultable) context.
We utilize read_cache_folio() to bring the desired folio into page cache, after which the rest of the logic works just the same at folio level.
--- lib/buildid.c | 44 ++++++++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 16 deletions(-)
lib/buildid.c +28 -16
@@ -20,6 +20,7 @@ struct freader {struct folio *folio;void *addr;loff_t folio_off; +bool may_fault; };struct {const char *data;
@@ -29,12 +30,13 @@ struct freader { };static void freader_init_from_file (struct freader *r,void *buf,u32 buf_sz, -struct address_space *mapping) +struct address_space *mapping,bool may_fault) {memset (r,0 ,sizeof (*r)); r->buf = buf; r->buf_sz = buf_sz; r->mapping = mapping; + r->may_fault = may_fault; }static void freader_init_from_mem (struct freader *r,const char *data,u64 data_sz)
@@ -63,6 +65,11 @@ static int freader_get_folio(struct freader *r, loff_t file_off)freader_put_folio (r); r->folio =filemap_get_folio (r->mapping, file_off >>PAGE_SHIFT ); + +/* if sleeping is allowed, wait for the page, if necessary */ +if (r->may_fault && (IS_ERR (r->folio) || !folio_test_uptodate (r->folio))) + r->folio =read_cache_folio (r->mapping, file_off >>PAGE_SHIFT ,NULL ,NULL ); +if (IS_ERR (r->folio) || !folio_test_uptodate (r->folio)) {if (!IS_ERR (r->folio))folio_put (r->folio);
@@ -284,18 +291,8 @@ static int get_build_id_64(struct freader *r, unsigned char *build_id, __u32 *si/* enough for Elf64_Ehdr, Elf64_Phdr, and all the smaller requests */ #define MAX_FREADER_BUF_SZ 64 -/* -* Parse build ID of ELF file mapped to vma -* @vma: vma object -* @build_id: buffer to store build id, at least BUILD_ID_SIZE long -* @size: returns actual build id size in case of success -* -* Assumes no page fault can be taken, so if relevant portions of ELF file are -* not already paged in, fetching of build ID fails. -* -* Return: 0 on success; negative error, otherwise -*/ -int build_id_parse_nofault (struct vm_area_struct *vma,unsigned char *build_id,__u32 *size) +static int __build_id_parse (struct vm_area_struct *vma,unsigned char *build_id, +__u32 *size,bool may_fault) {const Elf32_Ehdr *ehdr;struct freader r;
@@ -306,7 +303,7 @@ int build_id_parse_nofault(struct vm_area_struct *vma, unsigned char *build_id,if (!vma->vm_file)return -EINVAL ; -freader_init_from_file (&r, buf,sizeof (buf), vma->vm_file->f_mapping); +freader_init_from_file (&r, buf,sizeof (buf), vma->vm_file->f_mapping, may_fault);/* fetch first 18 bytes of ELF header for checks */ ehdr =freader_fetch (&r,0 ,offsetofend (Elf32_Ehdr, e_type));
@@ -334,6 +331,22 @@ int build_id_parse_nofault(struct vm_area_struct *vma, unsigned char *build_id,return ret; } +/* +* Parse build ID of ELF file mapped to vma +* @vma: vma object +* @build_id: buffer to store build id, at least BUILD_ID_SIZE long +* @size: returns actual build id size in case of success +* +* Assumes no page fault can be taken, so if relevant portions of ELF file are +* not already paged in, fetching of build ID fails. +* +* Return: 0 on success; negative error, otherwise +*/ +int build_id_parse_nofault (struct vm_area_struct *vma,unsigned char *build_id,__u32 *size) +{ +return __build_id_parse (vma, build_id, size, false/* !may_fault */ ); +} +/* * Parse build ID of ELF file mapped to VMA * @vma: vma object
@@ -347,8 +360,7 @@ int build_id_parse_nofault(struct vm_area_struct *vma, unsigned char *build_id,*/ int build_id_parse (struct vm_area_struct *vma,unsigned char *build_id,__u32 *size) { -/* fallback to non-faultable version for now */ -return build_id_parse_nofault (vma, build_id, size); +return __build_id_parse (vma, build_id, size, true/* may_fault */ ); }/**
Signature · 1 line
2.43.5
Re: [PATCH v4 bpf-next 06/10] lib/buildid: implement sleepable build_id_parse() API
Quoted 44 lines · Andrii Nakryiko
Extend freader with a flag specifying whether it's OK to cause page fault to fetch file data that is not already physically present in memory. With this, it's now easy to wait for data if the caller is running in sleepable (faultable) context.
We utilize
read_cache_folio()to bring the desired folio into page cache, after which the rest of the logic works just the same at folio level.Suggested-by Omar Sandoval <[email protected]>Cc Shakeel Butt <[email protected]>Cc Johannes Weiner <[email protected]>Signed-off-by Andrii Nakryiko <[email protected]>--- lib/buildid.c | 44 ++++++++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 16 deletions(-)lib/buildid.c +3
-1@@ -20,6 +20,7 @@ struct freader {struct folio *folio;void *addr;loff_t folio_off; +bool may_fault; };struct {const char *data;@@ -29,12 +30,13 @@ struct freader { };static void freader_init_from_file (struct freader *r,void *buf,u32 buf_sz, -struct address_space *mapping) +struct address_space *mapping,bool may_fault) {memset (r,0 ,sizeof (*r)); r->buf = buf; r->buf_sz = buf_sz; r->mapping = mapping; + r->may_fault = may_fault; }static void freader_init_from_mem (struct freader *r,const char *data,u64 data_sz)
lib/buildid.c static int freader_get_folio(struct freader *r, loff_t file_off) · line 71
Quoted 7 lines
lib/buildid.c +4
-0@@ -63,6 +65,11 @@ static int freader_get_folio(struct freader *r, loff_t file_off)freader_put_folio (r); r->folio =filemap_get_folio (r->mapping, file_off >>PAGE_SHIFT ); + +/* if sleeping is allowed, wait for the page, if necessary */ +if (r->may_fault && (IS_ERR (r->folio) || !folio_test_uptodate (r->folio))) + r->folio =read_cache_folio (r->mapping, file_off >>PAGE_SHIFT ,NULL ,NULL );
Willy's network fs comment is bugging me. If we pass NULL for filler, the kernel will going to use fs's read_folio() callback. I have checked read_folio() for fuse and nfs and it seems like for at least these two filesystems the callback is accessing file->private_data. So, if the elf file is on these filesystems, we might see null accesses.
Re: [PATCH v4 bpf-next 06/10] lib/buildid: implement sleepable build_id_parse() API
Quoted 48 lines · Shakeel Butt
Quoted 46 lines · Andrii Nakryiko
Extend freader with a flag specifying whether it's OK to cause page fault to fetch file data that is not already physically present in memory. With this, it's now easy to wait for data if the caller is running in sleepable (faultable) context.
We utilize
read_cache_folio()to bring the desired folio into page cache, after which the rest of the logic works just the same at folio level.Suggested-by Omar Sandoval <[email protected]>Cc Shakeel Butt <[email protected]>Cc Johannes Weiner <[email protected]>Signed-off-by Andrii Nakryiko <[email protected]>--- lib/buildid.c | 44 ++++++++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 16 deletions(-)lib/buildid.c +3
-1@@ -20,6 +20,7 @@ struct freader {struct folio *folio;void *addr;loff_t folio_off; +bool may_fault; };struct {const char *data;@@ -29,12 +30,13 @@ struct freader { };static void freader_init_from_file (struct freader *r,void *buf,u32 buf_sz, -struct address_space *mapping) +struct address_space *mapping,bool may_fault) {memset (r,0 ,sizeof (*r)); r->buf = buf; r->buf_sz = buf_sz; r->mapping = mapping; + r->may_fault = may_fault; }static void freader_init_from_mem (struct freader *r,const char *data,u64 data_sz)@@ -63,6 +65,11 @@ static int freader_get_folio(struct freader *r, loff_t file_off)freader_put_folio (r);
Quoted 12 lines
lib/buildid.c static int freader_get_folio(struct freader *r, loff_t file_off) · line 71
Quoted 5 lines
lib/buildid.c +4
-0r->folio =filemap_get_folio (r->mapping, file_off >>PAGE_SHIFT ); + +/* if sleeping is allowed, wait for the page, if necessary */ +if (r->may_fault && (IS_ERR (r->folio) || !folio_test_uptodate (r->folio))) + r->folio =read_cache_folio (r->mapping, file_off >>PAGE_SHIFT ,NULL ,NULL );Willy's network fs comment is bugging me. If we pass NULL for filler, the kernel will going to use fs's
read_folio()callback. I have checkedread_folio()for fuse and nfs and it seems like for at least these two filesystems the callback is accessingfile->private_data. So, if the elf file is on these filesystems, we might see null accesses.
Isn't that just a huge problem with the read_cache_folio() interface then? That file is optional, in general, but for some specific FS types it's not. How generic code is supposed to know this?
Or maybe it's a bug with the nfs_read_folio() and fuse_read_folio() implementation that they can't handle NULL file argument? netfs_read_folio(), for example, seems to be working with file == NULL just fine.
Matthew, can you please advise what's the right approach here? I can, of course, always get file refcount, but most of the time it will be just an unnecessary overhead, so ideally I'd like to avoid that. But if I have to check each read_folio callback implementation to know whether it's required or not, then that's not great...
Re: [PATCH v4 bpf-next 06/10] lib/buildid: implement sleepable build_id_parse() API
Quoted 62 lines · Andrii Nakryiko
Quoted 48 lines · Shakeel Butt
Quoted 46 lines · Andrii Nakryiko
Extend freader with a flag specifying whether it's OK to cause page fault to fetch file data that is not already physically present in memory. With this, it's now easy to wait for data if the caller is running in sleepable (faultable) context.
We utilize
read_cache_folio()to bring the desired folio into page cache, after which the rest of the logic works just the same at folio level.Suggested-by Omar Sandoval <[email protected]>Cc Shakeel Butt <[email protected]>Cc Johannes Weiner <[email protected]>Signed-off-by Andrii Nakryiko <[email protected]>--- lib/buildid.c | 44 ++++++++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 16 deletions(-)lib/buildid.c +3
-1@@ -20,6 +20,7 @@ struct freader {struct folio *folio;void *addr;loff_t folio_off; +bool may_fault; };struct {const char *data;@@ -29,12 +30,13 @@ struct freader { };static void freader_init_from_file (struct freader *r,void *buf,u32 buf_sz, -struct address_space *mapping) +struct address_space *mapping,bool may_fault) {memset (r,0 ,sizeof (*r)); r->buf = buf; r->buf_sz = buf_sz; r->mapping = mapping; + r->may_fault = may_fault; }static void freader_init_from_mem (struct freader *r,const char *data,u64 data_sz)@@ -63,6 +65,11 @@ static int freader_get_folio(struct freader *r, loff_t file_off)freader_put_folio (r);Quoted 12 lines
lib/buildid.c static int freader_get_folio(struct freader *r, loff_t file_off) · line 71
Quoted 5 lines
lib/buildid.c +4
-0r->folio =filemap_get_folio (r->mapping, file_off >>PAGE_SHIFT ); + +/* if sleeping is allowed, wait for the page, if necessary */ +if (r->may_fault && (IS_ERR (r->folio) || !folio_test_uptodate (r->folio))) + r->folio =read_cache_folio (r->mapping, file_off >>PAGE_SHIFT ,NULL ,NULL );Willy's network fs comment is bugging me. If we pass NULL for filler, the kernel will going to use fs's
read_folio()callback. I have checkedread_folio()for fuse and nfs and it seems like for at least these two filesystems the callback is accessingfile->private_data. So, if the elf file is on these filesystems, we might see null accesses.
Quoted 3 lines
Isn't that just a huge problem with the
read_cache_folio()interface then? That file is optional, in general, but for some specific FS types it's not. How generic code is supposed to know this?
I think you have to think about it the other way around. The file is required, unless you know the filler function that will be used doesn't use the file. Which you don't know when you're coming from generic code, so generic code has to pass in a file.
As far as I can tell, most of the callers of read_cache_folio() (via read_mapping_folio()) are inside filesystem implementations, not generic code, so they know what the filler function will do. You're generic code, so I think you have to pass in a file.
Quoted 10 lines
Or maybe it's a bug with the
nfs_read_folio()andfuse_read_folio()implementation that they can't handle NULL file argument?netfs_read_folio(), for example, seems to be working with file == NULL just fine.Matthew, can you please advise what's the right approach here? I can, of course, always get file refcount, but most of the time it will be just an unnecessary overhead, so ideally I'd like to avoid that. But if I have to check each
read_foliocallback implementation to know whether it's required or not, then that's not great...
Why would you need to increment the file refcount? As far as I can tell, all your accesses to the file would happen under __build_id_parse(), which is borrowing the refcounted reference from vma->vm_file; the file can't go away as long as your caller is holding the mmap lock.
Re: [PATCH v4 bpf-next 06/10] lib/buildid: implement sleepable build_id_parse() API
Quoted 65 lines · Jann Horn
Quoted 62 lines · Andrii Nakryiko
Quoted 60 lines · Shakeel Butt
Quoted 44 lines · Andrii Nakryiko
Extend freader with a flag specifying whether it's OK to cause page fault to fetch file data that is not already physically present in memory. With this, it's now easy to wait for data if the caller is running in sleepable (faultable) context.
We utilize
read_cache_folio()to bring the desired folio into page cache, after which the rest of the logic works just the same at folio level.Suggested-by Omar Sandoval <[email protected]>Cc Shakeel Butt <[email protected]>Cc Johannes Weiner <[email protected]>Signed-off-by Andrii Nakryiko <[email protected]>--- lib/buildid.c | 44 ++++++++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 16 deletions(-)lib/buildid.c +3
-1@@ -20,6 +20,7 @@ struct freader {struct folio *folio;void *addr;loff_t folio_off; +bool may_fault; };struct {const char *data;@@ -29,12 +30,13 @@ struct freader { };static void freader_init_from_file (struct freader *r,void *buf,u32 buf_sz, -struct address_space *mapping) +struct address_space *mapping,bool may_fault) {memset (r,0 ,sizeof (*r)); r->buf = buf; r->buf_sz = buf_sz; r->mapping = mapping; + r->may_fault = may_fault; }static void freader_init_from_mem (struct freader *r,const char *data,u64 data_sz)lib/buildid.c static int freader_get_folio(struct freader *r, loff_t file_off) · line 71
Quoted 7 lines
lib/buildid.c +4
-0@@ -63,6 +65,11 @@ static int freader_get_folio(struct freader *r, loff_t file_off)freader_put_folio (r); r->folio =filemap_get_folio (r->mapping, file_off >>PAGE_SHIFT ); + +/* if sleeping is allowed, wait for the page, if necessary */ +if (r->may_fault && (IS_ERR (r->folio) || !folio_test_uptodate (r->folio))) + r->folio =read_cache_folio (r->mapping, file_off >>PAGE_SHIFT ,NULL ,NULL );Willy's network fs comment is bugging me. If we pass NULL for filler, the kernel will going to use fs's
read_folio()callback. I have checkedread_folio()for fuse and nfs and it seems like for at least these two filesystems the callback is accessingfile->private_data. So, if the elf file is on these filesystems, we might see null accesses.
Quoted 5 lines
Quoted 3 lines
Isn't that just a huge problem with the
read_cache_folio()interface then? That file is optional, in general, but for some specific FS types it's not. How generic code is supposed to know this?I think you have to think about it the other way around. The file is
Fair enough:
Quoted 1 line
@file: Passed to filler function, may be NULL if not required.
But then you look at mapping_read_folio_gfp() which *always* unconditionally passes NULL for filler and file, and that makes you think that file is some special *extra* parameter.
But regardless, as you pointed out, I won't have to take extra ref, so my concerns about performance are wrong. I'll pass the file.
Quoted 9 lines
required, unless you know the filler function that will be used doesn't use the file. Which you don't know when you're coming from generic code, so generic code has to pass in a file.
As far as I can tell, most of the callers of
read_cache_folio()(viaread_mapping_folio()) are inside filesystem implementations, not generic code, so they know what the filler function will do. You're generic code, so I think you have to pass in a file.
Yep, I guess this is a bit of trailblazing use case. I was confused by some other helpers passing NULL for file unconditionally, which made me think that NULL is a supported default use case. Clearly I was wrong.
Quoted 11 lines
Quoted 10 lines
Or maybe it's a bug with the
nfs_read_folio()andfuse_read_folio()implementation that they can't handle NULL file argument?netfs_read_folio(), for example, seems to be working with file == NULL just fine.Matthew, can you please advise what's the right approach here? I can, of course, always get file refcount, but most of the time it will be just an unnecessary overhead, so ideally I'd like to avoid that. But if I have to check each
read_foliocallback implementation to know whether it's required or not, then that's not great...
Quoted 5 lines
Why would you need to increment the file refcount? As far as I can tell, all your accesses to the file would happen under
__build_id_parse(), which is borrowing the refcounted reference fromvma->vm_file; the file can't go away as long as your caller is holding the mmap lock.
Yep, agreed.
Re: [PATCH v4 bpf-next 06/10] lib/buildid: implement sleepable build_id_parse() API
In reply to Andrii NakryikoQuoted 62 lines · Andrii Nakryiko
Quoted 48 lines · Shakeel Butt
Quoted 46 lines · Andrii Nakryiko
Extend freader with a flag specifying whether it's OK to cause page fault to fetch file data that is not already physically present in memory. With this, it's now easy to wait for data if the caller is running in sleepable (faultable) context.
We utilize
read_cache_folio()to bring the desired folio into page cache, after which the rest of the logic works just the same at folio level.Suggested-by Omar Sandoval <[email protected]>Cc Shakeel Butt <[email protected]>Cc Johannes Weiner <[email protected]>Signed-off-by Andrii Nakryiko <[email protected]>--- lib/buildid.c | 44 ++++++++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 16 deletions(-)lib/buildid.c +3
-1@@ -20,6 +20,7 @@ struct freader {struct folio *folio;void *addr;loff_t folio_off; +bool may_fault; };struct {const char *data;@@ -29,12 +30,13 @@ struct freader { };static void freader_init_from_file (struct freader *r,void *buf,u32 buf_sz, -struct address_space *mapping) +struct address_space *mapping,bool may_fault) {memset (r,0 ,sizeof (*r)); r->buf = buf; r->buf_sz = buf_sz; r->mapping = mapping; + r->may_fault = may_fault; }static void freader_init_from_mem (struct freader *r,const char *data,u64 data_sz)@@ -63,6 +65,11 @@ static int freader_get_folio(struct freader *r, loff_t file_off)freader_put_folio (r);Quoted 12 lines
lib/buildid.c static int freader_get_folio(struct freader *r, loff_t file_off) · line 71
Quoted 5 lines
lib/buildid.c +4
-0r->folio =filemap_get_folio (r->mapping, file_off >>PAGE_SHIFT ); + +/* if sleeping is allowed, wait for the page, if necessary */ +if (r->may_fault && (IS_ERR (r->folio) || !folio_test_uptodate (r->folio))) + r->folio =read_cache_folio (r->mapping, file_off >>PAGE_SHIFT ,NULL ,NULL );Willy's network fs comment is bugging me. If we pass NULL for filler, the kernel will going to use fs's
read_folio()callback. I have checkedread_folio()for fuse and nfs and it seems like for at least these two filesystems the callback is accessingfile->private_data. So, if the elf file is on these filesystems, we might see null accesses.
Quoted 8 lines
Isn't that just a huge problem with the
read_cache_folio()interface then? That file is optional, in general, but for some specific FS types it's not. How generic code is supposed to know this?Or maybe it's a bug with the
nfs_read_folio()andfuse_read_folio()implementation that they can't handle NULL file argument?netfs_read_folio(), for example, seems to be working with file == NULL just fine.
If you go a bit down in netfs_alloc_request() there is the following code:
if (rreq->netfs_ops->init_request) {
ret = rreq->netfs_ops->init_request(rreq, file);
...
...I think this init_request is pointing to nfs_netfs_init_request which calls nfs_file_open_context(file) and access filp->private_data.
Quoted 6 lines
Matthew, can you please advise what's the right approach here? I can, of course, always get file refcount, but most of the time it will be just an unnecessary overhead, so ideally I'd like to avoid that. But if I have to check each
read_foliocallback implementation to know whether it's required or not, then that's not great...
I don't think we will need file refcnt. We have mmap lock in read mode in this context because we are accessing vma and this vma has reference to the file. So, this file can not go away under us here.
Re: [PATCH v4 bpf-next 06/10] lib/buildid: implement sleepable build_id_parse() API
Quoted 76 lines · Shakeel Butt
Quoted 62 lines · Andrii Nakryiko
Quoted 48 lines · Shakeel Butt
Quoted 46 lines · Andrii Nakryiko
Extend freader with a flag specifying whether it's OK to cause page fault to fetch file data that is not already physically present in memory. With this, it's now easy to wait for data if the caller is running in sleepable (faultable) context.
We utilize
read_cache_folio()to bring the desired folio into page cache, after which the rest of the logic works just the same at folio level.Suggested-by Omar Sandoval <[email protected]>Cc Shakeel Butt <[email protected]>Cc Johannes Weiner <[email protected]>Signed-off-by Andrii Nakryiko <[email protected]>--- lib/buildid.c | 44 ++++++++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 16 deletions(-)lib/buildid.c +3
-1@@ -20,6 +20,7 @@ struct freader {struct folio *folio;void *addr;loff_t folio_off; +bool may_fault; };struct {const char *data;@@ -29,12 +30,13 @@ struct freader { };static void freader_init_from_file (struct freader *r,void *buf,u32 buf_sz, -struct address_space *mapping) +struct address_space *mapping,bool may_fault) {memset (r,0 ,sizeof (*r)); r->buf = buf; r->buf_sz = buf_sz; r->mapping = mapping; + r->may_fault = may_fault; }static void freader_init_from_mem (struct freader *r,const char *data,u64 data_sz)@@ -63,6 +65,11 @@ static int freader_get_folio(struct freader *r, loff_t file_off)freader_put_folio (r);Quoted 12 lines
lib/buildid.c static int freader_get_folio(struct freader *r, loff_t file_off) · line 71
Quoted 5 lines
lib/buildid.c +4
-0r->folio =filemap_get_folio (r->mapping, file_off >>PAGE_SHIFT ); + +/* if sleeping is allowed, wait for the page, if necessary */ +if (r->may_fault && (IS_ERR (r->folio) || !folio_test_uptodate (r->folio))) + r->folio =read_cache_folio (r->mapping, file_off >>PAGE_SHIFT ,NULL ,NULL );Willy's network fs comment is bugging me. If we pass NULL for filler, the kernel will going to use fs's
read_folio()callback. I have checkedread_folio()for fuse and nfs and it seems like for at least these two filesystems the callback is accessingfile->private_data. So, if the elf file is on these filesystems, we might see null accesses.Quoted 8 lines
Isn't that just a huge problem with the
read_cache_folio()interface then? That file is optional, in general, but for some specific FS types it's not. How generic code is supposed to know this?Or maybe it's a bug with the
nfs_read_folio()andfuse_read_folio()implementation that they can't handle NULL file argument?netfs_read_folio(), for example, seems to be working with file == NULL just fine.If you go a bit down in
netfs_alloc_request()there is the following code:
Quoted 7 lines
if (rreq->netfs_ops->init_request) { ret = rreq->netfs_ops->init_request(rreq, file); ... ...I think this
init_requestis pointing tonfs_netfs_init_requestwhich callsnfs_file_open_context(file)and accessfilp->private_data.
That's "nfs", which we know requires a file. For netfs implementations (cifs_init_request() and v9fs_init_request()), they both treat file as optional consistently.
But regardless, that's just pointless code archeology, I'll just pass the file reference unconditionally.
Quoted 11 lines
Quoted 7 lines
Matthew, can you please advise what's the right approach here? I can, of course, always get file refcount, but most of the time it will be just an unnecessary overhead, so ideally I'd like to avoid that. But if I have to check each
read_foliocallback implementation to know whether it's required or not, then that's not great...I don't think we will need file refcnt. We have mmap lock in read mode in this context because we are accessing vma and this vma has reference to the file. So, this file can not go away under us here.
Yep, good point, then it's not a problem, thanks! Will update.
Andrii Nakryiko
maintainerPATCH v4 7/10 lib/buildid: don't limit .note.gnu.build-id to the first page in ELF
In reply to Andrii NakryikoWith freader we don't need to restrict ourselves to a single page, so let's allow ELF notes to be at any valid position with the file.
We also merge parse_build_id() and parse_build_id_buf() as now the only difference between them is note offset overflow, which makes sense to check in all situations.
--- lib/buildid.c | 27 ++++++--------------------- 1 file changed, 6 insertions(+), 21 deletions(-)
lib/buildid.c +6 -21
@@ -151,17 +151,19 @@ static void freader_cleanup(struct freader *r)* 32-bit and 64-bit system, because Elf32_Nhdr and Elf64_Nhdr are * identical. */ -static int parse_build_id_buf (struct freader *r, -unsigned char *build_id,__u32 *size, -loff_t note_offs,Elf32_Word note_size) +static int parse_build_id (struct freader *r,unsigned char *build_id,__u32 *size, +loff_t note_offs,Elf32_Word note_size) {const char note_name[] ="GNU" ;const size_t note_name_sz =sizeof (note_name);const Elf32_Nhdr *nhdr; -loff_t build_id_off, new_offs, note_end = note_offs + note_size; +loff_t build_id_off, new_offs, note_end;u32 name_sz, desc_sz;const char *data; +if (check_add_overflow (note_offs, note_size, ¬e_end)) +return -EINVAL ; +while (note_end - note_offs >sizeof (Elf32_Nhdr) + note_name_sz) { nhdr =freader_fetch (r, note_offs,sizeof (Elf32_Nhdr) + note_name_sz);if (!nhdr)
@@ -197,23 +199,6 @@ static int parse_build_id_buf(struct freader *r,return -EINVAL ; } -static inline int parse_build_id (struct freader *r, -unsigned char *build_id, -__u32 *size, -loff_t note_start_off, -Elf32_Word note_size) -{ -/* check for overflow */ -if (note_start_off + note_size < note_start_off) -return -EINVAL ; - -/* only supports note that fits in the first page */ -if (note_start_off + note_size >PAGE_SIZE ) -return -EINVAL ; - -return parse_build_id_buf (r, build_id, size, note_start_off, note_size); -} -/* Parse build ID from 32-bit ELF */ static int get_build_id_32 (struct freader *r,unsigned char *build_id,__u32 *size) {
Signature · 1 line
2.43.5
Andrii Nakryiko
maintainerPATCH v4 8/10 bpf: decouple stack_map_get_build_id_offset() from perf_callchain_entry
In reply to Andrii NakryikoChange stack_map_get_build_id_offset() which is used to convert stack trace IP addresses into build ID+offset pairs. Right now this function accepts an array of u64s as an input, and uses array of struct bpf_stack_build_id as an output.
This is problematic because u64 array is coming from perf_callchain_entry, which is (non-sleepable) RCU protected, so once we allows sleepable build ID fetching, this all breaks down.
But its actually pretty easy to make stack_map_get_build_id_offset() works with array of struct bpf_stack_build_id as both input and output. Which is what this patch is doing, eliminating the dependency on perf_callchain_entry. We require caller to fill out bpf_stack_build_id.ip fields (all other can be left uninitialized), and update in place as we do build ID resolution.
We make sure to READ_ONCE() and cache locally current IP value as we used it in a few places to find matching VMA and so on. Given this data is directly accessible and modifiable by user's BPF code, we should make sure to have a consistent view of it.
--- kernel/bpf/stackmap.c | 49 +++++++++++++++++++++++++++++-------------- 1 file changed, 33 insertions(+), 16 deletions(-)
kernel/bpf/stackmap.c +33 -16
@@ -124,8 +124,18 @@ static struct bpf_map *stack_map_alloc(union bpf_attr *attr)return ERR_PTR (err); } +/* +* Expects all id_offs[i].ip values to be set to correct initial IPs. +* They will be subsequently: +* - either adjusted in place to a file offset, if build ID fetching +* succeeds; in this case id_offs[i].build_id is set to correct build ID, +* and id_offs[i].status is set to BPF_STACK_BUILD_ID_VALID; +* - or IP will be kept intact, if build ID fetching failed; in this case +* id_offs[i].build_id is zeroed out and id_offs[i].status is set to +* BPF_STACK_BUILD_ID_IP. +*/ static void stack_map_get_build_id_offset (struct bpf_stack_build_id *id_offs, -u64 *ips,u32 trace_nr,bool user) +u32 trace_nr,bool user) {int i;struct mmap_unlock_irq_work *work =NULL ;
@@ -142,30 +152,28 @@ static void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,/* cannot access current->mm, fall back to ips */ for (i =0 ; i < trace_nr; i++) { id_offs[i].status =BPF_STACK_BUILD_ID_IP ; - id_offs[i].ip = ips[i];memset (id_offs[i].build_id,0 ,BUILD_ID_SIZE_MAX ); }return ; }for (i =0 ; i < trace_nr; i++) { -if (range_in_vma (prev_vma, ips[i], ips[i])) { +u64 ip =READ_ONCE (id_offs[i].ip); + +if (range_in_vma (prev_vma, ip, ip)) { vma = prev_vma; -memcpy (id_offs[i].build_id, prev_build_id, -BUILD_ID_SIZE_MAX ); +memcpy (id_offs[i].build_id, prev_build_id,BUILD_ID_SIZE_MAX ); goto build_id_valid; } - vma =find_vma (current->mm, ips[i]); + vma =find_vma (current->mm, ip);if (!vma ||build_id_parse_nofault (vma, id_offs[i].build_id,NULL )) {/* per entry fall back to ips */ id_offs[i].status =BPF_STACK_BUILD_ID_IP ; - id_offs[i].ip = ips[i];memset (id_offs[i].build_id,0 ,BUILD_ID_SIZE_MAX );continue ; } build_id_valid: - id_offs[i].offset = (vma->vm_pgoff <<PAGE_SHIFT ) + ips[i] - - vma->vm_start; + id_offs[i].offset = (vma->vm_pgoff <<PAGE_SHIFT ) + ip - vma->vm_start; id_offs[i].status =BPF_STACK_BUILD_ID_VALID ; prev_vma = vma; prev_build_id = id_offs[i].build_id;
@@ -216,7 +224,7 @@ static long __bpf_get_stackid(struct bpf_map *map, struct bpf_stack_map *smap = container_of(map, struct bpf_stack_map, map);struct stack_map_bucket *bucket, *new_bucket, *old_bucket;u32 skip = flags &BPF_F_SKIP_FIELD_MASK ; -u32 hash, id, trace_nr, trace_len; +u32 hash, id, trace_nr, trace_len, i;bool user = flags &BPF_F_USER_STACK ;u64 *ips;bool hash_matches;
@@ -238,15 +246,18 @@ static long __bpf_get_stackid(struct bpf_map *map,return id;if (stack_map_use_build_id (map)) { +struct bpf_stack_build_id *id_offs; +/* for build_id+offset, pop a bucket before slow cmp */ new_bucket = (struct stack_map_bucket *)pcpu_freelist_pop (&smap->freelist);if (unlikely (!new_bucket))return -ENOMEM ; new_bucket->nr = trace_nr; -stack_map_get_build_id_offset ( - (struct bpf_stack_build_id *)new_bucket->data, - ips, trace_nr, user); + id_offs = (struct bpf_stack_build_id *)new_bucket->data; +for (i =0 ; i < trace_nr; i++) + id_offs[i].ip = ips[i]; +stack_map_get_build_id_offset (id_offs, trace_nr, user); trace_len = trace_nr *sizeof (struct bpf_stack_build_id );if (hash_matches && bucket->nr == trace_nr &&memcmp (bucket->data, new_bucket->data, trace_len) ==0 ) {
@@ -445,10 +456,16 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, copy_len = trace_nr * elem_size; ips = trace->ip + skip; -if (user && user_build_id) -stack_map_get_build_id_offset (buf, ips, trace_nr, user); -else +if (user && user_build_id) { +struct bpf_stack_build_id *id_offs = buf; +u32 i; + +for (i =0 ; i < trace_nr; i++) + id_offs[i].ip = ips[i]; +stack_map_get_build_id_offset (buf, trace_nr, user); + }else {memcpy (buf, ips, copy_len); + }if (size > copy_len)memset (buf + copy_len,0 , size - copy_len);
Signature · 1 line
2.43.5
Andrii Nakryiko
maintainerPATCH v4 9/10 bpf: wire up sleepable bpf_get_stack() and bpf_get_task_stack() helpers
In reply to Andrii NakryikoAdd sleepable implementations of bpf_get_stack() and bpf_get_task_stack() helpers and allow them to be used from sleepable BPF program (e.g., sleepable uprobes).
Note, the stack trace IPs capturing itself is not sleepable (that would need to be a separate project), only build ID fetching is sleepable and thus more reliable, as it will wait for data to be paged in, if necessary. For that we make use of sleepable build_id_parse() implementation.
Now that build ID related internals in kernel/bpf/stackmap.c can be used both in sleepable and non-sleepable contexts, we need to add additional rcu_read_lock()/rcu_read_unlock() protection around fetching perf_callchain_entry, but with the refactoring in previous commit it's now pretty straightforward. We make sure to do rcu_read_unlock (in sleepable mode only) right before stack_map_get_build_id_offset() call which can sleep. By that time we don't have any more use of perf_callchain_entry.
Note, bpf_get_task_stack() will fail for user mode if task != current. And for kernel mode build ID are irrelevant. So in that sense adding sleepable bpf_get_task_stack() implementation is a no-op. It feel right to wire this up for symmetry and completeness, but I'm open to just dropping it until we support user && crosstask condition.
--- include/linux/bpf.h | 2 + kernel/bpf/stackmap.c | 90 ++++++++++++++++++++++++++++++++-------- kernel/trace/bpf_trace.c | 5 ++- 3 files changed, 77 insertions(+), 20 deletions(-)
include/linux/bpf.h +2 -0
@@ -3198,7 +3198,9 @@ extern const struct bpf_func_proto bpf_get_current_uid_gid_proto;extern const struct bpf_func_proto bpf_get_current_comm_proto;extern const struct bpf_func_proto bpf_get_stackid_proto;extern const struct bpf_func_proto bpf_get_stack_proto; +extern const struct bpf_func_proto bpf_get_stack_sleepable_proto;extern const struct bpf_func_proto bpf_get_task_stack_proto; +extern const struct bpf_func_proto bpf_get_task_stack_sleepable_proto;extern const struct bpf_func_proto bpf_get_stackid_proto_pe;extern const struct bpf_func_proto bpf_get_stack_proto_pe;extern const struct bpf_func_proto bpf_sock_map_update_proto;
kernel/bpf/stackmap.c +72 -18
202 lines not on this page · raw message
kernel/trace/bpf_trace.c +3 -2
@@ -1530,7 +1530,8 @@ bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)case BPF_FUNC_jiffies64:return &bpf_jiffies64_proto;case BPF_FUNC_get_task_stack: -return &bpf_get_task_stack_proto; +return prog->sleepable ? &bpf_get_task_stack_sleepable_proto + : &bpf_get_task_stack_proto;case BPF_FUNC_copy_from_user:return &bpf_copy_from_user_proto;case BPF_FUNC_copy_from_user_task:
@@ -1586,7 +1587,7 @@ kprobe_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)case BPF_FUNC_get_stackid:return &bpf_get_stackid_proto;case BPF_FUNC_get_stack: -return &bpf_get_stack_proto; +return prog->sleepable ? &bpf_get_stack_sleepable_proto : &bpf_get_stack_proto;#ifdef CONFIG_BPF_KPROBE_OVERRIDE case BPF_FUNC_override_return:return &bpf_override_return_proto;
Signature · 1 line
2.43.5
Andrii Nakryiko
maintainerPATCH v4 10/10 selftests/bpf: add build ID tests
In reply to Andrii NakryikoAdd a new set of tests validating behavior of capturing stack traces with build ID. We extend uprobe_multi target binary with ability to trigger uprobe (so that we can capture stack traces from it), but also we allow to force build ID data to be either resident or non-resident in memory (see also a comment about quirks of MADV_PAGEOUT).
That way we can validate that in non-sleepable context we won't get build ID (as expected), but with sleepable uprobes we will get that build ID regardless of it being physically present in memory.
Also, we add a small add-on linker script which reorders .note.gnu.build-id section and puts it after (big) .text section, putting build ID data outside of the very first page of ELF file. This will test all the relaxations we did in build ID parsing logic in kernel thanks to freader abstraction.
--- tools/testing/selftests/bpf/Makefile | 5 +- .../selftests/bpf/prog_tests/build_id.c | 118 ++++++++++++++++++ .../selftests/bpf/progs/test_build_id.c | 31 +++++ tools/testing/selftests/bpf/uprobe_multi.c | 41 ++++++ tools/testing/selftests/bpf/uprobe_multi.ld | 11 ++ 5 files changed, 204 insertions(+), 2 deletions(-) create mode 100644 tools/testing/selftests/bpf/prog_tests/build_id.c create mode 100644 tools/testing/selftests/bpf/progs/test_build_id.c create mode 100644 tools/testing/selftests/bpf/uprobe_multi.ld
tools/testing/selftests/bpf/Makefile +3 -2
@@ -787,9 +787,10 @@ $(OUTPUT)/veristat: $(OUTPUT)/veristat.o # Linking uprobe_multi can fail due to relocation overflows on mips. $(OUTPUT)/uprobe_multi: CFLAGS += $(if $(filter mips, $(ARCH)),-mxgot) -$(OUTPUT)/uprobe_multi: uprobe_multi.c +$(OUTPUT)/uprobe_multi: uprobe_multi.c uprobe_multi.ld $(call msg,BINARY,,$@) - $(Q)$(CC) $(CFLAGS) -O0 $(LDFLAGS) $^ $(LDLIBS) -o $@ + $(Q)$(CC) $(CFLAGS) -Wl,-T,uprobe_multi.ld -O0 $(LDFLAGS) \ + $(filter-out %.ld,$^) $(LDLIBS) -o $@ EXTRA_CLEAN := $(SCRATCH_DIR) $(HOST_SCRATCH_DIR) \ prog_tests/tests.h map_tests/tests.h verifier/tests.h \
tools/testing/selftests/bpf/prog_tests/build_id.c new file +118 -0
118 lines not on this page · raw message
tools/testing/selftests/bpf/progs/test_build_id.c new file +31 -0
@@ -0,0 +1,31 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */ + +#include "vmlinux.h" +#include <bpf/bpf_helpers.h> + +struct bpf_stack_build_id stack_sleepable[128 ]; +int res_sleepable; + +struct bpf_stack_build_id stack_nofault[128 ]; +int res_nofault; + +SEC("uprobe.multi/./uprobe_multi:uprobe") +int uprobe_nofault (struct pt_regs *ctx) +{ + res_nofault =bpf_get_stack (ctx, stack_nofault,sizeof (stack_nofault), +BPF_F_USER_STACK |BPF_F_USER_BUILD_ID ); + +return 0 ; +} + +SEC("uprobe.multi.s/./uprobe_multi:uprobe") +int uprobe_sleepable (struct pt_regs *ctx) +{ + res_sleepable =bpf_get_stack (ctx, stack_sleepable,sizeof (stack_sleepable), +BPF_F_USER_STACK |BPF_F_USER_BUILD_ID ); + +return 0 ; +} + +char _license[] SEC("license") = "GPL";
tools/testing/selftests/bpf/uprobe_multi.c +41 -0
61 lines not on this page · raw message
tools/testing/selftests/bpf/uprobe_multi.ld new file +11 -0
@@ -0,0 +1,11 @@ +SECTIONS +{ + . = ALIGN(4096); + .note.gnu.build-id : { *(.note.gnu.build-id) } + . = ALIGN(4096); +} +INSERT AFTER .text; + +build_id_start = ADDR(.note.gnu.build-id); +build_id_end = ADDR(.note.gnu.build-id) + SIZEOF(.note.gnu.build-id); +
Signature · 1 line
2.43.5