Re: [POC 4/5] sframe: add an SFrame format stack tracer

[Peter Zijlstra <peterz@infradead.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;
> +}