Re: [PATCH v3 09/19] unwind: Introduce sframe user space unwinding

On Mon, Oct 28, 2024 at 02:47:36PM -0700, Josh Poimboeuf wrote:

> +static int __sframe_add_section(unsigned long sframe_addr,
> +				unsigned long text_start,
> +				unsigned long text_end)
> +{
> +	struct maple_tree *sframe_mt = &current->mm->sframe_mt;
> +	struct sframe_section *sec;
> +	struct sframe_header shdr;
> +	unsigned long header_end;
> +	int ret;
> +
> +	if (copy_from_user(&shdr, (void __user *)sframe_addr, sizeof(shdr)))
> +		return -EFAULT;
> +
> +	if (shdr.preamble.magic != SFRAME_MAGIC ||
> +	    shdr.preamble.version != SFRAME_VERSION_2 ||
> +	    !(shdr.preamble.flags & SFRAME_F_FDE_SORTED) ||
> +	    shdr.auxhdr_len || !shdr.num_fdes || !shdr.num_fres ||
> +	    shdr.fdes_off > shdr.fres_off) {
> +		return -EINVAL;
> +	}
> +
> +	sec = kmalloc(sizeof(*sec), GFP_KERNEL);
> +	if (!sec)
> +		return -ENOMEM;
> +
> +	header_end = sframe_addr + SFRAME_HDR_SIZE(shdr);
> +
> +	sec->sframe_addr	= sframe_addr;
> +	sec->text_addr		= text_start;
> +	sec->fdes_addr		= header_end + shdr.fdes_off;
> +	sec->fres_addr		= header_end + shdr.fres_off;
> +	sec->fdes_nr		= shdr.num_fdes;
> +	sec->ra_off		= shdr.cfa_fixed_ra_offset;
> +	sec->fp_off		= shdr.cfa_fixed_fp_offset;
> +
> +	ret = mtree_insert_range(sframe_mt, text_start, text_end, sec, GFP_KERNEL);
> +	if (ret) {
> +		kfree(sec);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +int sframe_add_section(unsigned long sframe_addr, unsigned long text_start,
> +		       unsigned long text_end)
> +{
> +	struct mm_struct *mm = current->mm;
> +	struct vm_area_struct *sframe_vma;
> +
> +	mmap_read_lock(mm);

DEFINE_GUARD(mmap_read_lock, struct mm_struct *,
	     mmap_read_lock(_T), mmap_read_unlock(_T))

in include/linux/mmap_lock.h ?

> +
> +	sframe_vma = vma_lookup(mm, sframe_addr);
> +	if (!sframe_vma)
> +		goto err_unlock;
> +
> +	if (text_start && text_end) {
> +		struct vm_area_struct *text_vma;
> +
> +		text_vma = vma_lookup(mm, text_start);
> +		if (!(text_vma->vm_flags & VM_EXEC))
> +			goto err_unlock;
> +
> +		if (PAGE_ALIGN(text_end) != text_vma->vm_end)
> +			goto err_unlock;
> +	} else {
> +		struct vm_area_struct *vma, *text_vma = NULL;
> +		VMA_ITERATOR(vmi, mm, 0);
> +
> +		for_each_vma(vmi, vma) {
> +			if (vma->vm_file != sframe_vma->vm_file ||
> +			    !(vma->vm_flags & VM_EXEC))
> +				continue;
> +
> +			if (text_vma) {
> +				pr_warn_once("%s[%d]: multiple EXEC segments unsupported\n",
> +					     current->comm, current->pid);
> +				goto err_unlock;
> +			}
> +
> +			text_vma = vma;
> +		}
> +
> +		if (!text_vma)
> +			goto err_unlock;
> +
> +		text_start = text_vma->vm_start;
> +		text_end   = text_vma->vm_end;
> +	}
> +
> +	mmap_read_unlock(mm);
> +
> +	return __sframe_add_section(sframe_addr, text_start, text_end);
> +
> +err_unlock:
> +	mmap_read_unlock(mm);
> +	return -EINVAL;
> +}

> +int sframe_remove_section(unsigned long sframe_addr)
> +{
> +	struct mm_struct *mm = current->mm;
> +	struct sframe_section *sec;
> +	unsigned long index = 0;
> +
> +	mt_for_each(&mm->sframe_mt, sec, index, ULONG_MAX) {
> +		if (sec->sframe_addr == sframe_addr)
> +			return __sframe_remove_section(mm, sec);
> +	}
> +
> +	return -EINVAL;
> +}

> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -64,6 +64,7 @@
>  #include <linux/rcupdate.h>
>  #include <linux/uidgid.h>
>  #include <linux/cred.h>
> +#include <linux/sframe.h>
>  
>  #include <linux/nospec.h>
>  
> @@ -2784,6 +2785,16 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
>  	case PR_RISCV_SET_ICACHE_FLUSH_CTX:
>  		error = RISCV_SET_ICACHE_FLUSH_CTX(arg2, arg3);
>  		break;
> +	case PR_ADD_SFRAME:
> +		if (arg5)
> +			return -EINVAL;
> +		error = sframe_add_section(arg2, arg3, arg4);
> +		break;
> +	case PR_REMOVE_SFRAME:
> +		if (arg3 || arg4 || arg5)
> +			return -EINVAL;
> +		error = sframe_remove_section(arg2);
> +		break;
>  	default:
>  		error = -EINVAL;
>  		break;

So I realize that mtree has an internal lock, but are we sure we don't
want a lock around those prctl()s?