Re: [PATCH v4 19/39] unwind_user/sframe: Add support for reading .sframe contents

[Jens Remus <jremus@linux.ibm.com>]

On 22.01.2025 03:31, Josh Poimboeuf 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 <jpoimboe@kernel.org>

> diff --git a/kernel/unwind/sframe.c b/kernel/unwind/sframe.c

> +struct sframe_fre {
> +	unsigned int	size;
> +	s32		ip_off;

The IP offset (from function start) in the SFrame V2 FDE is unsigned:

u32 ip_off;

> +	s32		cfa_off;
> +	s32		ra_off;
> +	s32		fp_off;
> +	u8		info;
> +};

...

> +#define __UNSAFE_GET_USER_INC(to, from, type, label)			\
> +({									\
> +	type __to;							\
> +	unsafe_get_user(__to, (type __user *)from, label);		\
> +	from += sizeof(__to);						\
> +	to = (typeof(to))__to;							\
> +})
> +
> +#define UNSAFE_GET_USER_INC(to, from, size, label)			\
> +({									\
> +	switch (size) {							\
> +	case 1:								\
> +		__UNSAFE_GET_USER_INC(to, from, u8, label);		\
> +		break;							\
> +	case 2:								\
> +		__UNSAFE_GET_USER_INC(to, from, u16, label);		\
> +		break;							\
> +	case 4:								\
> +		__UNSAFE_GET_USER_INC(to, from, u32, label);		\
> +		break;							\
> +	default:							\
> +		return -EFAULT;						\
> +	}								\
> +})

This does not work for the signed SFrame fields, such as the FRE CFA,
RA, and FP offsets, as it does not perform the required sign extension.
One option would be to rename to UNSAFE_GET_USER_UNSIGNED_INC() and
re-introduce UNSAFE_GET_USER_SIGNED_INC() using s8, s16, and s32.

> +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;

The IP offset (from function start) in the SFrame V2 FRE is unsigned:

u32 ip_off;
s32 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)
> +		return -EFAULT;
> +
> +	UNSAFE_GET_USER_INC(ip_off, cur, addr_size, Efault);

Ok: The SFrame V2 FRE IP offset is unsigned u8, u16, or u32.

> +	if (fde_type == SFRAME_FDE_TYPE_PCINC && ip_off > fde->func_size)
> +		return -EFAULT;
> +
> +	UNSAFE_GET_USER_INC(info, cur, 1, Efault);

Ok: The SFrame V2 FRE info word is one byte of data.

> +	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)
> +		return -EFAULT;
> +
> +	fre->size = addr_size + 1 + (offset_count * offset_size);
> +
> +	UNSAFE_GET_USER_INC(cfa_off, cur, offset_size, Efault);

Issue: The SFrame V2 FRE CFA offset is signed s8, s16, or s32.  Sign
extension required when storing in s32.

> +	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);

Issue: The SFrame V2 FRE RA offset is signed s8, s16, or s32.  Sign
extension required when storing in s32.

> +	}
> +
> +	fp_off = sec->fp_off;
> +	if (!fp_off && offset_count) {
> +		offset_count--;
> +		UNSAFE_GET_USER_INC(fp_off, cur, offset_size, Efault);

Issue: The SFrame V2 FRE FP offset is signed s8, s16, or s32.  Sign
extension required when storing in s32.

> +	}
> +
> +	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];
> +	unsigned long fre_addr;
> +	bool which = false;
> +	unsigned int i;
> +	s32 ip_off;
> +
> +	ip_off = (s32)(ip - sec->sframe_start) - fde->start_addr;

The IP offset (from function start) in the SFrame V2 FRE is unsigned.
The function start address offset (from .sframe section begin) is signed.
Therefore:

u32 ip_off;

ip_off = ip - (sec->sframe_start + fde->start_addr);

> +
> +	if (fde_type == SFRAME_FDE_TYPE_PCMASK)
> +		ip_off %= fde->rep_size;

Following is a patch with the suggested changes, that were required to
make unwinding of user space using SFrame work on s390:

diff --git a/kernel/unwind/sframe.c b/kernel/unwind/sframe.c
index bba14c5fe0f5..ea2d491ea68f 100644
--- a/kernel/unwind/sframe.c
+++ b/kernel/unwind/sframe.c
@@ -19,7 +19,7 @@
  
  struct sframe_fre {
  	unsigned int	size;
-	s32		ip_off;
+	u32		ip_off;
  	s32		cfa_off;
  	s32		ra_off;
  	s32		fp_off;
@@ -136,7 +136,26 @@ static __always_inline int __find_fde(struct sframe_section *sec,
  	to = (typeof(to))__to;							\
  })
  
-#define UNSAFE_GET_USER_INC(to, from, size, label)			\
+#define UNSAFE_GET_USER_SIGNED_INC(to, from, size, label)		\
+({									\
+	switch (size) {							\
+	case 1:								\
+		__UNSAFE_GET_USER_INC(to, from, s8, label);		\
+		break;							\
+	case 2:								\
+		__UNSAFE_GET_USER_INC(to, from, s16, label);		\
+		break;							\
+	case 4:								\
+		__UNSAFE_GET_USER_INC(to, from, s32, label);		\
+		break;							\
+	default:							\
+		dbg_sec_uaccess("%d: bad UNSAFE_GET_USER_SIGNED__INC size %u\n",\
+				__LINE__, size);			\
+		return -EFAULT;						\
+	}								\
+})
+
+#define UNSAFE_GET_USER_UNSIGNED_INC(to, from, size, label)		\
  ({									\
  	switch (size) {							\
  	case 1:								\
@@ -149,7 +168,7 @@ static __always_inline int __find_fde(struct sframe_section *sec,
  		__UNSAFE_GET_USER_INC(to, from, u32, label);		\
  		break;							\
  	default:							\
-		dbg_sec_uaccess("%d: bad UNSAFE_GET_USER_INC size %u\n",\
+		dbg_sec_uaccess("%d: bad UNSAFE_GET_USER_UNSIGNED_INC size %u\n",\
  				__LINE__, size);			\
  		return -EFAULT;						\
  	}								\
@@ -163,7 +182,8 @@ static __always_inline int __read_fre(struct sframe_section *sec,
  	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;
+	u32 ip_off;
+	s32 cfa_off, ra_off, fp_off;
  	unsigned long cur = fre_addr;
  	unsigned char addr_size;
  	u8 info;
@@ -179,14 +199,14 @@ static __always_inline int __read_fre(struct sframe_section *sec,
  		return -EFAULT;
  	}
  
-	UNSAFE_GET_USER_INC(ip_off, cur, addr_size, Efault);
+	UNSAFE_GET_USER_UNSIGNED_INC(ip_off, cur, addr_size, Efault);
  	if (fde_type == SFRAME_FDE_TYPE_PCINC && ip_off > fde->func_size) {
  		dbg_sec_uaccess("fre starts past end of function: ip_off=0x%x, func_size=0x%x\n",
  				ip_off, fde->func_size);
  		return -EFAULT;
  	}
  
-	UNSAFE_GET_USER_INC(info, cur, 1, Efault);
+	UNSAFE_GET_USER_UNSIGNED_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) {
@@ -200,7 +220,7 @@ static __always_inline int __read_fre(struct sframe_section *sec,
  
  	fre->size = addr_size + 1 + (offset_count * offset_size);
  
-	UNSAFE_GET_USER_INC(cfa_off, cur, offset_size, Efault);
+	UNSAFE_GET_USER_SIGNED_INC(cfa_off, cur, offset_size, Efault);
  	offset_count--;
  
  	ra_off = sec->ra_off;
@@ -210,13 +230,13 @@ static __always_inline int __read_fre(struct sframe_section *sec,
  			return -EFAULT;
  		}
  
-		UNSAFE_GET_USER_INC(ra_off, cur, offset_size, Efault);
+		UNSAFE_GET_USER_SIGNED_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);
+		UNSAFE_GET_USER_SIGNED_INC(fp_off, cur, offset_size, Efault);
  	}
  
  	if (offset_count) {
@@ -247,9 +267,9 @@ static __always_inline int __find_fre(struct sframe_section *sec,
  	unsigned long fre_addr;
  	bool which = false;
  	unsigned int i;
-	s32 ip_off;
+	u32 ip_off;
  
-	ip_off = (s32)(ip - sec->sframe_start) - fde->start_addr;
+	ip_off = ip - (sec->sframe_start + fde->start_addr);
  
  	if (fde_type == SFRAME_FDE_TYPE_PCMASK)
  		ip_off %= fde->rep_size;

Regards,
Jens
-- 
Jens Remus
Linux on Z Development (D3303)
+49-7031-16-1128 Office
jremus@de.ibm.com

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/