From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0b-00082601.pphosted.com ([67.231.153.30]:48098 "EHLO mx0a-00082601.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751211AbeCIXgA (ORCPT ); Fri, 9 Mar 2018 18:36:00 -0500 From: Song Liu To: Daniel Borkmann CC: "netdev@vger.kernel.org" , "ast@kernel.org" , Peter Zijlstra , Kernel Team , "hannes@cmpxchg.org" , Teng Qin Subject: Re: [PATCH v3 1/2] bpf: extend stackmap to save binary_build_id+offset instead of address Date: Fri, 9 Mar 2018 23:34:50 +0000 Message-ID: <4D9BD156-E286-4FD3-9E9C-89DC2160CD9E@fb.com> References: <20180306194223.2364559-1-songliubraving@fb.com> In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-ID: <371CDC39027D574B982DA8C90851B1A6@namprd15.prod.outlook.com> Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Sender: netdev-owner@vger.kernel.org List-ID: > On Mar 9, 2018, at 3:25 PM, Daniel Borkmann wrote: >=20 > Hi Song, >=20 > On 03/06/2018 08:42 PM, Song Liu wrote: > [...]> +/* >> + * Parse build id from the note segment. This logic can be shared betwe= en >> + * 32-bit and 64-bit system, because Elf32_Nhdr and Elf64_Nhdr are >> + * identical. >> + */ >> +static inline int stack_map_parse_build_id(void *vm_start, >> + unsigned char *build_id, >> + void *note_start, >> + Elf32_Word note_size) >> +{ >> + Elf32_Word note_offs =3D 0, new_offs; >> + >> + /* check for overflow */ >> + if (note_start < vm_start || note_start + note_size < note_start) >> + return -EINVAL; >> + >> + /* only supports note that fits in the first page */ >> + if (note_start + note_size > vm_start + PAGE_SIZE) >> + return -EINVAL; >> + >> + while (note_offs + sizeof(Elf32_Nhdr) < note_size) { >> + Elf32_Nhdr *nhdr =3D (Elf32_Nhdr *)(note_start + note_offs); >> + >> + if (nhdr->n_type =3D=3D BPF_BUILD_ID && >> + nhdr->n_namesz =3D=3D sizeof("GNU") && >> + nhdr->n_descsz =3D=3D BPF_BUILD_ID_SIZE) { >> + memcpy(build_id, >> + note_start + note_offs + >> + ALIGN(sizeof("GNU"), 4) + sizeof(Elf32_Nhdr), >> + BPF_BUILD_ID_SIZE); >> + return 0; >> + } >> + new_offs =3D note_offs + sizeof(Elf32_Nhdr) + >> + ALIGN(nhdr->n_namesz, 4) + ALIGN(nhdr->n_descsz, 4); >> + if (new_offs <=3D note_offs) /* overflow */ >> + break; >> + note_offs =3D new_offs; >> + }; >> + return -EINVAL; >> +} >> + >> +/* Parse build ID from 32-bit ELF */ >> +static int stack_map_get_build_id_32(void *vm_start, >> + unsigned char *build_id) >> +{ >> + Elf32_Ehdr *ehdr =3D (Elf32_Ehdr *)vm_start; >> + Elf32_Phdr *phdr; >> + int i; >> + >> + /* only supports phdr that fits in one page */ >> + if (ehdr->e_phnum > >> + (PAGE_SIZE - sizeof(Elf32_Ehdr)) / sizeof(Elf32_Phdr)) >> + return -EINVAL; >> + >> + phdr =3D (Elf32_Phdr *)(vm_start + sizeof(Elf32_Ehdr)); >> + >> + for (i =3D 0; i < ehdr->e_phnum; ++i) >> + if (phdr[i].p_type =3D=3D PT_NOTE) >> + return stack_map_parse_build_id(vm_start, build_id, >> + vm_start + phdr[i].p_offset, >> + phdr[i].p_filesz); >> + return -EINVAL; >> +} >> + >> +/* Parse build ID from 64-bit ELF */ >> +static int stack_map_get_build_id_64(void *vm_start, >> + unsigned char *build_id) >> +{ >> + Elf64_Ehdr *ehdr =3D (Elf64_Ehdr *)vm_start; >> + Elf64_Phdr *phdr; >> + int i; >> + >> + /* only supports phdr that fits in one page */ >> + if (ehdr->e_phnum > >> + (PAGE_SIZE - sizeof(Elf64_Ehdr)) / sizeof(Elf64_Phdr)) >> + return -EINVAL; >> + >> + phdr =3D (Elf64_Phdr *)(vm_start + sizeof(Elf64_Ehdr)); >> + >> + for (i =3D 0; i < ehdr->e_phnum; ++i) >> + if (phdr[i].p_type =3D=3D PT_NOTE) >> + return stack_map_parse_build_id(vm_start, build_id, >> + vm_start + phdr[i].p_offset, >> + phdr[i].p_filesz); >> + return -EINVAL; >> +} >> + >> +/* Parse build ID of ELF file mapped to vma */ >> +static int stack_map_get_build_id(struct vm_area_struct *vma, >> + unsigned char *build_id) >> +{ >> + Elf32_Ehdr *ehdr; >> + struct page *page; >> + int ret; >> + >> + /* >> + * vm_start is user memory, so we need to be careful with it. >> + * We don't want too many copy_from_user to reduce overhead. >> + * Most ELF file is at least one page long, and the build_id >> + * is stored in the first page. Therefore, we limit the search of >> + * build_id to the first page only. This can be made safe with >> + * get_user_pages_fast(). If the file is smaller than PAGE_SIZE, >> + * or the build_id is not in the first page, the look up fails. >> + */ >> + if (vma->vm_end - vma->vm_start < PAGE_SIZE || >> + vma->vm_start & (PAGE_SIZE - 1)) /* page aligned */ >> + return -EINVAL; >> + >> + if (get_user_pages_fast(vma->vm_start, 1, 0, &page) !=3D 1) >=20 > Shouldn't this throw a splat? Implementations of get_user_pages_fast() > call down_read() which has might_sleep() and we're under RCU read side > here. >=20 >> + return -EFAULT; >> + >> + ret =3D -EINVAL; >> + ehdr =3D (Elf32_Ehdr *)page_address(page); >> + >> + /* compare magic x7f "ELF" */ >> + if (memcmp(ehdr->e_ident, ELFMAG, SELFMAG) !=3D 0) >> + goto out; >> + >> + /* only support executable file and shared object file */ >> + if (ehdr->e_type !=3D ET_EXEC && ehdr->e_type !=3D ET_DYN) >> + goto out; >> + >> + if (ehdr->e_ident[EI_CLASS] =3D=3D 1) >=20 > Minor nit: ehdr->e_ident[EI_CLASS] =3D=3D ELFCLASS32 >=20 >> + ret =3D stack_map_get_build_id_32(page_address(page), build_id); >> + else if (ehdr->e_ident[EI_CLASS] =3D=3D 2) >=20 > Minor nit: ehdr->e_ident[EI_CLASS] =3D=3D ELFCLASS64 >=20 >> + ret =3D stack_map_get_build_id_64(page_address(page), build_id); >> +out: >> + put_page(page); >> + return ret; >> +} >> + >> +static int stack_map_get_build_id_offset(struct bpf_map *map, >> + struct stack_map_bucket *bucket, >> + u64 *ips, u32 trace_nr) >> +{ >> + int i; >> + struct vm_area_struct *vma; >> + struct bpf_stack_build_id *id_offs; >> + int err; >> + int successful_lookup =3D 0; >> + > [...] >=20 > Thanks, > Daniel Thanks Daniel! I will fix these.=20 I also chatted with Teng about this, that we may miss some cases. I will also include that fix in the next version.=20 Song