From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id AADA3C77B73 for ; Tue, 2 May 2023 09:04:55 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233747AbjEBJEy (ORCPT ); Tue, 2 May 2023 05:04:54 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:32942 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233744AbjEBJEx (ORCPT ); Tue, 2 May 2023 05:04:53 -0400 Received: from desiato.infradead.org (desiato.infradead.org [IPv6:2001:8b0:10b:1:d65d:64ff:fe57:4e05]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 083762D63 for ; Tue, 2 May 2023 02:04:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=desiato.20200630; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=VkrPZwkB4nGxpzpQBLplH+7hHIRxMPmXdNrrrZnIpa8=; b=OCRH2MpLoNp/FbuuTfvExVgX6M K5CW03AGUe+xFK0Y4Y5URHeLK/g/mOkeauXk4U9nbhIOBAxE6oGOqkbiJFgsDw2HyB8dIB18azaNZ h7OKaVmp7CmevWluZlwixJVeQL+MS8sqS6MPEgszZBqxMU0U/Wd9MOzW2Hcs9WKEXm/0VhA9rJM6N kqbzHru/rsxpvrmXWADuEvBlLZ+/siigYrdRrJVMew6hEZTXXiUsXdCkX2u1lB9bncJFhmZwxeuuf vB8way7aB5AYwwdHKeOzNW8I/XkOCFTT5roNuK6cVRCdTazaSolv/gIRPPzn649WGoTIw/5yPQLVj Owuo7hUQ==; Received: from j130084.upc-j.chello.nl ([24.132.130.84] helo=noisy.programming.kicks-ass.net) by desiato.infradead.org with esmtpsa (Exim 4.96 #2 (Red Hat Linux)) id 1ptlwC-00GGer-2X; Tue, 02 May 2023 09:04:44 +0000 Received: from hirez.programming.kicks-ass.net (hirez.programming.kicks-ass.net [192.168.1.225]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits)) (Client did not present a certificate) by noisy.programming.kicks-ass.net (Postfix) with ESMTPS id 230E9300165; Tue, 2 May 2023 11:04:43 +0200 (CEST) Received: by hirez.programming.kicks-ass.net (Postfix, from userid 1000) id CA11E23C5C34C; Tue, 2 May 2023 11:04:43 +0200 (CEST) Date: Tue, 2 May 2023 11:04:43 +0200 From: Peter Zijlstra To: Indu Bhagat Cc: linux-toolchains@vger.kernel.org, daandemeyer@meta.com, andrii@kernel.org, rostedt@goodmis.org, kris.van.hees@oracle.com, elena.zannoni@oracle.com, nick.alcock@oracle.com Subject: Re: [POC 4/5] sframe: add an SFrame format stack tracer Message-ID: <20230502090443.GH1597476@hirez.programming.kicks-ass.net> References: <20230501200410.3973453-1-indu.bhagat@oracle.com> <20230501200410.3973453-5-indu.bhagat@oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230501200410.3973453-5-indu.bhagat@oracle.com> Precedence: bulk List-ID: X-Mailing-List: linux-toolchains@vger.kernel.org On Mon, May 01, 2023 at 01:04:09PM -0700, Indu Bhagat wrote: > +int iterate_phdr(int (*callback)(struct phdr_info *info, > + struct task_struct *task, > + void *data), > + struct task_struct *task, void *data) > +{ > + struct mm_struct *mm; > + struct vm_area_struct *vma_mt; > + struct page *page; > + > + Elf64_Ehdr *ehdr; > + struct phdr_info phinfo; > + > + int ret = 0, res = 0; > + int err = 0; > + bool first = true; > + > + memset(&phinfo, 0, sizeof(struct phdr_info)); > + > + mm = task->mm; > + > + MA_STATE(mas, &mm->mm_mt, 0, 0); > + > + mas_for_each(&mas, vma_mt, ULONG_MAX) { > + /* ELF header has a fixed place in the file, starting at offset > + * zero. > + */ This is a vile comment form; please abstain from using it. https://lkml.org/lkml/2016/7/8/625 > + if (vma_mt->vm_pgoff) > + continue; > + > + /* For the callback to infer if its the prog or DSO we are > + * dealing with. > + */ > + phinfo.pi_prog = first; > + first = false; > + /* FIXME TODO > + * - This code assumes 64-bit ELF by using Elf64_Ehdr. > + * - Detect the case when ELF program headers to be of > + * size > 1 page. > + */ > + > + /* FIXME TODO KERNEL > + * - get_user_pages_WHAT, which API. > + * What flags ? Is this correct ? > + */ > + ret = get_user_pages_remote(mm, vma_mt->vm_start, 1, FOLL_GET, > + &page, &vma_mt, NULL); > + if (ret <= 0) > + continue; > + > + /* The first page must have the ELF header. */ > + ehdr = vmap(&page, 1, VM_MAP, PAGE_KERNEL); > + if (!ehdr) > + goto put_page; > + > + /* Check for magic bytes to make sure this is ehdr. */ > + err = 0; > + err |= ((ehdr->e_ident[EI_MAG0] != ELFMAG0) > + || (ehdr->e_ident[EI_MAG1] != ELFMAG1) > + || (ehdr->e_ident[EI_MAG2] != ELFMAG2) > + || (ehdr->e_ident[EI_MAG3] != ELFMAG3)); And again, operators go at the end. > + if (err) > + goto unmap; > + > + /* > + * FIXME TODO handle the case when number of program headers is > + * greater than or equal to PN_XNUM later. > + */ Also note the glorious inconsistency in comment styles within a single function; why? > + if (ehdr->e_phnum == PN_XNUM) > + goto unmap; > + /* > + * FIXME TODO handle the case when Elf phdrs span more than one > + * page later ? > + */ > + if ((sizeof(Elf64_Ehdr) + ehdr->e_phentsize * ehdr->e_phnum) > + > PAGE_SIZE) Srsly, you can read this?, just bust the line limit or use a temporary somewhere. > + goto unmap; > + > + /* Save the location of program headers and the phnum. */ > + phinfo.pi_addr = vma_mt->vm_start; > + phinfo.pi_phdr = (void *)ehdr + ehdr->e_phoff; > + phinfo.pi_phnum = ehdr->e_phnum; > + > + res = callback(&phinfo, task, data); > +unmap: > + vunmap(ehdr); > +put_page: > + put_page(page); > + > + if (res < 0) > + break; > + } > + > + return res; > +}