On Tue, Jan 21, 2025 at 6:32 PM Josh Poimboeuf <[email protected]> wrote:
>
> In preparation for using sframe to unwind user space stacks, add an
> sframe_find() interface for finding the sframe information associated
> with a given text address.
>
> For performance, use user_read_access_begin() and the corresponding
> unsafe_*() accessors. Note that use of pr_debug() in uaccess-enabled
> regions would break noinstr validation, so there aren't any debug
> messages yet. That will be added in a subsequent commit.
>
> Signed-off-by: Josh Poimboeuf <[email protected]>
> ---
> include/linux/sframe.h | 5 +
> kernel/unwind/sframe.c | 295 ++++++++++++++++++++++++++++++++++-
> kernel/unwind/sframe_debug.h | 35 +++++
> 3 files changed, 331 insertions(+), 4 deletions(-)
> create mode 100644 kernel/unwind/sframe_debug.h
>
[...]
> +
> +static __always_inline int __read_fde(struct sframe_section *sec,
> + unsigned int fde_num,
> + struct sframe_fde *fde)
> +{
> + 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 = sec->sframe_start + fde->start_addr;
> + if (ip < sec->text_start || ip > sec->text_end)
ip >= sec->test_end ? ip == sec->test_end doesn't make sense, no?
> + return -EINVAL;
> +
> + return 0;
> +
> +Efault:
> + return -EFAULT;
> +}
> +
[...]
> +static __always_inline int __read_fre(struct sframe_section *sec,
> + struct sframe_fde *fde,
> + unsigned long fre_addr,
> + struct sframe_fre *fre)
> +{
> + unsigned char fde_type = SFRAME_FUNC_FDE_TYPE(fde->info);
> + unsigned char fre_type = SFRAME_FUNC_FRE_TYPE(fde->info);
> + unsigned char offset_count, offset_size;
> + s32 ip_off, cfa_off, ra_off, fp_off;
> + unsigned long cur = fre_addr;
> + unsigned char addr_size;
> + u8 info;
> +
> + addr_size = fre_type_to_size(fre_type);
> + if (!addr_size)
> + return -EFAULT;
> +
> + if (fre_addr + addr_size + 1 > sec->fres_end)
nit: isn't this the same as `fre_addr + addr_size >= sec->fres_end` ?
> + return -EFAULT;
> +
> + UNSAFE_GET_USER_INC(ip_off, cur, addr_size, Efault);
> + if (fde_type == SFRAME_FDE_TYPE_PCINC && ip_off > fde->func_size)
is ip_off == fde->func_size allowable?
> + return -EFAULT;
> +
> + UNSAFE_GET_USER_INC(info, cur, 1, Efault);
> + offset_count = SFRAME_FRE_OFFSET_COUNT(info);
> + offset_size = offset_size_enum_to_size(SFRAME_FRE_OFFSET_SIZE(info));
> + if (!offset_count || !offset_size)
> + return -EFAULT;
> +
> + if (cur + (offset_count * offset_size) > sec->fres_end)
offset_count * offset_size done in u8 can overflow, no? maybe upcast
to unsigned long or use check_add_overflow?
> + return -EFAULT;
> +
> + fre->size = addr_size + 1 + (offset_count * offset_size);
> +
> + UNSAFE_GET_USER_INC(cfa_off, cur, offset_size, Efault);
> + offset_count--;
> +
> + ra_off = sec->ra_off;
> + if (!ra_off) {
> + if (!offset_count--)
> + return -EFAULT;
> +
> + UNSAFE_GET_USER_INC(ra_off, cur, offset_size, Efault);
> + }
> +
> + fp_off = sec->fp_off;
> + if (!fp_off && offset_count) {
> + offset_count--;
> + UNSAFE_GET_USER_INC(fp_off, cur, offset_size, Efault);
> + }
> +
> + if (offset_count)
> + return -EFAULT;
> +
> + fre->ip_off = ip_off;
> + fre->cfa_off = cfa_off;
> + fre->ra_off = ra_off;
> + fre->fp_off = fp_off;
> + fre->info = info;
> +
> + return 0;
> +
> +Efault:
> + return -EFAULT;
> +}
> +
> +static __always_inline int __find_fre(struct sframe_section *sec,
> + struct sframe_fde *fde, 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];
you only need prev_fre->ip_off, so why all this `which` and `fres[2]`
business if all you need is prev_fre_off and a bool whether you have
prev_fre at all?
> + unsigned long fre_addr;
> + bool which = false;
> + unsigned int i;
> + s32 ip_off;
> +
> + ip_off = (s32)(ip - sec->sframe_start) - fde->start_addr;
> +
> + if (fde_type == SFRAME_FDE_TYPE_PCMASK)
> + ip_off %= fde->rep_size;
did you check that fde->rep_size is not zero?
> +
> + fre_addr = sec->fres_start + fde->fres_off;
> +
> + for (i = 0; i < fde->fres_num; i++) {
why not binary search? seem more logical to guard against cases with
lots of FREs and be pretty fast in common case anyways.
> + 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;
> +}
> +
[...]
