Hi Steve, et al.,
as discussed during yesterdays SFrame call I will be sending two RFC
fixup patches shortly as POC to demonstrate how this patch and
"[PATCH v11 14/15] unwind_user/sframe: Add .sframe validation option"
could benefit from introducing an internal FDE representation (e.g.
struct sframe_fde_internal) similar to the used internal FRE
representation (struct sframe_fre).
On 10/22/2025 4:43 PM, Jens Remus wrote:
> diff --git a/kernel/unwind/sframe.c b/kernel/unwind/sframe.c
> +static __always_inline int __read_fde(struct sframe_section *sec,
> + unsigned int fde_num,
> + struct sframe_fde *fde,
> + unsigned long *fde_start_base)
The goal would be to eliminate the passing through of fde_start_base as
well as the various computations of the effective function start address
(= *fde_start_base + fde->start_addr) throughout this module. The
internal FDE representation could then simply convey the effective
function start address via an "unsigned long func_start_addr" field.
> +{
> + unsigned long fde_addr, ip;
> +
> + fde_addr = sec->fdes_start + (fde_num * sizeof(struct sframe_fde));
> + unsafe_copy_from_user(fde, (void __user *)fde_addr,
> + sizeof(struct sframe_fde), Efault);
> +
> + ip = fde_addr + fde->start_addr;
> + if (ip < sec->text_start || ip > sec->text_end)
> + return -EINVAL;
> +
> + *fde_start_base = fde_addr;
> + return 0;
> +
> +Efault:
> + return -EFAULT;
> +}
> +
> +static __always_inline int __find_fde(struct sframe_section *sec,
> + unsigned long ip,
> + struct sframe_fde *fde,
> + unsigned long *fde_start_base)
fde_start_base would get eliminated.
> +{
> + unsigned long func_addr_low = 0, func_addr_high = ULONG_MAX;
> + struct sframe_fde __user *first, *low, *high, *found = NULL;
> + int ret;
> +
> + first = (void __user *)sec->fdes_start;
> + low = first;
> + high = first + sec->num_fdes - 1;
> +
> + while (low <= high) {
> + struct sframe_fde __user *mid;
> + s32 func_off;
> + unsigned long func_addr;
> +
> + mid = low + ((high - low) / 2);
> +
> + unsafe_get_user(func_off, (s32 __user *)mid, Efault);
> + func_addr = (unsigned long)mid + func_off;
> +
> + if (ip >= func_addr) {
> + if (func_addr < func_addr_low)
> + return -EFAULT;
> +
> + func_addr_low = func_addr;
> +
> + found = mid;
> + low = mid + 1;
> + } else {
> + if (func_addr > func_addr_high)
> + return -EFAULT;
> +
> + func_addr_high = func_addr;
> +
> + high = mid - 1;
> + }
> + }
> +
> + if (!found)
> + return -EINVAL;
> +
> + ret = __read_fde(sec, found - first, fde, fde_start_base);
fde_start_base would get eliminated.
> + if (ret)
> + return ret;
> +
> + /* make sure it's not in a gap */
> + if (ip < *fde_start_base + fde->start_addr ||
> + ip >= *fde_start_base + fde->start_addr + fde->func_size)
Would simplify to:
if (ip < fde->func_start_addr ||
ip >= fde->func_start_addr + fde->func_size)
> + return -EINVAL;
> +
> + return 0;
> +
> +Efault:
> + return -EFAULT;
> +}
> +static __always_inline int __find_fre(struct sframe_section *sec,
> + struct sframe_fde *fde,
> + unsigned long fde_start_base,
fde_start_base would get eliminated.
> + unsigned long ip,
> + struct unwind_user_frame *frame)
> +{
> + unsigned char fde_type = SFRAME_FUNC_FDE_TYPE(fde->info);
> + struct sframe_fre *fre, *prev_fre = NULL;
> + struct sframe_fre fres[2];
> + unsigned long fre_addr;
> + bool which = false;
> + unsigned int i;
> + u32 ip_off;
> +
> + ip_off = ip - (fde_start_base + fde->start_addr);
Would simplify to:
ip_off = ip - fde->func_start_addr;
> +
> + if (fde_type == SFRAME_FDE_TYPE_PCMASK)
> + ip_off %= fde->rep_size;
> +
> + fre_addr = sec->fres_start + fde->fres_off;
> +
> + for (i = 0; i < fde->fres_num; i++) {
> + int ret;
> +
> + /*
> + * Alternate between the two fre_addr[] entries for 'fre' and
> + * 'prev_fre'.
> + */
> + fre = which ? fres : fres + 1;
> + which = !which;
> +
> + ret = __read_fre(sec, fde, fre_addr, fre);
> + if (ret)
> + return ret;
> +
> + fre_addr += fre->size;
> +
> + if (prev_fre && fre->ip_off <= prev_fre->ip_off)
> + return -EFAULT;
> +
> + if (fre->ip_off > ip_off)
> + break;
> +
> + prev_fre = fre;
> + }
> +
> + if (!prev_fre)
> + return -EINVAL;
> + fre = prev_fre;
> +
> + frame->cfa_off = fre->cfa_off;
> + frame->ra_off = fre->ra_off;
> + frame->fp_off = fre->fp_off;
> + frame->use_fp = SFRAME_FRE_CFA_BASE_REG_ID(fre->info) ==
> SFRAME_BASE_REG_FP;
> +
> + return 0;
> +}
> +
> +int sframe_find(unsigned long ip, struct unwind_user_frame *frame)
> +{
> + struct mm_struct *mm = current->mm;
> + struct sframe_section *sec;
> + struct sframe_fde fde;
> + unsigned long fde_start_base;
fde_start_base would get eliminated.
> + int ret;
> +
> + if (!mm)
> + return -EINVAL;
> +
> + guard(srcu)(&sframe_srcu);
> +
> + sec = mtree_load(&mm->sframe_mt, ip);
> + if (!sec)
> + return -EINVAL;
> +
> + if (!user_read_access_begin((void __user *)sec->sframe_start,
> + sec->sframe_end - sec->sframe_start))
> + return -EFAULT;
> +
> + ret = __find_fde(sec, ip, &fde, &fde_start_base);
fde_start_base would get eliminated.
> + if (ret)
> + goto end;
> +
> + ret = __find_fre(sec, &fde, fde_start_base, ip, frame);
fde_start_base would get eliminated.
> +end:
> + user_read_access_end();
> + return ret;
> +}
Regards,
Jens
--
Jens Remus
Linux on Z Development (D3303)
+49-7031-16-1128 Office
[email protected]
IBM
IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats:
Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft:
Böblingen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/
